feat: support local filesystem path dependencies#270
Conversation
… pack guard - DependencyReference: is_local_path(), parse() for local paths, get_install_path() under _local/ prefix - LockedDependency: source/local_path fields, from_dependency_ref() support - Install command: _copy_local_package() helper, local path handling in download_callback and sequential loop - Pack command: guard rejecting local-path dependencies - Security: exclude protocol-relative URLs from local path detection - Fix existing tests for new local path behavior Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
- 47 unit tests for DependencyReference, LockedDependency, APMPackage, pack guard, _copy_local_package - 11 integration tests for full install/uninstall/deps/pack workflow with local paths - Fix pack guard to properly re-raise ValueError (not swallowed by FileNotFoundError handler) Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
- dependencies.md: add Local Path Dependencies section, update types table and examples - cli-commands.md: update install command arguments and examples - lockfile-spec.md: add source and local_path fields - manifest-schema.md: update ABNF grammar and examples - pack-distribute.md: add pack guard prerequisite Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
- Remove verbose docstring block in test file - Replace _L-prefixed aliases with clear names (local_pkg, local_ref, etc.) - Rename dep_deployed_local to dep_deployed_files for consistency Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
…nitized cache name) Use environment marker python_version>='3.10' since black 26.3.1 requires Python 3.10+ and the project supports Python 3.9+. Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Regenerated uv.lock after merging main to incorporate pyproject.toml changes from both branches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds support for installing APM dependencies from local filesystem paths (in addition to existing git-based dependencies), and updates lockfile/schema/docs/tests to reflect and validate the new behavior.
Changes:
- Extend
DependencyReference+ lockfile modeling to represent local path dependencies (source: local,local_path), including parsing and canonicalization rules. - Update install flow to copy local packages into
apm_modules/_local/…and reject local-path dependencies duringapm pack. - Add/adjust unit + integration tests and update reference docs to include local path dependency syntax.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/models/dependency.py |
Adds local-path detection/parsing and routes local deps to _local/ install paths. |
src/apm_cli/deps/lockfile.py |
Extends lockfile entries with source/local_path and uses local unique keys for local deps. |
src/apm_cli/commands/install.py |
Validates/copies local packages during install; integrates local packages into existing install pipeline. |
src/apm_cli/bundle/packer.py |
Adds guard to reject packing when apm.yml contains local path dependencies. |
tests/unit/test_local_deps.py |
New unit coverage for local dep parsing, lockfile serialization, and copy helper. |
tests/integration/test_local_install.py |
New end-to-end CLI workflow tests for installing/uninstalling local deps. |
tests/unit/test_auth_scoping.py |
Updates error expectations due to new {path: ...} behavior. |
tests/test_lockfile.py |
Updates lockfile-related test setup to include new DependencyReference fields. |
tests/test_apm_package_models.py |
Updates parsing expectations (absolute paths now treated as local). |
docs/src/content/docs/reference/manifest-schema.md |
Documents local path form in dependency grammar and examples. |
docs/src/content/docs/reference/lockfile-spec.md |
Documents lockfile representation for local deps (source, local_path). |
docs/src/content/docs/reference/cli-commands.md |
Documents apm install accepting local filesystem paths. |
docs/src/content/docs/guides/dependencies.md |
Adds local path dependency guidance and examples. |
docs/src/content/docs/guides/pack-distribute.md |
Documents pack restriction for local deps. |
pyproject.toml / uv.lock |
Bumps dev black constraint per security note in PR description. |
Comments suppressed due to low confidence (1)
src/apm_cli/models/dependency.py:291
- Using
Path(self.local_path).namefor the_local/install folder can cause collisions: two different local dependencies with the same basename (e.g.../pkg/skillsand./vendor/skills) will have different unique keys (local_path) but will copy into the sameapm_modules/_local/skillsdirectory and clobber each other. To make local deps composable, consider incorporating a stable disambiguator (e.g., short hash of the normalized absolute path) into the install directory name.
if self.is_local and self.local_path:
pkg_dir_name = Path(self.local_path).name
return apm_modules_dir / "_local" / pkg_dir_name
| if self.is_local and self.local_path: | ||
| pkg_dir_name = Path(self.local_path).name | ||
| return apm_modules_dir / "_local" / pkg_dir_name |
There was a problem hiding this comment.
DependencyReference.get_install_path() derives the local install directory from Path(self.local_path).name without sanitizing it. If a user passes a local path whose basename is ../. or empty (e.g. ../ or /), this produces an install path like apm_modules/_local/.. which can resolve outside _local/ and lead to deleting/copying into unintended locations during install. Consider validating the derived name (reject '', '.', '..', path separators) and/or using a safe deterministic encoding (e.g., hash of the resolved absolute path) for the _local folder name.
This issue also appears on line 288 of the same file.
| def _validate_package_exists(package): | ||
| """Validate that a package exists and is accessible on GitHub or Azure DevOps.""" | ||
| """Validate that a package exists and is accessible on GitHub, Azure DevOps, or locally.""" | ||
| import os | ||
| import subprocess | ||
| import tempfile |
There was a problem hiding this comment.
_validate_and_add_packages_to_apm_yml() still has a pre-check that rejects any package string without a forward slash (/). That will incorrectly reject valid local paths on Windows such as .\packages\my-pkg or ~\repos\my-pkg, even though DependencyReference.is_local_path() now supports them. Consider detecting DependencyReference.is_local_path(package) before the '/' not in package guard (or relaxing that guard) so local paths are accepted cross-platform.
src/apm_cli/commands/install.py
Outdated
| if integrate_vscode or integrate_claude: | ||
| try: | ||
| prompt_result = prompt_integrator.integrate_package_prompts( | ||
| package_info, project_root, | ||
| force=force, managed_files=managed_files, | ||
| diagnostics=diagnostics, |
There was a problem hiding this comment.
The local-dependency branch duplicates a large integration block (prompts/agents/skills/instructions/commands/hooks) that already exists for cached and freshly-downloaded packages later in this function. This increases the risk of future drift (e.g., new integration steps added in one path but not the others). Consider extracting the shared integration sequence into a helper (e.g., _integrate_package(package_info, ...)) so local/cached/downloaded paths all call the same implementation.
| dependency = url_form / shorthand_form / local_path_form | ||
| url_form = ("https://" / "http://" / "ssh://git@" / "git@") clone-url | ||
| shorthand_form = [host "/"] owner "/" repo ["/" virtual_path] ["#" ref] ["@" alias] | ||
| local_path_form = ("./" / "../" / "/") path |
There was a problem hiding this comment.
The manifest schema’s local_path_form only lists ./, ../, and /, but the implementation also treats ~/ (and some Windows-style prefixes like .\\ / ..\\ / ~\\) as local paths. The docs should either include these additional accepted forms or the parser should be narrowed to match the documented grammar.
| local_path_form = ("./" / "../" / "/") path | |
| local_path_form = ("./" / "../" / "/" / "~/" / ".\\" / "..\\" / "~\\") path |
|
|
||
| **Arguments:** | ||
| - `PACKAGES` - Optional APM packages to add and install. Accepts shorthand (`owner/repo`), HTTPS URLs, SSH URLs, or FQDN shorthand (`host/owner/repo`). All forms are normalized to canonical format in `apm.yml`. | ||
| - `PACKAGES` - Optional APM packages to add and install. Accepts shorthand (`owner/repo`), HTTPS URLs, SSH URLs, FQDN shorthand (`host/owner/repo`), or local filesystem paths (`./path`, `../path`, `/absolute/path`). All forms are normalized to canonical format in `apm.yml`. |
There was a problem hiding this comment.
The apm install docs list local filesystem paths as ./path, ../path, and /absolute/path, but the implementation (and PR description) also supports ~/... via expanduser(). Consider updating this argument description (and examples) to include ~/path so the documented surface matches actual behavior.
| - `PACKAGES` - Optional APM packages to add and install. Accepts shorthand (`owner/repo`), HTTPS URLs, SSH URLs, FQDN shorthand (`host/owner/repo`), or local filesystem paths (`./path`, `../path`, `/absolute/path`). All forms are normalized to canonical format in `apm.yml`. | |
| - `PACKAGES` - Optional APM packages to add and install. Accepts shorthand (`owner/repo`), HTTPS URLs, SSH URLs, FQDN shorthand (`host/owner/repo`), or local filesystem paths (`./path`, `../path`, `/absolute/path`, `~/path`). All forms are normalized to canonical format in `apm.yml`. |
src/apm_cli/bundle/packer.py
Outdated
| except (FileNotFoundError, ValueError): | ||
|
|
||
| # Guard: reject local-path dependencies (non-portable) | ||
| from ..models.apm_package import DependencyReference |
There was a problem hiding this comment.
There’s a function-local import of DependencyReference here that isn’t used (the loop already iterates DependencyReference objects from package.get_apm_dependencies()). Removing the unused import will keep the packer module cleaner and avoid lint warnings in environments that enforce unused-import checks.
| from ..models.apm_package import DependencyReference |
src/apm_cli/commands/install.py
Outdated
| shutil.copytree(local, install_path, dirs_exist_ok=False) | ||
| return install_path |
There was a problem hiding this comment.
shutil.copytree(local, install_path) uses the default symlinks=False, which follows symlinks and copies the symlink targets. A local package containing a symlink that points outside the package directory could cause unintended files to be copied into apm_modules/ (and potentially later integrated into .github//.claude/). Consider either preserving symlinks (symlinks=True) and/or validating that any symlinks stay within the local package root before copying.
| # Support dict-form local path: { path: ./local/dir } | ||
| if 'path' in entry and 'git' not in entry: | ||
| local = entry['path'] | ||
| if not isinstance(local, str) or not local.strip(): | ||
| raise ValueError("'path' field must be a non-empty string") | ||
| local = local.strip() | ||
| if not cls.is_local_path(local): | ||
| raise ValueError( | ||
| f"Object-style dependency must have a 'git' field, " | ||
| f"or 'path' must be a local filesystem path " | ||
| f"(starting with './', '../', '/', or '~')" | ||
| ) | ||
| return cls.parse(local) | ||
|
|
||
| if 'git' not in entry: | ||
| raise ValueError("Object-style dependency must have a 'git' field") | ||
| raise ValueError("Object-style dependency must have a 'git' or 'path' field") |
There was a problem hiding this comment.
parse_from_dict() now accepts object-style local dependencies ({path: ./local/dir}) when git is absent, but the manifest schema docs currently describe object form as requiring git. Either document this additional object form in the manifest schema, or consider rejecting/ignoring {path: ...} without git to keep the schema and parser aligned.
| | `repo_url` | string | MUST | Source repository URL, or `_local/<name>` for local path dependencies. | | ||
| | `host` | string | MAY | Git host identifier (e.g., `github.com`). Omitted when inferrable from `repo_url`. | | ||
| | `resolved_commit` | string | MUST | Full 40-character commit SHA that was checked out. | | ||
| | `resolved_ref` | string | MUST | Git ref (tag, branch, SHA) that resolved to `resolved_commit`. | | ||
| | `resolved_commit` | string | MUST | Full 40-character commit SHA that was checked out. Absent for local dependencies. | | ||
| | `resolved_ref` | string | MUST | Git ref (tag, branch, SHA) that resolved to `resolved_commit`. Absent for local dependencies. | |
There was a problem hiding this comment.
In the lockfile spec table, resolved_commit and resolved_ref are still marked as MUST, but the description says they are absent for local dependencies. This is internally inconsistent and will confuse implementers; consider making these conditional (e.g., MUST for source omitted/remote deps, MAY/absent for source: local). Also, because local deps are keyed by local_path in code, the “Unique Key” section later in this doc likely needs an update to reflect that local deps are not uniquely identified by repo_url alone.
See below for a potential fix:
`repo_url` (lexicographic). For local path dependencies where `repo_url` is
`_local/<name>`, tie-breaking SHOULD use `local_path` (lexicographic). Each entry is a YAML mapping with the following
| Field | Type | Required | Description |
|-------|------|----------|-------------|
| `repo_url` | string | MUST | Source repository URL, or `_local/<name>` for local path dependencies. |
| `host` | string | MAY | Git host identifier (e.g., `github.com`). Omitted when inferrable from `repo_url`. |
| `resolved_commit` | string | MUST (remote) | Full 40-character commit SHA that was checked out. Required for remote (git) dependencies; MUST be omitted for local (`source: "local"`) dependencies. |
| `resolved_ref` | string | MUST (remote) | Git ref (tag, branch, SHA) that resolved to `resolved_commit`. Required for remote (git) dependencies; MUST be omitted for local (`source: "local"`) dependencies. |
| # Create a lockfile | ||
| lock_data = { | ||
| "lockfile_version": "1", | ||
| "dependencies": { | ||
| "../packages/local-skills": { | ||
| "repo_url": "_local/local-skills", | ||
| "source": "local", | ||
| "local_path": "../packages/local-skills", | ||
| }, | ||
| }, | ||
| } | ||
| with open(consumer / "apm.lock", "w") as f: | ||
| yaml.dump(lock_data, f) | ||
|
|
There was a problem hiding this comment.
This test writes apm.lock with dependencies as a mapping, but the current LockFile format expects dependencies to be a list of dependency objects. With the mapping form, LockFile.read() will fail and apm pack will error out before reaching the “reject local deps” guard, so the test doesn’t reliably cover the intended behavior. Consider generating a valid lockfile via LockFile().write(...) (or by running apm install in the test setup) and then asserting on the specific error message about local path dependencies.
|
@copilot assess the PR review comments - are they legit? are they proven? are they pointing to any deeper issue with these changes, or things we may have overlooked? Eventually, if they're legit and proven and solid - fix them. In addition, if this reflection helps you find related issues, report them after proving them and if proven, fix them too. |
Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Security fixes:
- Validate local path basename: reject '', '.', '..' to prevent path
traversal via apm_modules/_local/.. (get_install_path + parse)
- Use symlinks=True in copytree to preserve symlinks instead of
following them outside the package boundary
- Skip slash-only guard for local paths (Windows backslash compat)
Code quality:
- Extract _integrate_package_primitives() helper to eliminate
duplicated 100-line integration block across local/cached/download paths
- Remove unused DependencyReference import in packer.py
Documentation:
- Add ~/ to local_path_form ABNF grammar (manifest-schema.md)
- Add ~/path to install CLI argument description (cli-commands.md)
- Make resolved_commit/resolved_ref conditional MUST(remote) in lockfile spec
- Update lockfile unique key section to mention local_path for local deps
- Document {path: ./local} object form in manifest schema
Tests:
- Add path traversal rejection tests (../, ./, /, ..)
- Add symlink preservation test for _copy_local_package
- Fix pack integration test to use LockFile API instead of raw dict
Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
…link test assertion Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Assessed all 10 review comments — every one is legitimate and proven. Fixed in Security (proven & fixed):
Code quality (fixed):
Documentation (all fixed):
Tests (fixed):
|
|
@copilot resolve the conflicts wisely and carefully, then push and explain the rationale |
…ring) Main branch (PR #227, Windows native support) standardized docstring arrows from '→' to '->' across the codebase. Our branch had added a local path line using '→'. Resolution: adopt main's '->' style for all arrows and keep our local path documentation line. The only conflict was in to_canonical() docstring in dependency.py. All other files auto-merged cleanly. Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Resolved in What conflicted: Main (PR #227, Windows native support) standardized docstring arrows from Resolution rationale: Adopted main's All other files (85 total from Windows native support) auto-merged cleanly. Verified: 63 local deps tests pass, 1953 unit tests pass (1 pre-existing failure in |
|
@copilot resolve merge conflicts. This branch has conflicts that must be resolved README.md |
…ation) Three files conflicted with main's PR #278 (Windows documentation normalization): 1. README.md — Adopted main's simplified `irm ... | iex` PowerShell command (commit 8205b49 removed the `powershell -ExecutionPolicy Bypass -c` wrapper). 2. docs/src/content/docs/getting-started/installation.md — Adopted main's restructured install page: Windows in description/requirements, clean section headings, simplified irm command, Homebrew section, scoped troubleshooting to macOS/Linux, narrowed manual binary table. 3. src/apm_cli/models/dependency.py — No change needed on our side; the docstring line `- Local paths are returned as-is -> ./packages/my-pkg` was already present from our previous merge resolution. Main simply doesn't have this line (it's our new feature documentation). Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Resolve single conflict in dependency.py: keep local-path docstring line with main's -> arrow style.
Resolved in Only one real conflict existed —
|
Description
Adds support for installing APM packages from local filesystem paths, enabling fast inner-loop iteration during package authoring, monorepo workflows, and offline development.
Model layer
DependencyReference.parse()detects local paths (./,../,/,~/) while rejecting protocol-relative URLs (//host/path)../,./,/) whose basename resolves to empty/./..to prevent escape fromapm_modules/_local/is_local_path()predicate,get_install_path()routes locals under_local/prefix inapm_modules/parse_from_dict()supports{path: ./local/dir}object form (withoutgit) for local depsLockedDependencygainssource(github|local) andlocal_pathfields; local deps store relative paths and omitresolved_commit/resolved_refInstall / uninstall
_copy_local_package()copies package contents viashutil.copytreewithsymlinks=True(preserves symlinks instead of following them outside the package boundary), validatesapm.yml/SKILL.mdpresence_integrate_package_primitives()extracted as shared helper for prompts/agents/skills/instructions/commands/hooks integration — used by local, cached, and download code paths to eliminate duplicationapm install(no SHA to short-circuit against).\\,..\\,~\\) accepted cross-platformDependencyReference.parse()routes correctlyPack guard
apm packrejects packages containing local-path dependencies with a clear error messageSecurity fix
black>=26.3.1(withpython_version>='3.10'marker) to address CVE for arbitrary file writes from unsanitized cache file namesDocumentation
~/to ABNF grammar in manifest schema and~/pathto CLI argument docsresolved_commit/resolved_refconditionalMUST (remote)in lockfile speclocal_pathfor local deps{path: ./local}object form in manifest schema §4.1.2Type of change
Testing
52 unit tests covering
DependencyReferenceparsing (including path traversal rejection for../,./,/,..),LockedDependencyserialization,APMPackageloading, pack guard,_copy_local_package, and symlink preservation. 11 integration tests covering full install/uninstall/deps/pack workflows with local paths, including edge cases (missing manifest, missing path, mixed local+remote deps, reinstall-from-lockfile).📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.