diff --git a/packages/client/README.md b/packages/client/README.md index fbd4004..ae572b3 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)] + + # 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) - for ref in refs: - print(ref.key, ref.version) + # 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,83 @@ 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. + +#### 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 +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 8b55f17..b573356 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,156 @@ 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. + +### 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 +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 +482,7 @@ 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 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/__init__.py b/packages/client/src/launchdarkly_ai_server/__init__.py index 3faff30..02c858f 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 8969f2e..28b7e81 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 a3294bd..eb3946c 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 0000000..1e517e4 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -0,0 +1,734 @@ +""" +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 json +import logging +import re +from dataclasses import dataclass +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 + +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" + +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 ' + "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, + *, + reason_code: IntegrityReasonCode, + version: Any = None, + expected_hash: Any = None, + observed_hash: str | None = None, +) -> None: + """ + 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. 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): + 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 + + # 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) + + +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, + reason_code="not_utf8", + 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, + reason_code="over_size_cap", + 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", + reason_code="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", + reason_code="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", + 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", 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", + 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", + reason_code="missing_content_hash", + 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 6c14020..17fbb68 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 38ca34f..203eeb4 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,1194 @@ 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() == [] + + +# --------------------------------------------------------------------------- +# 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.""" + + 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