feat(client): Agent Skills — retrieval through an injectable store (2/5) by XieX · Pull Request #51 · launchdarkly/python-ai-sdk

XieX · 2026-08-25T20:16:23Z

PR 2 of 5 splitting draft #45 for review. Stacked on #50 — review that first; this diff is against it.

main ← split/skills-references (#50) ← split/skills-retrieval ← split/skills-safe-fs ← split/skills-materialization ← split/skills-fs-hardening

Important

Aug 28 repivot: skills are now opaque byte buffers by construction. Skill.content is bytes (the verified verbatim bytes, exactly what was hashed), the frontmatter() convenience accessor and frontmatter.py are deleted, and the SDK no longer parses or interprets skill content anywhere. Consumers who want frontmatter parse it themselves. The stack was rebased in place to make each change in the slice that introduced the code; the TypeScript SDK is getting the mirror change (content: Uint8Array) separately.

What's here

The layer that turns a reference into content: an injectable store interface, integrity verification of everything it serves, and a body-free telemetry interface for the failures.

Export	Description
`get_skill(key, *, version=None)`	One verified skill, or `None`.
`get_skills(refs)`	Batch form; accepts `SkillReference` values and bare key strings.
`all_skills()`	Every verified skill the store holds, one per key at its newest version.
`SkillStore`	The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `add_listener(kind, fn)`.
`InMemorySkillStore(objects=None)`	Dict-backed store with `put(raw)`. Holds several versions of a key.

Configure with init_client(options={"skillStore": store}). The LaunchDarkly delivery transport drops in behind the same seam with no public API change.

Verification

Store data is untrusted; the transport is not part of the trust boundary. Key and version are revalidated, size is bounded, and the sha256 of the verbatim bytes must match the delivered contentHash. 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. Anything that does not verify is withheld and treated as missing, so no unverified content is ever returned. verified_bytes also accepts already-bytes content and hashes it directly, for the pre-write re-verification pass PR 4 adds — the "not encodable as UTF-8" branch applies only to wire-shaped str input. 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.

Integrity failures go through a private telemetry seam carrying hashes and byte counts only. 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 to publish the body through a signal that is otherwise body-free. The default emitter is a no-op; the three signal names are an allowlist maintained in one section of one module.

Review findings addressed

Finding 1 (blocking) — version pinning could not be expressed by the store seam. Version is now 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 answered a pinned reference with the newest object and then rejected it, turning the primary use case into a missing skill.

SkillStore.get_object(kind, key, version=None); version=None means "the newest you hold".
The post-fetch equality check stays, now as a defense rather than the selection mechanism: the store is untrusted, so an answer that is not the version asked for is withheld. TestVersionPinning::test_a_store_answering_with_the_wrong_version_is_withheld covers it.
InMemorySkillStore keys by (key, version) and holds both. An object whose version is unusable is still served, under its key alone — withholding it is verification's job, so a malformed object stays distinguishable from an absent one and still records a signal.
all_objects returns one entry per (key, version) under keys documented opaque; identity is read off each object's own fields. newest_by_key is the single place that collapses a whole-store read to one object per key, because a list holding two versions of one key is not a set of skills.
TestVersionPinning holds two versions of one key and asserts a pinned-old lookup, a latest lookup, both against one store, a mixed batch, and all_skills() returning one entry per key. Reverting just the seam change fails three of them, so they are not vacuous.

Finding 2 — contentHash mandatory here, optional on the wire. It stays mandatory; that default is right. What changes is the signalling: a run that withheld anything logs one WARN naming the counts, and a run where nothing verified says so explicitly and names contentHash — that case previously returned an empty result indistinguishable from "this project has no skills". TestWithholdingSummary covers total, partial, batch-scoped, and silent-on-success. The other half of finding 2 — whether the wire object needs a hashing-algorithm discriminator rather than a bare hex string — is a delivery-contract decision, not an SDK one; it is tracked on #45 rather than resolved here.

Finding 3 — the object kind may not match the real delivery kind. SKILL_OBJECT_KIND keeps its value but is no longer 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 — a claim this side cannot make, and hard to walk back once a caller depends on it. An adapter that needs to agree with it reaches it through skills_core. test_object_kind_is_not_public_api asserts it is absent from both __all__ and the package namespace. This needs the same change in the TypeScript SDK, as does finding 1's seam shape.

MAX_SKILL_CONTENT_BYTES stays internal for the adjacent reason: it is a local enforcement bound on content the platform produces, so exporting it would semver-lock a number this side does not own.

Notes for reviewers

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.
record_materialized and record_revoked land here with no caller — the materialization layer is PR 4. They live beside record_integrity_failure so the three-signal allowlist is one section of one file rather than three sites to audit.

Testing

uv run pytest → 1145 passed. ruff check, ruff format --check, and mypy packages/*/src all clean.

🤖 Generated with Claude Code

Note

Overview
Adds Agent Skills retrieval behind an injectable SkillStore, wired through init_client(options={"skillStore": …}) and cleared on shutdown().

New skills_core module centralizes store state, mandatory integrity verification (key/version, contentHash sha256, size cap), shared resolve_from_store, and a telemetry seam plus stable ld.skills.integrity_failure ERROR logs. skills.py grows InMemorySkillStore (multi-version get_object), get_skill / get_skills / all_skills, and get_skill_result returning SkillOutcome so apps can fail closed on tampering while get_skill stays None-only. Lookups treat version as part of store identity (pinned refs vs newest). safe_fs.py introduces descriptor-pinned atomic write primitives for later materialization. README and agents.md document the full skills architecture, integrity codes, and operational guidance.

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

donei003 · 2026-08-27T16:32:18Z

+    return ld_client
+
+
+async def _resolve_client(opts: InitClientOptions, client: Any) -> Any:


Do we need this new _resolve_client method? Or can we continue to use the init_client method as it was before and set the store at the appropriate point in the initialization?

Skill store is different than the other things we set in that,

We want to be able to add a store even if the SDK was initialized with no store or a different store, to support lazy initialization and then setting the store later when it's ready

We only want to set the store if the SDK init has been successful, so that we don't try to use it when we can't

That's why _resolve_client was introduced, just to wrap all of the return paths and set the skill store once, instead of 3 or 4 times throughout.

knfreemLD · 2026-08-28T14:09:34Z

+``launchdarkly_ai_server.skills_core``.
+"""
+
+MAX_SKILL_CONTENT_BYTES = 64 * 1024


Technically our cap is 50kb

Do we need to set this here? I think it makes more sense to enforce the cap on our server-side so we have more flexibility and control over it

This isn't reproducing the server-side cap, it's meant as a separate guard, hence why it's got some headroom over the actual limit. We're treating the store as untrusted, and in addition to bouncing anything whose hash doesn't match, we're also bouncing content that's way bigger than what we'd expect (it would be possible, for example, to switch in a several GB content payload with a matching hash).

Having said that, I'm wondering if this is enough headroom. Is the 50KB limit expected to grow, and if so by how much? We wouldn't want to increase the server-side limit and then have customer's blocked until they update their SDKs. Should we crank this up to 1MB or even 10MB?

What's the benefit to having the cap here as well as server side? It means two places (N places actually, as we expand this to other SDKs) to update if we ever do loosen this, and if down the line we ever want to offer larger skill support for bespoke customers it means awkward gating between flag releases & SDK rollouts.

Because here this would be tied to released and used SDK version too, it means we can't hotswap this cap and enforce it across our users.

I like the idea of having protections on our SDK! And I see the point about it being an untrusted store; but realistically I wonder how likely it is for this to cause problems. Can you go over what cases this limit on SDK-side protects us from so we can assess the security implications?

What's the benefit to having the cap here as well as server side?

As explained above, it's a different cap. The server side cap is "this is the largest skill we'll support". This limit is a "skills bigger than this obviously aren't legit" check.

if down the line we ever want to offer larger skill support for bespoke customers

I suggest cranking it up to 10MB, that's still orders of magnitude larger than a skill should be, and means we wouldn't have to change it (until we update the SDKs to support bundles, when we'd need to revisit it anyway).

Can you go over what cases this limit on SDK-side protects us from so we can assess the security implications?

It's not to protect us, it's to protect SDK consumers. Specifically it would be a CWE-770 vulnerability. This limit is addressing a line item from our security review.

Cool, let's do 10MB for now! If we can document it somewhere too that'd be great

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>

cursor

Stale Bugbot comment from a previous run.

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>

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)  --- > [!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. > > 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).

…network (#82) Stacked on the `watch_skills` PR (`xie/skills-watch`). Second of three PRs split out of #69; the transport that puts a connection underneath this follows. The half of the delivery transport that has no I/O: identifying a skill object on the wire, translating it into the raw object shape the `SkillStore` interface defines, holding it by `(key, objectVersion)`, and applying a payload's events as one consistent commit. Splitting it out lets the three decisions that matter most be reviewed without a socket in the way. ## Three things worth reviewing closely **1. The skill's version is in the object's `key`; `version` is the payload's.** Each version of a skill is its own object on the wire, identified as `<key>:<version>` (`pdf-extraction:3`), and that is the only place the skill's version appears. The event's `version` is the payload's, and confusing the two fails *silently*: the object verifies, the hash matches, and the caller gets content under a version number that means nothing. The wire key is split in exactly one place (`_split_wire_key`), both the put and the delete translation go through it, and `TestVersionTranslation` asserts it in both directions. 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. **2. Changes commit at `payload-transferred`, not per object.** A payload version is the unit of consistency. A half-applied full transfer would publish a state the server never described and would briefly empty the store, which, with pruning on, is the difference between a reconcile and deleting a customer's skill files. An interrupted transfer leaves last known good intact, and listeners fire once per commit. **3. A hashless object is held, not dropped.** Dropping it at the transport would report `absent`, indistinguishable from "no such skill", and would let a prune delete the last known-good copy on disk. Holding it means verification withholds it with `missing_content_hash`, which is diagnosable: an ERROR per `(key, version)`, deduped per reader rather than per process so two stores never quieten each other, a summary per wholly-hashless payload, and a `StoreDiagnostics.hashless_objects` counter. There is deliberately no fallback that synthesises a hash from the delivered content. ## Also here - **Skills are `kind == "skill"`; everything else is ignored, not rejected.** Object kinds on the SDK-facing channel are open strings and the agent-skill payload is classified `generic`, so a skill arrives under the kind its producer registered — the bare category name — with no `category` or `objectVersion` field ([streamer #4681](launchdarkly/streamer#4681), [gonfalon #70638](launchdarkly/gonfalon#70638)). An environment's assignment carries its flag payload alongside its agent-skill payload, so flag and segment objects arrive as a matter of course. Erroring on them would turn a normal payload into a permanent reconnect loop. - **`_SkillObjectSet`** holds several versions of one key, with lookup semantics identical to `InMemorySkillStore` down to the fall-through to a version-less entry. Its opaque snapshot keys are spelt `<key>:<version>`, the same as the wire, and a test pins that round trip. `TestInterfaceParity` asserts the two resolve identically. - **`_require_server_side_credential`** refuses a mobile key or client-side environment ID. Its tests arrive with the store constructor that calls it. Nothing here is exported yet; the store exports it. The module imports nothing from the feature but the version validator. ## Tests `test_skills_fdv2.py` drives `_ProtocolReader` directly: identification, version translation, full and change transfers, interruption, revocation, tombstones, mixed payloads, unknown kinds and events, error and goodbye, and the hashless dedupe across readers. The wire builders it introduces are shared with the transport PR's fake endpoint. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Adds **`skills_fdv2.py`**, a stdlib-only layer below `SkillStore` that parses LaunchDarkly FDv2 events without sockets. It maps wire `put-object`/`delete-object` into the raw store shape, keeps skills in **`_SkillObjectSet`**, and applies updates atomically in **`_ProtocolReader`** at **`payload-transferred`** (not per object). > > The critical wire rule is **`key` = `skillKey:objectVersion`** while the event’s **`version` is the payload revision** and is dropped—confusing them would silently serve the wrong pinned version. Non-`kind == "skill"` objects (flags, segments) are **ignored**, not errors. **Foreign payload** full transfers are declined once skills’ payload id is known, so a flag `xfer-full` cannot wipe held skills (and trigger prune). **Hashless** skills are retained for verification to withhold with `missing_content_hash`, with **`StoreDiagnostics`** and loud logging. **`_require_server_side_credential`** rejects mobile/client credentials (for the upcoming networked store). > > **`agents.md`** documents the transport contract; **`skills_core`** clarifies `SKILL_OBJECT_KIND` vs wire kind. **`test_skills_fdv2.py`** (~900 lines) exercises the reader, version translation, payload identity, and parity with `InMemorySkillStore`. Nothing is exported or wired from accessors yet—that’s the follow-on transport PR. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit efc4ca7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

…ls (#81) Stacked on #66 (`split/skills-review-closeout`). First of three PRs split out of #69; the FDv2 protocol layer and the transport follow on top of this one. `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 `SKILL.md` leaves the disk within a debounce interval of the store learning about it, rather than at the next restart. ## What's here **`skills_watch.py`** — `watch_skills` / `SkillWatcher`. Wired to the `SkillStore` interface, not to any one transport: it needs a store that implements `add_listener` and nothing more. It refuses loudly when the store does not, because a watcher that silently never fires looks exactly like one whose skills never changed. The listener itself only sets an event; the reconcile runs on a single worker thread, debounced, so a burst of changes coalesces into one pass and a slow disk never stalls the store's delivery thread. A reconcile that raises is logged and the watcher continues. `on_unavailable="keep"` stays the default: an outage must not read as "everything was revoked". **`remove_listener(kind, fn)`** 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 and skips detaching when a store does not implement it, so a customer's own store keeps working. ## Tests `test_skills_watch.py` drives the watcher through `InMemorySkillStore`, whose `put` notifies synchronously, and through small store doubles: initial reconcile, coalescing, refusal of a store without `add_listener`, detaching on close, and a store without `remove_listener`. Three tests on `InMemorySkillStore.remove_listener` join `test_skills.py`. The end-to-end case, a `delete-object` arriving over a live connection and pruning a file, lands with the transport PR where the fake endpoint lives. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Adds **`watch_skills`** and **`SkillWatcher`** so agent skill files stay in sync with the configured skill store without waiting for a process restart. The API runs an initial `write_skills` reconcile, then listens for store delivery changes and re-reconciles on a **debounced background thread** (listener only wakes the worker; disk I/O never blocks the delivery thread). Revocations can prune `SKILL.md` within the debounce window. **`on_unavailable="keep"`** remains the default so transport outages are not treated as mass revocations. > > Extends the optional **`SkillStore`** change-notification contract with **`remove_listener(kind, fn)`**, implemented on **`InMemorySkillStore`**, so **`SkillWatcher.close()`** can detach; stores without it still work but keep listeners registered. **`watch_skills`** raises if no store is configured or the store lacks **`add_listener`**, and cleans up the listener if the initial reconcile fails. > > Public exports and README/agents docs are updated; new tests cover watcher behavior (coalescing, mid-startup revocations, close/detach) and **`remove_listener`** semantics. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 29ad3fe. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

… remainder (#66) Stacked on #58. Closes the four items left open in [our response](https://launchdarkly.atlassian.net/wiki/spaces/PD/pages/5293965360) to the [Agent Skills security design review](https://launchdarkly.atlassian.net/wiki/spaces/PD/pages/5264506969). Three documentation items and one test item. **No behavior changes** — the code halves of rows 9 and 2 were already done and are deliberately untouched; the only change to `safe_fs.py` is its module docstring. ## 1. Privilege separation (row 9 docs half + counter-proposal for row 26) The proposed mitigation for **AZ-1**, the review's most serious finding. The README now documents reconcile-identity ≠ agent-identity as the recommended deployment, which is the whole reason the `0644`/`0755` modes deny anything: the agent reads its instructions and cannot rewrite them, or the manifest. 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 operator gets an explicit check to run, because the SDK cannot run it. **`ReconcileReport` deliberately grows no writability field.** Row 26 asked for one; we declined. The SDK knows only its own identity, which trivially has write access, having just written there. Any check it could make would answer a different question than the one asked and manufacture false confidence exactly where the review wants caution. `agents.md` records that reasoning so the field is not added later by someone reading its absence as an oversight. ## 2. 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; 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 — a corrupt manifest suppresses every destructive action wholesale, whereas these manifests give the implementation everything it needs to prune, and it must refuse anyway because the recorded *path* is not one this SDK could own. - **"Deleted nothing" runs through an unlink spy**, not through 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 — permissions would be doing the work. The spy proves the removal is never *attempted*. ## 3. 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. ## 4. Windows platform bound (row 2 residual) — deferred, explicitly **Decision: defer, with the bound written down.** Reparse-point checks (`GetFileAttributesW` / `FILE_FLAG_OPEN_REPARSE_POINT`) are not implemented, and that is now recorded in `safe_fs.py`, `agents.md` and the README rather than left implied. Rationale: - Windows is not a supported or tested platform for this release, and **neither repository has a Windows CI runner** — every matrix job is `ubuntu-latest` — so the checks would ship unverified. Both codebases already cite that absence as a reason not to add `os.name`/`process.platform` branches. - **The TypeScript SDK could not match them in any case.** Node exposes no `*at()` family on *any* platform, so its racy `lstat` floor is universal rather than Windows-only. Hardening Python alone would break the cross-language parity the two SDKs are held to. 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. That code stays — it keeps a root written on Linux usable when read from Windows — but it is not evidence that Windows is hardened. ## Parity The Node half is launchdarkly/js-ai-sdk#48, case for case. No customer-visible surface moved: the `ld.skills.integrity_failure` event name, the eight `reason_code` tokens, the five `SkillOutcomeReason` tokens, and the log-record serialization are untouched. ## Verification - `uv run pytest` — 1484 passed, 11 skipped - `uv run ruff check .` / `ruff format --check .` — clean - `uv run mypy packages/*/src` (the CI gate) — no issues in 43 source files 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Closes the remaining **Agent Skills security review** items with **documentation and tests only** — no runtime behavior changes beyond an expanded `safe_fs.py` module docstring. > > The **README** and **`agents.md`** now spell out deployment and platform limits: run **`write_skills` under a different identity than the agent** (with a shell check for agent writability), **decline a `ReconcileReport` root-writability field**, warn that **`"*"`** materializes the full skill library into context, and document the **POSIX-only** descriptor-pinned guarantee versus Windows’s racy `lstat` floor (reparse-point hardening explicitly deferred). > > **`test_skills_fs.py`** adds **`TestHostileManifestPrune`**: well-formed manifests that would prune `/etc/passwd`, traversal paths, or paths under a symlink-swapped parent must error without **`os.unlink`** being attempted (unlink spy), matching existing `_prune` defenses. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6a0e6aa. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

…failure (Gap 2 / LA-2) (#58) ## Gap 2 (finding LA-2): a distinguishable outcome for integrity failure versus absence **This PR adds public API and needs review as such.** Three new exported names: `get_skill_result`, `SkillOutcome`, `SkillOutcomeReason`. ### The defect `get_skill` returns `None` for four distinct outcomes — no such skill, the store raised, the requested version is not the one held, and content that failed hash verification. A customer therefore cannot fail closed on suspected tampering while tolerating a merely-absent skill, so no automated customer-side response is possible. LA-1 (shipped in #51) gave the *operator* a structured log record; this gives the *application* the same distinction. The information already existed internally. `Resolution` in `skills_core.py` distinguished all four cases — but only as prose in its `error` string, which `get_skill` discarded with `.skill`. ### The API ```python SkillOutcomeReason = Literal["absent", "integrity_failure", "ok", "store_unavailable", "wrong_version"] @DataClass(frozen=True) class SkillOutcome: skill: Skill | None reason: SkillOutcomeReason detail: str | None async def get_skill_result(key: str, *, version: int | None = None) -> SkillOutcome: ... ``` Chosen over a typed exception, an opt-in strict mode, and an init-time callback. `detail` is the existing human-readable reason string — already safe to surface (skill key and failure mode only, never content, never a filesystem path), and a test pins that it stays that way. `get_skill_result` raises `RuntimeError` only when no store is configured, with the same message as `get_skill`. ### `get_skill` does not change Its contract — `None` for every failure, never raises for one — is documented in its docstring and in the README, and every existing caller treats `None` as "no skill". A test drives all four failures through both accessors: the reason is distinguishable *and* the collapsed form still collapses. ### Not matched on the error string `Resolution` grows a typed `reason`, set explicitly at every construction site. Deriving the public reason by pattern-matching `Resolution.error` is exactly the fragility LA-2 is about, so the mapping is a table a reviewer can read: | `resolve_from_store` outcome | `reason` | |---|---| | the store raised (`unavailable=True`) | `store_unavailable` | | `raw` is not a dict | `absent` | | `verify_raw_skill` returned `None` | `integrity_failure` | | `skill.version != wanted_version` | `wrong_version` | | success | `ok` | `reason` is declared **first and without a default**, so a contributor adding a sixth internal outcome has to decide which public token it maps to rather than inheriting one. That made the two `Resolution` sites in `skills_fs.py` explicit as well — both are the "could not retrieve" path, both already `unavailable=True`, both now say `reason="store_unavailable"`. `Resolution.unavailable` is unchanged. It stays load-bearing on the prune path — only a raising store suppresses pruning, because deleting managed files after a failed lookup would turn an outage into data loss — and `store_unavailable` stays distinct from `absent` for the same reason. ### Three things deliberately not built - **No new telemetry or log record.** Gap 1's `ld.skills.integrity_failure` record already fired inside verification before `resolve_from_store` returned. A test asserts one failed retrieval still produces exactly one log record and exactly one signal. - **Gap 1's 8-token `IntegrityReasonCode` is not threaded into `SkillOutcome`.** `verify_raw_skill` returns `None` and does not surface which token fired; plumbing it up would change that function's return type for a detail the operator already gets from the log record. The five-token public reason is the actionable surface. - **No batch equivalents.** `get_skills` and `all_skills` keep omitting failed entries and keep logging the run-level WARN count. A `get_skills_result` / `all_skills_result` would double the accessor surface for a case nobody has asked for — **possible follow-up** if a customer needs per-key reasons from a batch. ### Tests `packages/client/tests/test_skills.py`, one new class plus three additions to existing export/immutability tests: - one case per reason token — `reason`, whether `skill` is populated, non-empty `detail` - `store_unavailable` is distinct from `absent`: a raising store and an empty store - every non-`ok` outcome carries a detail (swept, so a fifth failure path with no message is caught by a test whose name says what it is about) - `get_skill` still returns `None`, and still never raises, for all four failures - `get_skill_result` raises `RuntimeError` with no store, asserted equal to `get_skill`'s message - no second integrity record or signal from `get_skill_result` - `detail` never carries the skill content - `SkillOutcome` is frozen; `SkillOutcomeReason`'s tokens and the three new exports are pinned ### Docs - `packages/client/README.md` — `get_skill_result` in the Agent Skills API table, a fail-closed example under the Gap 1 observability material (exit on `integrity_failure`, tolerate `absent`), the reason table, and an explicit statement that `get_skill` is unchanged and the two accessors differ only in what they report. - `packages/client/agents.md` — the five-token vocabulary, the `Resolution` → reason mapping, and the instruction that a sixth internal outcome must choose a public token rather than defaulting to `absent`. ### Lockstep with TypeScript The type name, the accessor name, and the five reason tokens are fixed across both SDKs; a sibling PR implements the identical shape in `typescript/`. Tokens are alphabetical, matching how `IntegrityReasonCode` was written on both sides in Gap 1. ### Placement Based on `split/skills-self-healing` (#57), which is based on #54. A new PR at the top of the stack so it disturbs none of the existing reviews: Gap 2 spans `types.py`, `skills.py`, `skills_core.py` and `__init__.py`, which #50–#53 own, and #52/#53/#54 are approved while #50/#51 have live reviewer conversation. ### Checks `uv run pytest` (1481 passed, 11 skipped), `ruff check`, `ruff format --check` all clean. `uv run mypy .` fails repo-wide on a pre-existing duplicate-`conftest` error before checking anything; scoped to the files this PR touches, mypy reports only two pre-existing `unused-ignore` warnings that are present on the base commit too. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Adds **`get_skill_result`** and frozen **`SkillOutcome`** / **`SkillOutcomeReason`** so applications can tell **integrity failure** apart from a missing skill, store outage, or version mismatch—without changing **`get_skill`** (`None` for all failures, same path, no extra telemetry). > > Internal **`Resolution`** now carries a required **`reason`** mapped explicitly at each **`resolve_from_store`** site (including **`skills_fs`** store-unavailable paths); **`get_skill_result`** exposes **`skill`**, **`reason`**, and safe-to-log **`detail`**. > > README and **`agents.md`** document fail-closed handling; tests cover all five reasons, immutability, exports, no double integrity signals, and unchanged **`get_skill`** behavior. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit eda4ae1. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

…esystem can hold (#57) Sixth PR in the Agent Skills stack, and the first that is not part of the original five-PR split. Implements **Gap 3 (AV-3)** and **Gap 4 (DV-2)** from the Agent Skills SDK security design review. Both live in `skills_fs.py`, so they ship together. Stack: #50 references → #51 retrieval → #52 safe-fs → #53 materialization → #54 fs-hardening → **#57 (this)**. ## Why this is a new PR rather than an edit to #53 The code Gap 3 changes lives in `skills_fs.py`, which #53 (`split/skills-materialization`) owns. Editing #53 would be the wrong move: - **#53 and #54 are `APPROVED`.** Editing #53 revokes that approval on the second-largest PR in the stack and forces #54 to rebase again — it has already been rebased twice this week (the str→bytes pivot, then the Gap 1 log record). - **Gap 3 relaxes a clobber refusal.** That is the most safety-sensitive change in the whole gap set, and it needs to be reviewable in isolation rather than buried in a rebase of an already-approved PR. Being a new branch on top, nothing below needs rebasing and no approval is disturbed. ## Gap 3 — self-healing partial reconciles (review priority: Medium) `_write_all` reconciles every skill and only then does `_rewrite_manifest` write the manifest, atomically, once, last. A process killed after a skill file lands but before that write leaves the file at a managed path with **no manifest entry** — exactly the condition `_write_one` treats as an unmanaged-file collision. The skill was then 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: ``` if exists: read on-disk bytes via _read_regular_file if sha256(on_disk) == content_hash -> adopt: record the entry, report skipped_current if not managed -> refuse, with the existing message else -> action = "updated" else: action = "written" ``` This cannot weaken the clobber guarantee, because only byte-identical LaunchDarkly content is ever adopted. Differing unmanaged bytes are still refused, untouched. **The existing clobber tests pass unchanged** — including `test_unmanaged_file_is_never_overwritten`, which writes `"user authored\n"`. Three details carry the safety: - **A failed read 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 whose bytes could not be read has not been shown to be ours. - **The read stays on `_read_regular_file`, not `Path.read_bytes`.** Adoption widens it to genuinely foreign files, so its refusal of FIFOs and other non-regular files is now load-bearing rather than defensive. - **`skipped_current` is reused rather than adding an `adopted` kind**, so `ReconcileActionKind` in `types.py` — public, and owned by approved #50/#53 — does not change. Its documented meaning ("the bytes on disk already are the resolved content") already fits, and the reuse is documented in a comment. ### One decision the brief did not settle: a bound on the read Adoption reads files the manifest does not list, so `_read_regular_file` gained a `max_bytes` parameter and reads at most `len(content) + 1` bytes. Without it, adoption would be a memory amplifier: a multi-gigabyte file at a managed path would be slurped whole and then refused, where before the change it was refused without being read at all. The `+ 1` is load-bearing — a bound of exactly `len(content)` would adopt (and later prune) any customer file that merely *begins* with the resolved content. There is a test for that off-by-one, and it fails if the `+ 1` is removed. ### The caveat, stated plainly Adoption creates a manifest entry, so the adopted file becomes prunable on a later reconcile. That is correct and not a weakening: adoption only fires when the bytes are byte-identical to LaunchDarkly-resolved content, so a later prune removes content LaunchDarkly delivered anyway — exactly what would have happened had the crash not occurred. The guarantee that matters is preserved: **differing unmanaged bytes are never overwritten.** There is a test pinning the caveat too, so it cannot change silently. The review also floats a write-intent journal as an alternative. Assessed as over-engineered; **not built**. No case was found where the adoption rule fails. ## Gap 3, secondary — orphaned temp files (landed, not deferred) `atomic_write` names its temp file `.SKILL.md.<16 hex>.tmp` and unlinks it on exception — but not after a `SIGKILL`. `_prune` walks manifest entries, which an orphan never has, so nothing would ever notice one. The second-order effect is worse than the wasted bytes: `_prune_one`'s `rmdir` only succeeds on an empty directory, so **a single orphaned temp pins a skill's directory forever.** A sweep now runs on both the write and the prune path (before the `rmdir`, so it actually unblocks it). It 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 — a new `is_temp_name` / `temp_name_prefix` pair beside the naming code, matched with `fullmatch` so both ends are anchored. The pattern is **derived from the producers rather than copied**: `atomic_write` and `_mkstemp_at` now build their names from the same constants the recognizer reads, and the recognizer covers both producers (`secrets.token_hex(8)` on the descriptor path, `tempfile.mkstemp`'s own sequence on the Windows fallback); - only regular files, with the type read off the descriptor, never a followed path; - unlinked through the existing descriptor-pinned `unlink_file` — no new path-based unlink; - never raises, never aborts a run; a failure is a logged warning. ## Gap 4 — Windows reserved device names (review priority: Low) The key grammar `^[a-z0-9][a-z0-9-]*$` admits `con`, `nul`, `aux`, `prn`, `com1`–`com9`, `lpt1`–`lpt9` — 22 names, none of which can be a directory name on Windows. Rejected in `_key_rejection_reason`, after `is_valid_skill_key`, matching the existing ordering discipline in that function. ### It belongs in the filesystem layer, not the shared key grammar `types_validation.skill_key_rejection_reason` / `is_valid_skill_key` are deliberately **untouched**. Please do not "correct" this: - `parse_ai_config` fails closed on a bad `skills` entry, by design. A grammar-level rejection would therefore invalidate the **entire AI Config** — model, provider, instructions, tools — for a Linux customer, over a constraint that only exists on Windows. - `skill_refs` would silently shrink, and its own docstring explains the consequence: a dropped reference lets `write_skills` **prune** the skill's on-disk copy. That converts "this skill fails to write on Windows" into "this skill gets deleted on Linux." - **Precedent:** the 255-byte component bound already lives in this layer for exactly this reason, documented in both codebases in those words. - `_key_rejection_reason` is already shared by the write and prune paths, so one edit covers both destructive paths. A test asserts the layer choice directly: for each of the 22 names, `is_valid_skill_key` is still `True`, `parse_ai_config` still accepts a config referencing it, and `skill_refs` still projects it. ### Unconditional, not platform-gated No `if os.name == "nt"`. - A managed root written by a Linux container can be read from a Windows host — an ordinary deployment — so the on-disk result must not depend on the writer's OS. - Neither repo has a Windows CI runner (verified: every job in `ci.yml` is `ubuntu-latest`), so a platform-gated branch would be untestable in CI — the exact condition that produced this gap. **The trade, stated honestly and belonging in the release notes:** a customer who legitimately names a skill `aux` now gets a reported `error` action on Linux where it would previously have worked. The set is exactly 22 names. No suffix stripping and no case folding are needed, and a comment says why: the key grammar admits no `.` and no `$`, so the `con.txt` and `CONIN$` / `CONOUT$` forms are unreachable, and keys are already lowercase-only. `com0` / `lpt0` are **not** reserved and are not included — there is a test for that, and for `con1` / `nul2` / `conx`. ### Residual documented, not implemented `MAX_PATH` overflow. The 255-byte bound is per *component* and does not bound the total path — `<customer root>` + `<key>` + `/SKILL.md` can still exceed Windows' 260-character `MAX_PATH` with a legal key. The SDK cannot validate this because the root is the customer's, so it is one README sentence rather than a check. ## Scope and non-goals - Neither gap emits the Gap 1 integrity-failure log record. A key rejection and a clobber refusal are `ReconcileAction` errors, not integrity failures — no `reason_code` tokens, no `record_integrity_failure` call. - No check in `_unsafe_path_reason`, `_key_rejection_reason`, or `safe_fs.py` was relaxed. `agents.md` marks these non-relaxable, and the new material there says the same about the adoption rule and the sweep, so a later contributor cannot widen either. - **No public API change**: `types.py` is untouched. - Gap 2 (typed outcome for integrity failure vs absence) is deliberately **not** here — it is a separate public-API decision, planned next. Gap 1 landed in #51 and was not modified. ## Docs - `packages/client/README.md`: the adoption rule under the existing `write_skills` conservatism paragraph, the reserved-name rejection with the honest trade, and the `MAX_PATH` residual. - `packages/client/agents.md`: the 22-name list, the adoption rule and its non-widenable details, the layer-choice reasoning, and the sweep's bounds — all in the non-relaxable-checks material. ## Tests `packages/client/tests/test_skills_fs.py`, +388 lines, 91 new cases: - **Crash-mid-reconcile recovery** (the review asks for this by name in the abuse suite): a byte-identical file with no manifest entry is adopted, reported `skipped_current`, recorded in the manifest, and the next reconcile is an ordinary no-op. Plus: adoption writes nothing (`os.replace` spy sees no call), emits `skipped_current` telemetry, and the adopted file is prunable afterwards. - Byte-differing unmanaged content still refused, untouched, no write attempted. - The `len + 1` off-by-one: a longer file sharing the content prefix is not adopted. - An unmanaged FIFO refused, not read, and does not hang. - A read error on an unmanaged file refuses rather than overwriting, with the distinguishable message, and adds no manifest entry. - Orphan swept on write; orphan no longer blocks `rmdir` on prune; eight lookalike names left alone; a symlink wearing the temp name is not removed and its target survives. - All 22 reserved names through **both** `write_skills` and the prune path, plus the grammar-level assertion above and the non-reserved neighbours. Every new test was mutation-checked — reverting each of the four behaviours (reserved-name check, sweep, adoption, the `+ 1`) makes the suite fail. ## Checks `uv run ruff format --check .`, `uv run ruff check .`, `uv run mypy packages/*/src` (CI's command), `uv run pytest` — all clean: **1469 passed, 11 skipped**. Note: `uv run mypy .` (the `Makefile`'s `typecheck` target) fails on a pre-existing duplicate-`conftest` module-resolution error in `packages/ai/tests` / `packages/claude-agents/tests`, unrelated to this change and present on `main`. CI runs `mypy packages/*/src`, which is clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

**PR 5 of 5** splitting draft #45 for review. Stacked on #53. This is the tip of the stack — its tree is #45's, plus the changes for review findings 1, 2 and 3 and the new `safe_fs` unit tests. `main` ← #50 ← #51 ← #52 ← #53 ← **`split/skills-fs-hardening`** > [!NOTE] > **Aug 28:** rebased in place for the stack-wide bytes pivot (`Skill.content: bytes`, `frontmatter()` deleted). Test helpers here construct `Skill` values from encoded bytes now; the defenses under test are unchanged. This branch also carries the rename-containment assertion fix cherry-picked across the rebase. ## What's here Tests only, no source. The adversary: every filesystem defense PRs 3 and 4 introduced now has a test that fails if the defense is removed. - **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 containment check asserted on inode identity rather than on path strings. - **Symlink attacks** — a symlinked skill directory, a symlinked target file, and `<root>/<key>` swapped for a symlink at the exact instant of the rename and of the unlink. That last one is the narrowest possible version of the window descriptor pinning exists to close, fired from the interception point (`os.replace` / `os.unlink`) 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" must not 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. Where the platform has `renameat` that is asserted by descriptor identity (`src_dir_fd == dst_dir_fd`, resolving to the skill directory's `(st_dev, st_ino)`), which is stronger than comparing path strings because it also rules out the descriptor having been redirected between the check and the rename. - **Telemetry** — the three signal names are asserted as an **allowlist, not a floor**: any other name reaching the emitter fails the test. The two deliberately excluded names are called out explicitly rather than left to the subset check. No signal carries a filesystem path or the skill body, an emitter that raises never fails the reconcile, and `client.track()` is never reached. ## Notes for reviewers Two cases 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*. An arbitrary wrong hash is rejected by the mismatch check first and never exercises the encoder guard at all, so a naive test here passes against an implementation that reaches for `errors="surrogatepass"`. - The **redaction** cases smuggle the body through `contentHash` and through `key`. A sweep using a well-formed digest under a valid key reaches neither replacement branch. The set of tests in `test_skills_fs.py` at this tip matches #45's modulo the bytes pivot (helpers build `Skill.content` as `bytes`; the deleted `frontmatter()` tests were removed in PR 1) — nothing else was dropped in the split. Two classes are reordered, which is why the file shows more diff churn than the content change warrants. ## Testing `uv run pytest` → 1280 passed. `ruff check`, `ruff format --check`, and `mypy packages/*/src` all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > **Tests only** — expands `test_skills_fs.py` so every filesystem hardening behavior from earlier stack PRs has a regression test in one place (no implementation changes). > > The module gains interception helpers (`_ReplaceSpy`, `_SwapDirectoryDuring`, inode-based `_assert_atomic_rename_of`) to assert SDK validation blocks writes before `os.replace`, and to simulate rename/unlink races and injected rename failures. > > New coverage includes **atomicity** (0644, single in-directory rename, no partial files on failure), a parametrized **path traversal** matrix (hostile keys must not attempt `SKILL.md` renames), **symlink** refusal and descriptor-pinned TOCTOU cases (skipped when `SUPPORTS_DIR_FD` is false), a forced **no-`*at()`** Windows-shaped fallback, **FIFO/non-regular** paths, **clobber protection**, **corrupt manifest** fail-closed behavior, and **telemetry** allowlists (materialized/revoked/integrity only, no paths or bodies, manifest version redaction, integrity property parity across accessor vs write paths). > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit c88d5bf. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

…4/5) (#53) **PR 4 of 5** splitting draft #45 for review. Stacked on #52. `main` ← #50 ← #51 ← #52 ← **`split/skills-materialization`** ← `split/skills-fs-hardening` > [!IMPORTANT] > **Aug 28 repivot: skills are now opaque byte buffers by construction.** `Skill.content` is `bytes` (the verified verbatim bytes, exactly what was hashed), the `frontmatter()` convenience accessor and `frontmatter.py` are deleted, and the SDK no longer parses or interprets skill content anywhere. Consumers who want frontmatter parse it themselves. The stack was rebased in place to make each change in the slice that introduced the code; the TypeScript SDK is getting the mirror change (`content: Uint8Array`) separately. ## What's here `write_skills` writes `<root>/<key>/SKILL.md` and records what it owns in a manifest at `<root>/.launchdarkly-skills.json`, so it overwrites or removes **only** paths that manifest records. A file you placed yourself is reported and left untouched. ```python report = await write_skills(refs, ".claude/skills") for action in report.errors: print(f"skill {action.key or '<run>'}: {action.error}") ``` `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; callers grouping a report by key need to expect that sentinel. New exports: `write_skills`, `ReconcileAction`, `ReconcileReport`, `ReconcileActionKind`, `OnUnavailable`, `SKILL_FILENAME`, `MANIFEST_FILENAME`, `MANIFEST_VERSION`. ## The defenses 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 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. With the bytes pivot, this pass hands `Skill.content: bytes` to the same `verified_bytes` the accessors use, which hashes the bytes directly — same two-pass design, identical telemetry property keys. Pruning removes formerly-managed skills that are no longer referenced — that 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. 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. It reports a withholding count at WARN for the same reason the accessors do (finding 2). ## Notes for reviewers - **`write_skills` blocks.** It is `async` for parity with the other accessors and with the TypeScript SDK, but it awaits nothing: every read, write, `fsync` and rename runs inline. Wrap it in `asyncio.to_thread` if that matters on your loop. For the same reason `timeout` is checked *between* steps rather than interrupting one already in progress. Reconcile one root at a time — because nothing yields today a run is atomic against the rest of your loop, and wrapping it to run concurrently makes two runs against one root race on the manifest. (Review findings 4 and 6 on #45 both live here and are tracked there, not resolved in this PR.) - **The root's parent must exist.** `write_skills` creates the root itself but never its ancestors, so a typo cannot scatter a directory tree across a project. An absent parent, a root that is an existing file, and a root that is a symlink each raise `ValueError` — caller errors, distinct from the per-skill `error` actions in the report. - **The security abuse matrix is PR 5.** Every guard listed above is in this diff; what lands next is the adversary that proves each one fails without it — path traversal, symlink attacks, the directory swap fired at the instant of the rename, clobber protection, corrupt manifests, atomicity under an injected crash, and the telemetry allowlist. This PR's own tests cover the happy path, reconcile semantics, manifest round-tripping, resilience, and verify-before-write. Splitting `skills_fs.py` itself along that line would have meant shipping a deliberately weakened `_write_one`/`_prune` here and hardening it there, which `agents.md` marks non-relaxable. ## Testing `uv run pytest` → 1216 passed. `ruff check`, `ruff format --check`, and `mypy packages/*/src` all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Adds **Agent Skills materialization**: `write_skills` reconciles verified skills to `<root>/<key>/SKILL.md`, tracks SDK-owned paths in `<root>/.launchdarkly-skills.json`, and returns a **`ReconcileReport`** (`written` / `updated` / `skipped_current` / `removed` / `error`, plus `.ok` / `.errors`). > > Implementation lives in new **`skills_fs.py`** (split from retrieval in `skills.py`), using descriptor-pinned **`safe_fs`** writes, manifest-authorized overwrites/deletes, fail-closed behavior on corrupt manifests, **no prune** when retrieval is incomplete or verification fails (so outages/tampering cannot masquerade as revocation), re-verify before write, and optional **`"*"`** / **`prune`** / **`timeout`** / **`on_unavailable`**. > > **Public API** expands via `__init__.py`: `write_skills`, `ReconcileAction` / `ReconcileReport` / `ReconcileActionKind`, `OnUnavailable`, and `SKILL_FILENAME` / `MANIFEST_FILENAME` / `MANIFEST_VERSION`. README and **`agents.md`** document the flow, blocking async I/O note, and invariants; **`skill_refs`** docs note that dropped invalid entries can otherwise cause unintended prune. > > Tests add **`test_skills_fs.py`** (happy path, manifest, reconcile, root errors, resilience) plus report/export coverage in **`test_skills.py`**; adversarial FS tests are deferred to a follow-up PR per description. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 4c6d965. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

**PR 3 of 5** splitting draft #45 for review. Stacked on #51. `main` ← #50 ← #51 ← **`split/skills-safe-fs`** ← `split/skills-materialization` ← `split/skills-fs-hardening` > [!NOTE] > **Aug 28:** rebased in place for the stack-wide bytes pivot (`Skill.content: bytes`, `frontmatter()` deleted). No change to this slice's own content beyond the rebase. ## What's here `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 (PR 4) is its only caller. **Why descriptors and not paths.** 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 as `if dir_fd is not None` and cannot forget the `os.close`. - `atomic_write` creates its temp file with `O_CREAT | O_EXCL | O_NOFOLLOW` **at** that descriptor (`_mkstemp_at`, since `tempfile` has no `dir_fd` form), `fchmod`s the descriptor rather than `chmod`ing 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, 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. **The capability probe is not the obvious one.** `SUPPORTS_DIR_FD` deliberately names `os.rename` and `os.stat` rather than the `os.replace` and `os.lstat` this module actually 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 (Windows) every operation falls back to the identical full-path sequence, the per-component `lstat` floor. ## Notes for reviewers The 22 tests here exercise the module **directly**, on its own terms — they are new work, not present in #45, added so this slice does not ask you to read 311 lines of security-critical primitives with nothing asserting them. In #45 every one of these guarantees was reached only through `write_skills`. The TOCTOU races these primitives exist to close are proved in PR 5, which is what actually holds a descriptor across a sequence of operations and can fire a directory swap at the instant of the rename. ## Testing `uv run pytest` → 1167 passed. `ruff check`, `ruff format --check`, and `mypy packages/*/src` all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Introduces **`safe_fs.py`**, a skills-agnostic layer for **descriptor-pinned** directory I/O so writes and unlinks don’t re-resolve paths after validation (closing TOCTOU symlink-swap races). On POSIX it uses `*at()`-style operations via `pinned_directory`, `atomic_write` / `atomic_write_in`, and `unlink_file` (with **`SymlinkRefused`** when a symlink stands in for an expected file); on Windows it falls back to the documented per-component `lstat` path when **`SUPPORTS_DIR_FD`** is false, using a non-obvious capability probe so CPython’s `os.supports_dir_fd` doesn’t silently disable the defense. > > Adds **`test_safe_fs.py`** (~22 tests) for pinning, atomic writes (mode `0644`, temp cleanup, single `os.replace` site), unlink behavior, and the dir-fd probe. **`agents.md`** documents the module in the file map and a new “Descriptor-pinned filesystem access” section. Integration with skill materialization is left to a follow-up slice; nothing else in the client imports this module yet. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 9571dca. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

cursor

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

^{Reviewed by Cursor Bugbot for commit dad1d30. Configure here.}

cursor · 2026-09-15T14:38:32Z

+    if skill is None:
+        return Resolution(
+            reason="integrity_failure",
+            error=f"skill '{key}' failed integrity verification and was withheld",


Key mismatch crashes skill resolution

High Severity

The key-mismatch defense in resolve_from_store builds a Resolution without reason, which is required and has no default. A store that answers under a different key now raises TypeError instead of withholding the skill. That breaks get_skill's contract of returning None for every retrieval failure and never raising, and it also crashes get_skill_result and write_skills.

^{Reviewed by Cursor Bugbot for commit dad1d30. Configure here.}

cursor · 2026-09-15T14:38:32Z

+            return held.get(version) or self._loose.get(key)
+        if held:
+            return held[max(held)]
+        return self._loose.get(key)


Pinned miss returns malformed leftover

Medium Severity

_SkillObjectSet.get still falls through to the version-less entry on every pinned miss, even when well-formed versions of that key are already held. A pin for a version that was never delivered then surfaces a leftover malformed object, so verification records invalid_version and get_skill_result reports integrity_failure for a skill whose integrity is not in question.

^{Reviewed by Cursor Bugbot for commit dad1d30. Configure here.}

cursor Bot reviewed Aug 25, 2026

View reviewed changes

Comment thread packages/client/src/launchdarkly_ai_server/skills_core.py Outdated

Comment thread packages/client/src/launchdarkly_ai_server/skills.py

XieX requested review from donei003 and knfreemLD August 26, 2026 20:11

donei003 reviewed Aug 27, 2026

View reviewed changes

donei003 approved these changes Aug 27, 2026

View reviewed changes

knfreemLD requested changes Aug 28, 2026

View reviewed changes

XieX force-pushed the split/skills-retrieval branch from 3d4f0a1 to 27ef12f Compare August 28, 2026 18:03

XieX force-pushed the split/skills-references branch from e510acd to 47a2a4a Compare August 28, 2026 18:03

cursor Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread packages/client/src/launchdarkly_ai_server/skills.py Outdated

XieX and others added 6 commits August 28, 2026 16:11

XieX mentioned this pull request Aug 30, 2026

feat(client): Agent Skills — self-healing reconciles, and keys no filesystem can hold #57

Merged

XieX mentioned this pull request Aug 31, 2026

feat(client): Agent Skills — a distinguishable outcome for integrity failure (Gap 2 / LA-2) #58

Merged

XieX and others added 7 commits September 4, 2026 10:19

chore: merge split/skills-retrieval into split/skills-safe-fs

9571dca

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>

XieX and others added 2 commits September 11, 2026 14:25

chore: merge split/skills-safe-fs into split/skills-materialization

a6aae0a

Brings in the withheld-answer fix the reconcile regression tests below depend on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

XieX requested a review from knfreemLD September 11, 2026 18:36

XieX and others added 16 commits September 11, 2026 14:46

XieX merged commit 68153ee into split/skills-references Sep 15, 2026
4 checks passed

XieX deleted the split/skills-retrieval branch September 15, 2026 14:36

cursor Bot reviewed Sep 15, 2026

View reviewed changes

		return ld_client


		async def _resolve_client(opts: InitClientOptions, client: Any) -> Any:

Uh oh!

Conversation

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

Uh oh!

What's here

Verification

Review findings addressed

Notes for reviewers

Testing

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Sep 15, 2026

Choose a reason for hiding this comment

Key mismatch crashes skill resolution

Uh oh!

cursor Bot Sep 15, 2026

Choose a reason for hiding this comment

Pinned miss returns malformed leftover

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

XieX commented Aug 25, 2026 •

edited by cursor Bot

Loading

cursor Bot left a comment •

edited

Loading