Conversation
`FDv2SkillStore` built both `/sdk/poll` and `/sdk/stream` from a single `base_uri`, so the default configuration sent its streaming request to `sdk.launchdarkly.com`. LaunchDarkly serves streaming from a separate host, and `mode="stream"` is this store's default (TESTING.md §3.25), so this was the default path rather than an edge case: the first contact with a real environment would have gone to the wrong host. Both base server-side SDKs ship the two as distinct defaults, which is the evidence this follows: ldclient.config.Config base_uri='https://app.launchdarkly.com' stream_uri='https://stream.launchdarkly.com' @launchdarkly/js-server-sdk-common baseUri: 'https://sdk.launchdarkly.com' streamUri: 'https://stream.launchdarkly.com' `DEFAULT_BASE_URI` here already pointed at the FDv2 polling host (`sdk.launchdarkly.com`, not `ldclient`'s older `app.` value), so only the streaming host is new. Adds `DEFAULT_STREAM_URI` and a `stream_uri` keyword, mirroring `skills-fdv2.ts`: a `base_uri` given on its own still applies to both endpoints, so a relay or a private instance serving both from one host needs only the one option, and naming both overrides them independently. `_require_https_base_uri` becomes `_require_https_uri` and takes the option name, so a cleartext `stream_uri` is refused under its own name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two independent transport defects. **`Retry-After: 0` had no floor.** `_retry_after_seconds` returns `max(0.0, seconds)`, and the retry site capped the value but never floored it, so a server — or an intermediate proxy — answering `0` had the delivery loop reconnect as fast as it could schedule, spending the whole of `max_consecutive_failures` in milliseconds and hammering the endpoint on the way. It is bounded, so nothing runs away, but the retry budget is gone and `Retry-After` is honoured in the least useful direction. Floored at `initial_backoff`, the same floor our own backoff starts from, matching `skills-fdv2.ts`. A short `Retry-After` is therefore raised to `initial_backoff`, which is what made `test_a_retry_after_header_is_honoured` discriminate before: it asked for 0.25s against a 5s initial backoff. Rewritten to ask for *longer* than our own backoff instead, so honouring the header is still the only way the assertion can pass, and `test_a_retry_after_header_is_parsed_off_the_wire` likewise — it was passing only because `poll_store`'s 0.05s `max_backoff` capped the floored value, so it no longer told the two apart. **`objects_revoked` counted tombstones for keys the store never held.** The increment fired whenever a `delete-object` parsed, whether or not it took anything away. The counter is operator-facing and gets read exactly when somebody is working out whether a revocation landed (TESTING.md §3.25, "Counters are assertable facts"), so an inflated figure misleads at the worst moment. Gated on `_SkillObjectSet.delete` having returned something. `changes` still carries every tombstone, so listeners are unaffected. Note this is a divergence from TypeScript rather than a port of it: `skills-fdv2.ts`'s `deleteObject` increments unconditionally too, and its `keysFullyRevoked` applies to the full-transfer revoke-by-omission diff, which this side does not implement. The same fix is owed there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three §3.25 deviations, all in the transport's lifecycle and reconnection rules. Grouped because each one's existing test pinned the old answer and had to move in the same change. **`close()` is final** (§3.25 lifecycle). A subsequent `start()` now raises instead of resuming delivery. Finality is what gives `close()` a postcondition a caller can rely on — delivery has stopped — including when the join times out on a thread parked somewhere no interrupt reaches. A store that could be restarted from there leaves the caller unable to tell whether delivery stopped, and a restart that silently never delivered again is the failure this forecloses. To resume, build a new store. Giving up at the failure bound is *not* closing, so a store that gave up can still be restarted and the re-arm path is unchanged. `test_a_restarted_store_waits_again` therefore drives the give-up path now, and `test_a_close_that_timed_out_leaves_the_store_restartable` becomes `..._is_still_final` — it was asserting exactly the behaviour finality removes. **HTTP 404 is fatal** (§3.25 reconnection). A 404 on `/sdk/poll` or `/sdk/stream` means the endpoint does not exist for this credential or instance — a mistyped base URI, typically — and no reconnect produces one. It was recoverable, so it burned the whole retry budget first. **HTTP 400 is retried exactly once, then fatal** (§3.25 reconnection). A 400 is what a stale `basis` selector looks like: the request carried state the server no longer recognises, and a connection built from nothing fixes it. It was fatal, so that repair never ran. Implemented as `_StaleRequestStateError`, a `_RecoverableTransportError` subclass whose handling drops `basis` and `etag` and asks for a full transfer — and gives up when there was neither to drop, since a request carrying no client state was refused on its own terms. That bound is what keeps this from being "400 is recoverable", and it needs no counter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`add_listener` recorded a registration for any kind and then never notified it, on both shipped stores — `InMemorySkillStore`, whose `put` only accepts skill objects, and `FDv2SkillStore`, which only ever delivers `SKILL_OBJECT_KIND`. TESTING.md §3.21 now requires both to raise. The argument is §3.26's own, the one that already makes `watch_skills` refuse a store with no `add_listener` at all rather than degrading to a one-shot reconcile: a listener that silently never fires "looks exactly like a watcher whose skills never changed". Registering on a kind the store will never deliver is the same failure with the same signature, and a store that accepted the registration has promised something it cannot keep. `ValueError` rather than `TypeError`: `kind` is a `str`, which is the accepted type, carrying a value the store cannot serve — the same distinction A.12 records for `write_skills` versus `get_skills`. `remove_listener` for an unregistered kind stays a no-op, so a caller tearing down need not track what it attached. `test_put_does_not_notify_other_kind_listeners` pinned the old behaviour and is replaced by the raise assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…window
**`debounce=float("nan")` defeated the guard** (TESTING.md §3.26). `nan
< 0` is `False`, so it passed `if debounce < 0` and then collapsed the
coalescing window to nothing: `threading.Event().wait(nan)` returns
immediately, so every delivered object reconciled on its own with no
coalescing at all — the opposite of what the option is for. Now
`math.isfinite` is checked alongside the sign, matching how
`write_skills` already guards its `timeout`. `inf` is refused for the
same reason it is refused there: a window nothing ever leaves.
Also fills the §3.26 test gaps, which had no coverage either way:
negative and non-finite debounce, `on_reconcile` firing for subsequent
reports and not the initial one, a reconcile that raises not killing the
watcher, `prune` / `on_unavailable` pass-through, and an invalid root
raising out of `watch_skills` rather than into a worker thread's log.
**The coalescing test was one of the vacuous shapes §3.26 warns about**
— twelve puts inside a single synchronous listener pass, asserted with
an upper bound (`reconciles <= 2`). A burst that arrives before the
worker has woken collapses to one reconcile whether or not the debounce
exists, so that shape passes against an implementation with the
debouncing deleted. Replaced with six puts spread across one window, an
exact figure, and an assertion that the counter moved at all; verified
by mutation — removing the debounce sleep turns it red.
The exact figure is two, not one, and that is a real deviation from
§3.26's "exactly one reconcile per debounce window": this watcher clears
its wake flag *before* the window rather than after, so a change
arriving inside the window schedules a further pass instead of merging
into the reconcile that is about to run. TypeScript restarts its timer
on each notification and so does merge them. The behaviour is
deliberate and documented in `_run` (a redundant reconcile converges;
a missed one does not) and it avoids the starvation a restart-the-timer
debounce admits, so it is left alone here and raised separately rather
than changed under a test-coverage commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`parse_ai_config` gated on `if skills is not None`, so `skills: null` was accepted and read as absent. TESTING.md §3.5 now calls this case out specifically, and the reason is the reason it is worth a commit of its own: read as absent, it makes `skill_refs` return `[]`, and a `prune=True` reconcile then **deletes** previously-materialized skill files on the strength of a field the SDK could not parse. §3.21 establishes exactly this principle on the store path — reading a malformed thing as absent "would let a tampered object look like a deleted one … and would let prune delete the last known-good copy on disk" — and it applies identically here. Failing the whole parse is the louder and safer outcome, and it is what TypeScript already does. Gated on key presence instead, so `_parse_skills` sees the `None` and rejects it as a non-array, with `skills` in the message. There was no Python test either way; both halves are now pinned, including that an *absent* `skills` key is still valid, since rejecting `null` must not cost backward compatibility for configs that simply have no `skills`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`verified_bytes` encoded before checking the size cap, so content that
is both over the cap and carries a lone surrogate reported `not_utf8`.
TESTING.md §3.21 fixes the order as shape, then **size**, then
**encoding**, then hash, and that input is the one boundary where the
order is observable: without it, the code is whichever check the
implementation happens to reach first, and §3.21's "one code per failure
class" rule cannot be tested against it. Size first is also the cheap
check — running an encoding pass over a 10 MiB body before rejecting it
for being 10 MiB is a DoS foothold rather than a nicety.
Python cannot reorder these literally, because the size is a count of
encoded bytes and `str.encode("utf-8")` raises on an unpaired surrogate
rather than substituting. So the unencodable case now encodes a second
time with `errors="replace"`, purely to measure, and reports
`over_size_cap` or `not_utf8` in that order. Those replacement bytes are
never hashed and never returned — the `not_utf8` branch returns before
the hash comparison — which is what makes `errors="replace"` safe here
where `errors="surrogatepass"` is not safe anywhere: fabricated bytes
that reached the comparison could satisfy it. TypeScript arrives at the
same order for free, since `TextEncoder` substitutes U+FFFD silently
and its guard is an explicit round-trip check (A.12).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…README `agents.md`'s POSIX-only justification claimed the TypeScript SDK "could not match them at all — Node exposes no `*at()` family on *any* platform, so its racy floor is universal rather than Windows-only." The first half is still true; the second is not. TypeScript commit `0a15b10` added a `SUPPORTS_PROC_FD` probe and `/proc/self/fd/<fd>/<name>` addressing, which the Linux kernel resolves from the inode a descriptor holds rather than from the name it was opened under — closing the swap window there exactly as `*at()` does here. TypeScript's `lstat` floor now applies on macOS and Windows only, the same shape as this side's. The passage is load-bearing — it is why the Windows reparse-point checks are not implemented — so the rewrite says explicitly that the decision is unchanged and why: it never rested on TypeScript being equally exposed, it rests on there being no Windows CI runner to verify the checks against, which is still true of both repositories. README, for the behaviour changes in this branch: the two transport hosts and the `stream_uri` option, `close()` being final, `watch_skills`' `debounce` unit and bound and its `on_reconcile` contract, `add_listener` refusing a kind it will never deliver, and a non-array `skills` field failing the config parse rather than reading as absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ 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 4092de7. Configure here.
An HTTP 400 for a request carrying a `basis` selector or an etag means that state may be one the server no longer accepts, so the transport drops it and asks for a full transfer once before treating the status as fatal. That one request was not guaranteed to go out: it was prepared and then measured against the consecutive-failure bound like any other retry, so a 400 arriving on a budget an outage had already spent gave up while holding the only request known to repair it, and delivery stopped for the process lifetime over state the store had just dropped. The repair is now exempt from the bound. It cannot unbound the loop: the repaired request carries no state, so a second 400 is fatal on its own, and any other failure after it meets a budget still over the bound. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes surfaced by the TypeScript code review that needed fixing on the Python side. Includes bugs discovered with the Python implementation, and contract changes made during the review that needed to be ported over to Python.
6ac1577stream_uri, abovec69d880Retry-After: 0floored atinitial_backoff;objects_revokedcounts only revocations that removed something86fd04cclose()is final; HTTP 404 fatal; HTTP 400 retried exactly once from scratch, then fatal (§3.25)2f59b9aadd_listeneron a kind the store never delivers raises, on both shipped stores (§3.21)daeedecdebouncerejected; the §3.26 watcher test gaps filled and the coalescing test de-vacuumed (§3.26)4a204f8skills: nullfails the config parse instead of reading as absent (§3.5)030e8c3over_size_cap(§3.21)4092de7agents.md's stale cross-language parity claim, and the READMEBehaviour changes worth a second look
close()no longer resumes.start()on a closed store raises; constructa new one. Giving up at the failure bound is not closing, so that restart
path is unchanged. Three existing tests pinned the old answer and moved with
the change.
add_listener("flag", fn)now raisesValueErrorwhere it used to recorda listener that never fired.
skills: nullnow fails the whole config parse. Previously accepted.🤖 Generated with Claude Code, updated by @XieX
Note
Overview
Ports TypeScript review fixes into the Python Agent Skills stack: FDv2 transport, store lifecycle, config/verification edge cases, and watcher validation, plus README/
agents.mdupdates.FDv2 delivery now treats polling and streaming as separate HTTPS origins (
stream_uri, defaulthttps://stream.launchdarkly.com), while a lonebase_uristill covers both for relay/private hosts. HTTP 404 is fatal; 400 with clientbasis/etag is retried once after clearing state, then fatal.Retry-After: 0is floored atinitial_backoff.objects_revokedincrements only when a delete actually removed held content.FDv2SkillStore.close()is final —start()afterward raises (including after a timed-out join); restart after giving up at the failure limit is unchanged.add_listeneron a non-skill kind raises on both shipped stores instead of registering a no-op listener.Config parse rejects explicit
skills: nullso prune cannot wipe disk on an unparseable field. Integrity verification runs size → encoding → hash, so over-cap content with a lone surrogate reportsover_size_cap.watch_skillsrejects non-finitedebounce(e.g. NaN); tests cover coalescing,on_reconcile, and option passthrough.Reviewed by Cursor Bugbot for commit c2d482a. Bugbot is set up for automated code reviews on this repo. Configure here.