diff --git a/.apm/instructions/linting.instructions.md b/.apm/instructions/linting.instructions.md index 9654360c5..32a63a575 100644 --- a/.apm/instructions/linting.instructions.md +++ b/.apm/instructions/linting.instructions.md @@ -10,20 +10,41 @@ report) that claims CI is green. ## CI-mirror commands -The `Lint` job runs: +The `Lint` job runs (see `.github/workflows/ci.yml`): -- `uv run --extra dev ruff check src/ tests/` -- `uv run --extra dev ruff format --check src/ tests/` +1. `uv run --extra dev ruff check src/ tests/` +2. `uv run --extra dev ruff format --check src/ tests/` +3. YAML I/O safety guard (rejects raw `yaml.dump(..., handle)` outside + `utils/yaml_io.py`; mark approved exceptions with `# yaml-io-exempt`). +4. File length guardrail (no `src/**/*.py` may exceed **2450 lines**). +5. No raw `str(path.relative_to(...))` patterns -- use + `portable_relpath()` from `apm_cli.utils.paths`. +6. **Code duplication guardrail (pylint R0801):** + `uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/` +7. Auth-protocol boundary check: `bash scripts/lint-auth-signals.sh` -Both must be silent. +All seven must succeed. CI evaluates these on the **PR merge commit** +(HEAD merged with current `main`), so duplication introduced by a +recent main commit can fail your PR even if your own diff is clean. +Always merge `main` locally before running the mirror. ## Local workflow - **Auto-fix style+imports:** `uv run --extra dev ruff check src/ tests/ --fix` - **Apply formatter:** `uv run --extra dev ruff format src/ tests/` -- **Verify (must be silent):** `uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/` +- **Verify the full Lint job (must all be silent / exit 0):** + ```bash + uv run --extra dev ruff check src/ tests/ \ + && uv run --extra dev ruff format --check src/ tests/ \ + && uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ \ + && bash scripts/lint-auth-signals.sh + ``` + (The YAML, file-length, and `relative_to` guards are pure-grep one-liners + from `ci.yml`; run them directly if you have touched those surfaces.) -Always run the verify pair before `git push` -- the CI Lint job +Always run the verify chain before `git push` -- the CI Lint job fails on any remaining diagnostic. ## Common surprises @@ -36,11 +57,13 @@ fails on any remaining diagnostic. - `F401` / `F841` -- remove unused imports / unused locals. - `SIM103` -- inline negated returns where the body is one line. - `I001` -- import sort order (auto-fixable). +- `R0801` -- 10+ identical lines across two files. Extract the shared + block into a base class / helper module instead of disabling. ## Lifecycle binding This is the canonical lint contract for the repo. Skills that produce artifacts asserting green CI -- notably `pr-description-skill` (whose "Validation evidence" row covers CI checks) -- inherit this -gate transitively. Do NOT redefine ruff commands inside individual -skills; honor this instruction before invoking them. +gate transitively. Do NOT redefine ruff or pylint commands inside +individual skills; honor this instruction before invoking them. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 28118a193..1e6af7f2d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,5 +1,5 @@ - + @@ -11,20 +11,41 @@ report) that claims CI is green. ## CI-mirror commands -The `Lint` job runs: +The `Lint` job runs (see `.github/workflows/ci.yml`): -- `uv run --extra dev ruff check src/ tests/` -- `uv run --extra dev ruff format --check src/ tests/` +1. `uv run --extra dev ruff check src/ tests/` +2. `uv run --extra dev ruff format --check src/ tests/` +3. YAML I/O safety guard (rejects raw `yaml.dump(..., handle)` outside + `utils/yaml_io.py`; mark approved exceptions with `# yaml-io-exempt`). +4. File length guardrail (no `src/**/*.py` may exceed **2450 lines**). +5. No raw `str(path.relative_to(...))` patterns -- use + `portable_relpath()` from `apm_cli.utils.paths`. +6. **Code duplication guardrail (pylint R0801):** + `uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/` +7. Auth-protocol boundary check: `bash scripts/lint-auth-signals.sh` -Both must be silent. +All seven must succeed. CI evaluates these on the **PR merge commit** +(HEAD merged with current `main`), so duplication introduced by a +recent main commit can fail your PR even if your own diff is clean. +Always merge `main` locally before running the mirror. ## Local workflow - **Auto-fix style+imports:** `uv run --extra dev ruff check src/ tests/ --fix` - **Apply formatter:** `uv run --extra dev ruff format src/ tests/` -- **Verify (must be silent):** `uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/` +- **Verify the full Lint job (must all be silent / exit 0):** + ```bash + uv run --extra dev ruff check src/ tests/ \ + && uv run --extra dev ruff format --check src/ tests/ \ + && uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ \ + && bash scripts/lint-auth-signals.sh + ``` + (The YAML, file-length, and `relative_to` guards are pure-grep one-liners + from `ci.yml`; run them directly if you have touched those surfaces.) -Always run the verify pair before `git push` -- the CI Lint job +Always run the verify chain before `git push` -- the CI Lint job fails on any remaining diagnostic. ## Common surprises @@ -37,14 +58,16 @@ fails on any remaining diagnostic. - `F401` / `F841` -- remove unused imports / unused locals. - `SIM103` -- inline negated returns where the body is one line. - `I001` -- import sort order (auto-fixable). +- `R0801` -- 10+ identical lines across two files. Extract the shared + block into a base class / helper module instead of disabling. ## Lifecycle binding This is the canonical lint contract for the repo. Skills that produce artifacts asserting green CI -- notably `pr-description-skill` (whose "Validation evidence" row covers CI checks) -- inherit this -gate transitively. Do NOT redefine ruff commands inside individual -skills; honor this instruction before invoking them. +gate transitively. Do NOT redefine ruff or pylint commands inside +individual skills; honor this instruction before invoking them. --- diff --git a/CHANGELOG.md b/CHANGELOG.md index f6743eda6..1d80b4771 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Unit test coverage raised to 88% (gate: `fail_under = 80`); integration test coverage raised to 71% with first CI gate at 55%. (#1402) +### Fixed + +- Root `.apm` hooks no longer duplicate after renaming the project directory or using git worktrees; Claude, Codex, Cursor, Gemini, and Windsurf hook configs stay idempotent across checkouts. The hook source-id is now derived from `apm.yml`'s `name` field instead of `install_path.name`, and `apm install` silently heals stale same-content entries from prior checkout basenames. Copilot is unaffected (its hooks live in per-file namespaces under `.github/hooks/`, not a shared merged config). (#1392, closes #1329) + ## [0.14.1] - 2026-05-20 ### Added diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index d5e865539..a4869f27c 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -248,6 +248,30 @@ def _infer_registry_name(package): return "" + @classmethod + def _select_best_package(cls, packages): + """Select the best package for installation from available packages. + + Prioritizes packages in order: npm, docker, pypi, homebrew, others. + Uses ``_infer_registry_name`` so selection works even when the + registry API returns empty ``registry_name``. + + Args: + packages (list): List of package dictionaries. + + Returns: + dict: Best package to use, or None if no suitable package found. + """ + priority_order = ["npm", "docker", "pypi", "homebrew"] + + for target in priority_order: + for package in packages: + if cls._infer_registry_name(package) == target: + return package + + # If no priority package found, return the first one + return packages[0] if packages else None + @staticmethod def _warn_input_variables(mapping, server_name, runtime_label): """Emit a warning for each ``${input:...}`` reference found in *mapping*. @@ -542,6 +566,16 @@ def _replace_runtime(match): return processed + def _resolve_env_placeholders(self, value, resolved_env): + """Legacy thin wrapper for backward compatibility. + + Kept because external callers and the phase-3 test suite invoke + the pre-#1277 name. Delegates to ``_resolve_variable_placeholders`` + with an empty ``runtime_vars`` map. New code should call + ``_resolve_variable_placeholders`` directly. + """ + return self._resolve_variable_placeholders(value, resolved_env, {}) + # ------------------------------------------------------------------ # Shared server-info helpers (used by all adapter subclasses) # ------------------------------------------------------------------ diff --git a/src/apm_cli/adapters/client/codex.py b/src/apm_cli/adapters/client/codex.py index 43794a873..17615db3a 100644 --- a/src/apm_cli/adapters/client/codex.py +++ b/src/apm_cli/adapters/client/codex.py @@ -548,26 +548,3 @@ def _select_remote_with_url(remotes): if url: return remote return None - - def _select_best_package(self, packages): - """Select the best package for installation from available packages. - - Prioritizes packages in order: npm, docker, pypi, homebrew, others. - Uses ``_infer_registry_name`` so selection works even when the - registry API returns empty ``registry_name``. - - Args: - packages (list): List of package dictionaries. - - Returns: - dict: Best package to use, or None if no suitable package found. - """ - priority_order = ["npm", "docker", "pypi", "homebrew"] - - for target in priority_order: - for package in packages: - if self._infer_registry_name(package) == target: - return package - - # If no priority package found, return the first one - return packages[0] if packages else None diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 1769b98ed..778038849 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -1007,29 +1007,6 @@ def _select_remote_with_url(remotes): return remote return None - def _select_best_package(self, packages): - """Select the best package for installation from available packages. - - Prioritizes packages in order: npm, docker, pypi, homebrew, others. - Uses ``_infer_registry_name`` so selection works even when the - registry API returns empty ``registry_name``. - - Args: - packages (list): List of package dictionaries. - - Returns: - dict: Best package to use, or None if no suitable package found. - """ - priority_order = ["npm", "docker", "pypi", "homebrew"] - - for target in priority_order: - for package in packages: - if self._infer_registry_name(package) == target: - return package - - # If no priority package found, return the first one - return packages[0] if packages else None - def _is_github_server(self, server_name, url): """Securely determine if a server is a GitHub MCP server. diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 4c8272d88..c20342c01 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -51,9 +51,15 @@ from pathlib import Path from typing import Dict, List, Optional, Tuple # noqa: F401, UP035 +import yaml + from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult from apm_cli.utils.console import _rich_warning -from apm_cli.utils.path_security import PathTraversalError, ensure_path_within +from apm_cli.utils.path_security import ( + PathTraversalError, + ensure_path_within, + validate_path_segments, +) from apm_cli.utils.paths import portable_relpath _log = logging.getLogger(__name__) @@ -583,17 +589,286 @@ def _rewrite_hooks_data( return rewritten, unique_scripts - def _get_package_name(self, package_info) -> str: + @staticmethod + def _is_root_local_package(package_info, project_root: Path | None) -> bool: + """Return True when *package_info* represents the project's own .apm content.""" + if project_root is None: + return False + try: + return Path(package_info.install_path).resolve() == Path(project_root).resolve() + except (OSError, RuntimeError): + return False + + @staticmethod + def _safe_source_name(value: str | None, fallback: str = "_local") -> str: + """Return a stable source marker that is also safe for hook script paths.""" + if not isinstance(value, str) or not value: + return fallback + safe = re.sub(r"[^A-Za-z0-9._-]+", "-", value.strip()) + # Collapse any run of 2+ dots to a single dot before stripping edges. + # Embedded sequences like "foo..bar" would otherwise pass through the + # earlier guard and reach downstream Path joins as a parent-dir hop. + safe = re.sub(r"\.{2,}", ".", safe).strip(".-_") + if not safe or safe in {".", ".."}: + return fallback + return safe + + @staticmethod + def _get_root_local_package_name(package_info, project_root: Path) -> str: + """Get the stable source marker for root .apm content.""" + apm_yml = Path(project_root) / "apm.yml" + if apm_yml.exists(): + try: + from apm_cli.utils.yaml_io import load_yaml + + data = load_yaml(apm_yml) + if isinstance(data, dict): + manifest_name = HookIntegrator._safe_source_name(data.get("name")) + if manifest_name != "_local": + return manifest_name + except (OSError, ValueError, yaml.YAMLError) as exc: + _log.debug( + "Hook integrator: apm.yml manifest unreadable for %s (%s: %s), " + "falling back to install_path basename", + project_root, + exc.__class__.__name__, + exc, + ) + + package = getattr(package_info, "package", None) + package_name = HookIntegrator._safe_source_name(getattr(package, "name", None)) + if package_name != "_local": + return package_name + return "_local" + + def _get_package_name(self, package_info, project_root: Path | None = None) -> str: """Get a short package name for use in file/directory naming. Args: package_info: PackageInfo object + project_root: When provided and the package is the project root, + reads ``apm.yml`` ``name`` for a stable source marker instead + of falling back to ``install_path.name`` (which drifts on + directory renames and worktrees). See #1329. Returns: - str: Package name derived from install path + str: Package name used as hook source marker and script namespace """ + if self._is_root_local_package(package_info, project_root): + return HookIntegrator._get_root_local_package_name(package_info, Path(project_root)) return package_info.install_path.name + @staticmethod + def _get_hook_source_marker( + package_info, + project_root: Path, + package_name: str, + ) -> str: + """Get the marker stored in merged hook JSON for ownership cleanup.""" + if HookIntegrator._is_root_local_package(package_info, project_root): + if package_name == "_local": + return "_local" + return f"_local/{package_name}" + return package_name + + @staticmethod + def _hook_entry_content_key(entry: dict) -> str: + """Build a stable comparison key excluding APM ownership metadata.""" + comparable = {k: v for k, v in sorted(entry.items()) if k != "_apm_source"} + return json.dumps(comparable, sort_keys=True, separators=(",", ":")) + + @staticmethod + def _dependency_hook_sources(project_root: Path) -> set[str]: + """Return source markers that correspond to installed dependency dirs.""" + apm_modules = project_root / "apm_modules" + if not apm_modules.is_dir(): + return set() + + lockfile_paths, lockfile_readable = HookIntegrator._lockfile_dependency_paths(project_root) + if lockfile_readable: + sources: set[str] = set() + for rel_path in lockfile_paths: + package_path = HookIntegrator._safe_dependency_path(apm_modules, rel_path) + if package_path is None: + continue + HookIntegrator._add_dependency_source(sources, package_path) + return sources + + return HookIntegrator._bounded_dependency_hook_sources(apm_modules) + + @staticmethod + def _lockfile_dependency_paths(project_root: Path) -> tuple[list[str], bool]: + """Return installed dependency paths from a readable lockfile, if present.""" + try: + from apm_cli.deps.lockfile import LEGACY_LOCKFILE_NAME, LockFile, get_lockfile_path + + lockfile_path = get_lockfile_path(project_root) + if not lockfile_path.exists(): + legacy_path = project_root / LEGACY_LOCKFILE_NAME + if legacy_path.exists(): + lockfile_path = legacy_path + if not lockfile_path.exists(): + return [], False + lockfile = LockFile.read(lockfile_path) + if lockfile is None: + return [], False + return lockfile.get_installed_paths(project_root / "apm_modules"), True + except (AttributeError, OSError, TypeError, ValueError, KeyError): + return [], False + + @staticmethod + def _safe_dependency_path(apm_modules: Path, rel_path: str) -> Path | None: + """Return a lockfile dependency path without escaping apm_modules.""" + try: + validate_path_segments( + rel_path, + context="lockfile dependency path", + reject_empty=True, + ) + package_path = apm_modules / Path(rel_path) + ensure_path_within(package_path, apm_modules) + if HookIntegrator._has_symlink_component(apm_modules, package_path): + return None + return package_path + except (OSError, PathTraversalError, RuntimeError, TypeError): + return None + + @staticmethod + def _has_symlink_component(apm_modules: Path, package_path: Path) -> bool: + """Return True when any component below apm_modules is a symlink.""" + try: + relative = package_path.relative_to(apm_modules) + current = apm_modules + for part in relative.parts: + current = current / part + if current.is_symlink(): + return True + return False + except (OSError, ValueError): + return True + + @staticmethod + def _is_dependency_package_dir(path: Path) -> bool: + """Return True when *path* looks like an installed package root.""" + try: + hooks = path / "hooks" + apm_hooks = path / ".apm" / "hooks" + apm_yml = path / "apm.yml" + skill_md = path / "SKILL.md" + return ( + (hooks.is_dir() and not hooks.is_symlink()) + or (apm_hooks.is_dir() and not apm_hooks.is_symlink()) + or (apm_yml.is_file() and not apm_yml.is_symlink()) + or (skill_md.is_file() and not skill_md.is_symlink()) + ) + except OSError: + return False + + @staticmethod + def _add_dependency_source(sources: set[str], package_path: Path) -> bool: + """Add package_path.name to sources when package_path is a package root.""" + try: + if ( + not package_path.is_dir() + or package_path.is_symlink() + or not HookIntegrator._is_dependency_package_dir(package_path) + ): + return False + except OSError: + return False + sources.add(package_path.name) + return True + + @staticmethod + def _child_dependency_dirs(path: Path) -> list[Path]: + """Return direct non-hidden child dirs without following symlink roots.""" + try: + if path.is_symlink() or not path.is_dir(): + return [] + return sorted( + [ + child + for child in path.iterdir() + if not child.is_symlink() and child.is_dir() and not child.name.startswith(".") + ], + key=lambda child: child.name, + ) + except OSError: + return [] + + @staticmethod + def _collect_known_subdirectory_sources(sources: set[str], repo_root: Path) -> None: + """Collect dependency sources from known virtual subdirectory layouts.""" + for namespace in ("collections", "skills"): + for package_path in HookIntegrator._child_dependency_dirs(repo_root / namespace): + HookIntegrator._add_dependency_source(sources, package_path) + + apm_dir = repo_root / ".apm" + try: + if apm_dir.is_symlink() or not apm_dir.is_dir(): + return + except OSError: + return + for primitive in ("agents", "commands", "hooks", "instructions", "prompts", "skills"): + for package_path in HookIntegrator._child_dependency_dirs(apm_dir / primitive): + HookIntegrator._add_dependency_source(sources, package_path) + + @staticmethod + def _collect_remote_dependency_sources(sources: set[str], namespace: Path) -> None: + """Collect fallback sources from explicit remote install layouts.""" + if HookIntegrator._add_dependency_source(sources, namespace): + return + + for repo_or_project in HookIntegrator._child_dependency_dirs(namespace): + if HookIntegrator._add_dependency_source(sources, repo_or_project): + continue + + HookIntegrator._collect_known_subdirectory_sources(sources, repo_or_project) + + for ado_repo in HookIntegrator._child_dependency_dirs(repo_or_project): + if HookIntegrator._add_dependency_source(sources, ado_repo): + continue + HookIntegrator._collect_known_subdirectory_sources(sources, ado_repo) + + @staticmethod + def _collect_local_dependency_sources(sources: set[str], local_namespace: Path) -> None: + """Collect apm_modules/_local/ package roots only.""" + for local_package in HookIntegrator._child_dependency_dirs(local_namespace): + HookIntegrator._add_dependency_source(sources, local_package) + + @staticmethod + def _bounded_dependency_hook_sources(apm_modules: Path) -> set[str]: + """Fallback source scan limited to known apm_modules package layouts.""" + sources: set[str] = set() + + for package_root in HookIntegrator._child_dependency_dirs(apm_modules): + if package_root.name == "_local": + HookIntegrator._collect_local_dependency_sources(sources, package_root) + continue + + HookIntegrator._collect_remote_dependency_sources(sources, package_root) + return sources + + @staticmethod + def _should_remove_prior_merged_entry( + entry, + *, + source_marker: str, + fresh_content_keys: set[str], + heal_stale_root_source: bool, + dependency_sources: set[str], + remove_current_source: bool, + ) -> bool: + """Return True when an existing merged-hook entry should be replaced.""" + if not isinstance(entry, dict): + return False + source = entry.get("_apm_source") + if remove_current_source and source == source_marker: + return True + if not heal_stale_root_source or not source or source in dependency_sources: + return False + return HookIntegrator._hook_entry_content_key(entry) in fresh_content_keys + def integrate_package_hooks( self, package_info, @@ -633,7 +908,7 @@ def integrate_package_hooks( hooks_dir = project_root / root_dir / "hooks" hooks_dir.mkdir(parents=True, exist_ok=True) - package_name = self._get_package_name(package_info) + package_name = self._get_package_name(package_info, project_root) hooks_integrated = 0 scripts_copied = 0 scripts_adopted = 0 @@ -739,7 +1014,12 @@ def _integrate_merged_hooks( if not hook_files: return _empty - package_name = self._get_package_name(package_info) + package_name = self._get_package_name(package_info, project_root) + source_marker = self._get_hook_source_marker(package_info, project_root, package_name) + heal_stale_root_source = self._is_root_local_package(package_info, project_root) + dependency_sources = ( + self._dependency_hook_sources(project_root) if heal_stale_root_source else set() + ) hooks_integrated = 0 scripts_copied = 0 scripts_adopted = 0 @@ -825,7 +1105,12 @@ def _integrate_merged_hooks( # Mark each entry with APM source for sync/cleanup for entry in entries: if isinstance(entry, dict): - entry["_apm_source"] = package_name + entry["_apm_source"] = source_marker + fresh_content_keys = { + self._hook_entry_content_key(entry) + for entry in entries + if isinstance(entry, dict) + } # Idempotent upsert: drop any prior entries owned by this # package before appending fresh ones. Without this, every @@ -835,13 +1120,42 @@ def _integrate_merged_hooks( # with multiple hook files targeting the same event # contributes each file's entries in turn, and stripping # on every iteration would erase earlier files' work. - if event_name not in cleared_events: + remove_current_source = event_name not in cleared_events + if remove_current_source or heal_stale_root_source: # Clear from the normalised event - json_config["hooks"][event_name] = [ + prior_entries = json_config["hooks"][event_name] + kept_entries = [ e - for e in json_config["hooks"][event_name] - if not (isinstance(e, dict) and e.get("_apm_source") == package_name) + for e in prior_entries + if not self._should_remove_prior_merged_entry( + e, + source_marker=source_marker, + fresh_content_keys=fresh_content_keys, + heal_stale_root_source=heal_stale_root_source, + dependency_sources=dependency_sources, + remove_current_source=remove_current_source, + ) ] + if heal_stale_root_source: + kept_ids = {id(e) for e in kept_entries} + healed = sum( + 1 + for e in prior_entries + if isinstance(e, dict) + and e.get("_apm_source") + and e.get("_apm_source") != source_marker + and e.get("_apm_source") not in dependency_sources + and id(e) not in kept_ids + ) + if healed: + _log.debug( + "Hook integrator: healed %d stale same-content " + "merged hook entries for source %s in event %s", + healed, + source_marker, + event_name, + ) + json_config["hooks"][event_name] = kept_entries # Also clear from any alias events that map to # this normalised name (handles migration from # corrupted installs with mixed-case event keys). @@ -850,8 +1164,13 @@ def _integrate_merged_hooks( json_config["hooks"][alias] = [ e for e in json_config["hooks"][alias] - if not ( - isinstance(e, dict) and e.get("_apm_source") == package_name + if not self._should_remove_prior_merged_entry( + e, + source_marker=source_marker, + fresh_content_keys=fresh_content_keys, + heal_stale_root_source=heal_stale_root_source, + dependency_sources=dependency_sources, + remove_current_source=remove_current_source, ) ] # Remove the alias key entirely if now empty diff --git a/tests/integration/test_hook_root_source_drift_e2e.py b/tests/integration/test_hook_root_source_drift_e2e.py new file mode 100644 index 000000000..b29543333 --- /dev/null +++ b/tests/integration/test_hook_root_source_drift_e2e.py @@ -0,0 +1,209 @@ +"""End-to-end regression for #1329: stale root hook _apm_source heals on reinstall. + +Parametrized across every harness whose hooks live in a merged config file +(Claude, Codex, Cursor, Gemini, Windsurf). Copilot is intentionally excluded: +its hooks live in per-file namespaces under ``.github/hooks/`` and are not +subject to the merged-config drift this fix targets. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +CLI = [sys.executable, "-m", "apm_cli.cli"] + + +def _run(cwd: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(CLI + list(args), cwd=str(cwd), capture_output=True, text=True) + + +# Per-target on-disk layout descriptors: +# - settings_rel: path (relative to project root) of the merged hook config +# - sidecar_rel: path of the apm-hooks.json sidecar (schema-strict targets only) +# - event_key: event key under which PreToolUse entries land for the target +# +# Sidecar-bearing targets (currently Claude) keep _apm_source in the sidecar +# rather than in settings.json; the heal assertion reads from there. +_HARNESS_CASES = [ + pytest.param( + "claude", + ".claude/settings.json", + ".claude/apm-hooks.json", + "PreToolUse", + id="claude", + ), + pytest.param( + "codex", + ".codex/hooks.json", + None, + "PreToolUse", + id="codex", + ), + pytest.param( + "cursor", + ".cursor/hooks.json", + None, + "PreToolUse", + id="cursor", + ), + pytest.param( + "gemini", + ".gemini/settings.json", + None, + "BeforeTool", + id="gemini", + ), + pytest.param( + "windsurf", + ".windsurf/hooks.json", + None, + "PreToolUse", + id="windsurf", + ), +] + + +def _load_sources( + project: Path, + settings_rel: str, + sidecar_rel: str | None, + event_key: str, +) -> list[str]: + """Return the list of _apm_source markers for the target's PreToolUse-equivalent. + + Entries lacking ``_apm_source`` (user-owned hooks) are excluded so the + heal assertion can compare just the APM-owned slice. + """ + if sidecar_rel: + sidecar = json.loads((project / sidecar_rel).read_text(encoding="utf-8")) + return [ + e["_apm_source"] + for e in sidecar.get(event_key, []) + if isinstance(e, dict) and e.get("_apm_source") + ] + settings = json.loads((project / settings_rel).read_text(encoding="utf-8")) + return [ + e["_apm_source"] + for e in settings.get("hooks", {}).get(event_key, []) + if isinstance(e, dict) and e.get("_apm_source") + ] + + +def _rewrite_source( + project: Path, + settings_rel: str, + sidecar_rel: str | None, + event_key: str, + *, + old: str, + new: str, +) -> None: + """Mutate _apm_source markers in-place to simulate a stale checkout basename.""" + target_rel = sidecar_rel or settings_rel + target = project / target_rel + data = json.loads(target.read_text(encoding="utf-8")) + container = data if sidecar_rel else data.get("hooks", {}) + entries = container.get(event_key, []) + for entry in entries: + if isinstance(entry, dict) and entry.get("_apm_source") == old: + entry["_apm_source"] = new + target.write_text(json.dumps(data), encoding="utf-8") + + +@pytest.mark.parametrize("target, settings_rel, sidecar_rel, event_key", _HARNESS_CASES) +def test_root_hook_source_drift_heals_on_reinstall( + tmp_path: Path, + target: str, + settings_rel: str, + sidecar_rel: str | None, + event_key: str, +) -> None: + """After a checkout rename, a second `apm install` heals stale source markers. + + Strategy: install once cleanly so the integrator writes target-shaped entries + with `_local/myapp`; then rewrite the marker to simulate an old checkout + basename and append a user-owned hook; then install again and assert the + stale marker is gone, the user-owned hook survives, and exactly one APM + entry with `_local/myapp` remains. + """ + project = tmp_path / "myapp" + project.mkdir() + (project / "apm.yml").write_text( + f"name: myapp\nversion: 0.0.0\ntargets:\n - {target}\n", + encoding="utf-8", + ) + hooks_dir = project / ".apm" / "hooks" + hooks_dir.mkdir(parents=True) + (hooks_dir / "pre.json").write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo apm-managed"}], + } + ] + } + } + ), + encoding="utf-8", + ) + + # First install: produces target-shaped entries marked `_local/myapp`. + first = _run(project, "install") + assert first.returncode == 0, first.stderr or first.stdout + assert _load_sources(project, settings_rel, sidecar_rel, event_key) == ["_local/myapp"], ( + "First install must produce a single _local/myapp entry; " + f"got {_load_sources(project, settings_rel, sidecar_rel, event_key)}" + ) + + # Simulate a legacy install whose _apm_source came from an old checkout basename. + _rewrite_source( + project, + settings_rel, + sidecar_rel, + event_key, + old="_local/myapp", + new="old-checkout-name", + ) + + # Append a user-owned entry directly to the settings file (never in the sidecar). + settings_path = project / settings_rel + settings_data = json.loads(settings_path.read_text(encoding="utf-8")) + settings_data.setdefault("hooks", {}).setdefault(event_key, []).append( + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo user-owned"}], + } + ) + settings_path.write_text(json.dumps(settings_data), encoding="utf-8") + + # Second install: must heal the stale marker without touching the user-owned entry. + second = _run(project, "install") + assert second.returncode == 0, second.stderr or second.stdout + + sources = _load_sources(project, settings_rel, sidecar_rel, event_key) + assert sources == ["_local/myapp"], ( + f"Expected single _local/myapp entry after heal for {target}, got {sources}" + ) + + settings_data = json.loads(settings_path.read_text(encoding="utf-8")) + entries = settings_data.get("hooks", {}).get(event_key, []) + user_owned = [ + e + for e in entries + if isinstance(e, dict) + and isinstance(e.get("hooks"), list) + and e["hooks"] + and isinstance(e["hooks"][0], dict) + and e["hooks"][0].get("command") == "echo user-owned" + ] + assert len(user_owned) == 1, ( + f"User-owned hook entry must survive healing for {target}; entries={entries}" + ) diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 9ee3e2d00..0029c2962 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -3051,6 +3051,21 @@ def _make_pkg( (hooks_dir / filename).write_text(json.dumps(data), encoding="utf-8") return _make_package_info(pkg_dir, pkg_name) + def _make_pkg_at( + self, + project: Path, + relative_path: str, + pkg_name: str, + hook_files: dict, + ) -> PackageInfo: + """Create a package below apm_modules at a specific relative path.""" + pkg_dir = project / "apm_modules" / Path(relative_path) + hooks_dir = pkg_dir / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + for filename, data in hook_files.items(): + (hooks_dir / filename).write_text(json.dumps(data), encoding="utf-8") + return _make_package_info(pkg_dir, pkg_name) + def _read_claude_settings(self, project: Path) -> dict: """Return parsed .claude/settings.json (or empty dict if absent).""" path = project / ".claude" / "settings.json" @@ -3058,6 +3073,39 @@ def _read_claude_settings(self, project: Path) -> dict: return {} return json.loads(path.read_text(encoding="utf-8")) + def _read_claude_sidecar(self, project: Path) -> dict: + """Return parsed .claude/apm-hooks.json sidecar (or empty dict if absent).""" + path = project / ".claude" / "apm-hooks.json" + if not path.exists(): + return {} + return json.loads(path.read_text(encoding="utf-8")) + + def _claude_sources(self, project: Path, event: str) -> list[str]: + """Return _apm_source markers for an event, ordered by settings.json entries. + + Schema-strict Claude stores ownership in apm-hooks.json sidecar; match each + settings.json entry by content to its sidecar twin so order is preserved. + """ + entries = self._read_claude_settings(project).get("hooks", {}).get(event, []) + sidecar_entries = self._read_claude_sidecar(project).get(event, []) + pool = list(sidecar_entries) + sources: list[str] = [] + for entry in entries: + if not isinstance(entry, dict): + continue + cmp = {k: v for k, v in entry.items() if k != "_apm_source"} + match_idx = None + for idx, sc in enumerate(pool): + if not isinstance(sc, dict): + continue + sc_cmp = {k: v for k, v in sc.items() if k != "_apm_source"} + if sc_cmp == cmp: + match_idx = idx + break + if match_idx is not None: + sources.append(pool.pop(match_idx).get("_apm_source")) + return sources + def _read_cursor_hooks(self, project: Path) -> dict: """Return parsed .cursor/hooks.json (or empty dict if absent).""" path = project / ".cursor" / "hooks.json" @@ -3065,6 +3113,49 @@ def _read_cursor_hooks(self, project: Path) -> dict: return {} return json.loads(path.read_text(encoding="utf-8")) + def _read_codex_hooks(self, project: Path) -> dict: + """Return parsed .codex/hooks.json (or empty dict if absent).""" + path = project / ".codex" / "hooks.json" + if not path.exists(): + return {} + return json.loads(path.read_text(encoding="utf-8")) + + def _make_root_local_pkg( + self, + project: Path, + *, + manifest_name: str = "sample-project", + hook_data: dict | None = None, + ) -> PackageInfo: + """Create root .apm hook content plus apm.yml metadata.""" + if hook_data is None: + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + } + ] + } + } + (project / "apm.yml").write_text( + f"name: {manifest_name}\nversion: 0.0.0\n", + encoding="utf-8", + ) + hooks_dir = project / ".apm" / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + (hooks_dir / "pre-push-review.json").write_text( + json.dumps(hook_data), + encoding="utf-8", + ) + return _make_package_info(project, project.name) + # ------------------------------------------------------------------ # Group A: Target-aware file routing # ------------------------------------------------------------------ @@ -3545,6 +3636,466 @@ def test_content_dedup_preserves_cross_package(self, temp_project: Path) -> None f"Cross-package identical entries must both be present; got {len(entries)}" ) + def test_root_local_source_uses_manifest_name(self, temp_project: Path) -> None: + """Root .apm hooks use stable apm.yml metadata, not checkout basename.""" + manifest_name = f"{temp_project.name}-manifest" + pkg_info = self._make_root_local_pkg(temp_project, manifest_name=manifest_name) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == [f"_local/{manifest_name}"] + + def test_root_local_heals_stale_source_in_claude_settings( + self, + temp_project: Path, + ) -> None: + """Root .apm reinstall removes same-content entries from old checkout sources.""" + pkg_info = self._make_root_local_pkg(temp_project, manifest_name="sample-project") + settings_path = temp_project / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "suspicious-bardeen-e50cf8", + }, + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo user-owned"}], + }, + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == ["_local/sample-project"] + # settings.json must retain both the managed entry and the user-owned hook + assert len(entries) == 2 + user_owned = [e for e in entries if e["hooks"][0]["command"] == "echo user-owned"] + assert len(user_owned) == 1 + + def test_root_local_heals_stale_source_in_codex_hooks( + self, + temp_project: Path, + ) -> None: + """Codex merged hooks get the same stale root-source healing.""" + (temp_project / ".codex").mkdir() + pkg_info = self._make_root_local_pkg(temp_project, manifest_name="sample-project") + hooks_path = temp_project / ".codex" / "hooks.json" + hooks_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "suspicious-bardeen-e50cf8", + } + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_codex(pkg_info, temp_project) + + entries = self._read_codex_hooks(temp_project)["hooks"]["PreToolUse"] + assert len(entries) == 1 + assert entries[0]["_apm_source"] == "_local/sample-project" + + def test_root_local_healer_preserves_dependency_source_entries( + self, + temp_project: Path, + ) -> None: + """Same-content dependency hooks are not mistaken for stale root hooks.""" + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="sample-project", + hook_data=hook_data, + ) + dep_info = self._make_pkg(temp_project, "dep-hooks", {"hooks.json": hook_data}) + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(dep_info, temp_project) + + settings_path = temp_project / ".claude" / "settings.json" + settings = json.loads(settings_path.read_text(encoding="utf-8")) + settings["hooks"]["PreToolUse"].append( + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "suspicious-bardeen-e50cf8", + } + ) + settings_path.write_text(json.dumps(settings), encoding="utf-8") + + integrator.integrate_package_hooks_claude(root_info, temp_project) + + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == ["dep-hooks", "_local/sample-project"] + + @pytest.mark.parametrize( + "relative_path", + [ + "owner/dep-hooks", + "org/project/dep-hooks", + "owner/repo/collections/dep-hooks", + "org/project/repo/collections/dep-hooks", + "owner/repo/.apm/skills/dep-hooks", + "_local/dep-hooks", + ], + ) + def test_root_local_healer_preserves_bounded_dependency_layouts( + self, + temp_project: Path, + relative_path: str, + ) -> None: + """Bounded dependency scans preserve known package root layouts.""" + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="sample-project", + hook_data=hook_data, + ) + dep_info = self._make_pkg_at( + temp_project, + relative_path, + "dep-hooks", + {"hooks.json": hook_data}, + ) + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(dep_info, temp_project) + + settings_path = temp_project / ".claude" / "settings.json" + settings = json.loads(settings_path.read_text(encoding="utf-8")) + settings["hooks"]["PreToolUse"].append( + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "stale-root-name", + } + ) + settings_path.write_text(json.dumps(settings), encoding="utf-8") + + integrator.integrate_package_hooks_claude(root_info, temp_project) + + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == ["dep-hooks", "_local/sample-project"] + + def test_dependency_hook_sources_uses_lockfile_paths( + self, + temp_project: Path, + ) -> None: + """Readable lockfiles provide exact dependency roots without broad scans.""" + pkg_dir = temp_project / "apm_modules" / "owner" / "repo" / "collections" / "dep-hooks" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: dep-hooks\n", encoding="utf-8") + (temp_project / "apm.lock.yaml").write_text( + "\n".join( + [ + 'lockfile_version: "1"', + "dependencies:", + " - repo_url: owner/repo", + " virtual_path: collections/dep-hooks", + " is_virtual: true", + "", + ] + ), + encoding="utf-8", + ) + + assert HookIntegrator._dependency_hook_sources(temp_project) == {"dep-hooks"} + + def test_dependency_hook_sources_rejects_lockfile_symlink_root( + self, + temp_project: Path, + ) -> None: + """Lockfile dependency roots do not follow symlink package roots.""" + real_pkg = temp_project / "apm_modules" / "owner" / "real-dep" + real_pkg.mkdir(parents=True) + (real_pkg / "apm.yml").write_text("name: real-dep\n", encoding="utf-8") + link_pkg = temp_project / "apm_modules" / "owner" / "link-dep" + try: + link_pkg.symlink_to(real_pkg, target_is_directory=True) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlink unavailable: {exc}") + (temp_project / "apm.lock.yaml").write_text( + "\n".join( + [ + 'lockfile_version: "1"', + "dependencies:", + " - repo_url: owner/link-dep", + "", + ] + ), + encoding="utf-8", + ) + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_dependency_hook_sources_falls_back_when_lockfile_paths_are_invalid( + self, + temp_project: Path, + ) -> None: + """Invalid lockfile paths do not disable bounded fallback discovery.""" + pkg_dir = temp_project / "apm_modules" / "owner" / "dep-hooks" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: dep-hooks\n", encoding="utf-8") + (temp_project / "apm.lock.yaml").write_text( + "\n".join( + [ + 'lockfile_version: "1"', + "dependencies:", + " - repo_url: owner/repo", + " virtual_path: ../bad", + " is_virtual: true", + "", + ] + ), + encoding="utf-8", + ) + + assert HookIntegrator._dependency_hook_sources(temp_project) == {"dep-hooks"} + + def test_bounded_dependency_scan_stops_at_package_root( + self, + temp_project: Path, + ) -> None: + """Nested package content below a package root is not a dependency source.""" + root_info = self._make_root_local_pkg(temp_project, manifest_name="sample-project") + package_root = temp_project / "apm_modules" / "owner" / "repo" + package_root.mkdir(parents=True) + (package_root / "apm.yml").write_text("name: repo\n", encoding="utf-8") + nested_skill = package_root / "tools" / "deep-skill" + nested_skill.mkdir(parents=True) + (nested_skill / "SKILL.md").write_text("# Deep Skill\n", encoding="utf-8") + + settings_path = temp_project / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "deep-skill", + } + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_claude(root_info, temp_project) + + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == ["_local/sample-project"] + + def test_bounded_dependency_scan_ignores_unrecognized_nested_markers( + self, + temp_project: Path, + ) -> None: + """Fallback discovery does not scan arbitrary package internals.""" + nested = ( + temp_project / "apm_modules" / "owner" / "repo" / "tests" / "fixtures" / "dep-hooks" + ) + nested.mkdir(parents=True) + (nested / "SKILL.md").write_text("# Fixture Skill\n", encoding="utf-8") + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_bounded_dependency_scan_rejects_symlinked_namespace( + self, + temp_project: Path, + ) -> None: + """Fallback discovery does not follow namespace symlinks.""" + repo_root = temp_project / "apm_modules" / "owner" / "repo" + repo_root.mkdir(parents=True) + outside = temp_project / "outside" / "dep-hooks" + outside.mkdir(parents=True) + (outside / "SKILL.md").write_text("# External Skill\n", encoding="utf-8") + try: + (repo_root / "collections").symlink_to(outside.parent, target_is_directory=True) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlink unavailable: {exc}") + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_bounded_dependency_scan_rejects_symlinked_marker( + self, + temp_project: Path, + ) -> None: + """Fallback discovery does not accept symlinked package markers.""" + package_root = temp_project / "apm_modules" / "owner" / "repo" / "collections" / "dep-hooks" + package_root.mkdir(parents=True) + outside_hooks = temp_project / "outside" / "hooks" + outside_hooks.mkdir(parents=True) + try: + (package_root / "hooks").symlink_to(outside_hooks, target_is_directory=True) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlink unavailable: {exc}") + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_root_local_source_marker_does_not_collide_with_dependency_name( + self, + temp_project: Path, + ) -> None: + """Root source markers are namespaced even when apm.yml matches a dependency.""" + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo shared"}], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="matching-dep", + hook_data=hook_data, + ) + dep_info = self._make_pkg(temp_project, "matching-dep", {"hooks.json": hook_data}) + + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(dep_info, temp_project) + integrator.integrate_package_hooks_claude(root_info, temp_project) + + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == ["matching-dep", "_local/matching-dep"] + + def test_root_local_heals_stale_source_for_multiple_hook_files_same_event( + self, + temp_project: Path, + ) -> None: + """Stale-source healing applies to every root hook file for an event.""" + first = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo first"}], + } + ] + } + } + second = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo second"}], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="sample-project", + hook_data=first, + ) + (temp_project / ".apm" / "hooks" / "z-second.json").write_text( + json.dumps(second), + encoding="utf-8", + ) + settings_path = temp_project / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo second"}], + "_apm_source": "old-root-name", + } + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_claude(root_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + commands = [e["hooks"][0]["command"] for e in entries if isinstance(e, dict)] + sources = self._claude_sources(temp_project, "PreToolUse") + assert commands == ["echo first", "echo second"] + assert sources == ["_local/sample-project", "_local/sample-project"] + def test_reinstall_clears_aliased_events(self, temp_project: Path) -> None: """Re-integration removes stale postToolUse (camelCase) aliases. diff --git a/tests/unit/test_codex_adapter_compatibility.py b/tests/unit/test_codex_adapter_compatibility.py index 844eeaa33..82bea4cb5 100644 --- a/tests/unit/test_codex_adapter_compatibility.py +++ b/tests/unit/test_codex_adapter_compatibility.py @@ -208,7 +208,11 @@ def test_returns_false_when_server_not_found(self, tmp_path: Path) -> None: def test_returns_false_for_remote_only_server(self, tmp_path: Path) -> None: adapter = _make_adapter(project_root=tmp_path) - server_info = {"remotes": [{"url": "https://example.com/sse"}], "packages": []} + # SSE transport is rejected by Codex (streamable-http only). + server_info = { + "remotes": [{"url": "https://example.com/sse", "transport_type": "sse"}], + "packages": [], + } with patch.object(adapter, "_fetch_server_info", return_value=server_info): result = adapter.configure_mcp_server("owner/remote-server") assert result is False diff --git a/tests/unit/test_codex_adapter_phase3.py b/tests/unit/test_codex_adapter_phase3.py index d243eb723..8c851cb9b 100644 --- a/tests/unit/test_codex_adapter_phase3.py +++ b/tests/unit/test_codex_adapter_phase3.py @@ -208,7 +208,11 @@ def test_returns_false_when_server_not_found(self, tmp_path: Path) -> None: def test_returns_false_for_remote_only_server(self, tmp_path: Path) -> None: adapter = _make_adapter(project_root=tmp_path) - server_info = {"remotes": [{"url": "https://example.com/sse"}], "packages": []} + # SSE transport is rejected by Codex (streamable-http only). + server_info = { + "remotes": [{"url": "https://example.com/sse", "transport_type": "sse"}], + "packages": [], + } with patch.object(adapter, "_fetch_server_info", return_value=server_info): result = adapter.configure_mcp_server("owner/remote-server") assert result is False