Skip to content

Fix apm pack omitting version for INTERNAL/private github.com repos#1854

Merged
danielmeppiel merged 3 commits into
mainfrom
sergio-sisternes-epam-fix-pack-private-repo-metadata-fallback
Jun 20, 2026
Merged

Fix apm pack omitting version for INTERNAL/private github.com repos#1854
danielmeppiel merged 3 commits into
mainfrom
sergio-sisternes-epam-fix-pack-private-repo-metadata-fallback

Conversation

@sergio-sisternes-epam

Copy link
Copy Markdown
Collaborator

Description

apm pack silently drops the per-plugin version field from generated marketplace.json when marketplace packages reference INTERNAL or private repositories on github.com. The root cause is that _fetch_remote_metadata uses the raw.githubusercontent.com CDN for github.com hosts, but that CDN returns HTTP 404 for INTERNAL/private repos even with a valid token. The GitHub REST /contents API works correctly with the same token but was only used for GHES/GHE-Cloud hosts.

The fix unifies all GitHub host types (github.com, GHES, GHE Cloud) on the REST API Contents endpoint with Accept: application/vnd.github.raw, removing the CDN path entirely. This is architecturally consistent with how download_strategies.py handles the same trade-off (CDN only for unauthenticated public access) and avoids the double-latency penalty a try/except fallback would incur.

Fixes: #1847

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

Added test_metadata_fetch_github_com_uses_rest_api verifying that github.com packages use api.github.com with proper auth headers and the application/vnd.github.raw accept type.

Spec conformance (OpenAPM v0.1)

  • N/A -- this PR does not change OpenAPM-observable behaviour.

Replace the raw.githubusercontent.com CDN path in _fetch_remote_metadata
with the GitHub REST API (Contents endpoint with raw media type) for all
GitHub host types. The CDN returns HTTP 404 for INTERNAL and private
repositories even with a valid token, while the REST API works correctly.

This unifies github.com, GHES, and GHE Cloud on a single code path using
host_info.api_base, which is architecturally consistent with how
download_strategies.py handles the same trade-off.

Fixes #1847

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 19, 2026 17:59
@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label Jun 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes apm pack dropping per-plugin version for INTERNAL/private github.com repositories by removing the raw.githubusercontent.com fetch path and consistently using the GitHub REST Contents API (raw media type) for metadata enrichment.

Changes:

  • Switched github.com remote metadata fetching to use the REST Contents endpoint (with Accept: application/vnd.github.raw) instead of the raw CDN.
  • Added a unit test ensuring github.com metadata fetches hit api.github.com with the expected headers.
  • Updated an existing regression-test docstring to reflect the corrected host routing behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/apm_cli/marketplace/builder.py Unifies remote metadata fetch logic onto the GitHub REST Contents API for github.com (and keeps GHES/GHE Cloud on REST as well).
tests/unit/marketplace/test_builder.py Adds coverage asserting github.com metadata fetch uses api.github.com and appropriate headers.

Comment thread tests/unit/marketplace/test_builder.py Outdated
Comment on lines +2424 to +2427
parsed = urllib.parse.urlparse(req.full_url)
assert parsed.hostname == "api.github.com"
assert "/repos/acme/private-tools/contents/plugins/core/apm.yml" in parsed.path
assert req.get_header("Accept") == "application/vnd.github.raw"
Comment thread src/apm_cli/marketplace/builder.py Outdated
Comment on lines +908 to +910
Metadata is fetched via the GitHub REST API (Contents endpoint
with raw media type) on the package's host. For non-GitHub-class
hosts, metadata enrichment is skipped.
@github-actions

Copy link
Copy Markdown

APM Review Panel: ship_with_followups

Fix apm pack silent version-drop for INTERNAL/private github.com repos by routing all GitHub host types through REST API Contents endpoint; unblocks enterprise private-tooling pack workflows.

cc @sergio-sisternes-epam @danielmeppiel -- a fresh advisory pass is ready for your review.

The fix is architecturally correct. The silent version-drop for INTERNAL and private github.com repos was caused by raw.githubusercontent.com returning 404 for non-public content, leaving the version field silently absent in marketplace.json. Unifying all GitHub host types on the REST API Contents endpoint resolves this cleanly for every authenticated scenario. The auth-expert confirms token isolation is preserved and no cross-host leakage is introduced. This is a clear, bounded community bug fix that directly unblocks the enterprise internal-tooling use case.

The one genuine technical dissent is the rate-limit regression for unauthenticated public github.com packs. Five panelists independently flag that unauthenticated callers now draw from the 60 req/hr REST budget rather than the CDN (effectively unlimited). The PR author's 'double-latency penalty' objection is accurate for private repos -- where CDN fails first, forcing a second REST call -- but structurally invalid for public repos, where CDN succeeds and no fallback call is ever made. The performance-expert and auth-expert independently converge on the same resolution already codified in download_strategies.py: CDN-first-then-REST gated on token presence for public github.com requests; REST-only for authenticated. This is the architecturally preferred long-term solution. However, it is not a ship blocker: the failure mode for rate-limited unauthenticated packs is graceful (version field drops, debug-logged only, build does not fail), and any production or CI use of apm pack against private repos already requires a token, making the CDN path irrelevant for the primary bug audience. Track as a concrete follow-up with a clear acceptance criterion.

Two evidence-bearing signals elevate above the opinion tier. First, the test-coverage-expert's 'missing' outcome on the integration-with-fixtures tier for the marketplace builder surface is load-bearing: the new unit test proves URL routing at function level but does not exercise the build() -> _prefetch_metadata -> JSON composition -> disk write pipeline. This is a high-priority tracked follow-up, not a ship blocker for a community bug fix enterprise teams need now. Second, the CHANGELOG [Unreleased] section is confirmed empty. Both doc-writer and oss-growth-hacker independently flag this; it is pre-merge table stakes, trivially easy, and converts future search-landing users into upgraders. Add the entry before merge.

Dissent. The rate-limit mitigation approach is the only substantive inter-panelist disagreement. The PR author's 'double-latency penalty' framing covers private repos correctly but does not hold for public repos: CDN succeeds for public content, so a CDN-first gate with token-conditioned REST fallback incurs zero extra latency for public repos and eliminates the rate-limit regression entirely. CDN-first-then-REST gated on token presence is the right long-term fix, but not a ship blocker given the graceful failure mode and the enterprise audience (authenticated). The pre-existing SSRF-adjacent host_info=None path predates this PR and should be tracked in a dedicated issue.

Aligned with: Secure by default -- token isolation is preserved; unauthenticated rate-limit exposure is operational, not a security regression; surfacing 401/403 at WARNING is the follow-up. Pragmatic as npm -- silent failures in a core command erode trust faster than any missing feature; fixing the version-drop and upgrading 401/403 handling both serve this principle. OSS community-driven -- merging promptly and with a prominent CHANGELOG entry signals that APM's enterprise internal-tooling persona is actively maintained.

Growth signal. PR #1854 is a latent enterprise adoption unlock. Teams using apm pack for internal AI-agent tooling were silently losing the version field -- a symptom that reads as 'APM is broken for us' rather than a configuration gap, driving quiet churn before a ticket is ever filed. A symptom-first CHANGELOG entry and a troubleshooting FAQ note convert this fix into a discoverable upgrade signal for every enterprise team that searched for the symptom before filing #1847.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 1 Clean simplification: unified REST API removes duplicate header logic and fixes private/INTERNAL 404s; unauthenticated public-repo rate-limit trade-off is implicit and worth a comment.
CLI Logging Expert 0 1 2 No correctness regressions in logging; one recommended gap: no debug log of resolved URL; agents have zero visibility into which transport the fix activated.
DevX UX Expert 0 1 1 Fix is correct and closes the silent version-drop for private github.com repos; one pre-existing silent failure (no-token 401) remains unaddressed and worth naming in this PR.
Supply Chain Security Expert 0 0 1 No security regressions. Token isolation preserved; one nit on unauthenticated rate-limit exposure.
OSS Growth Hacker 0 2 1 Unblocks apm pack for enterprise/private GitHub.com repos -- a high-value enterprise adoption fix; earns a prominent changelog callout and a migration-note pattern worth reusing.
Auth Expert 0 1 2 Token isolation invariant holds; no cross-host leakage introduced. Main concern is rate-limit regression for unauthenticated public github.com users (CDN replaced by REST 60 req/hr cap).
Doc Writer 0 1 1 No stale or misleading docs found; one missing CHANGELOG [Unreleased] entry; one nit in manifest-schema.md on private-repo auth qualifier.
Test Coverage Expert 0 1 1 New unit test certifies github.com REST API routing with token; missing integration fixture for end-to-end version-in-JSON pipeline.
Performance Expert 0 2 2 REST unification fixes the bug but removes CDN rate-limit exemption for unauthenticated public packs; latency regresses ~100ms/pkg but concurrency=8 threadpool bounds wall-time.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Doc Writer] (blocking-severity) Add CHANGELOG.md [Unreleased] ### Fixed entry for the silent version-drop before merge -- The section is confirmed empty; a symptom-first entry ('apm pack now correctly populates version for INTERNAL/private github.com repos, closes [BUG] apm pack omits per-plugin version for INTERNAL/private github.com repos (raw.githubusercontent.com 404, no REST fallback) #1847') is pre-merge table stakes and converts future search-landing users into upgraders.
  2. [Test Coverage Expert] Add integration-with-fixtures test verifying per-plugin version reaches marketplace.json for a private github.com repo -- The unit test proves URL routing at function level but does not exercise the build() -> _prefetch_metadata -> JSON composition pipeline; a missing-test outcome on a governed-by-policy surface ranks above all recommended opinion findings.
  3. [Performance Expert] Gate unauthenticated github.com metadata fetches CDN-first, fall through to REST API only on 4xx -- mirror the download_strategies.py pattern -- For public repos CDN succeeds on the first request so there is zero double-latency penalty, eliminating the 60 req/hr REST budget drain for unauthenticated CI environments.
  4. [DevX UX Expert] Distinguish urllib.error.HTTPError 401/403 in the except block and log at WARNING with a 'run apm auth' hint -- A user running apm pack on a private repo without a token sees marketplace.json with no version field and zero terminal output explaining why.
  5. [CLI Logging Expert] Add logger.debug of the resolved URL and auth state before issuing the request in _fetch_remote_metadata -- The entire fix is a transport switch (CDN -> REST API) and without a debug trace there is zero confirmation the fix fired on the live request.

Architecture

classDiagram
    direction LR
    class MarketplaceBuilder {
        <<IOBoundary>>
        -_host str
        -_host_info HostInfo
        -_github_token str
        -_auth_resolver AuthResolver
        +_fetch_remote_metadata(pkg) dict
        +_prefetch_metadata(pkgs) dict
    }
    class ResolvedPackage {
        <<ValueObject>>
        +name str
        +source_repo str
        +subdir str
        +sha str
        +host str
    }
    class HostInfo {
        <<ValueObject>>
        +host str
        +kind str
        +api_base str
    }
    class AuthResolver {
        <<Strategy>>
        +resolve(host) AuthContext
        +classify_host(host) HostInfo
    }
    class AuthContext {
        <<ValueObject>>
        +token str
        +source str
    }
    class BuildOptions {
        <<Config>>
        +concurrency int
        +offline bool
    }
    MarketplaceBuilder *-- BuildOptions : configured by
    MarketplaceBuilder ..> ResolvedPackage : reads
    MarketplaceBuilder ..> AuthResolver : delegates to
    MarketplaceBuilder ..> HostInfo : reads
    AuthResolver ..> HostInfo : produces
    AuthResolver ..> AuthContext : returns
    class MarketplaceBuilder:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
    note for MarketplaceBuilder "_fetch_remote_metadata unified to REST API for all GitHub host kinds"
Loading
flowchart TD
    CLI["apm pack CLI entry"] --> Build["MarketplaceBuilder.build()"]
    Build --> Prefetch["_prefetch_metadata(resolved)"]
    Prefetch --> EnsureAuth["_ensure_auth() via AuthResolver"]
    EnsureAuth --> Offline{offline?}
    Offline -- yes --> Skip[return local-only results]
    Offline -- no --> Pool["ThreadPoolExecutor: _fetch_remote_metadata per pkg"]
    Pool --> HostCheck{"pkg.host matches\nbuilder host?"}
    HostCheck -- yes --> UseDefault["token = self._github_token"]
    HostCheck -- no --> ClassifyHost["AuthResolver.classify_host(pkg.host)\n_resolve_token_for_host()"]
    UseDefault --> KindCheck{"host_kind in\ngithub / ghe_cloud / ghes?"}
    ClassifyHost --> KindCheck
    KindCheck -- no --> ReturnNone1[return None -- skip non-GitHub]
    KindCheck -- yes --> GHECheck{"ghe_cloud AND\nno token?"}
    GHECheck -- yes --> ReturnNone2[return None -- requires auth]
    GHECheck -- no --> APIBase{"effective_host\n== github.com?"}
    APIBase -- yes --> GHBase["api_base = host_info.api_base\nOR https://api.github.com"]
    APIBase -- no --> GHESBase["api_base = host_info.api_base\nOR (host/redacted)
    GHBase --> BuildURL["url = api_base/repos/repo/contents/file?ref=sha"]
    GHESBase --> BuildURL
    BuildURL --> AcceptHdr["Accept: application/vnd.github.raw"]
    AcceptHdr --> TokenCheck{token?}
    TokenCheck -- yes --> AuthHdr["Authorization: token ..."]
    TokenCheck -- no --> HTTPReq
    AuthHdr --> HTTPReq["urlopen(req, timeout=5)"]
    HTTPReq --> ParseYAML["yaml.safe_load(raw)"]
    ParseYAML --> Return[return dict or None]
Loading
sequenceDiagram
    participant User
    participant CLI as apm CLI
    participant Builder as MarketplaceBuilder
    participant Auth as AuthResolver
    participant Worker as fetch worker
    participant GH as api.github.com

    User->>CLI: apm pack
    CLI->>Builder: build()
    Builder->>Auth: resolve(github.com)
    Auth-->>Builder: token=ghp_...
    Builder->>Worker: submit _fetch_remote_metadata(pkg)
    Worker->>Worker: effective_host = github.com
    Worker->>Worker: api_base = https://api.github.com
    Worker->>GH: GET /repos/org/private-repo/contents/apm.yml?ref=SHA
    Note over Worker,GH: Accept: application/vnd.github.raw
    Note over Worker,GH: Authorization: token ghp_...
    GH-->>Worker: 200 OK raw YAML
    Worker->>Worker: yaml.safe_load -- version: 2.1.0
    Worker-->>Builder: {version: 2.1.0}
    Builder->>CLI: marketplace.json with version populated
    CLI-->>User: apm pack complete
Loading

Recommendation

Ship after the author adds the CHANGELOG [Unreleased] entry (followup #1, trivial, pre-merge). The fix is architecturally correct and directly unblocks the enterprise private-repo use case that motivated the PR. The rate-limit regression for unauthenticated public packs is real but the failure mode is graceful, and the CDN-first-then-REST follow-up (followup #3) is a clean, bounded improvement that does not need to block this fix. The integration test gap (followup #2) is high-priority and should be the immediate post-merge action. All other followups are tracked improvements that do not affect correctness of the primary fix.


Full per-persona findings

Python Architect

  • [recommended] Unauthenticated public github.com fetches now consume REST API rate budget (60 req/hr) instead of CDN at src/apm_cli/marketplace/builder.py:950
    Before this PR, unauthenticated github.com requests went to raw.githubusercontent.com (CDN, no rate limit). After the PR, all github.com requests go to api.github.com. Unauthenticated callers have 60 req/hr; a marketplace with more than 60 packages run hourly without a GITHUB_TOKEN will silently produce missing version fields when rate-limited (HTTP 403 is swallowed by the broad except). The failure mode is graceful and the fix is architecturally correct.
    Suggested: Add a comment above if effective_host == 'github.com': explaining that raw.githubusercontent.com returns HTTP 404 for INTERNAL/private repos even with a valid token; since visibility is unknowable at call time, REST API is used for all github.com hosts; unauthenticated public-repo fetches consume the 60 req/hr budget but failures are gracefully swallowed.

  • [nit] test_no_auth_header_when_no_token does not assert URL hostname -- add assertion to pin the no-CDN invariant at tests/unit/marketplace/test_builder.py:1871
    Pre-PR this test exercised raw.githubusercontent.com; post-PR it exercises api.github.com. Adding URL assertions makes the no-CDN invariant machine-checkable.
    Suggested: parsed = urllib.parse.urlparse(req.full_url); assert parsed.hostname == 'api.github.com'

CLI Logging Expert

  • [recommended] No debug log of the resolved URL before issuing the request at src/apm_cli/marketplace/builder.py:958
    The entire fix is switching github.com from raw CDN to api.github.com REST API. That URL is constructed but never emitted to the log. An agent or on-call engineer has zero trace to confirm the fix fired.
    Suggested: logger.debug('Fetching metadata for %s from %s (auth=%s)', pkg.name, url, bool(token))

  • [nit] Success log at line 980 omits the source URL at src/apm_cli/marketplace/builder.py:980
    The log gives no confirmation that api.github.com was the endpoint on the success path.

  • [nit] No debug note for unauthenticated github.com REST fetch; rate-limit exposure change is silent at src/apm_cli/marketplace/builder.py:961
    Post-PR unauthenticated github.com reads hit api.github.com (60 req/hr) with no log signal when token is None.

DevX UX Expert

  • [recommended] HTTP 401/403 on metadata fetch is still swallowed at debug level; users get no signal when version is missing due to missing auth at src/apm_cli/marketplace/builder.py:985
    A user running apm pack on a private github.com repo without a token sees marketplace.json with no version field and zero terminal output explaining why. APM's failure-mode principle requires the error to name what failed, why, and one concrete next action.
    Suggested: Catch urllib.error.HTTPError before the blanket except Exception:; on 401/403 log at WARNING with 'run "apm auth" to configure a token'.

  • [nit] Docstring still calls this 'purely cosmetic enrichment' but version is a structured marketplace.json key at src/apm_cli/marketplace/builder.py:901
    Suggested: Change to 'best-effort structural enrichment'.

Supply Chain Security Expert

  • [nit] Unauthenticated github.com packs now hit REST API rate limit (60 req/hr) instead of CDN at src/apm_cli/marketplace/builder.py
    Not a security regression -- an operational reliability concern. Large apm pack runs with many public github.com dependencies and no PAT will silently skip version/description enrichment once rate-limited. Document in release notes so users know to configure a token.

OSS Growth Hacker

  • [recommended] Add a CHANGELOG entry under [Unreleased] that names the silent failure symptom, not just the fix
    Enterprise teams who hit the silent version drop diagnose it as 'apm pack is broken for us' and abandon the tool. The [Unreleased] section is blank. A symptom-first entry makes the fix discoverable and signals that private-repo workflows are a first-class supported path.

  • [recommended] Add a docs FAQ or troubleshooting note: 'apm pack drops version for private repos'
    Private-repo users who hit [BUG] apm pack omits per-plugin version for INTERNAL/private github.com repos (raw.githubusercontent.com 404, no REST fallback) #1847 almost certainly searched docs before filing. A single FAQ entry or troubleshooting note converts future search-landing visitors into upgraders.

  • [nit] Release post angle: 'APM now works with your private GitHub tooling' is the user-facing hook
    The CDN vs REST API detail is maintainer-facing; the user story is zero-configuration private-repo support.

Auth Expert

  • [recommended] Rate-limit regression: unauthenticated github.com metadata fetches now hit 60 req/hr REST API cap instead of effectively unlimited CDN at src/apm_cli/marketplace/builder.py
    Old code used raw CDN for unauthenticated github.com (no rate limit). New code routes all through api.github.com (60 req/hr unauthenticated). CI environments with many public github.com packages and no GITHUB_APM_PAT will hit this cap and see silent metadata failures.
    Suggested: (a) debug-level warning when no token available; (b) update auth docs recommending GITHUB_APM_PAT for pack operations; or (c) CDN fallback only when token is absent (mirrors download_strategies.py).

  • [nit] The or 'https://api.github.com' fallback on line 952 is dead code -- host_info is always non-None when effective_host == 'github.com' at src/apm_cli/marketplace/builder.py:952
    Both code paths reaching the github.com branch guarantee host_info is populated.

  • [nit] Exception-swallowed host_info=None + host_kind fallback to 'github' for non-github.com hosts silently bypasses early returns (pre-existing, not introduced by this PR) at src/apm_cli/marketplace/builder.py
    When AuthResolver.classify_host raises, host_info=None causes host_kind to fall back to 'github', bypassing the non-GitHub early return. Should be tracked as a dedicated issue.
    Suggested: Add if host_info is None: return None guard after the except block.

Doc Writer

  • [recommended] CHANGELOG.md [Unreleased] section is empty -- bug fix not recorded at CHANGELOG.md
    Keep a Changelog convention requires every notable fix to appear in [Unreleased] before release. The section is blank.
    Suggested: Add ### Fixed entry: 'apm pack now correctly populates the version field in marketplace.json for INTERNAL and private github.com repositories (closes [BUG] apm pack omits per-plugin version for INTERNAL/private github.com repos (raw.githubusercontent.com 404, no REST fallback) #1847).'

  • [nit] manifest-schema.md metadata-enrichment note omits the auth qualifier for private github.com repos at docs/src/content/docs/reference/manifest-schema.md
    The 'authenticated' qualifier appears only for GHE Cloud, implying github.com enrichment is unconditional. Private/INTERNAL github.com repos require GITHUB_APM_PAT for version enrichment to succeed.
    Suggested: Update the sentence to apply the auth qualifier to private github.com repos symmetrically.

Test Coverage Expert

  • [recommended] No integration-with-fixtures test verifies that per-plugin version reaches marketplace.json for a github.com private repo at tests/integration/marketplace/test_build_integration.py
    The new unit test proves URL routing at function level (tier: unit). Tier floor for marketplace builder surface is integration-with-fixtures. Probe: grep for urlopen/_prefetch_metadata in integration suite returned zero matches; golden.json fixture has no per-plugin version field. If the wiring between _prefetch_metadata and JSON composition drifts, no integration test will fail.
    Proof (missing at integration-with-fixtures): tests/integration/marketplace/test_build_integration.py::test_private_github_com_version_appears_in_marketplace_json -- proves: apm pack includes version field in marketplace.json for private github.com repos when REST API returns it [devx, governed-by-policy]

  • [nit] test_no_auth_header_when_no_token does not assert URL routes to api.github.com at tests/unit/marketplace/test_builder.py
    Proof (missing at unit): tests/unit/marketplace/test_builder.py::test_no_auth_header_when_no_token -- add URL hostname assertion -- proves: github.com public repos without a token route to REST API, not raw CDN [devx]

Performance Expert

  • [recommended] Unauthenticated users now consume the 60 req/hr REST rate limit for every public github.com package in a pack
    download_strategies.py explicitly documents that CDN is used to avoid the 60 req/hr unauthenticated limit. Post-PR, every public github.com package costs 1 token from that budget. A user packing a 60-plugin public marketplace in CI without GITHUB_TOKEN exhausts the quota in a single run. HTTP 403/429 is silently swallowed.
    Suggested: When token is None and effective_host == github.com, try raw.githubusercontent.com first; on 4xx fall through to REST API. Authenticated requests skip CDN (fixes the bug). Public unauthenticated requests stay on CDN.

  • [recommended] CDN-to-REST latency regression: ~50ms to ~100-200ms per package for public github.com; ~1.5s wall-time for 80 packages at concurrency=8
    raw.githubusercontent.com CDN returns in ~50ms cold. api.github.com is ~100-200ms. Existing threadpool (concurrency=8 default) bounds wall-time to ceil(N/8) * ~150ms. This is during apm pack only, not apm install critical path.
    Suggested: Re-introducing CDN for unauthenticated requests resolves both rate-limit and latency concerns simultaneously.

  • [nit] The 'double-latency penalty' claim in the PR body is accurate for private repos but elides the public-repo regression
    For public unauthenticated repos: CDN-first-then-REST incurs zero extra latency (CDN succeeds immediately). The claim conflates private-repo CDN failure with public-repo CDN success.

  • [nit] The hardcoded 5s urlopen timeout at builder.py:964 is appropriate for REST API; no change warranted
    api.github.com P99 under normal load is under 2s. 5s provides sufficient headroom.

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Generated by PR Review Panel for issue #1854 · sonnet46 15.4M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label Jun 19, 2026
Preserve the original raw.githubusercontent.com fast path for public github.com package metadata while adding the issue-scoped REST Contents fallback for raw 404 responses. This keeps public metadata enrichment cheap and covers INTERNAL/private repositories where raw returns 404 but the API works. Also adds the changelog entry and a mutation-proven regression test for issue #1847.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_now

Community-contributed CDN-first fallback is clean, tested, and strategically aligned.

cc @sergio-sisternes-epam @danielmeppiel -- a fresh advisory pass is ready for your review.

All panelists converged: the change is bounded, the auth perimeter is unchanged (host-scoped token stays on github.com / raw.githubusercontent.com / api.github.com), and the fallback triggers only on a 404 from the raw CDN path. That preserves the fast path for public repos while recovering when GitHub returns 404 for private or INTERNAL metadata. Non-404 errors correctly do not fall back, avoiding a slower API call that could mask unrelated failures. The mutation-break evidence is the strongest signal here: tests prove both the 404 fallback guard and the non-404 no-fallback guard are load-bearing.

Aligned with: secure-by-default: token scoping remains host-bound and no new credential source is introduced; portable-by-manifest: apm pack now preserves package metadata for private github.com package refs; oss-community-driven: the community author is credited in the changelog.

Growth signal. This community PR removes an enterprise/private-repo friction point and credits the contributor in the release narrative.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 0 Bounded fallback inside the existing metadata path; no architecture concerns after the fold.
CLI Logging Expert 0 0 0 No user-facing output changed; debug-only behavior remains appropriate.
DevX UX Expert 0 0 0 apm pack now silently restores the expected metadata version in generated marketplace output.
Supply Chain Security Expert 0 0 0 Token use remains scoped to github.com-owned endpoints; no supply-chain widening found.
OSS Growth Hacker 0 0 0 Community credit and private-repo support are release-note worthy.
Auth Expert 0 0 0 AuthResolver boundaries and per-host token isolation remain intact.
Doc Writer 0 0 0 Changelog-only documentation impact is appropriate; no docs corpus update needed.
Test Coverage Expert 0 0 0 Regression traps cover raw 404 fallback and non-404 no-fallback, both mutation-proven.
Performance Expert 0 0 0 CDN fast path is preserved; REST is used only for the private/INTERNAL 404 case.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Recommendation

Ready for maintainer review. No in-scope follow-ups remain after the shepherd fold.

Folded in this run

  • (panel) Preserve the raw.githubusercontent.com fast path and add REST Contents fallback only on raw HTTP 404 -- resolved in ce85aa1.
  • (panel) Add changelog credit for the community bug fix -- resolved in ce85aa1.
  • (panel) Add the converse non-404 regression trap so only 404 triggers REST fallback -- resolved in ce85aa1.

Regression-trap evidence (mutation-break gate)

  • tests/unit/marketplace/test_builder.py::TestFetchRemoteMetadataGHEHost::test_metadata_fetch_github_com_falls_back_to_rest_api_on_raw_404 -- changed the 404 guard to always raise; test FAILED as expected; guard restored.
  • tests/unit/marketplace/test_builder.py::TestFetchRemoteMetadataGHEHost::test_metadata_fetch_github_com_non_404_does_not_fallback -- changed the non-404 guard to allow fallback; test FAILED as expected; guard restored.

Lint contract

uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent.

Additional local guards: marketplace test file passed (140 tests), pylint R0801 passed, and bash scripts/lint-auth-signals.sh passed.

CI

gh pr checks 1854 --repo microsoft/apm --watch observed all reported checks passing, including CI Lint, Build & Test Shard 1/2, Coverage Combine, PR Binary Smoke, CodeQL, Spec conformance gate, NOTICE Drift Check, Merge Gate, APM Self-Check, and license/cla (after 0 CI fix iteration(s)).

Mergeability status

Captured from gh pr view 1854 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.

PR head SHA CEO stance iters folds defers Copilot rounds CI mergeable mergeStateStatus notes
#1854 ce85aa1 ship_now 1 3 0 2 green MERGEABLE BLOCKED awaiting maintainer review

Convergence

1 outer iteration(s); 2 Copilot round(s). Final panel verdict: ship_now.

Ready for maintainer review.


Full per-persona findings

No unresolved findings. The only in-scope recommendations surfaced during the run were folded into ce85aa1.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

@danielmeppiel danielmeppiel merged commit 69444e6 into main Jun 20, 2026
10 checks passed
@danielmeppiel danielmeppiel deleted the sergio-sisternes-epam-fix-pack-private-repo-metadata-fallback branch June 20, 2026 17:42
danielmeppiel added a commit that referenced this pull request Jun 20, 2026
…#1871)

Add the user-facing PRs merged after the v0.21.0 tag that were missing
from the changelog (#1830 Added; #1854, #1856 Fixed), add the external-
contributor credit on #1855, and move #1853 out of the released [0.21.0]
section into [Unreleased] since it merged after the v0.21.0 tag. No version
bump -- everything stays under [Unreleased].

Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] apm pack omits per-plugin version for INTERNAL/private github.com repos (raw.githubusercontent.com 404, no REST fallback)

3 participants