feat(client): Agent Skills — the FDv2 delivery protocol, without the network by XieX · Pull Request #82 · launchdarkly/python-ai-sdk

XieX · 2026-09-10T20:05:30Z

Stacked on the watch_skills PR (xie/skills-watch). Second of three PRs split out of #69; the transport that puts a connection underneath this follows.

The half of the delivery transport that has no I/O: identifying a skill object on the wire, translating it into the raw object shape the SkillStore interface defines, holding it by (key, objectVersion), and applying a payload's events as one consistent commit. Splitting it out lets the three decisions that matter most be reviewed without a socket in the way.

Three things worth reviewing closely

1. The skill's version is in the object's key; version is the payload's. Each version of a skill is its own object on the wire, identified as <key>:<version> (pdf-extraction:3), and that is the only place the skill's version appears. The event's version is the payload's, and confusing the two fails silently: the object verifies, the hash matches, and the caller gets content under a version number that means nothing. The wire key is split in exactly one place (_split_wire_key), both the put and the delete translation go through it, and TestVersionTranslation asserts it in both directions. A key that will not split cleanly is held rather than dropped — version-less, or with the offending text as its version — so verification withholds it with invalid_version under a key the caller recognises; only a key with nothing before the delimiter is dropped.

2. Changes commit at payload-transferred, not per object. A payload version is the unit of consistency. A half-applied full transfer would publish a state the server never described and would briefly empty the store, which, with pruning on, is the difference between a reconcile and deleting a customer's skill files. An interrupted transfer leaves last known good intact, and listeners fire once per commit.

3. A hashless object is held, not dropped. Dropping it at the transport would report absent, indistinguishable from "no such skill", and would let a prune delete the last known-good copy on disk. Holding it means verification withholds it with missing_content_hash, which is diagnosable: an ERROR per (key, version), deduped per reader rather than per process so two stores never quieten each other, a summary per wholly-hashless payload, and a StoreDiagnostics.hashless_objects counter. There is deliberately no fallback that synthesises a hash from the delivered content.

Also here

Skills are kind == "skill"; everything else is ignored, not rejected. Object kinds on the SDK-facing channel are open strings and the agent-skill payload is classified generic, so a skill arrives under the kind its producer registered — the bare category name — with no category or objectVersion field (streamer #4681, gonfalon #70638). An environment's assignment carries its flag payload alongside its agent-skill payload, so flag and segment objects arrive as a matter of course. Erroring on them would turn a normal payload into a permanent reconnect loop.
_SkillObjectSet holds several versions of one key, with lookup semantics identical to InMemorySkillStore down to the fall-through to a version-less entry. Its opaque snapshot keys are spelt <key>:<version>, the same as the wire, and a test pins that round trip. TestInterfaceParity asserts the two resolve identically.
_require_server_side_credential refuses a mobile key or client-side environment ID. Its tests arrive with the store constructor that calls it.

Nothing here is exported yet; the store exports it. The module imports nothing from the feature but the version validator.

Tests

test_skills_fdv2.py drives _ProtocolReader directly: identification, version translation, full and change transfers, interruption, revocation, tombstones, mixed payloads, unknown kinds and events, error and goodbye, and the hashless dedupe across readers. The wire builders it introduces are shared with the transport PR's fake endpoint.

🤖 Generated with Claude Code

Note

Overview
Adds FDv2SkillStore, a production SkillStore that pulls agent skills from LaunchDarkly over the SDK FDv2 channel (/sdk/poll and /sdk/stream), with streaming as the default, server-side SDK key enforcement, wait_for_skills, StoreDiagnostics, and last-known-good behavior on outages.

The adapter sits below the existing skills API: wire objects use key as skillKey:objectVersion (the event version is the payload revision and is not used as the skill version), commits happen only at payload-transferred, non-skill kinds are ignored, foreign payloads are not applied, and hashless objects are held for integrity withholding rather than dropped.

FDv2SkillStore and StoreDiagnostics are exported from the package; README and agents.md document setup with init_client and watch_skills. A large test_skills_fdv2.py suite covers the protocol reader, fake HTTP endpoint, retries, timeouts, and watch/revoke paths.

^{Reviewed by Cursor Bugbot for commit d2386c8. Bugbot is set up for automated code reviews on this repo. Configure here.}

…network The half of the delivery transport that has no I/O: identifying a skill object on the wire by kind inline-resource plus category skill, translating it into the raw object shape the SkillStore interface defines, holding it by (key, objectVersion), and applying a payload's events as one commit at payload-transferred. The store that puts a connection underneath this follows separately, so the three decisions that matter most can be reviewed on their own: - objectVersion is the skill's version; version is the payload's. The translation happens in one place and TestVersionTranslation asserts it in both directions, because confusing them fails silently. - Changes commit at payload-transferred, not per object. A half-applied full transfer would briefly empty the store, which with pruning on is the difference between a reconcile and deleting a customer's files. - A hashless object is held, not dropped, so verification withholds it with a reason code rather than the transport reporting it absent. Flag and segment objects share the connection and are skipped and counted, not rejected. Nothing here is exported yet; the store exports it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

cursor

Stale Bugbot comment from a previous run.

The protocol reader took payloads[0]'s intentCode and applied it to the skill object set, which is what the delivery protocol requires — one payload per credential, read the first intent, tolerate the rest — but it left the assumption behind that rule undocumented and unguarded. If the one-payload guarantee ever widens, an xfer-full for another payload would start an empty pending set and the next payload-transferred would publish it: every skill reported revoked, and with pruning on, a customer's files deleted. The first payload is still the payload that is read. What is new is that the reader now knows which payload skills actually arrive on — learnt from the intent's id, or from the (p:<id>:<version>) selector, since no object or transfer event carries a payload id of its own — and declines to apply a transfer of any other, holding last known good, warning once, and counting it in diagnostics.payloads_ignored. An intent describing more than one payload warns once on its own, because that is the one case the comparison cannot catch: another payload's transfer arriving before any skill has been seen has nothing to be compared against. Behaviour under one-payload delivery is unchanged, and a full transfer of the skill payload still empties it — every skill deleted is a real state the guard must not mask. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The SDK-facing FDv2 channel now delivers skills the way streamer #4681 and gonfalon #70638 spell them: object kinds are open strings, the agent-skill payload is classified `generic`, and every generic object carries only `key`, `kind`, `version` and `object`, exactly like a flag. A skill arrives under kind `skill` with its own version folded into the key as `<key>:<version>`. There is no `category` field and no `objectVersion` field; both came from an earlier streamer draft that never shipped. Identification is now the kind alone. The wire key is split in one place, `_split_wire_key`, and both the put and the delete translation go through it. A key that will not split cleanly is held rather than dropped — version-less, or with the offending text as its version — so verification withholds it with `invalid_version` under a key the caller recognises; only a key with nothing before the delimiter is dropped, since there is no identity to hold it under. `SDK_DATA_MODEL_VERSION` goes with it: the connection's `mv` parameter only accepts flag model versions, and generic payloads ignore it. The transport stops sending it in the following change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

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>

_Requester.stream wrapped only the connect as recoverable, so a read timeout, reset or truncated chunk in the body reached the delivery loop as whatever the socket raised. The loop read that as a bug and gave up: delivery stopped for the process lifetime, taking updates and revocations with it, the first time a socket died. read_timeout exists to bound a stream that has gone quiet so the loop can reconnect, and tripping it did the opposite. The body now carries the same promise the connect already did. Wrapping the line source rather than the whole read keeps protocol reader errors out of it: those are raised from the consumer's loop body, where they still surface as the bugs they are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…n promptly Three faults in the delivery loop, all of which left the store reporting itself healthy while doing less than it claimed. **An up-to-date stream tripped the failure cap.** The consecutive-failure count reset only at a commit, and an environment whose skills are not changing answers every reconnect with `intentCode: "none"` and transfers nothing. A stream only ever ends by being dropped, so each recycle of a perfectly healthy idle connection counted as a failure — announced with a `goodbye` or not — and `max_consecutive_failures + 1` of them stopped delivery for the process lifetime, revocations included. The reset on commit covered only the case where content had changed, which is the case that was easy to test and not the case that runs in production. `_TransferOutcome` now reports `up_to_date`, and a complete answer that transfers nothing breaks the row of failures exactly as a commit does. An intent this module does not recognise is still not an answer. **`close` could not interrupt a poll.** The interrupt reached the streaming connection only, so polling parked in its request with nothing to reach and `close` returned when its join timed out — on a 300s-class request, long after the process meant to exit. `_Requester` now tracks the response of a poll in flight and offers `interrupt`, which `close` calls alongside the stream's own. A request still inside its connect has no response to reach; that one is bounded by `read_timeout`, and `start` no longer leaves the store inert when a join times out around it. An interrupt we asked for is no longer recorded as a delivery failure. **`close` left a waiter parked.** `wait_for_skills` waited on the first payload alone, so a shutdown racing a waiter added the waiter's whole timeout to it. Delivery ending is now its own event: a waiter is released by a payload, a give-up or a close, and reports whether a payload actually arrived rather than merely that it was let go. That also settles what `_give_up` had been quietly asserting — it set the first-payload flag to unblock waiters, which made `wait_for_skills` answer `True` for a store holding nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

A stream only ever ends by being dropped, and LaunchDarkly — and any proxy in between — recycles a long-lived one. Every reconnect therefore logged "Skill delivery failed" at WARNING, for as long as the process ran. Until the previous commit that noise was bounded, because an idle stream gave up after eleven recycles and went quiet; now that delivery correctly survives them, it would run forever and describe a healthy store as failing. A connection that got a complete answer before it ended — a committed payload, or an up-to-date intent — delivered everything it was asked for, so its reconnect is now DEBUG and says so. A connection that ended without answering is the case the warning exists for and still gets it: a connect that never landed, or a transfer that died part-way through. Filling a customer's logs with a fault they do not have is not merely untidy; it teaches them that the level which means something can be ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

andrewklatzke · 2026-09-14T20:13:17Z

+    disconnect: str | None = None
+
+
+class _ProtocolReader:


Nit/question:

Did we think about standardizing any of this work around FDV2? This seems like a really good candidate for a parent class that we then inherit from and override just the handlers. Might be premature since we only have one currently, but I'd be surprised if this is the last thing we need to ship down this way.

Agreed. I'm taking the YAGNI approach, planning to generalize once there's another thing, since it may be awhile? If you want me to burn that bridge now rather than later I'm happy to, though, just say the word.

andrewklatzke

Left one nit/question around the architecture 👍

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 > **Rebase note.** The previous push of this branch had silently reverted the protocol PR's last commit (payload identity: `payloads_ignored`, `_is_foreign_payload`, `TestPayloadIdentity`). Rebasing onto the updated protocol branch restored it; the full suite passes with it present. `_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 skill delivery is not deployed.** The wire shape this store reads — kind `skill`, key `<key>:<version>`, generic payload — is what [streamer #4681](launchdarkly/streamer#4681) and [gonfalon #70638](launchdarkly/gonfalon#70638) emit; both are still open. No account can receive skill objects until they ship and the producer is enabled. - 🟡 **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.**~~ Resolved: the request sends no `mv`. That parameter selects the *flag* data model and the connection rejects any value but the flag default, while the generic agent-skill payload is served regardless of it. The `data_model_version` constructor argument is gone with it. - 🟡 **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](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Adds **`FDv2SkillStore`**, a production `SkillStore` that pulls agent skills over LaunchDarkly’s SDK FDv2 **`/sdk/poll`** and **`/sdk/stream`** endpoints (stdlib HTTP, background delivery thread, stream-by-default). It implements **`SkillStore`** (`get_object`, listeners, etc.) on top of the existing protocol reader, plus **`StoreDiagnostics`**, **`wait_for_skills`**, capped backoff with **`Retry-After`**, and **`close`** that interrupts blocked socket reads so shutdown is prompt. > > **Public surface:** `FDv2SkillStore` and `StoreDiagnostics` are exported from the package; README documents production setup with `init_client` and `watch_skills`. Server-side SDK keys only; mobile/client credentials are rejected. Outages keep last-known-good content; accessors above the store are unchanged. > > **Delivery-loop fixes** bundled here: reset consecutive-failure counts on successful commits / up-to-date answers (so healthy stream recycling does not stop delivery), safe handling of non-finite **`Retry-After`**, and not treating intentional **`close`** interrupts as transport failures. Removed unused **`connect_timeout`**; **`read_timeout`** is the single knob with mode-specific defaults. > > **Tests:** in-process fake FDv2 server exercises poll/stream, basis/ETag/304, revocations, retries, timeouts, hashless payloads, and **`watch_skills`** over the transport. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 88c225e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>

cursor

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

^{Reviewed by Cursor Bugbot for commit d2386c8. Configure here.}

cursor · 2026-09-15T14:40:19Z

+                target=self._run, name="ld-ai-skills-fdv2", daemon=True
+            )
+            self._thread.start()
+        return self


Restart leaves failure state sticky

Medium Severity

start after close or _give_up relaunches the delivery thread but never clears _failed_reason or _failures. failed is documented as None while delivery is running, yet it stays set, and a give-up caused by the consecutive-failure cap gives up again on the next error because the counter is already past the bound.

Additional Locations (2)

packages/client/src/launchdarkly_ai_server/skills_fdv2.py#L1383-L1388

packages/client/src/launchdarkly_ai_server/skills_fdv2.py#L1536-L1548

^{Reviewed by Cursor Bugbot for commit d2386c8. Configure here.}

cursor Bot reviewed Sep 10, 2026

View reviewed changes

Comment thread packages/client/src/launchdarkly_ai_server/skills_fdv2.py

XieX mentioned this pull request Sep 11, 2026

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

Closed

XieX and others added 4 commits September 11, 2026 14:46

XieX requested review from andrewklatzke, donei003 and knfreemLD September 11, 2026 19:41

XieX and others added 2 commits September 14, 2026 14:36

andrewklatzke reviewed Sep 14, 2026

View reviewed changes

andrewklatzke approved these changes Sep 14, 2026

View reviewed changes

XieX merged commit 96576aa into xie/skills-watch Sep 15, 2026
4 checks passed

XieX deleted the xie/skills-fdv2-protocol branch September 15, 2026 14:33

cursor Bot reviewed Sep 15, 2026

View reviewed changes

Navigation Menu

feat(client): Agent Skills — the FDv2 delivery protocol, without the network - #82

feat(client): Agent Skills — the FDv2 delivery protocol, without the network#82
XieX merged 8 commits into
xie/skills-watchfrom
xie/skills-fdv2-protocol

XieX commented Sep 10, 2026 •

edited by cursor Bot

Loading

Uh oh!

cursor Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

andrewklatzke Sep 14, 2026 •

edited

Loading

Uh oh!

XieX Sep 14, 2026

Uh oh!

andrewklatzke left a comment

Uh oh!

Uh oh!

cursor Bot left a comment

Uh oh!

cursor Bot Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

XieX commented Sep 10, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Three things worth reviewing closely

Also here

Tests

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewklatzke Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XieX Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

andrewklatzke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Sep 15, 2026

Choose a reason for hiding this comment

Restart leaves failure state sticky

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

XieX commented Sep 10, 2026 •

edited by cursor Bot

Loading

cursor Bot left a comment •

edited

Loading

andrewklatzke Sep 14, 2026 •

edited

Loading