fix: address Copilot review findings on #1055 and #991 (consolidated)

[danielmeppiel]

Summary

Folds the still-actionable findings from the post-merge Copilot reviews of #1055 and #991 into one consolidated PR (per board policy: no follow-up issue/PR sprawl; admin-merge gate now requires reviewing Copilot findings).

Findings folded

From #1055 (CLAUDE_CONFIG_DIR support)

Finding	Resolution
Path traversal via .. segments	Already mitigated in merged code: resolve(strict=False) collapses .. BEFORE relative_to(home). No change needed.
root_dir contract violation (abs path stored)	Comment-only fix. Documented why the absolute-path fallback is safe: every downstream consumer joins as project_root / target.root_dir and pathlib.Path / <abs> evaluates to <abs>. Behavior is correct.
Test coverage gap for CLAUDE_CONFIG_DIR install/uninstall	Already covered: test_scope_install_uninstall.py:382 + test_scope_integration.py:212-279 (outside-home, traversal, whitespace, project-scope).
Stale docs hardcoding ~/.claude/	Already updated in merged code: guides/dependencies.md:338,346 + integrations/ide-tool-integration.md:172.
if self.name == "claude" extensibility	Refactor scope; not a bug. Deferred.

From #991 (link_resolver guards)

Finding	Resolution
NUL-byte docstring claim (Path.exists() does not silently 'reject')	Docstring corrected. Test behavior unchanged.

Validation

• Full unit suite for affected modules:

  uv run pytest tests/unit/compilation/test_link_resolver.py \
                tests/unit/integration/test_scope_integration.py \
                tests/unit/integration/test_scope_install_uninstall.py
  61 passed in 2.43s
  

• uv run --extra dev ruff check src/ tests/ -> All checks passed
• uv run --extra dev ruff format --check -> 598 files already formatted

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

[Copilot]

Pull request overview

Consolidates follow-up fixes from prior Copilot reviews (#1055, #991) by clarifying behavior in two edge-case documentation/comments without changing runtime behavior.

Changes:

• Updates the NUL-byte edge-case test docstring for _resolve_path to describe the expected downstream behavior more accurately.
• Adds an explanatory comment for the CLAUDE_CONFIG_DIR outside-$HOME fallback in TargetProfile.for_scope().

Show a summary per file

File	Description
tests/unit/compilation/test_link_resolver.py	Updates the NUL-byte test’s docstring to reflect actual Path.exists() behavior.
src/apm_cli/integration/targets.py	Adds rationale for the absolute root_dir fallback when CLAUDE_CONFIG_DIR is outside $HOME.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 2

[Copilot]

This new comment claims that storing an absolute root_dir is safe because downstream code does project_root / target.root_dir, but the install pipeline also records deployed paths via target_path.relative_to(project_root) (see install/services.py::_deployed_path_entry). When root_dir is absolute and outside project_root (user scope: $HOME), _deployed_path_entry will raise RuntimeError because only dynamic-root targets translate out-of-tree paths. Either update this comment to reflect that out-of-home paths are not currently lockfile-safe (and will fail), or extend the implementation to support this fallback (e.g., treat it as a dynamic-root target / URI scheme).

Suggested change

	# Storing an absolute path is safe because every downstream
	# consumer joins it as ``project_root / target.root_dir`` and
	# ``pathlib.Path / <absolute>`` evaluates to ``<absolute>``,
	# so deploy + cleanup both write to the absolute target.
	# Keep the absolute path so local deploy-path resolution
	# still targets ``abs_path`` here, because
	# ``project_root / <absolute>`` evaluates to ``<absolute>``.
	# Note: this is not currently lockfile-safe for out-of-tree
	# paths. Deployed-path recording elsewhere expects a
	# project-relative path unless the target uses the
	# dynamic-root translation flow.

[Copilot]

The docstring says a downstream ValueError from Path.exists() is acceptable for NUL-byte paths, but current callers (resolve_markdown_links / validate_link_targets) call .exists() without catching ValueError. If _resolve_path returns a Path containing a NUL, compilation/link validation will raise and abort instead of producing a warning. Consider tightening _resolve_path to return None for inputs containing \x00 (or wrapping .exists() calls to treat ValueError like "missing").

See below for a potential fix:

    def test_embedded_nul_byte_returns_none(self, base_dir):
        """An embedded NUL byte must be rejected by _resolve_path.

        Returning `None` prevents downstream callers from invoking
        `Path.exists()` on an invalid path and raising `ValueError`
        during compilation or link validation.
        """
        result = _resolve_path("foo\x00bar", base_dir)
        assert result is None

[danielmeppiel]

Both Copilot findings folded in (HEAD):

1. NUL byte in _resolve_path -- you were right, downstream .exists() raises ValueError and callers don't catch it. Now rejected at the resolver boundary; test tightened to assert None.

2. targets.for_scope absolute fallback -- correct that _deployed_path_entry would RuntimeError on out-of-tree paths. Verified the path is unreachable today (user-scope CLAUDE installs don't flow through that translator), but rewrote the comment to record the constraint so a future refactor that lockfiles user-scope deploys treats absolute root_dir as a dynamic-root case.