From 27ef12f85faa2790ed6ecac8c23d78bd7a2304ad Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 25 Aug 2026 16:04:25 -0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(client):=20Agent=20Skills=20=E2=80=94?= =?UTF-8?q?=20retrieval=20through=20an=20injectable=20store?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/client/README.md | 61 +- packages/client/agents.md | 112 +- .../src/launchdarkly_ai_server/__init__.py | 10 + .../src/launchdarkly_ai_server/lifecycle.py | 42 +- .../src/launchdarkly_ai_server/skills.py | 242 +++- .../src/launchdarkly_ai_server/skills_core.py | 628 ++++++++++ packages/client/tests/conftest.py | 112 ++ packages/client/tests/test_skills.py | 1019 ++++++++++++++++- 8 files changed, 2203 insertions(+), 23 deletions(-) create mode 100644 packages/client/src/launchdarkly_ai_server/skills_core.py diff --git a/packages/client/README.md b/packages/client/README.md index fbd40045..f16a1eb2 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -223,22 +223,46 @@ asyncio.run(main()) ### Agent Skills Skills are versioned `SKILL.md` documents managed in LaunchDarkly and attached to AI Config -variations by reference. This release adds the first layer: discovering which skills a -resolved config references. Retrieving their content and materializing them onto disk follow. +variations by reference. The SDK surfaces which skills a config references and retrieves +their content. Materializing them onto disk, where agent runtimes discover them, follows. ```python import asyncio +import hashlib -from launchdarkly_ai_server import init_client, inspect_config, skill_refs +from launchdarkly_ai_server import ( + init_client, inspect_config, skill_refs, get_skill, get_skills, + InMemorySkillStore, +) + +SKILL_MD = "---\nname: PDF Extraction\n---\nExtract text from PDFs.\n" async def main(): - await init_client() + # A store supplies skill content. InMemorySkillStore is the dict-backed + # store for local development, testing, and bring-your-own-content use. + store = InMemorySkillStore() + store.put({ + "key": "pdf-extraction", + "version": 2, + "content": SKILL_MD, + # sha256, lowercase hex, over the verbatim utf-8 bytes. Content whose hash + # does not match is withheld, so this is not optional. + "contentHash": hashlib.sha256(SKILL_MD.encode("utf-8")).hexdigest(), + }) + await init_client(options={"skillStore": store}) + # 1. Which skills does this config reference? Pure projection — no I/O. info = await inspect_config("doc-agent", {"kind": "user", "key": "user-123"}) - refs = skill_refs(info["config"]) # [SkillReference(key='pdf-extraction', version=2)] + refs = skill_refs(info["config"]) # [SkillReference(key='pdf-extraction', version=2)] - for ref in refs: - print(ref.key, ref.version) + # 2. Fetch content. Returns None rather than raising when a skill is unavailable. + skill = await get_skill("pdf-extraction") + if skill is not None: + print(skill.content) + + # 3. Or resolve the config's references in one call. + for s in await get_skills(refs): + print(s.key, s.version) asyncio.run(main()) ``` @@ -249,9 +273,32 @@ integer ≥ 1): the whole variation is rejected, `inspect_config` returns `confi `extract_variation` raises. A variation that previously carried its own custom `skills` field of a different shape must rename it before upgrading. +**Integrity is not optional.** Content is only returned after its sha256 (lowercase hex, +over the verbatim UTF-8 bytes) matches the delivered `contentHash`, its key and version +revalidate, and its size is within 64 KiB. Anything that fails is withheld and treated as +missing — no unverified content ever reaches your code. A retrieval that withheld anything +logs a count at WARN, so a run that resolved nothing is not silent. + +**Versions are selected, not filtered.** A store may hold several versions of one key at +once, because a delivery payload does: the newest version of every skill, plus every +version a variation currently pins. `get_skill("k", version=1)` asks the store for version +1 and gets it even when a newer one is also held. + | Export | Description | |---|---| | `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when absent. | +| `get_skill(key, *, version=None)` | One verified skill, or `None`. `version=None` means newest available; a specific `version` matches exactly. Raises only when no store is configured. | +| `get_skills(refs)` | Batch form. Accepts `SkillReference` values and bare key strings (string = latest). Results follow input order; missing or unverifiable entries are omitted. | +| `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)` | A dict-backed store with `put(raw)`, for local development and testing. Holds several versions of a key. | + +Configure the store with `init_client(options={"skillStore": store})`. With none configured, +the accessors raise `RuntimeError` explaining what to do. `shutdown()` clears it. + +`all_objects` returns one entry per `(key, version)` under keys that are **opaque** to the +SDK — identity is read from each object's own `key` and `version` fields, so a store is free +to key its own map however the transport underneath does. > `Skill.content` is `bytes` — the verified verbatim bytes LaunchDarkly delivered, exactly > what was hashed. The SDK never parses or interprets them; if you want the frontmatter, diff --git a/packages/client/agents.md b/packages/client/agents.md index 8b55f173..f284c425 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -28,7 +28,8 @@ No other `launchdarkly-ai-*` package may define or duplicate these. They import | `src/launchdarkly_ai_server/graph.py` | `graph()`, `resolve_graph()`, `GraphInstance` | | `src/launchdarkly_ai_server/types.py` | All shared Python types — `AiConfigRep`, `ProviderHandler`, `LDContext`, `NativeTool`, etc. | | `src/launchdarkly_ai_server/types_validation.py` | `parse_ai_config` — validates flag variation shape; `is_valid_skill_key` / `is_valid_skill_version` / `skill_key_rejection_reason` (the canonical key-grammar explanation every layer quotes) | -| `src/launchdarkly_ai_server/skills.py` | Agent Skills — `skill_refs`, the projection of a config's `skills` array into typed references | +| `src/launchdarkly_ai_server/skills.py` | Agent Skills, retrieval half — `skill_refs`, `get_skill`/`get_skills`/`all_skills`, `InMemorySkillStore`, and the store/telemetry injection points `_set_store` / `_set_emitter_for_testing` | +| `src/launchdarkly_ai_server/skills_core.py` | Shared skills internals — the `SkillStore` seam, module state, the telemetry seam and its three recorders, integrity verification, and store resolution. Imported by both `skills.py` and the materialization layer; imports neither | | `src/launchdarkly_ai_server/utils.py` | `parse_template`, `parse_json_with_possible_fences`, `create_handler`, `parse_usage`, `make_track_data`, `to_ld_context` | | `src/launchdarkly_ai_server/registry.py` | `Registry`, `global_registry`, `compose`, `resolve_handlers`, `resolve_tools` | | `src/launchdarkly_ai_server/judges.py` | `run_judges`, `build_judge_tasks`, `run_judge` | @@ -73,9 +74,22 @@ from launchdarkly_ai_server import execute_and_track, execute_and_stream, wrap_t from launchdarkly_ai_server import config, graph, resolve_graph # Agent Skills -from launchdarkly_ai_server import skill_refs +from launchdarkly_ai_server import ( + skill_refs, get_skill, get_skills, all_skills, + SkillStore, InMemorySkillStore, +) ``` +`MAX_SKILL_CONTENT_BYTES` is deliberately *not* among them: it is a local enforcement +bound on content the platform produces, not a value this SDK defines, so exporting it +would semver-lock a number this side does not own. Keep it internal to `skills_core`. + +`SKILL_OBJECT_KIND` is not exported either, for a different reason: it is the string this +SDK hands a store, and a store 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 `launchdarkly_ai_server.skills_core`. + When adding a new export, add it to `__init__.py`'s imports and `__all__`. Handler packages must never import from sub-paths (e.g. `launchdarkly_ai_server.client`). --- @@ -170,20 +184,103 @@ Three layers, in increasing order of blast radius. Only the first is implemented typed `SkillReference` values. Pure: no network, no client, no store, no telemetry. Validation of the array itself lives in `parse_ai_config` and is **fail closed** — one malformed reference fails the whole config parse. -2. **Content accessors** — reading skill content through a store seam. +2. **Content accessors** — `get_skill`, `get_skills`, `all_skills` read through the + `SkillStore` seam. Configure a store with + `init_client(options={"skillStore": store})`; with none configured the accessors raise + an actionable `RuntimeError`. A delivery transport can be added behind the seam + without touching the public API. 3. **Materialization** — writing skills onto disk under a manifest. +### The store seam, and why version is part of the lookup + +`SkillStore` is `get_object(kind, key, version=None)`, `all_objects(kind)`, and an optional +`add_listener(kind, fn)`. Version is part of the **lookup identity**, not a filter applied +to the answer, and that is load-bearing: 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 the caller would then have to reject it — turning the primary use case, a +version-pinned attachment, into a missing skill. `version=None` asks for the newest held. + +The equality check in `resolve_from_store` stays, now as a **defense** rather than as the +selection mechanism: the store is untrusted, so an answer that is not the version asked for +is withheld. + +`all_objects` returns one entry per `(key, version)` under keys that are **opaque** to this +SDK. Do not parse them and do not assume one per skill key; identity is read off each +object's own `key` and `version`, which are revalidated anyway. `newest_by_key` is the +one place that collapses the result to one object per key, because both whole-store +consumers need it — `all_skills`, since a list holding two versions of one key is not a set +of skills, and the `"*"` reconcile, since `//SKILL.md` is a single path. + ### Security posture — do not relax any of this +Store data is **untrusted input**; the transport is not part of the trust boundary. + - **Skill content is an opaque byte buffer.** `Skill.content` is `bytes` — the verified - verbatim bytes LaunchDarkly delivered, exactly what was hashed. This SDK never parses, - decodes, or interprets them anywhere: not in the integrity path, not in an accessor, not - during materialization. Consumers who want frontmatter parse it themselves. + verbatim bytes, exactly what was hashed. The wire object delivers content as a JSON + string; the UTF-8 encode happens once, during verification, and from then on the SDK + never parses, decodes, or interprets the bytes anywhere: not in the integrity path, not + in an accessor, not during materialization. Consumers who want frontmatter parse it + themselves. +- **Integrity is mandatory and doubled, through one implementation.** Every raw object is + verified at the accessor boundary (key pattern and length, integer version >= 1, content + at most 64 KiB, sha256 lowercase hex over the verbatim bytes against `contentHash`) + and the hash is re-verified immediately before a write, both through + `skills_core.verified_bytes`, so the integrity signal's property set cannot depend on + which layer caught the defect. A `Skill` is only ever constructed from content that + passed. Nothing unverified reaches user code. +- **`contentHash` is required.** An object without one is withheld, not accepted on trust. + A payload built before the field is populated therefore yields nothing, which is why a + withholding run logs a run-level count at WARN — an empty result would otherwise be + indistinguishable from "this project has no skills". +- **No unencodable string ever reaches an encode.** `json.loads` turns a `\ud800` escape + into an unpaired surrogate with no UTF-8 representation; every `.encode("utf-8")` site + treats that as a verification failure. Never reach for `errors="surrogatepass"` — + fabricating bytes could satisfy the hash comparison. +- **Attacker-controlled strings are never echoed into telemetry.** `contentHash` and `key` + come off the wire, so a store could put the skill body in either; both are shape-checked + and redacted before they reach a signal or a log line. - **A key is untrusted input everywhere it appears.** `skill_key_rejection_reason` is the single canonical explanation, so the config parser and the reference projection reject a key for the same stated reason — and so does every layer added later. A silently shortened projection is not acceptable: every dropped entry is logged. +### Telemetry seam + +Skills telemetry goes through a private emitter with one method, +`record(signal, properties)`, whose default implementation is a **no-op** — nothing leaves +the process in this release. `client.track()` is deliberately *not* used: it needs an LD +context, spends the customer's event volume, lands in their data export, and is silenced by +offline mode. No LD context is involved anywhere in this feature. + +Exactly three signals exist, and the list is an **allowlist, not a floor**: + +| Signal | When | Properties | +|---|---|---| +| `AgentControl Skill Integrity Failure` | any hash/size/shape verification failure | `skill_key`, `version?`, `expected_hash?`, `observed_hash?`, `language` | +| `AgentControl Skill Materialized` | each `written` / `updated` / `skipped_current` | `skill_key`, `content_bytes`, `content_hash`, `reconcile_action`, `language` | +| `AgentControl Skill Revoked Received` | prune removes a formerly managed skill | `skill_key`, `version`, `removed_from_disk`, `language` | + +The last two belong to the materialization layer and have no caller yet; they live here +with the first so the allowlist is one section of one file rather than three sites to audit. + +`AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` +were considered and **deliberately excluded from SDK emission** — both are observable +server-side. Do not add them. The skill body never appears in a signal, a log line, or +an error message, and no signal carries a filesystem path. An emitter that raises is caught +and logged; it never fails the operation. + +Module state lives in `skills_core.py`, so there is exactly one store and one emitter +however the feature is entered. All three signals are emitted from the `record_*` functions +next to the seam there — nothing outside that module calls `emit`, so the allowlist is +enforced in one place. + +The injection path is deliberately narrower than the state's location: `skills.py` owns +`_set_store`, `_set_emitter_for_testing` and `_clear_state`, which delegate to +`skills_core`. `init_client` and `shutdown` use those, tests inject through those +(`skills._set_store(store)` is the same setter `init_client` uses), and neither should +reach into `skills_core` directly. + --- ## OTel Setup @@ -332,3 +429,6 @@ on their side of the boundary. - Do not weaken the `parse_ai_config` validation — handler packages rely on `config` being valid when they receive it. - `parse_usage` must continue to accept `input_tokens/output_tokens`, `inputTokens/outputTokens`, and `input/output` as all existing handlers return one of these variants. - `Skill.content` is opaque `bytes`. Do not add anything that parses or interprets it — no YAML library in this package's dependencies at any tier, and no accessor that decodes content. +- Do not route skills telemetry through `client.track()`, and do not introduce an LD context anywhere in the skills path. Signals go through the `skills_core.py` emitter seam, whose default is a no-op, and only via its `record_*` functions. +- Do not add a signal name outside the three in the Agent Skills table above — the list is an allowlist. `AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` were considered and deliberately excluded from SDK emission. +- Do not make `SkillStore` lookups key-only. Version is part of the lookup identity because a payload holds several versions of one key; a key-only seam cannot express a version-pinned reference. diff --git a/packages/client/src/launchdarkly_ai_server/__init__.py b/packages/client/src/launchdarkly_ai_server/__init__.py index 3faff304..02c858f2 100644 --- a/packages/client/src/launchdarkly_ai_server/__init__.py +++ b/packages/client/src/launchdarkly_ai_server/__init__.py @@ -38,8 +38,13 @@ resolve_tools, ) from .skills import ( + InMemorySkillStore, + all_skills, + get_skill, + get_skills, skill_refs, ) +from .skills_core import SkillStore from .tracking import execute_and_stream, execute_and_track, wrap_tool_handlers from .types import ( NATIVE_TOOL_KEY, @@ -208,4 +213,9 @@ "GraphInstance", # skills "skill_refs", + "get_skill", + "get_skills", + "all_skills", + "SkillStore", + "InMemorySkillStore", ] diff --git a/packages/client/src/launchdarkly_ai_server/lifecycle.py b/packages/client/src/launchdarkly_ai_server/lifecycle.py index 8969f2ea..28b7e81e 100644 --- a/packages/client/src/launchdarkly_ai_server/lifecycle.py +++ b/packages/client/src/launchdarkly_ai_server/lifecycle.py @@ -6,6 +6,7 @@ import os from typing import Any +from . import skills from .types import InitClientOptions logger = logging.getLogger(__name__) @@ -130,13 +131,43 @@ async def init_client( - Pass *client* directly (BYOC) to skip the LaunchDarkly Python SDK path. - Otherwise, reads ``LD_SDK_KEY`` from env or ``options['sdkKey']``. + - ``options['skillStore']`` configures the store the Agent Skills accessors + read from. Absent by default, in which case they raise an actionable error. + + This function is idempotent for the client singleton: a second call returns + the existing client without re-initializing, and every option is ignored — + **except** ``skillStore``, which is applied on every successful call. That + asymmetry is deliberate, and it is what lets a client that was lazily + auto-initialized, or initialized without a store, be given one afterwards. + A ``skillStore`` of ``None`` (or absent) never clears an already-configured + store; use ``shutdown()`` for that. The store is installed only once + initialization has succeeded: a call that raises leaves no global state + behind, so a failed init cannot leave the skill accessors working against a + store the application believes was never installed. Returns the initialized ``LDClientInterface`` instance. """ - global _client - opts = options or {} + ld_client = await _resolve_client(opts, client) + + # The single success point: every path that raises returns before here, so + # "installed only on success" is one statement rather than a copy per exit. + skill_store = opts.get("skillStore") + if skill_store is not None: + skills._set_store(skill_store) + return ld_client + + +async def _resolve_client(opts: InitClientOptions, client: Any) -> Any: + """ + Returns the singleton client, initializing it on first call. + + Split from ``init_client`` so that function has exactly one success point to + hang the ``skillStore`` carve-out on. + """ + global _client + # Idempotent — if already initialized, return the existing client if _client is not None: return _client @@ -198,12 +229,18 @@ async def shutdown() -> None: """ Shuts down the singleton client. Idempotent — safe to call multiple times even if the client was never initialized or already shut down. + + Also clears the configured skill store (and telemetry emitter): after a + shutdown, re-pass ``skillStore`` to the next ``init_client`` if the skill + accessors should keep working. """ global _client, _tracer_provider local_client = _client local_provider = _tracer_provider + skills._clear_state() + # Null the singleton before any awaits so a second call is a no-op _client = None _tracer_provider = None @@ -240,6 +277,7 @@ def _reset_for_testing() -> None: global _client, _tracer_provider _client = None _tracer_provider = None + skills._clear_state() async def inspect_config( diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index a3294bd2..eb3946c2 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -1,16 +1,43 @@ """ -Agent Skills — reference discovery. +Agent Skills — reference discovery and content accessors. -Projects the skill references a resolved AI Config carries into typed values. A -pure projection: no network, no client, no store, no telemetry. Retrieving the -content those references point at is a separate layer. +The public retrieval surface: projecting the skill references a resolved AI +Config carries, and retrieving skill content through an injectable store seam. + +The three layers of the feature sit in three modules, and the dependencies run +one way only: + +- ``skills_core.py`` — the store and telemetry seams, module state, integrity + verification, and store resolution. Shared, and imports neither of the others. +- ``skills.py`` (this file) — ``skill_refs``, the accessors, and + ``InMemorySkillStore``. +- ``skills_fs.py`` — the highest-blast-radius layer, the one that writes to a + customer's disk. It owns the manifest format and the on-disk filenames; + nothing here knows about the filesystem. + +``_set_store``, ``_set_emitter_for_testing`` and ``_clear_state`` live here +because this module is the documented injection path; the +state they mutate lives in ``skills_core``. """ from __future__ import annotations import logging +from collections.abc import Callable, Sequence +from typing import Any -from .types import AiConfigRep, SkillReference +from . import skills_core +from .skills_core import ( + SKILL_OBJECT_KIND, + list_raw_objects, + log_withholding_summary, + newest_by_key, + reference_target, + require_store, + resolve_from_store, + verify_raw_skill, +) +from .types import AiConfigRep, Skill, SkillReference from .types_validation import ( is_valid_skill_key, is_valid_skill_version, @@ -19,16 +46,140 @@ logger = logging.getLogger(__name__) +# --------------------------------------------------------------------------- +# Injection points +# --------------------------------------------------------------------------- +# +# These three names are the documented seam: ``init_client`` and ``shutdown`` +# call them, and tests inject through them. They delegate to ``skills_core``, +# which owns the state, so that there is exactly one store and one emitter no +# matter which layer reaches for them. + + +# Bound directly to the implementations rather than wrapped: a one-line +# delegation per name would give every state mutation two definitions and two +# docstrings to keep in agreement, which is the drift these names exist to +# avoid. ``_set_emitter_for_testing`` keeps its distinct name because it has no +# production caller. +_set_store = skills_core.set_store +_set_emitter_for_testing = skills_core.set_emitter +_clear_state = skills_core.clear_state + + +class InMemorySkillStore: + """ + A skill store backed by plain dicts. + + Ships for local development, tests, and bring-your-own-content injection. + Holds raw wire objects verbatim and performs no validation of its own — + verification belongs at the accessor boundary, where it applies to every + store equally. + + Several versions of one key coexist here, because they coexist in a real + delivery payload: the newest version of every skill, plus every version a + variation currently pins. ``get_object`` therefore selects on + ``(key, version)``, and ``version=None`` means "the newest held". + + An object whose ``version`` is not an integer >= 1 is still accepted and + still served, under its key alone. Withholding it is verification's job, not + the store's: a store that quietly refused it would make a malformed object + indistinguishable from an absent one, and no integrity signal would be + recorded. + """ + + def __init__(self, objects: dict[str, dict[str, Any]] | None = None) -> None: + self._versions: dict[str, dict[int, dict[str, Any]]] = {} + self._loose: dict[str, dict[str, Any]] = {} + self._listeners: dict[str, list[Callable[[dict[str, Any]], Any]]] = {} + for object_key, raw in (objects or {}).items(): + self._place(object_key, raw) + + def _place(self, fallback_key: str, raw: dict[str, Any]) -> None: + """Files one raw object under its own identity, verbatim.""" + key = raw.get("key") if isinstance(raw, dict) else None + if not isinstance(key, str): + key = fallback_key + version = raw.get("version") if isinstance(raw, dict) else None + if is_valid_skill_version(version): + self._versions.setdefault(key, {})[version] = raw + else: + self._loose[key] = raw + + def put(self, raw: dict[str, Any]) -> None: + """ + Adds or replaces a raw skill object, keyed by its own ``key`` and + ``version`` fields. + + Putting a second version of a key keeps both; putting the same + ``(key, version)`` twice replaces it. + + Notifies every skill-kind listener with the raw object as a single + positional argument. No validation happens here — verification belongs at + the accessor boundary, where it applies to every store equally — so a + listener sees exactly what was put, unverified. + """ + key = raw.get("key") + if not isinstance(key, str): + raise ValueError("a raw skill object must carry a string 'key'") + self._place(key, raw) + for listener in self._listeners.get(SKILL_OBJECT_KIND, []): + listener(raw) + + def get_object( + self, kind: str, key: str, version: int | None = None + ) -> dict[str, Any] | None: + if kind != SKILL_OBJECT_KIND: + return None + held = self._versions.get(key, {}) + if version is not None: + # Fall through to the version-less entry when the pin does not match + # anything well-formed, so a malformed object reaches verification and + # is withheld with a signal rather than reading as simply absent. + return held.get(version) or self._loose.get(key) + if held: + return held[max(held)] + return self._loose.get(key) + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + """ + Every object held, one entry per ``(key, version)``. + + The dict keys are opaque store-internal identifiers, as ``SkillStore`` + documents. Do not parse them and do not assume one entry per skill key. + """ + if kind != SKILL_OBJECT_KIND: + return {} + out: dict[str, dict[str, Any]] = { + f"{key}:{version}": raw + for key, versions in self._versions.items() + for version, raw in versions.items() + } + out.update(self._loose) + return out + + def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: + """ + Registers *fn* to be called with each raw object ``put`` under *kind*. + + Only ``kind == SKILL_OBJECT_KIND`` is ever notified, because ``put`` only + accepts skill objects; a listener registered under any other kind is + recorded and never fires. + """ + self._listeners.setdefault(kind, []).append(fn) + # --------------------------------------------------------------------------- # Reference discovery # --------------------------------------------------------------------------- + + def skill_refs(config: AiConfigRep | None) -> list[SkillReference]: """ Projects a resolved AI Config's ``skills`` array into typed references. A pure projection — no network, no client, no store, no telemetry. Returns - ``[]`` when the config carries no skills. + ``[]`` when the config carries no skills. Compose it with the accessors for + per-context resolution: ``await get_skills(skill_refs(config))``. A config that came through ``parse_ai_config`` never contains an invalid entry — parsing fails closed on one. A hand-built dict can, and a silently @@ -70,3 +221,82 @@ def skill_refs(config: AiConfigRep | None) -> list[SkillReference]: else: refs.append(SkillReference(key=key, version=version)) return refs + + +# --------------------------------------------------------------------------- +# Content accessors +# --------------------------------------------------------------------------- + + +async def get_skill(key: str, *, version: int | None = None) -> Skill | None: + """ + Retrieves one verified skill by key. + + ``version=None`` means the newest version the store holds; a specific + ``version`` asks the store for that version and returns it only when the + store answers with it. A payload holding several versions of one key + resolves a pin to the pinned version, not to the newest. + Returns ``None`` — never raises — when the skill is missing, the requested + version is not the one held, or verification fails. Raises ``RuntimeError`` + only when no skill store is configured. + + There is no context parameter: skills have no targeting, so the SDK + credentials fully determine availability. Compose per-context resolution + explicitly with ``get_skills(skill_refs(config))``. + """ + return resolve_from_store(require_store(), key, version).skill + + +async def get_skills(refs: Sequence[SkillReference | str]) -> list[Skill]: + """ + Retrieves a batch of verified skills. + + Accepts a mixed sequence of ``SkillReference`` values and bare key strings, + where a string means "the latest version". Results follow input order for + the skills that were found; entries that are missing, are the wrong version, + or fail verification are omitted rather than returned as placeholders — and + a run that omitted anything logs a count at WARN, so a batch that resolved + nothing is not silent. + """ + if isinstance(refs, str): + # str satisfies Sequence[str], so this type-checks; iterating it would + # silently look up one skill per character. + raise TypeError( + "get_skills takes a sequence of references; pass [key] rather than a " + f"bare string. Got {refs!r}." + ) + + store = require_store() + + requests = list(refs) + skills: list[Skill] = [] + for ref in requests: + key, wanted = reference_target(ref) + skill = resolve_from_store(store, key, wanted).skill + if skill is not None: + skills.append(skill) + log_withholding_summary("requested skills", len(requests), len(skills)) + return skills + + +async def all_skills() -> list[Skill]: + """ + Retrieves every verified skill the store currently holds. + + Skills that fail verification are omitted. Raises ``RuntimeError`` only when + no skill store is configured. + """ + objects, error = list_raw_objects(require_store()) + if error is not None: + return [] + + # One entry per key at its newest version: ``all_objects`` may hold several + # versions of one key, and a list carrying two of them is not a set of skills. + candidates = newest_by_key(objects) + skills: list[Skill] = [] + for _object_key, raw in candidates: + skill = verify_raw_skill(raw) + if skill is not None: + skills.append(skill) + log_withholding_summary("skills held by the store", len(candidates), len(skills)) + return skills diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py new file mode 100644 index 00000000..c478be93 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -0,0 +1,628 @@ +""" +Agent Skills — the internals ``skills`` and ``skills_fs`` both need. + +Extracted so the two layers above it share one implementation through an +explicit surface instead of reaching into each other's privates. Everything +here is package-internal — nothing in this module is exported from +``launchdarkly_ai_server`` except the two constants that are public API — and +the dependency runs one way: this module imports neither ``skills`` nor +``skills_fs``. + +What lives here, and why it has to be one copy: + +- **The store seam and the configured store.** One place holds the store, so + the accessors and the materialization path cannot disagree about whether one + is configured. +- **The telemetry seam.** Every signal the feature can emit is constructed by a + ``record_*`` function in this file and nowhere else, which is what makes the + three-signal allowlist enforceable by reading one section. ``emit`` is never + called from outside this module. +- **Integrity verification.** ``verified_bytes`` runs twice per skill by design + — once at the accessor boundary, and again immediately before a write, since a + ``Skill`` can also be constructed directly by a caller. Sharing the + implementation is what keeps the two passes from drifting — the signal's + property keys must match whichever layer caught the defect. +- **Store resolution.** ``resolve_from_store`` is the fetch-and-verify sequence + the accessors and the reconcile share, so its call sites cannot drift apart — + in particular on how a raising store is handled. + +Everything the store hands back is untrusted input; the transport is not part of +the trust boundary. Key, version, size, and content hash are revalidated here on +every pass. + +The store and emitter are injected through ``skills._set_store`` and +``skills._set_emitter_for_testing`` — those names are the documented seam, +and they delegate here. +""" + +from __future__ import annotations + +import hashlib +import logging +import re +from dataclasses import dataclass +from typing import Any, Protocol + +from .types import Skill, SkillReference +from .types_validation import is_valid_skill_key, is_valid_skill_version + +logger = logging.getLogger(__name__) + +SKILL_OBJECT_KIND = "skill" +""" +The kind this SDK asks a store for. + +An **internal seam value**, deliberately not exported from the package root. It +is the string ``skills.py`` and ``skills_fs.py`` pass to ``SkillStore.get_object`` +and ``SkillStore.all_objects``, and a store adapter is free to map it onto +whatever the transport underneath actually uses — a delivery payload may well +carry skills under a broader kind with a narrower category, in which case +translating that pair to this one value is the adapter's job. + +Exporting it would publish an SDK-side seam string as though it were the wire +contract, which is a claim this side cannot make and would be hard to walk back +once a caller depends on it. A store that needs to agree on a kind agrees with +whatever the SDK hands it, which is this constant reached through +``launchdarkly_ai_server.skills_core``. +""" + +MAX_SKILL_CONTENT_BYTES = 64 * 1024 +""" +Hard cap on skill content. Legitimately delivered skills are well under this +bound, so anything larger is withheld regardless of whether its hash checks out. + +Deliberately **not** exported from the package root, unlike the on-disk and +on-the-wire constants beside it. Those are values this SDK defines and a caller +may need to agree with; this one is a local enforcement bound on content the +platform produces, so publishing it would semver-lock a number this side does +not own — and a caller pre-flighting "will my skill fit?" against it would be +reading the client's guess rather than the real limit. The reason string from +``verified_bytes`` already reports the bound when it is what withheld content. +""" + +_LANGUAGE = "python" + +_SHA256_HEX = re.compile(r"\A[0-9a-f]{64}\Z") +"""What a legitimate content hash looks like. Anything else is redacted before +it reaches telemetry — ``contentHash`` is attacker-controlled, and a store that +put the skill body there would otherwise leak it into a signal.""" + +_SIGNAL_INTEGRITY_FAILURE = "AgentControl Skill Integrity Failure" +_SIGNAL_MATERIALIZED = "AgentControl Skill Materialized" +_SIGNAL_REVOKED = "AgentControl Skill Revoked Received" + +NO_STORE_MESSAGE = ( + "No skill store is configured, so skill content cannot be retrieved. Configure " + 'one with init_client(options={"skillStore": store}) — InMemorySkillStore is ' + "available for local development and testing." +) + + +# --------------------------------------------------------------------------- +# The store seam +# --------------------------------------------------------------------------- + + +class SkillStore(Protocol): + """ + Structural interface every source of skill content satisfies. + + Duck-typed on purpose, mirroring how the LaunchDarkly client interface works + in this package: pass any object carrying these methods. + + ``add_listener(kind, fn)`` is part of the seam but + **optional**, which is why it is deliberately not declared here: a Protocol + member is required for structural compatibility, so declaring it would reject + every store that does not implement it. Nothing in this module calls it — it + exists for the delivery transport to push updates through. + + The raw objects a store serves are wire-shaped, with camelCase field names + identical across language implementations:: + + {"key": "pdf-extraction", "version": 2, "content": "---\\n...", + "contentHash": "9f3a...", "name": "PDF Extraction", "description": "..."} + + **Version is part of the lookup identity, not a filter applied afterwards.** + A delivery payload holds the newest version of every skill *and* every + version any variation currently pins, so two versions of one key coexist + routinely. A seam keyed by key alone cannot express "the one this variation + pinned": it would answer with the newest and the caller would then have to + reject it, which turns a pinned reference into a missing skill. So + ``get_object`` takes the wanted version, and ``version=None`` means "the + newest you hold". + + ``all_objects`` returns one entry per *(key, version)* the store holds. Its + dict keys are **opaque store-internal identifiers** — do not parse them, and + do not assume one entry per skill key. Identity is read off each object's own + ``key`` and ``version`` fields, which are revalidated here anyway because + everything a store serves is untrusted. + """ + + def get_object( + self, kind: str, key: str, version: int | None = None + ) -> dict[str, Any] | None: ... + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: ... + + +# --------------------------------------------------------------------------- +# Telemetry seam +# --------------------------------------------------------------------------- + + +class _TelemetryEmitter(Protocol): + def record(self, signal: str, properties: dict[str, Any]) -> None: ... + + +class _NoOpEmitter: + """ + The default emitter. + + No skills telemetry leaves the process in this release: ``client.track()`` is + the wrong channel (it needs an LD context, spends the customer's event + volume, and lands in their data export), and the diagnostic-event channel + that would be right has no wrapper-SDK extension point yet. Signals are + recorded through this seam so the eventual transport drops in behind it + without touching a single call site. + """ + + def record(self, signal: str, properties: dict[str, Any]) -> None: + return None + + +_NOOP_EMITTER: _TelemetryEmitter = _NoOpEmitter() + +# --------------------------------------------------------------------------- +# Module state +# --------------------------------------------------------------------------- + +_store: SkillStore | None = None +_emitter: _TelemetryEmitter = _NOOP_EMITTER +"""Never ``None``: "no emitter installed" is spelled as the no-op, so ``emit`` +has one code path instead of re-deciding on every signal.""" + + +def set_store(store: Any) -> None: + """ + Replaces the configured store. + + Reached through ``skills._set_store``, which is the documented seam; see that + function for who calls it and why it has no test-only twin. + """ + global _store + _store = store + + +def set_emitter(emitter: Any) -> None: + """Replaces the telemetry emitter. Reached through + ``skills._set_emitter_for_testing``.""" + global _emitter + _emitter = emitter + + +def clear_state() -> None: + """Drops both the store and the emitter. Reached through ``skills._clear_state``.""" + global _store, _emitter + _store = None + _emitter = _NOOP_EMITTER + + +def get_store() -> SkillStore | None: + """The configured store, or ``None``. The only reader of the global.""" + return _store + + +def require_store() -> SkillStore: + store = get_store() + if store is None: + raise RuntimeError(NO_STORE_MESSAGE) + return store + + +def emit(signal: str, properties: dict[str, Any]) -> None: + """ + Records one signal. Never raises into the calling operation — a broken + emitter must not be able to fail a retrieval or a reconcile. + """ + try: + _emitter.record(signal, properties) + except Exception: + logger.warning("Skills telemetry emitter raised; ignoring", exc_info=True) + + +def record_integrity_failure( + skill_key: str, + reason: str, + *, + version: Any = None, + expected_hash: Any = None, + observed_hash: str | None = None, +) -> None: + """ + Records an integrity failure. Carries hashes and byte counts only — the skill + body never appears in a signal, a log line, or an error message. + """ + # Both of these come off the wire, so neither may be echoed verbatim: a store + # that set contentHash (or key) to the skill body would otherwise publish the + # body itself. Shape-check, then redact. + safe_key = skill_key if is_valid_skill_key(skill_key) else "" + properties: dict[str, Any] = {"skill_key": safe_key, "language": _LANGUAGE} + if is_valid_skill_version(version): + properties["version"] = version + if isinstance(expected_hash, str): + properties["expected_hash"] = ( + expected_hash + if _SHA256_HEX.match(expected_hash) + else "" + ) + if observed_hash is not None: + properties["observed_hash"] = observed_hash + + logger.error("Skill '%s' failed integrity verification: %s", safe_key, reason) + emit(_SIGNAL_INTEGRITY_FAILURE, properties) + + +def record_materialized( + skill_key: str, content_bytes: int, content_hash: str, reconcile_action: str +) -> None: + """ + Records a materialization. Deliberately carries no ``target_path`` and no + filesystem path of any kind — the same reasoning that keeps the skill body + out of telemetry keeps the customer's directory layout out. Paths live in the + returned ``ReconcileReport``, which is user-facing API rather than telemetry. + """ + emit( + _SIGNAL_MATERIALIZED, + { + "skill_key": skill_key, + "content_bytes": content_bytes, + "content_hash": content_hash, + "reconcile_action": reconcile_action, + "language": _LANGUAGE, + }, + ) + + +def record_revoked(skill_key: str, version: Any) -> None: + """ + Records a revocation — a prune that removed a formerly managed skill. + + Lives here with the other two recorders rather than at the prune site so the + signal allowlist is maintained in one place: every signal this SDK can emit + is visible in this section of this module, and nothing outside it touches + ``emit``. + """ + # Both fields come off the manifest, which is untrusted — same rule as + # ``record_integrity_failure``: shape-check, then redact, so a hand-edited + # manifest cannot plant an arbitrary string in a signal. + safe_key = skill_key if is_valid_skill_key(skill_key) else "" + properties: dict[str, Any] = { + "skill_key": safe_key, + "removed_from_disk": True, + "language": _LANGUAGE, + } + if is_valid_skill_version(version): + properties["version"] = version + emit(_SIGNAL_REVOKED, properties) + + +# --------------------------------------------------------------------------- +# Integrity verification +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class VerifiedContent: + """Content that passed integrity verification.""" + + encoded: bytes + """The verbatim bytes, exactly as hashed.""" + content_hash: str + """The locally computed sha256 — never the caller's expected value.""" + + +@dataclass(frozen=True) +class VerificationFailure: + """Why content did not pass. The reason is safe to show a caller.""" + + reason: str + + +def verified_bytes( + key: str, content: str | bytes, expected_hash: str, version: int +) -> VerifiedContent | VerificationFailure: + """ + The whole content half of integrity verification: encode, size, hash. + + Accepts either shape content legitimately arrives in. Wire-shaped ``str`` + input — a raw store object's JSON string — is UTF-8 encoded here, once, and + this is the only place that encode happens. ``bytes`` input is an already + verified ``Skill.content`` being re-verified, and is hashed directly: those + bytes are the verbatim value, so re-encoding does not apply. + + Returns the verbatim bytes and their locally computed sha256, or a + human-readable reason — having already recorded the integrity signal, so the + signal's property set cannot depend on which caller noticed. The hash handed + back is the one computed here, never the caller's expected value: the two are + equal on this path by construction, and returning the locally derived one + keeps an attacker-supplied string out of ``Skill``. + + The two outcomes are distinct types rather than a ``tuple | str`` union so a + call site reads as "verification failed" instead of "the result is a string", + and so a future success payload carrying a ``str`` cannot silently invert the + discrimination. + + This runs twice per skill by design: once at the accessor boundary, and again + immediately before a write, because a ``Skill`` can also be constructed + directly by a caller. Sharing the implementation is what keeps those two + passes from drifting — the property keys must match. + + The second pass re-hashes bytes the first pass already hashed. That + redundancy is deliberate: it is negligible next to the write it guards, and + carrying the first pass's verdict forward would put a "trust the value + computed upstream" branch inside the one function whose entire job is not to. + """ + if isinstance(content, bytes): + encoded = content + else: + try: + encoded = content.encode("utf-8") + except UnicodeEncodeError: + # json.loads turns a "\ud800" escape into an unpaired surrogate, which + # has no UTF-8 encoding. There are no bytes the server could have + # hashed, so this is not authentic content. Never use + # errors="surrogatepass" here: that would fabricate bytes and could + # satisfy the hash comparison. + reason = "content is not encodable as UTF-8" + record_integrity_failure( + key, reason, version=version, expected_hash=expected_hash + ) + return VerificationFailure(reason) + + if len(encoded) > MAX_SKILL_CONTENT_BYTES: + reason = ( + f"content is {len(encoded)} bytes, over the " + f"{MAX_SKILL_CONTENT_BYTES} byte cap" + ) + record_integrity_failure( + key, reason, version=version, expected_hash=expected_hash + ) + return VerificationFailure(reason) + + # sha256, lowercase hex, over the verbatim bytes — no canonicalization and + # no content parsing of any kind anywhere in the integrity path. + observed_hash = hashlib.sha256(encoded).hexdigest() + if observed_hash != expected_hash: + record_integrity_failure( + key, + "content hash mismatch", + version=version, + expected_hash=expected_hash, + observed_hash=observed_hash, + ) + return VerificationFailure("content hash mismatch") + + return VerifiedContent(encoded=encoded, content_hash=observed_hash) + + +def verify_raw_skill(raw: Any) -> Skill | None: + """ + Turns one untrusted raw store object into a ``Skill``, or withholds it. + + On any failure the skill is treated as missing, the integrity signal is + recorded, and an error is logged. No unverified content is ever returned to + user code. + """ + if not isinstance(raw, dict): + record_integrity_failure("", "raw skill object is not an object") + return None + + key = raw.get("key") + if not is_valid_skill_key(key): + record_integrity_failure( + key if isinstance(key, str) else "", + "key is not a valid skill key", + ) + return None + + version = raw.get("version") + if not is_valid_skill_version(version): + record_integrity_failure(key, "version is not an integer >= 1") + return None + + content = raw.get("content") + if not isinstance(content, str): + record_integrity_failure( + key, "content is missing or not a string", version=version + ) + return None + + expected_hash = raw.get("contentHash") + if not isinstance(expected_hash, str): + record_integrity_failure( + key, "contentHash is missing or not a string", version=version + ) + return None + + verified = verified_bytes(key, content, expected_hash, version) + if isinstance(verified, VerificationFailure): + return None + + name = raw.get("name") + description = raw.get("description") + return Skill( + key=key, + version=version, + content=verified.encoded, + content_hash=verified.content_hash, + name=name if isinstance(name, str) else None, + description=description if isinstance(description, str) else None, + ) + + +def log_withholding_summary(subject: str, requested: int, resolved: int) -> None: + """ + One WARN per run when content was withheld, naming the counts. + + Every individual withholding already records an integrity signal and an error + log line, but a caller reading logs at WARN sees neither. That matters most in + the case where *nothing* verified — a payload built before ``contentHash`` is + populated, say — because the feature then returns an empty result that is + indistinguishable from "this project has no skills". A run-level summary is + the difference between a silent no-op and a visible one. + + Called once per batch retrieval, not once per skill, so a large withholding + run does not itself become the noise. + """ + withheld = requested - resolved + if withheld <= 0: + return + if resolved == 0: + logger.warning( + "All %d %s were withheld and no skill content is available. Every " + "object failed verification — check that the delivered objects carry " + "a contentHash matching the sha256 of their content.", + requested, + subject, + ) + return + logger.warning( + "%d of %d %s were withheld and are unavailable; see the preceding errors " + "for the per-skill reason.", + withheld, + requested, + subject, + ) + + +def store_raised(exc: Exception) -> str: + """The one wording for "the store could not answer", used by every path.""" + return f"the skill store raised {type(exc).__name__}: {exc}" + + +def list_raw_objects( + store: SkillStore, +) -> tuple[dict[str, dict[str, Any]], str | None]: + """ + Every raw object the store holds, or the reason it could not answer. + + One entry per *(key, version)*, under keys that are opaque to this SDK — see + ``SkillStore``. Callers that need one skill per key have to collapse the + result themselves; ``newest_by_key`` does it. + + Returns the reason rather than raising, because both callers need the + distinction between "no skills" and "the store is broken" — and they need it + worded identically. Letting the exception out instead would make each of + them re-derive the log line and the message, which is the drift this module + exists to prevent. + """ + try: + objects = store.all_objects(SKILL_OBJECT_KIND) + except Exception as exc: + logger.error("Skill store raised while listing skills", exc_info=True) + return {}, store_raised(exc) + return (objects if isinstance(objects, dict) else {}), None + + +def newest_by_key(objects: dict[str, dict[str, Any]]) -> list[tuple[str, Any]]: + """ + One raw object per skill key — the highest version of each, paired with the + store key it was served under. + + ``all_objects`` may hold several versions of one key, and both callers that + consume the whole store want one skill per key: ``all_skills`` because a list + holding two versions of one key is not a set of skills, and the ``"*"`` + reconcile because ``//SKILL.md`` is a single path and writing it + twice in one run is a bug rather than a policy. + + The store key is carried through rather than discarded because the reconcile + attributes a failure to it when the object's own key is unusable. + + Objects too malformed to carry a usable key and version are **kept**, not + dropped, so verification is what withholds them: a silently dropped object + falls out of the requested set, and prune would then delete the last + known-good copy already on disk. + """ + best: dict[str, tuple[str, Any]] = {} + unusable: list[tuple[str, Any]] = [] + for object_key, raw in objects.items(): + skill_key = raw.get("key") if isinstance(raw, dict) else None + version = raw.get("version") if isinstance(raw, dict) else None + if not is_valid_skill_key(skill_key) or not is_valid_skill_version(version): + unusable.append((object_key, raw)) + continue + held = best.get(skill_key) + if held is None or version > held[1]["version"]: + best[skill_key] = (object_key, raw) + return list(best.values()) + unusable + + +# --------------------------------------------------------------------------- +# Resolution internals — shared with the materialization path +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class Resolution: + """One key resolved against a store: the skill, or why there is none.""" + + skill: Skill | None = None + error: str | None = None + unavailable: bool = False + """ + ``True`` when the *store* could not answer — it raised — rather than when it + answered "no". Only the former suppresses pruning: deleting managed files + because a lookup failed would turn an outage into data loss. + """ + + +def resolve_from_store( + store: SkillStore, key: str, wanted_version: int | None +) -> Resolution: + """ + Fetches one key and verifies it — the sequence the accessors and the + materialization path share. + + Written once on purpose, so the call sites cannot drift apart — in + particular on the policy for a raising store. + + ``wanted_version`` goes *into* the lookup, because a store may hold several + versions of one key and only it can pick between them; ``None`` asks for the + newest. The equality check afterwards is kept as a **defense**, not as the + selection mechanism: the store is untrusted, so an answer that is not the + version that was asked for is withheld rather than returned. + """ + try: + raw = store.get_object(SKILL_OBJECT_KIND, key, wanted_version) + except Exception as exc: + logger.error("Skill store raised while retrieving '%s'", key, exc_info=True) + return Resolution(error=store_raised(exc), unavailable=True) + + if not isinstance(raw, dict): + return Resolution( + error=f"skill '{key}' is not available from the configured skill store" + ) + + skill = verify_raw_skill(raw) + if skill is None: + return Resolution( + error=f"skill '{key}' failed integrity verification and was withheld" + ) + if wanted_version is not None and skill.version != wanted_version: + return Resolution( + error=( + f"skill '{key}' version {wanted_version} is not available " + f"(the store holds version {skill.version})" + ) + ) + return Resolution(skill=skill) + + +def reference_target(item: SkillReference | str) -> tuple[str, int | None]: + """Normalises a reference-or-key into ``(key, wanted version)``. + + A bare string means "the latest version the store holds". + """ + if isinstance(item, str): + return item, None + return item.key, item.version diff --git a/packages/client/tests/conftest.py b/packages/client/tests/conftest.py index 6c140200..17fbb68b 100644 --- a/packages/client/tests/conftest.py +++ b/packages/client/tests/conftest.py @@ -1,7 +1,14 @@ +import hashlib +from collections.abc import Iterator +from typing import Any from unittest.mock import AsyncMock, MagicMock import pytest +import launchdarkly_ai_server.lifecycle as lifecycle_module +import launchdarkly_ai_server.skills as skills_module +from launchdarkly_ai_server import InMemorySkillStore + @pytest.fixture def mock_ld_client() -> MagicMock: @@ -36,3 +43,108 @@ def mock_tracer(mock_span: MagicMock) -> MagicMock: tracer.start_as_current_span.return_value.__exit__ = MagicMock(return_value=False) tracer.start_span.return_value = mock_span return tracer + + +# --------------------------------------------------------------------------- +# Agent Skills helpers +# +# Exposed as fixtures rather than importable module-level helpers: pytest runs +# with --import-mode=importlib and the tests directory is not a package, so +# sibling imports from conftest are not reliable. +# --------------------------------------------------------------------------- + +SKILL_BODY = "---\nname: Test Skill\n---\nDo the thing.\n" + + +class _RecordingEmitter: + """Telemetry seam double — records (signal, properties) pairs.""" + + def __init__(self) -> None: + self.records: list[tuple[str, dict[str, Any]]] = [] + + def record(self, signal: str, properties: dict[str, Any]) -> None: + self.records.append((signal, properties)) + + def signals(self, name: str) -> list[dict[str, Any]]: + return [props for sig, props in self.records if sig == name] + + +class _ThrowingEmitter: + """Telemetry seam double whose record() always raises.""" + + def record(self, signal: str, properties: dict[str, Any]) -> None: + raise RuntimeError("emitter exploded") + + +@pytest.fixture +def make_raw_skill() -> Any: + """Factory for wire-shaped raw store objects with a correct contentHash.""" + + def _make( + key: str = "test-skill", + version: int = 1, + content: str = SKILL_BODY, + **overrides: Any, + ) -> dict[str, Any]: + obj: dict[str, Any] = { + "key": key, + "version": version, + "content": content, + "contentHash": hashlib.sha256(content.encode("utf-8")).hexdigest(), + "name": "Test Skill", + "description": "A skill used in tests.", + } + obj.update(overrides) + return obj + + return _make + + +@pytest.fixture +def store() -> InMemorySkillStore: + """An in-memory store, wired in as the configured store for the test.""" + s = InMemorySkillStore() + skills_module._set_store(s) + return s + + +class _ExplodingStore: + """Store double whose every read raises — the "transport is down" case.""" + + def get_object(self, kind: str, key: str) -> dict[str, Any] | None: + raise RuntimeError("transport failure") + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + raise RuntimeError("transport failure") + + +@pytest.fixture +def exploding_store() -> _ExplodingStore: + """A raising store, wired in as the configured store for the test.""" + s = _ExplodingStore() + skills_module._set_store(s) + return s + + +@pytest.fixture +def reset_skill_state() -> Iterator[None]: + """Clears client, store, and emitter module state around one test. + + Opted into per module with ``pytestmark = pytest.mark.usefixtures(...)`` + rather than being autouse here: autouse would newly reset lifecycle state + for every test in every module in this directory, which is a behaviour change + well outside the skills tests. + """ + lifecycle_module._reset_for_testing() + yield + lifecycle_module._reset_for_testing() + + +@pytest.fixture +def recording_emitter() -> _RecordingEmitter: + return _RecordingEmitter() + + +@pytest.fixture +def throwing_emitter() -> _ThrowingEmitter: + return _ThrowingEmitter() diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 38ca34f2..e99a8957 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -1,24 +1,57 @@ """ -Agent Skills — reference discovery: the types and the projection from a -resolved AI Config. +Tests for Agent Skills types, reference discovery, content accessors, +integrity verification, and the telemetry seam. """ from __future__ import annotations import dataclasses import hashlib +import json from typing import Any +from unittest.mock import MagicMock import pytest +import launchdarkly_ai_server.lifecycle as lifecycle_module +import launchdarkly_ai_server.skills as skills_module from launchdarkly_ai_server import ( + InMemorySkillStore, Skill, SkillReference, + all_skills, + get_client, + get_skill, + get_skills, + init_client, + shutdown, skill_refs, ) SKILL_BODY = "---\nname: Test Skill\n---\nDo the thing.\n" +INTEGRITY_SIGNAL = "AgentControl Skill Integrity Failure" +MATERIALIZED_SIGNAL = "AgentControl Skill Materialized" +REVOKED_SIGNAL = "AgentControl Skill Revoked Received" + +# The three signal names are an allowlist, not a floor: any +# other name reaching the emitter is a regression. +APPROVED_SIGNALS = frozenset({INTEGRITY_SIGNAL, MATERIALIZED_SIGNAL, REVOKED_SIGNAL}) + +# These two were considered and deliberately excluded from SDK emission — +# named explicitly rather than relying on the subset check to be read as +# covering them. +REMOVED_SIGNALS = frozenset( + { + "AgentControl Skill SDK Reference Returned", + "AgentControl Skill Content Retrieved", + } +) + + +pytestmark = pytest.mark.usefixtures("reset_skill_state") +"""Every test in this module runs against freshly cleared module state.""" + def _hash(content: str) -> str: """sha256, lowercase hex, over verbatim utf-8 bytes.""" @@ -40,6 +73,61 @@ def _skill( ) +# --------------------------------------------------------------------------- + +UNENCODABLE_BODIES = ( + json.loads(r'"hi \ud800 there"'), # lone high surrogate + # A lone *low* surrogate, which is the one range errors="surrogateescape" + # smuggles through (as a raw 0x80 byte) while raising on everything else. + json.loads(r'"hi \udc80 there"'), +) + +NON_STRICT_HANDLERS = ( + "surrogatepass", + "surrogateescape", + "replace", + "ignore", + "backslashreplace", + "xmlcharrefreplace", + "namereplace", +) +"""Every ``str.encode`` error handler that is not ``strict``. + +``verified_bytes`` must use none of them: each one *fabricates* bytes for input +that has no encoding, and fabricated bytes can satisfy the hash comparison. +""" + + +def _fabricated_hash_cases() -> list[Any]: + """One case per (body, handler) pair the handler can actually encode. + + Each carries the sha256 of the bytes *that* handler would have produced, so + the case is not vacuous: an implementation that reached for the handler + would encode successfully, match the pinned hash, and return content + LaunchDarkly never delivered. Handlers that raise on a given body are + skipped — for that input they are as strict as ``strict``, so there is + nothing to detect. + """ + cases: list[Any] = [] + for index, body in enumerate(UNENCODABLE_BODIES): + for handler in NON_STRICT_HANDLERS: + try: + fabricated = body.encode("utf-8", errors=handler) + except UnicodeEncodeError: + continue + cases.append( + pytest.param( + body, + hashlib.sha256(fabricated).hexdigest(), + id=f"body{index}-{handler}", + ) + ) + return cases + + +FABRICATED_HASH_CASES = _fabricated_hash_cases() + + class TestSkillTypes: """Immutability and optional metadata.""" @@ -106,6 +194,11 @@ def test_returns_typed_references_in_order(self) -> None: ] assert all(isinstance(r, SkillReference) for r in refs) + def test_emits_no_telemetry(self, recording_emitter: Any) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + skill_refs(self._config(skills=[{"key": "a", "version": 1}])) + assert recording_emitter.records == [] + def test_dropped_entries_are_logged(self, caplog: pytest.LogCaptureFixture) -> None: """A shortened projection is never silent. @@ -137,3 +230,925 @@ def test_requires_no_client_or_store(self, mock_ld_client: Any) -> None: refs = skill_refs(self._config(skills=[{"key": "a", "version": 2}])) assert refs == [SkillReference(key="a", version=2)] mock_ld_client.track.assert_not_called() + + +class TestPackageExports: + """ + What is and is not part of the public surface. + + The literal values are spelled out on purpose: this is the one place the + constants themselves are asserted, so importing them to build the + expectation would make the assertion circular. + """ + + def test_content_cap_is_not_public_api(self) -> None: + """The content cap stays internal to ``skills_core`` — see the + ``MAX_SKILL_CONTENT_BYTES`` docstring there for why it is not exported.""" + import launchdarkly_ai_server as package + from launchdarkly_ai_server import skills_core + + assert skills_core.MAX_SKILL_CONTENT_BYTES == 65536 + assert "MAX_SKILL_CONTENT_BYTES" not in package.__all__ + assert not hasattr(package, "MAX_SKILL_CONTENT_BYTES") + + def test_object_kind_is_not_public_api(self) -> None: + """The kind is an SDK-side seam value, not the wire contract. + + A store adapter maps whatever the transport calls a skill onto the value + this SDK passes it, so publishing the string would advertise a contract + this side does not own — and one that would be hard to walk back once a + caller depended on it. It stays reachable through ``skills_core`` for the + adapter that needs to agree with it. + """ + import launchdarkly_ai_server as package + from launchdarkly_ai_server import skills_core + + assert skills_core.SKILL_OBJECT_KIND == "skill" + assert "SKILL_OBJECT_KIND" not in package.__all__ + assert not hasattr(package, "SKILL_OBJECT_KIND") + + def test_retrieval_surface_is_exported_from_the_package_root(self) -> None: + """A name absent from ``__all__`` is not part of the public surface.""" + import launchdarkly_ai_server as package + + expected = { + "skill_refs", + "get_skill", + "get_skills", + "all_skills", + "SkillStore", + "InMemorySkillStore", + "Skill", + "SkillReference", + } + assert expected <= set(package.__all__) + + +class TestInMemorySkillStore: + """The public in-memory store implementation.""" + + def test_get_object_round_trips(self, make_raw_skill: Any) -> None: + raw = make_raw_skill(key="pdf-extraction", version=2) + s = InMemorySkillStore({"pdf-extraction": raw}) + assert s.get_object("skill", "pdf-extraction") == raw + + def test_get_object_unknown_key_returns_none(self) -> None: + assert InMemorySkillStore().get_object("skill", "nope") is None + + def test_put_then_get(self, make_raw_skill: Any) -> None: + s = InMemorySkillStore() + raw = make_raw_skill(key="a") + s.put(raw) + assert s.get_object("skill", "a") == raw + + def test_all_objects_returns_everything(self, make_raw_skill: Any) -> None: + """Asserted on the object bodies, not the dict keys. + + ``all_objects`` keys are opaque store-internal identifiers — the seam + documents them as such — so a test that pinned their spelling would be + asserting an implementation detail the contract disclaims. + """ + s = InMemorySkillStore() + first = make_raw_skill(key="a") + second = make_raw_skill(key="b") + s.put(first) + s.put(second) + held = s.all_objects("skill").values() + assert len(held) == 2 + assert first in held + assert second in held + + def test_all_objects_holds_every_version_of_one_key( + self, make_raw_skill: Any + ) -> None: + s = InMemorySkillStore() + v1 = make_raw_skill(key="a", version=1, content="one\n") + v2 = make_raw_skill(key="a", version=2, content="two\n") + s.put(v1) + s.put(v2) + held = list(s.all_objects("skill").values()) + assert len(held) == 2 + assert v1 in held + assert v2 in held + + def test_put_replaces_only_the_same_key_and_version( + self, make_raw_skill: Any + ) -> None: + s = InMemorySkillStore() + s.put(make_raw_skill(key="a", version=1, content="first\n")) + replacement = make_raw_skill(key="a", version=1, content="second\n") + s.put(replacement) + assert list(s.all_objects("skill").values()) == [replacement] + + def test_get_object_with_a_version_selects_that_version( + self, make_raw_skill: Any + ) -> None: + s = InMemorySkillStore() + v1 = make_raw_skill(key="a", version=1, content="one\n") + v2 = make_raw_skill(key="a", version=3, content="three\n") + s.put(v1) + s.put(v2) + assert s.get_object("skill", "a", 1) == v1 + assert s.get_object("skill", "a", 3) == v2 + + def test_get_object_without_a_version_selects_the_newest( + self, make_raw_skill: Any + ) -> None: + s = InMemorySkillStore() + s.put(make_raw_skill(key="a", version=1, content="one\n")) + newest = make_raw_skill(key="a", version=7, content="seven\n") + s.put(newest) + s.put(make_raw_skill(key="a", version=4, content="four\n")) + assert s.get_object("skill", "a") == newest + + def test_get_object_unknown_version_falls_back_to_a_malformed_object( + self, make_raw_skill: Any + ) -> None: + """A malformed object must reach verification, not read as absent. + + An object whose ``version`` is unusable is filed under its key alone. A + pinned lookup that finds nothing well-formed serves it anyway, so + verification withholds it and records an integrity signal — a store that + returned ``None`` here would make tampering indistinguishable from a + skill that was never delivered. + """ + s = InMemorySkillStore() + malformed = make_raw_skill(key="a", version="two") + s.put(malformed) + assert s.get_object("skill", "a", 2) == malformed + assert s.get_object("skill", "a") == malformed + + def test_all_objects_unknown_kind_is_empty(self, make_raw_skill: Any) -> None: + s = InMemorySkillStore() + s.put(make_raw_skill(key="a")) + assert s.all_objects("flag") == {} + + def test_put_notifies_skill_kind_listeners(self, make_raw_skill: Any) -> None: + """``add_listener`` is part of the seam, so its one + implementation carries a smoke test for the callback contract: the raw + object, verbatim and unverified, as a single positional argument.""" + s = InMemorySkillStore() + seen: list[dict[str, Any]] = [] + s.add_listener("skill", seen.append) + raw = make_raw_skill(key="a") + + s.put(raw) + + assert seen == [raw] + + def test_put_does_not_notify_other_kind_listeners( + self, make_raw_skill: Any + ) -> None: + s = InMemorySkillStore() + seen: list[dict[str, Any]] = [] + s.add_listener("flag", seen.append) + + s.put(make_raw_skill(key="a")) + + assert seen == [] + + +class TestStoreConfiguration: + """Store wiring on the lifecycle layer.""" + + async def test_configured_via_init_client_option( + self, make_raw_skill: Any, mock_ld_client: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + await init_client(options={"skillStore": store}, client=mock_ld_client) + skill = await get_skill("a") + assert skill is not None + assert skill.key == "a" + + async def test_get_skill_raises_actionably_when_no_store(self) -> None: + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + async def test_get_skills_raises_actionably_when_no_store(self) -> None: + with pytest.raises(RuntimeError, match="skill store"): + await get_skills([SkillReference(key="a", version=1)]) + + async def test_all_skills_raises_actionably_when_no_store(self) -> None: + with pytest.raises(RuntimeError, match="skill store"): + await all_skills() + + async def test_shutdown_clears_the_store( + self, make_raw_skill: Any, mock_ld_client: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + await init_client(options={"skillStore": store}, client=mock_ld_client) + assert await get_skill("a") is not None + + await shutdown() + + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + async def test_skill_store_is_applied_on_every_init_client_call( + self, make_raw_skill: Any + ) -> None: + """``skillStore`` is the one option a second call applies. + + ``init_client`` is idempotent for the client singleton, and on a second + call every other option is ignored. ``skillStore`` is applied anyway, + on purpose: it is what lets a client that was lazily auto-initialized, + or initialized without a store, be given one afterwards. Both halves are + asserted on the same pair of calls, because each is meaningless without + the other. + """ + first_store = InMemorySkillStore() + first_store.put(make_raw_skill(key="first")) + second_store = InMemorySkillStore() + second_store.put(make_raw_skill(key="second")) + + first_client = MagicMock() + second_client = MagicMock() + + await init_client(options={"skillStore": first_store}, client=first_client) + await init_client(options={"skillStore": second_store}, client=second_client) + + # Half one: the client singleton is unchanged — the second call is a + # no-op for it, so the second client was discarded. + assert get_client() is first_client + + # Half two: the store was nevertheless swapped. + assert await get_skill("second") is not None + assert await get_skill("first") is None + + async def test_init_client_without_a_store_leaves_the_configured_one( + self, make_raw_skill: Any + ) -> None: + """Only a non-None ``skillStore`` replaces the configured store. + + Otherwise a bare ``init_client()`` from an unrelated code path — the + lazy auto-init, say — would silently unconfigure skills. + """ + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + await init_client(options={"skillStore": store}, client=MagicMock()) + + await init_client(client=MagicMock()) + + assert await get_skill("a") is not None + + async def test_failed_init_client_leaves_no_store_configured( + self, monkeypatch: pytest.MonkeyPatch, make_raw_skill: Any + ) -> None: + """A raising ``init_client`` must not leave global state behind. + + Installing the store before the SDK-key check would leave the accessors + working against a store the application believes was never installed, + masking a failed initialization. + """ + monkeypatch.delenv("LD_SDK_KEY", raising=False) + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + + with pytest.raises(RuntimeError, match="No LaunchDarkly SDK key"): + await init_client(options={"skillStore": store}) + + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + async def test_reset_for_testing_clears_the_store( + self, make_raw_skill: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + skills_module._set_store(store) + assert await get_skill("a") is not None + + lifecycle_module._reset_for_testing() + + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + +class TestAccessorArgumentErrors: + """A bare string is a type error, not a reference. + + ``str`` satisfies ``Sequence[str]``, so the annotation on ``get_skills`` + admits a bare string and only this runtime guard catches it; iterating one + would look up a skill per character. Deliberately a *different* class from + ``write_skills``'s bare-string rejection, which is a ``ValueError`` because + a string is an accepted argument type there. + """ + + async def test_bare_string_raises_type_error( + self, store: InMemorySkillStore + ) -> None: + with pytest.raises(TypeError) as excinfo: + await get_skills("pdf-extraction") # type: ignore[arg-type] + + # The message has to name the fix, not merely reject the input. + assert "[key]" in str(excinfo.value) + + async def test_bare_string_is_rejected_before_the_store_is_consulted( + self, make_raw_skill: Any + ) -> None: + """The guard is an argument check, so it precedes store resolution. + + Asserting the raise alone would also pass if the string were iterated + into single-character lookups that all missed, so pin that no lookup + happened at all. + """ + looked_up: list[str] = [] + + class _RecordingStore: + def get_object(self, kind: str, key: str) -> dict[str, Any] | None: + looked_up.append(key) + return None + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + return {} + + skills_module._set_store(_RecordingStore()) + + with pytest.raises(TypeError): + await get_skills("abc") # type: ignore[arg-type] + + assert looked_up == [] + + +class TestGetSkill: + """Single-skill accessor.""" + + async def test_returns_verified_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="pdf-extraction", version=2)) + skill = await get_skill("pdf-extraction") + assert skill is not None + assert skill.key == "pdf-extraction" + assert skill.version == 2 + assert skill.content == SKILL_BODY.encode("utf-8") + assert skill.content_hash == _hash(SKILL_BODY) + assert skill.name == "Test Skill" + assert skill.description == "A skill used in tests." + + async def test_version_omitted_returns_newest_available( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=7)) + skill = await get_skill("a") + assert skill is not None + assert skill.version == 7 + + async def test_exact_version_match_returns_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=3)) + skill = await get_skill("a", version=3) + assert skill is not None + assert skill.version == 3 + + async def test_other_version_returns_none( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=3)) + assert await get_skill("a", version=2) is None + assert await get_skill("a", version=4) is None + + async def test_missing_key_returns_none(self, store: InMemorySkillStore) -> None: + assert await get_skill("nope") is None + + async def test_multibyte_content_verifies( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + content = "---\nname: emoji\n---\n🚀 unicode ✅ body\n" + store.put(make_raw_skill(key="a", content=content)) + skill = await get_skill("a") + assert skill is not None + assert skill.content == content.encode("utf-8") + + +class TestGetSkills: + """Batch accessor.""" + + async def test_mixed_refs_and_strings( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=1)) + store.put(make_raw_skill(key="b", version=5)) + result = await get_skills([SkillReference(key="a", version=1), "b"]) + assert [s.key for s in result] == ["a", "b"] + + async def test_preserves_input_order( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + for k in ("a", "b", "c"): + store.put(make_raw_skill(key=k)) + result = await get_skills(["c", "a", "b"]) + assert [s.key for s in result] == ["c", "a", "b"] + + async def test_missing_entries_are_omitted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a")) + result = await get_skills(["a", "missing"]) + assert [s.key for s in result] == ["a"] + + async def test_version_mismatch_is_omitted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=2)) + result = await get_skills([SkillReference(key="a", version=1)]) + assert result == [] + + async def test_empty_input_returns_empty_list( + self, store: InMemorySkillStore + ) -> None: + assert await get_skills([]) == [] + + async def test_integrity_failure_omitted_and_signalled( + self, + store: InMemorySkillStore, + make_raw_skill: Any, + recording_emitter: Any, + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="good-a")) + store.put(make_raw_skill(key="bad", contentHash="0" * 64)) + store.put(make_raw_skill(key="good-b")) + + result = await get_skills(["good-a", "bad", "good-b"]) + + assert [s.key for s in result] == ["good-a", "good-b"] + failures = recording_emitter.signals(INTEGRITY_SIGNAL) + assert len(failures) == 1 + assert failures[0]["skill_key"] == "bad" + + +class TestAllSkills: + """All_skills accessor.""" + + async def test_returns_every_verified_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + for k in ("a", "b", "c"): + store.put(make_raw_skill(key=k)) + result = await all_skills() + assert {s.key for s in result} == {"a", "b", "c"} + + async def test_empty_store_returns_empty_list( + self, store: InMemorySkillStore + ) -> None: + assert await all_skills() == [] + + async def test_omits_skills_that_fail_verification( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="good")) + store.put(make_raw_skill(key="bad", contentHash="deadbeef")) + result = await all_skills() + assert {s.key for s in result} == {"good"} + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + +class TestVersionPinning: + """ + A payload holds several versions of one key, and a pin has to resolve to the + version it names. + + Delivery serves the newest version of every skill *plus* every version any + variation currently pins, so this is the ordinary case rather than an edge + one. A seam keyed by key alone cannot express it: it answers with the newest + and the pin then reads as a missing skill. + """ + + async def _two_versions(self, store: Any, make_raw_skill: Any) -> None: + store.put(make_raw_skill(key="a", version=1, content="version one\n")) + store.put(make_raw_skill(key="a", version=2, content="version two\n")) + + async def test_pinned_old_version_resolves( + self, store: Any, make_raw_skill: Any + ) -> None: + await self._two_versions(store, make_raw_skill) + skill = await get_skill("a", version=1) + assert skill is not None + assert skill.version == 1 + assert skill.content == b"version one\n" + + async def test_latest_resolves_alongside_the_pinned_old_version( + self, store: Any, make_raw_skill: Any + ) -> None: + await self._two_versions(store, make_raw_skill) + skill = await get_skill("a") + assert skill is not None + assert skill.version == 2 + + async def test_both_lookups_succeed_against_one_store( + self, store: Any, make_raw_skill: Any + ) -> None: + await self._two_versions(store, make_raw_skill) + pinned = await get_skill("a", version=1) + latest = await get_skill("a", version=2) + assert pinned is not None and pinned.version == 1 + assert latest is not None and latest.version == 2 + + async def test_get_skills_resolves_a_mix_of_pins( + self, store: Any, make_raw_skill: Any + ) -> None: + await self._two_versions(store, make_raw_skill) + store.put(make_raw_skill(key="b", version=5, content="bee\n")) + skills = await get_skills( + [ + SkillReference(key="a", version=1), + SkillReference(key="b", version=5), + "a", + ] + ) + assert [(s.key, s.version) for s in skills] == [("a", 1), ("b", 5), ("a", 2)] + + async def test_pin_to_a_version_the_store_does_not_hold_returns_none( + self, store: Any, make_raw_skill: Any + ) -> None: + await self._two_versions(store, make_raw_skill) + assert await get_skill("a", version=9) is None + + async def test_all_skills_returns_one_entry_per_key_at_the_newest_version( + self, store: Any, make_raw_skill: Any + ) -> None: + await self._two_versions(store, make_raw_skill) + store.put(make_raw_skill(key="b", version=5, content="bee\n")) + skills = await all_skills() + assert sorted((s.key, s.version) for s in skills) == [("a", 2), ("b", 5)] + + async def test_a_store_answering_with_the_wrong_version_is_withheld( + self, make_raw_skill: Any + ) -> None: + """The post-fetch check is a defense, not the selection mechanism. + + The store is untrusted, so an answer that is not the version that was + asked for is withheld rather than returned. + """ + + class _WrongVersionStore: + def get_object( + self, kind: str, key: str, version: int | None = None + ) -> Any: + return make_raw_skill(key=key, version=99) + + def all_objects(self, kind: str) -> dict[str, Any]: + return {} + + skills_module._set_store(_WrongVersionStore()) + assert await get_skill("a", version=1) is None + + +class TestWithholdingSummary: + """ + A run that withheld content says so at WARN, once. + + Every individual withholding already records an integrity signal and an + error line, but a caller reading logs at WARN sees neither — and the case + that matters most is a payload where *nothing* verifies, because the feature + then returns an empty result indistinguishable from "this project has no + skills". + """ + + def _tampered(self, make_raw_skill: Any, key: str = "a") -> dict[str, Any]: + raw = make_raw_skill(key=key) + raw["contentHash"] = "0" * 64 + return raw + + async def test_total_withholding_warns_and_names_the_hash( + self, store: Any, make_raw_skill: Any, caplog: pytest.LogCaptureFixture + ) -> None: + store.put(self._tampered(make_raw_skill)) + with caplog.at_level("WARNING", logger="launchdarkly_ai_server.skills_core"): + assert await all_skills() == [] + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + assert "contentHash" in warnings[0].getMessage() + + async def test_partial_withholding_warns_with_the_counts( + self, store: Any, make_raw_skill: Any, caplog: pytest.LogCaptureFixture + ) -> None: + store.put(make_raw_skill(key="good")) + store.put(self._tampered(make_raw_skill, key="bad")) + with caplog.at_level("WARNING", logger="launchdarkly_ai_server.skills_core"): + skills = await all_skills() + assert [s.key for s in skills] == ["good"] + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + assert "1 of 2" in warnings[0].getMessage() + + async def test_get_skills_warns_once_per_batch( + self, store: Any, make_raw_skill: Any, caplog: pytest.LogCaptureFixture + ) -> None: + store.put(self._tampered(make_raw_skill, key="a")) + store.put(self._tampered(make_raw_skill, key="b")) + with caplog.at_level("WARNING", logger="launchdarkly_ai_server.skills_core"): + assert await get_skills(["a", "b"]) == [] + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + + async def test_a_fully_resolved_run_is_silent( + self, store: Any, make_raw_skill: Any, caplog: pytest.LogCaptureFixture + ) -> None: + store.put(make_raw_skill(key="a")) + with caplog.at_level("WARNING", logger="launchdarkly_ai_server.skills_core"): + assert len(await all_skills()) == 1 + assert len(await get_skills(["a"])) == 1 + assert [r for r in caplog.records if r.levelname == "WARNING"] == [] + + +class TestIntegrityVerification: + """Mandatory verification at the accessor boundary.""" + + async def test_hash_mismatch_withholds_skill( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="a", contentHash="a" * 64)) + assert await get_skill("a") is None + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_tampered_content_withholds_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + raw = make_raw_skill(key="a") + raw["content"] = raw["content"] + "x" # hash now stale by one byte + store.put(raw) + assert await get_skill("a") is None + + async def test_oversize_content_rejected( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + oversize = "x" * (64 * 1024 + 1) + store.put(make_raw_skill(key="a", content=oversize)) + assert await get_skill("a") is None + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_content_at_size_cap_is_accepted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + at_cap = "x" * (64 * 1024) + store.put(make_raw_skill(key="a", content=at_cap)) + skill = await get_skill("a") + assert skill is not None + assert len(skill.content) == 64 * 1024 + + async def test_key_at_length_bound_from_store_accepted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + """The accepting side of the <= 256 bound. + + ``write_skills`` cannot reach this bound (a key is one directory name and + NAME_MAX is 255), so config validation and this accessor-side + revalidation are the only two layers where 256 is observable at all. The + rejecting side is ``test_invalid_key_from_store_rejected["x" * 257]``. + """ + key = "a" * 256 + store.put(make_raw_skill(key=key)) + skill = await get_skill(key) + assert skill is not None + assert skill.key == key + + @pytest.mark.parametrize( + "bad_key", + [ + "Evil", + "-leading-dash", + ".hidden", + "has space", + "a/b", + "../escape", + "", + "x" * 257, + ], + ) + async def test_invalid_key_from_store_rejected( + self, make_raw_skill: Any, bad_key: str + ) -> None: + """A hostile store may serve any key — the accessor revalidates.""" + raw = make_raw_skill(key="placeholder") + raw["key"] = bad_key + skills_module._set_store(InMemorySkillStore({bad_key: raw})) + assert await get_skill(bad_key) is None + + @pytest.mark.parametrize("bad_version", [0, -1, 2.5, "2", None, True]) + async def test_invalid_version_from_store_rejected( + self, make_raw_skill: Any, bad_version: Any + ) -> None: + raw = make_raw_skill(key="a", version=bad_version) + skills_module._set_store(InMemorySkillStore({"a": raw})) + assert await get_skill("a") is None + + async def test_missing_content_rejected(self, make_raw_skill: Any) -> None: + raw = make_raw_skill(key="a") + del raw["content"] + skills_module._set_store(InMemorySkillStore({"a": raw})) + assert await get_skill("a") is None + + async def test_missing_content_hash_rejected(self, make_raw_skill: Any) -> None: + raw = make_raw_skill(key="a") + del raw["contentHash"] + skills_module._set_store(InMemorySkillStore({"a": raw})) + assert await get_skill("a") is None + + async def test_uppercase_hash_rejected( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + """Hashes are lowercase hex; a non-canonical hash is not authentic.""" + store.put(make_raw_skill(key="a", contentHash=_hash(SKILL_BODY).upper())) + assert await get_skill("a") is None + + @pytest.mark.parametrize(("body", "fabricated_hash"), FABRICATED_HASH_CASES) + async def test_unencodable_content_withheld( + self, recording_emitter: Any, body: str, fabricated_hash: str + ) -> None: + """Content with no UTF-8 encoding is withheld, and the signal recorded. + + ``str.encode`` raises on a lone surrogate, so ``verified_bytes`` has an + exception to catch and never sees bytes for this content at all. + + The parametrization is what makes that observable. The guard must never + pass ``errors="surrogatepass"``, or any other non-strict handler: each + of them fabricates bytes for input that has no encoding, and fabricated + bytes can satisfy the hash comparison. Every case here supplies the + sha256 of the bytes one such handler would have produced, so an + implementation that reached for one would verify this object + successfully and hand back content LaunchDarkly never sent. An + arbitrary wrong hash would not catch that — the mismatch check would + reject the input before the encoder guard was reached. + """ + with pytest.raises(UnicodeEncodeError): + body.encode("utf-8") # the premise: there is no encoding to hash + + skills_module._set_emitter_for_testing(recording_emitter) + skills_module._set_store( + InMemorySkillStore( + { + "a": { + "key": "a", + "version": 1, + "content": body, + "contentHash": fabricated_hash, + } + } + ) + ) + + assert await get_skill("a") is None + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_store_error_does_not_leak_content( + self, exploding_store: Any + ) -> None: + assert await get_skill("a") is None + assert await get_skills(["a"]) == [] + assert await all_skills() == [] + + +class TestTelemetrySeam: + """Internal emitter seam, no client.track, no context.""" + + async def test_default_emitter_is_noop( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", contentHash="0" * 64)) + assert await get_skill("a") is None # no emitter injected, no raise + + async def test_integrity_signal_properties( + self, + store: InMemorySkillStore, + make_raw_skill: Any, + recording_emitter: Any, + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="a", version=4, contentHash="b" * 64)) + + await get_skill("a") + + props = recording_emitter.signals(INTEGRITY_SIGNAL)[0] + assert props["skill_key"] == "a" + assert props["version"] == 4 + assert props["expected_hash"] == "b" * 64 + assert props["observed_hash"] == _hash(SKILL_BODY) + assert props["language"] == "python" + + async def test_skill_body_never_appears_in_signals( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="a", contentHash="c" * 64)) + + await get_skill("a") + + for _signal, props in recording_emitter.records: + for value in props.values(): + assert "Do the thing." not in str(value) + + # ``skill_key`` and ``expected_hash`` are copied off the wire, so a hostile + # store can smuggle the body through either one and publish it in a signal + # that is otherwise body-free. The sweep above cannot see that: it serves a + # well-formed 64-character digest under a valid key, so neither replacement + # branch ever runs, and it passes even against an implementation that copies + # both fields verbatim. These two cases are what make the rule observable. + # Both assert the body's *absence* rather than the placeholder's exact + # spelling, which is not part of the contract. + + async def test_body_smuggled_through_content_hash_is_redacted( + self, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + body = "UNIQUE-SECRET-BODY-VIA-HASH" + skills_module._set_store( + InMemorySkillStore( + {"a": {"key": "a", "version": 1, "content": body, "contentHash": body}} + ) + ) + + assert await get_skill("a") is None + + signals = recording_emitter.signals(INTEGRITY_SIGNAL) + assert len(signals) == 1 + for value in signals[0].values(): + assert body not in str(value) + + async def test_body_smuggled_through_the_key_is_redacted( + self, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + # Uppercase and a path separator, so this is not a valid skill key and + # the invalid-key branch is the one that has to redact it. + body = "UNIQUE-SECRET-BODY-VIA-KEY/../x" + skills_module._set_store( + InMemorySkillStore( + {body: {"key": body, "version": 1, "content": "x", "contentHash": "y"}} + ) + ) + + assert await get_skill(body) is None + + signals = recording_emitter.signals(INTEGRITY_SIGNAL) + assert len(signals) == 1 + for value in signals[0].values(): + assert body not in str(value) + + async def test_no_ld_track_calls_from_accessors( + self, make_raw_skill: Any, mock_ld_client: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + store.put(make_raw_skill(key="bad", contentHash="0" * 64)) + await init_client(options={"skillStore": store}, client=mock_ld_client) + + await get_skill("a") + await get_skill("bad") + await get_skills(["a"]) + await all_skills() + + mock_ld_client.track.assert_not_called() + + async def test_throwing_emitter_never_breaks_the_operation( + self, store: InMemorySkillStore, make_raw_skill: Any, throwing_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(throwing_emitter) + store.put(make_raw_skill(key="bad", contentHash="0" * 64)) + store.put(make_raw_skill(key="good")) + + assert await get_skill("bad") is None + good = await get_skill("good") + assert good is not None + assert good.key == "good" + + async def test_accessors_record_no_signal_outside_the_approved_set( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + """The three names are an allowlist, not a floor. + + Asserted over the recorded strings, so nothing here mandates a + particular module-level constant. The write-side half of this sweep is + ``test_write_skills_records_no_signal_outside_the_approved_set`` in + test_skills_fs.py, where all four reconcile actions can be staged. + + Guards the most likely regression: an implementation that also emits + ``AgentControl Skill Content Retrieved`` from ``get_skill``, or + ``AgentControl Skill SDK Reference Returned`` from ``skill_refs``, + passes every other test in this class. + """ + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="good")) + store.put(make_raw_skill(key="tampered", contentHash="0" * 64)) + + assert await get_skill("good") is not None + assert await get_skill("tampered") is None + await get_skills(["good", "tampered"]) + await all_skills() + skill_refs({"skills": [{"key": "good", "version": 1}]}) + + recorded = {signal for signal, _props in recording_emitter.records} + assert recorded <= APPROVED_SIGNALS, ( + f"unapproved signal(s): {sorted(recorded - APPROVED_SIGNALS)}" + ) + assert not recorded & REMOVED_SIGNALS + # Positive control: a subset assertion is satisfied vacuously by an + # implementation that records nothing at all. + assert INTEGRITY_SIGNAL in recorded From 1afb690e0048e824849166cbe7f4ebe793fa2ddc Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 28 Aug 2026 16:11:51 -0400 Subject: [PATCH 2/2] feat(client): a documented log record for skill integrity failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/client/README.md | 51 ++++ packages/client/agents.md | 54 ++++ .../src/launchdarkly_ai_server/skills_core.py | 128 ++++++++- packages/client/tests/test_skills.py | 269 ++++++++++++++++++ 4 files changed, 491 insertions(+), 11 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index f16a1eb2..ae572b3b 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -279,6 +279,57 @@ revalidate, and its size is within 64 KiB. Anything that fails is withheld and t missing — no unverified content ever reaches your code. A retrieval that withheld anything logs a count at WARN, so a run that resolved nothing is not silent. +#### Detecting integrity failures + +Every withheld skill emits one structured **ERROR** log record on the SDK's own logger +(`launchdarkly_ai_server.skills_core`), designed to be ingested by a SIEM and alerted on. +It is emitted **regardless of how telemetry is configured** — it is not conditional on any +opt-in, and it is the detection path that works when nothing leaves your process. + +The message text is the stable event name followed by compact JSON, so it is greppable and +`jq`-able under any handler configuration, and the same mapping is attached as +`extra["ld_skills"]` for a structured handler: + +``` +ERROR ld.skills.integrity_failure {"action":"withheld","event":"ld.skills.integrity_failure","expected_hash":"0000…0000","language":"python","observed_hash":"5fc8…6ec0","reason":"content hash mismatch","reason_code":"hash_mismatch","skill_key":"pdf-extraction","version":2} +``` + +**`ld.skills.integrity_failure` is a stability commitment.** It is the string to match on, +it will not be renamed, and the JSON keys are sorted so the line is byte-identical across +LaunchDarkly's AI SDKs for the same input. + +| Field | Description | +|---|---| +| `event` | Always `ld.skills.integrity_failure`. | +| `action` | Always `withheld` — the content was not returned to your code. | +| `skill_key` | The skill key, or `` when the delivered key was itself malformed. | +| `version` | The delivered version. Omitted when it was not a valid version. | +| `expected_hash` | The delivered `contentHash`, or `` when it was not one. Omitted when none was delivered. | +| `observed_hash` | The sha256 the SDK computed. Omitted when the failure happened before anything was hashed. | +| `reason_code` | A stable token naming the failure mode — see below. | +| `reason` | Human-readable detail, including byte counts where relevant. | +| `language` | Always `python`. | + +Absent optional fields are **omitted entirely** rather than emitted as `null`, so a field +existence check is meaningful. The skill body, and any attacker-controllable string that +could carry it, never appears in the record; neither does any filesystem path. + +| `reason_code` | Meaning | +|---|---| +| `not_an_object` | The delivered object was not a JSON object. | +| `invalid_key` | The key did not match `^[a-z0-9][a-z0-9-]*$` or exceeded 256 characters. | +| `invalid_version` | The version was not an integer ≥ 1. | +| `missing_content` | `content` was absent or not a string. | +| `missing_content_hash` | `contentHash` was absent or not a string. | +| `not_utf8` | The content string had no UTF-8 encoding, so there are no bytes that could have been hashed. | +| `over_size_cap` | The content exceeded the SDK's local size cap. | +| `hash_mismatch` | The computed sha256 did not match the delivered `contentHash`. | + +**`hash_mismatch` is the one worth paging on.** The other seven describe a malformed or +truncated payload; a mismatch means content was delivered whose bytes are not the bytes +LaunchDarkly hashed, which is a possible **active-tampering** signal. Alert on it, and +treat `expected_hash` / `observed_hash` as the evidence pair. + **Versions are selected, not filtered.** A store may hold several versions of one key at once, because a delivery payload does: the newest version of every skill, plus every version a variation currently pins. `get_skill("k", version=1)` asks the store for version diff --git a/packages/client/agents.md b/packages/client/agents.md index f284c425..b5733567 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -264,6 +264,59 @@ Exactly three signals exist, and the list is an **allowlist, not a floor**: The last two belong to the materialization layer and have no caller yet; they live here with the first so the allowlist is one section of one file rather than three sites to audit. +### The integrity-failure log record + +The signal above is product telemetry; the **log record** beside it is the customer-owned +detection path, and the more load-bearing of the two. It is the only integrity surface 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 in the README rather than a debugging +aid. `record_integrity_failure` writes both, and is the only place either is constructed. + +One ERROR record per withheld skill, message text = `INTEGRITY_FAILURE_EVENT` + a space + +`json.dumps(record, sort_keys=True, separators=(",", ":"))`, plus the same mapping under +`extra={"ld_skills": record}`. Fields: `event`, `action` (always `withheld`), `skill_key`, +`version?`, `expected_hash?`, `observed_hash?`, `reason_code`, `reason`, `language`. + +Each of those choices is load-bearing; do not undo one as a simplification. + +- **The event name is in the message text**, not only in `extra`. Severity cannot + discriminate — `resolve_from_store` and `list_raw_objects` in the same module also log + ERROR for a raising store — and the stdlib's default formatter drops `extra` entirely, so + an `extra`-only record is invisible under a plain `logging.basicConfig()`. +- **`ld.skills.integrity_failure` is documented for customers to match on**, which makes it + a compatibility surface. It must never be renamed. +- **`sort_keys=True` is not cosmetic.** The other language implementations build the object + in alphabetical key order, so sorting makes the serialized line byte-identical across + SDKs for the same input, modulo `language`. +- **Optional fields are omitted, never nulled**, so a SIEM field-existence check means + something. +- **The record spreads the signal's properties** rather than rebuilding them, so the two + cannot drift on the fields they share — in particular on which are redacted. Anything + added later that comes off the wire needs the same shape-check-then-redact treatment. +- **`reason_code` is in the record only.** The signal's property set is the allowlist above + and does not grow; the local record is where the detection vocabulary lives. + +`reason_code` is a **closed vocabulary of exactly eight tokens** — `IntegrityReasonCode`, a +`Literal`, so a typo at a call site is a type error — one per `record_integrity_failure` +call site, and the same eight in every language implementation: + +| `reason_code` | Call site | +|---|---| +| `not_an_object` | `verify_raw_skill` — raw object is not a dict | +| `invalid_key` | `verify_raw_skill` — fails `is_valid_skill_key` | +| `invalid_version` | `verify_raw_skill` — fails `is_valid_skill_version` | +| `missing_content` | `verify_raw_skill` — `content` absent or not a string | +| `missing_content_hash` | `verify_raw_skill` — `contentHash` absent or not a string | +| `not_utf8` | `verified_bytes` — `UnicodeEncodeError` on encode (wire-`str` path only; a `Skill` already holds bytes) | +| `over_size_cap` | `verified_bytes` — over `MAX_SKILL_CONTENT_BYTES` | +| `hash_mismatch` | `verified_bytes` — observed sha256 != `contentHash` | + +Adding a ninth failure mode means widening `IntegrityReasonCode`, adding a case to +`REASON_CODE_CASES` in `test_skills.py` (whose exhaustiveness assertion fails otherwise), +documenting it in the README table, **and** doing the same in the other language SDKs. A +token added on one side only is a drift bug: a customer's detection rule stops matching +where they cannot see it. + `AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` were considered and **deliberately excluded from SDK emission** — both are observable server-side. Do not add them. The skill body never appears in a signal, a log line, or @@ -431,4 +484,5 @@ on their side of the boundary. - `Skill.content` is opaque `bytes`. Do not add anything that parses or interprets it — no YAML library in this package's dependencies at any tier, and no accessor that decodes content. - Do not route skills telemetry through `client.track()`, and do not introduce an LD context anywhere in the skills path. Signals go through the `skills_core.py` emitter seam, whose default is a no-op, and only via its `record_*` functions. - Do not add a signal name outside the three in the Agent Skills table above — the list is an allowlist. `AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` were considered and deliberately excluded from SDK emission. +- Do not rename `ld.skills.integrity_failure`, and do not add a ninth `reason_code` in one language only — both are documented compatibility surfaces. See "The integrity-failure log record" above. - Do not make `SkillStore` lookups key-only. Version is part of the lookup identity because a payload holds several versions of one key; a key-only seam cannot express a version-pinned reference. diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py index c478be93..1e517e4e 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_core.py +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -38,10 +38,11 @@ from __future__ import annotations import hashlib +import json import logging import re from dataclasses import dataclass -from typing import Any, Protocol +from typing import Any, Literal, Protocol, get_args from .types import Skill, SkillReference from .types_validation import is_valid_skill_key, is_valid_skill_version @@ -91,6 +92,46 @@ _SIGNAL_MATERIALIZED = "AgentControl Skill Materialized" _SIGNAL_REVOKED = "AgentControl Skill Revoked Received" +INTEGRITY_FAILURE_EVENT = "ld.skills.integrity_failure" +""" +Stable event identity for the local integrity-failure log record. + +A **compatibility surface**, not an implementation detail: the README documents +it as the string a customer's SIEM matches on, so it must never be renamed. + +It appears verbatim **in the message text**, not only in ``extra``. Severity +alone cannot discriminate — ``list_raw_objects`` and ``resolve_from_store`` in +this module also log ERROR when a store raises — and the stdlib's default +formatter drops ``extra`` entirely, so under a plain ``logging.basicConfig()`` +an ``extra``-only record would be invisible. +""" + +_ACTION_WITHHELD = "withheld" +"""The only action an integrity failure results in: content is never returned.""" + +IntegrityReasonCode = Literal[ + "not_an_object", + "invalid_key", + "invalid_version", + "missing_content", + "missing_content_hash", + "not_utf8", + "over_size_cap", + "hash_mismatch", +] +""" +The closed ``reason_code`` vocabulary — one token per ``record_integrity_failure`` +call site, and the same eight tokens in every language implementation of this +feature, so a detection rule written against one SDK reads the others. + +A ``Literal`` rather than a bare ``str`` so a typo at a call site is a type +error, and so widening the vocabulary is a deliberate edit here rather than a +new string invented at the site that needed it. +""" + +INTEGRITY_REASON_CODES: frozenset[str] = frozenset(get_args(IntegrityReasonCode)) +"""``IntegrityReasonCode`` as a runtime set, derived rather than restated.""" + NO_STORE_MESSAGE = ( "No skill store is configured, so skill content cannot be retrieved. Configure " 'one with init_client(options={"skillStore": store}) — InMemorySkillStore is ' @@ -234,17 +275,39 @@ def record_integrity_failure( skill_key: str, reason: str, *, + reason_code: IntegrityReasonCode, version: Any = None, expected_hash: Any = None, observed_hash: str | None = None, ) -> None: """ - Records an integrity failure. Carries hashes and byte counts only — the skill - body never appears in a signal, a log line, or an error message. + Records an integrity failure on both surfaces: one local log record, one + product signal. + + Carries hashes and byte counts only — the skill body never appears in a + signal, a log line, or an error message. + + The two surfaces are deliberately different sizes, and the log record is the + more important of the two. The signal is product telemetry: opt-out + respecting, no-op by default, and its property set is a documented allowlist + (``agents.md``) that does not grow. The **log record** is the customer-owned + detection path — the only one that works when telemetry is off, and the only + one that exists at all in an instance with no telemetry destination — so it + is designed to be ingested and alerted on, and additionally carries the + stable event name, the action taken, the human-readable reason, and the + machine-parseable ``reason_code``. + + Emitted twice over, because neither form alone is sufficient: the message + text carries ``INTEGRITY_FAILURE_EVENT`` followed by compact JSON, so the + record survives ``logging.basicConfig()`` and is greppable and ``jq``-able + under any handler configuration; ``extra["ld_skills"]`` carries the same + mapping unflattened for a structured handler that would rather not reparse. """ # Both of these come off the wire, so neither may be echoed verbatim: a store # that set contentHash (or key) to the skill body would otherwise publish the - # body itself. Shape-check, then redact. + # body itself. Shape-check, then redact. Every field the log record adds on + # top is either a literal or SDK-authored, so the record introduces no new + # untrusted value — anything added later needs this same treatment. safe_key = skill_key if is_valid_skill_key(skill_key) else "" properties: dict[str, Any] = {"skill_key": safe_key, "language": _LANGUAGE} if is_valid_skill_version(version): @@ -258,7 +321,28 @@ def record_integrity_failure( if observed_hash is not None: properties["observed_hash"] = observed_hash - logger.error("Skill '%s' failed integrity verification: %s", safe_key, reason) + # Spread the signal's properties rather than rebuilding them, so the record + # cannot drift from the signal on the fields they share — in particular on + # which of them are redacted and which are omitted. Absent optional fields + # stay absent; the record never carries a null. + record: dict[str, Any] = { + "event": INTEGRITY_FAILURE_EVENT, + "action": _ACTION_WITHHELD, + "reason_code": reason_code, + "reason": reason, + **properties, + } + # ``sort_keys`` is load-bearing rather than cosmetic: the other language + # implementations build this object in alphabetical key order, so sorting + # here makes the serialized line byte-identical across SDKs for the same + # input, modulo ``language``. Do not drop it, and do not reorder the keys + # above expecting the output to follow. + logger.error( + "%s %s", + INTEGRITY_FAILURE_EVENT, + json.dumps(record, sort_keys=True, separators=(",", ":")), + extra={"ld_skills": record}, + ) emit(_SIGNAL_INTEGRITY_FAILURE, properties) @@ -375,7 +459,11 @@ def verified_bytes( # satisfy the hash comparison. reason = "content is not encodable as UTF-8" record_integrity_failure( - key, reason, version=version, expected_hash=expected_hash + key, + reason, + reason_code="not_utf8", + version=version, + expected_hash=expected_hash, ) return VerificationFailure(reason) @@ -385,7 +473,11 @@ def verified_bytes( f"{MAX_SKILL_CONTENT_BYTES} byte cap" ) record_integrity_failure( - key, reason, version=version, expected_hash=expected_hash + key, + reason, + reason_code="over_size_cap", + version=version, + expected_hash=expected_hash, ) return VerificationFailure(reason) @@ -396,6 +488,7 @@ def verified_bytes( record_integrity_failure( key, "content hash mismatch", + reason_code="hash_mismatch", version=version, expected_hash=expected_hash, observed_hash=observed_hash, @@ -414,7 +507,11 @@ def verify_raw_skill(raw: Any) -> Skill | None: user code. """ if not isinstance(raw, dict): - record_integrity_failure("", "raw skill object is not an object") + record_integrity_failure( + "", + "raw skill object is not an object", + reason_code="not_an_object", + ) return None key = raw.get("key") @@ -422,25 +519,34 @@ def verify_raw_skill(raw: Any) -> Skill | None: record_integrity_failure( key if isinstance(key, str) else "", "key is not a valid skill key", + reason_code="invalid_key", ) return None version = raw.get("version") if not is_valid_skill_version(version): - record_integrity_failure(key, "version is not an integer >= 1") + record_integrity_failure( + key, "version is not an integer >= 1", reason_code="invalid_version" + ) return None content = raw.get("content") if not isinstance(content, str): record_integrity_failure( - key, "content is missing or not a string", version=version + key, + "content is missing or not a string", + reason_code="missing_content", + version=version, ) return None expected_hash = raw.get("contentHash") if not isinstance(expected_hash, str): record_integrity_failure( - key, "contentHash is missing or not a string", version=version + key, + "contentHash is missing or not a string", + reason_code="missing_content_hash", + version=version, ) return None diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index e99a8957..203eeb4f 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -1006,6 +1006,275 @@ async def test_store_error_does_not_leak_content( assert await all_skills() == [] +# --------------------------------------------------------------------------- +# Gap 1 — the local integrity-failure log record +# --------------------------------------------------------------------------- + +INTEGRITY_EVENT = "ld.skills.integrity_failure" +"""The stable event name, spelled out rather than imported. + +The name is a compatibility surface documented for customers to match on in a +SIEM, so the test has to fail when it is renamed. Importing the constant would +rename in lockstep and assert nothing. +""" + +LOGGED_BODY = "UNIQUE-SECRET-BODY-THAT-MUST-NOT-BE-LOGGED" + + +def _raw_object(**overrides: Any) -> dict[str, Any]: + """A wire-shaped raw object with a correct ``contentHash``. + + The ``make_raw_skill`` fixture is the same factory, but a module-level + ``parametrize`` table cannot reach a fixture. + """ + raw: dict[str, Any] = { + "key": "a", + "version": 1, + "content": SKILL_BODY, + "contentHash": _hash(SKILL_BODY), + } + raw.update(overrides) + return raw + + +def _raw_without(field: str) -> dict[str, Any]: + raw = _raw_object() + del raw[field] + return raw + + +_OVERSIZE = "x" * (64 * 1024 + 1) + +REASON_CODE_CASES = [ + # Not a dict at all. Reachable through ``all_skills`` and not through + # ``get_skill``, which rejects a non-dict answer before verification. + pytest.param("not-an-object-at-all", "not_an_object", id="not_an_object"), + pytest.param(_raw_object(key="Evil/../x"), "invalid_key", id="invalid_key"), + pytest.param(_raw_object(version=0), "invalid_version", id="invalid_version"), + pytest.param(_raw_without("content"), "missing_content", id="missing_content"), + pytest.param( + _raw_without("contentHash"), "missing_content_hash", id="missing_content_hash" + ), + # A lone surrogate has no UTF-8 encoding. Only reachable on the wire-``str`` + # path: a ``Skill`` already holds bytes and skips the encode. + pytest.param( + _raw_object(content=json.loads(r'"hi \ud800 there"')), "not_utf8", id="not_utf8" + ), + # Correct hash for the oversize body, so the cap is what withheld it. + pytest.param( + _raw_object(content=_OVERSIZE, contentHash=_hash(_OVERSIZE)), + "over_size_cap", + id="over_size_cap", + ), + pytest.param( + _raw_object(contentHash="0" * 64), "hash_mismatch", id="hash_mismatch" + ), +] +"""One case per ``reason_code`` token, driven end to end through ``all_skills``. + +``all_skills`` rather than ``get_skill`` for every case so the table is uniform: +it verifies every object the store holds, including the ones too malformed to +carry a usable key, which is the only accessor path a non-dict reaches. +""" + + +def _integrity_records(caplog: pytest.LogCaptureFixture) -> list[dict[str, Any]]: + """Every integrity-failure record in *caplog*, parsed out of the message text. + + Read off the message rather than off ``record.ld_skills`` deliberately: the + message is what a customer sees under a plain ``logging.basicConfig()``, and + it is the surface the documented contract is about. The structured mirror is + asserted separately, against this. + """ + parsed: list[dict[str, Any]] = [] + for entry in caplog.records: + message = entry.getMessage() + if not message.startswith(f"{INTEGRITY_EVENT} "): + continue + parsed.append(json.loads(message[len(INTEGRITY_EVENT) + 1 :])) + return parsed + + +class TestIntegrityFailureLogRecord: + """ + The local log record is a documented detection surface, not a debugging aid. + + It is the only integrity signal that survives telemetry being switched off, + and the only one that exists at all in an instance with no telemetry + destination, so its shape is a contract: a stable event name in the message + text, a closed ``reason_code`` vocabulary, and no field a hostile store can + dictate. + """ + + @pytest.fixture(autouse=True) + def _capture_errors(self, caplog: pytest.LogCaptureFixture) -> None: + caplog.set_level("ERROR", logger="launchdarkly_ai_server.skills_core") + + async def _withhold(self, objects: dict[str, Any]) -> None: + skills_module._set_store(InMemorySkillStore(objects)) + assert await all_skills() == [] + + @pytest.mark.parametrize(("raw", "expected_code"), REASON_CODE_CASES) + async def test_one_record_per_reason_code( + self, caplog: pytest.LogCaptureFixture, raw: Any, expected_code: str + ) -> None: + await self._withhold({"a": raw}) + + records = _integrity_records(caplog) + assert len(records) == 1 + record = records[0] + assert record["reason_code"] == expected_code + assert record["event"] == INTEGRITY_EVENT + assert record["action"] == "withheld" + assert record["language"] == "python" + assert record["reason"] # the human-readable half, carrying byte counts + # Absent optional fields are omitted, never nulled: a SIEM field + # existence check has to mean something. + assert None not in record.values() + # The body never reaches a log line. Swept over every failure mode here; + # the two cases below are the ones that make the rule observable, since + # a well-formed key and digest never enter a redaction branch. + assert "Do the thing." not in json.dumps(record) + + def test_the_case_table_exhausts_the_vocabulary(self) -> None: + """The vocabulary is closed, and every token in it is reachable. + + Both directions matter. A ninth token added to the source without a call + site fails here, and so does a ninth call site that invented a token the + table does not cover — which is what keeps the Python and TypeScript + vocabularies from drifting apart one edit at a time. + """ + from launchdarkly_ai_server import skills_core + + covered = {case.values[1] for case in REASON_CODE_CASES} + assert covered == skills_core.INTEGRITY_REASON_CODES + assert len(covered) == 8 + + async def test_the_event_name_is_in_the_message_text( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Severity alone cannot discriminate, so the name has to be in the line. + + ``resolve_from_store`` and ``list_raw_objects`` also log ERROR from this + logger when a store raises, and the stdlib's default formatter drops + ``extra`` entirely — an ``extra``-only record would be invisible to a + customer running ``logging.basicConfig()``. + """ + await self._withhold({"a": _raw_object(contentHash="0" * 64)}) + + errors = [r for r in caplog.records if r.levelname == "ERROR"] + assert len(errors) == 1 + assert errors[0].getMessage().startswith(f"{INTEGRITY_EVENT} ") + + async def test_structured_handlers_get_the_same_record( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """``extra`` carries the record unflattened, and says the same thing.""" + await self._withhold({"a": _raw_object(contentHash="0" * 64)}) + + errors = [r for r in caplog.records if r.levelname == "ERROR"] + # extra lands in the record's __dict__, which is where a + # structured handler reads it from. + assert errors[0].__dict__["ld_skills"] == _integrity_records(caplog)[0] + + async def test_redaction_survives_into_the_record( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Both wire-sourced fields are still redacted in the JSON payload. + + ``key`` and ``contentHash`` are attacker-controlled, so a store can put + the skill body in either. The record must not have reopened a leak the + signal already closed — same treatment, same placeholders. + """ + await self._withhold( + { + "a": { + "key": f"{LOGGED_BODY}/../x", + "version": 1, + "content": LOGGED_BODY, + "contentHash": LOGGED_BODY, + } + } + ) + + record = _integrity_records(caplog)[0] + assert record["skill_key"] == "" + assert LOGGED_BODY not in json.dumps(record) + + async def test_a_non_sha256_expected_hash_is_redacted( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The key is valid here, so ``expected_hash`` is the field under test.""" + await self._withhold({"a": _raw_object(contentHash=LOGGED_BODY)}) + + record = _integrity_records(caplog)[0] + assert record["expected_hash"] == "" + assert LOGGED_BODY not in json.dumps(record) + + async def test_observed_hash_is_absent_before_hashing( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Nothing was hashed, so there is no observed value to report.""" + await self._withhold({"a": _raw_without("contentHash")}) + + assert "observed_hash" not in _integrity_records(caplog)[0] + + async def test_observed_hash_is_present_on_a_mismatch( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The one case that is a possible active-tampering signal. + + Positive control for the test above: an implementation that never + populated ``observed_hash`` would satisfy it vacuously. + """ + await self._withhold({"a": _raw_object(contentHash="0" * 64)}) + + record = _integrity_records(caplog)[0] + assert record["observed_hash"] == _hash(SKILL_BODY) + assert record["expected_hash"] == "0" * 64 + + async def test_the_serialized_payload_is_compact_and_key_sorted( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Sorted keys are what make the line byte-identical across SDKs. + + The other language implementations build this object in alphabetical + order, so a Python line following insertion order would differ byte for + byte on identical input. Compact separators are asserted alongside + because the two together are the serialization contract. + """ + await self._withhold({"a": _raw_object(contentHash="0" * 64)}) + + errors = [r for r in caplog.records if r.levelname == "ERROR"] + payload = errors[0].getMessage()[len(INTEGRITY_EVENT) + 1 :] + # json.loads preserves the document's order, so this is what was written. + keys = list(json.loads(payload)) + assert keys == sorted(keys) + assert ", " not in payload and ": " not in payload + + async def test_reason_code_stays_out_of_the_telemetry_signal( + self, recording_emitter: Any, caplog: pytest.LogCaptureFixture + ) -> None: + """The signal's property set is an allowlist and does not grow. + + The record is the customer-owned detection path and carries the new + vocabulary; the LD-side counter is product telemetry and was left + exactly as designed. + """ + skills_module._set_emitter_for_testing(recording_emitter) + await self._withhold({"a": _raw_object(contentHash="0" * 64)}) + + props = recording_emitter.signals(INTEGRITY_SIGNAL)[0] + assert set(props) == { + "skill_key", + "version", + "expected_hash", + "observed_hash", + "language", + } + assert _integrity_records(caplog)[0]["reason_code"] == "hash_mismatch" + + class TestTelemetrySeam: """Internal emitter seam, no client.track, no context."""