Skip to content

fix(install): rewrite in-package relative links to apm_modules/ paths (#1147)#1160

Merged
danielmeppiel merged 3 commits into
mainfrom
fix/issue-1147-link-resolver-generalization
May 6, 2026
Merged

fix(install): rewrite in-package relative links to apm_modules/ paths (#1147)#1160
danielmeppiel merged 3 commits into
mainfrom
fix/issue-1147-link-resolver-generalization

Conversation

@danielmeppiel

Copy link
Copy Markdown
Collaborator

TL;DR

Fixes #1147. Sibling references from instructions/prompts/agents/commands to in-package assets (e.g. [guide](../../standards/style.md)) used to be deployed verbatim. After the .agents/.github split (#1103), those paths point nowhere because each primitive type now lands at a host-tool-specific destination that no longer matches the package's authoring layout. UnifiedLinkResolver now rewrites any in-package relative markdown link to point at the package's install location under apm_modules/ at install time.

Problem (WHY)

APM's deploy layout is host-tool-determined, not package-determined. A single APM package may ship instructions, prompts, agents, and skills, and each type lands at a different destination per target:

Intra-package relative references in primitive bodies cross this routing boundary and break by design. PR #1103 made the breakage user-visible because skills moved out of the host's root, but the underlying constraint has always existed for cross-target installs.

UnifiedLinkResolver already rewrites links at install time, but only for .context.md/.memory.md files (hardcoded extension allow-list). It did not cover SKILL.md bodies, agents, prompts, or arbitrary asset paths.

Approach (WHAT)

Generalize UnifiedLinkResolver._rewrite_markdown_links so that, at install time only, any markdown link whose resolved target lands inside the source package gets rewritten to its apm_modules/ location in the consumer.

Scope:

  • Install only — the compile path is deliberately untouched. Compile receives synthetic source dirs without per-link package provenance, so the same logic doesn't apply.
  • Authoritative package_root — passed from package_info.install_path, so ADO 4-segment deps, virtual subdirs, monorepos, and local _local/<name>/ packages all work without shape inference.
  • Containment — uses the existing ensure_path_within helper from path_security, which handles symlinks and Windows extended-prefix paths.
  • Conservative skip-list — fragment-only links (#section), scheme links (http:, mailto:), root-absolute paths (/foo), and any link whose resolved target escapes the package root are left untouched.

Implementation (HOW)

  • src/apm_cli/compilation/link_resolver.py

    • LinkResolutionContext gains package_root: Path | None and enable_asset_rewrite: bool (defaults to compile-safe).
    • UnifiedLinkResolver.__init__ accepts an optional package_root field.
    • resolve_links_for_installation populates package_root and sets enable_asset_rewrite=True.
    • resolve_links_for_compilation explicitly passes package_root=None, enable_asset_rewrite=False.
    • _rewrite_markdown_links keeps the existing context-file branch and adds an in-package asset branch behind the new flags.
    • New helpers: _is_rewritable_relative_link, _split_link_target (preserves #fragment / ?query), _resolve_in_package_asset_link. The new helper is intentionally isolated from _resolve_context_link to prevent regression.
  • src/apm_cli/integration/base_integrator.py

    • init_link_resolver sets self.link_resolver.package_root = Path(scan_root) only when scan_root equals package_info.install_path. The narrowed-scan_root cases (e.g. $HOME/.apm/ for user scope) intentionally skip this so asset links cannot escape the .apm/ boundary.

Validation evidence

  • Lint: uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both silent.
  • Unit tests: full uv run --extra dev pytest tests/unit -q -> 7684 passed.
  • Integration: uv run --extra dev pytest tests/integration/test_local_install.py -q -> 18 passed (includes the new TestLocalInstallSiblingLinkRewriting::test_sibling_link_is_rewritten_to_apm_modules which reproduces [BUG] Package relative paths broken due to .agents and .github splitting #1147 end-to-end via apm install).
  • Manual end-to-end repro (/tmp/apm-1147-repro/) confirmed: deployed instruction now contains ../../apm_modules/_local/producer/standards/style.md and that path resolves on disk.

New test coverage

  • 10 new unit tests in tests/unit/compilation/test_link_resolver.py:
    • Happy-path rewrite to apm_modules/.
    • Missing-on-disk targets preserved (no false-positive rewrite).
    • Escape-package-root preserved (security: cannot point out of source package).
    • Fragment-only links and ?query suffixes preserved.
    • URL-scheme and root-absolute links left untouched.
    • Subdirectory packages (virtual subdir / ADO 4-segment) contained correctly.
    • Opt-out when package_root is unset.
    • Regression guard: compile path is NOT broadened.
  • 1 integration test reproducing [BUG] Package relative paths broken due to .agents and .github splitting #1147 via real apm install.

How to test

mkdir -p /tmp/apm-1147 && cd /tmp/apm-1147
cat > producer/apm.yml <<'YML'
name: producer
version: 1.0.0
YML
mkdir -p producer/.apm/instructions producer/standards
echo "# Style guide" > producer/standards/style.md
cat > producer/.apm/instructions/foo.instructions.md <<'MD'
---
applyTo: "**/*.py"
---
# Foo

Follow the [style guide](../../standards/style.md).
MD

mkdir -p consumer/.github
cat > consumer/apm.yml <<'YML'
name: consumer
version: 1.0.0
dependencies:
  apm: []
YML

cd consumer
apm install ../producer
grep -F 'apm_modules/_local/producer/standards/style.md' .github/instructions/foo.instructions.md

Trade-offs

  • Install-time warning for unresolved in-package links is deferred to a follow-up PR. The current DiagnosticCollector would surface one warning per primitive, which would spam typical installs. A separate aggregation design is needed.
  • Markdown title syntax ([text](path "title")) passes through unchanged — supported but not stripped/re-emitted.
  • Compile path is intentionally not broadened. Compile lacks per-link provenance about which source package a link came from, and the compile output is consumed differently than installed primitives.

Closes #1147.

Sibling references from instructions/prompts/agents/commands to
in-package assets (e.g. `../../standards/style.md`) used to deploy
verbatim. After the .agents/.github split (#1103) those paths point
nowhere because primitives land at host-tool-specific destinations
that no longer match the package's authoring layout.

UnifiedLinkResolver now rewrites any in-package relative markdown
link at install time, scoped to the source package via the
authoritative `package_info.install_path` (handles ADO 4-segment
deps, virtual subdirs, monorepos, local _local/<name>/ packages).
Containment is enforced via `ensure_path_within`. Compile path is
deliberately untouched.

Tests: 10 new unit tests (rewrite happy path, fragments preserved,
escapes preserved, scheme/root-absolute skipped, opt-out when no
package_root, compile path NOT broadened) + 1 integration test that
reproduces #1147 end-to-end via apm install.

Docs: new guides/package-relative-links.md describing the contract.

Fixes #1147

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 6, 2026 11:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes broken relative markdown links between APM package primitives (instructions/prompts/agents/commands) and in-package assets by rewriting eligible in-package relative links at install time to point at the package's installed location under apm_modules/, preserving link functionality across host-tool-specific deploy directories.

Changes:

  • Extend UnifiedLinkResolver to optionally rewrite in-package relative markdown links during installation (guarded by package_root + an enable flag).
  • Wire BaseIntegrator.init_link_resolver() to set the authoritative package_root for install-time rewriting.
  • Add unit + integration coverage, new user documentation, and a changelog entry for #1147.
Show a summary per file
File Description
src/apm_cli/compilation/link_resolver.py Adds install-time in-package asset link rewriting logic gated by package_root/flags.
src/apm_cli/integration/base_integrator.py Sets link_resolver.package_root when scanning the full installed package root.
tests/unit/compilation/test_link_resolver.py Adds unit tests for the new install-time rewrite behavior and compile-path regression coverage.
tests/integration/test_local_install.py Adds an end-to-end repro asserting sibling link rewrite works after apm install.
docs/src/content/docs/guides/package-relative-links.md Documents the package-relative link rewrite contract and caveats.
CHANGELOG.md Adds a Fixed entry describing the #1147 behavior change.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 3

Comment on lines +431 to +447
verbatim. Markdown link titles (``"title"`` after a space) are
intentionally NOT stripped here -- the existing ``LINK_PATTERN``
treats the whole inside of the parentheses as a single group, so
a title would be embedded in ``link_path``. Such links are passed
through unchanged by ``_is_rewritable_relative_link`` indirectly
(they typically contain a space and resolve to nothing).

Returns:
``(path_part, suffix)`` where ``suffix`` includes its leading
delimiter (``#`` or ``?``) or is the empty string.
"""
for sep in ("#", "?"):
if sep in link_path:
idx = link_path.index(sep)
return link_path[:idx], link_path[idx:]
return link_path, ""

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch -- this was a real bug. Fixed in ce91ce3: _split_link_target now picks the earliest occurrence of # or ? so a combined doc.md?x=1#sec preserves the full ?x=1#sec suffix on the rewritten target. Added test_preserve_query_and_fragment_on_rewritten_link covering the combined case (passes locally with the rest of the resolver suite).


assert "../../apm_modules/_local/producer/standards/style.md" in result
# And it actually resolves on disk relative to target_file's parent.
rewritten = re.search(r"\(([^)]+)\)", result).group(1)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Adopted in ce91ce3 -- explicit assert match is not None, ... before .group(1) so a regex miss surfaces a clear failure message instead of an opaque AttributeError.

Comment thread CHANGELOG.md Outdated
### Fixed

- Fix `apm install` against a branch ref so it re-downloads when upstream has advanced past the lockfile-recorded SHA, and self-heal lockfiles produced by APM <= 0.12.2 on next install. (#1158)
- Fix broken sibling links from instructions/prompts/agents to in-package assets after the `.agents/.github` split. The link resolver now rewrites any in-package relative markdown link to its `apm_modules/` location at install time, so cross-tool deploy paths no longer break intra-package references. (#1147)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Shortened in ce91ce3 to one line per repo convention: Rewrite in-package relative markdown links to their apm_modules/ location at install time so sibling references survive the .agents/.github deploy split. (#1147).

Daniel Meppiel and others added 2 commits May 6, 2026 13:38
Wires a dedicated end-to-end suite proving the install-time link
rewriter behaves correctly across the .agents/.github split that
broke intra-package relative paths in PR #1103.

tests/integration/test_link_rewrite_e2e.py drives the real apm
install CLI against fixture packages and asserts the deployed
markdown link targets, covering:

  - instruction -> sibling asset (happy path)
  - prompt -> sibling asset (happy path)
  - mixed link shapes in one body: rewritable, external URL,
    bare #fragment, and relative+#fragment (suffix preserved)
  - path-traversal escape outside the package: link is left
    untouched (security contract)
  - skill bundle internal link: in-bundle layout preserved
  - multi-target install (copilot + claude): rewrite applied
    independently per host-tool deploy root

Wired into scripts/test-integration.sh as its own block following
the established log_info / pytest / log_success pattern.

Removes the thin TestLocalInstallSiblingLinkRewriting case from
test_local_install.py and leaves a NOTE pointing at the dedicated
E2E module.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three fixes from the Copilot reviewer pass:

1. _split_link_target ordering bug. The previous loop iterated
   ('#', '?') and returned on the first hit, so a link like
   'doc.md?x=1#sec' split at '#' and yielded path_part='doc.md?x=1'.
   The on-disk lookup then failed and the link was left unrewritten.
   Now finds the earliest occurrence of either delimiter so the full
   suffix ('?x=1#sec') is preserved on the rewritten target.
   Adds test_preserve_query_and_fragment_on_rewritten_link covering
   the combined case.

2. Defensive assert in test_resolve_links_for_installation_basic
   before .group(1) so a regex miss surfaces a clear failure instead
   of an opaque AttributeError.

3. Shorten the CHANGELOG entry to one concise line per repo
   convention while keeping the (#1147) suffix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel danielmeppiel merged commit aa584f6 into main May 6, 2026
25 checks passed
@danielmeppiel danielmeppiel deleted the fix/issue-1147-link-resolver-generalization branch May 6, 2026 11:58
sergio-sisternes-epam pushed a commit that referenced this pull request May 19, 2026
…#1147) (#1160)

* fix(install): rewrite in-package relative links to apm_modules/ paths

Sibling references from instructions/prompts/agents/commands to
in-package assets (e.g. `../../standards/style.md`) used to deploy
verbatim. After the .agents/.github split (#1103) those paths point
nowhere because primitives land at host-tool-specific destinations
that no longer match the package's authoring layout.

UnifiedLinkResolver now rewrites any in-package relative markdown
link at install time, scoped to the source package via the
authoritative `package_info.install_path` (handles ADO 4-segment
deps, virtual subdirs, monorepos, local _local/<name>/ packages).
Containment is enforced via `ensure_path_within`. Compile path is
deliberately untouched.

Tests: 10 new unit tests (rewrite happy path, fragments preserved,
escapes preserved, scheme/root-absolute skipped, opt-out when no
package_root, compile path NOT broadened) + 1 integration test that
reproduces #1147 end-to-end via apm install.

Docs: new guides/package-relative-links.md describing the contract.

Fixes #1147

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

* test(install): add E2E coverage for #1147 in-package link rewrite

Wires a dedicated end-to-end suite proving the install-time link
rewriter behaves correctly across the .agents/.github split that
broke intra-package relative paths in PR #1103.

tests/integration/test_link_rewrite_e2e.py drives the real apm
install CLI against fixture packages and asserts the deployed
markdown link targets, covering:

  - instruction -> sibling asset (happy path)
  - prompt -> sibling asset (happy path)
  - mixed link shapes in one body: rewritable, external URL,
    bare #fragment, and relative+#fragment (suffix preserved)
  - path-traversal escape outside the package: link is left
    untouched (security contract)
  - skill bundle internal link: in-bundle layout preserved
  - multi-target install (copilot + claude): rewrite applied
    independently per host-tool deploy root

Wired into scripts/test-integration.sh as its own block following
the established log_info / pytest / log_success pattern.

Removes the thin TestLocalInstallSiblingLinkRewriting case from
test_local_install.py and leaves a NOTE pointing at the dedicated
E2E module.

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

* fix(link-resolver): address #1160 review feedback

Three fixes from the Copilot reviewer pass:

1. _split_link_target ordering bug. The previous loop iterated
   ('#', '?') and returned on the first hit, so a link like
   'doc.md?x=1#sec' split at '#' and yielded path_part='doc.md?x=1'.
   The on-disk lookup then failed and the link was left unrewritten.
   Now finds the earliest occurrence of either delimiter so the full
   suffix ('?x=1#sec') is preserved on the rewritten target.
   Adds test_preserve_query_and_fragment_on_rewritten_link covering
   the combined case.

2. Defensive assert in test_resolve_links_for_installation_basic
   before .group(1) so a regex miss surfaces a clear failure instead
   of an opaque AttributeError.

3. Shorten the CHANGELOG entry to one concise line per repo
   convention while keeping the (#1147) suffix.

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

---------

Co-authored-by: Daniel Meppiel <copilot-rework@github.com>
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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Package relative paths broken due to .agents and .github splitting

2 participants