feat(bootstrap): fold panel follow-ups for enterprise mirror mode (post-#1733)#1745
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reapplies the post-merge follow-ups for the enterprise bootstrap mirror mode work (installers + apm self-update), tightening mirror safety/consistency, improving auth behavior in constrained environments, expanding unit coverage, and consolidating documentation.
Changes:
- Add dedicated unit coverage for bootstrap mirror helpers and tighten URL path handling (dot-segment rejection).
- Route version-check token resolution through
AuthResolverwith external-helper fallback disabled; ensure Windows installer’s first GHES metadata fetch uses auth. - Improve fail-closed UX (self-update help epilog, manual guidance) and dedupe docs while keeping the v0 mirror env var contract explicit.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_version_checker.py | Adds tests asserting version-check token resolution uses AuthResolver and avoids probing external helpers. |
| tests/unit/test_enterprise_bootstrap_installers.py | Adds installer-focused regression tests (PowerShell GHES auth-first, install.sh fail-closed helper consistency). |
| tests/unit/test_bootstrap_mirror.py | New unit tests for env URL normalization, URL joining, and dot-segment rejection. |
| tests/unit/commands/test_self_update_air_gapped.py | Extends air-gapped self-update tests for fail-closed manual guidance and help discoverability. |
| src/apm_cli/utils/version_checker.py | Uses AuthResolver (no external fallback) for token resolution; threads mirror metadata override through URL builder. |
| src/apm_cli/core/auth.py | Adds allow_external_fallback toggle to prevent gh/git helper probing for non-blocking checks. |
| src/apm_cli/commands/self_update.py | Improves manual fail-closed guidance and expands --help text with mirror env vars; small log UX tweaks. |
| src/apm_cli/bootstrap_mirror.py | Validates URL path parts to reject traversal/dot segments; minor constant rename for clarity. |
| packages/apm-guide/.apm/skills/apm-usage/installation.md | Dedupes the mirror-mode explanation and points to canonical docs section. |
| install.sh | Centralizes fail-closed error messaging via a helper and tweaks public-GitHub detection defaulting. |
| install.ps1 | Ensures first GHES metadata fetch uses auth headers (no initial unauthenticated request). |
| docs/src/content/docs/reference/cli/self-update.md | Clarifies mirror env var semantics and points to canonical fail-closed/smoke-recipe docs. |
| docs/src/content/docs/getting-started/installation.md | Updates mirror-mode intro text and makes smoke-recipe failure assertion clearer/robust. |
| CHANGELOG.md | Expands enterprise mirror mode entry to include the canonical env var names. |
Copilot's findings
- Files reviewed: 14/14 changed files
- Comments generated: 2
| def test_manual_command_no_direct_fallback_unix_uses_env_reference(self) -> None: | ||
| """Fail-closed manual Unix guidance should reference the mirror env var.""" |
| def test_manual_command_no_direct_fallback_windows_uses_env_reference(self) -> None: | ||
| """Fail-closed manual Windows guidance should reference the mirror env var.""" |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | AuthResolver extension is clean Strategy+guard; module-level singleton with RLock and clear_cache is well-shaped. Ship. |
| CLI Logging Expert | 0 | 0 | 0 | All prior logging findings folded cleanly; fail_closed_error is POSIX-safe, help epilog fits 80 cols, logger.info for guidance is correct semantic level, manual-update fallback names the fix. Ship. |
| DevX UX Expert | 0 | 0 | 0 | All DevX UX follow-ups folded; no remaining CLI surface, error wording, or flow concerns. Ship. |
| Supply Chain Security Expert | 0 | 0 | 0 | Token-boundary guards correct (mirror URLs never receive credentials), dot-segment validation uses canonical path_security, AuthResolver least-privilege via allow_external_fallback=False is sound. Ship. |
| OSS Growth Hacker | 0 | 0 | 0 | Prior growth follow-ups folded; remaining changes improve discoverability and reduce doc drift. Ship. |
| Auth Expert | 0 | 0 | 0 | All prior auth findings folded correctly; token-boundary, fallback-suppression, and resolver threading invariants hold. Ship. |
| Doc Writer | 0 | 0 | 0 | All folded doc edits verified: canonical mirror section single-sourced, cross-refs resolve, CHANGELOG/anchors correct. Ship. |
| Test Coverage Expert | 0 | 0 | 0 | All behavior changes carry regression-trap tests; mutation-break killed the AuthResolver allow_external_fallback guard; 82 focused tests pass. Ship. |
| Performance Expert | 0 | 0 | 0 | AuthResolver with allow_external_fallback=False avoids subprocess RTTs on version checks; clear_cache per call is noise at 1 invocation/run. Ship. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction LR
class AuthResolver {
<<Strategy>>
+resolve(host, org) AuthContext
+clear_cache() None
-_allow_external_fallback bool
-_cache dict
-_lock Lock
}
class GitHubTokenManager {
<<Collaborator>>
+get_token_for_purpose(purpose) str
+resolve_credential_from_gh_cli(host) str
+resolve_credential_from_git(host) str
}
class AuthContext {
<<ValueObject>>
+token str
+source str
}
class VersionChecker {
<<Module>>
+_get_github_token(url, repo) str
+get_latest_version_from_github(repo, timeout) str
-_VERSION_CHECK_AUTH_RESOLVER AuthResolver
-_VERSION_CHECK_AUTH_RESOLVER_LOCK RLock
}
class BootstrapMirror {
<<Module>>
+append_url_path(base, parts) str
+get_release_metadata_url() str
+release_metadata_public_lookup_blocked(url) bool
}
class PathSecurity {
<<Guard>>
+validate_path_segments(segment, context) None
}
AuthResolver *-- GitHubTokenManager : delegates
AuthResolver ..> AuthContext : returns
VersionChecker *-- AuthResolver : singleton instance
VersionChecker ..> BootstrapMirror : reads mirror config
BootstrapMirror ..> PathSecurity : validates URL parts
note for AuthResolver "allow_external_fallback=False\nskips gh-cli and git-credential steps"
note for VersionChecker "RLock guards singleton;\nclear_cache before each resolve"
class AuthResolver:::touched
class VersionChecker:::touched
class BootstrapMirror:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["CLI startup / self-update --check"] --> B["version_checker.get_latest_version_from_github()"]
B --> C{"release_metadata_public_lookup_blocked?"}
C -- yes --> D["return None"]
C -- no --> E["_build_releases_api_url(github_url, repo, metadata_url)"]
E --> F["_get_github_token(github_url, repo)"]
F --> G["[LOCK] RLock acquire"]
G --> H["_get_version_check_auth_resolver() singleton"]
H --> I["resolver.clear_cache()"]
I --> J["resolver.resolve(host, org)"]
J --> K{"allow_external_fallback?"}
K -- False --> L["skip gh-cli / git-credential"]
L --> M["return env token or None"]
K -- True --> N["probe gh-cli then git-credential"]
M --> O["[NET] urllib.request with token header"]
O --> P["parse tag_name from JSON response"]
P --> Q["return version string"]
subgraph bootstrap_mirror
E2["append_url_path(base, parts)"] --> V["[FS] validate_path_segments"]
V --> W{"dot segment?"}
W -- yes --> X["raise ValueError"]
W -- no --> Y["join and return URL"]
end
Recommendation
Merge at maintainer discretion. All panelists ship-clean, 82 focused tests pass, mutation-break confirms the security gate is load-bearing, and PR checks are green. No follow-ups to track.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert
No findings.
Doc Writer
No findings.
Test Coverage Expert
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
danielmeppiel
force-pushed
the
danielmeppiel/1680-bootstrap-mirror-followups
branch
from
e776499 to
e513b52
Compare
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>
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>
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>
danielmeppiel
force-pushed
the
danielmeppiel/1680-bootstrap-mirror-followups
branch
from
e513b52 to
3de1bc6
Compare
Summary
This PR salvages the apm-review-panel follow-ups for #1680 that were folded after #1733 had already been squash-merged by a human. The merge landed at head 0251f0f, while the fold pass continued on the old branch through a69632f; this fresh branch reapplies only that follow-up delta on current main.
Salvaged follow-ups
bootstrap_mirrorunit tests.install.shfail-closed helper consistency.install.ps1first GHES metadata auth.Scope fence
apm doctor network.Validation
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.shuv run --extra dev pytest tests/unit/test_bootstrap_mirror.py tests/unit/test_version_checker.py tests/unit/test_enterprise_bootstrap_installers.py tests/unit/commands/test_self_update_air_gapped.py -qstr(path.relative_to(...))CI guards.