Stop severing skill artifact pulls at fixed timeouts

[samuv]

Summary

Installing a skill whose OCI artifact takes a while to pull fails, and the error blames the wrong thing:

Error: failed to install skill: could not reach ToolHive API server — is 'thv serve' running?
Hint: ensure 'thv serve' is running

The server is running. It was pulling the artifact. Investigating turned up two independent ceilings, and fixing only the client one is not enough — I verified that by fixing the client first and watching the same install fail again, 30s later:

1. The client gave up at 30s and reported unreachability. WithTimeout existed but was not reachable from the CLI, so there was no way out. Worse, abandoning the request cancels its context server-side, so the server logs context canceled and discards the work — retrying restarts from nothing rather than resuming.

2. The skills router inherited a flat 60s cap. It sat in standardRouters in setupDefaultRoutes, all of which get middleware.Timeout(60s). The workload router is already mounted outside that set, with the comment "image pulls can take minutes". Skills pull the same artifacts the same way; they were simply never given the same treatment.

Changes:

• ErrRequestTimeout, distinct from ErrServerUnreachable, so a healthy-but-slow server is not reported as absent. Caller cancellation (Ctrl-C) is classified as neither and surfaces as context.Canceled.
• Client default raised, with TOOLHIVE_API_TIMEOUT (a Go duration, e.g. 45s, 30m) as an override. An explicit WithTimeout still outranks the env var; an unparseable or non-positive value is ignored with a warning rather than disabling the timeout.
• SkillsRouter gains per-route timeouts mirroring WorkloadRouter — long on install, sync, upgrade, build, push; short on the read routes — and is mounted outside standardRouters.
• The CLI hint now matches the failure and names the override.

Closes #6212

Type of change

• Bug fix

Test plan

• Unit tests (task test)
• Linting (task lint-fix)
• Manual testing (describe below)

New tests: TestTimeoutFromEnv (table: unset / durations / whitespace / bare number / garbage / zero / negative), TestNewDefaultClientTimeoutPrecedence (default → env → explicit option, plus env beating the client discovery installs), and three classification tests using real sockets — a slow server yields ErrRequestTimeout and not ErrServerUnreachable, a closed port yields the reverse, and a cancelled caller context yields neither.

Manually, a cold pull of security-review against a local thv serve:

• before: failed at 0:30 with the unreachable message; after raising only the client timeout, failed again at 1:00 with Internal Server Error and context deadline exceeded server-side
• after both fixes: succeeded at 1:16, with a correct lock entry and verified provenance, and zero context canceled / context deadline exceeded in the server log

- name: security-review
  resolvedReference: ghcr.io/stacklok/dockyard/skills/security-review:0.1.1
  provenance:
    signerIdentity: /.github/workflows/build-skills.yml
    repositoryUri: https://github.com/stacklok/dockyard

Does this introduce a user-facing change?

Yes. Skill operations that pull OCI artifacts no longer fail on slow or large pulls. Timeout failures now say the request timed out instead of claiming the server is unreachable, and TOOLHIVE_API_TIMEOUT overrides the client limit.

Special notes for reviewers

Two things worth a deliberate look:

Scope. The issue is written about the client timeout, and this PR also changes server-side routing. I would normally split that, but the server cap makes the client fix useless on its own — the user-visible bug is not fixed by either half alone.

The default. The client default is now generous, on the reasoning that this timeout is a backstop against a wedged local server rather than a budget for the operation: the transfer size is unbounded, and cutting it short destroys the work instead of deferring it. If you would rather keep a short default and have callers opt in per-operation, say so — it is a one-line change plus threading an option through the pull-backed commands.

PluginsRouter is in standardRouters and pluginsvc pulls artifacts too, so it likely has the same 60s ceiling. Left alone here to keep this PR to the reported bug; happy to file it.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.47%. Comparing base (8863eb0) to head (ec21e65).

Files with missing lines	Patch %	Lines
pkg/skills/client/client.go	92.85%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6224      +/-   ##
==========================================
- Coverage   72.48%   72.47%   -0.01%     
==========================================
  Files         739      739              
  Lines       76728    76751      +23     
==========================================
+ Hits        55613    55622       +9     
- Misses      17151    17163      +12     
- Partials     3964     3966       +2     

☔ View full report in Codecov by Harness.
📢 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.