Conversation
First of five slices splitting the Agent Skills feature for review. This one
adds the layer with no I/O in it at all: the types, the validation, and the
projection from a resolved AI Config to the skills it references.
- `skill_refs(config)` projects a config's `skills` array into
`list[SkillReference]`. Pure — no client, no store, no network, no telemetry.
- `Skill` and `SkillReference` are frozen dataclasses, exported from the package
root. `Skill.content` is `bytes` — the verified verbatim bytes LaunchDarkly
delivered, exactly what was hashed. Skills are opaque byte buffers by
construction: the SDK never parses, decodes, or interprets skill content
anywhere. `content_hash` is the sha256 (lowercase hex) over those bytes, and
the optional display metadata comes from LaunchDarkly, never from the content.
- `parse_ai_config` now validates the optional `skills` array and fails closed
on a malformed one. Key grammar, length bound, and the version predicate live
in `types_validation.py` as one canonical rejection reason, so every layer
added on top rejects a key for the same stated reason.
Note one behaviour change for existing users: `parse_ai_config` fails closed on
a `skills` value that is not a list of `{key, version}` objects, where before
any value parsed and was ignored. A variation carrying its own differently
shaped `skills` field must rename it before upgrading.
The two layers that follow — retrieval through a store seam, and
materialization onto disk — are separate slices. `agents.md` names all three
and the one-way dependencies between them.
Testing: `uv run pytest` → 1048 passed. `ruff check`,
`ruff format --check`, and `mypy packages/*/src` all clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second of five slices. Adds the layer that turns a reference into content: an
injectable store seam, integrity verification of everything it serves, and a
body-free telemetry seam for the failures.
- `get_skill(key, *, version=None)` returns one verified skill, or None.
- `get_skills(refs)` is the batch form, accepting references and bare keys.
- `all_skills()` returns every verified skill the store holds, one per key.
- `SkillStore` is the structural interface content arrives through —
`get_object(kind, key, version=None)`, `all_objects(kind)`, and an optional
`add_listener(kind, fn)` — configured with
`init_client(options={"skillStore": store})`. `InMemorySkillStore` ships for
local development and testing. A delivery transport drops in behind the same
seam with no public API change.
Store data is untrusted. Key and version are revalidated, size is bounded, and
the sha256 of the verbatim bytes must match the delivered `contentHash`;
anything that does not verify is withheld and treated as missing, so no
unverified content is ever returned. The wire object delivers content as a JSON
string; the UTF-8 encode happens exactly once, inside verification, and the
`Skill` handed to user code carries the verified verbatim bytes
(`Skill.content: bytes`) — the exact byte sequence that was hashed, never a
re-derived value. Content carrying an unpaired surrogate has no UTF-8 encoding
at all and is withheld too — `str.encode` is called strictly, never with an
error handler that would fabricate bytes a hash comparison could then accept.
`verified_bytes` also accepts already-bytes content, hashing it directly, for
the pre-write re-verification pass a later slice adds.
Integrity failures are reported through a private telemetry seam carrying
hashes and byte counts only, never the skill body. The two properties copied
off the wire, `skill_key` and `expected_hash`, are shape-checked and replaced
when malformed, so a hostile store cannot use either one to publish the body
through a signal that is otherwise body-free. The default emitter is a no-op:
nothing leaves the process in this release, and the three signal names are an
allowlist maintained in one section of one module.
Version is part of the lookup identity rather than a filter applied to the
answer. A delivery payload carries the newest version of every skill plus every
version any variation currently pins, so two versions of one key coexist
routinely; a seam keyed by key alone would answer a pinned reference with the
newest object and then reject it, turning the primary use case into a missing
skill. `InMemorySkillStore` holds several versions of a key, `get_object` takes
the wanted version, and `version=None` means "the newest you hold". The
equality check afterwards is kept as a defense — the store is untrusted, so an
answer that is not the version asked for is withheld.
`all_objects` returns one entry per key-and-version under keys that are opaque
to this SDK; identity is read off each object's own fields. `newest_by_key` is
the single place that collapses the result to one object per key.
A run that withheld anything now logs a count at WARN. Every individual
withholding already records a signal and an error line, but a caller reading
logs at WARN saw neither, and a payload where nothing verifies otherwise
returns an empty result indistinguishable from "this project has no skills".
`SKILL_OBJECT_KIND` is deliberately **not** exported from the package root. It
is the string this SDK hands a store, and an adapter maps whatever the transport
underneath calls a skill onto it; publishing it would advertise an SDK-side seam
value as the wire contract. An adapter that needs to agree with it reaches it
through `skills_core`. `MAX_SKILL_CONTENT_BYTES` stays internal for the
adjacent reason.
Note one behaviour change: `shutdown()` clears the configured skill store along
with the client. `init_client` applies `skillStore` on every successful call,
even the idempotent ones, which is what lets a lazily auto-initialized client be
given a store afterwards.
Testing: `uv run pytest` → 1145 passed. `ruff check`,
`ruff format --check`, and `mypy packages/*/src` all clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An integrity failure now writes a structured, machine-parseable ERROR record on the SDK's own logger, designed to be ingested by a SIEM and alerted on. This is the detection path that works when telemetry is off, and the only one that exists at all in an instance with no telemetry destination — so it is a documented contract rather than a debugging aid. The LD-side counter is left exactly as designed: opt-out respecting, no-op by default, property set unchanged. `reason_code` lives in the log record only. - `ld.skills.integrity_failure` is the stable event name, and it appears in the message text rather than only in `extra`. Severity cannot discriminate — a raising store also logs ERROR from this module — and the stdlib's default formatter drops `extra`, so an `extra`-only record is invisible under a plain `logging.basicConfig()`. - The message is the event name plus compact key-sorted JSON, so the line is greppable, `jq`-able, and byte-identical across LaunchDarkly's AI SDKs for the same input. The same mapping is attached as `extra["ld_skills"]`. - `reason_code` is a closed vocabulary of eight tokens, one per `record_integrity_failure` call site, typed as a `Literal` so a typo at a call site is a type error. - The record spreads the signal's properties rather than rebuilding them, so the two cannot drift on which fields are redacted or omitted. Optional fields are omitted, never nulled. No new untrusted value, and no path. Documented for customers in the README and for contributors in agents.md, including the full vocabulary, so a ninth reason cannot land in one language only.
Third of five slices. Adds `safe_fs.py`, the "write a file under a directory something else may be racing you for" problem solved once. Nothing here knows what a skill is; the materialization layer is its only caller, and it lands next. A path check is only as good as the last path resolution after it. Every `lstat` and containment check validates an inode, but a following `os.replace(tmp, dir / name)` re-resolves `dir` from its name — so anything holding write permission there can move the validated directory aside, leave a symlink in its place, and redirect the write or the unlink somewhere else. Narrowing that window is not a fix; the race is winnable at any width. So the checks hand off to a descriptor and nothing re-resolves a path afterwards. - `open_directory_nofollow` opens with `O_RDONLY | O_DIRECTORY | O_NOFOLLOW` and confirms `S_ISDIR` on the `fstat`, since not every platform defines `O_DIRECTORY`. `open_or_create_directory` adds `os.mkdir` plus an `lstat` on the `FileExistsError` path, because `Path.mkdir(exist_ok=True)` accepts a symlink-to-directory as "already there" and would reopen the hole the caller's check just closed. `pinned_directory` holds either for a block, so a caller states the platform split once and cannot forget the close. - `atomic_write` creates its temp file with `O_CREAT | O_EXCL | O_NOFOLLOW` at that descriptor, `fchmod`s the descriptor rather than a path, writes, fsyncs, renames, and fsyncs the directory so the rename survives a crash. Mode is set explicitly at 0644, never inherited from the umask and never executable. `os.replace` is the single rename call site and `os.rename` must not be substituted for it. - `unlink_file` probes and unlinks descriptor-relative. `unlink` never follows a trailing symlink but it does resolve the directory above it, so the same swap turns a removal into a delete of an arbitrary file. A symlink found where this SDK expects its own file raises `SymlinkRefused` rather than being tidied away — the state on disk is not what the caller believes, and that is the caller's to report. `SUPPORTS_DIR_FD` gates all of it, and the probe deliberately names `os.rename`/`os.stat` rather than the `os.replace`/`os.lstat` this module calls: `os.supports_dir_fd` is populated per underlying syscall, and CPython registers `renameat` under `rename` only and `fstatat` under `stat` only. Probing the names actually called reports "unsupported" on every POSIX platform and silently turns the defense off. Where the family is absent, every operation falls back to the identical full-path sequence. The tests here exercise the module directly, on its own terms. The TOCTOU races these primitives exist to close are proved through the materialization layer, which is what holds a descriptor across a sequence of operations. Testing: `uv run pytest` → 1260 passed, 11 skipped. `ruff check`, `ruff format --check`, and `mypy packages/*/src` all clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fifth and last of five slices. The adversary. Every filesystem defense the previous two slices introduced now has a test that fails if the defense is removed, plus the materialization telemetry allowlist. - **Path traversal.** A key that escapes the root, a key that is the manifest filename, an over-long key, and a key that resolves outside after `realpath` — each refused before any filesystem call, and the resolved containment check asserted on inode identity rather than on path strings. - **Symlink attacks.** A symlinked skill directory, a symlinked target file, and the `<root>/<key>` directory swapped for a symlink at the exact instant of the rename and of the unlink — the narrowest version of the window the descriptor pinning exists to close, fired from the interception point rather than from implementation internals. - **The no-`*at()` shape.** The path fallback Windows takes for every operation, exercised with the capability probe forced off, so the platform that cannot pin a descriptor is not the untested one. The TOCTOU tests skip off that same flag deliberately: a probe that wrongly reported "unsupported" cannot also silently skip the tests that would have caught it. - **Non-regular files and clobber protection.** A fifo or a directory where `SKILL.md` belongs, and a file at a managed path with no matching manifest entry — reported and left alone, never overwritten and never removed. - **Corrupt manifests.** Unreadable, unparseable, not an object, malformed entries, and a `manifestVersion` this release cannot read: no overwrites, no prunes, an error action naming the manifest, and the manifest itself left as it was found. - **Atomicity.** A crash injected between the write and the rename leaves neither a partial file nor a temp file, and the one recorded rename is proved to have moved `SKILL.md` within the target's own directory — by descriptor identity where the platform has `renameat`, which also rules out the descriptor having been redirected between the check and the rename. - **Telemetry.** The three signal names are asserted as an allowlist rather than a floor: any other name reaching the emitter fails, the two deliberately excluded names are called out by name, no signal carries a filesystem path or the skill body, an emitter that raises never fails the reconcile, and `client.track()` is never reached. Two of these are worth naming, because the obvious test does not reach the guard. The unencodable-content cases pin `contentHash` to the sha256 of the bytes a non-strict encoder would have fabricated, since an arbitrary wrong hash is rejected by the mismatch check first and never exercises the encoder guard. The redaction cases smuggle the body through `contentHash` and through `key`, since a sweep using a well-formed digest under a valid key reaches neither replacement branch. Testing: `uv run pytest` → 1280 passed. `ruff check`, `ruff format --check`, and `mypy packages/*/src` all clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fourth of five slices. Adds `write_skills`, which writes
`<root>/<key>/SKILL.md` and reconciles against a manifest recording what the
SDK owns, so it overwrites or removes only files it wrote — a file you placed
yourself is reported and left untouched.
report = await write_skills(refs, ".claude/skills")
`skills` accepts `Skill` values, references, bare keys, or the literal `"*"` for
everything the store holds. Every outcome is visible in the returned
`ReconcileReport`: one `ReconcileAction` per skill, carrying `written`,
`updated`, `skipped_current`, `removed` or `error`, plus `.ok` and `.errors`.
A failure belonging to the run rather than to one skill — an unreadable
manifest, a retrieval that failed before any key was known — carries the empty
string as its key.
Writes are atomic, at mode 0644, and every destructive step runs against a
descriptor pinned to a directory that was already checked, so a path swapped
after the check cannot redirect a write or an unlink out of the managed root.
Where the platform has no `*at()` family the identical sequence runs against
full paths.
The defenses, all of them deliberate and all of them tested:
- The key is re-validated here regardless of upstream validation, before any
filesystem call, because a key becomes a directory name. The data model
allows 256 characters and `NAME_MAX` is 255 bytes, so an over-long key is
refused too.
- Never write or unlink through a symlink, on the write path or the prune path.
- Destruction only on manifest-listed paths whose key matches.
- A corrupt manifest fails closed: no overwrites and no prunes, brand-new paths
may still be written, an error action names the manifest, and the manifest is
not rewritten.
- An incomplete retrieval suppresses pruning, so a transport outage cannot read
as "everything was revoked".
- Content is re-verified immediately before the write, because a `Skill` can
also be constructed directly by a caller.
Pruning removes formerly-managed skills that are no longer referenced, which is
how revocation takes effect. `timeout` bounds retrieval, the writes and the
pruning; only the final manifest rewrite runs past it, so files already written
are never orphaned.
`write_skills` performs synchronous filesystem I/O and does not yield — it is
`async` for signature parity with the other accessors. Reconcile one root at a
time: a run is atomic against the rest of the loop today, so wrapping it to run
concurrently makes two runs against one root race on the manifest.
The `"*"` form collapses to one object per key at its newest version, since
`<root>/<key>/SKILL.md` is a single path and writing it twice in one run is a
bug rather than a policy, and it reports a withholding count at WARN for the
same reason the accessors do.
The security abuse matrix — path traversal, symlink attacks, clobber
protection, corrupt manifests, atomicity under an injected crash, and the
materialization telemetry allowlist — is the next slice. The guards it exercises
are all here; what lands next is the adversary that proves each one fails
without them.
Testing: `uv run pytest` → 1216 passed. `ruff check`,
`ruff format --check`, and `mypy packages/*/src` all clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
``test_key_at_the_data_model_bound_is_reported_not_raised`` read ``dst_dir_id`` directly, but that field is only populated when ``os.replace`` is called with ``dir_fd`` kwargs. On the path fallback (``SUPPORTS_DIR_FD`` false — the shape Windows takes) it stays ``None``, so the assertion failed even though the valid skill had been renamed correctly into its own directory. ``_assert_atomic_rename_of`` already branches on both call shapes and asserts the same containment property, plus the single-rename count the list comparison implied. Use it. Verified by forcing the probe off for a whole session: this was the only test in the module that broke under the no-``*at()`` shape, and the helper-based check passes under both. Reported by Cursor Bugbot on #54. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…esystem can hold Two gaps from the Agent Skills security design review, both in `skills_fs.py`: AV-3 (partial reconciles are unrecoverable) and DV-2 (the key grammar admits Windows device names). **AV-3 — adopt a file whose bytes already are the resolved content.** `_write_all` reconciles every skill and only then rewrites the manifest, once, last. A process killed in that window leaves a skill file at a managed path with no manifest entry — exactly the condition `_write_one` treats as an unmanaged-file collision, so the skill was wedged permanently: every later reconcile took the same refusal branch. Boot-time execution under a ten-second budget makes the crash window realistic. `_write_one` now reads and hashes first and decides from the bytes. Content byte-identical to what LaunchDarkly resolved is adopted — manifest entry recorded, reported `skipped_current` — and anything else falls through to the same refusal as before. This cannot weaken the clobber guarantee: differing unmanaged bytes are never overwritten, and the existing clobber tests pass unchanged. Three details carry the safety: - A read that fails is a refusal, never an overwrite, with a message distinguishable from the byte-mismatch refusal — it is the comparison that would otherwise authorize the write, and a file that could not be read has not been shown to be ours. - The read stays on `_read_regular_file`. Adoption widens it to genuinely foreign files, so its refusal of FIFOs and other non-regular files is now load-bearing rather than defensive. It gains a `max_bytes` bound of `len(content) + 1` — enough to prove inequality for anything longer, and what keeps a foreign file of arbitrary size out of memory. A bound of exactly `len(content)` would adopt every file that merely begins with the resolved content. - `skipped_current` is reused rather than adding an `adopted` action kind, so `ReconcileActionKind` — public, and owned by an approved PR — does not change. Its documented meaning already fits. Adoption also makes the file prunable later. That is correct rather than a weakening: only byte-identical LaunchDarkly content is ever adopted, so a later prune removes content LaunchDarkly delivered anyway — what would have happened had the crash not occurred. The review also floats a write-intent journal. Assessed as over-engineered; not built. **AV-3, secondary — sweep orphaned temp files.** `atomic_write` unlinks its temp file on any exception but not after a `SIGKILL`, and `_prune` walks manifest entries, which an orphan never has, so nothing would ever notice one. The second-order effect is worse than the disk: `_prune_one`'s `rmdir` only succeeds on an empty directory, so a single orphan pins a skill's directory forever. The sweep runs on both the write and the prune path, and is the one place this SDK removes a file the manifest does not list, so it is bounded on every axis: inside `<root>/<key>/` only, for a key that passes `_key_rejection_reason`; only names `safe_fs` itself recognizes, via a new `is_temp_name` beside the naming code rather than a copy of the format string that could drift from it; only regular files, with the type read off the descriptor; unlinked through the pinned descriptor. It never raises and never aborts a run. **DV-2 — reject the 22 Windows reserved device names.** `con`, `prn`, `aux`, `nul`, `com1`–`com9`, `lpt1`–`lpt9` are all valid skill keys and none can be a directory name on Windows. Rejected in `_key_rejection_reason`, which the write and prune paths already share, and *not* in the key grammar: `parse_ai_config` fails closed, so a grammar-level rejection would invalidate an entire AI Config for a Linux customer over a Windows-only constraint, and would silently shrink `skill_refs` — which is what authorizes a prune, turning "fails to write on Windows" into "gets deleted on Linux". The 255-byte component bound is in this layer for the same reason. Unconditional, not platform-gated: a root written from a Linux container is routinely read from a Windows host, and neither repository has a Windows CI runner, so a gated branch would be untestable — the condition that produced the gap. No suffix stripping and no case folding: the grammar admits no `.` and no `$`, so `con.txt` and `CONIN$` are unreachable, and keys are lowercase-only. `com0` and `lpt0` are not reserved and are not included. The trade is real and belongs in the release notes: a customer who legitimately names a skill `aux` now gets a reported `error` action on Linux where it previously worked. Neither gap emits the integrity-failure log record. A key rejection and a clobber refusal are `ReconcileAction` errors, not integrity failures. Tests cover crash-mid-reconcile recovery end to end (adopted, reported, recorded, and the next reconcile an ordinary no-op), byte-differing content still refused untouched, an unmanaged FIFO refused without hanging, a read failure refusing rather than overwriting, the `len + 1` off-by-one, the sweep and the `rmdir` it unblocks, lookalike temp names and a symlink wearing one left alone, all 22 reserved names through both destructive paths, and — the point of the layer choice — each reserved name still valid to `is_valid_skill_key`, `parse_ai_config`, and `skill_refs`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…failure ``get_skill`` returns ``None`` for four unrelated outcomes: no such skill, the store raised, the requested version is not the one held, and content that failed hash verification. A caller cannot fail closed on suspected tampering while tolerating a merely-absent skill, so no automated customer-side response is possible — finding LA-2 of the Agent Skills security design review. The information already existed internally, as prose in ``Resolution.error``. This gives it a token: ``Resolution`` grows a typed ``reason``, set explicitly at every construction site and declared without a default so a sixth outcome added later has to choose which public token it maps to. ``get_skill_result`` maps that straight through to a frozen ``SkillOutcome`` (``skill``, ``reason``, ``detail``). Deriving the public reason by matching the error string is the fragility LA-2 is about, so the mapping is readable in one table. ``get_skill`` is untouched — its ``None``-for-every-failure contract is documented in its docstring and in the README, and a test now pins that all four failures still collapse to ``None`` and still never raise. Nothing new is emitted: Gap 1's integrity record already fired inside verification before ``resolve_from_store`` returned, and a test asserts one failed retrieval still produces exactly one record and one signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… remainder The four items left open in the response to the Agent Skills security design review. Three are documentation, one is tests; no behavior changes, and the code halves of rows 9 and 2 are deliberately untouched. **Privilege separation** (row 9 docs half, and the agreed counter-proposal for row 26). The recommended deployment runs the reconcile as a different identity than the agent, which is the whole reason the ``0644``/``0755`` modes deny anything: the agent reads its instructions and cannot rewrite them, or the manifest. That is the mitigation for AZ-1, a prompt-injected agent editing its own skills. Write access to the manifest is the worse half — it is what tells the *next* reconcile which paths the SDK may delete — which is why ``_prune`` re-validates every entry from scratch rather than trusting it. The README documents the pattern and hands the operator the check to run, because the SDK cannot run it: it knows only its own identity, which trivially has write access, having just written there. So ``ReconcileReport`` grows no writability field — the review asked for one and we declined, since any check the SDK could make would answer a different question than the one asked and manufacture false confidence exactly where caution is wanted. ``agents.md`` records that reasoning so the field is not added later by someone reading its absence as an oversight. **Three hostile-manifest prune tests** (row 12 remainder): a well-formed manifest listing ``/etc/passwd``, ``../../../etc/passwd``, and a path under a parent that has since become a symlink. ``_prune`` already refuses all three, so these turn asserted into verified. Two things make them worth more than their line count. They are deliberately *well-formed* — the corrupt-manifest suite above them proves nothing here, because a corrupt manifest suppresses every destructive action wholesale, whereas these manifests give the implementation everything it needs to prune. And "deleted nothing" is asserted through an unlink spy rather than by checking that ``/etc/passwd`` still exists: the test process cannot delete that file anyway, so the obvious assertion would pass against an implementation with no path check at all. **One sentence on** ``"*"`` (row 16 remainder). It materializes the whole project library, so every skill's ``description`` enters the agent's context — including skills no AI Config references and skills belonging to other teams. **The Windows platform bound is now explicit** (row 2 residual), in ``safe_fs.py``, ``agents.md`` and the README. Reparse-point checks (``GetFileAttributesW`` / ``FILE_FLAG_OPEN_REPARSE_POINT``) are not implemented, by decision: Windows is not a supported or tested platform for this release, neither repository has a Windows CI runner so the checks would ship unverified, and the TypeScript SDK could not match them in any case because Node exposes no ``*at()`` family on *any* platform. Implementing them in Python alone would break cross-language parity and trade a documented bound for an unverified one. Two consequences are recorded rather than left to be rediscovered: on Windows, write permission on the managed root is the only boundary, which is what makes privilege separation the mitigation and not merely advice; and this retroactively lowers the priority of the row 25 reserved-device-name work, noted where that code lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…does not close Three expected-failure tests (TestRootSwapRaces) for SEC-8985 row 2. The SDR response says every destructive operation runs relative to a descriptor held for the duration of the reconcile. The code pins <root>/<key> per operation and never holds the root: _resolve_root validates it once and returns a path, and each write and prune re-opens <root>/<key> by path with O_NOFOLLOW, which guards only the final component. A root swapped for a symlink after validation redirects the open, and every descriptor-relative step behind it, into the attacker's directory. Precondition is write permission on the root's parent, which the README checklist does not mention. The tests state the contract (nothing lands outside the root, no outside file is overwritten, no outside file is removed) and are marked xfail(strict=True, raises=AssertionError) so the suite stays green, the gap is recorded next to the other race tests, and the fix cannot land without removing the marker. Run with --runxfail to see the three escapes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit c22688b)
The expected-failure marker made the suite green while the contract was violated. A red run is the demonstration: the tests assert the contract, the code does not meet it, and they go green when the root is pinned for the reconcile, with nothing to remove. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 37ddb6a)
SEC-8985 row 2. The SDR response says every destructive operation runs relative to a descriptor held for the duration of the reconcile. It did not. `_resolve_root` validated the root and returned a plain `Path`; nothing held it open. Each write and each prune then opened `<root>/<key>` by path with `O_NOFOLLOW|O_DIRECTORY` and pinned that — and `O_NOFOLLOW` guards only the final component, so the root and every ancestor were re-resolved on every such open. A root renamed aside and replaced with a symlink after validation redirected the open, and with it every descriptor-relative step behind it, into the attacker's directory; on the create path `os.mkdir(<root>/<key>)` followed the link as well, and `mkdir` follows a symlink at its parent. The manifest write was the one operation that pinned the root, and it ran last, by which time the skill files were already outside it. The attacker precondition is write access to the root's *parent* — `.claude` for a root of `.claude/skills` — which the README checklist did not mention. `write_skills` now opens the root once, immediately after `_resolve_root`, with `O_RDONLY|O_DIRECTORY|O_NOFOLLOW`, confirms `S_ISDIR` on the descriptor, and holds it until the call returns. The descriptor is threaded through `_write_all`, `_prune`, `_rewrite_manifest`, the orphan sweep and the per-skill helpers, and every destructive step names a bare component against it: `os.mkdir(key, dir_fd=root_fd)`, `os.open(key, ..., dir_fd=root_fd)` for the skill directory, `os.rmdir(key, dir_fd=root_fd)`, and `atomic_write(..., dir_fd=root_fd)` for the manifest. A root swapped in the one interval left — after validation, before the open — fails `O_NOFOLLOW` and is reported as a run-level error with nothing touched, rather than as the `ValueError` an unusable root raises. `safe_fs`'s three openers take a `dir_fd` for the *parent* rather than growing a parallel API, and `SUPPORTS_DIR_FD` now probes `os.mkdir` and `os.rmdir` alongside the four it already named. Where the `*at()` family is absent the per-component `lstat` floor runs exactly as before: the root open returns `None` there, and every call site keeps its full-path branch. `_unsafe_path_reason` stays and still runs, but it is documented as defense in depth rather than the boundary — every check in it inspects a path, so each is a check-then-use against anything that can rename a component of that path. Security's three tests fired only when the intercepted `os.mkdir`/`os.open` was handed the absolute `<root>/<key>`, which the fix stops passing — they would have gone green while asserting nothing. The trigger now matches the bare key as well, so the swap fires in both worlds and the tests fail before the fix and pass after it. Two tests added: the root swapped before the pin is refused at the run level, and an audit that across a full reconcile no destructive call names an absolute path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The privilege-separation checklist denied the agent identity the managed root, the per-skill directories, the files and the manifest, and said nothing about the root's parent. That was the precondition for the SEC-8985 row 2 root swap: renaming any ancestor is what lets the root be replaced with a symlink, and in the documented `<app>/.claude/skills` layout the parent is `.claude`, which an agent identity is otherwise likely to own outright. The checklist and its shell snippet now walk every ancestor up to `/`. Write access to one of them is a strictly larger capability than racing the reconcile — no timing is involved, it persists until someone notices, and descriptor pinning inside `write_skills` cannot address it, because the substituted tree is what the agent reads rather than what the SDK wrote. The platform-bound paragraph claimed a descriptor held for the whole reconcile before that was true of the root; it now describes what is actually held and for how long, and names the root's ancestors alongside the root as the security boundary on every platform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
write_skills is a one-shot reconcile, so a revocation takes effect at the next process restart. watch_skills runs that reconcile now and again whenever the configured store reports a change, so a revoked skill's files leave the disk within a debounce interval of the store learning about it. It is wired to the SkillStore interface, not to any one transport: it needs a store that implements add_listener and nothing more, and refuses loudly when the store does not, since a watcher that silently never fires looks exactly like one whose skills never changed. Above the interface, remove_listener joins add_listener as the optional second half of change notification, on the SkillStore contract and on InMemorySkillStore. SkillWatcher.close needs it to detach; without it a store held every watcher ever created for the rest of its life. The watcher probes for it, so a store without it keeps working. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…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>
``InMemorySkillStore.get_object`` consulted the version-less entry on any pinned miss, while the unpinned path consulted it only when nothing well-formed was filed under the key. A key holding both well-formed versions and one malformed object therefore answered a pin for an undelivered version with the malformed object, and verification recorded an integrity failure — an alert pointed at a skill whose integrity was never in question — where the honest answer is that the version is not held. Both paths now follow the one rule: the version-less entry answers only when nothing well-formed is filed under the key, which is the case it exists for. A malformed object that is all the store holds still reaches verification and is still withheld with a signal, so tampering cannot read as a skill that was never delivered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two untrusted-store answers were read as data rather than as failures.
``list_raw_objects`` collapsed a non-mapping listing to ``{}`` with no
error, so a store that served nothing usable was indistinguishable from
one holding no skills. ``resolve_from_store`` read identity off the
object without checking it against the key that was asked for, so an
answer served under a different key came back under the caller's key
while carrying its own.
Both are now withheld and reported, alongside the version check that
already guarded the same way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oncile watch_skills awaited write_skills and only then constructed SkillWatcher, which is where the store listener attaches. The reconcile snapshots the store as its first step and then spends the rest of its time on the filesystem, so every write, fsync, prune and manifest rewrite in that first pass ran with nothing listening. A change delivered in that window was never seen, and since nothing re-reconciles on a timer, a revocation that landed there waited for the next unrelated change — on a quiet root, the next restart. Exactly the gap watch_skills exists to close. The watcher now attaches its listener before the initial reconcile and starts its worker after. notify only sets an event, so a change arriving mid-reconcile is recorded and picked up by the worker's first pass, while holding the thread back keeps write_skills's one-root-one-reconcile contract: the worker cannot race the caller's own reconcile over the same manifest. A reconcile that raises detaches the listener on the way out, since the caller is handed an exception rather than a watcher to close. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings in the withheld-answer fix for broken store answers, which the materialization path above this branch has regression tests for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings in the withheld-answer fix the reconcile regression tests below depend on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the reconcile side of the withheld-answer fix: a listing that is not a mapping leaves every managed file alone rather than reading as a full revocation, and an answer served under a different key writes nothing, is reported against the key that was asked for, and does not reach that other key's file. Each one previously deleted a file and reported a clean run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
The client-side cap was 64 KiB, close enough to the platform's own limit that any backend increase would force an SDK release. Raise it to 10 MiB so the guard stays a backstop against absurd input rather than a second enforcement of a bound this side does not own, and the real limit can grow without the SDKs moving. 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>
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) <!-- CURSOR_SUMMARY --> --- > [!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_SUMMARY -->
…tered Addresses Bugbot on #126. The previous commit gated the global teardown on `_tracer_provider` being set, which says we *built* a provider, not that we own the global. `_setup_telemetry` assigns the handle after `set_tracer_provider`, whose set is refused when another library got there first — so in a process with an existing provider (auto-instrumentation, an APM agent, or an app that configures its own), `shutdown()` cleared that provider and reset the `Once`, leaving the global a no-op proxy and silently killing the host application's tracing. Track whether our set actually took, via `trace.get_tracer_provider() is provider`, and gate the release on that. The provider is still shut down either way since we built it and it owns an exporter and a batch timer. Also warn when the set is refused: the caller's `otlpEndpoint`/`serviceName` cannot take effect, and the only existing signal is OTel's own terse warning. The same flaw was in the JS change this ports from; fixed there too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`_client is not None` is `_resolve_client`'s idempotency guard, but `_client` was assigned before `_setup_telemetry` ran. When setup raised — a malformed `OTEL_EXPORTER_OTLP_TIMEOUT` does, with a ValueError from the OTLP exporter — `_client` stayed set. The next `init_client()` returned that half-initialized client as a silent success with no telemetry, hiding the config error, and on the SDK-key path the LD client's connection was never closed. That contradicts `init_client`'s documented promise that a call which raises leaves no global state behind. Assign `_client` only after setup succeeds, on both paths. On the SDK-key path close the client we built when setup fails; on the BYOC path leave it open, since the caller owns it. Found in a pass over the lifecycle guards following the ownership fix; the JS SDK's counterpart was a cached failed-init promise, fixed in launchdarkly/js-ai-sdk#103. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 2, 2026
The three blocks the previous commit added were arguing the whole case inline —
the prune chain, the forward-compatibility reasoning, the cross-language note —
in 50-odd lines of comment on a 14-line change. This source gets read by
customers and their agents working out how to consume the SDK, and that much
nuance about internals they cannot reach is a wall to read past rather than
help.
Each block now states the rule and the one consequence that is visible from
outside: `is_initialized()` / `isInitialized()` goes true on a commit, and that
is what authorizes a prune of the files on disk. The long version already has
two homes it belongs in — `agents.md` in this package, and TESTING.md §3.25 —
so nothing is lost, and the clause that stops a plausible wrong fix ("not up to
date either: only the `none` intent says that") is kept.
Comments only; no behaviour change. Tests untouched, where the long-form
reasoning is the point.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Trim comment blocks to the essentials, restructure public API docs as summary + bullets, drop development-history narration and internal references, and correct comments that no longer matched the code. Comment/docstring/markdown changes only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Break long single-paragraph sections into short leads and bullets, cut design justification and history narration, and correct statements that did not match the code. Headings, tables, code examples, option names, status codes and security guidance are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jeffdupont
reviewed
Oct 2, 2026
jeffdupont
left a comment
jeffdupont
left a comment
Contributor
There was a problem hiding this comment.
Review focused on GA 1.0 readiness: what would be hard or breaking to change once 1.0 freezes the API.
The implementation is careful, and the safe-fs, verification and secret-handling layers held up well. Four GA blockers are inline. Three of them come from ai-sdks-monorepo/TESTING.md and exist identically in launchdarkly/js-ai-sdk#71, so I think they want a spec PR first and then matching changes in both SDKs:
- Live changes after a
noneintent are dropped. The spec is silent here, and the reference server SDK applies them. - The skill-key grammar is stricter than the API's, so a mismatch fails the whole config.
- The transport gives up permanently after 10 consecutive failures (spec §3.25).
- Revocation doesn't reach disk for explicit request lists (spec §3.22).
I also have a list of should-fix items (a corrupt SSE event still commits the transfer, connect vs read timeout, poll ETag adopted without a commit, the watcher reconciling against the global store rather than the one it listens to, and a few more). Happy to share those too.
andrewklatzke
left a comment
andrewklatzke
left a comment
Contributor
There was a problem hiding this comment.
Approving - reviewed individual PRs that led into this one and Christie ran local integration tests.
Worth addressing the 1.0 GA feedback pre-merge imo
… failures indefinitely - A `none` intent now leaves the FDv2 reader expecting changes, as the base SDK's `ChangeSetBuilder.expect_changes()` does. Previously a put-object or delete-object following `none` on the same stream was dropped while the following payload-transferred still advanced the basis, so a skill revoked after a routine reconnect kept being served. - Remove `max_consecutive_failures`. Recoverable failures are retried on the capped backoff for as long as the store runs; only a fatal status stops delivery. Clamp the backoff exponent so a long outage cannot overflow it. - README: scope the "revoked SKILL.md leaves disk" claim to "*", and say what an explicit skill list does instead. Correct the _resolve_requests docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
knfreemLD
approved these changes
Oct 2, 2026
knfreemLD
left a comment
knfreemLD
left a comment
There was a problem hiding this comment.
dropping approval based on Andrew's approval comment. Also reviewed all the child PRs that went into this & this change in totality
Review follow-ups on the unbounded-retry change: - Validate initial_backoff and max_backoff: positive, finite, and initial <= max. Without a failure bound they are the only limit on the retry loop, and zero reconnected ~775k times a second. - Reset the backoff delay only after a stream stays open 60s, as the base SDKs do, or after a completed poll. connection_failures still resets on a commit or a none intent. A server that answers and drops is now backed off instead of reconnected about once a second. - A response over MAX_RESPONSE_BYTES is fatal, like 422, instead of being re-downloaded on every backoff step forever. - A goodbye after a completed exchange is not counted or reported as a failure, matching the JS SDK; the reader logs goodbyes at debug. - Tests matching the JS suite: wait_for_skills runs to its timeout during an outage, and the two 400-repair cases. - Docs: scope the skills_watch docstring and agents.md §6 to "*", and fix a stale "budget" comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves conflicts with the comment/docs trim on the base (744199f, c547be2): - skills_fdv2.py: keep this branch's 304 and etag-adoption logic and comments. The base's `not foreign` guard on `basis` is subsumed by the `applied` early return, so it is dropped. - agents.md: keep this branch's "a 304 is not one" section alongside the base's trimmed first-payload-intent section. - wait_for_skills docstring: the base's trim said a 304 releases it; under this branch only a commit does, so the wording is corrected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves the shutdown() docstring conflict with the base's comment trim (744199f): keep the base's reworded skill-store sentence and this branch's paragraph on releasing the process-global tracer provider. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
) ## Summary Fixes a path in the FDv2 delivery transport that could report a store holding **nothing** as initialized, which authorizes `write_skills("*")` to prune every managed `SKILL.md` on disk. Raised by Bugbot on #87 (comment [r4146430929](#87 (comment))); verified reachable, and traced to two claims that were wider than what happened. A poll adopted the response `ETag` after any body that did not raise, and a 304 published a first payload unconditionally. Between them, a body this SDK could not apply — a future `intentCode`, or a `payload-transferred` with no recognized intent — lent its etag to the next request, and the 304 that answered it reported the empty committed set as current: initialized, healthy, and with nothing in a 304 to notice it on. Three claims narrowed: - `payload-transferred` reports a commit only when it applied a pending set. A `none` intent builds none, nor does an unrecognized `intentCode`, and a foreign payload's contents are declined — those transfers now report nothing, where they reported a commit. They no longer adopt the selector of a payload that was never applied either, which on its own left a store resuming from content it does not hold while every diagnostic read healthy. - A poll adopts the response `ETag` only from a body that **completed an exchange**: a committed payload, or a `none` intent, which is the server saying the content held is what the etag describes. Gating on the commit alone would have broken the steady state — the `none` body's etag is how a poll goes from a tiny 200 every interval to a bodiless 304. - A 304 no longer publishes a first payload. It confirms the payload the store holds; the exchange it stands in for, the `none` intent, does not publish one either. Not a new rule: TESTING.md §3.25 already defines `is_initialized()` as true "once a payload has committed", and §3.21/§3.22 turn prune suppression on it. The spec gains the bullets that were missing for the poll path in launchdarkly/ai-sdks-monorepo#34, and js-ai-sdk carries the identical fix in launchdarkly/js-ai-sdk#104. `test_a_304_before_any_payload_still_releases_wait_for_skills` justified the old behavior as "a reconnect with a cached basis", which this transport has no mechanism for — `_basis` and `_etag` both start as `None` with no injection point — so a 304 reaching a store that holds nothing takes a server answering a request that carried no etag at all. It now fails closed, and the test asserts that. ## Test plan - [x] `pytest` — 2164 passed, 11 skipped - [x] `ruff check` / `ruff format --check` / `mypy packages/*/src` clean - [x] New: a body under an unrecognized intent leaves the next request carrying no `If-None-Match`, the store uninitialized, nothing held, and no selector adopted - [x] New: all four shapes that reach `payload-transferred` with no pending set report neither a commit nor an up-to-date answer, and still count the transfer - [x] Rewritten: a 304 answering a request that carried no etag leaves `wait_for_skills` false and `is_initialized()` false without recording a failure - [x] The over-cap poll test was leaning on the same path for its "delivery carries on" assertion; its retry now gets a real payload 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Tightens FDv2 poll/stream semantics so an empty or unapplied store cannot be treated as initialized and authorize `write_skills("*")` to prune every managed skill on disk. > > **`payload-transferred`** now reports a commit (and adopts a resume `basis`) only when a pending set was actually applied. Transfers with `none`, unknown intent codes, foreign payloads, or no pending work return an empty outcome—still counted in diagnostics but not a commit or up-to-date signal. > > **Poll etag handling** adopts the response `ETag` only after a body **completed an exchange** (committed payload or `none` on `server-intent`). Unapplied bodies no longer lend their etag to a follow-up **304**. > > **HTTP 304** no longer calls `_publish_first_payload` or satisfies `wait_for_skills`; it only confirms content already held after a real commit. > > Agent docs in `agents.md` spell out the three linked invariants. Tests were added/rewritten for 304-before-payload, unrecognized-intent etag chains, and non-committing transfers. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit daaf208. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…arning get_skills passed the full request count to log_withholding_summary, so an absent key, a pin miss, a wrong-version answer, or a store that raised was reported as withheld. When nothing resolved, the warning blamed contentHash: a get_skills call against an empty store at boot raised a false integrity alarm. Only resolutions where the store served an object (ok or integrity_failure) now count toward the summary. all_skills already counted only what the store held. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
XieX
added a commit
to launchdarkly/js-ai-sdk
that referenced
this pull request
Oct 5, 2026
… failures indefinitely (#108) 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](launchdarkly/python-ai-sdk#87 (comment))). `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](launchdarkly/python-ai-sdk#87 (comment))). 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](launchdarkly/python-ai-sdk#87 (comment)), 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](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!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. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1c8b75c. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Resolves conflicts with #129 (a 304 confirms a payload rather than establishing one): - FDv2SkillStore._apply keeps this branch's recycled= flag on the recoverable error and takes the base's new boolean return, which _poll_once uses to decide when to adopt an etag. - The over-cap poll test keeps this branch's version: an oversized response is now fatal, so the base's retry-then-recover variant no longer applies. The recovery path still queues a full payload before start(), so it does not rely on a 304 to release the waiter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… 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) <!-- CURSOR_SUMMARY --> --- > [!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> <!-- /CURSOR_SUMMARY -->
…arning (#137) `get_skills` passed the full request count to `log_withholding_summary`, so a skill that was never withheld was still counted as withheld. That covered: - an absent key - a pin miss - a wrong-version answer - a store that raised When nothing resolved, the warning said every object failed verification and pointed at `contentHash`. So a `get_skills` call against an empty store at boot raised a false integrity alarm. Cursor Bugbot found this on the TypeScript port, in launchdarkly/js-ai-sdk#107 ([comment](launchdarkly/js-ai-sdk#107 (comment))). The port copied this function from here, so the same fix is pushed to that PR to keep the two SDKs matching. ## Change `get_skills` now counts only the resolutions where the store served an object (`ok` or `integrity_failure`) and passes that count to the summary as the requested total. The subject is now "requested skills the store served", so the counts in the message match what they describe. `all_skills` and the `"*"` path in `write_skills` already counted only what the store held, so they are unchanged. ## Tests New tests in `TestWithholdingSummary`: - absent keys don't warn - a pin miss doesn't warn - a wrong-version answer doesn't warn - a raising store doesn't trigger the summary warning - a batch mixing a good skill, a tampered skill and a miss reports `1 of 2` All five fail when the fix is reverted. `make lint`, `make format-check`, `make typecheck` and `make test` pass (2169 passed, 11 skipped). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **`get_skills`** no longer treats every requested reference as part of the withholding summary. It increments a **served** count only when `resolve_from_store` reports **`ok`** or **`integrity_failure`** (the store actually returned an object), then calls `log_withholding_summary` with subject **"requested skills the store served"** instead of the full batch size. > > That stops false **contentHash** / verification warnings for absent keys, version pin misses, wrong-version answers, and store outages—cases that were never withheld skills. The docstring now states that verification warnings exclude plain misses. > > **Tests** add five `TestWithholdingSummary` cases: silent behavior for missing keys, pin misses, wrong-version stores, and raising stores; and **`1 of 2`** when one served skill verifies and one tampered skill does not, with a missing key ignored. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 770a004. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Companion to launchdarkly/js-ai-sdk#103, which fixes the same lifecycle bugs in JS. Two of them exist here, and one has a Python-specific counterpart. ## 1. An init/shutdown/init cycle exported nothing `shutdown()` dropped its provider handle but left OpenTelemetry's global tracer provider registered. That global is once-guarded — a second `set_tracer_provider` logs `Overriding of current TracerProvider is not allowed` and keeps the provider already in place — so after a re-init, every span routed to the provider that had just been shut down. Observed across two cycles: the global `service.name` stayed `cycle1` and now correctly becomes `cycle2`. Releasing it means resetting **both** the global slot and the `Once` that guards it; clearing the slot alone leaves the guard tripped, so the next set is a silent no-op. Both are private, since `opentelemetry-python` has no public way to unset them, so `_release_otel_globals`'s docstring explains the reach. The propagator needs no reset — `set_global_textmap` is a plain assignment. The release only happens when **our** `set_tracer_provider` actually took effect (`trace.get_tracer_provider() is provider`). Bugbot [caught](#126 (comment)) that the first version gated it on having built a provider, which would have wiped a host app's already-registered provider; the JS PR had the same flaw. A refused set now logs a warning. ## 2. A failed telemetry setup left a half-initialized client behind `_client is not None` is the idempotency guard, but `_client` was assigned *before* `_setup_telemetry` ran. A malformed `OTEL_EXPORTER_OTLP_TIMEOUT` makes setup raise, and `_client` stayed set: the next `init_client()` returned the client as a silent success with no telemetry, and on the SDK-key path its connection was never closed. `_client` is now assigned only after setup succeeds. On the SDK-key path the client we built is closed on failure; a BYOC client is left open, since the caller owns it. JS's counterpart was a cached failed-init promise. ## Not a bug here: BYOC idempotency JS checked the singleton *below* the pre-initialized-client branch and re-registered OTel on every `initClient(client)`. `_resolve_client` checks it first — 1 `_setup_telemetry` call across three inits — and two tests pin that ordering. ## Verification - 8 new tests; each one covering a bug here was confirmed to **fail** against the code it fixes. The end-to-end cycle and env-var tests drive the real `_setup_telemetry`, with the OTLP exporter stubbed or failing before any network use. - Full suite **1356 pass**. ruff check, ruff format, and mypy are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Fixes Python client lifecycle bugs aligned with the JS SDK: **init/shutdown/init** no longer leaves spans routed to a shut-down tracer provider, and **failed telemetry setup** no longer leaves a half-initialized singleton client. > > **`shutdown()`** now clears OpenTelemetry’s once-guarded global tracer registration (via `_release_otel_globals`) only when this SDK’s `set_tracer_provider` actually won (`_owns_otel_globals`). If another library registered first, setup logs a warning and shutdown does not tear down the host app’s provider. Test reset mirrors the same release so multi-init suites do not leak globals. > > **`init_client`** assigns `_client` only after `_setup_telemetry` succeeds. On the SDK-key path, a telemetry failure closes the newly created LD client; BYOC clients are left open for the caller. Repeat inits still run telemetry setup at most once (idempotency check first). > > Docs in `agents.md` describe the shutdown/release behavior; **`test_lifecycle.py`** adds coverage for cycles, foreign providers, failed setup, and idempotency. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ebb6c1e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
XieX
added a commit
to launchdarkly/js-ai-sdk
that referenced
this pull request
Oct 5, 2026
) ## Summary Fixes a path in the FDv2 delivery transport that could report a store holding **nothing** as initialized, which authorizes `writeSkills('*')` to prune every managed `SKILL.md` on disk. Found by reviewing a Bugbot comment on the Python side and checking whether this SDK shared the gap — it did, line for line, including the tests. A poll adopted the response `ETag` after any body that did not throw, and a 304 published a first payload unconditionally. Between them, a body this SDK could not apply — a future `intentCode`, or a `payload-transferred` with no recognized intent — lent its etag to the next request, and the 304 that answered it reported the empty committed set as current: initialized, healthy, and with nothing in a 304 to notice it on. Three claims narrowed: - `payloadTransferred` reports a commit only when it applied a pending set. A `none` intent builds none, nor does an unrecognized `intentCode`, and a foreign payload's contents are declined — those transfers now report nothing, where they reported a commit. They no longer adopt the selector of a payload that was never applied either, which on its own left a store resuming from content it does not hold while every diagnostic read healthy. - A poll adopts the response `ETag` only from a body that **completed an exchange**: a committed payload, or a `none` intent, which is the server saying the content held is what the etag describes. An unrecognized intent says the opposite — the body carried objects `ignoreUnderUnknownIntent` dropped — so leaving that poll unconditional is also what keeps the body arriving and its warning repeating, rather than silenced behind a 304. Gating on the commit alone would have broken the steady state, since the `none` body's etag is how a poll goes from a tiny 200 every interval to a bodiless 304. - A 304 no longer publishes a first payload. It confirms the payload the store holds; the exchange it stands in for, the `none` intent, does not publish one either. Not a new rule: TESTING.md §3.25 already defines `isInitialized()` as true "once a payload has committed", and §3.21/§3.22 turn prune suppression on it. The spec gains the bullets that were missing for the poll path in launchdarkly/ai-sdks-monorepo#34, and python-ai-sdk carries the identical fix in launchdarkly/python-ai-sdk#129 (where Bugbot raised it, on launchdarkly/python-ai-sdk#87). Five tests had pinned the old behavior here, against Python's one. Three were only this side's optional `changes` / `basis` reading `undefined` where Python's dataclass defaults give `[]` / `null`; the new outcome states both, so the two languages return the same object. The other two were real, and the second is the clearest evidence this drifted rather than being designed: - `releases waitForSkills on a 304 before any payload` justified it as "a reconnect with a cached basis", which this transport has no mechanism for — `basis` and `etag` both start `null` with no injection point. - A JS-only lifecycle test asserted `a 304 counts as initialized — the payload held is confirmed current`, over a store holding no payload. It now exercises the 304 this store can actually reach, offering an etag it earned. ## Test plan - [x] `yarn build && yarn test` — full workspace green; client 1040 passed, 10 skipped - [x] `tsc --noEmit` and `biome check` clean - [x] New: a body under an unrecognized intent leaves the next request carrying no `If-None-Match`, the store uninitialized, nothing held, and no selector adopted - [x] New: all four shapes that reach `payloadTransferred` with no pending set report neither a commit nor an up-to-date answer, and still count the transfer - [x] Rewritten: a 304 answering a request that carried no etag leaves `waitForSkills` false and `isInitialized()` false without recording a failure 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Fixes FDv2 poll delivery so an empty store cannot become initialized and authorize `writeSkills('*')` to prune every managed skill on disk.** > > `payload-transferred` now reports a **commit** only when a pending set was actually applied. Transfers with no pending work (`none`, unknown intent, lone transfer) return no commit, no up-to-date signal, and **no resume `basis`**—they still increment `payloadsTransferred`. > > Polling no longer calls **`markFirstPayload()` on HTTP 304**; a 304 only confirms content already held. Response **ETags are adopted only after a completed exchange** (committed payload or `none` intent on its own event), so bodies under unrecognized intents cannot lend an etag that makes the next 304 look current over an empty store. > > `agents.md` documents the three linked rules (commit vs 304 vs etag adoption). Tests were updated and added to match—e.g. 304 before any payload leaves `waitForSkills` false and `isInitialized()` false, and unrecognized-intent polls do not send `If-None-Match`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit fdb2b54. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
TESTING.md §3.25 asks for all four shapes that reach payload-transferred with nothing to apply. The test covered three; the declined foreign payload was missing, so dropping `not foreign` from `applied` still passed the suite, because the foreign tests assert only the basis. Add it, priming the reader with a skill commit so the payload is recognisably foreign, and assert the committed set is unchanged rather than empty. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up to #129, and the Python half of the review comment on launchdarkly/js-ai-sdk#104. TESTING.md §3.25 says to assert all four shapes that reach `payload-transferred` with nothing to apply: a `none` intent, an unrecognised `intentCode`, a lone transfer with no intent, and a declined foreign payload. `test_a_transfer_that_applied_nothing_is_not_a_commit` covered three. The foreign case was missing. **Why it matters:** if `not foreign` is dropped from `applied` and the old `basis` guard is put back, the full suite still passes, because the foreign-payload tests check only the basis, `payloads_ignored` and the held set. The outcome would report `committed=True`, which resets `connection_failures` and lets a poll keep the etag for content it never applied. **Change:** - The foreign shape joins the loop. A payload only counts as foreign once a skill payload has committed, so each shape can now run an earlier payload first. - The test now checks that the committed set is unchanged and that `payloads_transferred` went up by one, instead of checking for an empty store and a count of 1. **Verified:** - The test passes on the current code. - With the mutation above it fails. - `make lint`, `format-check`, `typecheck` and `test` all pass (2200 passed). The matching JS change is pushed to launchdarkly/js-ai-sdk#104. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Extends **`test_a_transfer_that_applied_nothing_is_not_a_commit`** so it matches TESTING.md §3.25: the loop now covers all four ways a transfer can finish with nothing applied, including a **declined foreign payload** (`env-flags` after a real skill commit). > > Each scenario can run an optional **`prior`** event sequence (needed so “foreign” only applies once the skill payload is known). Assertions were generalized to **`committed` / `up_to_date` / `basis` unchanged**, **held objects unchanged** vs a snapshot, and **`payloads_transferred` increments by one**—so a regression that wrongly sets `committed=True` on ignored transfers would fail even when basis-only tests still pass. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 4f672f6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
_resolve_client assigns _client only on success, so a call made before LD_SDK_KEY is available already leaves nothing behind and the next call retries. Nothing asserted it. These tests match the ones added to TypeScript in launchdarkly/js-ai-sdk#105, where the first rejection was cached for the life of the process: - fail without a key, then init_client({"sdkKey": ...}) succeeds - fail, shutdown(), then init succeeds - inspect_config, which swallows the init error, reports disabled while the key is missing and enabled once it is set Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jeffdupont
approved these changes
Oct 5, 2026
jeffdupont
left a comment
jeffdupont
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed at e43d9f7. All four of my threads are resolved, and #129, #131, #137 and #138 are in. uv run pytest: 2200 passed, 11 skipped, exit 0. #138 covers the fourth no-commit shape I asked about on js #104, the declined foreign payload (test_skills_fdv2.py:957), so both SDKs now test all four.
Approving, on two conditions before this merges:
- CI on this head didn't finish. Tests was cancelled after 15 minutes (run 37363000203). The run before it took under a minute, and every run in this repo and js-ai-sdk since about 19:29 UTC was cancelled or is still queued. So it looks like the runners, not this branch, but it needs a green rerun.
- #139 should land first. It's the one-line
connection_failuresreset on restart that monorepo #40 now specifies. Without it, a store that gave up and was started again reports the old run's failures. #140 (failedinit_clientcan be retried) is a test only. The library already behaves that way, and I probed it one43d9f7. It's worth having before GA because JS had exactly this bug.
Two things still need a decision before 1.0 ships, but they don't need to hold this merge:
- Key grammar. gonfalon #73433 fixes new keys, but by its own description it checks only at create time, leaves existing skills alone, and leaves the OpenAPI spec unchanged. Still unanswered: do any existing skills have keys outside
^[a-z0-9][a-z0-9-]*$? They would still fail their whole AI Config in both SDKs. And do copy, import or Terraform go throughCreateAgentSkill? I haven't checked either. - Pre-init reads. With a store that isn't initialized yet, the accessors still return
absentand log nothing (raised on #137).absentvsstore_unavailablefreezes at 1.0, so it needs a spec decision for both SDKs.
## Summary Adds tests asserting that a failed `init_client` is not cached, so a later call can retry. No library change: `_resolve_client` already sets `_client` only on success. Nothing asserted it, though, and the TypeScript side had exactly this bug. There, `singleton.initPromise` cached the first rejection for the life of the process (fixed in launchdarkly/js-ai-sdk#105). The new tests in `tests/test_lifecycle.py`: - `TestInitClientSDKKeyPath::test_retries_after_a_failed_init_instead_of_replaying_it`: with no key, `init_client()` raises. Then `init_client({"sdkKey": "late-key"})` returns the client, and `Config` is built with `late-key`. - `TestShutdown::test_allows_initialization_after_a_failed_init`: a failed init, then `shutdown()`, then an init that succeeds. - `TestInspectConfig::test_recovers_once_sdk_key_is_available_after_a_failed_lazy_init`: `inspect_config` swallows init errors, so a cached failure would serve disabled configs forever. Here it reports disabled while the key is missing and enabled once it is set. The TypeScript PR adds four more tests that have no Python counterpart: - **Concurrent callers sharing one rejected init promise, and `shutdown()` dropping an in-flight init.** Python has no shared in-flight promise. - **Closing the built client after a `waitForInitialization` timeout, including when `close()` also throws.** `start_wait` doesn't raise. The analogous close-on-failure case is already covered by `test_a_failed_telemetry_setup_leaves_no_client_behind`. ## Tests - Each new test fails against a mutation that caches the missing-key failure. - `uv run pytest packages/client`: 1389 passed. `ruff check` and `ruff format --check` are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
5 participants
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.
Agent Skills
Skills are versioned
SKILL.mddocuments managed in LaunchDarkly and attached to AI Config variations by reference. This adds the full server-side surface for them: the SDK reports which skills a resolved config references, retrieves their content over LaunchDarkly's FDv2 delivery channel, verifies it, and materializes it onto disk as<root>/<key>/SKILL.md— where the Claude Agent SDK and anything else following that convention discovers it.Layers
types.py/types_validation.pySkill,SkillReference,SkillOutcome,ReconcileReport; the canonical key grammar (^[a-z0-9][a-z0-9-]*$) and version predicateskills.pyInMemorySkillStoreskills_core.pySkillStoreinterface, verification, and the structured integrity log recordskills_fdv2.pyFDv2SkillStore— delivery over the SDK-facingGET /sdk/pollandGET /sdk/streamendpointsskills_fs.py/safe_fs.pyskills_watch.pywatch_skills— re-reconcile on every delivery changeDependencies flow downward only; nothing above the store can tell which store produced an object.
API
skill_refs(config)skillsarray intolist[SkillReference]. Pure — no client, store, or networkget_skill(key, *, version=None)None.version=Nonemeans newest availableget_skill_result(key, *, version=None).skill,.reason(ok/absent/integrity_failure/store_unavailable/wrong_version),.detailget_skills(refs)/all_skills()write_skills(skills, root, *, prune=True, timeout=10.0, on_unavailable="keep")root, returning aReconcileReport. Pass"*"for the whole librarywatch_skills(skills, root, …, debounce=…)write_skillsplus re-reconcile on delivery change. Returns(initial report, SkillWatcher)SkillStoreget_object,all_objects, optional listenersInMemorySkillStore(objects=None)put(raw), for tests and bring-your-own-contentFDv2SkillStore(sdk_key, *, base_uri=…, mode="stream", poll_interval=30.0, read_timeout=None, …)start(),wait_for_skills(),close(),diagnostics,failed; also a context managerStoreDiagnosticsPlus the closed-set types (
ReconcileActionKind,OnUnavailable,SkillOutcomeReason) and the fixed on-disk constants (SKILL_FILENAME,MANIFEST_FILENAME,MANIFEST_VERSION).init_client(options={"skillStore": store})configures the store;shutdown()clears it.The primary use case
Notes for reviewers
parse_ai_confignow fails closed on askillsvalue that is not a list of{key, version}objects, where before any value parsed and was ignored. A variation carrying its own differently-shapedskillsfield must rename it before upgrading.contentHash, its key and version revalidate, and its size is within 10 MiB. There is no fallback that skips it. Every withheld skill emits one structured ERROR record —ld.skills.integrity_failure, a stability commitment, byte-identical across LaunchDarkly's AI SDKs — on the SDK's own logger, independent of telemetry configuration.write_skillstouches only what it owns. It writes<root>/<key>/SKILL.md, records what it owns in<root>/.launchdarkly-skills.json, and will overwrite or delete only manifest-recorded paths. It treats that manifest as untrusted input and re-validates every entry. The one adoption exception — a byte-identical file at a managed path — is what makes a crashed reconcile recoverable.lstat, which is a check-then-use race rather than a closed window. Write permission on the managed root or any ancestor is the security boundary on every platform, and on Windows the only one. The README's privilege-separation section is the deployment contract — the recommended shape runs the reconcile as a different identity than the agent."*".watch_skills("*", root)removes a revoked skill'sSKILL.mdwithout a restart. With an explicit list such asskill_refs(config), the list is fixed: a skill the store answersabsentfor is reported as anerrorand its files are kept, and the watcher listens only to the skill store, so unpinning a skill or moving it to a new version is not seen until the refs are read again (at the next boot, typically). Making the watcher follow config changes is a planned follow-up and is additive.write_skillsblocks. It isasyncfor parity with the other accessors and with the TypeScript SDK, but awaits nothing. Wrap it inasyncio.to_threadif a large reconcile holding the event loop matters.ld-relaydoes not speak the FDv2 endpoints.Testing
403 skills tests across five files —
test_skills(125),test_skills_fdv2(139),test_skills_fs(107),test_safe_fs(22),test_skills_watch(10) — including a filesystem abuse matrix and root-swap race tests that fail outright rather than skip.🤖 Generated with Claude Code, reviewed by @XieX.
Note
Overview
Adds Agent Skills to the Python server SDK: versioned
SKILL.mdcontent referenced from AI Config variations, fetched through a pluggableSkillStore, verified (sha256contentHash, key/version rules, size cap), and optionally written to<root>/<key>/SKILL.mdwith a manifest-backed reconcile.New public surface includes
skill_refs,get_skill/get_skill_result,get_skills/all_skills,write_skills,watch_skills,InMemorySkillStore,FDv2SkillStore(poll/stream delivery), and related types/constants.init_client(options={"skillStore": …})wires the store (re-applied on later inits);shutdown()clears it.Breaking parse behavior:
parse_ai_confignow rejects invalidskillsarrays (includingnull), where bad values were previously ignored.Safety-focused materialization uses descriptor-pinned I/O in
safe_fs/skills_fs(manifest-only deletes, symlink refusal, corrupt-manifest fail-closed, pruning suppressed when delivery is uninitialized or retrieval incomplete).lifecyclealso fixes init ordering on telemetry failure and releases the global OTel tracer provider on shutdown when this SDK owns it.Docs in
README.mdandagents.mddocument usage, integrity logging (ld.skills.integrity_failure), FDv2 caveats, and privilege separation for deployments.Reviewed by Cursor Bugbot for commit 542233b. Bugbot is set up for automated code reviews on this repo. Configure here.