feat(client): Agent Skills — a distinguishable outcome for integrity failure (Gap 2 / LA-2) - #58
Merged
Merged
Conversation
…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>
…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>
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>
…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>
andrewklatzke
approved these changes
Sep 14, 2026
Stacked on the protocol-layer PR (`xie/skills-fdv2-protocol`). Third of three PRs split out of #69, and the one that makes the feature real: the network underneath the protocol reader, exported as `FDv2SkillStore`. ## Why this shape Skill content arrives over `GET /sdk/poll` and `GET /sdk/stream`, authenticated with the environment's server-side SDK key. These are the SDK-facing endpoints the base SDK's FDv2 data source uses, and the channel that payload signing will eventually cover. No private route is involved, and no credential other than the environment's own SDK key ships to a customer host. Standard library only, so the content path adds no dependency to a package whose sole runtime dependency is `opentelemetry-api`. ## What's here **`FDv2SkillStore`.** Authenticates, streams (default) or polls, carries `basis` across requests, sends `If-None-Match` and treats 304 as a first-class current answer, and serves `get_object` / `all_objects` / `add_listener` / `remove_listener` from what the protocol reader has committed. Capped jittered backoff; `Retry-After` honoured but clamped to `max_backoff` and rejected when non-finite; bounded consecutive-failure retries, where a committed payload resets the count. One network timeout, `read_timeout`, whose default follows the mode: 10s for a whole poll, 300s between reads on a stream. A mobile key or client-side environment ID raises from the constructor. Last known good survives every failure; `diagnostics` and `failed` report the degradation. **`close` interrupts the socket.** The delivery thread parks in a read no flag can reach, and closing a urllib response from another thread does not unblock CPython's buffered reader, so `_interrupt_read` shuts the socket down underneath it. Without that every shutdown of a *healthy* stream blocked for the full join timeout. **Above the interface**, two strings: `NO_STORE_MESSAGE` now names `FDv2SkillStore` first, since it is the first thing a user sees on a missing store and offering only the development store was wrong once a production transport existed; and `watch_skills`' refusal message names it as the store with a delivery transport. ## Bugs found and fixed while testing the loop Five, all sharing one shape: the store stopped delivering while continuing to report itself healthy. - **The consecutive-failure counter never reset in stream mode.** `_stream_once` always ends by raising, so a reset on return was unreachable and `failures` grew for the whole process lifetime. Eleven *fully successful* payload transfers were enough to trip `max_consecutive_failures` and stop delivery for good, revocations included. A commit now resets the count, in `_apply`. - **A non-finite `Retry-After` killed the delivery thread.** `float("inf")` parses, and `Event.wait(inf)` raises `OverflowError` from inside the recoverable-error handler. Non-finite values are rejected and every honoured delay is clamped to `max_backoff`. - **`close()` during the initial connect waited out its full join timeout.** `self._connection` was assigned after the connect returned, so a `close()` in that window found nothing to interrupt. The stop flag is re-checked immediately after the assignment. - **`close()` blocked for the full join timeout on every healthy stream.** See `_interrupt_read` above. - **A stream interrupted by our own `close` was reported as a delivery failure.** Also: `connect_timeout` was accepted and never used, so a poll against a black-holed host hung for 300s rather than 10. It is gone, with the request timeout now chosen by mode, and `TestTimeouts` measures the bound against a socket that accepts and never answers. ## Tests > **Rebase note.** The previous push of this branch had silently reverted the protocol PR's last commit (payload identity: `payloads_ignored`, `_is_foreign_payload`, `TestPayloadIdentity`). Rebasing onto the updated protocol branch restored it; the full suite passes with it present. `_FakeFDv2Endpoint` is an in-process `ThreadingHTTPServer` implementing the wire contract, so request construction and header handling are exercised over real sockets rather than mocked. Covers skill put/delete over the wire, mixed payloads, 304, `basis` round-tripping, reconnect/backoff in both modes, `Retry-After` including non-finite and oversized values, bounded retries and the reset on commit, prompt shutdown during connect and during a healthy stream, hashless envelopes end to end through the accessors, server-side-only credentials, timeouts, and `watch_skills` over the transport: a wire-level revocation pruning a file without a restart. Full suite 1629 passing, 11 skipped; `ruff`, `ruff format`, and `mypy` clean. ## Open items, none in this PR's scope - 🔴 **`contentHash` is not on the wire yet.** Against a real environment today every skill resolves to nothing. This PR makes that loud (an error per hashless object, a summary per wholly-hashless payload, `diagnostics.hashless_objects`) rather than surviving it. - 🔴 **Server-side skill delivery is not deployed.** The wire shape this store reads — kind `skill`, key `<key>:<version>`, generic payload — is what [streamer #4681](launchdarkly/streamer#4681) and [gonfalon #70638](launchdarkly/gonfalon#70638) emit; both are still open. No account can receive skill objects until they ship and the producer is enabled. - 🟡 **FDv2 is opt-in per account.** A real environment returns 403 today; the store reports it as fatal and explains what to do. - ~~🟡 **`mv` is a guess.**~~ Resolved: the request sends no `mv`. That parameter selects the *flag* data model and the connection rejects any value but the flag default, while the generic agent-skill payload is served regardless of it. The `data_model_version` constructor argument is gone with it. - 🟡 **No payload signing** on this channel yet, so Beta is TLS-only. - 🟡 **The connection also carries the environment's flags.** Skipped and counted; a transport property, not fixable here. - 🟡 **`ld-relay` does not speak the FDv2 endpoints**, so relay-only deployments cannot receive skills in Beta. **Nothing here has touched a real LaunchDarkly environment**, because it cannot yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Adds **`FDv2SkillStore`**, a production `SkillStore` that pulls agent skills over LaunchDarkly’s SDK FDv2 **`/sdk/poll`** and **`/sdk/stream`** endpoints (stdlib HTTP, background delivery thread, stream-by-default). It implements **`SkillStore`** (`get_object`, listeners, etc.) on top of the existing protocol reader, plus **`StoreDiagnostics`**, **`wait_for_skills`**, capped backoff with **`Retry-After`**, and **`close`** that interrupts blocked socket reads so shutdown is prompt. > > **Public surface:** `FDv2SkillStore` and `StoreDiagnostics` are exported from the package; README documents production setup with `init_client` and `watch_skills`. Server-side SDK keys only; mobile/client credentials are rejected. Outages keep last-known-good content; accessors above the store are unchanged. > > **Delivery-loop fixes** bundled here: reset consecutive-failure counts on successful commits / up-to-date answers (so healthy stream recycling does not stop delivery), safe handling of non-finite **`Retry-After`**, and not treating intentional **`close`** interrupts as transport failures. Removed unused **`connect_timeout`**; **`read_timeout`** is the single knob with mode-specific defaults. > > **Tests:** in-process fake FDv2 server exercises poll/stream, basis/ETag/304, revocations, retries, timeouts, hashless payloads, and **`watch_skills`** over the transport. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 88c225e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…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) <!-- CURSOR_SUMMARY --> --- > [!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. > > <sup>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).</sup> <!-- /CURSOR_SUMMARY -->
…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) <!-- CURSOR_SUMMARY --> --- > [!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. > > <sup>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).</sup> <!-- /CURSOR_SUMMARY -->
… 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) <!-- CURSOR_SUMMARY --> --- > [!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. > > <sup>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).</sup> <!-- /CURSOR_SUMMARY -->
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 51f06a6. Configure here.
| committed=True, | ||
| changes=changes, | ||
| basis=state if isinstance(state, str) and state else None, | ||
| ) |
There was a problem hiding this comment.
Declined transfer still advances basis
High Severity
A declined foreign payload still comes back as a commit and still carries that payload's state as basis. _apply then overwrites the reconnect cursor, so the next poll or stream asks the server for updates from the wrong payload. Skill puts and revocations can be skipped after that, which leaves revoked content in memory and on disk.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 51f06a6. Configure here.
| target=self._run, name="ld-ai-skills-fdv2", daemon=True | ||
| ) | ||
| self._thread.start() | ||
| return self |
There was a problem hiding this comment.
Failed store cannot be restarted
Medium Severity
start after _give_up spawns a new delivery thread but leaves _failed_reason and _failures untouched. failed then stays set for the rest of the process even if delivery is running again, and the first recoverable error on the new thread immediately gives up because the counter is already over the cap.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 51f06a6. Configure here.
XieX
added a commit
that referenced
this pull request
Sep 15, 2026
`resolve_from_store` withholds an answer served under a key other than the one asked for — identity is read off the object itself, so a substituted skill would otherwise be handed back under the caller's key while carrying its own. That construction site was left without a `reason` when #58 made the field required, so the path raised `TypeError` instead of returning a `Resolution`: a store substituting one skill for another crashed the caller rather than being withheld from it, which is the opposite of what the check is for. Three tests were failing on it (`test_a_store_answering_under_a_different_key_is_withheld` and the two `..._another_key_...` cases in `test_skills_fs.py`), along with `mypy`, which caught the missing argument exactly as the no-default design intended. The token is `integrity_failure`. Content was delivered and its identity did not verify, and that is the one outcome a caller is expected to fail closed on; reporting `absent` would file a substituting store in the bucket the same caller is invited to tolerate. It is not `wrong_version` either — that names a version mismatch specifically, and there is deliberately no `wrong_key` to parallel it. The choice is not recoverable from the error message, so a test now pins it rather than leaving it to the next reader. Two unrelated failures remain on this branch, both from the content cap moving to 10 MB while the tests still build their oversize fixture at 64 KiB + 1: `test_oversize_skill_aborts_the_write` and `test_integrity_signal_property_keys_match_across_layers`. Left alone — that change is in flight elsewhere and its owner should decide whether the fixture or the cap is what moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_skillreturnsNonefor four distinct outcomes — no such skill, the store raised, therequested 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.
Resolutioninskills_core.pydistinguishedall four cases — but only as prose in its
errorstring, whichget_skilldiscarded with.skill.The API
Chosen over a typed exception, an opt-in strict mode, and an init-time callback.
detailisthe 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_resultraisesRuntimeErroronly when no store is configured, with the samemessage as
get_skill.get_skilldoes not changeIts contract —
Nonefor every failure, never raises for one — is documented in itsdocstring and in the README, and every existing caller treats
Noneas "no skill". A testdrives all four failures through both accessors: the reason is distinguishable and the
collapsed form still collapses.
Not matched on the error string
Resolutiongrows a typedreason, set explicitly at every construction site. Deriving thepublic reason by pattern-matching
Resolution.erroris exactly the fragility LA-2 is about,so the mapping is a table a reviewer can read:
resolve_from_storeoutcomereasonunavailable=True)store_unavailablerawis not a dictabsentverify_raw_skillreturnedNoneintegrity_failureskill.version != wanted_versionwrong_versionokreasonis declared first and without a default, so a contributor adding a sixthinternal outcome has to decide which public token it maps to rather than inheriting one. That
made the two
Resolutionsites inskills_fs.pyexplicit as well — both are the"could not retrieve" path, both already
unavailable=True, both now sayreason="store_unavailable".Resolution.unavailableis unchanged. It stays load-bearing on the prune path — only araising store suppresses pruning, because deleting managed files after a failed lookup would
turn an outage into data loss — and
store_unavailablestays distinct fromabsentfor thesame reason.
Three things deliberately not built
ld.skills.integrity_failurerecord alreadyfired inside verification before
resolve_from_storereturned. A test asserts one failedretrieval still produces exactly one log record and exactly one signal.
IntegrityReasonCodeis not threaded intoSkillOutcome.verify_raw_skillreturnsNoneand does not surface which token fired; plumbing it upwould 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.
get_skillsandall_skillskeep omitting failed entries andkeep logging the run-level WARN count. A
get_skills_result/all_skills_resultwoulddouble 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 existingexport/immutability tests:
reason, whetherskillis populated, non-emptydetailstore_unavailableis distinct fromabsent: a raising store and an empty storeokoutcome carries a detail (swept, so a fifth failure path with no message iscaught by a test whose name says what it is about)
get_skillstill returnsNone, and still never raises, for all four failuresget_skill_resultraisesRuntimeErrorwith no store, asserted equal toget_skill's messageget_skill_resultdetailnever carries the skill contentSkillOutcomeis frozen;SkillOutcomeReason's tokens and the three new exports are pinnedDocs
packages/client/README.md—get_skill_resultin the Agent Skills API table, afail-closed example under the Gap 1 observability material (exit on
integrity_failure,tolerate
absent), the reason table, and an explicit statement thatget_skillisunchanged and the two accessors differ only in what they report.
packages/client/agents.md— the five-token vocabulary, theResolution→ 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
IntegrityReasonCodewas 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 thestack so it disturbs none of the existing reviews: Gap 2 spans
types.py,skills.py,skills_core.pyand__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 --checkall clean.uv run mypy .fails repo-wide on a pre-existing duplicate-conftesterror before checkinganything; scoped to the files this PR touches, mypy reports only two pre-existing
unused-ignorewarnings that are present on the base commit too.🤖 Generated with Claude Code
Note
Overview
Adds
get_skill_resultand frozenSkillOutcome/SkillOutcomeReasonso apps can tell integrity failure and store outage apart from a missing skill, whileget_skillstill returnsNonefor every failure. InternalResolutionnow carries an explicitreasonat each construction site (no default), wired through materialization paths inskills_fs.Also ships production skill delivery and live disk sync: new
FDv2SkillStore(poll/stream FDv2, diagnostics, server-side key checks),watch_skills/SkillWatcher(debounced re-write_skillson store changes), andremove_listeneronInMemorySkillStoreand the FDv2 store. Public exports and docs expand for FDv2 usage, fail-closed tampering handling, POSIX vs Windows filesystem bounds, and privilege-separated reconcile vs agent deployment.Reviewed by Cursor Bugbot for commit 51f06a6. Bugbot is set up for automated code reviews on this repo. Configure here.