Skip to content

feat(client): Agent Skills — the FDv2 delivery transport - #83

Open
XieX wants to merge 1 commit into
xie/skills-fdv2-protocolfrom
xie/fdv2-transport-split
Open

feat(client): Agent Skills — the FDv2 delivery transport#83
XieX wants to merge 1 commit into
xie/skills-fdv2-protocolfrom
xie/fdv2-transport-split

Conversation

@XieX

@XieX XieX commented Sep 10, 2026

Copy link
Copy Markdown

Stacked on the protocol-layer PR (xie/skills-fdv2-protocol). Third of three PRs split out of #69, and the one that makes the feature real: the network underneath the protocol reader, exported as FDv2SkillStore.

Why this shape

Skill content arrives over GET /sdk/poll and GET /sdk/stream, authenticated with the environment's server-side SDK key. These are the SDK-facing endpoints the base SDK's FDv2 data source uses, and the channel that payload signing will eventually cover. No private route is involved, and no credential other than the environment's own SDK key ships to a customer host. Standard library only, so the content path adds no dependency to a package whose sole runtime dependency is opentelemetry-api.

What's here

FDv2SkillStore. Authenticates, streams (default) or polls, carries basis across requests, sends If-None-Match and treats 304 as a first-class current answer, and serves get_object / all_objects / add_listener / remove_listener from what the protocol reader has committed. Capped jittered backoff; Retry-After honoured but clamped to max_backoff and rejected when non-finite; bounded consecutive-failure retries, where a committed payload resets the count. One network timeout, read_timeout, whose default follows the mode: 10s for a whole poll, 300s between reads on a stream. A mobile key or client-side environment ID raises from the constructor. Last known good survives every failure; diagnostics and failed report the degradation.

close interrupts the socket. The delivery thread parks in a read no flag can reach, and closing a urllib response from another thread does not unblock CPython's buffered reader, so _interrupt_read shuts the socket down underneath it. Without that every shutdown of a healthy stream blocked for the full join timeout.

Above the interface, two strings: NO_STORE_MESSAGE now names FDv2SkillStore first, since it is the first thing a user sees on a missing store and offering only the development store was wrong once a production transport existed; and watch_skills' refusal message names it as the store with a delivery transport.

Bugs found and fixed while testing the loop

Five, all sharing one shape: the store stopped delivering while continuing to report itself healthy.

  • The consecutive-failure counter never reset in stream mode. _stream_once always ends by raising, so a reset on return was unreachable and failures grew for the whole process lifetime. Eleven fully successful payload transfers were enough to trip max_consecutive_failures and stop delivery for good, revocations included. A commit now resets the count, in _apply.
  • A non-finite Retry-After killed the delivery thread. float("inf") parses, and Event.wait(inf) raises OverflowError from inside the recoverable-error handler. Non-finite values are rejected and every honoured delay is clamped to max_backoff.
  • close() during the initial connect waited out its full join timeout. self._connection was assigned after the connect returned, so a close() in that window found nothing to interrupt. The stop flag is re-checked immediately after the assignment.
  • close() blocked for the full join timeout on every healthy stream. See _interrupt_read above.
  • A stream interrupted by our own close was reported as a delivery failure.

Also: connect_timeout was accepted and never used, so a poll against a black-holed host hung for 300s rather than 10. It is gone, with the request timeout now chosen by mode, and TestTimeouts measures the bound against a socket that accepts and never answers.

Tests

_FakeFDv2Endpoint is an in-process ThreadingHTTPServer implementing the wire contract, so request construction and header handling are exercised over real sockets rather than mocked. Covers skill put/delete over the wire, mixed payloads, 304, basis round-tripping, reconnect/backoff in both modes, Retry-After including non-finite and oversized values, bounded retries and the reset on commit, prompt shutdown during connect and during a healthy stream, hashless envelopes end to end through the accessors, server-side-only credentials, timeouts, and watch_skills over the transport: a wire-level revocation pruning a file without a restart.

Full suite 1629 passing, 11 skipped; ruff, ruff format, and mypy clean.

Open items, none in this PR's scope

  • 🔴 contentHash is not on the wire yet. Against a real environment today every skill resolves to nothing. This PR makes that loud (an error per hashless object, a summary per wholly-hashless payload, diagnostics.hashless_objects) rather than surviving it.
  • 🔴 Server-side inline-resource delivery is not deployed. No account can receive skill objects until it ships.
  • 🟡 FDv2 is opt-in per account. A real environment returns 403 today; the store reports it as fatal and explains what to do.
  • 🟡 mv is a guess. Defaulted to 1, overridable via constructor. The most likely thing to be wrong on first contact with a real server.
  • 🟡 No payload signing on this channel yet, so Beta is TLS-only.
  • 🟡 The connection also carries the environment's flags. Skipped and counted; a transport property, not fixable here.
  • 🟡 ld-relay does not speak the FDv2 endpoints, so relay-only deployments cannot receive skills in Beta.

Nothing here has touched a real LaunchDarkly environment, because it cannot yet.

🤖 Generated with Claude Code


Note

Overview
Adds production skill delivery via FDv2SkillStore, a SkillStore that pulls agent skills from LaunchDarkly over the SDK-facing FDv2 GET /sdk/poll and GET /sdk/stream endpoints (stdlib HTTP/SSE, server-side SDK key only). Streaming is the default; poll mode is optional. The store runs delivery on a background thread, commits payloads at payload-transferred, keeps last known good on outages, exposes StoreDiagnostics / failed, and notifies watch_skills through add_listener.

Lifecycle and resilience: start(), wait_for_skills(), context-manager support, mode-specific read_timeout, capped exponential backoff with bounded Retry-After, and close() that shuts down the socket so blocked reads unblock promptly.

Public surface: FDv2SkillStore and StoreDiagnostics are exported; README/agents docs describe production setup, beta caveats, and hashless-object diagnostics. NO_STORE_MESSAGE and watch_skills errors now point at FDv2SkillStore first.

Tests: In-process fake FDv2 server plus broad coverage (basis/ETag/304, revocations, retries, timeouts, credentials, end-to-end accessors and watch_skills).

Reviewed by Cursor Bugbot for commit a442a7c. Bugbot is set up for automated code reviews on this repo. Configure here.

FDv2SkillStore puts LaunchDarkly's SDK-facing FDv2 channel underneath the
protocol layer: GET /sdk/poll and GET /sdk/stream, authenticated with the
environment's server-side SDK key, streaming by default. It carries
basis across requests, sends If-None-Match and treats 304 as a current
answer, retries with capped jittered backoff, honours Retry-After only up
to max_backoff, gives up after a bounded run of consecutive failures
where a committed payload resets the count, and keeps serving last known
good through every failure. A mobile key or client-side environment ID
is refused in the constructor. Standard library only.

close interrupts the socket rather than only setting a flag, because the
delivery thread lives in a read no flag can reach; without that every
shutdown of a healthy stream waited out the full join timeout.

The no-store message now names FDv2SkillStore first, and watch_skills
points at it as the store with a delivery transport.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a442a7c. Configure here.

if self._stop.is_set():
# ``close`` interrupted the read on purpose.
return
raise

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stream read errors stop delivery

High Severity

A timeout, reset, or incomplete read on a live stream is re-raised as a raw exception. _run then treats it as unexpected and calls _give_up, so delivery stops for the process lifetime. Connect failures on the same stream are already wrapped as _RecoverableTransportError; only the body path is not. read_timeout exists to bound a dead stream, but tripping it permanently disables updates and revocations.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a442a7c. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant