fix(client): apply changes after a none intent, and retry recoverable failures indefinitely by XieX · Pull Request #108 · launchdarkly/js-ai-sdk

XieX · 2026-10-02T17:44:11Z

JS counterpart of launchdarkly/python-ai-sdk#131, from review comments on launchdarkly/python-ai-sdk#87 that also apply to #71. Targets xie/agent-skills-feature-ac9ac7.

Changes

Apply objects that follow a none intent (r4167784120). serverIntent() now sets the intent to xfer-changes on none, as js-core's protocolHandler.ts does. Before, a put-object / delete-object after none went to ignoreUnderUnknownIntent() and was dropped while the next payload-transferred still advanced the basis, so a skill revoked after a routine reconnect kept being served.
Remove maxConsecutiveFailures; retry recoverable failures indefinitely (r4167784131). Only fatal statuses stop delivery and set failed. The 400-retried-once repair is unchanged.
Clamp the backoff exponent. 2 ** n reaches Infinity at large attempt numbers, and with a zero initialBackoffMs that gave 0 * Infinity = NaN.
Scope the on-disk revocation claims to '*' (r4167784141, docs only), in the README, agents.md and the skills-watch.ts header. Also fixes the resolveRequests doc and agents.md §4b, which said an unresolved reference makes a run incomplete.

Tests

New: put and delete after none (reader level), and a listener getting the tombstone on a 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; waitForSkills runs to its timeout during an outage; fails/succeeds/fails reports 1; an intent-then-drop repeated 5 times reads connectionFailures === 5; backoff at attempt 10,000 is finite; maxConsecutiveFailures is absent (source check plus runtime).
yarn test (1177 passed, 10 skipped), yarn typecheck, yarn code:check all pass.

Spec: launchdarkly/ai-sdks-monorepo#36

🤖 Generated with Claude Code

Note

Overview
Aligns FDv2 skill delivery with the Python SDK: fixes post-none updates, changes retry/fatal semantics, and tightens docs around disk revocation.

Protocol: After a none server intent, the reader now treats the connection as xfer-changes so later put-object / delete-object events apply instead of being ignored while the basis still advances—fixing revocations (and puts) after a routine reconnect.

Retry policy: maxConsecutiveFailures is removed; recoverable errors retry for the store’s lifetime and only fatal conditions (401/403/422, oversize bodies/events past 64 Mi, etc.) set failed and stop delivery. connectionFailures remains a diagnostic counter; backoff uses a separate backoffAttempt that grows on every reconnect and resets after a stream stays open 60s or a poll completes. Constructor now validates initialBackoffMs / maxBackoffMs (positive, finite, ordered).

Docs / reconcile semantics: README, agents.md, and skills-watch clarify that automatic on-disk revocation via watchSkills applies to '*', not an explicit skill list; skills-fs docs distinguish incomplete retrieval (no store, uninitialized, timeout) from absent references.

Tests: Extensive skills-fdv2 updates for indefinite retry, backoff behavior, fatal oversize handling, and §3.25 protocol cases.

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

A `none` intent says the basis is current, not that the connection is finished: later edits arrive on the same stream as put-object / delete-object and payload-transferred with no second server-intent. The reader kept `none` as an intent that carries no objects, so it dropped those events under objectsIgnored and then adopted the new selector as its basis anyway, leaving a revoked skill served across reconnects. Treat `none` as `xfer-changes` with nothing pending, as the base SDKs do (Python's ChangeSetBuilder.expect_changes(), js-core's protocol handler). The foreign-payload check on the following payload-transferred is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Remove the maxConsecutiveFailures option. A count bound turns a few minutes of outage into a process that receives no updates, and no revocations, for the rest of its life, and the base server-side SDKs do not give up on a recoverable error either. Only a fatal status (401, 403, 404, 405, 406, 414, 422, 501, the second or stateless 400, a catastrophic goodbye, an unexpected error) now stops delivery. connectionFailures still counts consecutive recoverable failures, drives the backoff, and is reset by a commit or a `none` intent. The 400 repair no longer needs an exemption from a budget, and is still retried once because the retry drops the state that made it retryable. backoffDelayMs clamps its exponent, so an unbounded attempt number stays finite (and a zero base no longer yields NaN). Tests that gave up after N failures are rewritten to assert retrying past ten, per-drop counting, fails/succeeds/fails reporting one failure, a finite backoff at attempt 10,000, and the option's absence by name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

watchSkills removes a revoked skill's SKILL.md without a restart only for writeSkills('*'). With an explicit list such as skillRefs(config), a skill the store answers absent for stays requested as an error action and is not pruned, and the watcher listens to the skill store only, so unpinning a skill from a config is not seen either. Also correct the docs that implied an absent reference makes a reconcile incomplete: only a missing, uninitialized or throwing store, or an exhausted timeout, does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jeffdupont

Parity follow-up to my review of python #131 (launchdarkly/python-ai-sdk#131), against my GA review of #71. I reviewed 9f0b870 against its base, xie/agent-skills-feature-ac9ac7 at 86265f3. yarn test: all workspaces pass, client 1177 passed and 10 skipped, exit 0. yarn typecheck and yarn code:check are clean. I reverted each fix on its own and re-ran the new tests. Without this.intent = INTENT_TRANSFER_CHANGES, all three none tests fail. Without the clamp, the finite-backoff test fails. With the base skills-fdv2.ts put back, the five retry/no-option tests fail.

Blocker status matches Python. (1) Changes after none and (3) giving up after 10 failures are resolved, and the behaviour matches #131. (2) The skill-key grammar is untouched and still needs the SDK-or-API decision. (4) Explicit-list revocation is docs only, which I think is acceptable for 1.0 for the reason in the #131 review: the frozen default is the safe one, and the fixes are additive. Removing maxConsecutiveFailures isn't breaking, because FDv2SkillStore has never been on main (checked with git grep origin/main).

One thing to settle before merge, the same as Python:

initialBackoffMs / maxBackoffMs aren't validated, and with no budget, a zero or negative value spins forever. Reproduced with an always-failing requester: about 860 attempts/s for initialBackoffMs: 0, maxBackoffMs: 0 and maxBackoffMs: -5, with failed staying null. The base branch stops after 11. The new test even asserts a 0 ms delay as valid output. Inline.

Smaller notes:

Parity is good on behaviour and names. The none handling, the removed option, the 400 one-retry repair and the fatal set all match. Python clamps the exponent at 62 and JS at 30. Both are fine, since either is far above any realistic cap.
connectionFailures disagrees with Python on a goodbye after a complete answer. JS exempts it (reachedServer, line 1810); Python counts it and sets last_error. This predates the PR, but the counter is now the main outage signal. The spec (launchdarkly/ai-sdks-monorepo#36) should say which is right.
Restart after a fatal error differs. Python's start() resumes a store that gave up. Here start() does nothing once failed is set (docstring at line 1536), and the give-up log says "until the process restarts". This predates the PR; I'm noting it because both PRs now describe recovery.
These apply to both languages; details in #131: Retry-After is used without jitter (line 1758), so a fleet reconnects together. Backoff resets on every none, where the base SDK waits for 60s of uptime, so a server that answers none and drops gets about 1/s from every process. A response over MAX_RESPONSE_CHARS is now re-downloaded every 15-30s and never sets failed.
The JS tests are more complete than Python's. waitForSkills running to its timeout, fail/succeed/fail retrying at the first backoff step, and the rewritten 400 tests are all missing on the Python side. I've asked for them there.
Spec #36 matches this PR on both behaviour changes. I've suggested adding backoff-option validation and goodbye counting there.

With no consecutive-failure bound, initialBackoffMs and maxBackoffMs are the only limit on the retry loop: a zero, negative or NaN value turns every wait into none, and a store pointed at a failing server reconnects as fast as the network allows. Both must now be positive and finite, and initialBackoffMs may not exceed maxBackoffMs; the constructor throws otherwise, as it does for pollIntervalMs (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…s open 60s The backoff delay followed connectionFailures, which a commit or a none intent resets. A degraded server that answers none and then drops every connection was therefore reconnected at initialBackoffMs indefinitely, by every process; with no failure bound, nothing else stopped that. The delay now has its own attempt number. It advances on every reconnect that follows a failure or a dropped stream, a goodbye recycle included, and returns to the first step only when the stream that just ended had been open for BACKOFF_RESET_INTERVAL_MS (60s), as js-core's Backoff does. A completed poll also resets it, since pollIntervalMs already spaces the requests. connectionFailures keeps its meaning: reset by a commit or a none intent. The interval is internal; tests shorten it through the instance's _backoffResetIntervalMs (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A poll body or streamed event over MAX_RESPONSE_CHARS (64 Mi characters) was a recoverable failure. The payload's size belongs to the environment, not the connection, so every retry was refused the same way: the store re-downloaded up to 64 Mi characters on every backoff step, from every process, for the life of the process, and never set failed. It is now a FatalTransportError, accounted like a 422: failed and lastError are set, connectionFailures does not move, last known good is still served, and exactly one request is made. As with a 422, a store that has given up does not resume; recovery is a restart or a new store, as the README already says for 422 (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A goodbye after a completed exchange is how the server recycles a long-lived stream, and the delivery loop already exempted it from connectionFailures and lastError. But ProtocolReader warned for every non-silent goodbye before anything knew whether the exchange had completed, so a healthy recycling stream still logged a warning. The reader now logs the goodbye at debug, and the loop logs an exempt reconnect at debug too. A goodbye before any completed exchange still counts, and still warns from the loop. Tests pin both: repeated answer-then-goodbye recycles read connectionFailures 0 and lastError null throughout with no warning, and a goodbye-only server warns (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Since the backoff step was split from connectionFailures, resetting the counter no longer changes the retry delay. Two test comments and one agents.md bullet still described a counter reset as what would pin a failing server to the initial backoff; they now describe what it does hide, the failure from diagnostics. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…uses The finite-backoff test asserted backoffDelayMs(10_000, 0, 30_000) === 0 as valid output. A zero base is no wait at all against a failing server, and the constructor now rejects it, so the case is unreachable; the constructor tests for initialBackoffMs cover it instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jeffdupont

Approving this, but there are still a couple gaps from the spec before we merge #71

… failures indefinitely (#131) Addresses three review comments on #87. Targets `xie/agent-skills`. ## Changes - **Apply objects that follow a `none` intent** ([r4167784120](#87 (comment))). 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](#87 (comment))). 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](#87 (comment)), 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](https://claude.com/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. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a06237c. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>

Picks up #108 (retry recoverable failures indefinitely, oversized responses are fatal). The one textual conflict in skills-fdv2.test.ts keeps both sides. #108 removed maxConsecutiveFailures, so there is no retry budget for a restart to reset. The "fresh retry budget" test is replaced by one that a restart starts connectionFailures over, and start() now also starts the backoff over. README and agents.md drop the retry-budget wording, and the oversize paragraph points at start() because its recovery is the 422 recovery, which this branch makes start(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…rst-payload Conflicts were the base's comment and agents.md tightening against this branch's 304/etag changes. Kept this branch's behaviour (no markFirstPayload on a 304; withhold commit and selector when nothing applied; adopt the etag only after a completed exchange) and the base's trimmed prose. Since #108 objects after a `none` intent now build a pending set, so the "applied" comment and the agents.md paragraph say "a `none` intent no object followed". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

XieX and others added 3 commits October 2, 2026 13:41

XieX mentioned this pull request Oct 2, 2026

fix(skills): apply changes after a none intent, and retry recoverable failures indefinitely launchdarkly/python-ai-sdk#131

Merged

XieX requested review from andrewklatzke and jeffdupont October 2, 2026 17:46

jeffdupont reviewed Oct 2, 2026

View reviewed changes

Comment thread packages/client/src/skills-fdv2.ts

Comment thread packages/client/src/__tests__/skills-fdv2.test.ts Outdated

Comment thread packages/client/src/skills-fdv2.ts

XieX and others added 6 commits October 2, 2026 16:38

XieX requested a review from jeffdupont October 5, 2026 16:10

jeffdupont approved these changes Oct 5, 2026

View reviewed changes

XieX merged commit 2709c66 into xie/agent-skills-feature-ac9ac7 Oct 5, 2026
10 checks passed

XieX deleted the xie/skills-fdv2-transport-fixes branch October 5, 2026 16:56

jeffdupont mentioned this pull request Oct 5, 2026

fix(client): a 304 confirms a payload rather than establishing one #104

Merged

5 tasks done

This was referenced Oct 5, 2026

fix(skills): close four behaviour gaps with the Python SDK #107

Merged

feat(skills): Agent Skills #71

Merged

Navigation Menu

Repository navigation

fix(client): apply changes after a none intent, and retry recoverable failures indefinitely - #108

fix(client): apply changes after a none intent, and retry recoverable failures indefinitely#108
XieX merged 9 commits into
xie/agent-skills-feature-ac9ac7from
xie/skills-fdv2-transport-fixes

XieX commented Oct 2, 2026 •

edited by cursor Bot

Loading

Uh oh!

jeffdupont left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeffdupont left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Changes

Tests

Uh oh!

jeffdupont left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeffdupont left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

XieX commented Oct 2, 2026 •

edited by cursor Bot

Loading