Skip to content

feat(distribution): add enterprise bootstrap mirrors#1733

Merged
danielmeppiel merged 7 commits into
mainfrom
danielmeppiel/1680-enterprise-bootstrap-mirror
Jun 11, 2026
Merged

feat(distribution): add enterprise bootstrap mirrors#1733
danielmeppiel merged 7 commits into
mainfrom
danielmeppiel/1680-enterprise-bootstrap-mirror

Conversation

@danielmeppiel

@danielmeppiel danielmeppiel commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

feat(distribution): add enterprise bootstrap mirrors

TL;DR

This PR adds a bounded v0 enterprise bootstrap mirror contract for install.sh, install.ps1, and apm self-update. The locked env vars now route release metadata, release assets, installer scripts, and PyPI fallback through internal mirrors, with APM_NO_DIRECT_FALLBACK=1 turning missing mirror coverage into a non-zero, actionable failure.

Note

Closes #1680. Homebrew and Scoop remain docs-only in v0, and apm doctor network --no-public-egress is intentionally left for a follow-up.

Problem (WHY)

  • The accepted issue says the bootstrap path still had public-host gaps: "Today the documented install paths are split across public upstreams".
  • Enterprises asked for an explicit hard-fail mode: "fail closed instead of falling back to public hosts".
  • apm self-update could route a GHES-style host, but not the final v0 mirror names required by the board decision.
  • [!] Installer pip fallback still used public PyPI unless the operator passed custom pip arguments outside the APM bootstrap contract.
  • [!] Documentation had no copy-pasteable no-egress smoke recipe covering GitHub, aka.ms, PyPI, Homebrew, and Scoop denial.

Why these matter: this is a distribution and supply-chain path, so the PR keeps the change small and explicit per "Favor small, chainable primitives over monolithic frameworks." The tests and smoke recipe make the network contract verifiable, following "Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action."

Approach (WHAT)

# Fix Principle Source
1 Add shared Python helpers for the five locked env vars and fail-closed predicates. "Favor small, chainable primitives over monolithic frameworks." src/apm_cli/bootstrap_mirror.py
2 Make apm self-update prefer mirror metadata and installer base URLs before public GitHub or aka.ms. "Context arrives just-in-time, not just-in-case." src/apm_cli/commands/self_update.py
3 Make Unix and Windows installers build metadata, asset, checksum, and pip fallback URLs from mirror env vars. "agents pattern-match well against concrete structures" install.sh, install.ps1
4 Add tests plus a documented no-egress smoke recipe for the acceptance gate. "do the work, run a validator (a script, a reference checklist, or a self-check), fix any issues, and repeat until validation passes." tests/, docs

Implementation (HOW)

  • src/apm_cli/bootstrap_mirror.py — centralizes the env-var names, URL trimming/joining, and fail-closed predicates so self-update and version checking share one contract.
  • src/apm_cli/utils/version_checker.py — lets APM_RELEASE_METADATA_URL override the GitHub releases API and returns None without network I/O when APM_NO_DIRECT_FALLBACK=1 would otherwise hit public GitHub.
  • src/apm_cli/commands/self_update.py — routes installer-script downloads through APM_INSTALLER_BASE_URL, reports mirror-specific failure guidance, and preserves legacy GITHUB_URL / APM_REPO / VERSION behavior when mirror env vars are absent.
  • install.sh and install.ps1 — add mirror URL builders for release metadata, release assets, checksums, and pip fallback. If fail-closed mode is active and the needed mirror is missing or unreachable, the scripts exit non-zero instead of trying public fallback.
  • tests/unit/commands/test_self_update_air_gapped.py, tests/unit/test_version_checker.py, tests/unit/test_enterprise_bootstrap_installers.py — encode the new Python behavior and static installer contract first, including the no-public-host self-update path.
  • Docs and changelog — update Starlight installation/self-update docs, apm-guide usage docs, and [Unreleased] with the v0 scope, Homebrew/Scoop docs-only fence, and the no-egress smoke-test recipe.

Diagrams

Legend: The new mirror env layer fans into the existing self-update and installer paths before any public source is considered.

flowchart LR
    subgraph Overrides[Mirror env]
        Metadata[APM_RELEASE_METADATA_URL]
        Installer[APM_INSTALLER_BASE_URL]
        Assets[APM_RELEASE_BASE_URL]
        PyPI[APM_PYPI_INDEX_URL]
        FailClosed[APM_NO_DIRECT_FALLBACK]
    end
    subgraph SelfUpdate[apm self-update]
        VersionChecker[version_checker]
        InstallerFetch[self_update installer fetch]
    end
    subgraph Scripts[installer scripts]
        ScriptMetadata[release metadata]
        ScriptAssets[release assets]
        PipFallback[pip fallback]
    end
    Metadata --> VersionChecker
    Metadata --> ScriptMetadata
    Installer --> InstallerFetch
    Assets --> ScriptAssets
    PyPI --> PipFallback
    FailClosed --> VersionChecker
    FailClosed --> InstallerFetch
    FailClosed --> ScriptMetadata
    FailClosed --> ScriptAssets
    FailClosed --> PipFallback
    classDef new stroke-dasharray: 5 5;
    class Metadata,Installer,Assets,PyPI,FailClosed new;
Loading

Trade-offs

  • Env vars over config file schema. Chose process env because installers and apm self-update already inherit it; rejected config-file wiring because day-0 bootstrap happens before APM config can be trusted.
  • Static installer tests plus documented smoke recipe. Chose static unit coverage for shell/PowerShell wiring and a copy-paste no-egress recipe; rejected adding a new integration harness because the board scoped shell changes to a documented smoke test where feasible.
  • Homebrew/Scoop docs-only. Chose to document package-manager mirroring expectations; rejected code hooks into Homebrew/Scoop because the board explicitly deferred those code paths for v0.
  • No doctor command. Chose the bounded mirror contract; rejected apm doctor network --no-public-egress because it is out of scope for this issue's v0 decision.

Benefits

  1. Five locked env vars now cover the v0 code paths: installer script, release metadata, release asset, PyPI fallback, and fail-closed behavior.
  2. apm self-update --check can prove mirror metadata is used without running the installer.
  3. Fail-closed mode exits non-zero before public GitHub or aka.ms is contacted when the required mirror env var is missing.
  4. The docs include a no-egress smoke recipe that denies GitHub, aka.ms, PyPI, Homebrew, and Scoop upstreams.

Validation

uv run --extra dev pytest tests/unit/commands/test_self_update_air_gapped.py tests/unit/test_version_checker.py tests/unit/test_enterprise_bootstrap_installers.py tests/unit/test_update_command.py -q && bash -n install.sh && if command -v pwsh >/dev/null 2>&1; then pwsh -NoProfile -Command '$null = [scriptblock]::Create((Get-Content -Raw install.ps1)); "ps1 ok"'; else echo 'pwsh unavailable'; fi

.................................................................................                                [100%]
81 passed in 0.90s
ps1 ok
Lint contract output

uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/ && uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ && bash scripts/lint-auth-signals.sh && git --no-pager diff --check

All checks passed!
1236 files already formatted

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

[*] Rule A: get_bearer_provider boundary (any reference)
[*] Rule B: git ls-remote auth-delegated annotation
[+] auth-signal lint clean

Scenario Evidence

# Scenario (user promise) Principle(s) Test(s) proving it Type
1 apm self-update --check with fail-closed mode does not touch public GitHub when no metadata mirror is configured Secure by default, Governed by policy, DevX tests/unit/commands/test_self_update_air_gapped.py::TestEnterpriseBootstrapSelfUpdate::test_no_direct_fallback_blocks_public_metadata_lookup unit
2 apm self-update with mirror env vars fetches metadata and installer scripts only from the mirror host Secure by default, Vendor-neutral, DevX tests/unit/commands/test_self_update_air_gapped.py::TestEnterpriseBootstrapSelfUpdate::test_self_update_uses_mirrors_without_public_hosts unit
3 Release metadata checks use APM_RELEASE_METADATA_URL, and fail-closed mode skips public metadata lookup when no mirror is set Secure by default, Governed by policy tests/unit/test_version_checker.py::TestAirGappedEnvVars::test_release_metadata_url_overrides_github_api_url
tests/unit/test_version_checker.py::TestAirGappedEnvVars::test_no_direct_fallback_without_mirror_skips_public_request
unit
4 Unix and Windows installers expose all five v0 enterprise bootstrap env vars DevX, Secure by default tests/unit/test_enterprise_bootstrap_installers.py unit

How to test

  • Run the targeted pytest command above; expect 81 tests to pass and both installer syntax checks to succeed.
  • Set APM_NO_DIRECT_FALLBACK=1 without APM_RELEASE_METADATA_URL, then run apm self-update --check; expect a non-zero exit and an action telling you to set APM_RELEASE_METADATA_URL or VERSION.
  • Set all four mirror URLs plus APM_NO_DIRECT_FALLBACK=1, then run apm self-update --check; expect only the metadata mirror to be queried.
  • Follow the no-egress smoke recipe in docs/src/content/docs/getting-started/installation.md; expect the fake archive failure but no denied public-host request.

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

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 10, 2026 22:17

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

Adds a v0 “enterprise bootstrap mirror” contract across the bootstrap surfaces (install.sh, install.ps1, and apm self-update) so operators can route release metadata/assets/installer scripts/PyPI fallback through internal mirrors, with APM_NO_DIRECT_FALLBACK=1 enabling fail-closed behavior.

Changes:

  • Introduces src/apm_cli/bootstrap_mirror.py helpers for the locked env vars and “no direct fallback” predicates, and wires them into version checking + self-update.
  • Extends both installer scripts to build metadata/asset/pip URLs from mirror env vars and to fail closed when mirror coverage is incomplete.
  • Adds unit tests plus documentation updates for the new mirror contract and a “no-egress” smoke recipe.
Show a summary per file
File Description
src/apm_cli/bootstrap_mirror.py Centralizes env var parsing, URL joining, and fail-closed predicates for bootstrap mirrors.
src/apm_cli/utils/version_checker.py Allows APM_RELEASE_METADATA_URL override and skips public GitHub lookup when fail-closed would otherwise require public egress.
src/apm_cli/commands/self_update.py Routes installer script download via APM_INSTALLER_BASE_URL, improves mirror-specific failure guidance.
install.sh Adds mirror URL builders and fail-closed guards for metadata/assets and pip fallback.
install.ps1 Adds mirror URL builders and fail-closed guards for metadata/assets and pip fallback (PowerShell).
tests/unit/test_version_checker.py Adds coverage for metadata URL override + fail-closed no-public-metadata behavior.
tests/unit/commands/test_self_update_air_gapped.py Adds self-update tests ensuring mirror-only requests under fail-closed mode.
tests/unit/test_enterprise_bootstrap_installers.py Static contract checks that installers reference all five mirror env vars.
docs/src/content/docs/getting-started/installation.md Documents mirror env vars, mirror layout, and a no-egress smoke test recipe.
docs/src/content/docs/reference/cli/self-update.md Documents the mirror contract for apm self-update and fail-closed semantics.
packages/apm-guide/.apm/skills/apm-usage/installation.md Updates guide content to reflect apm self-update and the mirror env var contract.
CHANGELOG.md Adds a changelog entry for enterprise bootstrap mirror mode (needs placement/format adjustment).

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 8

Comment thread install.sh
Comment on lines +300 to 303
DOWNLOAD_URL=$(release_asset_url "$TAG_NAME" "$DOWNLOAD_BINARY")
echo -e "${GREEN}Version: $TAG_NAME${NC}"
echo -e "${BLUE}Download URL: $DOWNLOAD_URL${NC}"
fi
Comment thread install.sh
Comment on lines +327 to +332
if [ -n "$APM_RELEASE_METADATA_URL" ] && { [ $CURL_EXIT_CODE -ne 0 ] || [ -z "$LATEST_RELEASE" ]; }; then
echo -e "${RED}Error: Failed to fetch release metadata from APM_RELEASE_METADATA_URL${NC}"
echo "Mirror URL: $APM_RELEASE_METADATA_URL"
echo "Check that the mirror is reachable and publishes GitHub-compatible latest.json."
exit 1
fi
Comment thread install.sh
Comment on lines +469 to +474
if [ -n "$APM_RELEASE_BASE_URL" ]; then
echo -e "${RED}Error: Failed to download APM CLI from APM_RELEASE_BASE_URL mirror${NC}"
echo "Mirror URL: $DOWNLOAD_URL"
echo "Check that the mirror is reachable and contains $TAG_NAME/$DOWNLOAD_BINARY."
exit 1
fi
Comment thread install.sh
Comment on lines +491 to +496
if [ -n "$APM_RELEASE_BASE_URL" ]; then
echo -e "${RED}Error: Failed to download APM CLI from APM_RELEASE_BASE_URL mirror${NC}"
echo "Mirror URL: $DOWNLOAD_URL"
echo "Check that the mirror is reachable and contains $TAG_NAME/$DOWNLOAD_BINARY."
exit 1
fi
Comment thread install.ps1 Outdated
Comment on lines +482 to +484
Write-ErrorText "Failed to fetch release metadata from APM_RELEASE_METADATA_URL."
Write-Host "Mirror URL: $releaseMetadataUrl"
if ($metadataError) { Write-Host "Details: $metadataError" }
Comment thread install.ps1
Comment on lines +616 to +621
if (-not $downloadOk -and $releaseBaseUrl) {
Write-ErrorText "Failed to download APM CLI from APM_RELEASE_BASE_URL mirror."
Write-Host "Mirror URL was: $directUrl"
Write-Host "Check that the mirror is reachable and contains $tagName/$assetName."
exit 1
}
Comment thread src/apm_cli/commands/self_update.py Outdated
Comment on lines +73 to +83
def _get_manual_update_command() -> str:
"""Return the manual update command for the current platform."""
try:
installer_url = _get_update_installer_url()
except RuntimeError:
installer_url = "<set APM_INSTALLER_BASE_URL>/install.ps1"
if not _is_windows_platform():
installer_url = "<set APM_INSTALLER_BASE_URL>/install.sh"
if _is_windows_platform():
return 'powershell -ExecutionPolicy Bypass -c "irm https://aka.ms/apm-windows | iex"'
return "curl -sSL https://aka.ms/apm-unix | sh"
return f'powershell -ExecutionPolicy Bypass -c "irm {installer_url} | iex"'
return f"curl -sSL {installer_url} | sh"
Comment thread CHANGELOG.md
Comment on lines 8 to 15
## [Unreleased]

## [0.19.0] - 2026-06-09

### Added

- Enterprise bootstrap mirror mode lets `install.sh`, `install.ps1`, and `apm self-update` use internal release, installer, and PyPI mirrors with fail-closed public fallback. (closes #1680)
- `apm install <package> --target openclaw` adds OpenClaw as a new experimental
danielmeppiel and others added 2 commits June 11, 2026 01:07
Folds copilot-pull-request-reviewer follow-ups (credential redaction in mirror output, copy-pasteable self-update fallback, changelog placement).

Refs #1680

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

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_with_followups

Bounded v0 enterprise bootstrap mirror contract: five locked env vars route release metadata, assets, installer scripts, and PyPI fallback through internal mirrors and fail closed with no public egress.

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

This PR lands the board-approved v0 cleanly and inside its fence. The five locked env var names match the ratified spec, fail-closed mode produces actionable errors with non-zero exits, Homebrew/Scoop stay docs-only, and the binding acceptance gate -- a copy-pasteable no-egress smoke recipe that denies GitHub, aka.ms, PyPI, Homebrew, and Scoop -- is present in the installation docs. CI is fully green and the required v0 deliverables (env-var reference, smoke recipe, docs-only fence) are all satisfied.

The panel converges on ship. The strongest signal is an auth-surface nuance rather than a regression: on Unix, install.sh attaches the resolved GitHub token to mirror metadata and asset requests, while install.ps1 leaves mirror downloads unauthenticated -- a cross-host token-transmission asymmetry worth a deliberate decision. Supply-chain and test-coverage raise hardening follow-ups, none of which block the bounded scope.

Dissent. None. Personas agree the change is correct and in-scope; disagreement is only on how aggressively to harden the token-to-mirror path, which the CEO weighs as a fast follow rather than a merge blocker.

Aligned with: Secure by default -- fail-closed with no public fallback and credential-redacted installer output; Governed by policy -- explicit APM_NO_DIRECT_FALLBACK contract with a documented, verifiable no-egress gate.

Growth signal. Fully air-gapped install and self-update is a concrete enterprise adoption unlock. Worth a dedicated line in the release notes and an enterprise-bootstrap callout in the install docs landing.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 1 Clean single-purpose bootstrap_mirror module of pure helpers shared across self-update and version-check; one import-style nit.
CLI Logging Expert 0 0 0 Fail-closed errors are actionable, exit non-zero, and printed URLs are credential-redacted on both installers.
DevX UX Expert 0 0 0 Coherent enterprise ergonomics: locked env vars + which-var-to-set failures + env-placeholder manual command.
Supply Chain Security 0 2 0 No-egress proof + redaction land; egress-boundary scoping and pip-path coverage are documentation/hardening notes.
Auth Expert 0 1 0 install.sh sends the GitHub token to operator mirror URLs; install.ps1 does not -- asymmetry + cross-host token.
OSS Growth Hacker 0 0 0 Air-gapped bootstrap is a real enterprise story; amplify in release notes.
Test Coverage Expert 0 1 0 Python paths well covered (urlparse assertions); installer fail-closed is static-string only.
Doc Writer 0 0 1 All v0 doc deliverables present; one clarity nit on the smoke recipe.

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

Top 4 follow-ups

  1. [Auth Expert] Decide and document whether the GitHub token should ride along to mirror hosts. install.sh attaches Authorization: token to APM_RELEASE_METADATA_URL / APM_RELEASE_BASE_URL requests; install.ps1 keeps mirror downloads unauthenticated -- align the two and make cross-host token transmission an explicit, opt-in choice. -- A GitHub PAT reaching an arbitrary operator-configured mirror host is a token-scoping surprise; the Unix/Windows split also yields inconsistent enterprise behavior.
  2. [Supply Chain Security] Document that fail-closed scoping keys off the public github.com default: a custom GHES GITHUB_URL plus APM_NO_DIRECT_FALLBACK=1 still egresses to that GHES host. -- Operators reading "fail closed / no direct fallback" may assume zero egress; the GHES coexistence boundary should be stated so the guarantee is not over-read.
  3. [Test Coverage] Add an executable fail-closed exit-code check (a scripted shell/smoke harness) for the installer branches now asserted only by static string presence. -- Locks in the non-zero-exit + no-public-host contract so a future edit cannot silently regress the binding acceptance gate.
  4. [Supply Chain Security] Note in the smoke recipe that the curl deny-list does not wrap the pip fallback path; pip egress is gated by APM_NO_DIRECT_FALLBACK + APM_PYPI_INDEX_URL rather than exercised by the wrapper. -- Keeps the proof honest about which code path the recipe actually verifies.

Architecture

classDiagram
    class bootstrap_mirror {
        +get_env_url(name) str_or_None
        +env_flag_enabled(name) bool
        +no_direct_fallback_enabled() bool
        +append_url_path(base, parts) str
        +get_release_metadata_url() str_or_None
        +get_release_base_url() str_or_None
        +get_installer_base_url() str_or_None
        +get_pypi_index_url() str_or_None
        +is_public_github_url(url) bool
        +release_metadata_public_lookup_blocked(url) bool
        +installer_public_download_blocked(url) bool
        +build_mirrored_release_asset_url(tag, asset) str_or_None
    }
    class self_update {
        +_get_update_installer_url()
        +_get_manual_update_command()
        +self_update(check)
    }
    class version_checker {
        +_build_releases_api_url(url, repo)
        +get_latest_version_from_github(repo, timeout)
    }
    self_update ..> bootstrap_mirror : uses
    version_checker ..> bootstrap_mirror : uses
Loading
flowchart TD
    Start([self-update / installer entry]) --> Q1{APM_NO_DIRECT_FALLBACK set?}
    Q1 -- no --> Pub[Use mirror if set, else public GitHub / aka.ms / PyPI]
    Q1 -- yes --> Q2{Required mirror env var configured?}
    Q2 -- yes --> Mirror[Resolve URL from mirror base]
    Q2 -- no --> Q3{Host is public github.com?}
    Q3 -- yes --> Fail[Actionable error + non-zero exit, NO public fallback]
    Q3 -- no --> GHES[Egress to configured GITHUB_URL / GHES host]
    Mirror --> Done([Download via mirror])
    Pub --> Done
    GHES --> Done
Loading

Recommendation

Ship with follow-ups. The bounded v0 is correct, in-scope, fully documented, and CI-green; nothing blocks merge. Track follow-up #1 (token-to-mirror decision + Unix/Windows alignment) as the highest-signal item for a fast follow, and fold the fail-closed-scoping doc note (#2) whenever the install docs are next touched.


Full per-persona findings

Python Architect

  • [nit] Import-style inconsistency for the new shared module at src/apm_cli/utils/version_checker.py
    version_checker imports from apm_cli.bootstrap_mirror import ... (absolute) while src/apm_cli/commands/self_update.py uses from ..bootstrap_mirror import ... (relative). The module itself is exemplary: pure functions, single responsibility, modern str | None typing, no hidden state -- a good seam shared by both consumers.
    Suggested: Pick one convention (relative within the package) for consistency.

CLI Logging Expert

No blocking or recommended findings. Fail-closed messages name the exact env var to set and pair logger.error with an actionable logger.info; installers exit non-zero before any public host is contacted. Printed URLs pass through redact_url_credentials / Redact-UrlCredentials, and the Python failure paths avoid echoing raw mirror URLs (manual command emits the literal $APM_INSTALLER_BASE_URL placeholder, not an expanded value).

DevX UX Expert

No findings. The five locked env vars are consistent and discoverable, failure guidance is prescriptive (which variable to set), and the manual-update command degrades gracefully to an env-var placeholder under fail-closed mode.

Supply Chain Security Expert

  • [recommended] Fail-closed scoping is anchored to the public github.com default across install.sh, install.ps1, self_update.py, and version_checker.py
    The *_public_*_blocked predicates only trip when is_public_github_url is true. A custom GHES GITHUB_URL with APM_NO_DIRECT_FALLBACK=1 and no mirror still egresses to that GHES host. This is a defensible coexistence design, but "no direct fallback" can be over-read as zero egress.
    Suggested: State the boundary in the self-update / installation docs.

  • [recommended] No-egress smoke recipe wraps curl only; the pip fallback path is not exercised by the deny-list
    The recipe proves the binary path stays on the mirror, but pip egress is governed by APM_NO_DIRECT_FALLBACK + APM_PYPI_INDEX_URL rather than the wrapper.
    Suggested: Add one sentence so the proof's scope is explicit.

    Positive: credential/URL redaction and fail-closed-with-no-public-fallback (the core threat model) are correctly implemented; checksum sidecars are fetched from the mirror base in mirror mode, preserving integrity verification.

Auth Expert -- active (diff changes host/fallback selection and the install.sh Authorization path)

  • [recommended] install.sh transmits the resolved GitHub token to mirror hosts; install.ps1 does not
    In install.sh, curl -s -H "Authorization: token $AUTH_HEADER_VALUE" "$LATEST_RELEASE_URL" and the authenticated asset download use the mirror URL when APM_RELEASE_METADATA_URL / APM_RELEASE_BASE_URL are set, so a GitHub PAT/GITHUB_TOKEN is sent to the operator-configured mirror. install.ps1 gates its authenticated retry behind -not $releaseBaseUrl and uses unauthenticated Invoke-RestMethod for mirror metadata, so it never sends the token to the mirror.
    Suggested: Align the two scripts and make cross-host token attachment an explicit, opt-in behavior (or suppress the GitHub token for non-GitHub mirror hosts).

OSS Growth Hacker

No findings. Air-gapped/enterprise-mirror install and self-update is a high-value adoption narrative; recommend amplifying it in the release notes and an enterprise-bootstrap callout near the top of the install docs.

Test Coverage Expert

  • [recommended] Installer fail-closed behavior is verified by static string-presence assertions, not execution
    tests/unit/test_enterprise_bootstrap_installers.py asserts the helper names and redaction strings exist, and the Python paths have strong behavioral coverage (test_self_update_air_gapped.py asserts requested hostnames collapse to the mirror; test_version_checker.py asserts the no-direct-fallback skip and metadata override, all via urlparse). The shell/ps1 exit-code branches themselves are not executed.
    Suggested: Add a lightweight scripted exit-code check for the fail-closed branches to harden the binding acceptance gate.

Doc Writer -- active (env-var reference, no-egress smoke recipe, Homebrew/Scoop docs-only)

  • [nit] All required v0 doc deliverables are present (env-var tables in installation + self-update + apm-guide, the copy-paste no-egress smoke recipe, and the Homebrew/Scoop docs-only fence). Minor clarity gap: the smoke recipe does not state that its curl deny-list excludes the pip fallback path.
    Suggested: Add one clarifying sentence to the recipe.

Performance Expert -- inactive

The diff does not touch deps/**, cache/**, install/phases/**, install/pipeline.py, install/resolve.py, transport, and claims no performance win. No performance lens applies.

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

danielmeppiel and others added 3 commits June 11, 2026 09:55
install.sh attached the resolved GitHub token to APM_RELEASE_METADATA_URL
and APM_RELEASE_BASE_URL mirror requests, and install.ps1 leaked it on the
asset final-fallback and checksum-retry paths. Gate every auth attach on
mirror-env absence so the token rides only to the canonical GitHub /
configured GHES host, never to an operator mirror. The two scripts are now
symmetric. Adds static symmetry traps plus executable fail-closed exit-code
tests (no network) for the installer guards.

addresses CEO follow-up #1 (Auth Expert / Supply Chain) and Test Coverage
follow-up #3 on PR #1733.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document that APM_NO_DIRECT_FALLBACK keys off the public github.com default:
a custom GHES GITHUB_URL still egresses to that host, so 'no direct fallback'
means no public fallback, not zero egress. Extend the no-egress smoke recipe
to wrap pip alongside curl so the PyPI fallback path is actually proven, and
note the token-to-canonical-host-only rule across installer and self-update
docs and the apm-guide usage doc.

addresses CEO follow-up #2 (Supply Chain), #4 (smoke pip path), and Doc
Writer nit on PR #1733.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Align version_checker.py with self_update.py: import the shared
bootstrap_mirror module via a package-relative path (from ..bootstrap_mirror)
for one consistent convention across both consumers.

addresses Python Architect nit on PR #1733.

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

Copy link
Copy Markdown
Collaborator Author

Shepherd advisory: follow-ups folded, CI green

The board-approved v0 panel follow-ups have been folded into this PR. No
fresh multi-persona panel was run; this terminal advisory records the
convergence. The panel's ship_with_followups items are now in the diff.

Folded in this run

  • (panel) Auth Expert FU1 + Test Coverage FU3 -- install.sh no longer sends
    the GitHub token to APM_RELEASE_METADATA_URL / APM_RELEASE_BASE_URL
    mirror hosts, and install.ps1 no longer leaks it on the asset
    final-fallback or checksum-retry paths. Auth now attaches only to the
    canonical GitHub / configured GHES host; the two scripts are symmetric.
    Added static symmetry traps plus executable (no-network) fail-closed
    exit-code tests. -- resolved in c3b6e2c.
  • (panel) Supply Chain FU2 -- documented that fail-closed scoping keys off the
    public github.com default: a custom GHES GITHUB_URL + APM_NO_DIRECT_FALLBACK
    still egresses to that GHES host ("no public fallback", not "zero egress").
    Added to installation.md, self-update.md, and the apm-guide usage doc.
    -- resolved in 2f480a2.
  • (panel) Supply Chain FU4 + Doc Writer nit -- the no-egress smoke recipe now
    wraps pip/pip3 alongside curl, so the PyPI fallback path is actually
    proven, with a clarifying sentence on what the deny-list covers.
    -- resolved in 2f480a2.
  • (panel) Python Architect nit -- version_checker.py now imports
    bootstrap_mirror via a package-relative path, matching self_update.py.
    -- resolved in 90ff579.

Copilot signals reviewed

The Copilot reviewer left 8 inline comments (6x URL-credential redaction on
install.sh/install.ps1, the self_update.py manual-command placeholder,
and CHANGELOG placement). All 8 are NOT-LEGIT for this run: they were already
addressed at the pre-run head (cdf7593) -- printed URLs pass through
redact_url_credentials / Redact-UrlCredentials, the manual command emits
the literal $APM_INSTALLER_BASE_URL placeholder, and the CHANGELOG entry is
already under [Unreleased] ending in (#1733).

Deferred (out-of-scope follow-ups)

None. Every panel follow-up and nit was in-scope and folded. The GHES
coexistence egress boundary is a documented design, not a deferral.

Regression-trap evidence (mutation-break gate)

  • test_unix_installer_fail_closed_metadata_exit_code -- deleted the
    fail-closed metadata guard in install.sh; the test FAILED as expected
    (the actionable APM_RELEASE_METADATA_URL is not configured message was
    gone); guard restored.

Lint contract

uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent (exit 0).
pylint R0801 10.00/10 and scripts/lint-auth-signals.sh clean.

CI

All required checks green on head 90ff579a (Lint, Build & Test Shards 1-2,
PR Binary Smoke, Spec conformance gate, CodeQL, Analyze python/actions,
Coverage Combine, gate, build; deploy skipped). 0 CI recovery iterations.

Mergeability status

PR head SHA iters folds defers Copilot CI mergeable mergeStateStatus
#1733 90ff579a 1 4 0 1 round (8 NOT-LEGIT) green MERGEABLE BLOCKED

MERGEABLE confirms no merge conflict. BLOCKED reflects the required
human-review gate, not a defect -- this shepherd never merges. Ready for
maintainer review.

@danielmeppiel

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: needs_rework

Enterprise bootstrap mirrors unlock air-gapped installs, but the Python version-check path still sends GitHub tokens to mirror metadata URLs.

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

The panel converges on one high-severity security/auth defect: src/apm_cli/utils/version_checker.py attaches Authorization: token ... to the URL returned by _build_releases_api_url(), and that URL is now the operator-configured APM_RELEASE_METADATA_URL when a metadata mirror is set. The shell and PowerShell installers already suppress tokens for mirror metadata and asset hosts, so the Python version-check path is the asymmetric leak. The fix is tightly scoped: attach the header only when get_release_metadata_url() is None, then add the matching regression test.

Beyond that, the feature is strategically strong and mostly low-drama. The mirror contract covers the v0 env vars, fail-closed behavior, docs, and smoke recipe. Architecture, logging, DevX, docs, growth, and test coverage findings are useful polish or follow-up traps, but they orbit the same core recommendation: fix the token-to-mirror path first, then the PR is close to a clean ship.

Dissent. None. Auth and supply-chain independently identified the same token-to-mirror issue; other personas agree the remaining items are polish or follow-up hardening.

Aligned with: Secure by default -- Needs repair on this commit because version checking can send GitHub tokens to mirror metadata hosts.; Governed by policy -- Advanced by explicit fail-closed mirror routing that enterprises can verify and enforce.; Pragmatic as npm -- The five-env-var bootstrap contract is copy-pasteable and easy to explain.

Growth signal. Enterprise mirror v0 is a concrete governed-by-policy adoption story. The no-egress smoke recipe is release-note proof, not just positioning.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 2 Clean module placement; env-var contract centralized correctly; one recommended finding on module location convention.
CLI Logging Expert 0 2 2 Mirror error messages are actionable and redacted; minor inconsistencies in logger method choice and message phrasing.
Devx UX Expert 0 2 2 Mirror env-var contract is discoverable and actionable; recovery command and docs density deserve polish.
Supply Chain Security Expert 1 2 1 Python version checking sends GitHub tokens to mirror metadata hosts; shell scripts correctly avoid it.
OSS Growth Hacker 0 1 2 Enterprise mirror docs are copy-paste ready and strengthen the trust signal; changelog entry buries the release story.
Auth Expert 1 1 1 Python version checker leaks GitHub tokens to mirror metadata hosts; shell installers are symmetric and correct.
Doc Writer 0 2 2 Docs are accurate vs code; tighten duplicated fail-closed text, Windows checksum wording, and GitHub-only self-update framing.
Test Coverage Expert 0 1 0 Fail-closed metadata and mirror-only flows have traps; installer-download fail-closed path lacks a direct Python CLI test.

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

Top 5 follow-ups

  1. [Supply Chain Security Expert] (blocking-severity) Fix token leak in version_checker.py by attaching Authorization only when get_release_metadata_url() is None. -- A GitHub PAT or token must not be transmitted to an operator mirror host.
  2. [Supply Chain Security Expert] Add a unit test asserting Authorization is absent when APM_RELEASE_METADATA_URL is set. -- The current override test proves the mirror URL is used, but not that the header is withheld.
  3. [Test Coverage Expert] Add a test for the apm self-update installer-download fail-closed path with no APM_INSTALLER_BASE_URL. -- That distinct non-zero exit path is a secure-by-default contract and currently lacks a direct regression trap.
  4. [Supply Chain Security Expert] Track integrity verification for installer scripts fetched from APM_INSTALLER_BASE_URL. -- The mirror installer script is executed after download; checksum or signature coverage would tighten the bootstrap trust boundary.
  5. [Doc Writer] Clarify docs around Windows-only .sha256 sidecars and make installation.md the canonical fail-closed explanation. -- Keeps enterprise mirror operators from over-reading Unix checksum behavior or chasing duplicated docs semantics.

Architecture

classDiagram
    direction LR
    class BootstrapMirror {
        <<Module>>
        +get_env_url(name) str_or_None
        +env_flag_enabled(name) bool
        +no_direct_fallback_enabled() bool
        +append_url_path(base, parts) str
        +get_release_metadata_url() str_or_None
        +get_release_base_url() str_or_None
        +get_installer_base_url() str_or_None
        +get_pypi_index_url() str_or_None
        +is_public_github_url(url) bool
        +release_metadata_public_lookup_blocked(url) bool
        +installer_public_download_blocked(url) bool
    }
    class SelfUpdateCommand {
        <<ClickCommand>>
        +self_update(check)
        -_get_update_installer_url() str
        -_get_manual_update_command() str
    }
    class VersionChecker {
        <<Module>>
        +get_latest_version_from_github(repo, timeout) str_or_None
        -_build_releases_api_url(url, repo) str
    }
    SelfUpdateCommand ..> BootstrapMirror : imports predicates
    VersionChecker ..> BootstrapMirror : imports predicates
    SelfUpdateCommand ..> VersionChecker : calls
    class BootstrapMirror:::touched
    class SelfUpdateCommand:::touched
    class VersionChecker:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A[apm self-update --check] --> B{get_release_metadata_url()}
    B -->|set| C[fetch mirrored metadata URL]
    B -->|unset| D{release_metadata_public_lookup_blocked()}
    D -->|true| E[error + exit 1]
    D -->|false| F[fetch api.github.com releases latest]
    C --> G{newer version?}
    F --> G
    G -->|no| H[up to date]
    G -->|yes + check only| I[print update available]
    G -->|yes + update| J{get_installer_base_url()}
    J -->|set| K[fetch mirror installer script]
    J -->|unset| L{installer_public_download_blocked()}
    L -->|true| M[error + exit 1]
    L -->|false| N[fetch public or GHES installer]
    K --> O[write temp script + execute]
    N --> O
Loading
sequenceDiagram
    participant User
    participant CLI as apm self-update
    participant BM as bootstrap_mirror
    participant VC as version_checker
    participant Net as Network
    User->>CLI: apm self-update --check
    CLI->>BM: get_release_metadata_url()
    BM-->>CLI: mirror URL or None
    CLI->>BM: release_metadata_public_lookup_blocked()
    BM-->>CLI: bool
    alt no mirror and public denied
        CLI-->>User: error + exit 1
    else mirror configured
        CLI->>VC: get_latest_version_from_github()
        VC->>Net: GET mirror URL
        Net-->>VC: latest.json
        VC-->>CLI: version string
    else public allowed
        CLI->>VC: get_latest_version_from_github()
        VC->>Net: GET api.github.com
        Net-->>VC: release JSON
        VC-->>CLI: version string
    end
Loading

Recommendation

Recommended next action: fix the version_checker.py Authorization guard and add the mirror-token regression test, then re-run CI. After that, the remaining findings are suitable follow-ups or docs polish.


Full per-persona findings

Python Architect

  • [recommended] bootstrap_mirror.py lives at package root instead of core/ or utils/ at src/apm_cli/bootstrap_mirror.py
    The APM module organization convention puts domain logic in src/apm_cli/core/ and cross-cutting helpers in src/apm_cli/utils/. bootstrap_mirror.py is a cross-cutting environment helper consumed by both commands/self_update.py and utils/version_checker.py. Placing it at the package root breaks the two-tier convention established by auth, locking, and resolution modules. This is low-risk today but compounds if more mirror helpers land.
    Suggested: Move to src/apm_cli/utils/bootstrap_mirror.py. Update import sites in commands/self_update.py and utils/version_checker.py.
  • [nit] build_mirrored_release_asset_url is exported but unused in this PR at src/apm_cli/bootstrap_mirror.py:84
    Dead code in a first PR increases the API surface that must be maintained. If it is intentionally pre-staged, a test or comment should document intent.
  • [nit] _get_manual_update_command duplicates mirror-vs-fallback branching for Unix and Windows at src/apm_cli/commands/self_update.py:75
    The Unix and Windows branches repeat the same try/except RuntimeError pattern. A small URL-display helper would remove duplication and keep the function declarative.
    Suggested: Extract a _resolve_installer_display_url(script) helper.

CLI Logging Expert

  • [recommended] logger.progress used for follow-up guidance instead of logger.info in the non-mirror fetch-failure branch at src/apm_cli/commands/self_update.py
    In self_update.py the non-mirror branch calls logger.progress as a follow-up to logger.error. The traffic-light rule says info is for actionable next-step hints, while progress is for lifecycle state transitions. The mirror branch correctly uses logger.info for the same role.
    Suggested: Replace logger.progress with logger.info for the remediation hint.
  • [recommended] install.sh mirror errors use raw echo patterns instead of a reusable error helper at install.sh
    The PR adds several mirror error sites that repeat raw echo and color escapes. A helper would enforce consistent outcome, redacted URL, and remediation structure across all mirror exit paths.
    Suggested: Extract a mirror_error() shell helper for redacted mirror errors and action hints.
  • [nit] APM_RELEASE_METADATA_URL override progress message omits the configured URL or host at src/apm_cli/commands/self_update.py
    The GITHUB_URL progress message includes the host for diagnostics, but the mirror metadata message only says mirrored metadata. A redacted URL or host would help verbose debugging.
  • [nit] install.sh pip mirror info message lacks the script's status prefix at install.sh
    The informational APM_PYPI_INDEX_URL line lacks the [i] or [>] prefix used elsewhere in installer output, creating a small visual grammar mismatch.
    Suggested: Prefix the info line with [i].

Devx UX Expert

  • [recommended] Manual update command in fail-closed mode prints an unexpanded env-var reference at src/apm_cli/commands/self_update.py
    When APM_NO_DIRECT_FALLBACK=1 is set without APM_INSTALLER_BASE_URL, _get_manual_update_command returns a command using $APM_INSTALLER_BASE_URL even though it is unset. A copy-paste recovery command should not fail in the exact state that produced the error.
    Suggested: Emit guidance to set APM_INSTALLER_BASE_URL before showing the templated command, or omit the non-functional command.
  • [recommended] No-egress smoke recipe is dense for the getting-started installation page at docs/src/content/docs/getting-started/installation.md
    The recipe is useful for enterprise operators, but its 40+ shell lines add high cognitive load to first-run installation docs. A collapsed section or dedicated enterprise deployment page would keep the getting-started page approachable.
    Suggested: Move the smoke recipe into a collapsed details block or link out to a dedicated enterprise guide.
  • [nit] Metadata JSON contract uses ambiguous 'usually' wording at docs/src/content/docs/reference/cli/self-update.md
    Mirror publishers need the exact minimum JSON shape. The docs say latest.json usually has at least tag_name, but do not formally define tag_name as required.
  • [nit] install.sh header examples omit two of the five locked env vars at install.sh
    The script header lists release base, PyPI, and fail-closed examples, but omits APM_RELEASE_METADATA_URL and APM_INSTALLER_BASE_URL. Listing all five improves discoverability for readers opening the script.

Supply Chain Security Expert

  • [blocking] version_checker.py sends GitHub token to APM_RELEASE_METADATA_URL mirror host at src/apm_cli/utils/version_checker.py:116
    install.sh guards token transmission with an empty APM_RELEASE_METADATA_URL check and install.ps1 fetches mirror metadata unauthenticated, but version_checker.py unconditionally attaches the GitHub token header to whatever _build_releases_api_url returns, including an operator-controlled mirror. A compromised or rogue mirror receives the user's GITHUB_APM_PAT, GITHUB_TOKEN, or GH_TOKEN.
    Suggested: Only attach Authorization when get_release_metadata_url() is None.
    Proof (passed): tests/unit/test_version_checker.py::TestAirGappedEnvVars::test_release_metadata_url_overrides_github_api_url -- proves: Mirror metadata URL is targeted, but the test does not assert Authorization header absence.
    self.assertEqual(parsed.hostname, 'mirror.corp.example')
  • [recommended] No test asserts token is withheld from mirror metadata requests in Python path at tests/unit/test_version_checker.py
    The shell scripts have explicit regression traps for not sending tokens to mirrors, but tests/unit/test_version_checker.py lacks an equivalent assertion for the Python metadata path.
    Suggested: Assert Authorization is absent from requests.get headers when APM_RELEASE_METADATA_URL is set.
  • [recommended] self_update.py executes mirror installer script without integrity verification at src/apm_cli/commands/self_update.py
    The installer script fetched from APM_INSTALLER_BASE_URL is written to disk and executed without checksum or signature verification. Release assets keep checksum verification, but the installer script itself lacks equivalent protection in this path.
    Suggested: Consider an install.sh.sha256 / install.ps1.sha256 mirror sidecar or document a tracked v0 limitation.
  • [nit] Mirror env URLs do not reject http:// schemes at src/apm_cli/bootstrap_mirror.py
    Mirror URLs are accepted without an https scheme check. Even after token scoping is fixed, metadata and assets could travel unencrypted if an operator configures http://.

OSS Growth Hacker

  • [recommended] CHANGELOG entry is feature-dense but not story-shaped for a release note at CHANGELOG.md
    The changelog line packs five concepts into one sentence. A reader scanning release notes needs a repostable hook such as installing behind a corporate firewall with no public egress.
    Suggested: Lead with the user benefit, then parenthetically list the five env vars and issue closure.
  • [nit] Enterprise section has no upstream funnel from README or quickstart at docs/src/content/docs/getting-started/installation.md
    Enterprise evaluators often land on README or quickstart first. A one-line callout to enterprise bootstrap mirrors would route them to the new proof material.
  • [nit] No-egress smoke recipe could invite actionable enterprise feedback at docs/src/content/docs/getting-started/installation.md
    The recipe is concrete proof material. A short note inviting proxy logs when gaps appear would convert enterprise validation into useful issues.

Auth Expert

  • [blocking] GitHub token sent to APM_RELEASE_METADATA_URL mirror host in version_checker.py at src/apm_cli/utils/version_checker.py:116
    version_checker.py unconditionally attaches Authorization: token to the URL returned by _build_releases_api_url, which now returns APM_RELEASE_METADATA_URL when set. The shell scripts correctly suppress tokens for mirror URLs. This can transmit GITHUB_APM_PAT, GITHUB_TOKEN, or GH_TOKEN to an operator-configured mirror host and violates the scope fence that installer tokens must not leak to mirror hosts.
    Suggested: Use headers = {"Authorization": f"token {token}"} if token and not get_release_metadata_url() else {}
  • [recommended] self_update.py lacks an explicit future guard against auth headers on mirror installer fetches at src/apm_cli/commands/self_update.py:211
    requests.get(install_script_url, timeout=10) currently sends no auth header, which is correct, but no nearby assertion or test prevents a future refactor from introducing session-level auth to mirror installer downloads.
    Suggested: Add a test or comment documenting that APM_INSTALLER_BASE_URL fetches must remain unauthenticated.
  • [nit] install.sh redaction does not cover token query parameters at install.sh
    redact_url_credentials redacts userinfo, but not query-token patterns such as SAS URLs. If operators use credentialed mirror URLs, error output may still reveal query credentials.

Doc Writer

  • [recommended] Fail-closed scoping paragraph is duplicated across installation.md and self-update.md at docs/src/content/docs/reference/cli/self-update.md:66
    The detailed semantics for public fallback, GHES coexistence, and token-to-mirror behavior now live authoritatively in two docs pages. When the scope evolves, these copies can drift independently. installation.md should be canonical; self-update.md should summarize and link.
    Suggested: Replace the full paragraph with a one-line summary plus a relative link to the installation enterprise mirror section.
  • [recommended] APM_RELEASE_BASE_URL row implies every asset has a .sha256 sidecar, but only Windows verifies one at docs/src/content/docs/getting-started/installation.md:97
    installation.md describes assets as {base}/{tag}/{asset} and {asset}.sha256, which reads as universal. install.sh does not fetch or verify .sha256 for Unix archives; install.ps1 verifies the Windows zip sidecar. The layout example is more accurate than the table text.
    Suggested: Clarify that the Windows zip needs a .sha256 sidecar verified by install.ps1, while Unix archives are not checksum-verified.
  • [nit] self-update.md opens with GitHub-only framing despite mirror metadata support at docs/src/content/docs/reference/cli/self-update.md:3
    The page description and behavior text say latest GitHub release and fetches from GitHub, while APM_RELEASE_METADATA_URL can provide metadata without GitHub contact. Enterprise readers should see mirror support in the first sentence.
  • [nit] No-egress smoke recipe hardcodes /usr/bin/pip in the pip wrapper at docs/src/content/docs/getting-started/installation.md:183
    /usr/bin/pip often does not exist on runners or venv-based setups. The happy path may not reach pip, but adapted recipes could fail with a confusing not found error.

Test Coverage Expert

  • [recommended] No test exercises the installer-download fail-closed exit path in apm self-update at tests/unit/commands/test_self_update_air_gapped.py
    self_update.py raises RuntimeError and exits 1 when APM_NO_DIRECT_FALLBACK=1, metadata mirror is set, and APM_INSTALLER_BASE_URL is unset. This is distinct from the metadata-lookup block, which is tested. Searches for installer_public_download_blocked and APM_INSTALLER_BASE_URL not configured found no direct test for this branch.
    Suggested: Add test_no_direct_fallback_blocks_installer_download_without_mirror with APM_RELEASE_METADATA_URL set and APM_INSTALLER_BASE_URL omitted; assert exit_code == 1 and output names APM_INSTALLER_BASE_URL.
    Proof (missing): tests/unit/commands/test_self_update_air_gapped.py::TestEnterpriseBootstrapSelfUpdate::test_no_direct_fallback_blocks_installer_download_without_mirror -- proves: apm self-update in fail-closed mode exits non-zero with actionable guidance when installer mirror is not configured
    assert result.exit_code == 1; assert 'APM_INSTALLER_BASE_URL' in result.output

Performance Expert -- inactive

Touched files (bootstrap_mirror.py, self_update.py, version_checker.py, install.sh, install.ps1, docs, tests) are in the one-shot installer/self-update path, not the resolve/fetch/materialize/verify hot loop; no dependency-pipeline cache, transport, or materialization surface is affected.

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

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

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_with_followups

Enterprise bootstrap mirrors route APM install/update traffic through internal hosts, with validated fail-closed mode and the Python auth-token scoping fix now preventing credential leakage to mirrors.

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

All eight active panelists converge on ship. The auth-token scoping fix -- the highest-severity item from the prior round -- is now folded with a mutation-break-validated test (TestGitHubVersionFetchAuth::test_token_header_scoped_to_public_release_metadata_request) that failed before the fix, passed after it, and failed again when the guard was removed. No panelist raised a blocking-severity finding; the strongest signals are recommended-tier follow-ups around doc duplication, install.sh error-style consistency, and env-var discoverability in apm self-update --help.

The test-coverage expert flagged bootstrap_mirror.py lacking a dedicated unit test file. Transitive coverage through test_version_checker.py and test_self_update_air_gapped.py is adequate for v0, but this secure-by-default surface deserves a follow-up regression-trap file so env-var parsing cannot drift silently.

Dissent. No true dissent. The auth expert noted GHES metadata URLs still receive tokens when no mirror override is set; the panel treats that as intentional GHES behavior, not a code issue.

Aligned with: Secure by default: token scoping keeps credentials off mirror endpoints and fail-closed mode prevents unintended public egress; Governed by policy: five env vars give operators declarative bootstrap network control; Multi-harness multi-host: install.sh, install.ps1, and Python self-update share the mirror contract; Pragmatic as npm: env-var mirror config matches pip/npm/cargo operator mental models.

Growth signal. This is APM's first air-gapped install path. The launch story is strong: APM ships native fail-closed enterprise mirrors -- five env vars, no public fallback.

Panel summary

Persona B R N Takeaway
Python architect 0 0 2 Clean procedural module for mirror env-var contract; auth scoping fix is correct; no architectural faults.
CLI logging expert 0 0 2 Mirror output uses CommandLogger correctly with actionable messages; credential redaction is thorough; one minor semantic mismatch in logger method choice.
DevX UX expert 0 2 3 Mirror env contract is well-designed with actionable errors; minor discoverability and error consistency nits.
Supply chain security expert 0 0 2 Token-scoping fix is sound; mirror contract fails closed correctly; no credential exfiltration paths found.
OSS growth hacker 0 1 2 Enterprise mirror mode unlocks a previously closed-off adoption segment; docs carry a strong copy-paste smoke recipe. Ship.
Auth expert 0 1 2 Token-scoping fix correctly prevents credential leakage to mirrors; no blocking auth concerns remain.
Doc writer 0 1 1 Docs are accurate and correctly reflect the token-scoping fix; main concern is fail-closed scoping prose triplicated across three pages.
Test coverage expert 0 0 1 All four critical user-promise surfaces have regression-trap tests at unit tier; auth-token scoping fix has a dedicated mutation-break-validated test; ship.

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

Top 5 follow-ups

  1. [Test coverage expert] Add dedicated tests/unit/test_bootstrap_mirror.py with parametrized edge-case coverage for env-var parsing and URL joining -- module sits on a secure-by-default surface; transitive coverage is adequate for ship but a dedicated file prevents silent drift on the parsing contract.
  2. [Doc writer] Deduplicate fail-closed scoping prose: keep getting-started/installation.md canonical, trim self-update.md and apm-guide to cross-references -- three near-identical copies will drift on the next change.
  3. [DevX UX expert] Add an Environment variables epilog to apm self-update --help listing the five APM_* mirror vars -- when a user hits the fail-closed error, --help is their first impulse.
  4. [OSS growth hacker] Add a positioning hook sentence before the env-var table in enterprise docs and name APM_NO_DIRECT_FALLBACK in the CHANGELOG entry -- enterprise ops teams scan changelogs by keyword.
  5. [Auth expert] Document that GHES metadata URLs intentionally receive tokens when no mirror override is set; consider follow-up to unify version_checker token resolution with AuthResolver -- this clarifies the auth boundary between GHES and mirror paths.

Architecture

classDiagram
    direction LR
    class bootstrap_mirror {
        <<Module / Pure Functions>>
        +get_env_url(name) str | None
        +env_flag_enabled(name) bool
        +no_direct_fallback_enabled() bool
        +append_url_path(base_url, *parts) str
        +get_release_metadata_url() str | None
        +get_release_base_url() str | None
        +get_installer_base_url() str | None
        +get_pypi_index_url() str | None
        +is_public_github_url(github_url) bool
        +release_metadata_public_lookup_guard(github_url) bool
        +installer_public_download_guard(github_url) bool
        +build_mirrored_release_asset_url(tag, asset) str | None
    }
    class version_checker {
        <<Module / IOBoundary>>
        +get_latest_version_from_github(repo, timeout) str | None
        -_build_releases_api_url(github_url, repo) str
        -_get_github_token() str | None
    }
    class self_update {
        <<Click Command>>
        +self_update(check) None
        -_get_update_installer_url() str
        -_log_no_direct_metadata_error(logger) None
    }
    class CommandLogger {
        <<Base Class>>
        +progress(msg)
        +error(msg)
        +info(msg)
    }
    note for bootstrap_mirror "Strategy via env vars: mirror URL -> GHES -> public GitHub"
    version_checker ..> bootstrap_mirror : imports predicates
    self_update ..> bootstrap_mirror : imports URL builders + predicates
    self_update ..> version_checker : calls get_latest_version_from_github
    self_update ..> CommandLogger : uses
Loading
flowchart TD
    A["apm self-update --check"] --> B{"release_metadata_public_lookup_guard()"}
    B -- yes --> C["log actionable mirror error and exit"]
    B -- no --> D{"get_release_metadata_url() returns mirror URL?"}
    D -- yes --> E["requests.get(mirror_url), no Authorization header"]
    D -- no --> F["api.github.com or GHES metadata URL"]
    F --> G["requests.get(github_url), Authorization if token present"]
    E --> H{"latest_version obtained?"}
    G --> H
    H -- no --> I["mirror-specific or generic error"]
    H -- yes --> J{"is_newer_version?"}
    J -- no --> K["already up to date"]
    J -- yes --> L["update available or fetch installer"]
Loading

Recommendation

Ship this PR as-is. The auth-token scoping fix is proven by a mutation-break test, all panelists converge, and no blocking-severity findings exist. Track the dedicated test_bootstrap_mirror.py file and the doc-deduplication pass as immediate follow-ups. The growth-hacker's CHANGELOG and hook-sentence suggestions are low-effort, high-signal tweaks worth folding before or immediately after merge at the maintainer's discretion.


Full per-persona findings

Python architect

  • [nit] get_release_metadata_url() called twice in the hot path of get_latest_version_from_github at src/apm_cli/utils/version_checker.py
    In version_checker.py, get_release_metadata_url() is called once inside _build_releases_api_url and again at the call site. Both are cheap os.environ reads, but a local variable would make the contract clearer: URL came from mirror therefore no token.
    Suggested: Assign mirror_url = get_release_metadata_url() once before the _build_releases_api_url call, pass it through or use it for both the URL override and the auth guard.
  • [nit] bootstrap_mirror.py VERSION constant shadows the semantic VERSION concept at src/apm_cli/bootstrap_mirror.py:10
    The module exports VERSION = "VERSION" as a string constant naming the env var. A name like VERSION_ENV_VAR would be clearer, though the current form is tolerable.

CLI logging expert

  • [nit] Fallback error remediation uses logger.progress instead of logger.info at src/apm_cli/commands/self_update.py:177
    The new mirror-failure path correctly uses logger.info() for remediation hints, but the existing non-mirror path uses logger.progress for the same semantic purpose.
    Suggested: Change logger.progress("Please check your internet connection or try again later") to logger.info("Check your internet connection or try again later.") for consistency.
  • [nit] install.sh pip mirror message lacks actionable fix on success path at install.sh
    The message tells the user what is happening but does not name the resolved mirror host. Minor since the URL might contain credentials and the redact helper exists.

DevX UX expert

  • [recommended] Error messages in install.sh use inconsistent prefixes vs self_update.py, hurting muscle memory at install.sh
    The content is actionable, but shell and Python paths use visually distinct error styles for the same conceptual failure.
    Suggested: Consider extracting the error preamble into a shell helper so fail-closed guards use one structure.
  • [recommended] No --help or env-var listing in CLI for the new mirror variables; discoverability relies entirely on docs at src/apm_cli/commands/self_update.py
    A user hitting apm self-update --help sees options but no mention of the five new env vars.
    Suggested: Add an Environment variables epilog to the self_update click command docstring listing the five APM_* vars and GITHUB_URL/APM_REPO/VERSION.
  • [nit] Manual install fallback prints '/install.ps1' literally in fail-closed mode at src/apm_cli/commands/self_update.py
    The placeholder is useful but can look like a rendering bug.
    Suggested: Consider phrasing as Set APM_INSTALLER_BASE_URL and re-run without embedding a pseudo-URL.
  • [nit] Docs smoke recipe uses test $? -eq 1 which is fragile if exit code changes at docs/src/content/docs/getting-started/installation.md
    A future non-zero code other than 1 would make the documented recipe brittle.
    Suggested: Document as || test $? -ne 0 or note that any non-zero exit is expected.
  • [nit] install.sh helper is_public_github_url has no fallback for unset GITHUB_URL at install.sh
    Correct because GITHUB_URL defaults earlier, but non-obvious to maintainers.

Supply chain security expert

  • [nit] install.ps1 first metadata fetch is always unauthenticated, even for GHES private repos at install.ps1:484
    For a private GHES repo without a mirror, this means an unnecessary unauthenticated round-trip before auth retry. Not a security issue.
  • [nit] append_url_path does not validate scheme or reject path-traversal segments in env var values at src/apm_cli/bootstrap_mirror.py:42
    Defense-in-depth only, because the operator controls the env.
    Suggested: Add a guard rejecting . or .. path segments in mirror URL path parts.

OSS growth hacker

  • [recommended] Add a one-liner hook sentence before the env-var table in the enterprise section at docs/src/content/docs/getting-started/installation.md
    A single positioning sentence gives enterprise evaluators a quotable claim to paste into internal RFCs.
    Suggested: Add before the env-var table: APM ships native enterprise mirror support -- five env vars route all bootstrap traffic through internal hosts, with fail-closed mode ensuring no public fallback.
  • [nit] CHANGELOG entry could name the five env vars for scanners at CHANGELOG.md
    Enterprise ops teams scan changelogs by keyword. Mentioning APM_NO_DIRECT_FALLBACK makes it grep-findable.
  • [nit] The apm-guide skill doc duplicates the table without linking to the canonical reference at packages/apm-guide/.apm/skills/apm-usage/installation.md
    Two copies of the env-var table will drift.

Auth expert

  • [recommended] GHES metadata URL still receives Authorization header even when APM_RELEASE_METADATA_URL is unset at src/apm_cli/utils/version_checker.py:114
    This is intentional for GHES but worth documenting so operators understand that mirrors and GHES have different auth boundaries.
  • [nit] version_checker._get_github_token does not use AuthResolver at src/apm_cli/utils/version_checker.py:57
    This pre-dates this PR and is acceptable for the version-check hot path, but a future AuthResolver unification could improve per-host scoping.
  • [nit] install.ps1 reads GITHUB_APM_PAT but bootstrap_mirror.py does not surface it for shell scripts at install.ps1
    Low risk since mirror URLs skip the GitHub auth branch, but a static PowerShell assertion could increase confidence.

Doc writer

  • [recommended] Fail-closed scoping explanation and env-var table are duplicated near-verbatim across self-update.md, getting-started/installation.md, and the apm-guide skill at docs/src/content/docs/reference/cli/self-update.md:66
    Three copies of the same contract drift independently on the next change and add doc bloat.
    Suggested: Keep getting-started/installation.md as canonical. In self-update.md, trim the duplicated scoping paragraph to one sentence and link to the installation page; reduce apm-guide prose to a cross-reference.
  • [nit] self-update.md APM_RELEASE_BASE_URL row leans on implementation detail at docs/src/content/docs/reference/cli/self-update.md:44
    The reader cares that asset downloads route through the mirror, not subprocess inheritance.
    Suggested: Reword to: Base URL containing release assets at {base}/{tag}/{asset}. Used when self-update fetches the binary archive.

Test coverage expert

  • [nit] bootstrap_mirror.py has no dedicated unit test file; helpers are only tested transitively at src/apm_cli/bootstrap_mirror.py
    The new module is exercised through consumer tests, but a dedicated parametrized file would prevent env-var parsing drift.
    Suggested: Add tests/unit/test_bootstrap_mirror.py with parametrized tests for get_env_url edge cases and append_url_path joining logic.
    Proof (missing at): tests/unit/test_bootstrap_mirror.py::test_get_env_url_strips_quotes_and_trailing_slash -- proves: Env-var parsing strips quotes, whitespace, and trailing slashes before returning a canonical URL to callers. [secure-by-default,devx]
    assert get_env_url('X') == 'https://mirror.corp.example/path'

Performance expert -- inactive

PR touches bootstrap installer URL routing and version-check endpoint selection; no dependency resolution, fetch protocol, materialization, or cache-layer hot path is affected.

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 d6fd0ee into main Jun 11, 2026
15 checks passed
@danielmeppiel danielmeppiel deleted the danielmeppiel/1680-enterprise-bootstrap-mirror branch June 11, 2026 10:04
danielmeppiel added a commit that referenced this pull request Jun 11, 2026
Salvage the apm-review-panel follow-up delta that landed after PR #1733 was squash-merged for issue #1680.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Jun 11, 2026
Salvage the apm-review-panel follow-up delta that landed after PR #1733 was squash-merged for issue #1680.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Jun 11, 2026
#1733) (#1745)

* feat(bootstrap): fold enterprise mirror follow-ups

Salvage the apm-review-panel follow-up delta that landed after PR #1733 was squash-merged for issue #1680.

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

* fix(bootstrap): fold panel review follow-ups

Fold the first advisory panel's in-scope findings for the enterprise bootstrap mirror follow-up PR.

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

* docs(bootstrap): tighten mirror follow-up guidance

Fold remaining advisory polish around mirror help text, changelog wording, and agent-facing verification guidance.

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

* fix(bootstrap): fold second panel polish

Fold the second advisory pass findings around version-check resolver locking, doc anchors, and fail-closed guidance.

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

---------

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.

[FEATURE] Enterprise bootstrap mirror mode for installing and updating the APM CLI through Artifactory-compatible proxies

2 participants