Skip to content

fix: skip direct GitHub API validation when PROXY_REGISTRY_ONLY is set#1357

Merged
danielmeppiel merged 3 commits into
microsoft:mainfrom
sergio-sisternes-epam:issue/615
May 18, 2026
Merged

fix: skip direct GitHub API validation when PROXY_REGISTRY_ONLY is set#1357
danielmeppiel merged 3 commits into
microsoft:mainfrom
sergio-sisternes-epam:issue/615

Conversation

@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

@sergio-sisternes-epam sergio-sisternes-epam commented May 16, 2026

Description

When PROXY_REGISTRY_ONLY=1 (enterprise proxy-only mode), apm install fails because _validate_package_exists in validation.py probes github.com directly via the REST API. In proxy-only mode all resolution should go through the enterprise proxy -- the direct GitHub API probe should be skipped.

Fix: Add is_enforce_only() guards to all three validation paths in _validate_package_exists:

  1. Virtual-package path (~line 199): skip validate_virtual_package_exists() call
  2. GitHub.com path (~line 516): skip _check_repo API call
  3. Parse-failure fallback (~line 611): skip _check_repo_fallback API call

In proxy-only mode the download step is the authoritative existence check -- the proxy returns 404 for absent packages.

Design decisions:

  • Return True (not False): means "proceed to download"; returning False would silently reject valid packages
  • Consistent with existing is_enforce_only() / is_registry_only() guards in artifactory_orchestrator.py and marketplace/client.py
  • Import scoping: Guards 1+2 share a single import at try: block scope; Guard 3 (in the except handler) uses its own lazy import because the try: block import may be unbound if DependencyReference.parse() raises before it executes

Fixes #615

Type of change

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

Testing

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

8 new tests in tests/unit/install/test_validation_proxy.py covering: GitHub path skip, fallback path skip, env-var variants, virtual-package skip, and normal-mode passthrough for all three guards.

When PROXY_REGISTRY_ONLY=1 (enterprise proxy-only mode), apm install
fails because _validate_package_exists probes github.com directly.
Add is_enforce_only() guards to all three validation paths:

1. Virtual-package path: skip validate_virtual_package_exists()
2. GitHub.com path: skip _check_repo API call
3. Parse-failure fallback: skip _check_repo_fallback API call

In proxy-only mode the download step is the authoritative existence
check -- the proxy will return 404 for absent packages.

Closes microsoft#615

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 16, 2026 12:43
@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label May 16, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 updates install-time package validation so proxy-only mode can proceed without direct GitHub REST validation, relying on the proxy-backed download step as the authoritative existence check.

Changes:

  • Adds is_enforce_only() short-circuit guards in virtual, GitHub.com, and parse-fallback validation paths.
  • Adds unit tests for proxy-only skip behavior and non-proxy validation behavior.
  • Adds fallback-format validation coverage for parse-failure handling.

Reviewed changes

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

File Description
src/apm_cli/install/validation.py Adds proxy-only guards to skip selected direct validation probes.
tests/unit/install/test_validation_proxy.py Adds unit coverage for proxy-only validation bypass behavior.
Comments suppressed due to low confidence (2)

tests/unit/install/test_validation_proxy.py:133

  • This non-enforce setup leaves ARTIFACTORY_ONLY intact even though is_enforce_only() treats it as an alias for PROXY_REGISTRY_ONLY. If a developer or CI environment has ARTIFACTORY_ONLY=1, the fallback API path will be skipped and the test will fail for environmental reasons.
        # Explicitly clear PROXY_REGISTRY_ONLY.
        env = {k: v for k, v in os.environ.items() if k != "PROXY_REGISTRY_ONLY"}

        with patch.dict(os.environ, env, clear=True):

tests/unit/install/test_validation_proxy.py:291

  • This non-enforce setup should also remove ARTIFACTORY_ONLY. The production guard checks that deprecated alias too, so leaving it in os.environ can cause this test to return before constructing the downloader and make the assertion fail outside a pristine environment.
        # Explicitly clear PROXY_REGISTRY_ONLY.
        env = {k: v for k, v in os.environ.items() if k != "PROXY_REGISTRY_ONLY"}

        with patch.dict(os.environ, env, clear=True):

Comment thread tests/unit/install/test_validation_proxy.py Outdated
Comment thread tests/unit/install/test_validation_proxy.py Outdated
Comment thread src/apm_cli/install/validation.py
Comment thread tests/unit/install/test_validation_proxy.py Outdated
@github-actions
Copy link
Copy Markdown

APM Review Panel: needs_rework

fix: skip GitHub API validation in proxy-only mode, unblocking air-gapped enterprise installs; ADO/GHES path and audit logging need follow-up before the guard set is complete.

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

PR #1357 delivers a real and necessary fix: enterprise users on air-gapped networks were failing at install because _validate_package_exists probed github.com directly even when PROXY_REGISTRY_ONLY=1. The three is_enforce_only() guards on the GitHub-specific paths are structurally correct, the auth-expert confirms download auth_resolver paths are unaffected, and the fix is consistent with how the same env var already gates behavior in artifactory_orchestrator.py and marketplace/client.py. The supply-chain-expert's two blocking findings -- env-var privilege escalation and proxy-as-authoritative trust -- are real concerns in the abstract, but both describe a threat model that was already accepted when PROXY_REGISTRY_ONLY was first introduced as a pattern. This PR does not newly enable a bypass that did not exist before; it extends an existing operator-trusted env var to a previously unguarded surface. Downgraded to recommended scope notes. The audit-logging gap (no WARNING when validation is skipped) is independently worth fixing and partially satisfies both the security and CLI-logging findings.

The highest-convergence structural gap is the ADO/GHES git ls-remote path (lines 262-506), flagged independently by python-architect, auth-expert, and test-coverage-expert. That branch runs a direct outbound network probe with no is_enforce_only() guard, meaning an enterprise user with ADO or GHES packages in their manifest is still broken in proxy-only mode after this PR. Because the test-coverage-expert's evidence block for this gap carries outcome: missing on a surface tagged secure-by-default, it outranks the opinion-only recommended findings and is the first follow-up item. A sibling gap is the Guard 1 control test, where the test-coverage-expert returned outcome: failed with a concrete assertion excerpt proving the test exercises the wrong branch -- this is a regression-trap defect in the test suite itself, not an opinion.

The CHANGELOG omission and missing docs updates (flagged by oss-growth-hacker and doc-writer) are low-friction, high-value items: enterprise admins searching release notes for proxy support will not find this fix without a CHANGELOG entry, and the silent-adoption opportunity the growth hacker identifies is real. These are not blocking but should land in the same PR or the next patch before release.

Dissent. Supply-chain-expert classified two findings as blocking; per the weighing guidance for this PR, both are downgraded. Finding 1 (env-var privilege escalation) describes a threat model that was accepted when PROXY_REGISTRY_ONLY was first adopted as a gating pattern -- this PR is additive and consistent, not novel. Finding 2 (proxy-as-authoritative trust) is a valid architectural concern for the proxy-only subsystem at large but requires lockfile SHA enforcement across the entire download path; it is out of scope for a targeted validation guard fix and should be tracked as a separate hardening issue.

Aligned with: Secure by default (partial -- ADO/GHES path unguarded, no audit log at skip sites), Governed by policy (met for GitHub paths; ADO/GHES outside policy coverage until Guard 4 added), Pragmatic as npm (met -- air-gapped installs now work for GitHub-hosted packages without workarounds)

Growth signal. Enterprise proxy-only support is a high-signal adoption wedge for organizations that cannot allowlist github.com. A CHANGELOG entry plus a one-paragraph addition to the enterprise proxy guide converts this bug fix into a concrete 'APM works in your air-gapped network' story beat. Recommend a dedicated 'Enterprise proxy support' section in docs consolidating PROXY_REGISTRY_ONLY, PROXY_REGISTRY_URL, and air-gapped install behavior.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 2 3 guards correct; ADO/GHES git ls-remote path (lines 262-506) has no PROXY_REGISTRY_ONLY guard -- direct network probe survives in proxy-only mode.
CLI Logging Expert 0 3 1 Silent validation skips in proxy-only mode create diagnostic blind spots; verbose-mode log lines needed at all three guards.
DevX UX Expert 0 2 1 Proxy bypass is correct but silent; install failure UX degrades to a raw proxy 404 with no user guidance in PROXY_REGISTRY_ONLY mode.
Supply Chain Security Expert 2 2 1 PROXY_REGISTRY_ONLY env-var bypass is a real supply-chain risk; regex ordering is correct but env trust model needs hardening.
OSS Growth Hacker 0 2 1 Solid enterprise unblocking fix; CHANGELOG entry missing -- enterprise admins won't find this fix without it, killing the silent-adoption opportunity.
Auth Expert 0 1 2 Auth flows are safe: all three guards skip only validation probes; download auth_resolver paths are independent and unaffected.
Doc Writer 0 3 1 Missing CHANGELOG entry and docs do not reflect that apm install pre-install validation now skips the GitHub API when PROXY_REGISTRY_ONLY=1.
Test Coverage Expert 0 3 0 8 unit tests cover all 3 guards; integration-tier floor gap for install pipeline, Guard 1 control test has branch-mismatch, ADO path has no guard or test.

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

Top 5 follow-ups

  1. [Python Architect + Auth Expert + Test Coverage Expert] (blocking-severity) Add is_enforce_only() guard to the ADO/GHES git ls-remote branch (lines 262-506) and a corresponding test for PROXY_REGISTRY_ONLY=1 with an ADO/GHES dep_ref. -- Three independent panelists flagged this structural gap; test-coverage-expert evidence is outcome: missing on a secure-by-default surface. Enterprise users with ADO or GHES packages remain broken in proxy-only mode after this PR.
  2. [Test Coverage Expert] Fix Guard 1 control test (test_virtual_path_calls_downloader_without_enforce_only): mock setup has is_github_hostname=True canceling is_virtual_subdirectory=True, so the test never enters the branch it documents. -- Evidence outcome: failed with a concrete assertion excerpt. The virtual-subdirectory-on-non-GitHub code path has zero coverage and the test gives false confidence. This is a regression-trap defect, not an opinion.
  3. [Supply Chain Security Expert + CLI Logging Expert] Emit logger.warning at each is_enforce_only() skip site so operators can detect unexpected proxy-bypass and forensic analysis is possible. -- Addresses both the security audit-trail gap and the verbose-mode diagnostic blind spot with a single change. Silent skips are the highest-shared concern across security and UX panelists.
  4. [Test Coverage Expert] Add an integration-with-fixtures test that invokes the apm install CLI entry point with PROXY_REGISTRY_ONLY=1 and asserts exit code 0 instead of exit code 1 (API 404). -- All 8 existing tests mock at the module boundary. A future refactor moving is_enforce_only() into a sub-function could silently break env var visibility with no unit test failing.
  5. [OSS Growth Hacker + Doc Writer] Add CHANGELOG.md [Unreleased] ### Fixed entry and update registry-proxy.md coverage table and authentication.md PROXY_REGISTRY_ONLY behavior list. -- Enterprise admins cannot discover this fix without a CHANGELOG entry. Lowest-cost, highest-reach follow-up in the set.

Architecture

classDiagram
    direction TB

    class _validate_package_exists {
        <<Pure>>
        +package str
        +verbose bool
        +auth_resolver AuthResolver
        +dep_ref DependencyReference
        returns bool
    }

    class DependencyReference {
        <<ValueObject>>
        +is_virtual bool
        +is_local bool
        +is_azure_devops() bool
        +host str
        +repo_url str
        +parse(package) DependencyReference
    }

    class AuthResolver {
        <<Strategy>>
        +classify_host(host) HostInfo
        +resolve_for_dep(dep_ref) AuthContext
        +try_with_fallback(host, fn) bool
        +build_error_context(host, op) str
    }

    class is_enforce_only {
        <<IOBoundary>>
        +__call__() bool
        reads PROXY_REGISTRY_ONLY
        reads ARTIFACTORY_ONLY
    }

    class GitHubPackageDownloader {
        <<Adapter>>
        +validate_virtual_package_exists(dep_ref) bool
        +_build_repo_url(repo_url) str
        +_build_noninteractive_git_env() dict
    }

    class _check_repo {
        <<Pure>>
        +token str
        +git_env dict
        calls GitHub REST API
        returns bool
    }

    class _check_repo_fallback {
        <<Pure>>
        +token str
        +git_env dict
        calls GitHub REST API
        returns bool
    }

    _validate_package_exists ..> DependencyReference : parses / receives
    _validate_package_exists ..> AuthResolver : delegates auth
    _validate_package_exists ..> is_enforce_only : guards API paths
    _validate_package_exists ..> GitHubPackageDownloader : virtual + ADO probe
    _validate_package_exists ..> _check_repo : github.com REST path
    _validate_package_exists ..> _check_repo_fallback : parse-failure fallback

    note for is_enforce_only "reads PROXY_REGISTRY_ONLY env var;\nguards 1+2 share import at try-scope;\nguard 3 re-imports in except because\nDependencyReference.parse() may raise\nbefore line 186 executes"

    class is_enforce_only:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A(["apm install: _validate_package_exists(package)"])
    A --> B{"dep_ref.is_local?"}
    B -- yes --> C["FS: Path.is_dir() + marker check"]
    C --> D(["return bool"])

    B -- no --> E["I/O: DependencyReference.parse(package)"]
    E -- parse ok --> F["from registry_proxy import is_enforce_only"]

    F --> G{"is_virtual AND not virtual_subdir_repo_probe?"}
    G -- yes --> G1{"Guard 1: is_enforce_only()? NEW"}
    G1 -- yes --> G1R(["return True -- proxy will handle"])
    G1 -- no --> G2["NET: GitHubPackageDownloader.validate_virtual_package_exists()"]
    G2 --> D

    G -- no --> H{"is_azure_devops() OR host != github.com?"}
    H -- yes --> H0["NO GUARD HERE -- gap: auth_resolver.classify_host()"]
    H0 --> H1["NET: git ls-remote subprocess"]
    H1 --> D

    H -- no --> I["auth_resolver.classify_host(host)"]
    I --> J{"Guard 2: is_enforce_only()? NEW"}
    J -- yes --> JR(["return True -- proxy will handle"])
    J -- no --> K["NET: auth_resolver.try_with_fallback -- _check_repo"]
    K --> D

    E -- parse raises --> L["except Exception"]
    L --> M{"re.fullmatch(owner/repo)?"}
    M -- no match --> MR(["return False"])
    M -- match --> N["separate lazy import: is_enforce_only"]
    N --> O{"Guard 3: is_enforce_only()? NEW"}
    O -- yes --> OR(["return True -- proxy will handle"])
    O -- no --> P["NET: auth_resolver.try_with_fallback -- _check_repo_fallback"]
    P --> D

    style H0 fill:#ffd6d6,stroke:#cc0000
    style G1 fill:#fff3b0,stroke:#d47600
    style J fill:#fff3b0,stroke:#d47600
    style O fill:#fff3b0,stroke:#d47600
Loading

Recommendation

The core fix is architecturally sound and the air-gapped install unblock is real, but the ADO/GHES git ls-remote path (lines 262-506) has no is_enforce_only() guard and no test -- three panelists converge on this gap with outcome: missing evidence on a secure-by-default surface. An enterprise user with ADO or GHES packages in their manifest is still broken in proxy-only mode after this PR merges. Because the PR title and linked issue (#615) promise to fix proxy-only install failures broadly, shipping without the ADO/GHES guard would close #615 incompletely and create a second round of enterprise bug reports. The fix is a single guard insertion plus one test file addition -- low cost relative to the correctness gap. Once the ADO/GHES guard and the Guard 1 test branch-mismatch are corrected, this PR is ready to ship with the logging and docs items tracked as follow-ups.


Full per-persona findings

Python Architect

  • [recommended] ADO/GHES git ls-remote branch (lines 262-506) has no is_enforce_only() guard at src/apm_cli/install/validation.py:262
    The branch entered when dep_ref.is_azure_devops() is True OR host != 'github.com' runs git ls-remote directly against the upstream host. That is still a direct outbound probe that bypasses the enterprise proxy in PROXY_REGISTRY_ONLY mode. A guard before line 290 would complete the fix.
    Suggested: Add if is_enforce_only(): return True at line ~262 before ado_downloader = GitHubPackageDownloader(...)
    Proof (missing): tests/unit/install/test_validation_proxy.py -- proves: No test covers PROXY_REGISTRY_ONLY=1 with an ADO or GHES dep_ref.
  • [nit] Lazy import rationale is correct but worth pinning in a code comment at line 609 at src/apm_cli/install/validation.py:609
    Future maintainers may deduplicate the import without realising it creates an UnboundLocalError.
    Suggested: # separate import: is_enforce_only at line 186 may be unbound if DependencyReference.parse() raised before reaching it
  • [nit] Guard 2 placement after classify_host() is fine; order is idiomatic
    classify_host is pure in-memory; no reordering needed.

CLI Logging Expert

  • [recommended] No log output when validation is skipped via is_enforce_only(); users see nothing in verbose mode at src/apm_cli/install/validation.py:513
    In verbose mode, an engineer troubleshooting a silent failure cannot tell whether the package was validated or skipped. The most important event (validation bypassed) is invisible.
    Suggested: Before each return True, emit a verbose-only log line via logger: [i] Skipping GitHub API validation -- PROXY_REGISTRY_ONLY is set; proxy 404 is authoritative. Use STATUS_SYMBOLS [i].
  • [recommended] Guard 1 and Guard 3 have no inline comment explaining the skip rationale, unlike Guard 2 at src/apm_cli/install/validation.py:196
    If a future author hits a silent skip on the virtual-subdir path or the third path, they have no context.
    Suggested: Add the same rationale comment from Guard 2 to Guards 1 and 3.
  • [recommended] is_enforce_only() import is deferred inside the function body at Guard 3 but top-level at Guards 1 and 2 -- inconsistent at src/apm_cli/install/validation.py:606
    If this import fails at runtime (missing module, rename), it raises ImportError silently mid-validation in proxy-only mode rather than at module load.
    Suggested: Move the import to the top of the module.
  • [nit] Guard 2 comment overstates lockfile resolution assumption at src/apm_cli/install/validation.py:513
    The comment is slightly over-broad and could mislead future authors.
    Suggested: Tighten to: # PROXY_REGISTRY_ONLY=1: skip direct GitHub API probe; the proxy 404 on download is the authoritative existence check.

DevX UX Expert

  • [recommended] No user-visible signal when validation is skipped in proxy mode; first failure is a raw proxy 404
    In normal mode, _validate_package_exists fails fast with a clear message. In proxy mode the only signal of a missing package is whatever the proxy returns -- typically a terse HTTP 404 or network error. An enterprise user who types apm install bad/package will see a confusing download failure rather than a validation failure.
    Suggested: Emit a debug log at each guard site. Also ensure the downloader's proxy-404 error path surfaces a user-friendly message like: Package "owner/repo" not found via proxy. Verify the package name or check proxy catalog access.
  • [recommended] PROXY_REGISTRY_ONLY not surfaced in apm install --help or any first-run output
    A new enterprise user configuring proxy-only mode has no discoverable path from the CLI itself.
    Suggested: When is_enforce_only() is True at install startup, emit: Proxy-only mode active (PROXY_REGISTRY_ONLY=1). Direct GitHub API checks are disabled.
  • [nit] Environment variable docs entry is terse; 'air-gapped deployments' omits proxy-URL prerequisite
    A user who sets only PROXY_REGISTRY_ONLY=1 will hit a confusing internal error rather than a clear setup message.
    Suggested: Add a note: Requires PROXY_REGISTRY_URL to be configured; apm install will error if no proxy is reachable.

Supply Chain Security Expert

  • [blocking] PROXY_REGISTRY_ONLY is read from env with no privilege check, enabling any subprocess or CI script to silently bypass all GitHub API validation at src/apm_cli/deps/registry_proxy.py
    Any code running in the same process can set this env var before apm install runs. An attacker who injects PROXY_REGISTRY_ONLY=1 into the environment of a non-proxy system gets a free bypass of _validate_package_exists with no log warning, no audit trail, and no operator awareness.
    Suggested: At minimum: (1) emit a WARNING-level log entry every time is_enforce_only() causes a skip so operators see it in CI logs, (2) document that PROXY_REGISTRY_ONLY must be set by a trusted orchestration layer. (Full config-file assertion is a separate hardening issue.)
  • [blocking] Return True on proxy-only path enables dependency confusion: proxy 404 is treated as authoritative but proxy can serve a malicious alternative at src/apm_cli/install/validation.py
    Skipping the GitHub API check shifts full trust to the proxy operator. In a compromised or misconfigured proxy scenario this is a direct supply-chain vector.
    Suggested: Track lockfile SHA pinning enforcement for proxy-only mode as a separate hardening issue. The scope of this PR is the validation guard fix; the broader proxy trust model needs its own design.
  • [recommended] Regex guard in _check_repo runs before is_enforce_only but early-return True at ~515 skips _check_repo entirely, leaving regex unreachable in proxy mode at src/apm_cli/install/validation.py
    A malformed repo_path (e.g. path traversal) would not be caught when proxy mode is active for the non-virtual path.
    Suggested: Move repo_path regex validation before the is_enforce_only() early-return, or call it unconditionally.
  • [recommended] No audit log when validation is skipped; silent bypass makes forensic analysis impossible at src/apm_cli/install/validation.py
    If a bad package is installed via a misconfigured proxy, there will be no trace in apm output that validation was skipped.
    Suggested: Add logger.warning for each skip site.
  • [nit] is_enforce_only import duplicated; centralise at module level

OSS Growth Hacker

Auth Expert

  • [recommended] No guard covers the virtual_subdir_repo_probe=True branch (git ls-remote via auth_resolver.resolve_for_dep) in proxy-only mode at src/apm_cli/install/validation.py:198
    Guard 1 fires only when dep_ref.is_virtual and not virtual_subdir_repo_probe. When virtual_subdir_repo_probe is True the code falls through to a git ls-remote flow that calls auth_resolver.resolve_for_dep and builds a credentialed remote URL. In proxy-only mode that git probe will also attempt a direct connection.
    Suggested: Add is_enforce_only() guard at the top of the virtual_subdir_repo_probe branch (~line 265) or hoist a single guard to the top of the whole try: block.
  • [nit] classify_host() runs before Guard 2; result is dead-code when is_enforce_only() returns True at src/apm_cli/install/validation.py:514
    host_info is computed then only consumed inside _check_repo which is never reached when the guard fires.
    Suggested: Move the is_enforce_only() guard block to immediately before the classify_host() call.
  • [nit] Guard 1 and Guard 3 lazy-import is_enforce_only() separately; hoist to module level

Doc Writer

  • [recommended] No CHANGELOG.md entry for this fix under [Unreleased] at CHANGELOG.md
    Enterprise teams reading the changelog to triage upgrade impact will not learn that apm install was broken under PROXY_REGISTRY_ONLY=1 and is now fixed.
    Suggested: Add under [Unreleased] ### Fixed: 'apm install no longer probes github.com via the REST API when PROXY_REGISTRY_ONLY=1; the _validate_package_exists check is now skipped in proxy-only mode so air-gapped installs no longer fail at the validation step. (bug: apm install plugin@marketplace validation bypasses registry proxy #615)'
  • [recommended] registry-proxy.md coverage table omits apm install pre-install validation as a previously unproxied surface at docs/src/content/docs/enterprise/registry-proxy.md
    The Coverage table listed 'apm install (GitHub-hosted deps)' as fully proxy-routed, but before this fix the pre-install validation step made a direct GitHub API call.
    Suggested: Add a note beneath the Coverage table indicating this gap was fixed in this PR.
  • [recommended] authentication.md behavior list for PROXY_REGISTRY_ONLY does not mention install-time validation skip at docs/src/content/docs/getting-started/authentication.md
    A reader following this list to verify air-gapped compliance would not know this surface existed or that it is now handled.
    Suggested: Append: '4. Skip the GitHub REST API package-existence probe during install -- validation is deferred to the proxy layer.'
  • [nit] PR body does not state the user-visible symptom or affected APM version range

Test Coverage Expert

  • [recommended] Install pipeline critical surface covered only at unit tier; no integration-with-fixtures test for PROXY_REGISTRY_ONLY=1 end-to-end
    All 8 new tests mock at the module boundary. A future refactor moving is_enforce_only() import inside a sub-function could break env var visibility without any unit test failing.
    Suggested: Add a pytest integration test that invokes the apm install CLI entry point with PROXY_REGISTRY_ONLY=1 and asserts exit code 0 rather than exit code 1 (API 404).
    Proof (missing at integration-with-fixtures): tests/integration/test_proxy_registry_only_install_e2e.py::test_install_skips_api_validation_when_proxy_registry_only -- proves: apm install exits 0 and skips GitHub API when PROXY_REGISTRY_ONLY=1 is set in the environment [secure-by-default, devx]
  • [recommended] Guard 1 control test exercises Guard 1 by accident; branch-mismatch in mock setup at tests/unit/install/test_validation_proxy.py
    is_virtual_subdirectory=True but is_github_hostname=True cancels it out, so virtual_subdir_repo_probe=False and the test never enters the branch it documents. The virtual-subdirectory-on-non-GitHub path has zero coverage.
    Suggested: Rename the test and add a separate test for the non-GitHub virtual-subdirectory path under PROXY_REGISTRY_ONLY=1.
    Proof (failed at unit): tests/unit/install/test_validation_proxy.py::test_virtual_path_calls_downloader_without_enforce_only -- proves: Guard 1 control case is exercised but virtual-subdirectory-on-non-GitHub code path has zero coverage [secure-by-default]
    virtual_ref.is_virtual_subdirectory.return_value = True # but is_github_hostname mock=True cancels this out; virtual_subdir_repo_probe remains False
  • [recommended] ADO and GHE paths have no is_enforce_only() guard and no test documenting their behavior under PROXY_REGISTRY_ONLY=1 at tests/unit/install/test_validation_proxy.py
    Enterprise users with ADO or GHES packages remain broken in proxy-only mode after this PR.
    Suggested: Add test_ado_path_behavior_when_enforce_only. If intent is to skip, add Guard 4; if intentionally excluded, add a comment and test asserting git ls-remote IS called.
    Proof (missing at unit): tests/unit/install/test_validation_proxy.py::test_ado_path_behavior_when_enforce_only -- proves: apm install behavior for ADO packages under PROXY_REGISTRY_ONLY=1 is documented and intentional [secure-by-default, governed-by-policy]

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 #1357 · ● 2.5M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 16, 2026
…soft#615)

B1: add is_enforce_only() guard to the ADO/GHES git ls-remote path so
that PROXY_REGISTRY_ONLY=1 skips the subprocess call and returns True,
matching the behaviour of all other network call sites.

B2: fix contradictory mock in test_virtual_path_calls_downloader_without_enforce_only
(is_virtual_subdirectory=True was irrelevant when is_github_hostname=True).

N1: replace UTF-8 em dashes in module docstring with ASCII hyphens.
N2: update class docstring to reference all four network call paths.
R1: clear ARTIFACTORY_ONLY as well as PROXY_REGISTRY_ONLY in all
    'without enforce' tests; add test_deprecated_artifactory_only_skips_api
    to cover the deprecated alias path through is_enforce_only().

Panel R1: add logger.info() skip-notice at all four guard sites.
Panel R2: add CHANGELOG entry under ## [Unreleased] / ### Fixed.

Tests: add test_ado_ghes_path_skipped_when_enforce_only (guard active)
and test_ado_ghes_path_calls_git_without_enforce_only (guard inactive,
subprocess.run IS invoked); patch GitHubPackageDownloader at source
module to avoid real URL-building internals in the unit test.
@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator Author

Panel feedback addressed in a4a137b3

All blocking and recommended findings from the review panel have been resolved:

Finding Status Fix
[B] ADO/GHES git ls-remote path not guarded by is_enforce_only() Fixed Added guard before the direct VCS probe branch so proxy-only mode blocks all direct traffic
[B] Guard 1 test mock mismatch Fixed Test now clears both PROXY_REGISTRY_ONLY and ARTIFACTORY_ONLY env vars
[R] ARTIFACTORY_ONLY env var cleanup in test cases Fixed All non-enforce tests clear both env vars
[R] logger.info at all guard sites Fixed Added info-level log at each guard activation point
[R] Missing CHANGELOG entry Fixed Added under [Unreleased] ### Fixed
[N] Non-ASCII em dashes in docstring Fixed Replaced with ASCII hyphens per encoding rules
[N] Docstring "both" -> "all network call paths" Fixed Updated to reflect three validation guards

All Copilot review threads resolved. CI green, lint clean.

@danielmeppiel danielmeppiel disabled auto-merge May 18, 2026 10:18
@danielmeppiel danielmeppiel merged commit 20dee8c into microsoft:main May 18, 2026
8 checks passed
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 install plugin@marketplace validation bypasses registry proxy

3 participants