fix(skills): apply changes after a none intent, and retry recoverable failures indefinitely

[XieX]

Addresses three review comments on #87. Targets xie/agent-skills.

Changes

• Apply objects that follow a none intent (r4167784120). After none, the reader now expects changes, as the base SDK's ChangeSetBuilder.expect_changes() does. Before, a put-object / delete-object after none on the same stream was dropped while the following payload-transferred still advanced the basis. A skill revoked after a routine reconnect kept being served, and reconnecting didn't fix it.
• Remove max_consecutive_failures; retry recoverable failures indefinitely (r4167784131). Only fatal statuses (401/403/404/422 etc., a second 400) stop delivery and set failed. The 400-retried-once repair is unchanged.
• Clamp the backoff exponent. float(2 ** n) in _backoff_delay raises OverflowError past ~1024 consecutive failures. The old budget kept the count from getting there; with unlimited retries a long outage (~8.5 h at the default cap) would have crashed the delivery thread.
• Scope the README revocation claim to "*" (r4167784141, docs only). With an explicit list, an absent skill stays requested with an error action and isn't pruned, and the watcher doesn't see config changes. Also fixes the _resolve_requests docstring, which said absent makes a run incomplete.

Tests

• New: put after none, delete after none (reader level), and a revocation after a reconnect answered none (end to end on the stream). All three fail with the fix reverted.
• Budget tests rewritten: retried well past 10 failures (poll and stream) with last known good still served; an announced-then-dropped transfer counts each drop; restart and commit reset the count; max_consecutive_failures is absent by name; backoff at attempt 10,000 is finite. Tests that used a small budget only to stop the store now use a fatal status. Two tests about the budget's 400 exemption were deleted.
• make test (2166 passed, 11 skipped), make lint, make format-check, make typecheck all pass. Ran the affected tests 10x with no flakes.

The same changes for JS: launchdarkly/js-ai-sdk#108.
Spec: launchdarkly/ai-sdks-monorepo#36

🤖 Generated with Claude Code

---

Note

Overview
Fixes FDv2 skill delivery so revocations and edits are not dropped after a routine none reconnect, aligns retry policy with the base SDKs, and tightens docs around disk revocation.

Protocol: After server-intent with intentCode: "none", the reader now expects further changes on the same connection (matching base SDK expect_changes()). Previously, put-object / delete-object after none were ignored while payload-transferred still advanced the basis—so a skill revoked right after an up-to-date reconnect could keep being served.

Transport / FDv2SkillStore: max_consecutive_failures is removed; recoverable errors retry for as long as the store runs, with initial_backoff / max_backoff validated and a separate backoff attempt counter (reset after a completed poll or a stream held ≥60s). Oversized poll/stream bodies now raise _ResponseTooLargeError (fatal) instead of retrying forever. Goodbyes after a completed exchange are treated as routine recycles (not counted as connection_failures).

Filesystem reconcile: An absent skill in an explicit ref list no longer marks the run “incomplete” for pruning—files stay and the skill reports error. README / watch_skills docs clarify that only watch_skills("*", …) prunes revoked skills on disk.

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

[jeffdupont]

Follow-up to my GA review of #87. I reviewed f6bb491 against its base, xie/agent-skills at c547be2, not main. make test: 2166 passed, 11 skipped, exit 0. make typecheck is clean. I also reverted each fix on its own and re-ran the new tests. Without the none change, all three none tests fail. Without the exponent clamp, the backoff test fails with OverflowError. With the base branch's skills_fdv2.py put back, the no-option test and both retry-indefinitely tests fail. So the new tests do catch the bugs they're meant to.

Where the four #87 blockers stand:

• (1) Changes after none: resolved. This now matches the LaunchDarkly Python SDK's expect_changes().
• (3) Giving up after 10 failures: resolved. Removing max_consecutive_failures is not a breaking change. It never reached main (git grep on origin/main finds neither it nor FDv2SkillStore), so fix(skills) without a ! is right.
• (2) Skill-key grammar: not touched. Still open. Per your reply on #87, it needs an SDK-or-API decision, and it stays on the GA list until that's made.
• (4) Revocation for explicit lists: docs only. I think that's acceptable for 1.0. What 1.0 freezes is the conservative behaviour: absent keys keep their files and report an error. The likely fixes are additive and can ship in a minor: let watch_skills take a callable that returns the current refs, or add an opt-in prune. The one change that would be hard to make after 1.0 is making explicit lists prune absent keys by default, because 1.x would then delete files that 1.0 kept. If we never want that default, docs-only is fine. A few unscoped claims are left; details inline.

One thing to settle before merge:

1. The backoff options aren't validated, and without the budget, a zero or negative value now spins forever. With a fake requester that always fails: initial_backoff=0 made 773,754 attempts in one second; max_backoff=0 and max_backoff=-5 made about 775k each. failed stayed None. On the base branch, the same probe stops after 11 attempts with failed set. Against the real service, that's a fleet hammering LaunchDarkly over a config typo. JS #108 does the same (about 860 attempts/s). I'd validate both as positive and finite, as poll_interval already is, and add that to the spec. Inline.

Smaller notes:

• Fleet behaviour in an outage is fine for a hard outage. The cap is 30s with subtractive 50% jitter, the same as the LaunchDarkly Python SDK's FDv2 streaming (MAX_RETRY_DELAY = 30, JITTER_RATIO = 0.5, ldclient/impl/datasourcev2/streaming.py:55-57). I found two gaps, both older than this PR, but they now last for the whole outage instead of 10 attempts. First, Retry-After is used without jitter (skills_fdv2.py:1651-1655), so a fleet told Retry-After: 30 reconnects at the same moment. Second, backoff resets on every none, while the base SDK resets only after a connection has stayed open 60s (BACKOFF_RESET_INTERVAL = 60, streaming.py:56,118). A server that answers none and then drops each connection gets reconnects at about 1/s indefinitely. My probe with default backoff saw 27 connections in 20s. Both are spec questions.
• The clamp is correct at the boundaries. Attempts up to 1 give the base delay, and attempt 63 and above hit the clamp. float(2**62) is exact, and attempt 10,000 gives exactly 30.0.
• An oversized response is now retried forever. A body over MAX_RESPONSE_BYTES (64 MiB) is recoverable, so the store downloads up to 64 MiB again every 15-30s and never sets failed. The rewritten test at test_skills_fdv2.py:2777 now asserts exactly that. Before, the budget turned this into failed. It's unlikely, but it fails the same way every time, so it could be fatal. I haven't checked how large a real payload can get.
• Outage visibility. failed no longer signals an outage; only connection_failures and last_error do. A last-success timestamp on StoreDiagnostics would help health checks. That's additive, so it can come after 1.0.
• A stale comment at skills_fdv2.py:1711-1712 still mentions "exhausting its budget". #108 updated the same comment.
• Test parity with #108. JS has tests that Python doesn't, and spec #36 asks for two of them: waitForSkills running to its timeout during an outage, and fail/succeed/fail retrying at the first backoff step. JS also replaced the two 400-budget tests with "a 400 after a recoverable failure still repairs" and "the second 400 still stops"; this PR deletes them with no replacement.
• connection_failures means different things in the two languages. Python counts a goodbye after a complete answer as a failure: it sets last_error, and test_skills_fdv2.py:2406-2408 allows the count to read 1. JS exempts it (reachedServer, js skills-fdv2.ts:1810). This predates the PR, but the counter is now the main outage signal, so the two should agree and the spec should say which way.
• Spec: launchdarkly/ai-sdks-monorepo#36 matches both implementations on the two behaviour changes, the absence-by-name assertion and the finite-backoff assertion. I'd add four points there: the backoff options must be positive and finite; whether a goodbye after an answer counts toward connection_failures; when backoff resets; and whether Retry-After gets jitter.

JS counterpart: launchdarkly/js-ai-sdk#108 (same findings, noted there).

[jeffdupont]

Without the failure budget, nothing limits the retry loop when these two options are zero, negative or NaN. Reproduced at f6bb491 with a requester that always raises _RecoverableTransportError: initial_backoff=0 gives 773,754 attempts in 1s, and max_backoff=0 or -5 gives about 775k. failed stays None throughout. The base branch stops the same probe at 11 attempts. delay = min(delay, self._max_backoff) (line 1655) turns any non-positive cap into a zero wait.

Could these get the same math.isfinite(x) and x > 0 check poll_interval has, plus initial_backoff <= max_backoff? The docstring at line 1321 already says the constructor raises for an invalid interval. JS #108 needs the same check.

[XieX]

Done in a06237c: initial_backoff and max_backoff must be positive and finite, with initial_backoff <= max_backoff, checked the same way as poll_interval. JS has the same check in launchdarkly/js-ai-sdk@be65e93, and the spec in launchdarkly/ai-sdks-monorepo#36.

[jeffdupont]

This resolves the #87 blocker. Reverting just the _intent line fails all three new tests, including the end-to-end one.

This isn't new, but it now matters for longer: up_to_date=True resets the backoff immediately. The base SDK resets only after a connection has stayed open 60s (ldclient/impl/datasourcev2/streaming.py:56, retry_delay_reset_threshold=BACKOFF_RESET_INTERVAL). A degraded server that answers none and then drops gets reconnected at about 1/s by every process, indefinitely. With default backoff, I saw 27 connections in 20s. Resetting the count here and the delay only after N seconds of uptime would keep the diagnostics honest without the hammering. That's probably a spec #36 decision rather than something to change here.

[XieX]

Agreed, and done here rather than deferred, since without a budget nothing else stops it. The backoff step is now separate from connection_failures. It resets only after a stream has stayed open 60s (matching BACKOFF_RESET_INTERVAL, and js-core's Backoff) or after a completed poll. The count still clears on a commit or none. Python a06237c, JS launchdarkly/js-ai-sdk@fdc0d81, plus the spec in #36.

[jeffdupont]

The two deleted tests were also the only Python coverage of a 400 that follows a recoverable failure. The repair still has to send a stateless request, and a second 400 still has to be fatal. #108 rewrote them for the no-budget world (asks from scratch after a 400 that follows a recoverable failure, still stops on the second 400 when a recoverable failure sits between them). Could Python keep equivalents, so the two suites stay in step?

[XieX]

Added in a06237c: test_a_400_after_a_recoverable_failure_still_repairs and test_the_second_400_still_stops_with_a_recoverable_failure_between. Also added from the JS side: wait_for_skills runs to its timeout during an outage, and fail/succeed/fail retries at the first backoff step.

[jeffdupont]

This scoping is right, and I'm fine with docs-only for 1.0 (see the review body). Three places still make the unscoped claim:

• skills_watch.py module docstring (lines 4-7): "a skill revoked in LaunchDarkly is removed from disk within a debounce interval". feat(AIC-3449): Support pulling in configs for evals from code #108 scoped the matching skills-watch.ts header to '*'.
• agents.md §6 has no explicit-list caveat. feat(AIC-3449): Support pulling in configs for evals from code #108 added one to its §4d.
• feat: Agent Skills #87's description example is watch_skills(refs, ...) with the comment "a revoked skill's SKILL.md leaves disk without a restart". That's the exact case this paragraph says doesn't work, and it will probably end up as the squash commit message.

[XieX]

All three fixed. The skills_watch.py docstring and agents.md §6 are scoped to "*" in a06237c. The #87 description now drops the "leaves disk" comment and adds a reviewer note on explicit lists. Making the watcher follow config changes is in progress as a separate, additive change.

[XieX]

Thanks for the review. The items in the review body:

• Oversized response: now fatal, handled like a 422. failed is set, connection_failures doesn't move, and last known good is still served. Python a06237c, JS launchdarkly/js-ai-sdk@232396d.
• connection_failures across languages: Python now matches JS. A goodbye after a completed exchange isn't counted, doesn't set last_error, and logs at debug in both SDKs. A goodbye before any answer still counts and warns.
• Stale "budget" comment: fixed.
• Spec (launchdarkly/ai-sdks-monorepo#36): all four points are added: positive, finite backoff options; the goodbye rule; when backoff resets; and Retry-After. Retry-After is deliberately left unjittered for now. The base SDKs don't read it at all, and there's no room under the cap for additive jitter.
• Deferred: the last-success timestamp on StoreDiagnostics (additive, after 1.0).
• Skill-key grammar (feat: Agent Skills #87 blocker 2): this has been fixed on the API side.