feat(client): Agent Skills — retrieval through an injectable store (2/5)

[XieX]

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 the content retrieval layer for Agent Skills: callers configure an injectable SkillStore via init_client(options={"skillStore": store}) and use get_skill, get_skills, and all_skills, plus InMemorySkillStore for local/testing use. Public exports also include the SkillStore protocol.

Implementation is split into new skills_core (shared verification, store resolution, telemetry) and expanded skills (accessors and in-memory store). SkillStore.get_object now takes version as part of lookup identity so pinned references resolve correctly when multiple versions of one key coexist.

Untrusted store data is verified before any Skill is returned: key/version rules, mandatory contentHash, 64 KiB cap, strict UTF-8 encoding (no surrogate fabrication), and sha256 over verbatim bytes. Failures are withheld (return None / omit from batches), with per-skill ld.skills.integrity_failure ERROR logs (stable SIEM contract) and an allowlisted internal telemetry seam (default no-op; not client.track()).

Lifecycle: skillStore is applied on every successful init_client (even when the LD client singleton is already initialized); shutdown() and test reset clear the store. README and agents.md document the API, integrity logging, and invariants.

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

[donei003]

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?

[XieX]

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

1. 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
2. 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]

Technically our cap is 50kb

[knfreemLD]

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

[XieX]

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?

[knfreemLD]

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?

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

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

[cursor]

Pinned miss serves leftover malformed object

Medium Severity

A versioned get_object falls through to _loose whenever the exact pin is absent, even if other well-formed versions of that key exist. The unversioned path only uses _loose when _versions is empty. Asking for a version that is not held can therefore hand a leftover malformed object to verification and record an integrity failure for a simple miss.

 

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