From 54e2480c13433380bf82ce2e09eeaed1f2301b1b Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 30 Apr 2026 12:51:24 +0200 Subject: [PATCH 1/2] fix: address Copilot review findings on #1055 and #991 (consolidated) Folds the still-actionable findings from the post-merge Copilot reviews of PRs #1055 and #991 into a single PR (no follow-up issue / PR sprawl). #1055 (CLAUDE_CONFIG_DIR support): - Document why the absolute-path fallback in TargetProfile.for_scope() is safe for downstream consumers (pathlib's 'rhs absolute wins' rule on '/' joins). Comment-only; no behavior change. - Findings already addressed by the original PR (verified): * resolve(strict=False) collapses '..' before relative_to(home) * docs/guides/dependencies.md + integrations/ide-tool-integration.md already cover CLAUDE_CONFIG_DIR in the merged version * test_scope_install_uninstall.py:382 + test_scope_integration.py already exercise install/uninstall + outside-home + traversal cases #991 (link_resolver guards): - Correct the embedded-NUL-byte test docstring: Path.exists() raises ValueError on NUL on most platforms (it does not silently return False). Cosmetic; test behavior unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/integration/targets.py | 6 ++++++ tests/unit/compilation/test_link_resolver.py | 9 ++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index 7a10a26c9..dc5f474c4 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -235,6 +235,12 @@ def for_scope(self, user_scope: bool = False) -> TargetProfile | None: # Keep ``root_dir`` home-relative so cleanup prefix matching holds. new_root = abs_path.relative_to(home).as_posix() except ValueError: + # Fallback: when CLAUDE_CONFIG_DIR points outside $HOME the + # user has explicitly opted out of the home-relative model. + # Storing an absolute path is safe because every downstream + # consumer joins it as ``project_root / target.root_dir`` and + # ``pathlib.Path / `` evaluates to ````, + # so deploy + cleanup both write to the absolute target. new_root = str(abs_path) if self.unsupported_user_primitives: diff --git a/tests/unit/compilation/test_link_resolver.py b/tests/unit/compilation/test_link_resolver.py index be97540d4..e661cb142 100644 --- a/tests/unit/compilation/test_link_resolver.py +++ b/tests/unit/compilation/test_link_resolver.py @@ -477,9 +477,12 @@ def test_whitespace_only_returns_none(self, base_dir): def test_embedded_nul_byte_does_not_crash(self, base_dir): """An embedded NUL byte must not crash _resolve_path itself. - The current containment relies on the caller's `.exists()` check to - reject the resulting path -- this lock-in is documented in the issue - ("Current containment code handles these correctly"). + Downstream containment is the caller's responsibility: on most + platforms `Path.exists()` raises `ValueError` for paths containing + NUL bytes (it does NOT silently return False). Either an early + `None` return from the resolver or a downstream ValueError on + `.exists()` is acceptable -- what we guard here is that the + resolver itself does not crash. """ # Either return value is acceptable; what matters is no exception. result = _resolve_path("foo\x00bar", base_dir) From 077ea3a558f8a6bec63a75abcb1a7f4abd3177f7 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 30 Apr 2026 13:13:20 +0200 Subject: [PATCH 2/2] fix: address Copilot review on #1065 Two valid findings from copilot-pull-request-reviewer: 1. link_resolver._resolve_path: NUL byte in input previously returned a Path that crashed downstream .exists() / .read_text() with ValueError, aborting markdown link resolution. Reject NUL at the resolver boundary so callers (resolve_markdown_links, validate_link_targets) get None as documented. Tightened test to assert None instead of 'either is fine'. 2. integration/targets.for_scope: prior comment claimed absolute root_dir was 'safe' for all downstream consumers. install/services. _deployed_path_entry would actually raise RuntimeError for an out-of-tree absolute path. Today this is unreachable because user-scope CLAUDE installs do not flow through that translator, but the comment now records the constraint so a future refactor that lockfiles user-scope deploys treats it as a dynamic-root case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/compilation/link_resolver.py | 7 ++++++ src/apm_cli/integration/targets.py | 17 ++++++++----- tests/unit/compilation/test_link_resolver.py | 25 ++++++++++---------- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/apm_cli/compilation/link_resolver.py b/src/apm_cli/compilation/link_resolver.py index 5dd10c5d1..3914c3146 100644 --- a/src/apm_cli/compilation/link_resolver.py +++ b/src/apm_cli/compilation/link_resolver.py @@ -433,6 +433,13 @@ def _resolve_path(path: str, base_path: Path) -> Path | None: """ if not path or not path.strip(): return None + # NUL bytes survive ``Path()`` construction on POSIX but every downstream + # filesystem call (``.exists()``, ``.is_file()``, ``.read_text()``) raises + # ``ValueError``. Callers in this module do not catch ``ValueError`` so an + # unguarded NUL would abort markdown link resolution / validation. Reject + # at the resolver boundary instead. + if "\x00" in path: + return None try: if Path(path).is_absolute(): return Path(path) diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index dc5f474c4..afb1932f6 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -235,12 +235,17 @@ def for_scope(self, user_scope: bool = False) -> TargetProfile | None: # Keep ``root_dir`` home-relative so cleanup prefix matching holds. new_root = abs_path.relative_to(home).as_posix() except ValueError: - # Fallback: when CLAUDE_CONFIG_DIR points outside $HOME the - # user has explicitly opted out of the home-relative model. - # Storing an absolute path is safe because every downstream - # consumer joins it as ``project_root / target.root_dir`` and - # ``pathlib.Path / `` evaluates to ````, - # so deploy + cleanup both write to the absolute target. + # Fallback: when CLAUDE_CONFIG_DIR points outside $HOME we + # store an absolute path. ``pathlib.Path / `` is + # ```` so deploy + cleanup write to the right + # place. Caveat: the lockfile path translator + # (``install/services._deployed_path_entry``) calls + # ``relative_to(project_root)`` and raises ``RuntimeError`` + # for out-of-tree paths that are not dynamic-root targets. + # Today this is unreachable because user-scope CLAUDE + # installs do not flow through that translator, but any + # future refactor that lockfiles user-scope deploys must + # treat absolute ``root_dir`` as a dynamic-root case. new_root = str(abs_path) if self.unsupported_user_primitives: diff --git a/tests/unit/compilation/test_link_resolver.py b/tests/unit/compilation/test_link_resolver.py index e661cb142..40ef8f127 100644 --- a/tests/unit/compilation/test_link_resolver.py +++ b/tests/unit/compilation/test_link_resolver.py @@ -474,19 +474,20 @@ def test_whitespace_only_returns_none(self, base_dir): assert _resolve_path("\t", base_dir) is None assert _resolve_path("\n", base_dir) is None - def test_embedded_nul_byte_does_not_crash(self, base_dir): - """An embedded NUL byte must not crash _resolve_path itself. - - Downstream containment is the caller's responsibility: on most - platforms `Path.exists()` raises `ValueError` for paths containing - NUL bytes (it does NOT silently return False). Either an early - `None` return from the resolver or a downstream ValueError on - `.exists()` is acceptable -- what we guard here is that the - resolver itself does not crash. + def test_embedded_nul_byte_returns_none(self, base_dir): + """An embedded NUL byte must produce ``None``, not a ``Path``. + + NUL bytes survive ``Path()`` construction on POSIX, but every + downstream filesystem call (``.exists()``, ``.is_file()``, + ``.read_text()``) raises ``ValueError``. Callers in + ``link_resolver`` (``resolve_markdown_links`` / + ``validate_link_targets``) do not catch ``ValueError``, so + returning a ``Path`` here would abort markdown link + resolution. The resolver rejects NUL at its boundary instead. """ - # Either return value is acceptable; what matters is no exception. - result = _resolve_path("foo\x00bar", base_dir) - assert result is None or isinstance(result, Path) + assert _resolve_path("foo\x00bar", base_dir) is None + assert _resolve_path("\x00", base_dir) is None + assert _resolve_path("a/b\x00c.md", base_dir) is None def test_posix_backslash_traversal_stays_relative(self, base_dir): """Backslashes are literal characters on POSIX, so the path stays under base_dir."""