Skip to content

Return 502 Bad Gateway when OCI skill pull fails#4956

Merged
rdimitrov merged 2 commits intomainfrom
bad-gateway
Apr 21, 2026
Merged

Return 502 Bad Gateway when OCI skill pull fails#4956
rdimitrov merged 2 commits intomainfrom
bad-gateway

Conversation

@samuv
Copy link
Copy Markdown
Contributor

@samuv samuv commented Apr 21, 2026

Summary

  • Follow-up on review feedback from Add GetContent API for on-the-fly SKILL.md retrieval from OCI and git sources #4810 (discussion): registry.Pull failures in getContentFromOCI and installFromOCI were mapped to 400 Bad Request, but auth errors, unreachable registries, missing manifests, and network timeouts are upstream failures the caller cannot fix by changing their input.
  • Return 502 Bad Gateway instead, matching the git-resolve path in getContentFromGit which already uses 502 for the equivalent upstream failure.
  • Parse / validation / "reference not found locally and registry not configured" paths continue to return 400 — those are genuine caller errors.

Type of change

  • Bug fix (HTTP status semantics)

Test plan

  • Unit tests (go test ./pkg/skills/skillsvc/...)
  • Existing tests updated to assert 502 where appropriate; 400 assertions for parse / missing-registry / invalid-reference cases left intact

Changes

File Change
pkg/skills/skillsvc/content.go getContentFromOCI: StatusBadRequestStatusBadGateway on registry.Pull failure
pkg/skills/skillsvc/install_oci.go installFromOCI: StatusBadRequestStatusBadGateway on registry.Pull failure
pkg/skills/skillsvc/content_test.go Rename pull failure propagates as 400...as 502; assert StatusBadGateway
pkg/skills/skillsvc/install_oci_test.go Update wantCode to StatusBadGateway in four pull-failure fall-back tests (explicit tag, qualified name, digest ref, multi-segment ref)

Does this introduce a user-facing change?

Yes — API clients that previously received 400 Bad Request on a remote OCI pull failure now receive 502 Bad Gateway. This is a more accurate signal that the failure originated upstream (registry/network) rather than from a malformed request, and matches the behaviour already in place for git pulls (and already documented in the swagger annotation introduced in #4810).

@samuv samuv requested a review from JAORMX as a code owner April 21, 2026 09:10
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Apr 21, 2026
@samuv samuv self-assigned this Apr 21, 2026
samuv added 2 commits April 21, 2026 11:39
getContentFromOCI previously mapped registry.Pull failures to 400 Bad Request, but auth errors, unreachable registries, missing manifests, and network timeouts are upstream failures the caller cannot fix by changing their input. Align with getContentFromGit, which already returns 502 Bad Gateway for git resolve failures, so clients see consistent semantics across both backends.
installFromOCI previously mapped registry.Pull failures to 400 Bad Request, which incorrectly blamed the caller for upstream registry problems (auth, network, manifest unknown). Return 502 Bad Gateway to match the behaviour just applied to GetContent and the existing git install path.
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Apr 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.48%. Comparing base (b2f6717) to head (8e2660a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4956      +/-   ##
==========================================
- Coverage   69.49%   69.48%   -0.01%     
==========================================
  Files         551      551              
  Lines       55817    55817              
==========================================
- Hits        38788    38786       -2     
- Misses      14041    14043       +2     
  Partials     2988     2988              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rdimitrov rdimitrov merged commit ccf439d into main Apr 21, 2026
41 checks passed
@rdimitrov rdimitrov deleted the bad-gateway branch April 21, 2026 10:03
@jhrozek
Copy link
Copy Markdown
Contributor

jhrozek commented Apr 21, 2026

One small follow-up on the swagger side (non-blocking, feel free to punt to a separate PR):

installFromGit already returns 502, and with this change installFromOCI does too — but the @Failure list on installSkill in pkg/api/v1/skills.go still only advertises 400/409/500. Might be worth adding @Failure 502 {string} string "Bad Gateway" there and regenerating the swagger docs so clients see the real contract. Pre-existing gap, not introduced here.

samuv added a commit that referenced this pull request Apr 21, 2026
)

Follow-up to #4956: installFromOCI now returns 502 on upstream pull
failures (matching installFromGit), but the @failure list on the
installSkill swagger annotation still only advertised 400/409/500.

Add @failure 502 {string} string "Bad Gateway" and regenerate the
spec so clients see the real contract for POST /api/v1beta/skills.
yrobla pushed a commit that referenced this pull request Apr 21, 2026
* Return 502 Bad Gateway when OCI content pull fails

getContentFromOCI previously mapped registry.Pull failures to 400 Bad Request, but auth errors, unreachable registries, missing manifests, and network timeouts are upstream failures the caller cannot fix by changing their input. Align with getContentFromGit, which already returns 502 Bad Gateway for git resolve failures, so clients see consistent semantics across both backends.

* Return 502 Bad Gateway when OCI install pull fails

installFromOCI previously mapped registry.Pull failures to 400 Bad Request, which incorrectly blamed the caller for upstream registry problems (auth, network, manifest unknown). Return 502 Bad Gateway to match the behaviour just applied to GetContent and the existing git install path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants