Prototype repository-driven OCI package resolution#900
Prototype repository-driven OCI package resolution#900vlsi wants to merge 3 commits intomicrosoft:mainfrom
Conversation
vlsi
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
This PR prototypes a transport-agnostic dependency model where apm.yml can declare logical requirements (for example owner/repo#version) and have them resolved via a client-side repository configuration, with initial Git and OCI backends and lockfile annotations of the resolved source.
Changes:
- Add
PackageRequirementand parse manifest dependency entries into logical requirements when applicable. - Introduce repository configuration (
~/.apm/repositories.yml) plus anArtifactResolverthat tries configured repositories (git/oci) in priority order. - Add an OCI fetch path using
oras pulland basic tarball extraction/validation, plus lockfile fields to record resolved repository/source metadata.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_repository_resolution.py | Adds unit coverage for parsing requirements, default repositories, resolver annotation, and OCI tarball extraction. |
| src/apm_cli/repositories/resolver.py | Implements repository-driven artifact resolution and fetch for git and oci backends. |
| src/apm_cli/repositories/config.py | Adds repository config model, default repositories, and YAML loader for user config. |
| src/apm_cli/models/dependency/requirement.py | Introduces the PackageRequirement model with parsing and install-path computation. |
| src/apm_cli/models/dependency/init.py | Re-exports PackageRequirement alongside existing dependency models. |
| src/apm_cli/models/apm_package.py | Updates manifest parsing to emit PackageRequirement and returns mixed APM dependency types. |
| src/apm_cli/deps/oci_registry.py | Adds OCI pull and tar.gz extraction into an install directory, then validates as an APM package. |
| src/apm_cli/deps/lockfile.py | Extends lockfile schema to store resolved repository/source_type/OCI metadata and alters unique-key behavior for OCI. |
| src/apm_cli/commands/uninstall/engine.py | Extends uninstall parsing to accept PackageRequirement and adjusts some cleanup logic. |
| src/apm_cli/commands/install.py | Integrates ArtifactResolver into install flow and adapts --only parsing and download logic for requirements. |
| name, version = value, None | ||
| if "#" in value: | ||
| name, version = value.rsplit("#", 1) | ||
| version = version.strip() or None | ||
| name = name.strip().strip("/") | ||
| if not name or "/" not in name: | ||
| raise ValueError("Package requirement must be in 'owner/repo' form") | ||
| validate_path_segments(name, context="package name") | ||
| return cls(name=name, version=version) |
There was a problem hiding this comment.
PackageRequirement.parse() accepts strings where the first segment looks like a hostname (contains a '.'), e.g. "gitlab.com/group/repo". Other call sites (install --only parsing, uninstall parsing) attempt PackageRequirement.parse() before DependencyReference.parse(), so host-qualified git dependencies can be misclassified as requirements and end up with incorrect install/uninstall paths. Consider rejecting host-qualified inputs here (e.g., if '.' in first segment) to keep parsing consistent across the CLI.
| for dep_str in updated_data.get("dependencies", {}).get("apm", []) or []: | ||
| try: | ||
| ref = _parse_dependency_entry(dep_str) | ||
| remaining_deps.add(ref.get_unique_key()) | ||
| except (ValueError, TypeError, AttributeError, KeyError): | ||
| remaining_deps.add(dep_str) | ||
| remaining_deps.add(str(dep_str)) |
There was a problem hiding this comment.
Uninstall orphan detection mixes lockfile keys (LockedDependency.get_unique_key()) with manifest keys (ref.get_unique_key()). For OCI deps, the lockfile key becomes "oci::", but PackageRequirement.get_unique_key() returns just "owner/repo". This can cause OCI dependencies to be considered orphans even when still present in apm.yml, leading to incorrect deletions. Use a consistent identity (e.g., compare by dep.repo_url + virtual_path/local_path) or translate manifest requirements into the same lockfile key format before subtracting.
| dep_ref = _parse_dependency_entry(package) | ||
| if not hasattr(dep_ref, "get_install_path"): | ||
| logger.progress(f"Removed {package} from manifest (OCI dependency)") | ||
| removed += 1 | ||
| continue |
There was a problem hiding this comment.
The hasattr(dep_ref, "get_install_path") branch is effectively dead: both DependencyReference and PackageRequirement implement get_install_path(), so this path (and the "OCI dependency" message) is unlikely to be hit and may confuse future maintenance. Prefer explicit isinstance checks if a special case is needed, otherwise remove the branch.
| def repositories_config_path() -> Path: | ||
| """Return the repository config path.""" | ||
| return Path.home() / ".apm" / "repositories.yml" |
There was a problem hiding this comment.
This change introduces a new user-facing dependency form (logical requirements) and a new repositories config file (~/.apm/repositories.yml) with git/oci backends, but no documentation updates are included. Please update the Starlight docs (docs/src/content/docs/) and the apm-guide skill docs (packages/apm-guide/.apm/skills/apm-usage/, especially dependencies.md) to describe the new syntax, resolution order, and config file format.
| """Returns unique key for this dependency.""" | ||
| if self.source_type == "oci": | ||
| registry = self.oci_registry or "default" | ||
| repository = self.oci_repository or self.repo_url | ||
| return f"oci:{registry}:{repository}" |
There was a problem hiding this comment.
LockedDependency.get_unique_key() now returns an "oci::" key for OCI deps. Several existing code paths look up lockfile entries by dep_ref.get_unique_key()/repo_url (e.g., prune/orphan detection uses "owner/repo" keys), so OCI entries will no longer be found and downstream cleanup/lookup logic can silently misbehave. Consider keeping lockfile keys stable (e.g., always key by repo_url/virtual/local identity) and storing OCI transport details in fields, or add an explicit secondary index/lookup that maps repo_url -> lockfile entry for OCI deps.
| """Returns unique key for this dependency.""" | |
| if self.source_type == "oci": | |
| registry = self.oci_registry or "default" | |
| repository = self.oci_repository or self.repo_url | |
| return f"oci:{registry}:{repository}" | |
| """Return the stable lockfile lookup key for this dependency. | |
| The key must remain aligned with dependency reference identity so that | |
| lockfile lookups, prune/orphan detection, and reconciliation logic can | |
| consistently find the same entry across dependency sources. | |
| OCI transport details are stored in dedicated fields on this model and | |
| must not change the canonical lockfile key shape. | |
| """ |
| assert req.resolved_source_type == "git" | ||
| assert req.resolved_repository == "github" | ||
| assert req.resolved_ref.endswith("microsoft/apm-standards.git#v1.2.0") |
There was a problem hiding this comment.
The test asserts on a URL-like string using a substring check (endswith). Our test suite convention (and CodeQL) requires URL assertions to parse the URL with urllib.parse.urlparse and compare specific components (e.g., hostname/path) rather than substring matching, otherwise CI can fail with py/incomplete-url-substring-sanitization.
| with tarfile.open(archive_path, "r:gz") as tar: | ||
| for member in tar.getmembers(): | ||
| if member.name.startswith("/") or ".." in Path(member.name).parts: | ||
| raise RuntimeError( | ||
| f"Refusing to extract unsafe archive entry: {member.name}" | ||
| ) | ||
| if member.issym() or member.islnk(): | ||
| raise RuntimeError( | ||
| f"Refusing to extract symlink/hardlink from OCI archive: {member.name}" | ||
| ) | ||
| if sys.version_info >= (3, 12): | ||
| tar.extractall(target_path, filter="data") | ||
| else: | ||
| tar.extractall(target_path) # noqa: S202 |
There was a problem hiding this comment.
Tar archive traversal protection misses backslash-based paths (e.g. "..\..\x"): Path(member.name).parts will treat backslashes as normal characters on POSIX, so the '..' check won't fire. Normalize member.name by replacing '\' with '/' (or use PurePosixPath) before validating parts, and ensure the final extracted path is contained within target_path (e.g., with ensure_path_within) before extracting.
| try: | ||
| ref = DependencyReference.parse(p) | ||
| ref = PackageRequirement.parse(p) | ||
| only_identities.add(ref.get_identity()) | ||
| except Exception: | ||
| only_identities.add(p) | ||
| try: | ||
| ref = DependencyReference.parse(p) | ||
| only_identities.add(ref.get_identity()) | ||
| except Exception: | ||
| only_identities.add(p) |
There was a problem hiding this comment.
--only parsing tries PackageRequirement.parse() first. PackageRequirement.parse() currently accepts host-qualified strings like "gitlab.com/group/repo", so those will be treated as logical requirements instead of DependencyReference, changing identity matching and potentially selecting the wrong package set. Use the same heuristic as manifest parsing (skip requirements when the first path segment contains a '.') or tighten PackageRequirement.parse() to reject host-qualified inputs so the fallback to DependencyReference.parse() works.
| def _matches_requested_package(dep_ref, only_packages) -> bool: | ||
| """Return True when *dep_ref* matches one of the requested package tokens.""" | ||
| if not only_packages: | ||
| return True | ||
|
|
||
| tokens = { | ||
| dep_ref.get_unique_key(), | ||
| dep_ref.get_display_name(), | ||
| str(dep_ref), | ||
| } | ||
| package = getattr(dep_ref, "package", None) | ||
| if package: | ||
| tokens.add(package) | ||
| tokens.add(package.split("/")[-1]) | ||
| repo_url = getattr(dep_ref, "repo_url", None) | ||
| if repo_url: | ||
| tokens.add(repo_url) | ||
| tokens.add(repo_url.split("/")[-1]) | ||
|
|
||
| return any(pkg in tokens for pkg in only_packages) | ||
|
|
||
|
|
There was a problem hiding this comment.
_matches_requested_package() is newly introduced but appears unused within install.py (no call sites found). If it's not needed yet, removing it avoids dead code; if it is intended for --only filtering, consider wiring it into the dependency selection logic and adding a focused unit test around the matching behavior.
| def _matches_requested_package(dep_ref, only_packages) -> bool: | |
| """Return True when *dep_ref* matches one of the requested package tokens.""" | |
| if not only_packages: | |
| return True | |
| tokens = { | |
| dep_ref.get_unique_key(), | |
| dep_ref.get_display_name(), | |
| str(dep_ref), | |
| } | |
| package = getattr(dep_ref, "package", None) | |
| if package: | |
| tokens.add(package) | |
| tokens.add(package.split("/")[-1]) | |
| repo_url = getattr(dep_ref, "repo_url", None) | |
| if repo_url: | |
| tokens.add(repo_url) | |
| tokens.add(repo_url.split("/")[-1]) | |
| return any(pkg in tokens for pkg in only_packages) |
# Conflicts: # src/apm_cli/commands/install.py # src/apm_cli/deps/lockfile.py # src/apm_cli/models/apm_package.py # src/apm_cli/models/dependency/__init__.py
vlsi
force-pushed
the
feat/repository-driven-oci-resolution
branch
from
cb9db25 to
4d37555
Compare
danielmeppiel
added
panel-review
Python Architect -- PR #900 ReviewArchitectural VerdictThis PR introduces a well-factored new resolution axis (repository-driven OCI + git) with clean value objects ( 1. OO / class diagram (mermaid)classDiagram
direction LR
class DependencyReference {
<<ValueObject>>
+repo_url str
+host str
+reference str
+is_virtual bool
+virtual_path str
+parse(raw) DependencyReference
+get_install_path(dir) Path
+get_unique_key() str
}
class PackageRequirement {
<<ValueObject>>
+name str
+version str
+repository str
+alias str
+resolved_source_type str
+resolved_repository str
+resolved_ref str
+resolved_digest str
+dependency_kind str
+repo_url property
+reference property
+parse(raw) PackageRequirement
+get_install_path(dir) Path
+get_unique_key() str
}
class RepositoryDefinition {
<<ValueObject / frozen>>
+name str
+type str
+base str
+priority int
}
class ResolvedArtifact {
<<ValueObject / frozen>>
+repository_name str
+source_type str
+locator str
+host str
}
class ArtifactResolver {
<<Chain of Responsibility>>
+git_downloader GitHubPackageDownloader
+oci_client OCIRegistryClient
+fetch_requirement(req, path)
-_resolve_artifact(req, repo) ResolvedArtifact
-_fetch_artifact(req, artifact, path)
}
class OCIRegistryClient {
<<Adapter>>
+pull_package(ref, path, req) PackageInfo
-_pull_artifact(ref, dir)
-_locate_package_archive(dir) Path
-_extract_package_archive(arc, path) Path
}
class GitHubPackageDownloader {
+download_package(dep_ref, path) PackageInfo
}
class LockedDependency {
<<ValueObject>>
+repo_url str
+source_type str
+repository_name str
+oci_registry str
+oci_repository str
+oci_tag str
+oci_digest str
+from_dependency_ref(dep_ref, ...) LockedDependency
+to_dependency_ref() DependencyReference
}
class APMPackage {
+dependencies dict
+from_apm_yml(path) APMPackage
+get_apm_dependencies() list
}
ArtifactResolver *-- GitHubPackageDownloader : delegates git
ArtifactResolver *-- OCIRegistryClient : delegates oci
ArtifactResolver ..> RepositoryDefinition : iterates by priority
ArtifactResolver ..> ResolvedArtifact : produces
ArtifactResolver ..> PackageRequirement : reads and mutates
OCIRegistryClient ..> PackageInfo : returns
APMPackage ..> PackageRequirement : parses into
APMPackage ..> DependencyReference : parses into
LockedDependency ..> PackageRequirement : consumes via from_dependency_ref
LockedDependency ..> DependencyReference : consumes and produces
note for ArtifactResolver "Chain of Responsibility:\nrepo1(priority 100) -> repo2(90) -> repo3(80)\nFirst successful fetch wins."
note for PackageRequirement "Mutable: ArtifactResolver sets resolved_* fields in-place"
note for OCIRegistryClient "Adapter: wraps subprocess oras CLI"
class PackageRequirement:::touched
class RepositoryDefinition:::touched
class ResolvedArtifact:::touched
class ArtifactResolver:::touched
class OCIRegistryClient:::touched
class LockedDependency:::touched
class APMPackage:::touched
classDef touched fill:#fff3b0,stroke:#d47600
2. Execution flow diagram (mermaid)flowchart TD
A["User: apm install<br/>(manifest has 'microsoft/apm-standards#v1.2.0')"]
B["APMPackage.from_apm_yml()<br/>src/apm_cli/models/apm_package.py:131"]
C{"_should_parse_as_requirement()<br/>src/apm_cli/models/apm_package.py:317"}
D["PackageRequirement.parse(raw)<br/>src/apm_cli/models/dependency/requirement.py:48"]
E["DependencyReference.parse(raw)<br/>src/apm_cli/models/dependency/reference.py:915"]
F["--- WIRING GAP ---<br/>install pipeline does NOT call ArtifactResolver<br/>src/apm_cli/install/ has no PackageRequirement handling"]
G["ArtifactResolver.fetch_requirement(req, path)<br/>src/apm_cli/repositories/resolver.py:36"]
H["load_repositories()<br/>[I/O] reads ~/.apm/repositories.yml<br/>src/apm_cli/repositories/config.py:34"]
I{"repository.type?"}
J["_resolve_artifact() builds git locator<br/>e.g. https://github.com/microsoft/apm-standards.git#v1.2.0"]
K["_resolve_artifact() builds OCI locator<br/>e.g. ghcr.io/apm/acme/security-pack:1.0.0"]
L["[NET] git_downloader.download_package(dep_ref, path)<br/>src/apm_cli/deps/github_downloader.py:179"]
M["[EXEC][NET] oci_client._pull_artifact(ref, dir)<br/>subprocess: oras pull<br/>src/apm_cli/deps/oci_registry.py:91"]
N["[FS] _extract_package_archive(arc, path)<br/>tarfile.extractall<br/>src/apm_cli/deps/oci_registry.py:121"]
O["validate_apm_package(root)<br/>src/apm_cli/models/validation.py:251"]
P["requirement.resolved_source_type = 'git' or 'oci'<br/>(in-place mutation)<br/>src/apm_cli/repositories/resolver.py:98-109"]
Q["LockedDependency.from_dependency_ref(req, ...)<br/>fast-path: dependency_kind == 'package_requirement'<br/>src/apm_cli/deps/lockfile.py:208"]
R["[FS] lockfile.save()<br/>writes apm.lock.yaml"]
A --> B
B --> C
C -- "no '://', no 'git@', no '//',<br/>no '.' in first segment" --> D
C -- "otherwise" --> E
D --> F
E --> F
F -. "prototype: not wired yet" .-> G
G --> H
H --> I
I -- "git" --> J
I -- "oci" --> K
J --> L
K --> M
M --> N
N --> O
L --> P
O --> P
P --> Q
Q --> R
style F fill:#ffcccc,stroke:#cc0000
3. Design patternsDesign patterns
4. Specific IssuesBLOCKING
NON-BLOCKING (should fix before GA, acceptable in prototype)
|
CLI Logging / Output UX ReviewReviewed from the perspective of: Does the new code give humans (default) and agents ( Overall the prototype is well-structured, but its output story has significant gaps. The three new library modules ( [F1] MEDIUM -- OCI pull is a black hole for output
Compare with the established pattern in Suggest: Accept an optional [F2] MEDIUM -- Resolver swallows per-repository failures silently
try:
artifact = self._resolve_artifact(requirement, repository)
return self._fetch_artifact(requirement, artifact, target_path)
except Exception as exc:
last_error = exc
continue
```
When a requirement is tried against multiple repositories, each failed attempt is silently discarded. Only the **last** error survives in the final `RuntimeError`. This violates the progressive-disclosure rule -- in `--verbose` mode, the user (or agent) should see:
```
[i] Trying repository 'github' for owner/repo...
[!] github: connection refused
[i] Trying repository 'ghcr' for owner/repo...Without this, debugging "why did resolution fail?" requires guesswork. Suggest: Accept an optional [F3] HIGH -- Silent config fallback hides broken user config
try:
raw = yaml.safe_load(path.read_text(encoding="utf-8")) or {}
except Exception:
return list(DEFAULT_REPOSITORIES)If a user writes Similarly, lines 46-47 silently fall back if Suggest: This is a pure-data loader, so a full CommandLogger dependency is heavy. Instead, consider either:
[F4] LOW -- "Removed from manifest (OCI dependency)" is misleading
if not hasattr(dep_ref, "get_install_path"):
logger.progress(f"Removed {package} from manifest (OCI dependency)")
removed += 1
continueTwo issues:
Suggest: Reword to something like [F5] INFO -- No install-pipeline integration means no progress story yet
This is not a blocker -- just a reminder that the logging contract is incomplete until the install integration lands. Summary
F3 is the highest-priority fix because it violates a core principle: if the user wrote a config file and it is being ignored, they must be told. The rest are prototype gaps that should be resolved before the feature exits prototype status.
|
Supply Chain Security -- Threat Model AssessmentPR #900 introduces a repository-driven resolution layer with OCI transport support. This review evaluates the new attack surfaces against APM's documented security model ( Verdict: HOLD -- 2 critical, 3 high, 3 medium findings must be resolved before merge. 1. Tarball Extraction Safety (tarslip, symlink, hardlink)Severity: HIGH The pre-validation loop at lines 124-132 checks for absolute paths, # Lines 124-136
for member in tar.getmembers(): # CHECK
if member.name.startswith("/") or ".." in Path(member.name).parts:
raise RuntimeError(...)
if member.issym() or member.islnk():
raise RuntimeError(...)
if sys.version_info >= (3, 12):
tar.extractall(target_path, filter="data") # SAFE
else:
tar.extractall(target_path) # USE -- no filter, no members= restrictionIssue A -- Missing safe_members = []
for member in tar.getmembers():
# ... validate ...
safe_members.append(member)
tar.extractall(target_path, members=safe_members)Issue B -- ad-hoc Issue C -- No size/count limits. A zip-bomb-style tarball with many entries or enormous files would exhaust disk. Consider adding max-entry-count and max-total-bytes guards. Recommendation:
2. Locator Injection via name / version / baseSeverity: MEDIUM Locators are built by string interpolation: # git:
locator = f"{repository.base.rstrip('/')}/{requirement.name}.git"
# oci:
locator = f"{repository.base.rstrip('/')}/{requirement.name}:{requirement.version}"2a -- 2b -- locator = f".../{requirement.name}:{requirement.version}"An OCI tag is syntactically constrained by the OCI spec to Recommendation: Add an OCI-tag-format validator on the 2c -- 3. User-Controlled Repository Config as Attack SurfaceSeverity: MEDIUM
3a -- No URL scheme validation on
3b -- Config file is user-scoped ( 3c -- Priority ordering is user-controlled. A hostile config could set a malicious OCI registry to priority 999, overriding the built-in GitHub source. Combined with the fallback behavior in Recommendation:
4. OCI Auth Gap and Credential ManagementSeverity: CRITICAL The OCI path completely bypasses APM's proc = subprocess.run(
["oras", "pull", resolved_reference, "--output", str(output_dir)],
capture_output=True, text=True, check=False,
)This means:
This is the most significant security gap in this PR. The security model ( Recommendation:
5. Content Integrity for OCI PackagesSeverity: CRITICAL 5a -- No content_hash computation for OCI packages. For git packages, This means subsequent installs cannot verify cached OCI packages against the lockfile. A tampered cache will be silently accepted. This directly violates the lockfile spec section 4.4: "On subsequent installs, cached packages are verified against this hash." 5b -- Even if it were populated, there is no code path that verifies 5c -- No The lockfile spec's reproducibility guarantee -- "the same lock file yields the same deployed files on every machine, every time" -- is not met for OCI packages. Recommendation:
6. Dependency Confusion via Multi-Backend ResolutionSeverity: HIGH The default repositories are: DEFAULT_REPOSITORIES = [
RepositoryDefinition(name="github", type="git", base="https://github.com", priority=100),
RepositoryDefinition(name="gitlab", type="git", base="(gitlab.com/redacted)", priority=90),
RepositoryDefinition(name="ghcr", type="oci", base="ghcr.io/apm", priority=80),
]The resolver tries each repository in priority order and falls back on failure (resolver.py:39-47): for repository in load_repositories():
try:
artifact = self._resolve_artifact(requirement, repository)
return self._fetch_artifact(requirement, artifact, target_path)
except Exception as exc:
last_error = exc
continue # try next repo6a -- Silent fallback enables dependency confusion. If 6b -- The ghcr default namespace 6c -- No repository pinning in the lockfile. Once a package is resolved, the Recommendation:
7. Subprocess Invocation SafetySeverity: LOW proc = subprocess.run(
["oras", "pull", resolved_reference, "--output", str(output_dir)],
capture_output=True, text=True, check=False,
)7a -- List-form subprocess is safe against shell injection. Using 7b -- Argument injection via ["oras", "pull", "--", resolved_reference, "--output", str(output_dir)]7c -- No timeout. The subprocess has no timeout. A malicious or misconfigured registry could hang the connection indefinitely, causing 7d -- Error output could leak registry URLs or paths. The stderr is included in the RuntimeError at line 104: raise RuntimeError(f"Failed to pull OCI artifact {resolved_reference}: {stderr}")The Recommendation:
8. Security Model Documentation DriftSeverity: HIGH The security model currently states:
And:
And in the attack surface table:
This PR introduces OCI registry support, which directly contradicts all three statements. The security model document MUST be updated in the same PR to reflect:
Per the rule in the supply-chain security expert charter: "If a code change weakens or contradicts any guarantee in Summary Table
Blocking items for merge: Findings 1, 4, 5, 6, and 8 represent security regressions against APM's documented guarantees. Finding 5 (no content integrity for OCI) is particularly concerning because it silently breaks the lockfile reproducibility contract that enterprise users depend on. I recognize this is labeled as a prototype. If the intent is to merge behind a feature flag or
|
Auth Expert Review -- PR #900: OCI Package ResolutionScope: Authentication architecture, credential flow, token leakage, host classification. Finding 1 -- MEDIUM:
|
| # | Severity | Finding |
|---|---|---|
| 1 | MEDIUM | subprocess.run leaks full env (incl. APM tokens) to oras |
| 2 | HIGH | No credential forwarding to oras for private OCI registries |
| 3 | MEDIUM | OCI artifacts have no host field, invisible to host classification |
| 4 | LOW | No URL scheme validation on repositories.yml base |
| 5 | LOW | OCI error messages lack build_error_context() auth guidance |
| 6 | LOW | GitHubPackageDownloader naming misleading for multi-host use |
Verdict: Finding 2 is the critical feature gap -- private ghcr.io will silently fail. Finding 1 is the auth-safety concern that should be fixed before merge (env sanitization is a small change). Findings 3-5 are reasonable to track as follow-up issues if this is explicitly a prototype/RFC PR. Finding 6 is cosmetic.
This is NOT a security regression -- no existing credentials are sent where they should not go. But the env inheritance in Finding 1 creates a latent leakage vector that should be closed proactively, and Finding 2 means the OCI path is effectively public-only until auth integration is added.
Generated by PR Review Panel for issue #900 · ● 29.6M · ◷
OSS Growth Assessment -- PR #900: Repository-Driven OCI Package ResolutionReviewer: OSS Growth Hacker 1. Growth/Adoption Implications of Shipping This PrototypeThe upside is real. OCI registry support is the single most requested capability for enterprise adoption of any package manager. It answers the question every platform engineering team asks in the first 10 minutes of evaluation: "Can I host packages in our own registry?" Before this PR, the answer was "git repos only." After this PR, the answer is "yes, OCI registries too -- Harbor, Artifactory, GHCR, whatever you run." That is a conversion unlock for the enterprise segment. The However, the prototype ships documentation for features that do not work end-to-end. The Specific concern: The quickstart example Recommendation: Either (a) gate the OCI/logical-requirement docs behind an 2. Disambiguation / Mental Model Change RiskThe parsing change is the most growth-sensitive part of this PR. Before this PR, For the default case, this is invisible -- the But the mental model shift matters for docs and messaging. The term "logical requirement" is new jargon. The dependencies guide now distinguishes "logical requirement strings" from "direct source strings" -- and the distinguishing heuristic (does the first segment contain a dot?) is not intuitive. Recommendation: The docs should lead with the simple case first: "Most packages are just 3. External Contributor Momentum vs. Incomplete FeaturesThis is the highest-signal external contribution APM has received. Vladimir is building enterprise-grade infrastructure, not filing typo fixes. Shipping this with appropriate scoping (experimental flag, clear prototype language) sends the message that APM takes external contributors seriously and merges real work. The risk of NOT merging (or stalling) is higher than the risk of merging incomplete. A motivated external contributor who sees their PR languish will not submit a second one. The OSS contributor funnel is: drive-by issue -> small PR -> substantial PR -> repeat contributor. Vladimir is at stage 3. Protect that. However, "prototype" language in shipped docs is a growth anti-pattern. Five instances of "prototype" and "consume-only" in the docs undermine confidence. Users do not read caveats -- they read the first example, try it, and form an opinion. Recommendation: Merge the implementation and tests. For the docs, replace "prototype" with "preview" (same honesty, less tentativeness) and add explicit scope markers:
4. Side-Channel to CEO: Strategic Implications of OCI SupportFor the CEO's consideration:
5. Growth Strategy UpdateYes, WIP/growth-strategy.md update is warranted. Key insight to add:
Summary of Actionable Items
Bottom line: This PR has high strategic value and should be merged, but the docs-to-implementation gap is a conversion risk that must be closed before it ships to users. The fastest path is gating the OCI-specific docs behind the existing experimental feature-flag system.
|
DevX UX Review -- PR #900: Repository-Driven OCI Package ResolutionAssessment SummaryThis PR introduces a significant new concept (logical package requirements + repository-driven resolution + OCI transport) into a tool whose UX contract is "install works like npm". The architectural direction is sound -- separating package identity from transport is what every mature package manager does. However, the current prototype has several user-facing gaps that range from a silent breaking regression to missing discoverability surfaces. I'm focusing strictly on UX; auth and supply-chain concerns are deferred to those specialist reviewers. HIGH severityH1. Virtual-path references silently break (regression)Files: The disambiguation heuristic checks whether the first path segment contains a dot. If it doesn't, the string is routed to This breaks every GitHub shorthand that contains a virtual path (subdirectory, file, or collection): Before this PR: After this PR: This is a silent regression that breaks the quick-start example ( Suggested fix direction: Count path segments before the H2. Install pipeline is not wired -- user hits a wallFiles: The
This means Recommendation: Either (a) wire up the install pipeline so it detects MEDIUM severityM1.
|
| # | Severity | Action |
|---|---|---|
| H1 | HIGH | Fix _should_parse_as_requirement to exclude 3+ segment paths (virtual packages) |
| H2 | HIGH | Wire install pipeline OR add clear early error for PackageRequirement |
| M1 | MEDIUM | Update cli-commands.md in this PR |
| M2 | MEDIUM | Warn on malformed repositories.yml instead of silent fallback |
| M3 | MEDIUM | Add way to inspect active repositories (apm config or new subcommand) |
| M4 | MEDIUM | Add install instructions to oras missing error |
| M5 | MEDIUM | Show tried repositories and per-repo errors on resolution failure |
| M6 | MEDIUM | Add changelog/migration note about semantic change |
| L1-L6 | LOW | Address as polish |
Generated by PR Review Panel for issue #900 · ● 29.6M · ◷
|
@microsoft-github-policy-service agree |
Prototype repository-driven package resolution for APM dependencies.
This changes dependency handling so
apm.ymlcan declare logical package requirements likeowner/repo#versionwithout embedding the transport type in the dependency itself. Resolution is performed through configured repositories, with initial support for Git and OCI backends.How it works
*.tar.gzartifact with raw APM package sources.apm.ymldo not depend on the repository that stores the dependency.This means the same package declaration can be resolved from different repositories based on client configuration.
apm.ymlcan be used unchanged in open source and in corporate environments.Notes
This is still a prototype.
Current limitations:
orasType of change
Testing