fix(client): Agent Skills pre-release must-fix findings

[XieX]

Fixes the must-fix-before-merge findings from the pre-release review of Agent Skills (#87, reviewed at 0306c69). One commit per fix. Each code fix has a test that fails on 0306c69 and passes here.

1. A stream event whose data isn't JSON no longer commits the rest of its transfer

_iter_sse used to log the event and skip it. The payload-transferred that followed then committed the transfer without the event and advanced basis. A corrupted delete-object left a revoked skill served indefinitely, because the reconnect asked only for changes after a basis that already covered the revocation. A corrupted put-object in an xfer-full revoked that skill by omission.

It now raises _RecoverableTransportError, matching ldclient's FDv2 stream, which interrupts on JSONDecodeError. The in-flight payload is dropped and the reconnect sends the last committed basis. The drop is logged at WARNING in _iter_sse, because _run logs a disconnect after a completed exchange at debug, and a revocation usually arrives after a commit on the same connection.

Only the six events handle reads are parsed (server-intent, put-object, delete-object, payload-transferred, goodbye, error). A heart-beat or an unknown event is yielded with None data, so its data can't drop the connection. ldclient likewise calls json.loads only inside its known-event branches.

Tests: test_a_malformed_event_abandons_its_transfer_and_keeps_the_basis, parametrized over two cases:

• delete-object-in-xfer-changes commits a:1 and revoked:1, then sends an xfer-changes containing a clean put fresh:1 and a truncated delete revoked:1, then payload-transferred. At the reconnect, nothing from that transfer is committed: revoked is still held and fresh is absent.
• put-object-in-xfer-full commits a:1, then sends an xfer-full with a clean put b:1 and a truncated put a:1. At the reconnect, a is still held and b is absent.

In both cases the reconnect carries basis-1, a clean retransmission then applies in full, and a WARNING names the event. test_an_event_handle_does_not_read_is_not_parsed covers heart-beat and x-future-event with data: not json. test_an_event_handle_reads_is_parsed pins the parse set (all six), and the store-level catastrophe test goes through _iter_sse.

Empty data on a known event is not JSON either (bf559fb), as json.loads("") raises in ldclient. Read as None, a delete-object was ignored and a payload-transferred committed with no selector. The malformed-event test has a third case, empty-delete-object-in-xfer-changes, and test_an_event_handle_reads_with_no_data_drops_the_connection covers all six names. Two reader tests pin the experimental-stage rule for a put or delete with no usable key: it is warned and ignored, and its transfer commits (TESTING.md §3.25, ai-sdks-monorepo#48).

2. A goodbye with catastrophe: true no longer stops delivery

It was raised as fatal, so delivery stopped until something called start(). Python ldclient 9.16 never reads catastrophe, and the Go SDK only logs it. It is now a recoverable disconnect, logged at ERROR. It is not marked recycled, so it counts toward connection_failures even after a completed exchange. _TransferOutcome.fatal had no other producer, so it is removed.

Tests: test_a_catastrophic_goodbye_is_fatal is flipped to test_a_catastrophic_goodbye_is_a_counted_disconnect. New: test_a_catastrophic_goodbye_reconnects_and_is_counted. No docstring or README said the goodbye was fatal; only the code and that test did.

3. skill_refs raises on a config that is not a dict

skill_refs(None) returned [], and None is the config a failed inspect_config returns. So write_skills(skill_refs(info["config"]), root) pruned every managed skill during an outage. A non-dict config now raises ValueError, as a malformed skills field already does. A dict without a skills key still returns []. The docstring, the README (quick-start comment, validation paragraph, API table), and agents.md are updated.

Test: test_a_config_that_is_not_a_dict_raises, parametrized over None, a list, and a string.

4. Docs: the ld.skills.integrity_failure commitment

The README said the event "will not be renamed". It now says that during the experimental stage the name, fields, and reason_code values may change in a minor release, with a changelog entry under Experimental (TESTING.md §0.1's wording, and js-ai-sdk's). agents.md is updated to match. Docs only, so there is no test.

Checks

uv run pytest (repo root): 2321 passed, 11 skipped. make lint, make format-check, and make typecheck are clean.

Spec: https://github.com/launchdarkly/ai-sdks-monorepo/pull/48. JS counterparts: launchdarkly/js-ai-sdk#120.

🤖 Generated with Claude Code

---

Note

Overview
Addresses pre-release review must-fixes for Agent Skills in the Python client: safer reference discovery, FDv2 stream consistency, and doc alignment.

skill_refs now raises ValueError when the config is not a dict (including None from a failed inspect_config), instead of returning []. That stops write_skills(skill_refs(...), root) from pruning every managed skill during a config outage. A dict with no skills field still returns [].

FDv2 streaming treats non-JSON data on the six events the protocol reader consumes as a recoverable disconnect (_RecoverableTransportError), matching base ldclient behavior and abandoning the in-flight payload so a later payload-transferred cannot commit a partial transfer and advance basis. heart-beat and unknown events are not parsed. goodbye with catastrophe: true reconnects with backoff (ERROR log, counted failure) instead of stopping delivery fatally; _TransferOutcome.fatal is removed.

Docs (README.md, agents.md) describe the skill_refs contract, stream/keyless-object behavior, and soften the ld.skills.integrity_failure stability wording for the experimental stage (minor-release changes with changelog). Tests cover malformed SSE, catastrophic goodbye, non-dict configs, and keyless put/delete experimental rules.

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

[jeffdupont]

All four fixes do what the PR body says, and each named test fails when its fix is reverted. Two things should still change before this merges: a dropped stream is now logged only at debug in the common case, and an unknown event with non-JSON data now drops the connection. At ecb21b21 I get 2297 passed, 11 skipped. ruff, format (134 files) and mypy (60 files) are clean, and CI is green. The base hasn't moved. main is already contained in it, so the trial merges onto xie/agent-skills and onto main are both clean.

Reverting each fix fails its tests:

• fix 1 (skip the event with a warning, as before): 2 tests fail, test_a_malformed_event_abandons_its_transfer_and_keeps_the_basis and the _iter_sse test
• fix 2 (catastrophe raises fatal): 2 fail, both catastrophe tests
• fix 3 (return [] for a non-dict): 3 fail, all three params

Near-miss mutations that also fail a test: a fatal error instead of a recoverable one, yielding None instead of raising, a recycled catastrophe, a catastrophe logged at WARNING, a catastrophe that clears the held set, None-only [] (a list or string still raising), TypeError, raising on an absent skills key, and a truthiness check ([] for any falsy config).

I also ran a probe against the real _iter_sse and delivery loop:

• A truncated put-object inside an xfer-full commits nothing. The held set is unchanged, and the reconnect sends basis-1. So the drop does apply to xfer-full.
• A repeated catastrophic goodbye backs off 1, 2, 3, 4, counts 4 failures, logs 4 ERRORs, and never sets failed. With silent: true it still logs at ERROR, as js #120 does.
• 401, 403, 404 and 422 still stop delivery after one connection. fatal had no other producer, and _classify_status (skills_fdv2.py:931-977) and _ResponseTooLargeError still reach _give_up. Removing it leaves nothing that should stop unstopped.
• 304/etag and restart: the change is stream-only. A poll body that isn't JSON was already recoverable (:1194), and an etag is adopted only after a completed exchange (:1838-1846). A recoverable error never reaches _give_up, so the #129/#139 restart rules aren't involved.

1. A non-JSON event after a commit on the same connection is logged only at debug. That is the normal case for a revocation: the stream commits the first payload, and later changes arrive on the same connection. _run logs every recoverable error at debug once the connection has answered (skills_fdv2.py:1748-1755, if answered:), whether or not it was counted. My probe logs DEBUG The FDv2 stream ended after a complete answer (an FDv2 'delete-object' event's data was not JSON ...), and nothing at WARNING. Before this PR, _iter_sse logged a WARNING. Only a non-JSON event before any answer gets the "Skill delivery failed" warning. js #120 warns in both cases, because its loop warns on any disconnect that isn't expected (skills-fdv2.ts:1861-1862). The catastrophe fix already handles this by logging in the reader. The smallest fix is a logger.warning in the except at :1266, before the raise. A test for it would be the malformed-event test with caplog.
2. An unknown event, or a heart-beat, with non-JSON data now drops the connection. _iter_sse parses every event before handle routes it. So event: some-future-event with data: hello reconnects, and the same goes for heart-beat with data: ping (probed: 2 connections, connection_failures 1). That contradicts handle's "Unknown event names are ignored, by contract" (:565), and TESTING.md :1943 on #48, the line right after the new non-JSON bullet: "Assert an unrecognised event neither raises nor disturbs state". ldclient 9.16 doesn't parse first. It calls json.loads inside each known-event branch and only logs an unknown one (ldclient/impl/datasourcev2/streaming.py:339-391). That makes the PR's "matching ldclient" true only for known events. LaunchDarkly's streamer sends heartbeats as the comment :\n (streamer v2_common.go:13), so this is a forward-compatibility risk, not a live one. Still, it's cheap to fix: raise only for the six names handle reads, and pass any other event through without parsing it. js #120 has the same issue (dispatch, skills-fdv2.ts:1217-1231), so both should change, and #48 :1942 should say "a known event".
3. The xfer-full half of fix 1 has no test. If I drop the connection only for a non-JSON delete-object and skip any other non-JSON event, every test still passes (1471 passed). The PR body, agents.md (:303-309) and #48 all say a lost put-object in an xfer-full revokes that skill by omission, and agents.md names this test as asserting it. Could you parametrize the test, or add a second one, with a truncated put-object inside an xfer-full? That probe passes here.

Smaller items

• A delete that is valid JSON but the wrong shape still commits the rest of the transfer. I put these into an xfer-changes in place of delete-object revoked:1: data "revoked:1", [1], empty, {kind, version} with no key, an integer key, and no kind. In all six, payload-transferred commits, revoked is still held, and the basis moves to basis-2. That's the bug fix 1 closes, reached through a server bug instead of corrupt bytes. ldclient interrupts on five of the six (DeleteObject.from_dict raises on a missing field, and json.loads("") raises). Ignoring a delete with no kind follows this SDK's rule for unknown kinds, so that one is a choice. A skill-kind delete with no usable key (_tombstone_from_delete, :376-382), or empty data on a put or delete, is a lost revocation, though. Whether these should drop the transfer is worth deciding in #48. js #120 behaves the same (tombstoneFromDelete, skills-fdv2.ts:441-446). Not a blocker.
• skill_refs still accepts None in its type (skills.py:202, AiConfigRep | None). Since None now always raises, config: AiConfigRep would let mypy flag skill_refs(info["config"]) with no guard, before it runs. JS has the same | null | undefined in its API report. Optional, and it should change in both or neither.
• The README's changelog promise is weaker than the spec's. README :574 says "called out in the changelog". #48 and js #120 say "a changelog entry under Experimental". Neither repo's release-please-config.json defines an Experimental section, so as written, nothing produces that heading. Worth one line in #48 on how it gets made.
• Two _abandon_in_flight calls survive removal: the loop's (:1720) and the goodbye's (:739). The next server-intent resets the pending set anyway, so neither one is a bug. Both predate this PR.

Cross-SDK and spec

js #120 (fa44c1a9) makes the same four changes with the same semantics: the non-JSON drop, the counted and recoverable catastrophe logged at error whatever silent says, skillRefs throwing TypeError for null, undefined, arrays and primitives while an object with no skills returns [], and the experimental-stage wording. It shares issues 2 and 3. It doesn't share issue 1, because JS warns. Its fifth fix (the referenced waitForSkills timer) has no Python equivalent, and #48 says so. Its PR body still lists "Python's skill_refs(None) still returns []" under divergences, and this PR resolves that.

#48 (35e6a6e2) specifies all four fixes, and the code matches it. The one conflict is :1942 against :1943 (issue 2). Before this, main's TESTING.md said nothing about skill_refs(None). §3.20 (:1596-1599) covers only an absent field, an empty field and malformed entries. #48 adds the rule (:1597). #48 still has the row-6 TODO for js #121's option names, and like #46 it leaves the stale malformed-skills rules alone (§3.5 :968-980, §3.20 :1599).

The READMEs and agents.md in both repos now match the new behaviour, and I found no stale "[] when nullish" text. The public docs page (ld-docs-private #8635, merged) needs a follow-up:

• It guards on info["meta"] is None (agent-skills.mdx:233, :255) and info.meta === null (:375). inspect_config returns a non-None meta with config: None for a disabled config and for one that fails to parse (lifecycle.py:431, :440). In both cases the docs example now raises ValueError / TypeError out of the else branch. Before this PR it pruned every managed skill. The guard should be on config.
• The JS watcher example (:394) and both "discover" examples (:165, :336) call skillRefs(info.config) with no guard at all.
• :566 still says the event name "will not change outside a major release". That contradicts fix 4 and #48's "Neither README may promise more than its stage allows".
• Pre-existing, but it's the same page: :246 says watch_skills(skill_refs(...)) prunes a revoked skill within seconds, while the README says an explicit list never prunes on revocation (README.md:756-760).

#87

This PR doesn't change my 5 Oct approval of #87's code, and it removes the catastrophe fatal I hadn't flagged. #87 still shouldn't reach main before:

• this PR, with issues 1-3 fixed
• #48
• the malformed-skills spec change that neither #46 nor #48 makes

[XieX]

@jeffdupont, thanks. Issues 1–3 are fixed at fca6f05, with replies inline. 2300 passed and 11 skipped, and ruff, format and mypy are clean. Each new test fails when its fix is reverted.

On the smaller items:

• A delete that is valid JSON but the wrong shape: not changed here. Whether a skill-kind delete with no usable key, or empty data on a put or delete, should abandon the transfer is a spec decision, and it applies to both SDKs. I've left it open for ai-sdks-monorepo#48 rather than pick an answer in one SDK.
• skill_refs' | None: left as is. See inline: inspect_config returns dict[str, Any], so mypy wouldn't catch the unguarded call in Python.
• The Experimental changelog heading: the README wording now matches §0.1 and JS. How the heading actually gets produced is still open (see inline).
• The two surviving _abandon_in_flight calls: no change. As you say, they aren't a bug.
• The public docs page (ld-docs-private#8635): I agree on all four points (guard on config, not meta; the unguarded JS calls; the "outside a major release" line; the watch_skills(skill_refs(...)) pruning claim). It's a separate repo, so it'll be a follow-up PR there: https://github.com/launchdarkly/ld-docs-private/pull/8644

ai-sdks-monorepo#48 is updated too (dcc9d5e). It says "a known event" in the non-JSON rule, adds the non-JSON case to the heart-beat bullet and asserts the xfer-full case. It also records the ValueError/TypeError split in A.12 and fills in the row-6 TODO from js-ai-sdk#121.

[jeffdupont]

Re-reviewed at fca6f051. All three fixes I asked for are in, and I checked each of your replies against the code. They're accurate. One small test gap (below) should be closed before merge, and then I'd approve.

At fca6f051 I get 2300 passed and 11 skipped. ruff, format (134 files) and mypy (60 files) are clean, and CI is green. The head already contains xie/agent-skills (9c22ba9) and main (0b4df05).

Each fix, reverted:

• Warning on a dropped event (skills_fdv2.py:1291): I deleted the logger.warning line, and both cases of test_a_malformed_event_abandons_its_transfer_and_keeps_the_basis fail.
• Parse only known events (_EVENTS_WITH_DATA, :114-123): this is exactly the six names handle passes data to (:580-591). heart-beat never reads its data (:592), and an unknown event only logs at debug (:594), so None data is safe for both. When I restored parse-every-event, both cases of test_an_event_handle_does_not_read_is_not_parsed failed.
• xfer-full case: my earlier mutation (drop the connection only for a non-JSON delete-object) now fails [put-object-in-xfer-full] and nothing else.

Fix before merge: nothing tests that a goodbye reaches handle with its data parsed. I deleted _EVENT_GOODBYE from _EVENTS_WITH_DATA, and the client suite still passes (1472 passed). A goodbye would then arrive as None, catastrophe: true would never be read, and a catastrophic goodbye would become an uncounted recycle. That quietly undoes fix 2. Both catastrophe tests skip the SSE parser. One calls handle directly (:926), and the store test feeds already-parsed tuples (:2722). The parse set is new in this PR, so this is a regression path this PR opened. One case through _iter_sse closes it. Inline. js #120 has the same gap.

Your two "leave as is" replies hold up too:

• | None on skill_refs: inspect_config returns dict[str, Any] (lifecycle.py:385-388). I tried config: AiConfigRep with an unguarded skill_refs(info["config"]), and mypy reports no issues. Fine to leave it.
• Changelog wording: README :574-575 now matches TESTING.md §0.1 on main (:118). Producing the Experimental heading is still a release-process gap, but it isn't this PR's to fix.

Nit, inline: a non-JSON event that arrives before the first answer now logs two WARNINGs for one disconnect.

Still open outside this PR:

• #48: the wrong-shape-delete decision you deferred to #48 isn't written down there yet. I've asked there.
• ld-docs-private#8635: the follow-up PR you agreed to.
• #87 → main: still needs this PR, #48, and the malformed-skills spec change.

[XieX]

@jeffdupont, thanks. The goodbye gap is closed (inline), and I've left the double-warning nit as it is (inline). Since your review there are three new commits:

• 4c1fa65: the parse set is pinned, and the store-level catastrophe goes through _iter_sse.
• 60c4b8b: that store test's goodbye is silent: true, since feat: carry judge reasoning in the judge track payload and telemetry #48 now requires a silent catastrophe to log at ERROR.
• bf559fb: the wrong-shape decision, below.

Wrong-shape objects, decided.

• Empty data on a known event now drops the connection. An empty string isn't JSON, and json.loads("") raises in ldclient too. Read as None, a delete-object was ignored, and a payload-transferred committed with no selector. The malformed-event test has an empty-delete-object-in-xfer-changes case, and all six names are covered with the data: line missing and with it empty. Reverting the change fails 13 tests.
• A put or delete that is valid JSON but has no usable key stays warned and ignored, for the experimental stage. feat: carry judge reasoning in the judge track payload and telemetry #48 §3.25 now states both consequences, adds a note that this is to be revisited before promotion, and records that ldclient interrupts instead. The consequences are a lost revocation, and a keyless put in an xfer-full being revoked by omission and then pruned. Interrupting isn't strictly safer: a server that keeps resending the same bad object would block every later commit, later revocations included. Two reader tests pin the current rule, so changing it is a deliberate flip.

At bf559fb: 2321 passed and 11 skipped, and ruff, format and mypy are clean.

[andrewklatzke]

@jeffdupont Mind confirming the fixes here?