Skip to content

GET .../skills answers 200 [] when the registry is unreachable or the token is revoked #6752

Description

@feiiiiii5

What happens

thv serve answers GET /registry/{name}/v0.1/x/dev.toolhive/skills with HTTP 200 and an empty skills array when the upstream registry is unreachable or the token has been revoked. A client cannot tell "this registry has no skills" from "the registry is down" or "your token is no longer valid".

Affected component

  • thv serve (registry API route)
  • thv CLI
  • Kubernetes operator

Steps to reproduce

  1. Point thv serve at a registry that is not reachable (or answer 500 from the registry), and query the skills endpoint.
  2. Observe HTTP 200 with {"skills":[],"metadata":{"total":0,...}}.
  3. Separately, with a warm skills cache, revoke the token so the registry answers 401, wait for the cache TTL, and query again: the stale skills list is served and no error is reported.

Cause

pkg/registry/provider_cached.go, ListAvailableSkills, in the ListSkills error branch:

  • no authentication check, so a 401/403 falls into the stale-cache branch;
  • return nil, nil when no cache is warm, discarding the error entirely.

The route then substitutes an empty slice for a nil result and encodes it with 200.

The same file already gets this right for plugins

ListAvailablePlugins documents the intended contract at provider_cached.go:476-484:

authentication failures (401/403 ...) are always propagated — stale cache must never mask a changed authentication state, or a revoked token would silently serve stale data and hide the need to re-auth; other failures (network blip, 5xx) degrade gracefully to stale cache when one is present; with no stale cache, the error is returned (never nil,nil), so the v0.1 registry route surfaces a real failure instead of an empty 200.

refreshCache (:133-141) propagates auth errors for the servers cache too. The skills path is the one that never got the same treatment, and the skillsClient construction failure branch a few lines above already matches plugins exactly.

Expected

The documented behaviour: 401/403 propagate even with a warm cache, and a cold-cache failure returns the error so the route can surface it rather than answering 200 with an empty list.

Notes

  • No test covers the current behaviour, so this is not a pinned contract.
  • I did not change the route handler itself. registry_v01_skills.go declares @Failure 503 ... "Registry authentication required or upstream registry unavailable" but never calls writeRegistryUnavailableError, so that documented 503 is currently unreachable from this handler. Making the error propagate is the prerequisite for it; wiring the handler is a separate change and I would rather not bundle it.
  • I have not measured end-to-end against a real registry; the reproduction above is from reading the code and the route's encoding path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

needs-triageIssue needs initial triage by a maintainer

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions