feat(skills): support package namespaces for skills (+ doc accuracy fixes) - #2649
Open
Sergio Sisternes (sergio-sisternes-epam) wants to merge 21 commits into
Open
feat(skills): support package namespaces for skills (+ doc accuracy fixes)#2649Sergio Sisternes (sergio-sisternes-epam) wants to merge 21 commits into
Sergio Sisternes (sergio-sisternes-epam) wants to merge 21 commits into
Conversation
Add an optional namespace field to apm.yml, deploy package-owned skills and promoted sub-skills under skills/<namespace>/<skill-name>, and persist the namespace in apm.lock.yaml. Also document the namespace schema and add coverage for validation, namespaced deployment, lockfile round-tripping, and lockfile assembly.
…ge-namespaces # Conflicts: # src/apm_cli/install/phases/lockfile.py # src/apm_cli/integration/skill_integrator.py # src/apm_cli/models/apm_package.py
Resolve conflicts: keep namespace fields and modern PEP 604 union syntax; align skill_integrator collision tracking with namespace-aware owner keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_build_ownership_maps now keys by skill identity (path under skills/), not just leaf SKILL.md, so the test fixture's expected key changes from 'SKILL.md' to 'remote-skill/SKILL.md'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
round 2) Folds 8 panel-required findings from the round-2 verdict. Code beneath namespace routing was sound but the feature was invisible at every CLI and docs surface. This commit closes that visibility gap. - CHANGELOG [Unreleased]: add namespace feature entry and BREAKING entry for the removal of .github/copilot-instructions.md generation (findings 3, 7, 8). - skill_integrator: emit verbose_detail per skill when namespace routing changes the deploy path (finding 1). - services (install summary): preserve the namespace segment in the tree-output label and add per-skill confirmation 'skill ns/name integrated -> .github/skills/ns/' so namespaced installs are visible without --verbose (findings 2, 4). - templates/hello-world/apm.yml: commented namespace field for discoverability via 'apm init' (finding 5). - tests/unit/install/test_mcp_warnings.py: restored from origin/main; the file was deleted on this branch with no replacement, regressing SSRF and shell-metachar coverage on the MCP trust surface (finding 6). - migration.md: note that APM no longer writes .github/copilot-instructions.md so the 'leaves untouched' contract remains accurate (finding 8). - packages/apm-guide/.apm/skills/apm-usage/package-authoring.md: document the namespace field and the user-visible install signals (Rule 4 alignment). Regression tests: - verbose log surfaces namespace and is silent when absent - IntegrationResult.target_paths preserves the namespace segment - install tree label preserves namespace and per-skill ns/name label Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address all 3 required findings from PR #1028 round 3 review panel: 1. supply-chain-security: add runtime ensure_path_within containment guard inside _skill_dir (skill_integrator.py:163). Parse-time regex blocks traversal in namespace values, but a symlink planted at skills/<namespace> by a prior malicious install would otherwise let shutil.copytree write outside target_skills_root. Guard covers both namespaced and flat branches at the single chokepoint. 2. cli-logging: normalise capital-S in namespaced per-skill confirmation message (services.py:266) so namespaced and non-namespaced events share identical casing. 3. cli-logging: drop the redundant '(namespace: ...)' suffix from the sub-skills integrated message (services.py:272-274). The target path already encodes the namespace; the parenthetical was pure duplication. Adds TestSkillDirContainmentGuard regression tests (3) exercising the exact symlink-planted attack vector. Full unit suite (6904 tests) green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…kage-namespaces # Conflicts: # CHANGELOG.md
- guides/skills.md: before/after trees showed .github/skills/ (legacy path); change to .agents/skills/ which is the default routing for converged clients (Copilot, Cursor, OpenCode, Codex, Gemini). - package-authoring.md: described nested skills/<namespace>/<skill-name>/ layout but implementation uses flat skills/<namespace>-<skill-name>/ (direct child of skills root). Fix all three references and update the install-output example to match actual format. Addresses blocking findings from APM review panel (PR #1028). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sergio Sisternes (sergio-sisternes-epam)
requested a review
from Daniel Meppiel (danielmeppiel)
as a code owner
August 21, 2026 11:01
Copilot started reviewing on behalf of
Sergio Sisternes (sergio-sisternes-epam)
August 21, 2026 11:02
View session
Sergio Sisternes (sergio-sisternes-epam)
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates APM’s “package namespaces” feature end-to-end (manifest parsing, skill deployment routing, lockfile persistence, install output, and docs) so that namespaced skills deploy as direct children of the target skills/ root using the flat <namespace>-<skill-name> directory format.
Changes:
- Add/propagate
namespace:inapm.ymlthrough package models, install context, lockfile serialization, andapm init --namespace. - Update skill integration to deploy namespaced skills as
skills/<namespace>-<skill-name>/and surface<namespace>/<skill-name>identity in install output. - Update docs/specs/templates and extend unit/integration tests to cover namespace behavior and security guards.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_self_entry_caller_guards.py | Updates ownership-map expectations for deployed skill keying. |
| tests/unit/test_init_command.py | Adds apm init --namespace tests and updates interactive input sequences. |
| tests/unit/integration/test_skill_integrator.py | Adds namespace deployment tests and a containment-guard regression suite. |
| tests/unit/install/test_services.py | Adds install-tree output regression tests for namespaced skill labels. |
| tests/unit/install/test_mcp_warnings.py | Refactors tests (logger setup + comment cleanup) while preserving URL-parse assertions. |
| tests/test_lockfile.py | Adds lockfile namespace round-trip + builder attachment tests. |
| tests/test_apm_package_models.py | Adds manifest parsing/validation tests for namespace:. |
| templates/hello-world/apm.yml | Documents the optional namespace: field in the template. |
| src/apm_cli/models/apm_package.py | Implements validate_namespace() and parses namespace: from apm.yml. |
| src/apm_cli/integration/skill_integrator.py | Routes deployment to namespace-skill dirs, rewrites SKILL frontmatter name, and adjusts ownership key logic. |
| src/apm_cli/install/sources.py | Captures package namespaces into install context for later lockfile assembly. |
| src/apm_cli/install/services.py | Updates install summary labeling/collapse logic to show namespace identity. |
| src/apm_cli/install/phases/lockfile.py | Attaches recorded namespaces into lockfile dependencies during assembly. |
| src/apm_cli/install/context.py | Adds package_namespaces to install context state. |
| src/apm_cli/drift.py | Hardens drift replay override logic with explicit attribute extraction/typing checks. |
| src/apm_cli/deps/lockfile.py | Adds namespace field to LockedDependency with validation-on-load and conditional serialization. |
| src/apm_cli/commands/init.py | Adds --namespace option and writes namespace: to generated apm.yml in --yes mode. |
| src/apm_cli/commands/_helpers.py | Ensures minimal manifest writer includes namespace when present. |
| packages/apm-guide/.apm/skills/apm-usage/package-authoring.md | Documents the flat skills/<namespace>-<skill-name>/ layout and install output format. |
| docs/src/content/docs/reference/manifest-schema.md | Adds namespace to manifest schema docs with pattern/semantics. |
| docs/src/content/docs/reference/lockfile-spec.md | Documents optional namespace in lockfile dependency entries. |
| docs/src/content/docs/reference/cli-commands.md | Documents apm init --namespace (but currently overstates interactive prompting behavior). |
| docs/src/content/docs/guides/skills.md | Adds a “Package Namespaces” section and updates directory layout examples. |
| docs/src/content/docs/getting-started/migration.md | Adds guidance for the unreleased compile --target vscode/all behavior change. |
| CHANGELOG.md | Adds changelog entries for namespaces and a breaking compile output change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+669
to
+672
| target = _skill_dir(target_skills_root, sub_name, namespace) | ||
| deployed_name = _deployed_skill_name(sub_name, namespace) | ||
| owner_key = _skill_owner_key(sub_name, namespace) | ||
| rel_path = f"{rel_prefix}/{owner_key}" |
| owner_key = _skill_owner_key(sub_name, namespace) | ||
| rel_path = f"{rel_prefix}/{owner_key}" | ||
| if target.exists(): | ||
| # Content-identical → skip entirely (no copy, no warning) |
|
|
||
|
|
||
| # ============================================================================= | ||
| # Path-containment guard (PR #1028 round 4 — supply-chain-security finding) |
Comment on lines
68
to
70
| - **Interactive mode**: Prompts for project details unless `--yes` specified | ||
| - **Namespace** (`--namespace`): Writes `namespace:` to `apm.yml`; interactive mode also prompts for it and allows an empty value | ||
| - **Auto-detection**: Automatically detects author from `git config user.name` and description from project context |
Comment on lines
+60
to
+75
| def validate_namespace(namespace: str, *, field_name: str = "namespace") -> str: | ||
| """Return a normalized package namespace or raise ``ValueError``.""" | ||
| if not isinstance(namespace, str): | ||
| raise ValueError( | ||
| f"Invalid '{field_name}' field: expected string, got {type(namespace).__name__}" | ||
| ) | ||
|
|
||
| normalized = namespace.strip() | ||
| if not normalized: | ||
| raise ValueError(f"'{field_name}' must not be empty") | ||
| if "/" in normalized or "\\" in normalized: | ||
| raise ValueError(f"'{field_name}' must be a single path segment") | ||
| if len(normalized) > 64: | ||
| raise ValueError(f"'{field_name}' must be 1-64 characters") | ||
| if "--" in normalized: | ||
| raise ValueError(f"'{field_name}' must not contain consecutive hyphens") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
This PR supersedes #1028. It carries the full namespace feature from
@shreejaykurhadeplus two blocking doc accuracy fixes identified by the advisory panel review.Problem
PR #1028 introduced opt-in package namespaces for skill deployment but had two
blocking-severity doc accuracy errors:docs/src/content/docs/guides/skills.md-- before/after trees and description paragraph referenced.github/skills/(legacy path). The default routing for converged clients is.agents/skills/.packages/apm-guide/.apm/skills/apm-usage/package-authoring.md-- described namespace layout asskills/<namespace>/<skill-name>/(nested subdirectory). The implementation deploys flat:skills/<namespace>-<skill-name>/(direct child ofskills/root).Changes
All changes from #1028 (namespace feature, tests, docs) plus:
docs/src/content/docs/guides/skills.md: updated description paragraph and before/after directory trees to show.agents/skills/as default target.packages/apm-guide/.apm/skills/apm-usage/package-authoring.md: corrected all three path references from nestedskills/<ns>/<name>/to flatskills/<ns>-<name>/; updated install-output example to match actual format.Related
Validation
uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/-- passes clean