GeezerVison is showing github.com — fix(deps): report the proxy archive rejection instead of a later HTTP 404 by MohammedAlkindi · Pull Request #3375 · microsoft/apm
Conversation
Description
Behind PROXY_REGISTRY_URL, a rejected proxy archive (symlink member or non-zip body) was followed by fallback 404s, so the error read "Last error: HTTP 404". The download now keeps the first rejection and reports it as Archive error: when no URL succeeds. A later valid archive still wins.
Issue and approved scope
Issue: Refs #3110
Human scope-approval comment: #3110 (comment)
Does this PR complete the issue, or what remains? No: diagnostic slice only; symlink handling stays deferred.
Type of change
- Bug fix
- New feature
- Documentation
- Maintenance / refactor
Testing
- Tested locally
- All existing tests pass (one existing exec-bit test fails on Windows on main too)
- Added tests for new functionality (if applicable)
Spec conformance (OpenAPM v0.1)
- Spec edit:
docs/src/content/docs/specs/openapm-v0.1.md updated (new/changed <a id="req-XXX"></a> anchor + prose + Appendix C row).
- Manifest edit:
docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.yml updated.
- Test edit: a
@pytest.mark.req("req-XXX") test under tests/spec_conformance/ added or extended.
-
CONFORMANCE.{md,json} regenerated via uv run --extra dev python -m tests.spec_conformance.gen_statement and committed.
- N/A -- this PR does not change OpenAPM-observable behaviour.
Partial: Refs #3110 -- delivers only the apm-scope:v1 diagnostic-only slice (report the original proxy archive rejection instead of a later HTTP 404). Remaining in #3110 and deliberately not closed here: safe symlink materialization for proxy archives (still deferred per the scope record).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
When a registry proxy URL returns 200 but the archive is rejected (unsafe
member such as a symlink, or a body that is not a zip), the loop moved on
to the remaining fallback URLs and their 404s overwrote the cause, so the
error ended in "Last error: HTTP 404". Track the first archive rejection
separately and report it when no URL succeeds. A later valid archive
still wins.
Refs microsoft#3110
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thank you for contributing this diagnostic-only correction. Issue #3110 has an explicit maintainer-approved scope for reporting the original proxy archive rejection. The requested reviewers include Daniel Meppiel (@danielmeppiel) and Sergio Sisternes (@sergio-sisternes-epam). The remaining symlink-handling design is still deferred, so this PR does not close the full issue. This is a recommendation for human review only, not merge approval.
Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Advisory panel review (not merge approval)
Headline: Surfaces the real archive-rejection diagnostic instead of a misleading HTTP 404 (diagnostic slice of #3110; symlink handling stays deferred).
panel-mode=full; personas=supply-chain-security-expert, test-coverage-expert, doc-writer, apm-ceo
Ship recommendation: ship with follow-ups. The change is correct and well scoped. Neither follow-up below gates it.
Panel summary
| Persona |
Blocking |
Recommended |
Nit |
Summary |
| Supply chain security |
0 |
0 |
0 |
Diagnostic only. Containment and symlink refusal are unchanged, and no tokens appear in the error text. |
| Test coverage |
0 |
1 |
0 |
The two core regression traps are solid. The 404 then rejection ordering is untested. |
| Doc writer |
0 |
2 |
1 |
The docs sentence overstates the non-zip case. |
Recommended follow-ups
- Tests: add one parametrize case for the order 404, then archive rejection, then 404s. The code is correct today, since
archive_error = archive_error or last_error keeps the first rejection. The case would lock that ordering against later refactors of the loop. (tests/unit/test_artifactory_support.py)
- Docs:
enterprise/registry-proxy.md says the error reports the archive's "URL and reason". The non-zip path (Invalid zip archive from <url>) has no reason, so reword to "URL (and the reason, where available)".
- Nit, author's call: the second CHANGELOG sentence ("containment checks are unchanged...") is reassurance. It could be trimmed.
Arbitration
No dissent among panelists. Size-limit rejections also go through ArchiveError, so they now surface as Archive error:. That is sensible.
Existing CODEOWNERS reviewers (@danielmeppiel, @sergio-sisternes-epam) remain the human decision makers.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.
Generated by PR Review Panel for #3375 · copilot · auto · 191.2 AIC · ⌖ 0.543 AIC · ⊞ 5.8K · ◷
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The regression traps covered a rejected archive at the first URL only. Add cases for a 404 before the rejection and for two rejections, so a later refactor of the fallback loop cannot report a later cause or a 404 instead of the first archive rejection. Mutation-checked against last-wins and dropped-capture variants. Addresses panel follow-up 1 (test-coverage-expert), refs microsoft#3110.
Co-authored-by: Copilot App <[email protected]>
The non-zip path reports 'Invalid zip archive from <url>' with no reason, so the guide no longer promises a reason for every archive rejection. Addresses panel follow-up 2 (doc-writer), refs microsoft#3110.
Co-authored-by: Copilot App <[email protected]>
Merging main left two Fixed headings under Unreleased; move the entry into the existing list and trim the reassurance clause to the one fact readers need (symlinks are still refused). Addresses panel nit 3, refs microsoft#3110.
Co-authored-by: Copilot App <[email protected]>
APM Review Panel: ship_now
Diagnostic-only fix surfaces the real proxy archive rejection instead of a misleading fallback 404 -- all three prior follow-ups folded, both panelists clean.
panel-mode=lean; personas=supply-chain-security-expert,test-coverage-expert,apm-ceo
cc Mohammed Alkindi (@MohammedAlkindi) -- a fresh advisory pass is ready for your review.
Both panelists converge with zero findings. The supply-chain-security-expert confirms no credential leakage, no containment regression, and all extraction guards intact (archive URLs are path-only, auth is header-only). The test-coverage-expert confirms every Done-when criterion of the #3110 apm-scope:v1 record carries a regression trap.
The material delta since the 2026-11-04 advisory is exactly the three follow-ups it requested, plus a clean merge of current main: (1) ordering tests with mutation-break evidence, (2) doc wording "URL (and the reason, where available)", (3) CHANGELOG trim and consolidation into the single Unreleased Fixed list. No production logic changed between heads.
Scope note: this is a deliberate partial of #3110. The human scope record approves only the diagnostic slice and excludes closing the issue, so the PR keeps Refs #3110 without a closing keyword; safe symlink materialization stays deferred.
Aligned with: secure by default (containment and symlink refusal untouched), pragmatic as npm (proxy users see the actionable cause, not a 404), OSS community driven (community fix inside a human-approved slice).
Panel summary
| Persona |
B |
R |
N |
Takeaway |
| Supply chain security expert |
0 |
0 |
0 |
No credential leakage or containment regression; archive URLs are path-only, auth is header-only, and all extraction guards are intact. |
| Test coverage expert |
0 |
0 |
0 |
All Done-when criteria have regression traps; ordering follow-up addressed; containment tests untouched and passing. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
All three follow-ups from the 2026-11-04 advisory are folded, both panelists returned zero findings, and local and CI evidence is green. No open items remain for this diagnostic slice. Daniel Meppiel (@danielmeppiel) remains the requested CODEOWNER reviewer and review contact; Sergio Sisternes (@sergio-sisternes-epam) has approved.
Folded in this run
- (panel) Ordering cases for the proxy archive diagnostic (404 then rejection; two rejections, first wins) -- resolved in 9e1f9a4.
- (panel)
registry-proxy.md now says the error reports the URL (and the reason, where available) -- resolved in b2551b8.
- (panel) CHANGELOG reassurance trimmed; entry moved into the single Unreleased Fixed list after merging main -- resolved in 183b678.
Copilot signals reviewed
- review 5632977270 -- NOT-LEGIT: quota notice only, no review content; re-fetch at 183b678 returned zero inline items (drained).
Regression-trap evidence (mutation-break gate)
test_first_archive_rejection_reported_regardless_of_order[first-rejection-wins] -- replaced archive_error = archive_error or last_error with last-wins; test FAILED as expected; guard restored.test_first_archive_rejection_reported_regardless_of_order[404-then-rejection] -- deleted the ArchiveError-branch capture; test FAILED as expected; guard restored.
Lint contract
ruff check and ruff format --check (src/, tests/, architecture linter scripts) both silent; pylint R0801 10.00/10; lint-auth-signals.sh and lint-architecture-boundaries.sh exit 0.
CI
All 27 checks green at 183b678 (CI, Merge Gate, Spec conformance, CodeQL, NOTICE Drift, Deploy Docs, license/cla) (after 0 CI fix iteration(s)).
Mergeability status
| PR |
head SHA |
CEO stance |
iters |
folds |
defers |
Copilot rounds |
CI |
mergeable |
mergeStateStatus |
notes |
| #3375 |
183b678 |
ship_now |
1 |
3 |
0 |
2 |
green |
MERGEABLE |
BLOCKED |
pending required review |
Convergence
1 outer iteration(s); 2 Copilot round(s). Final panel stance: ship_now.
Ready for maintainer review.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by autopilot-pr-merge-worker. This comment is AI-generated and may contain errors.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch has not been deployed
No deployments
Reviewers
At least 0 approving reviews are required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
area/enterprise
Air-gapped/GHE configurability, registry proxy, rulesets, adoption playbook.
status/accepted
Human scope approval; verify the issue's approval record and review contact before work.
theme/security
Secure by default. Content scanning, lockfile integrity, MCP trust boundaries.
triage/recommended
Automated advice completed; not human scope approval.
type/bug
Something does not work as documented.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|