feat: apply manifest namespace to local .apm skills#755
feat: apply manifest namespace to local .apm skills#755shreejaykurhade wants to merge 0 commit intomicrosoft:mainfrom
Conversation
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Great first contribution, @shreejaykurhade! Namespace support is a feature our users are actively asking for (#739) and you've covered the core design well -- validation, normalization, test coverage, and thorough doc updates across 5 pages. Solid work.
I'm requesting three focused changes before we can merge. Two are small (~10 lines total) and one is a rebase. Everything else we'll track as follow-up issues that we own -- not your burden.
Blocking (3 items):
-
Rebase onto current
main-- The install engine was refactored while your PR was in flight (#764)._integrate_local_contentnow lives insrc/apm_cli/install/services.py. After rebasing, move yourproject_namespace=parameter to that function. We're happy to help if you get stuck -- just ping us here. -
Narrow the bare
except Exceptioninsync_integration-- see inline comment. Silent fallback toNonecan cause valid namespaced skills to be deleted as "orphans" whenapm.ymlis corrupted or unreadable. (~5 lines) -
Add
validate_path_segments()defence-in-depth inbuild_deployed_skill_name()-- see inline comment. The namespace regex is solid, but our security model requires all user-derived path components to pass throughpath_security.pyas a second layer. (~3 lines)
Nice-to-have (address if energy remains, else we'll follow up):
- Add a one-sentence advisory in
manifest-schema.mdsection 3.3: "Addingnamespaceto an existing project renames deployed skill directories on the nextapm install. Update any hardcoded skill paths accordingly." - Type-check that
namespacefrom YAML is actually a string infrom_apm_yml()(non-string values like lists or booleans will crash the validator)
Follow-ups we'll file ourselves:
- Store
namespacein the lockfile (LockedDependency) so sync cleanup survivesapm_modules/deletion - Add namespace to
apm initscaffold template - Parse-time validation in manifest loading pipeline
- Adversarial test vectors for path traversal edge cases
Naming convention: The dot-separated format (acme.design.skill-name) is the right call -- it's the most natural namespace separator in computing and all major IDEs handle dotted directories without issue. Well chosen.
Review produced with input from the APM Expert Review Panel: Python Architect, CLI Logging Expert, DevX UX Expert, Supply Chain Security Expert, with CEO synthesis.
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Inline comments with code suggestions for the three blocking items above.
| namespace = None | ||
| skill_name, _ = build_deployed_skill_name(raw_name, namespace) | ||
| installed_skill_names.add(skill_name) |
There was a problem hiding this comment.
Narrow this exception handler + emit a diagnostic
A bare except Exception here silently drops the namespace when apm.yml is corrupted, which changes the computed skill name and can cause _clean_orphaned_skills() to delete the correctly-namespaced directory as an "orphan".
Suggested fix:
except (FileNotFoundError, ValueError) as exc:
import logging
logging.getLogger(__name__).debug(
"Could not read namespace from %s: %s", apm_yml_path, exc
)
namespace = NoneAt minimum, don't swallow silently -- log at debug level so users can diagnose with --verbose.
| def _integrate_local_content( | ||
| project_root, | ||
| *, | ||
| project_namespace=None, |
There was a problem hiding this comment.
Heads-up: this function moved during a recent refactor
_integrate_local_content was extracted to src/apm_cli/install/services.py in #764. After rebasing onto current main, apply this project_namespace parameter to the function there instead. The call site that passes project_namespace=apm_package.namespace likely moved too -- check src/apm_cli/install/phases/local_content.py.
Happy to help with the rebase if you need a hand!
| skill_name = f"{normalized_namespace}.{skill_name}" | ||
|
|
||
| return skill_name, " ".join(warnings) if warnings else None | ||
|
|
There was a problem hiding this comment.
Add validate_path_segments() as defence-in-depth
The namespace regex blocks traversal today, but APM's security model (path_security.py) requires all user-derived path components to pass through the centralised guards. One-liner addition before this return:
from apm_cli.utils.path_security import validate_path_segments
validate_path_segments(skill_name, context="deployed skill name")
return skill_name, " ".join(warnings) if warnings else NoneThis ensures any future regression in the regex is caught by the path-level guard (16 other call-sites already follow this pattern).
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Marking as changes requested per the three blocking items in the review above (rebase, narrow exception handler, add path security guard). Looking forward to the updated PR!
Description
This PR completes support for namespaced skill deployment by propagating the root
apm.ymlnamespaceinto local.apm/skillsintegration.The namespace behavior for skill deployment was already implemented; this change makes project-local skills follow the same rule as dependency-installed skills, so local sub-skills are deployed with names like
acme.design.lintinginstead of unprefixed names.This PR also adds regression coverage for the local
.apmpath and updates docs to reflect the expected namespaced skill folder behavior.Fixes #739
Type of change
Testing
Issue-scoped tests passed locally:
tests/unit/test_local_content_install.pytests/unit/integration/test_skill_integrator.pytests/integration/test_local_install.py -k "namespace or root_apm_skills_use_project_namespace"Note:
24 failed, 4453 passed, 160 skipped, 9 deselected, 11 warnings