Repository navigation
Conversation
…ontents API call Virtual-package validation on public github.com repos spent anonymous Contents API calls per marker file before discovering the layout, and `apm marketplace add` re-fetched the catalog immediately after the discovery probe had just downloaded it. On the default 60 req/h anonymous budget a single Claude-plugin-layout install could burn 4+ calls (validation + registration combined). - validate_virtual_package_exists: probe every marker path through the raw CDN first (zero API calls) before falling back to the existing per-marker Contents API probes; fallbacks and non-public hosts are unchanged - download_raw_file: new raw_only pass-through for CDN-only probes - marketplace add: reuse the catalog response captured during path discovery instead of re-fetching it (sidecar cache semantics kept) - tests: 12 new unit tests covering sweep ordering, API-spend assertions, fallback retention, and catalog reuse Fixes microsoft#3283
ShaoLOWu
requested review from
Daniel Meppiel (danielmeppiel) and
Sergio Sisternes (sergio-sisternes-epam)
as code owners
October 22, 2026 12:09
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Incorrect and incomplete test mocks cause assertion failures and unintended network requests.
3 open issues
What changed in this PR
This PR addresses #3283 by reducing redundant GitHub API requests during virtual-package validation and marketplace registration.
Changes:
- Sweep package markers through the raw CDN before allowing Contents API fallback.
- Reuse the discovered marketplace catalog while preserving cache writes.
- Add regression tests and update existing discovery mocks.
| File | Description |
|---|---|
tests/unit/marketplace/test_marketplace_commands.py |
Updates discovery mocks to return path/payload tuples. |
tests/unit/marketplace/test_catalog_discovery_reuse.py |
Tests payload reuse, refresh behavior, and cache writes. |
tests/unit/deps/test_virtual_package_cdn_first_sweep.py |
Tests CDN-first validation and fallback behavior. |
tests/unit/deps/test_github_downloader_validation.py |
Allows keyword arguments in an existing download mock. |
src/apm_cli/marketplace/client.py |
Returns discovery payloads and supports reusing them. |
src/apm_cli/deps/github_downloader.py |
Passes the raw_only option through downloader methods. |
src/apm_cli/deps/github_downloader_validation.py |
Adds the CDN-only marker sweep before legacy probes. |
src/apm_cli/deps/download_strategies.py |
Prevents API fallback for raw-only requests. |
src/apm_cli/commands/marketplace/__init__.py |
Passes discovered catalog data into marketplace fetching. |
Review effort: Balanced
| discovered_manifest = None | ||
| else: | ||
| detected_path = _auto_detect_path(probe_source) | ||
| detected_path, discovered_manifest = _auto_detect_path_with_payload(probe_source) |
| "uses_public_github_anonymous_first", | ||
| return_value=False, | ||
| ), | ||
| patch.object(downloader._strategies, "try_raw_download") as cdn_mock, |
|
|
||
| with ( | ||
| patch.object(downloader, "download_raw_file", side_effect=wrapper), | ||
| patch.object(downloader._strategies, "try_raw_download", return_value=None), |
Author
|
@microsoft-github-policy-service agree |
1 similar comment
Author
|
@microsoft-github-policy-service agree |
…de-network sweep tests Addresses Copilot review feedback on microsoft#3348: - Retarget the traceback tests (phase3/surface) and the five wave7 patch sites from _auto_detect_path to _auto_detect_path_with_payload, returning (path, payload) tuples so discovery stays mocked instead of executing the real fetch path. - test_non_public_host_skips_sweep: use an actual non-public-host dependency and simulate legacy per-marker misses on the download strategy seam (a bare MagicMock return was accepted as file content), stubbing the directory and ls-remote fallbacks at the validation module boundary so the host boundary is checked fully offline. - test_virtual_file_probe_keeps_legacy_semantics: mock the auth callback and return controlled API 404s (matching the full-miss test) so the single-file probe never attempts real GitHub requests. Sweep test suite runtime drops from ~57s (network-dependent) to ~3s. Verified: 6/6 sweep tests green; 332 tests across the touched marketplace/deps suites pass; tests/unit/deps failure set identical to the pre-change Windows baseline (7 pre-existing symlink/routing failures); ruff clean.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the maintainer-approved scope from #3283 (apm-scope:v1): stop spending anonymous Contents API calls during virtual-package validation and marketplace registration on public github.com sources.
1. Virtual-package validation: raw CDN first sweep
validate_virtual_package_existspreviously probed marker files one at a time, and each miss on a public github.com repo escalated to an anonymous Contents API call. A Claude-plugin-layout repo (plugin.jsondeep in the marker list) could burn 4+ calls before the layout was found — a large share of the 60 req/h anonymous budget.raw.githubusercontent.com, no rate limit) via a newraw_onlypass-through ondownload_raw_file/download_github_file. A single hit validates the package with zero API calls.GitHubThrottleErrorstill propagates untouched.2. Marketplace add: reuse the discovery response
_auto_detect_pathprobes candidate locations, thenfetch_marketplace(force_refresh=True)immediately re-fetched the same catalog it had just downloaded._auto_detect_path_with_payloadreturns(path, data); the command layer passes the captured payload tofetch_marketplace(discovery_payload=...), which skips the redundant network round-trip.force_refresh/freshness semantics are identical.Testing
New unit tests (12):
tests/unit/deps/test_virtual_package_cdn_first_sweep.py(6): sweep hits every marker raw-only; first-hit success spends zero Contents API calls; full-miss path escalates to API and keeps fallback behavior; non-public hosts skip the sweep; single-file virtual packages unaffected.tests/unit/marketplace/test_catalog_discovery_reuse.py(6): discovery payload is reused (no second fetch); explicitforce_refreshstill fetches; cache file still written.Local run (Python 3.13, Windows): all new tests pass; the pre-existing failure set (
tests/unit/deps/,test_github_downloader.py,test_virtual_package_multi_install.py) is byte-identical before and after this change (13 Windows/Unix-environment failures: symlinks, GitLab/Gitea routing,/dev/null— none touched by this PR).ruff checkclean on all modified files.AI-Generated PR disclosure
This PR was authored with AI assistance (Claude-based coding agent working under human direction): issue analysis, fix design, implementation, and test authoring were agent-assisted; scope was user-approved against the maintainer's approved-scope comment on #3283 before implementation. All changes were validated locally as described above, and the human reviewed and confirmed the final diff before submission.
Fixes #3283