feat(client): Agent Skills — umbrella for the #50–#54 split

[XieX]

📋 This PR is now the umbrella — review the stack below

The change was split into five stacked PRs, small enough to actually read. This branch is deliberately untouched and will not be force-pushed, so every review comment below keeps its anchor. Review #50 first; each PR's diff is against the one before it.

#	PR	What it adds	+ / −
1	#50	Reference discovery — the types, config validation, frontmatter, skill_refs	+810 / −15
2	#51	Retrieval — the store seam, integrity verification, telemetry seam, accessors	+2,188 / −20
3	#52	safe_fs.py — descriptor-pinned filesystem primitives	+604
4	#53	Materialization — write_skills, the manifest, reconcile	+1,879 / −26
5	#54	The filesystem abuse matrix (tests only)	+1,057 / −7

main ← #50 ← #51 ← #52 ← #53 ← #54

The tip of the stack has the same tree as this branch, plus the changes for review findings 1, 2 and 3 and 22 new direct unit tests for safe_fs. make lint, make format-check, mypy packages/*/src and the full test suite are clean on every slice, not just the tip.

Where the review findings landed

Finding	Status	Where
1 — version pinning cannot be satisfied by the store seam (blocking)	✅ Resolved	#51
2 — contentHash mandatory here, optional on the wire	✅ Resolved (SDK half)	#51, one call site in #53
3 — SKILL_OBJECT_KIND may not match the delivery kind	✅ Resolved	#51
4 — write_skills is async but fully synchronous	⏸ Deferred	code in #53
5 — unavailability semantics differ between all_skills() and write_skills("*")	⏸ Deferred	code in #51 / #53
6 — nothing enforces one reconcile at a time	⏸ Deferred	code in #53
7 — Skill / SkillReference accept invalid values from direct callers	⏸ Deferred	code in #50
8 — silent omissions in get_skills	🟡 Partly addressed by finding 2's WARN summary	#51
9 — frontmatter() depends on a dev-only dependency	⏸ Deferred — a deliberate design decision, not an oversight	code in #50
10 — idiomatic / cleanliness items	⏸ Deferred	#50, #51, #53
11 — test gaps	🟡 The finding-1 gap is closed in #51; the rest deferred	#51, #54

Finding 1 — the fix

Version is now part of the lookup identity rather than a filter applied to the answer: SkillStore.get_object(kind, key, version=None), where None means "the newest you hold". The post-fetch equality check stays, but as a defense against an untrusted store answering with the wrong object rather than as the selection mechanism. InMemorySkillStore keys by (key, version) and holds both; all_objects returns one entry per (key, version) under keys documented opaque, with newest_by_key as the single place that collapses a whole-store read to one object per key.

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 only the seam change fails three of them, so they are not vacuous.

Finding 2 — the fix

contentHash stays mandatory; that default is right. What changed 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 — previously that returned an empty result indistinguishable from "this project has no skills".

Still open on the delivery side: whether the wire object needs a hashing-algorithm discriminator rather than a bare hex string. The SDK assumes SHA-256 lowercase hex. That is a wire-contract decision rather than an SDK one, so it is not resolved in the stack.

Finding 3 — the decision

SKILL_OBJECT_KIND keeps its value but is no longer exported from the package root. It is the string the 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. test_object_kind_is_not_public_api asserts it is absent from both __all__ and the package namespace, and it stays reachable through skills_core for the adapter that needs to agree with it.

This deliberately does not pick a kind/category string, so it needs no confirmation from the delivery side to land.

⚠️ Two cross-language items

Findings 1 and 3 both change a seam the TypeScript SDK mirrors. The strings are untouched, but the shape is not:

• get_object(kind, key, version) — the protocol needs the same third parameter in TypeScript.
• SKILL_OBJECT_KIND should be un-exported there too, for symmetry.

Also worth knowing

make typecheck (mypy ., including tests) fails on main already, with Duplicate module named "conftest" (packages/ai/tests vs packages/claude-agents/tests), which aborts before checking anything. Pre-existing and unrelated to this work; mypy packages/*/src is the gate every slice passes. Worth a separate fix with --explicit-package-bases or an __init__.py.

---

Original description

Adds support for Agent Skills: versioned SKILL.md documents managed in LaunchDarkly and attached to Config variations by reference. The SDK surfaces which skills a config references, retrieves their content, and materializes them onto disk where agent runtimes discover them.

Public API

Export	Description
skill_refs(config)	Project a config's skills array into list[SkillReference]. Pure — no client, store, or network.
get_skill(key, *, version=None)	One verified skill, or None. version=None means newest available.
get_skills(refs)	Batch form; accepts SkillReference values and bare key strings.
all_skills()	Every verified skill the store holds.
write_skills(skills, root, *, prune=True, timeout=10.0, on_unavailable="keep")	Materialize under root, returning a ReconcileReport.

New types: Skill, SkillReference, ReconcileAction, ReconcileReport, plus the ReconcileActionKind and OnUnavailable unions and the fixed on-disk / on-the-wire constants — all exported from the package root.

How content arrives

Content comes through an injectable SkillStore seam — get_object(kind, key), all_objects(kind), and an optional add_listener(kind, fn) — configured with init_client(options={"skillStore": store}). InMemorySkillStore ships for local development and testing.

The LaunchDarkly delivery transport is a follow-up release and drops in behind the same seam with no public API change.

Verification

Content is verified before it reaches caller code: key and version are revalidated, size is bounded, and the sha256 of the verbatim UTF-8 bytes must match the delivered contentHash. Anything that does not verify is withheld and treated as missing, so no unverified content is ever returned.

Materialization

write_skills writes <root>/<key>/SKILL.md and records what it owns in a manifest, so it overwrites or removes only files it wrote — a file you placed yourself is reported and left untouched. Writes are atomic, at mode 0644. Pruning removes formerly-managed skills that are no longer referenced, which is how revocation works. Every outcome is visible in the returned ReconcileReport (.actions, .ok, .errors).

write_skills performs synchronous filesystem I/O and does not yield — it is async for signature parity with the other accessors. Wrap it in asyncio.to_thread if that matters on your loop.

Notes for reviewers

• Skill.frontmatter() needs pyyaml, which is deliberately not a runtime dependency of this package. It returns None rather than raising when the library is absent or the block cannot be parsed.
• examples/skills_example.py runs the whole path end to end, fully offline — no SDK key, no API key, no sockets.
• This branch forked from an older main and is behind it; it needs a rebase before merge.

Testing

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

🤖 Generated with Claude Code

[devin-ai-integration]

Code review — Agent Skills (discovery, retrieval, materialization)

Reviewed against the internal SDK design page (SDK Design: Agent Skills Integration (Python/Node), PD/5215649997) and the delivery-side contract in AIC-2905. Overall this is unusually careful work: the filesystem layer's descriptor-pinned writes, manifest ownership model, prune suppression on incomplete retrieval, and telemetry redaction are all right, and the test suite (163 tests) covers the security surface well. Findings below are ordered by severity.

1. Version pinning cannot be satisfied by the store seam (spec inconsistency, blocking)

The spec: "Version selection is by pin (attached) or latest (standalone) … the payload contains every referenced skill in the project plus the newest version of each skill." AIC-2905 confirms delivery: the payload is the union of the latest version of every skill and every version currently referenced by any variation attachment. So multiple versions of the same key coexist in one payload.

But the store seam is keyed by key alone:

class SkillStore(Protocol):
    def get_object(self, kind: str, key: str) -> dict[str, Any] | None: ...

and resolve_from_store fetches one object, then compares versions after the fact:

raw = store.get_object(SKILL_OBJECT_KIND, key)
...
if wanted_version is not None and skill.version != wanted_version:
    return Resolution(error=...)

InMemorySkillStore likewise stores one object per (kind, key). Consequence: a variation pinned to an older version resolves the newest object and then rejects it — get_skill("k", version=1) returns None, get_skills([...]) silently omits it, and write_skills reports it as unresolvable. Pinning only works when the pin happens to equal the newest version, which is exactly the case the tests exercise (test_version_omitted_returns_newest_available and test_exact_version_match_returns_skill each put a single object).

Suggested fix: make version part of the lookup identity — either get_object(kind, key, version) / a get_skill_object(key, version | None) method on the protocol, or an agreed composite object key ("<key>:<version>" with a key-only alias for latest). Whatever is chosen needs to match the real FDv2 store adapter, and a test needs to hold two versions of one key simultaneously and assert both a pinned-old and a latest lookup succeed.

2. contentHash is mandatory here but optional on the wire

verify_raw_skill withholds any object without a string contentHash, and that is the correct default. But AIC-2905 records that ContentHash was dropped from delivery PR 2 and returns in PR 6 as an additive omitempty field, pending the security review's hashing-algorithm decision. Against a payload built before that lands, every skill fails verification and the whole feature silently returns nothing — the only signal is a log line plus an integrity signal. Worth (a) confirming the delivery/SDK sequencing so this can't ship in that order, and (b) making a total-withholding run louder than a per-object debug log (e.g. one WARN summarizing "N of M skills withheld").

Related: the SDK assumes SHA-256 hex lowercase (test_uppercase_hash_rejected) while the algorithm decision is still open server-side. If there is any chance of an algorithm change, the wire object needs an algorithm discriminator rather than a bare hex string.

3. SKILL_OBJECT_KIND = "skill" doesn't match the FDv2 kind

The constant is public API and documented as wire-shaped and "identical across language implementations", but the delivery decision in AIC-2905 is Kind: "inline-resource" + Category: "skill" (flag/segment keep bare Kind). Either the store adapter maps ("inline-resource", "skill") → "skill" (then this constant is an SDK-internal seam value and the docstring overstates it), or the constant is simply wrong. Please reconcile before the export is public, since it's hard to walk back.

4. write_skills is async but fully synchronous

Every path — hashing, os.stat, atomic_write, fsync, os.replace, manifest rewrite — runs inline on the calling event loop, and timeout is enforced only by time.monotonic() >= deadline checks between steps, so a single slow write (NFS, a full disk) cannot be interrupted. For a boot-time API this is mostly tolerable, but the signature promises otherwise. Options: keep the async signature and wrap the synchronous core in asyncio.to_thread, or expose a sync write_skills_sync and make the async one a thin wrapper. Either way, the docstring should state that timeout bounds progress between filesystem operations, not any individual one.

Spec note in the same area: §startup resilience says "the timeout bounds the whole call including retrieval; retries are bounded inside it", whereas the README states this layer performs no retries. One of the two should change.

5. Inconsistent unavailability semantics between all_skills() and write_skills("*")

async def all_skills() -> list[Skill]:
    objects, error = list_raw_objects(require_store())
    if error is not None:
        return []

write_skills("*") deliberately distinguishes "store raised" from "store is empty" and suppresses pruning; all_skills() collapses both into []. The natural composition await write_skills(await all_skills(), root) therefore prunes the entire managed root on a transport outage — precisely the data loss _resolve_all was written to prevent. Either propagate the failure from all_skills() (raise, or return a result type), or document the hazard prominently and steer callers to write_skills("*"). There is also no test for all_objects raising in all_skills(); every other error path here has one.

6. Nothing enforces "one reconcile at a time"

The module documents that concurrent reconciles against the same root can interleave and lose manifest entries, leaving files unmanaged, but nothing prevents it — and thread-wrapped calls are the natural workaround for finding #4. A module-level asyncio.Lock keyed by resolved root (plus, if cross-process matters, an O_EXCL lockfile inside the root) is cheap insurance. No test covers concurrent reconciles today.

7. Skill / SkillReference accept invalid values from direct callers

Both are frozen dataclasses with no __post_init__ validation, so Skill(key="../../etc", version=-1, content=..., content_hash="") constructs fine. The filesystem layer re-validates (good — test_hostile_key_is_rejected), but write_skills([Skill(...)]) bypasses hash verification entirely for caller-supplied skills, and accessors trust store-produced values only. Given Skill is exported and the docs show constructing it for local development, add __post_init__ validation (key grammar, version >= 1, hash shape, size) or verify caller-supplied skills on the write path.

8. Silent omissions in get_skills

Missing, version-mismatched, and integrity-failed references are all dropped from the returned list with no way for a caller to tell which, or how many. For an API whose failure mode is "the agent silently loses a capability", consider returning references alongside skills (or a companion errors list), and at minimum log at WARN rather than debug.

9. Skill.frontmatter() depends on a dev-only dependency

PyYAML is a dev dependency, import yaml is lazy, and every failure — including ImportError — degrades to None, which is indistinguishable from "this skill has no frontmatter". In a production install frontmatter() therefore always returns None, and test_returns_none_when_yaml_unavailable locks that in as intended behavior. Either add an optional extra (launchdarkly-ai-server[frontmatter]) and raise a clear error when it's absent, or hand-roll the tiny scalar-mapping parser the frontmatter contract actually needs and drop the dependency.

10. Non-idiomatic / cleanliness

• parse_block defines class _BoundedSafeLoader(yaml.SafeLoader) inside the function, so a new class object is built per call, and _depth is a class attribute mutated as if it were instance state (fine only because each call gets a fresh class — a latent trap if the class is ever hoisted naively). Hoist it to module scope with __init__-initialized instance state, behind a module-level lazy import.
• Skill.frontmatter() does a function-local import to dodge a cycle. A free function (parse_frontmatter(skill)) in frontmatter.py would keep types.py a pure data module and satisfy the "imports at the top" convention.
• Inconsistent argument-error types: bare string → TypeError in get_skills, ValueError in write_skills; and "*" is meaningful in one and not the other. Pick one convention.
• SKILL_KEY_GRAMMAR = "^[a-z0-9][a-z0-9-]*$" is exported while the compiled matcher uses \A…\Z. The public constant isn't the pattern actually enforced (^…$ admits a trailing newline); derive one from the other.
• _write_all catches only OSError, while store-facing code catches Exception. A non-OSError escaping the write loop (a custom Skill subclass, an encoding surprise) aborts the whole reconcile mid-run and skips the manifest rewrite, leaving disk and manifest divergent. Catch Exception there and turn it into an error action.
• ReconcileReport is frozen=True but holds a mutable list, and ok/errors rebuild a list on each access. tuple[ReconcileAction, ...] plus any(...) for ok is both cheaper and actually immutable. Similarly, ReconcileAction permits nonsense combinations (action="written" with error=...); a __post_init__ assertion or separate types would make the contract self-enforcing.
• Duplicate references ([SkillReference("a", 1), "a"], or two attachments pinning different versions of one key) are neither deduped nor rejected: config validation allows them, skill_refs returns them verbatim, get_skills fetches twice, and write_skills writes the same path twice, reporting two actions. Dedupe by key (and decide what a conflicting double-pin means) — today it's last-write-wins by list order.
• _parse_skills validates only key and version and ignores unknown fields in each entry. That's probably right for forward compatibility, but it should be a stated decision rather than an omission.

11. Test gaps

Coverage is strong; these are the holes I'd want closed:

• Two versions of the same key in the store, with a pinned-old lookup (finding feat: initial commit — LaunchDarkly AI SDK for Python #1) — currently unrepresentable, which is the point.
• all_objects raising under all_skills() (finding fix: set bootstrap-sha to pre-migration commit #5).
• Duplicate references across skill_refs, get_skills, write_skills.
• Concurrent write_skills against one root (finding chore: release main #6).
• A non-OSError raised inside the write loop, and a manifest-rewrite failure after successful writes (partial-state behavior isn't asserted anywhere).
• Frontmatter with CRLF line endings and with a leading UTF-8 BOM — both are realistic for SKILL.md authored on Windows, and both currently fall out as "no frontmatter".
• write_skills([Skill(...)]) with a caller-constructed skill whose content_hash doesn't match content (finding Revert "chore: release main" #7).

Merge confidence

Moderate-to-high on the filesystem and telemetry layers — I'd merge those largely as-is. Low on the retrieval layer until finding #1 is resolved, since version-pinned references are the primary use case and the current store seam cannot express them; findings #2 and #3 also need a decision recorded against the delivery side before the constants and required fields become public API.

[knfreemLD]

Sent off a Devin PR here mostly around correctness and idiomatic code; I think points 1 and 3 we should look to resolve here, and point 2 I believe is a quick enough fix. Everything else I believe is lower priority or quick fixes

[XieX]

Split this into five stacked PRs so each one is small enough to read. This branch is untouched and will not be force-pushed — the review above keeps every one of its anchors, and this PR becomes the umbrella.

main ← #50 ← #51 ← #52 ← #53 ← #54

#	PR	What it adds	+ / −
1	#50	Reference discovery — types, config validation, frontmatter, skill_refs	+810 / −15
2	#51	Retrieval — store seam, integrity verification, telemetry seam, accessors	+2,188 / −20
3	#52	safe_fs.py — descriptor-pinned filesystem primitives	+604
4	#53	Materialization — write_skills, manifest, reconcile	+1,879 / −26
5	#54	The filesystem abuse matrix (tests only)	+1,057 / −7

Per @knfreemLD's triage, findings 1, 2 and 3 are resolved in the stack; the rest are recorded against the slice whose code they touch.

Finding	Status	PR
1 — version pinning cannot be satisfied by the store seam (blocking)	✅ Resolved	#51
2 — contentHash mandatory here, optional on the wire	✅ Resolved (SDK half)	#51, one call site in #53
3 — SKILL_OBJECT_KIND doesn't match the delivery kind	✅ Resolved	#51
4 — write_skills async but fully synchronous	⏸ Deferred	code in #53
5 — all_skills() vs write_skills("*") unavailability semantics	⏸ Deferred	code in #51 / #53
6 — nothing enforces one reconcile at a time	⏸ Deferred	code in #53
7 — Skill / SkillReference accept invalid values from direct callers	⏸ Deferred	code in #50
8 — silent omissions in get_skills	🟡 Partly addressed by finding 2's WARN summary	#51
9 — frontmatter() on a dev-only dependency	⏸ Deferred — a deliberate design decision	code in #50
10 — idiomatic / cleanliness	⏸ Deferred	#50, #51, #53
11 — test gaps	🟡 The finding-1 gap is closed in #51	#51, #54

Finding 1. Version is now part of the lookup identity rather than a filter applied afterwards: get_object(kind, key, version=None), where None means "the newest you hold". The post-fetch equality check stays, but as a defense against an untrusted store answering with the wrong object rather than as the selection mechanism. InMemorySkillStore keys by (key, version) and holds both; all_objects returns one entry per (key, version) under keys documented opaque, and newest_by_key is the single place that collapses a whole-store read to one object per key — so write_skills("*") cannot write <root>/<key>/SKILL.md twice in one run. TestVersionPinning holds two versions of one key and asserts both a pinned-old and a latest lookup succeed; reverting only the seam change fails three of its tests, so they are not vacuous.

Finding 2. contentHash stays mandatory — that default is right. The signalling changed: a run that withheld anything logs one WARN naming the counts, and a run where nothing verified says so explicitly and names contentHash. Still open on your side: whether the wire object needs a hashing-algorithm discriminator rather than a bare hex string. That is a wire-contract decision, not an SDK one, so the stack doesn't decide it.

Finding 3. Took the first branch you offered: the constant keeps its value but is no longer exported from the package root, and its docstring now says what it actually is — the string the SDK hands a store, which an adapter maps ("inline-resource", "skill") onto. That drops the public commitment that was hard to walk back without locking in a string this side can't confirm, and it needs no decision from delivery to land. test_object_kind_is_not_public_api asserts it's absent from both __all__ and the package namespace.

⚠️ Both findings 1 and 3 change a seam the TypeScript SDK mirrors. The strings are untouched, but the shape is not: get_object needs the same third parameter there, and SKILL_OBJECT_KIND should be un-exported for symmetry.

One unrelated thing found on the way: make typecheck (mypy ., including tests) already fails on main with Duplicate module named "conftest" (packages/ai/tests vs packages/claude-agents/tests), which aborts before checking anything. Every slice passes mypy packages/*/src, the gate this PR's own commit message cites. Worth fixing separately.

[XieX]

Closing in favor of the split, see PR description for links to the stacked PRs.