From 8ea3b8cc998d863ea2a1fadfed46f87587c20116 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 6 Oct 2026 13:01:20 -0400 Subject: [PATCH 1/5] feat(client): publish Agent Skills from the experimental entry point Agent Skills is exported only from launchdarkly_ai_server.experimental.skills, and none of its names is exported from the package root. Its names may change in a minor release while it is experimental. - Add set_skill_store(store) and remove the skillStore option from init_client: core client options do not name an experimental feature. - shutdown() still clears the skill store, through an internal hook whose failure is logged rather than raised. - Pin the experimental surface to an explicit allowlist in tests, and assert that no Skills name is reachable from the package root. Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 30 ++- packages/client/agents.md | 26 +- .../src/launchdarkly_ai_server/__init__.py | 52 ---- .../experimental/__init__.py | 8 + .../experimental/skills.py | 70 ++++++ .../src/launchdarkly_ai_server/lifecycle.py | 33 ++- .../src/launchdarkly_ai_server/skills.py | 21 +- .../src/launchdarkly_ai_server/skills_core.py | 8 +- .../src/launchdarkly_ai_server/skills_fdv2.py | 4 +- .../launchdarkly_ai_server/skills_watch.py | 2 +- packages/client/tests/conftest.py | 2 +- packages/client/tests/test_skills.py | 229 +++++++++--------- packages/client/tests/test_skills_fdv2.py | 53 ++-- packages/client/tests/test_skills_fs.py | 5 +- packages/client/tests/test_skills_watch.py | 56 +++-- 15 files changed, 347 insertions(+), 252 deletions(-) create mode 100644 packages/client/src/launchdarkly_ai_server/experimental/__init__.py create mode 100644 packages/client/src/launchdarkly_ai_server/experimental/skills.py diff --git a/packages/client/README.md b/packages/client/README.md index bd770abf..094a60f0 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -345,7 +345,11 @@ asyncio.run(main()) --- -### Agent Skills +### Agent Skills (experimental) + +> **Experimental.** Import Agent Skills from `launchdarkly_ai_server.experimental.skills`; +> none of these names is exported from the package root. They may change in a minor release, +> and each change is listed in the changelog under **Experimental**. Skills are versioned `SKILL.md` documents managed in LaunchDarkly and attached to AI Config variations by reference. The SDK tells you which skills a config references, retrieves their @@ -357,9 +361,9 @@ import asyncio import hashlib from pathlib import Path -from launchdarkly_ai_server import ( - init_client, inspect_config, skill_refs, get_skill, write_skills, - InMemorySkillStore, +from launchdarkly_ai_server import init_client, inspect_config +from launchdarkly_ai_server.experimental.skills import ( + InMemorySkillStore, get_skill, set_skill_store, skill_refs, write_skills, ) SKILL_MD = "---\nname: PDF Extraction\n---\nExtract text from PDFs.\n" @@ -376,7 +380,8 @@ async def main(): # hash does not match is withheld. "contentHash": hashlib.sha256(SKILL_MD.encode("utf-8")).hexdigest(), }) - await init_client(options={"skillStore": store}) + set_skill_store(store) + await init_client() # 1. Which skills does this config reference? Pure projection — no I/O. info = await inspect_config("doc-agent", {"kind": "user", "key": "user-123"}) @@ -542,7 +547,7 @@ retrieval, verification, and telemetry as `get_skill`, but reports which of five happened instead of collapsing them all to `None`. ```python -from launchdarkly_ai_server import get_skill_result +from launchdarkly_ai_server.experimental.skills import get_skill_result outcome = await get_skill_result("pdf-extraction") @@ -590,13 +595,15 @@ authenticated with the environment's server-side SDK key. ```python import os -from launchdarkly_ai_server import FDv2SkillStore, init_client, watch_skills +from launchdarkly_ai_server.experimental.skills import ( + FDv2SkillStore, set_skill_store, watch_skills, +) store = FDv2SkillStore(os.environ["LD_SDK_KEY"]).start() if not store.wait_for_skills(timeout=10): # No payload arrived. Reconciling now would find an empty store; see below. print(f"skill delivery has not answered yet: {store.failed or 'still waiting'}") -await init_client(options={"skillStore": store}) +set_skill_store(store) # Materialize now, and re-materialize whenever delivery changes. report, watcher = await watch_skills("*", ".claude/skills") @@ -709,9 +716,10 @@ that skips verification. | `watch_skills(skills, root, *, debounce=0.5, on_reconcile=None, …)` | `write_skills` plus a re-reconcile on every delivery change, so revocation takes effect within `debounce` rather than at the next restart. Returns `(initial report, SkillWatcher)`; close the watcher when done. `debounce` is in **seconds**, non-negative and finite. `on_reconcile` receives each *subsequent* report. One watcher per root. | | `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `last_error`. | -Configure the store with `init_client(options={"skillStore": store})`. With none configured, -the accessors raise `RuntimeError` explaining what to do, and `write_skills` reports the -failure (or raises, with `on_unavailable="raise"`). `shutdown()` clears it. +Configure the store with `set_skill_store(store)`. It applies on every call, before or after +`init_client`, and `None` never clears a configured store. With none configured, the accessors +raise `RuntimeError` explaining what to do, and `write_skills` reports the failure (or raises, +with `on_unavailable="raise"`). `shutdown()` clears it. `ReconcileReport.actions` holds one `ReconcileAction` per outcome (`written`, `updated`, `skipped_current`, `removed`, or `error`), each with `key`, `version`, the resolved `path`, and diff --git a/packages/client/agents.md b/packages/client/agents.md index a57c3389..b5f8920e 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -61,7 +61,6 @@ from launchdarkly_ai_server import ( TrackData, UsageDict, HandlerResult, HandlerStreamEvent, StreamEvent, StreamChunkEvent, StreamDoneEvent, ExecuteStreamEvent, ExecuteStreamDoneEvent, VariationMeta, InitClientOptions, JudgeResult, ParseResult, ParseSuccess, ParseFailure, - Skill, SkillReference, ReconcileAction, ReconcileReport, ) # Utilities @@ -79,15 +78,23 @@ from launchdarkly_ai_server import execute_and_track, execute_and_stream, wrap_t # Entry points from launchdarkly_ai_server import config, graph, resolve_graph, init_evaluations -# Agent Skills -from launchdarkly_ai_server import ( +# Agent Skills (experimental: none of these is exported from the package root) +from launchdarkly_ai_server.experimental.skills import ( + set_skill_store, SkillStore, InMemorySkillStore, FDv2SkillStore, StoreDiagnostics, skill_refs, get_skill, get_skill_result, get_skills, all_skills, write_skills, - SkillStore, InMemorySkillStore, SkillOutcome, + watch_skills, SkillWatcher, + Skill, SkillReference, SkillOutcome, ReconcileAction, ReconcileReport, SKILL_FILENAME, MANIFEST_FILENAME, MANIFEST_VERSION, ReconcileActionKind, OnUnavailable, SkillOutcomeReason, # the three closed-set unions ) ``` +An experimental feature is exported only from its module under +`launchdarkly_ai_server.experimental`, and its names may change in a minor release. No core +public type, function or `init_client` option may name it. Core reaches it only through an +internal hook (as `shutdown` clears the skill store), and an error there is logged, never +raised into the core call. + `MAX_SKILL_CONTENT_BYTES` and `SKILL_OBJECT_KIND` are deliberately **not** exported; both stay internal to `skills_core`: @@ -97,7 +104,7 @@ internal to `skills_core`: would imply otherwise. An adapter that needs it imports it from `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`). +When adding a new core export, add it to `__init__.py`'s imports and `__all__`. An Agent Skills export goes in `experimental/skills.py` instead, and in `EXPERIMENTAL_SKILLS_SURFACE` in `tests/test_skills.py`. Handler packages must never import from sub-paths (e.g. `launchdarkly_ai_server.client`). --- @@ -200,8 +207,8 @@ Three layers, in increasing order of blast radius: 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** — `get_skill`, `get_skill_result`, `get_skills`, `all_skills` read - through the `SkillStore` seam. Configure a store with - `init_client(options={"skillStore": store})`; with none configured the accessors raise + through the `SkillStore` seam. Configure a store with `set_skill_store(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** — `write_skills(skills, root)` writes `//SKILL.md` and @@ -638,9 +645,8 @@ three signals are emitted by the `record_*` functions there; nothing else calls the allowlist is enforced in one place. **Injection goes through `skills.py`.** `_set_store`, `_set_emitter_for_testing` and -`_clear_state` delegate to `skills_core`. `init_client`, `shutdown` and tests use those -(`skills._set_store(store)` is the setter `init_client` uses); none should reach into -`skills_core` directly. +`_clear_state` delegate to `skills_core`. `set_skill_store`, `shutdown` and tests use +those; none should reach into `skills_core` directly. ### Descriptor-pinned filesystem access diff --git a/packages/client/src/launchdarkly_ai_server/__init__.py b/packages/client/src/launchdarkly_ai_server/__init__.py index e2e8a8d7..a5e74f1d 100644 --- a/packages/client/src/launchdarkly_ai_server/__init__.py +++ b/packages/client/src/launchdarkly_ai_server/__init__.py @@ -63,24 +63,6 @@ resolve_tools, ) from .sdk_info import SDK_INFO_CONTEXT, SDK_INFO_EVENT, register_ai_sdk_package -from .skills import ( - InMemorySkillStore, - all_skills, - get_skill, - get_skill_result, - get_skills, - skill_refs, -) -from .skills_core import SkillStore -from .skills_fdv2 import FDv2SkillStore, StoreDiagnostics -from .skills_fs import ( - MANIFEST_FILENAME, - MANIFEST_VERSION, - SKILL_FILENAME, - OnUnavailable, - write_skills, -) -from .skills_watch import SkillWatcher, watch_skills from .tracking import execute_and_stream, execute_and_track, wrap_tool_handlers from .types import ( NATIVE_TOOL_KEY, @@ -111,13 +93,6 @@ ProviderGraphResponse, ProviderHandler, ProviderResponse, - ReconcileAction, - ReconcileActionKind, - ReconcileReport, - Skill, - SkillOutcome, - SkillOutcomeReason, - SkillReference, StreamChunkEvent, StreamDoneEvent, StreamEvent, @@ -181,11 +156,6 @@ "ProviderGraphResponse", "ProviderHandler", "ProviderResponse", - "ReconcileAction", - "ReconcileReport", - "Skill", - "SkillOutcome", - "SkillReference", "StreamChunkEvent", "StreamDoneEvent", "StreamEvent", @@ -284,28 +254,6 @@ "graph", "resolve_graph", "GraphInstance", - # skills - "skill_refs", - "get_skill", - "get_skill_result", - "get_skills", - "all_skills", - "write_skills", - "SkillStore", - "InMemorySkillStore", - # skills — LaunchDarkly delivery store and on-change re-reconcile - "FDv2SkillStore", - "StoreDiagnostics", - "watch_skills", - "SkillWatcher", - # skills — literal types for typed consumers - "ReconcileActionKind", - "OnUnavailable", - "SkillOutcomeReason", - # skills — on-disk filenames and manifest version - "SKILL_FILENAME", - "MANIFEST_FILENAME", - "MANIFEST_VERSION", ] register_ai_sdk_package("launchdarkly-ai-server", __version__) diff --git a/packages/client/src/launchdarkly_ai_server/experimental/__init__.py b/packages/client/src/launchdarkly_ai_server/experimental/__init__.py new file mode 100644 index 00000000..9519e121 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/experimental/__init__.py @@ -0,0 +1,8 @@ +""" +Experimental features of the LaunchDarkly AI SDK. + +Each feature is a submodule, for example ``launchdarkly_ai_server.experimental.skills``. +Names imported from here may change in a minor release; each change is listed in +the changelog under **Experimental**. Names from the package root change only in +a major release. +""" diff --git a/packages/client/src/launchdarkly_ai_server/experimental/skills.py b/packages/client/src/launchdarkly_ai_server/experimental/skills.py new file mode 100644 index 00000000..323210f2 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/experimental/skills.py @@ -0,0 +1,70 @@ +""" +Agent Skills (experimental). + +Configure a store with ``set_skill_store``, then read verified skill content with +``get_skill``, ``get_skills`` or ``all_skills``, or write it to disk with +``write_skills``. These names may change in a minor release; see the changelog's +**Experimental** section. +""" + +from ..skills import ( + InMemorySkillStore, + all_skills, + get_skill, + get_skill_result, + get_skills, + set_skill_store, + skill_refs, +) +from ..skills_core import SkillStore +from ..skills_fdv2 import FDv2SkillStore, StoreDiagnostics +from ..skills_fs import ( + MANIFEST_FILENAME, + MANIFEST_VERSION, + SKILL_FILENAME, + OnUnavailable, + write_skills, +) +from ..skills_watch import SkillWatcher, watch_skills +from ..types import ( + ReconcileAction, + ReconcileActionKind, + ReconcileReport, + Skill, + SkillOutcome, + SkillOutcomeReason, + SkillReference, +) + +__all__ = [ # noqa: RUF022 + # configuration + "set_skill_store", + "SkillStore", + "InMemorySkillStore", + # LaunchDarkly delivery store and on-change re-reconcile + "FDv2SkillStore", + "StoreDiagnostics", + "watch_skills", + "SkillWatcher", + # accessors + "skill_refs", + "get_skill", + "get_skill_result", + "get_skills", + "all_skills", + "write_skills", + # value types + "Skill", + "SkillReference", + "SkillOutcome", + "ReconcileAction", + "ReconcileReport", + # literal types for typed consumers + "ReconcileActionKind", + "OnUnavailable", + "SkillOutcomeReason", + # on-disk filenames and manifest version + "SKILL_FILENAME", + "MANIFEST_FILENAME", + "MANIFEST_VERSION", +] diff --git a/packages/client/src/launchdarkly_ai_server/lifecycle.py b/packages/client/src/launchdarkly_ai_server/lifecycle.py index b2c44111..92a38407 100644 --- a/packages/client/src/launchdarkly_ai_server/lifecycle.py +++ b/packages/client/src/launchdarkly_ai_server/lifecycle.py @@ -150,25 +150,12 @@ 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']`` sets the store the Agent Skills accessors read - from. Without one, the accessors raise ``RuntimeError``. - Idempotent: later calls return the existing client and ignore every option - **except** ``skillStore``, which is applied on every successful call, so you - can add a store after initialization. A ``None`` store never clears the - current one (use ``shutdown()``), and a call that raises installs nothing. + Idempotent: later calls return the existing client and ignore every option. Returns the initialized ``LDClientInterface`` instance. """ - opts = options or {} - - ld_client = await _resolve_client(opts, client) - - # Reached only on success, so a failed init installs no store. - skill_store = opts.get("skillStore") - if skill_store is not None: - skills._set_store(skill_store) - return ld_client + return await _resolve_client(options or {}, client) async def _resolve_client(opts: InitClientOptions, client: Any) -> Any: @@ -286,13 +273,21 @@ def _release_otel_globals() -> None: logger.debug("Could not release the global OTel tracer provider", exc_info=True) +def _clear_experimental_state() -> None: + """Clears experimental feature state. A failure is logged, never raised.""" + try: + skills._clear_state() + except Exception: + logger.warning("Could not clear the Agent Skills state", exc_info=True) + + 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; pass ``skillStore`` again to the - next ``init_client`` to keep using the skill accessors. + Also clears any experimental feature state, such as a configured skill + store. When telemetry was running, this also releases the process-global tracer provider so a later ``init_client`` can install its own — see @@ -304,7 +299,7 @@ async def shutdown() -> None: local_provider = _tracer_provider owned_globals = _owns_otel_globals - skills._clear_state() + _clear_experimental_state() # Null the singleton before any awaits so a second call is a no-op _client = None @@ -351,7 +346,7 @@ def _reset_for_testing() -> None: _client = None _tracer_provider = None _owns_otel_globals = False - skills._clear_state() + _clear_experimental_state() # Mirrors shutdown(): without this a suite that inits more than once leaves # every later span on the first test's provider. if owned_globals: diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index 378c9d28..81a3435f 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -18,6 +18,7 @@ from . import skills_core from .skills_core import ( SKILL_OBJECT_KIND, + SkillStore, list_raw_objects, log_withholding_summary, newest_by_key, @@ -39,13 +40,29 @@ # Injection points # --------------------------------------------------------------------------- # -# Used by ``init_client`` and ``shutdown`` (and tests). The state itself lives in -# ``skills_core``, so there is exactly one store and one emitter. +# Used by ``set_skill_store`` and ``shutdown`` (and tests). The state itself lives +# in ``skills_core``, so there is exactly one store and one emitter. _set_store = skills_core.set_store _set_emitter_for_testing = skills_core.set_emitter _clear_state = skills_core.clear_state +def set_skill_store(store: SkillStore | None) -> None: + """ + Sets the store the skill accessors and ``write_skills`` read from. + + Applies on every call, including after ``init_client``, so a lazily + initialized client can be given a store afterwards. ``None`` never clears a + configured store; ``shutdown()`` does that. + + Args: + store: ``FDv2SkillStore`` to receive skills from LaunchDarkly, or + ``InMemorySkillStore`` for local development and tests. + """ + if store is not None: + _set_store(store) + + class InMemorySkillStore: """ An in-memory skill store, for local development, tests, and diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py index cb08632e..a8c8cf03 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_core.py +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -3,7 +3,7 @@ interface and configured store, telemetry, integrity verification, and store resolution. -Package-internal except ``SkillStore``, which the package root re-exports. This +Package-internal except ``SkillStore``, which ``experimental.skills`` re-exports. This module imports neither ``skills`` nor ``skills_fs``. **Everything a store returns is untrusted.** Key, version, size and content hash @@ -93,9 +93,9 @@ NO_STORE_MESSAGE = ( "No skill store is configured, so skill content cannot be retrieved. Configure " - 'one with init_client(options={"skillStore": store}) — FDv2SkillStore receives ' - "content from LaunchDarkly, and InMemorySkillStore is available for local " - "development and testing." + "one with set_skill_store(store) from launchdarkly_ai_server.experimental.skills " + "— FDv2SkillStore receives content from LaunchDarkly, and InMemorySkillStore is " + "available for local development and testing." ) """What the accessors report when no store is configured. Callers match on "skill store"; keep that phrase if the wording changes.""" diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index fa19aef6..d3108f1e 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -1313,13 +1313,13 @@ class FDv2SkillStore: A ``SkillStore`` fed by LaunchDarkly's SDK-facing FDv2 delivery channel. Constructed with the environment's server-side SDK key, started explicitly, - and passed to ``init_client``:: + and passed to ``set_skill_store``:: store = FDv2SkillStore(sdk_key=os.environ["LD_SDK_KEY"]) store.start() if not store.wait_for_skills(timeout=10): ... # no payload yet: see is_initialized - await init_client(options={"skillStore": store}) + set_skill_store(store) skill = await get_skill("pdf-extraction") ... diff --git a/packages/client/src/launchdarkly_ai_server/skills_watch.py b/packages/client/src/launchdarkly_ai_server/skills_watch.py index 452c3eec..f7cd27c5 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_watch.py +++ b/packages/client/src/launchdarkly_ai_server/skills_watch.py @@ -228,7 +228,7 @@ async def watch_skills( if store is None: raise RuntimeError( "watch_skills needs a configured skill store. Configure one with " - 'init_client(options={"skillStore": store}).' + "set_skill_store(store)." ) add_listener = getattr(store, "add_listener", None) if not callable(add_listener): diff --git a/packages/client/tests/conftest.py b/packages/client/tests/conftest.py index de69af0c..14f7a7dd 100644 --- a/packages/client/tests/conftest.py +++ b/packages/client/tests/conftest.py @@ -7,7 +7,7 @@ import launchdarkly_ai_server.lifecycle as lifecycle_module import launchdarkly_ai_server.skills as skills_module -from launchdarkly_ai_server import InMemorySkillStore +from launchdarkly_ai_server.experimental.skills import InMemorySkillStore @pytest.fixture diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 5c2d878a..e7ac8fb0 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -16,7 +16,8 @@ import launchdarkly_ai_server.lifecycle as lifecycle_module import launchdarkly_ai_server.skills as skills_module -from launchdarkly_ai_server import ( +from launchdarkly_ai_server import get_client, init_client, shutdown +from launchdarkly_ai_server.experimental.skills import ( InMemorySkillStore, ReconcileAction, ReconcileReport, @@ -24,12 +25,10 @@ SkillOutcome, SkillReference, all_skills, - get_client, get_skill, get_skill_result, get_skills, - init_client, - shutdown, + set_skill_store, skill_refs, ) from launchdarkly_ai_server.skills_core import list_raw_objects, require_store @@ -320,24 +319,78 @@ def test_requires_no_client_or_store(self, mock_ld_client: Any) -> None: mock_ld_client.track.assert_not_called() +EXPERIMENTAL_SKILLS_SURFACE = frozenset( + { + "set_skill_store", + "SkillStore", + "InMemorySkillStore", + "FDv2SkillStore", + "StoreDiagnostics", + "watch_skills", + "SkillWatcher", + "skill_refs", + "get_skill", + "get_skill_result", + "get_skills", + "all_skills", + "write_skills", + "Skill", + "SkillReference", + "SkillOutcome", + "ReconcileAction", + "ReconcileReport", + "ReconcileActionKind", + "OnUnavailable", + "SkillOutcomeReason", + "SKILL_FILENAME", + "MANIFEST_FILENAME", + "MANIFEST_VERSION", + } +) +"""Every name ``launchdarkly_ai_server.experimental.skills`` exports, spelled out +so that any addition — an accessor that interprets ``Skill.content``, say — +fails here and is seen in review.""" + + 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 + Agent Skills is experimental: every name is exported from + ``launchdarkly_ai_server.experimental.skills`` and none from the package + root. 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_experimental_surface_is_exactly_the_documented_set(self) -> None: + """An allowlist, not a denylist of guessed names: a new export of any + name, including a reader over ``Skill.content``, fails this test.""" + from launchdarkly_ai_server.experimental import skills as experimental + + assert set(experimental.__all__) == EXPERIMENTAL_SKILLS_SURFACE + for name in EXPERIMENTAL_SKILLS_SURFACE: + assert hasattr(experimental, name), name + + def test_no_skills_name_is_exported_from_the_package_root(self) -> None: + import launchdarkly_ai_server as package + import launchdarkly_ai_server.experimental.skills + + for name in EXPERIMENTAL_SKILLS_SURFACE: + assert name not in package.__all__, name + assert not hasattr(package, name), name + 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 + from launchdarkly_ai_server.experimental import skills as experimental assert skills_core.MAX_SKILL_CONTENT_BYTES == 10485760 - assert "MAX_SKILL_CONTENT_BYTES" not in package.__all__ - assert not hasattr(package, "MAX_SKILL_CONTENT_BYTES") + for module in (package, experimental): + assert "MAX_SKILL_CONTENT_BYTES" not in module.__all__ + assert not hasattr(module, "MAX_SKILL_CONTENT_BYTES") def test_object_kind_is_not_public_api(self) -> None: """The kind is an SDK-side interface value, not the wire contract. @@ -350,46 +403,22 @@ def test_object_kind_is_not_public_api(self) -> None: """ import launchdarkly_ai_server as package from launchdarkly_ai_server import skills_core + from launchdarkly_ai_server.experimental import skills as experimental assert skills_core.SKILL_OBJECT_KIND == "skill" - assert "SKILL_OBJECT_KIND" not in package.__all__ - assert not hasattr(package, "SKILL_OBJECT_KIND") + for module in (package, experimental): + assert "SKILL_OBJECT_KIND" not in module.__all__ + assert not hasattr(module, "SKILL_OBJECT_KIND") - def test_constants_are_exported_from_the_package_root(self) -> None: - import launchdarkly_ai_server as package + def test_constants_have_their_documented_values(self) -> None: + from launchdarkly_ai_server.experimental import skills as experimental - assert package.SKILL_FILENAME == "SKILL.md" - assert package.MANIFEST_FILENAME == ".launchdarkly-skills.json" - assert package.MANIFEST_VERSION == 1 + assert experimental.SKILL_FILENAME == "SKILL.md" + assert experimental.MANIFEST_FILENAME == ".launchdarkly-skills.json" + assert experimental.MANIFEST_VERSION == 1 - def test_constants_are_listed_in_dunder_all(self) -> None: - """A name absent from ``__all__`` is not part of the public surface.""" - import launchdarkly_ai_server as package - - expected = { - "SKILL_FILENAME", - "MANIFEST_FILENAME", - "MANIFEST_VERSION", - } - assert expected <= set(package.__all__) - - def test_closed_set_types_are_exported_from_the_package_root(self) -> None: - """The two closed-set unions are public API, not implementation detail. - - ``ReconcileActionKind`` types the ``ReconcileAction.action`` field every - consumer of a report reads and switches on, and ``OnUnavailable`` types - a public keyword argument of ``write_skills``. ``agents.md`` forbids - handler packages from importing sub-path modules, so a name exported - only from the implementation module has no supported import path. - """ - import launchdarkly_ai_server as package - - assert hasattr(package, "ReconcileActionKind") - assert hasattr(package, "OnUnavailable") - assert {"ReconcileActionKind", "OnUnavailable"} <= set(package.__all__) - - def test_exported_action_union_admits_exactly_the_five_actions(self) -> None: - """The union must match the actions a report can actually carry. + def test_exported_closed_set_types_admit_exactly_their_tokens(self) -> None: + """Each union must match the values it actually types. Spelled out rather than imported from the implementation for the same reason as the constants above: deriving the expectation from the thing @@ -397,17 +426,17 @@ def test_exported_action_union_admits_exactly_the_five_actions(self) -> None: """ import typing - import launchdarkly_ai_server as package + from launchdarkly_ai_server.experimental import skills as experimental - assert set(typing.get_args(package.ReconcileActionKind)) == { + assert set(typing.get_args(experimental.ReconcileActionKind)) == { "written", "updated", "skipped_current", "removed", "error", } - assert set(typing.get_args(package.OnUnavailable)) == {"keep", "raise"} - assert set(typing.get_args(package.SkillOutcomeReason)) == { + assert set(typing.get_args(experimental.OnUnavailable)) == {"keep", "raise"} + assert set(typing.get_args(experimental.SkillOutcomeReason)) == { "absent", "integrity_failure", "ok", @@ -415,25 +444,6 @@ def test_exported_action_union_admits_exactly_the_five_actions(self) -> None: "wrong_version", } - 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_skill_result", - "get_skills", - "all_skills", - "SkillStore", - "InMemorySkillStore", - "Skill", - "SkillOutcome", - "SkillOutcomeReason", - "SkillReference", - } - assert expected <= set(package.__all__) - class TestInMemorySkillStore: """The public in-memory store implementation.""" @@ -661,18 +671,28 @@ def test_remove_listener_of_an_unregistered_callable_is_a_no_op(self) -> None: class TestStoreConfiguration: - """Store wiring on the lifecycle layer.""" + """Store wiring through ``set_skill_store``.""" - async def test_configured_via_init_client_option( + async def test_configured_via_set_skill_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) + await init_client(client=mock_ld_client) + set_skill_store(store) skill = await get_skill("a") assert skill is not None assert skill.key == "a" + async def test_set_skill_store_works_without_init_client( + self, make_raw_skill: Any + ) -> None: + """The setter does not go through ``init_client``, so it needs no client.""" + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + set_skill_store(store) + assert await get_skill("a") is not None + async def test_get_skill_raises_actionably_when_no_store(self) -> None: with pytest.raises(RuntimeError, match="skill store"): await get_skill("a") @@ -685,14 +705,18 @@ 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_the_no_store_message_names_the_delivery_store_first(self) -> None: + async def test_the_no_store_message_names_the_setter_and_both_stores( + self, + ) -> None: """ - A deployment that hits this message must be pointed at the store that - receives content from LaunchDarkly, not only at the development one. + A deployment that hits this message must be told how to configure a + store, and pointed at the store that receives content from LaunchDarkly, + not only at the development one. """ with pytest.raises(RuntimeError) as reported: await get_skill("a") message = str(reported.value) + assert "set_skill_store" in message assert "FDv2SkillStore" in message assert "InMemorySkillStore" in message assert message.index("FDv2SkillStore") < message.index("InMemorySkillStore") @@ -702,7 +726,8 @@ async def test_shutdown_clears_the_store( ) -> None: store = InMemorySkillStore() store.put(make_raw_skill(key="a")) - await init_client(options={"skillStore": store}, client=mock_ld_client) + await init_client(client=mock_ld_client) + set_skill_store(store) assert await get_skill("a") is not None await shutdown() @@ -710,68 +735,51 @@ async def test_shutdown_clears_the_store( with pytest.raises(RuntimeError, match="skill store"): await get_skill("a") - async def test_skill_store_is_applied_on_every_init_client_call( + async def test_a_second_set_skill_store_replaces_the_store( self, make_raw_skill: Any ) -> None: - """``skillStore`` is the one option a second call applies. + """The setter applies on every call, including after the client exists. - ``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. + That is what lets a client that was lazily auto-initialized, or + initialized before a store was ready, be given one afterwards. The + client singleton is asserted unchanged in the same test, because the + setter must not reach ``init_client``'s idempotency. """ first_store = InMemorySkillStore() first_store.put(make_raw_skill(key="first")) second_store = InMemorySkillStore() second_store.put(make_raw_skill(key="second")) + client = MagicMock() - 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 + await init_client(client=client) + set_skill_store(first_store) + set_skill_store(second_store) - # Half two: the store was nevertheless swapped. + assert get_client() is client 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( + async def test_set_skill_store_none_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. - """ + """``None`` never clears a store; ``shutdown()`` does that.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a")) - await init_client(options={"skillStore": store}, client=MagicMock()) + set_skill_store(store) + set_skill_store(None) 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 + async def test_init_client_ignores_a_skill_store_option( + self, 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) + """Core client options do not configure an experimental feature.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a")) - with pytest.raises(RuntimeError, match="No LaunchDarkly SDK key"): - await init_client(options={"skillStore": store}) + await init_client(options={"skillStore": store}, client=MagicMock()) with pytest.raises(RuntimeError, match="skill store"): await get_skill("a") @@ -2480,7 +2488,8 @@ async def test_no_ld_track_calls_from_accessors( 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 init_client(client=mock_ld_client) + set_skill_store(store) # See the note in test_no_ld_track_calls_from_write_skills: the # sdk-info flush belongs to init_client, not to the accessors. mock_ld_client.track.reset_mock() diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 6734f40c..560a4ba3 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -34,19 +34,18 @@ import pytest -from launchdarkly_ai_server import ( +from launchdarkly_ai_server import init_client, skills_core, skills_fdv2 +from launchdarkly_ai_server import skills as skills_module +from launchdarkly_ai_server.experimental.skills import ( FDv2SkillStore, InMemorySkillStore, all_skills, get_skill, get_skill_result, - init_client, - skills_core, - skills_fdv2, + set_skill_store, watch_skills, write_skills, ) -from launchdarkly_ai_server import skills as skills_module from launchdarkly_ai_server.skills_core import SKILL_OBJECT_KIND from launchdarkly_ai_server.skills_fdv2 import ( DEFAULT_BASE_URI, @@ -2479,7 +2478,8 @@ async def test_a_store_that_gave_up_on_a_422_prunes_nothing( assert wait_until(lambda: store.failed is not None) assert store.is_initialized() is False assert store.wait_for_skills(timeout=0.1) is False - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) report = await write_skills("*", root) assert report.ok is False assert (stale / "SKILL.md").read_text(encoding="utf-8") == "not ours to delete" @@ -3326,7 +3326,8 @@ async def test_a_hashless_skill_is_withheld_with_the_right_reason( endpoint.queue_poll(full_payload(("put-object", put_skill(omit_hash=True)))) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) outcome = await get_skill_result("pdf-extraction") assert outcome.skill is None @@ -3349,7 +3350,8 @@ async def test_the_object_is_still_held_so_the_outcome_is_not_absent( raw = store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") assert raw is not None assert "contentHash" not in raw - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) assert (await get_skill_result("pdf-extraction")).reason != "absent" def test_the_store_counts_hashless_objects(self, endpoint: Any) -> None: @@ -3468,7 +3470,8 @@ async def test_a_hash_that_does_not_match_is_a_different_failure( with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) assert store.diagnostics.hashless_objects == 0 - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) assert ( await get_skill_result("pdf-extraction") ).reason == "integrity_failure" @@ -3478,7 +3481,8 @@ async def test_a_hashed_skill_resolves_end_to_end(self, endpoint: Any) -> None: endpoint.queue_poll(full_payload(("put-object", put_skill()))) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) skill = await get_skill("pdf-extraction") assert skill is not None @@ -3499,7 +3503,8 @@ async def test_a_pinned_reference_resolves_to_the_pinned_object_version( ) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) pinned = await get_skill("pdf-extraction", version=2) assert pinned is not None @@ -3529,7 +3534,8 @@ async def test_a_missed_pin_is_absent_even_beside_a_malformed_sibling( ) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) missed = await get_skill_result("pdf-extraction", version=9) assert missed.skill is None @@ -3556,7 +3562,8 @@ async def test_the_payload_version_is_not_resolvable_as_a_skill_version( ) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) by_payload_version = await get_skill_result("pdf-extraction", version=42) assert by_payload_version.skill is None assert by_payload_version.reason == "absent" @@ -3760,7 +3767,8 @@ async def test_a_revocation_prunes_without_a_restart( with poll_store(endpoint, poll_interval=0.2) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) report, watcher = await watch_skills( "*", tmp_path / "skills", debounce=0.05 ) @@ -3786,7 +3794,8 @@ async def test_a_full_transfer_that_omits_every_skill_prunes( with poll_store(endpoint, poll_interval=0.2) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) report, watcher = await watch_skills( "*", tmp_path / "skills", debounce=0.05 ) @@ -3814,7 +3823,8 @@ async def test_a_new_version_is_rewritten_without_a_restart( with poll_store(endpoint, poll_interval=0.2) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: written = tmp_path / "s" / "pdf-extraction" / "SKILL.md" @@ -3832,7 +3842,8 @@ async def test_the_default_keeps_last_known_good_during_an_outage( endpoint.queue_poll(status=500) with poll_store(endpoint, poll_interval=0.05) as store: store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: written = tmp_path / "s" / "pdf-extraction" / "SKILL.md" @@ -4040,10 +4051,11 @@ def test_it_adds_no_dependency(self) -> None: assert _module_imports(skills_fdv2) == stdlib | within_the_feature def test_nothing_in_the_feature_imports_it(self) -> None: - """Only the package's own entry point names it, to re-export two types. + """Only the experimental entry point names it, to re-export two types. That re-export is the published surface rather than a dependency: it is - what makes ``FDv2SkillStore`` reachable without a sub-path import. A + what makes ``FDv2SkillStore`` reachable without importing the + implementation module. A *feature* module importing the transport would be the layering inverting, and the accessors would start being able to tell which store answered them. @@ -4068,7 +4080,8 @@ def test_nothing_in_the_feature_imports_it(self) -> None: assert importers == [] # The positive control: the entry point does name it, so an empty result # above cannot be a misspelt filename list finding nothing anywhere. - assert "skills_fdv2" in (source_dir / "__init__.py").read_text(encoding="utf-8") + entry_point = source_dir / "experimental" / "skills.py" + assert "skills_fdv2" in entry_point.read_text(encoding="utf-8") class TestTransportEmitsNoTelemetry: diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 3949cd1d..e23c5527 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -22,13 +22,12 @@ import launchdarkly_ai_server.skills as skills_module import launchdarkly_ai_server.skills_core as skills_core_module import launchdarkly_ai_server.skills_fs as skills_fs_module -from launchdarkly_ai_server import ( +from launchdarkly_ai_server import init_client, parse_ai_config +from launchdarkly_ai_server.experimental.skills import ( InMemorySkillStore, Skill, SkillReference, get_skill, - init_client, - parse_ai_config, skill_refs, write_skills, ) diff --git a/packages/client/tests/test_skills_watch.py b/packages/client/tests/test_skills_watch.py index 7cb7972f..e4d9dd6e 100644 --- a/packages/client/tests/test_skills_watch.py +++ b/packages/client/tests/test_skills_watch.py @@ -22,7 +22,12 @@ import pytest -from launchdarkly_ai_server import InMemorySkillStore, init_client, watch_skills +from launchdarkly_ai_server import init_client +from launchdarkly_ai_server.experimental.skills import ( + InMemorySkillStore, + set_skill_store, + watch_skills, +) from launchdarkly_ai_server.skills_core import SKILL_OBJECT_KIND pytestmark = pytest.mark.usefixtures("reset_skill_state") @@ -50,7 +55,8 @@ async def test_the_in_memory_store_can_also_drive_a_watch( store.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a", version=1, content="body")) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: written = tmp_path / "s" / "a" / "SKILL.md" @@ -74,7 +80,8 @@ async def test_a_burst_of_changes_coalesces_into_a_debounce_window( arriving inside it schedules one further pass. """ store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.6) try: assert watcher.reconciles == 0 @@ -97,7 +104,8 @@ async def test_changes_spread_beyond_the_window_do_not_coalesce( reconciles once and then never again. """ store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: store.put(make_raw_skill(key="one")) @@ -121,7 +129,8 @@ async def test_a_negative_or_non_finite_debounce_raises( ``write_skills`` already guards its ``timeout`` this way. """ store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) with pytest.raises(ValueError, match="debounce"): await watch_skills("*", tmp_path / "s", debounce=debounce) # Refused before anything was attached, so there is no watcher to close. @@ -133,7 +142,8 @@ async def test_on_reconcile_receives_each_subsequent_report( """Not the initial report: that one is returned to the caller directly.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a", version=1, content="body")) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) seen: list[Any] = [] report, watcher = await watch_skills( "*", tmp_path / "s", debounce=0.05, on_reconcile=seen.append @@ -153,7 +163,8 @@ async def test_a_reconcile_that_raises_does_not_kill_the_watcher( ) -> None: """A watcher that died on one bad run would silently stop pruning.""" store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) calls: list[int] = [] def explodes_once(_report: Any) -> None: @@ -190,7 +201,8 @@ def all_objects(self, _kind: str) -> dict[str, Any]: raise RuntimeError("store is down") store = Unavailable() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) with pytest.raises(RuntimeError, match="store is down"): await watch_skills( "*", tmp_path / "s", on_unavailable="raise", debounce=0.05 @@ -202,7 +214,8 @@ async def test_prune_is_passed_through_and_can_be_turned_off( ) -> None: store = InMemorySkillStore() store.put(make_raw_skill(key="a", version=1, content="body")) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) report, watcher = await watch_skills( "*", tmp_path / "s", prune=False, debounce=0.05 ) @@ -218,7 +231,8 @@ async def test_an_invalid_root_raises_out_of_watch_skills( """The initial reconcile runs on the caller's thread, so a bad root is the caller's exception rather than a line in a worker thread's log.""" store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) not_a_directory = tmp_path / "file" not_a_directory.write_text("") with pytest.raises(ValueError, match="not a directory"): @@ -234,7 +248,9 @@ def get_object(self, *_a: Any, **_k: Any) -> None: def all_objects(self, _kind: str) -> dict[str, Any]: return {} - await init_client(options={"skillStore": NoListeners()}, client=object()) + await init_client(client=object()) + + set_skill_store(NoListeners()) with pytest.raises(RuntimeError, match="add_listener"): await watch_skills("*", tmp_path / "s") @@ -276,7 +292,8 @@ def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: store = RevokesAfterSnapshot() store.put(make_raw_skill(key="pdf-extraction", version=1, content="body")) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: @@ -295,7 +312,8 @@ async def test_a_failed_initial_reconcile_leaves_no_listener_behind( """Registering first means a reconcile that raises has to detach: the caller is handed an exception, not a watcher to close.""" store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) not_a_directory = tmp_path / "file" not_a_directory.write_text("") @@ -323,7 +341,8 @@ async def test_a_closed_watcher_is_no_longer_notified( ) -> None: store = InMemorySkillStore() store.put(make_raw_skill(key="pdf-extraction", version=1, content="first")) - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) assert watcher.notify in self._skill_listeners(store) @@ -344,7 +363,8 @@ async def test_a_closed_watcher_is_no_longer_notified( async def test_close_twice_does_not_raise(self, tmp_path: Any) -> None: store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) watcher.close() watcher.close() @@ -354,7 +374,8 @@ async def test_repeated_watchers_leave_no_listeners_behind( self, tmp_path: Any ) -> None: store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) for _ in range(5): _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) assert len(self._skill_listeners(store)) == 1 @@ -381,7 +402,8 @@ def add_listener(self, _kind: str, fn: Any) -> None: self.listeners.append(fn) store = AddOnly() - await init_client(options={"skillStore": store}, client=object()) + await init_client(client=object()) + set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) assert store.listeners == [watcher.notify] From 01bfad1d0723d745dd765c9025fa94058178cb47 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 6 Oct 2026 15:06:51 -0400 Subject: [PATCH 2/5] refactor(client): warn on unknown init_client options and tighten set_skill_store - init_client logs a warning for any option key it does not read, so a retired option such as skillStore is reported rather than dropped silently. - Inline _resolve_client into init_client; the split existed only to install the skill store after a successful resolve. - set_skill_store takes a SkillStore and raises TypeError on None. - _reset_for_testing clears skills state directly so a failed reset fails the test instead of being logged. - Soften the changelog wording for experimental names, and note in the launchdarkly-ai-python README where experimental features are imported from. - Clarify in agents.md that AiConfigRep still documents skills references: the experimental part is the SDK API, not the data model. - Drop init_client calls that tests no longer need before set_skill_store. Co-Authored-By: Claude Opus 5.5 --- packages/ai/README.md | 7 ++++ packages/client/README.md | 4 +-- packages/client/agents.md | 3 +- .../experimental/__init__.py | 5 ++- .../experimental/skills.py | 4 +-- .../src/launchdarkly_ai_server/lifecycle.py | 32 +++++++++++++----- .../src/launchdarkly_ai_server/skills.py | 16 ++++++--- packages/client/tests/test_lifecycle.py | 33 +++++++++++++++++++ packages/client/tests/test_skills.py | 17 ++++++---- packages/client/tests/test_skills_fdv2.py | 24 ++++---------- packages/client/tests/test_skills_watch.py | 18 ---------- 11 files changed, 99 insertions(+), 64 deletions(-) diff --git a/packages/ai/README.md b/packages/ai/README.md index 829bb567..daad0caa 100644 --- a/packages/ai/README.md +++ b/packages/ai/README.md @@ -75,6 +75,13 @@ result = await evals.run( `LD_API_TOKEN` is required. Configure `LD_SDK_KEY` — or initialize your own client with `init_client(client=...)` — to emit one `$ld:ai:offline-evals:generation` event per generated row, plus one `$ld:ai:offline-evals:criterion` event per `(row, criterion)` when `criteria` are supplied, through the standard SDK event transport. The SDK reports scores; LaunchDarkly rules on them at ingest. A judge served by a different provider than `generation` needs a handler for it in `judge_handlers`. Each row's tool calls are recorded during generation and rendered into the judge's `{{message_history}}`, between the row input and the generated output, so a rubric can grade the tool trajectory as well as the final answer. Use `LD_API_BASE_URI` for staging or local management API traffic; it is separate from the SDK delivery setting `LD_BASE_URI`. Evaluation-run links use the explicit `ui_base_uri` option or `LD_UI_BASE_URI`, defaulting to `https://app.launchdarkly.com`; set it when the project is not in production, or a run created elsewhere still links to the production app. See the [core evaluations guide](../client/README.md#run-an-evaluation-from-code). +## Experimental features + +Experimental features are not re-exported from this package. Import them from +`launchdarkly_ai_server.experimental`, which this package installs. For example, Agent Skills +lives in `launchdarkly_ai_server.experimental.skills`; see the +[Agent Skills guide](https://github.com/launchdarkly/python-ai-sdk/blob/main/packages/client/README.md#agent-skills-experimental). + --- All exports, types, and behaviors are identical to `launchdarkly-ai-server`. See the [core client README](../client/README.md) for the full API reference. diff --git a/packages/client/README.md b/packages/client/README.md index 094a60f0..bbee0313 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -349,7 +349,7 @@ asyncio.run(main()) > **Experimental.** Import Agent Skills from `launchdarkly_ai_server.experimental.skills`; > none of these names is exported from the package root. They may change in a minor release, -> and each change is listed in the changelog under **Experimental**. +> so review the changelog when you upgrade. Skills are versioned `SKILL.md` documents managed in LaunchDarkly and attached to AI Config variations by reference. The SDK tells you which skills a config references, retrieves their @@ -717,7 +717,7 @@ that skips verification. | `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `last_error`. | Configure the store with `set_skill_store(store)`. It applies on every call, before or after -`init_client`, and `None` never clears a configured store. With none configured, the accessors +`init_client`; passing `None` raises `TypeError`. With none configured, the accessors raise `RuntimeError` explaining what to do, and `write_skills` reports the failure (or raises, with `on_unavailable="raise"`). `shutdown()` clears it. diff --git a/packages/client/agents.md b/packages/client/agents.md index b5f8920e..3797aa01 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -93,7 +93,8 @@ An experimental feature is exported only from its module under `launchdarkly_ai_server.experimental`, and its names may change in a minor release. No core public type, function or `init_client` option may name it. Core reaches it only through an internal hook (as `shutdown` clears the skill store), and an error there is logged, never -raised into the core call. +raised into the core call. The experimental part is the SDK's API, not the LaunchDarkly data +model: `AiConfigRep` still documents the `skills` references a config carries. `MAX_SKILL_CONTENT_BYTES` and `SKILL_OBJECT_KIND` are deliberately **not** exported; both stay internal to `skills_core`: diff --git a/packages/client/src/launchdarkly_ai_server/experimental/__init__.py b/packages/client/src/launchdarkly_ai_server/experimental/__init__.py index 9519e121..80ecfd7d 100644 --- a/packages/client/src/launchdarkly_ai_server/experimental/__init__.py +++ b/packages/client/src/launchdarkly_ai_server/experimental/__init__.py @@ -2,7 +2,6 @@ Experimental features of the LaunchDarkly AI SDK. Each feature is a submodule, for example ``launchdarkly_ai_server.experimental.skills``. -Names imported from here may change in a minor release; each change is listed in -the changelog under **Experimental**. Names from the package root change only in -a major release. +Names imported from here may change in a minor release, so review the changelog +when you upgrade. Names from the package root change only in a major release. """ diff --git a/packages/client/src/launchdarkly_ai_server/experimental/skills.py b/packages/client/src/launchdarkly_ai_server/experimental/skills.py index 323210f2..18a2c9c6 100644 --- a/packages/client/src/launchdarkly_ai_server/experimental/skills.py +++ b/packages/client/src/launchdarkly_ai_server/experimental/skills.py @@ -3,8 +3,8 @@ Configure a store with ``set_skill_store``, then read verified skill content with ``get_skill``, ``get_skills`` or ``all_skills``, or write it to disk with -``write_skills``. These names may change in a minor release; see the changelog's -**Experimental** section. +``write_skills``. These names may change in a minor release, so review the +changelog when you upgrade. """ from ..skills import ( diff --git a/packages/client/src/launchdarkly_ai_server/lifecycle.py b/packages/client/src/launchdarkly_ai_server/lifecycle.py index 92a38407..32d1cf51 100644 --- a/packages/client/src/launchdarkly_ai_server/lifecycle.py +++ b/packages/client/src/launchdarkly_ai_server/lifecycle.py @@ -15,6 +15,19 @@ _LD_DEFAULT_OTLP_ENDPOINT = "https://otel.observability.app.launchdarkly.com" +_INIT_CLIENT_OPTIONS = frozenset( + { + "sdkKey", + "baseUri", + "streamUri", + "eventsUri", + "otlpEndpoint", + "serviceName", + "environment", + } +) +"""Every ``init_client`` option key that is read. Any other key is warned about.""" + def _env(name: str) -> str | None: """Read an env var, treating blank/whitespace-only values as unset.""" @@ -152,18 +165,19 @@ async def init_client( - Otherwise, reads ``LD_SDK_KEY`` from env or ``options['sdkKey']``. Idempotent: later calls return the existing client and ignore every option. + An option this function does not read is logged as a warning and ignored. Returns the initialized ``LDClientInterface`` instance. """ - return await _resolve_client(options or {}, client) - - -async def _resolve_client(opts: InitClientOptions, client: Any) -> Any: - """ - Returns the singleton client, initializing it on first call. - """ global _client + opts = options or {} + unknown = sorted(str(key) for key in opts if key not in _INIT_CLIENT_OPTIONS) + if unknown: + logger.warning( + "Ignoring unrecognized init_client option(s): %s", ", ".join(unknown) + ) + # Idempotent — if already initialized, return the existing client if _client is not None: flush_ai_sdk_info(_client) @@ -346,7 +360,9 @@ def _reset_for_testing() -> None: _client = None _tracer_provider = None _owns_otel_globals = False - _clear_experimental_state() + # Directly, not through _clear_experimental_state: a failed reset should + # fail the test rather than leak state into the next one. + skills._clear_state() # Mirrors shutdown(): without this a suite that inits more than once leaves # every later span on the first test's provider. if owned_globals: diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index 81a3435f..97b09862 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -47,20 +47,26 @@ _clear_state = skills_core.clear_state -def set_skill_store(store: SkillStore | None) -> None: +def set_skill_store(store: SkillStore) -> None: """ Sets the store the skill accessors and ``write_skills`` read from. Applies on every call, including after ``init_client``, so a lazily - initialized client can be given a store afterwards. ``None`` never clears a - configured store; ``shutdown()`` does that. + initialized client can be given a store afterwards. ``shutdown()`` clears + it. Args: store: ``FDv2SkillStore`` to receive skills from LaunchDarkly, or ``InMemorySkillStore`` for local development and tests. + + Raises: + TypeError: If *store* is ``None``. """ - if store is not None: - _set_store(store) + if store is None: + raise TypeError( + "set_skill_store needs a store; shutdown() clears the configured one." + ) + _set_store(store) class InMemorySkillStore: diff --git a/packages/client/tests/test_lifecycle.py b/packages/client/tests/test_lifecycle.py index 7ffc33ae..e4b7c1b1 100644 --- a/packages/client/tests/test_lifecycle.py +++ b/packages/client/tests/test_lifecycle.py @@ -116,6 +116,39 @@ async def test_repeat_call_does_not_rerun_telemetry_setup(self) -> None: assert setup.call_count == 1 assert setup.call_args.args[1] == {"serviceName": "first"} + async def test_warns_about_an_unrecognized_option( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """A misspelt or retired option would otherwise be dropped silently.""" + stub = _make_stub_client() + with ( + patch.object(lifecycle_module, "_setup_telemetry", return_value=None), + caplog.at_level("WARNING", logger="launchdarkly_ai_server.lifecycle"), + ): + await init_client({"serviceName": "svc", "sdkkey": "sdk-x"}, stub) + warnings = [r.getMessage() for r in caplog.records] + assert warnings == ["Ignoring unrecognized init_client option(s): sdkkey"] + + async def test_does_not_warn_about_documented_options( + self, caplog: pytest.LogCaptureFixture + ) -> None: + stub = _make_stub_client() + options = { + "sdkKey": "sdk-x", + "baseUri": "https://base.example", + "streamUri": "https://stream.example", + "eventsUri": "https://events.example", + "otlpEndpoint": "https://otlp.example", + "serviceName": "svc", + "environment": "test", + } + with ( + patch.object(lifecycle_module, "_setup_telemetry", return_value=None), + caplog.at_level("WARNING", logger="launchdarkly_ai_server.lifecycle"), + ): + await init_client(options, stub) + assert caplog.records == [] + async def test_repeat_call_does_not_swap_the_client(self) -> None: first = _make_stub_client() second = _make_stub_client() diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index e7ac8fb0..664ce456 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -759,28 +759,31 @@ async def test_a_second_set_skill_store_replaces_the_store( assert await get_skill("second") is not None assert await get_skill("first") is None - async def test_set_skill_store_none_leaves_the_configured_one( + async def test_set_skill_store_refuses_none_and_keeps_the_store( self, make_raw_skill: Any ) -> None: - """``None`` never clears a store; ``shutdown()`` does that.""" + """``None`` is refused rather than read as "clear"; ``shutdown()`` clears.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a")) set_skill_store(store) - set_skill_store(None) - await init_client(client=MagicMock()) + with pytest.raises(TypeError, match="shutdown"): + set_skill_store(None) # type: ignore[arg-type] assert await get_skill("a") is not None async def test_init_client_ignores_a_skill_store_option( - self, make_raw_skill: Any + self, make_raw_skill: Any, caplog: pytest.LogCaptureFixture ) -> None: - """Core client options do not configure an experimental feature.""" + """Core client options do not configure an experimental feature, and + the ignored option is reported rather than dropped silently.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a")) - await init_client(options={"skillStore": store}, client=MagicMock()) + with caplog.at_level("WARNING", logger="launchdarkly_ai_server.lifecycle"): + await init_client(options={"skillStore": store}, client=MagicMock()) + assert any("skillStore" in message for message in caplog.messages) with pytest.raises(RuntimeError, match="skill store"): await get_skill("a") diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 560a4ba3..6e3770db 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -34,8 +34,8 @@ import pytest -from launchdarkly_ai_server import init_client, skills_core, skills_fdv2 from launchdarkly_ai_server import skills as skills_module +from launchdarkly_ai_server import skills_core, skills_fdv2 from launchdarkly_ai_server.experimental.skills import ( FDv2SkillStore, InMemorySkillStore, @@ -2478,7 +2478,6 @@ async def test_a_store_that_gave_up_on_a_422_prunes_nothing( assert wait_until(lambda: store.failed is not None) assert store.is_initialized() is False assert store.wait_for_skills(timeout=0.1) is False - await init_client(client=object()) set_skill_store(store) report = await write_skills("*", root) assert report.ok is False @@ -3326,7 +3325,6 @@ async def test_a_hashless_skill_is_withheld_with_the_right_reason( endpoint.queue_poll(full_payload(("put-object", put_skill(omit_hash=True)))) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) outcome = await get_skill_result("pdf-extraction") @@ -3350,7 +3348,6 @@ async def test_the_object_is_still_held_so_the_outcome_is_not_absent( raw = store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") assert raw is not None assert "contentHash" not in raw - await init_client(client=object()) set_skill_store(store) assert (await get_skill_result("pdf-extraction")).reason != "absent" @@ -3470,7 +3467,6 @@ async def test_a_hash_that_does_not_match_is_a_different_failure( with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) assert store.diagnostics.hashless_objects == 0 - await init_client(client=object()) set_skill_store(store) assert ( await get_skill_result("pdf-extraction") @@ -3481,7 +3477,6 @@ async def test_a_hashed_skill_resolves_end_to_end(self, endpoint: Any) -> None: endpoint.queue_poll(full_payload(("put-object", put_skill()))) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) skill = await get_skill("pdf-extraction") @@ -3503,7 +3498,6 @@ async def test_a_pinned_reference_resolves_to_the_pinned_object_version( ) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) pinned = await get_skill("pdf-extraction", version=2) @@ -3534,7 +3528,6 @@ async def test_a_missed_pin_is_absent_even_beside_a_malformed_sibling( ) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) missed = await get_skill_result("pdf-extraction", version=9) @@ -3562,7 +3555,6 @@ async def test_the_payload_version_is_not_resolvable_as_a_skill_version( ) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) by_payload_version = await get_skill_result("pdf-extraction", version=42) assert by_payload_version.skill is None @@ -3767,7 +3759,6 @@ async def test_a_revocation_prunes_without_a_restart( with poll_store(endpoint, poll_interval=0.2) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) report, watcher = await watch_skills( "*", tmp_path / "skills", debounce=0.05 @@ -3794,7 +3785,6 @@ async def test_a_full_transfer_that_omits_every_skill_prunes( with poll_store(endpoint, poll_interval=0.2) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) report, watcher = await watch_skills( "*", tmp_path / "skills", debounce=0.05 @@ -3823,7 +3813,6 @@ async def test_a_new_version_is_rewritten_without_a_restart( with poll_store(endpoint, poll_interval=0.2) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: @@ -3842,7 +3831,6 @@ async def test_the_default_keeps_last_known_good_during_an_outage( endpoint.queue_poll(status=500) with poll_store(endpoint, poll_interval=0.05) as store: store.wait_for_skills(timeout=5) - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: @@ -4051,14 +4039,14 @@ def test_it_adds_no_dependency(self) -> None: assert _module_imports(skills_fdv2) == stdlib | within_the_feature def test_nothing_in_the_feature_imports_it(self) -> None: - """Only the experimental entry point names it, to re-export two types. + """Only the experimental entry point names it, to re-export + ``FDv2SkillStore`` and ``StoreDiagnostics``. That re-export is the published surface rather than a dependency: it is what makes ``FDv2SkillStore`` reachable without importing the - implementation module. A - *feature* module importing the transport would be the layering - inverting, and the accessors would start being able to tell which store - answered them. + implementation module. A *feature* module importing the transport would + be the layering inverting, and the accessors would start being able to + tell which store answered them. """ feature = [ "skills.py", diff --git a/packages/client/tests/test_skills_watch.py b/packages/client/tests/test_skills_watch.py index e4d9dd6e..72dee6d6 100644 --- a/packages/client/tests/test_skills_watch.py +++ b/packages/client/tests/test_skills_watch.py @@ -22,7 +22,6 @@ import pytest -from launchdarkly_ai_server import init_client from launchdarkly_ai_server.experimental.skills import ( InMemorySkillStore, set_skill_store, @@ -55,7 +54,6 @@ async def test_the_in_memory_store_can_also_drive_a_watch( store.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a", version=1, content="body")) - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: @@ -80,7 +78,6 @@ async def test_a_burst_of_changes_coalesces_into_a_debounce_window( arriving inside it schedules one further pass. """ store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.6) try: @@ -104,7 +101,6 @@ async def test_changes_spread_beyond_the_window_do_not_coalesce( reconciles once and then never again. """ store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) try: @@ -129,7 +125,6 @@ async def test_a_negative_or_non_finite_debounce_raises( ``write_skills`` already guards its ``timeout`` this way. """ store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) with pytest.raises(ValueError, match="debounce"): await watch_skills("*", tmp_path / "s", debounce=debounce) @@ -142,7 +137,6 @@ async def test_on_reconcile_receives_each_subsequent_report( """Not the initial report: that one is returned to the caller directly.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a", version=1, content="body")) - await init_client(client=object()) set_skill_store(store) seen: list[Any] = [] report, watcher = await watch_skills( @@ -163,7 +157,6 @@ async def test_a_reconcile_that_raises_does_not_kill_the_watcher( ) -> None: """A watcher that died on one bad run would silently stop pruning.""" store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) calls: list[int] = [] @@ -201,7 +194,6 @@ def all_objects(self, _kind: str) -> dict[str, Any]: raise RuntimeError("store is down") store = Unavailable() - await init_client(client=object()) set_skill_store(store) with pytest.raises(RuntimeError, match="store is down"): await watch_skills( @@ -214,7 +206,6 @@ async def test_prune_is_passed_through_and_can_be_turned_off( ) -> None: store = InMemorySkillStore() store.put(make_raw_skill(key="a", version=1, content="body")) - await init_client(client=object()) set_skill_store(store) report, watcher = await watch_skills( "*", tmp_path / "s", prune=False, debounce=0.05 @@ -231,7 +222,6 @@ async def test_an_invalid_root_raises_out_of_watch_skills( """The initial reconcile runs on the caller's thread, so a bad root is the caller's exception rather than a line in a worker thread's log.""" store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) not_a_directory = tmp_path / "file" not_a_directory.write_text("") @@ -248,8 +238,6 @@ def get_object(self, *_a: Any, **_k: Any) -> None: def all_objects(self, _kind: str) -> dict[str, Any]: return {} - await init_client(client=object()) - set_skill_store(NoListeners()) with pytest.raises(RuntimeError, match="add_listener"): await watch_skills("*", tmp_path / "s") @@ -292,7 +280,6 @@ def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: store = RevokesAfterSnapshot() store.put(make_raw_skill(key="pdf-extraction", version=1, content="body")) - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) @@ -312,7 +299,6 @@ async def test_a_failed_initial_reconcile_leaves_no_listener_behind( """Registering first means a reconcile that raises has to detach: the caller is handed an exception, not a watcher to close.""" store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) not_a_directory = tmp_path / "file" not_a_directory.write_text("") @@ -341,7 +327,6 @@ async def test_a_closed_watcher_is_no_longer_notified( ) -> None: store = InMemorySkillStore() store.put(make_raw_skill(key="pdf-extraction", version=1, content="first")) - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) assert watcher.notify in self._skill_listeners(store) @@ -363,7 +348,6 @@ async def test_a_closed_watcher_is_no_longer_notified( async def test_close_twice_does_not_raise(self, tmp_path: Any) -> None: store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) watcher.close() @@ -374,7 +358,6 @@ async def test_repeated_watchers_leave_no_listeners_behind( self, tmp_path: Any ) -> None: store = InMemorySkillStore() - await init_client(client=object()) set_skill_store(store) for _ in range(5): _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) @@ -402,7 +385,6 @@ def add_listener(self, _kind: str, fn: Any) -> None: self.listeners.append(fn) store = AddOnly() - await init_client(client=object()) set_skill_store(store) _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) assert store.listeners == [watcher.notify] From 56bfacaa283c1115b89e69524b1328ef1d653742 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 6 Oct 2026 15:36:03 -0400 Subject: [PATCH 3/5] fix(client): ignore None in set_skill_store, matching the JS SDK Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 6 +++--- .../client/src/launchdarkly_ai_server/skills.py | 16 +++++----------- packages/client/tests/test_skills.py | 7 +++---- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index bbee0313..6087f491 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -717,9 +717,9 @@ that skips verification. | `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `last_error`. | Configure the store with `set_skill_store(store)`. It applies on every call, before or after -`init_client`; passing `None` raises `TypeError`. With none configured, the accessors -raise `RuntimeError` explaining what to do, and `write_skills` reports the failure (or raises, -with `on_unavailable="raise"`). `shutdown()` clears it. +`init_client`, and `None` is ignored rather than clearing a configured store. With none +configured, the accessors raise `RuntimeError` explaining what to do, and `write_skills` +reports the failure (or raises, with `on_unavailable="raise"`). `shutdown()` clears it. `ReconcileReport.actions` holds one `ReconcileAction` per outcome (`written`, `updated`, `skipped_current`, `removed`, or `error`), each with `key`, `version`, the resolved `path`, and diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index 97b09862..54f0b2d2 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -47,26 +47,20 @@ _clear_state = skills_core.clear_state -def set_skill_store(store: SkillStore) -> None: +def set_skill_store(store: SkillStore | None) -> None: """ Sets the store the skill accessors and ``write_skills`` read from. Applies on every call, including after ``init_client``, so a lazily - initialized client can be given a store afterwards. ``shutdown()`` clears - it. + initialized client can be given a store afterwards. ``None`` is ignored and + never clears a configured store; ``shutdown()`` does that. Args: store: ``FDv2SkillStore`` to receive skills from LaunchDarkly, or ``InMemorySkillStore`` for local development and tests. - - Raises: - TypeError: If *store* is ``None``. """ - if store is None: - raise TypeError( - "set_skill_store needs a store; shutdown() clears the configured one." - ) - _set_store(store) + if store is not None: + _set_store(store) class InMemorySkillStore: diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 664ce456..112061db 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -759,16 +759,15 @@ async def test_a_second_set_skill_store_replaces_the_store( assert await get_skill("second") is not None assert await get_skill("first") is None - async def test_set_skill_store_refuses_none_and_keeps_the_store( + async def test_set_skill_store_none_leaves_the_configured_one( self, make_raw_skill: Any ) -> None: - """``None`` is refused rather than read as "clear"; ``shutdown()`` clears.""" + """``None`` never clears a store; ``shutdown()`` does that.""" store = InMemorySkillStore() store.put(make_raw_skill(key="a")) set_skill_store(store) - with pytest.raises(TypeError, match="shutdown"): - set_skill_store(None) # type: ignore[arg-type] + set_skill_store(None) assert await get_skill("a") is not None From f5c9aa2cf9f19ee79aa900c582965890af7058f0 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 6 Oct 2026 16:40:22 -0400 Subject: [PATCH 4/5] fix(client): pin the root surface and address review of the experimental move - Pin the root `__all__` to a checked-in snapshot, so a root export such as a `frontmatter` accessor fails a test rather than relying on a denylist. The experimental module's `dir()` is pinned too, not only its `__all__`. - Test that a failure clearing experimental state in `shutdown()` is logged, not raised, and the client is still closed. - `set_skill_store` raises `TypeError` for a value without callable `get_object` and `all_objects`, instead of failing later as `store_unavailable`. Its docstring says replacing a store does not close the old one and a running watcher keeps the store it started with. - Document all seven `init_client` options in the docstring and README. - The README quickstart stops before `write_skills` when `inspect_config` returns no config, since an empty reference list would prune every skill. - `AiConfigRep` points at `experimental.skills.skill_refs`, and the `watch_skills` no-store message names the import path. Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 22 ++- .../src/launchdarkly_ai_server/lifecycle.py | 15 +- .../src/launchdarkly_ai_server/skills.py | 24 ++- .../launchdarkly_ai_server/skills_watch.py | 2 +- .../src/launchdarkly_ai_server/types.py | 3 +- packages/client/tests/test_lifecycle.py | 20 +++ packages/client/tests/test_public_surface.py | 138 ++++++++++++++++++ packages/client/tests/test_skills.py | 24 +++ 8 files changed, 242 insertions(+), 6 deletions(-) create mode 100644 packages/client/tests/test_public_surface.py diff --git a/packages/client/README.md b/packages/client/README.md index 6087f491..3a0ad130 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -200,6 +200,20 @@ asyncio.run(main()) | `shutdown()` | Flush all events and telemetry, then close the client. Await before process exit. | | `inspect_config(key, context)` | Read an AI Config variation without invoking the model. Never raises. Returns `{"enabled", "config", "meta"}`. | +`init_client` reads these option keys. Each overrides the environment variable of the same +purpose (see [Environment Variables](#environment-variables)); any other key is logged as a +warning and ignored. + +| Option | Environment variable | Description | +|---|---|---| +| `sdkKey` | `LD_SDK_KEY` | LaunchDarkly server-side SDK key | +| `baseUri` | `LD_BASE_URI` | Polling base URI. Not used with `client=...` | +| `streamUri` | `LD_STREAM_URI` | Streaming URI. Not used with `client=...` | +| `eventsUri` | `LD_EVENTS_URI` | Events URI. Not used with `client=...` | +| `otlpEndpoint` | `OTEL_EXPORTER_OTLP_ENDPOINT` | OTLP endpoint spans are exported to | +| `serviceName` | `LD_SERVICE_NAME` | OTel `service.name` resource attribute | +| `environment` | `LD_ENVIRONMENT` | `deployment.environment` resource attribute | + ### `config(**args)` The primary entry point for AI config invocations. Accepts either a single handler or a list of handlers and routes to the correct one at invoke-time based on the flag variation's provider and mode. @@ -385,6 +399,10 @@ async def main(): # 1. Which skills does this config reference? Pure projection — no I/O. info = await inspect_config("doc-agent", {"kind": "user", "key": "user-123"}) + if info["config"] is None: + # The config could not be resolved. Stop here: an empty reference list + # passed to write_skills would prune every skill it manages. + return refs = skill_refs(info["config"]) # [SkillReference(key='pdf-extraction', version=2)] # 2. Fetch content. Returns None rather than raising when a skill is unavailable. @@ -717,7 +735,9 @@ that skips verification. | `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `last_error`. | Configure the store with `set_skill_store(store)`. It applies on every call, before or after -`init_client`, and `None` is ignored rather than clearing a configured store. With none +`init_client`, and `None` is ignored rather than clearing a configured store. Anything else +without callable `get_object` and `all_objects` raises `TypeError`. Replacing a store does not +close the previous one, and a running watcher keeps the store it started with. With none configured, the accessors raise `RuntimeError` explaining what to do, and `write_skills` reports the failure (or raises, with `on_unavailable="raise"`). `shutdown()` clears it. diff --git a/packages/client/src/launchdarkly_ai_server/lifecycle.py b/packages/client/src/launchdarkly_ai_server/lifecycle.py index 32d1cf51..e71c4380 100644 --- a/packages/client/src/launchdarkly_ai_server/lifecycle.py +++ b/packages/client/src/launchdarkly_ai_server/lifecycle.py @@ -164,8 +164,21 @@ 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 (each overrides the environment variable in parentheses): + + - ``sdkKey`` (``LD_SDK_KEY``): the server-side SDK key. + - ``baseUri`` (``LD_BASE_URI``), ``streamUri`` (``LD_STREAM_URI``), + ``eventsUri`` (``LD_EVENTS_URI``): LaunchDarkly service URIs, passed to + the SDK config. Not used on the BYOC path. + - ``otlpEndpoint`` (``OTEL_EXPORTER_OTLP_ENDPOINT``): where spans are + exported. + - ``serviceName`` (``LD_SERVICE_NAME``): the ``service.name`` resource + attribute. + - ``environment`` (``LD_ENVIRONMENT``): the ``deployment.environment`` + resource attribute. + Idempotent: later calls return the existing client and ignore every option. - An option this function does not read is logged as a warning and ignored. + An option not listed above is logged as a warning and ignored. Returns the initialized ``LDClientInterface`` instance. """ diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index 54f0b2d2..542b0a4d 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -55,12 +55,32 @@ def set_skill_store(store: SkillStore | None) -> None: initialized client can be given a store afterwards. ``None`` is ignored and never clears a configured store; ``shutdown()`` does that. + Replacing a store does not close the previous one; close it yourself if it + holds a connection. A running ``watch_skills`` keeps listening to the store + it started with, so close the watcher and start a new one to follow the + replacement. + Args: store: ``FDv2SkillStore`` to receive skills from LaunchDarkly, or ``InMemorySkillStore`` for local development and tests. + + Raises: + TypeError: If *store* is not ``None`` and has no callable + ``get_object`` and ``all_objects``. """ - if store is not None: - _set_store(store) + if store is None: + return + missing = [ + name + for name in ("get_object", "all_objects") + if not callable(getattr(store, name, None)) + ] + if missing: + raise TypeError( + f"set_skill_store needs a SkillStore; {type(store).__name__} has no " + f"callable {' or '.join(missing)}." + ) + _set_store(store) class InMemorySkillStore: diff --git a/packages/client/src/launchdarkly_ai_server/skills_watch.py b/packages/client/src/launchdarkly_ai_server/skills_watch.py index f7cd27c5..0283a2d3 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_watch.py +++ b/packages/client/src/launchdarkly_ai_server/skills_watch.py @@ -228,7 +228,7 @@ async def watch_skills( if store is None: raise RuntimeError( "watch_skills needs a configured skill store. Configure one with " - "set_skill_store(store)." + "set_skill_store(store) from launchdarkly_ai_server.experimental.skills." ) add_listener = getattr(store, "add_listener", None) if not callable(add_listener): diff --git a/packages/client/src/launchdarkly_ai_server/types.py b/packages/client/src/launchdarkly_ai_server/types.py index c4d27baa..faf34e66 100644 --- a/packages/client/src/launchdarkly_ai_server/types.py +++ b/packages/client/src/launchdarkly_ai_server/types.py @@ -93,7 +93,8 @@ class Message: """ Raw AI config dict as returned by ``parse_ai_config``. Fields include ``model``, ``provider``, at least one of ``instructions`` / ``messages``, and an -optional ``skills`` array of ``{key, version}`` references (see ``skill_refs``). +optional ``skills`` array of ``{key, version}`` references (see +``launchdarkly_ai_server.experimental.skills.skill_refs``). """ VariationMeta = dict[str, Any] diff --git a/packages/client/tests/test_lifecycle.py b/packages/client/tests/test_lifecycle.py index e4b7c1b1..7ad6a5ba 100644 --- a/packages/client/tests/test_lifecycle.py +++ b/packages/client/tests/test_lifecycle.py @@ -525,6 +525,26 @@ async def test_idempotent_double_shutdown(self) -> None: await shutdown() await shutdown() # must not raise + async def test_a_failure_clearing_experimental_state_is_logged_not_raised( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Experimental behaviour must not break a core call (TESTING.md §0.3).""" + stub = _make_stub_client() + with patch.object(lifecycle_module, "_setup_telemetry", return_value=None): + await init_client(client=stub) + with ( + patch.object( + lifecycle_module.skills, + "_clear_state", + side_effect=RuntimeError("clear failed"), + ), + caplog.at_level("WARNING", logger="launchdarkly_ai_server.lifecycle"), + ): + await shutdown() # must not raise + stub.close.assert_called_once() + assert lifecycle_module._client is None + assert "Could not clear the Agent Skills state" in caplog.messages + async def test_completes_teardown_even_if_flush_throws(self) -> None: stub = _make_stub_client() stub.flush = AsyncMock(side_effect=RuntimeError("flush failed")) diff --git a/packages/client/tests/test_public_surface.py b/packages/client/tests/test_public_surface.py new file mode 100644 index 00000000..df1fd49f --- /dev/null +++ b/packages/client/tests/test_public_surface.py @@ -0,0 +1,138 @@ +""" +The package root's public surface. + +``ROOT_SURFACE`` is the checked-in snapshot of ``launchdarkly_ai_server.__all__`` +(TESTING.md §0.3). It is spelled out so that any change to the root, including +an experimental name leaking onto it, fails here and is seen in review. Update +it deliberately, in the same change that adds or removes a root export. +""" + +from __future__ import annotations + +ROOT_SURFACE = frozenset( + { + "AIConfig", + "AiConfigRep", + "ConfigInstance", + "ConversationIdSpanProcessor", + "Criterion", + "DatasetRow", + "EvalRunResult", + "EvaluationsError", + "EvaluationsModule", + "ExecuteStreamDoneEvent", + "ExecuteStreamEvent", + "GenerationConfig", + "GraphArgs", + "GraphDefinition", + "GraphEdge", + "GraphInstance", + "GraphNode", + "GraphOptions", + "GraphStreamEvent", + "GraphTopology", + "HandlerResult", + "HandlerStreamEvent", + "InitClientOptions", + "InputTokenDetails", + "Judge", + "JudgeResult", + "JudgeRunResult", + "JudgeTask", + "LDClientInterface", + "LDContext", + "Message", + "NATIVE_TOOL_KEY", + "NativeTool", + "ParseFailure", + "ParseResult", + "ParseSuccess", + "ProviderGraphResponse", + "ProviderHandler", + "ProviderResponse", + "Registry", + "RunSummary", + "RunUsage", + "SDK_INFO_CONTEXT", + "SDK_INFO_EVENT", + "Scorer", + "SpanMessage", + "SpanMessagePart", + "SpanUsage", + "StreamChunkEvent", + "StreamDoneEvent", + "StreamEvent", + "ToolDefinitionInput", + "TrackData", + "UsageDict", + "VariationMeta", + "add_cached_tokens_to_input", + "any_multimodal", + "build_judge_tasks", + "compose", + "compose_history", + "config", + "content_to_text", + "conversation_id", + "create_handler", + "create_run_usage", + "end_span_once", + "end_unfinished_spans", + "execute_and_stream", + "execute_and_track", + "extract_variation", + "get_client", + "global_registry", + "graph", + "has_multimodal_content", + "image_block_to_url", + "init_client", + "init_evaluations", + "inspect_config", + "is_content_blocks", + "lang_chain_content_text", + "lang_chain_finish_reasons", + "lang_chain_span_messages", + "lang_chain_span_usage", + "make_track_data", + "model_stamps_from_meta", + "normalize_mode", + "number_or_zero", + "omit_model_stamps", + "parse_ai_config", + "parse_json_with_possible_fences", + "parse_template", + "parse_usage", + "register_ai_sdk_package", + "resolve_graph", + "resolve_handlers", + "resolve_tools", + "run_judge", + "run_judges", + "set_conversation_id_if_absent", + "set_input_content_attributes", + "set_ld_span_attributes", + "set_model_identity_attributes", + "set_openllmetry_completion", + "set_openllmetry_prompt", + "set_output_content_attributes", + "set_tool_call_content_attributes", + "set_tool_definition_attributes", + "set_usage_span_attributes", + "shutdown", + "text_message", + "to_ld_context", + "to_semconv_finish_reason", + "to_usage_dict", + "wrap_tool_handlers", + } +) + + +def test_root_surface_is_exactly_the_snapshot() -> None: + import launchdarkly_ai_server as package + + assert set(package.__all__) == ROOT_SURFACE + assert len(package.__all__) == len(ROOT_SURFACE), "duplicate name in __all__" + for name in ROOT_SURFACE: + assert hasattr(package, name), name diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 112061db..18c3fa12 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -372,6 +372,14 @@ def test_experimental_surface_is_exactly_the_documented_set(self) -> None: for name in EXPERIMENTAL_SKILLS_SURFACE: assert hasattr(experimental, name), name + def test_experimental_module_defines_no_other_public_name(self) -> None: + """``__all__`` is what ``import *`` and the docs follow, but a public + name defined on the module and left out of it is still importable.""" + from launchdarkly_ai_server.experimental import skills as experimental + + public = {name for name in dir(experimental) if not name.startswith("_")} + assert public == EXPERIMENTAL_SKILLS_SURFACE + def test_no_skills_name_is_exported_from_the_package_root(self) -> None: import launchdarkly_ai_server as package import launchdarkly_ai_server.experimental.skills @@ -771,6 +779,22 @@ async def test_set_skill_store_none_leaves_the_configured_one( assert await get_skill("a") is not None + @pytest.mark.parametrize("not_a_store", ["x", {}, 0, object()]) + async def test_set_skill_store_rejects_a_non_store( + self, make_raw_skill: Any, not_a_store: Any + ) -> None: + """A value without the store methods fails where it is passed, not as a + ``store_unavailable`` on the first accessor call, and leaves the + configured store in place.""" + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + set_skill_store(store) + + with pytest.raises(TypeError, match="SkillStore"): + set_skill_store(not_a_store) + + assert await get_skill("a") is not None + async def test_init_client_ignores_a_skill_store_option( self, make_raw_skill: Any, caplog: pytest.LogCaptureFixture ) -> None: From 0f303f4bed53c4c597bb284e5414fcca4a831735 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Wed, 7 Oct 2026 11:20:58 -0400 Subject: [PATCH 5/5] fix(client): validate skills in skill_refs, not in the config parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parse_ai_config` rejected a whole AI Config when its `skills` field was malformed, so a bad experimental field failed `config().invoke()`, `stream()`, graph nodes and `extract_variation` for apps that never use Agent Skills. That breaks TESTING.md §0.3: experimental behaviour must not break a core call. The parser now passes `skills` through unchecked. `skill_refs` validates it instead and raises `ValueError` when the field is present but malformed, including `skills: null` and a single bad entry. It used to drop bad entries with a warning, which was safe only because the parser had already rejected them. Raising keeps the original reason for the check: `write_skills` with `prune=True` never receives an empty or partial list for a field that is present. An absent field, or a config that is not a dict, still returns `[]`. Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 13 +- packages/client/agents.md | 10 +- .../src/launchdarkly_ai_server/skills.py | 60 +++------ .../src/launchdarkly_ai_server/types.py | 5 +- .../types_validation.py | 18 ++- packages/client/tests/test_lifecycle.py | 27 ++++ packages/client/tests/test_schema.py | 118 ++++-------------- packages/client/tests/test_skills.py | 72 +++++++---- packages/client/tests/test_skills_fs.py | 10 +- 9 files changed, 149 insertions(+), 184 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 3a0ad130..22aec481 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -425,11 +425,12 @@ project library**, which puts every skill's `description` into the agent's conte skills no AI Config references and skills belonging to other teams. `write_skills(skill_refs(...), root)`, as above, writes only what the resolved variation asked for. -**`skills` is a validated field.** Config parsing fails closed on a `skills` value that is not -a list of `{key, version}` objects (key matching `^[a-z0-9][a-z0-9-]*$`, version an integer -≥ 1): the whole variation is rejected, `inspect_config` returns `config: None`, and -`extract_variation` raises. If your variations carry a custom `skills` field of a different -shape, rename it before upgrading. +**`skill_refs` validates the `skills` field.** It raises `ValueError` when `skills` is present +but is not a list of `{key, version}` objects (key matching `^[a-z0-9][a-z0-9-]*$`, version an +integer ≥ 1), including `skills: null`. One bad entry rejects the whole field, so +`write_skills` never receives a partial list that would prune skills the config still +references. Config parsing does not check `skills`, so a malformed field never fails +`config().invoke()` or other core calls. **Integrity is not optional.** Content is returned only when its sha256 (lowercase hex, over the verbatim UTF-8 bytes) matches the delivered `contentHash`, its key and version revalidate, @@ -722,7 +723,7 @@ that skips verification. | Export | Description | |---|---| -| `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when the field is absent. A `skills` field that is present but not a list (including `null`) fails the config parse instead, so an unreadable field never reaches a pruning reconcile as "no skills". | +| `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when the field is absent or the config is not a dict. Raises `ValueError` when the field is present but malformed (including `null`), so an unreadable field never reaches a pruning reconcile as "no skills". | | `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_skill_result(key, *, version=None)` | The same retrieval, reporting **why**: a frozen `SkillOutcome` with `.skill`, `.reason` (`ok` / `absent` / `integrity_failure` / `store_unavailable` / `wrong_version`), and `.detail`. See *Failing closed on tampering* above. 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. | diff --git a/packages/client/agents.md b/packages/client/agents.md index 3797aa01..29eb25f0 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -205,8 +205,10 @@ Three layers, in increasing order of blast radius: 1. **Reference discovery** — `skill_refs(config)` projects the config's `skills` array into 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. + It also validates the array, and **fails closed**: a present but malformed field + (including `null`, or one bad entry) raises `ValueError` rather than returning a partial + list that would authorize a prune. `parse_ai_config` deliberately does not check `skills`, + so an experimental field cannot fail a core config call (TESTING.md §0.3). 2. **Content accessors** — `get_skill`, `get_skill_result`, `get_skills`, `all_skills` read through the `SkillStore` seam. Configure a store with `set_skill_store(store)`; with none configured the accessors raise @@ -527,8 +529,8 @@ Store data is **untrusted input**; the transport is not part of the trust bounda - **Those two bounds live in `_key_rejection_reason`, not in the key grammar, and must not move.** `is_valid_skill_key` / `skill_key_rejection_reason` deliberately admit an over-long or reserved key, because: - - `parse_ai_config` fails closed on a bad `skills` entry, so a grammar rejection would - invalidate the *entire* AI Config for a Linux customer over a Windows-only constraint; + - `skill_refs` fails closed on a bad `skills` entry, so a grammar rejection would + reject *every* skill reference for a Linux customer over a Windows-only constraint; - it would also shrink `skill_refs`, which authorizes a prune, turning "fails to write on Windows" into "deleted on Linux". diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index 542b0a4d..c02e1a6c 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -28,11 +28,7 @@ verify_raw_skill, ) from .types import AiConfigRep, Skill, SkillOutcome, SkillReference -from .types_validation import ( - is_valid_skill_key, - is_valid_skill_version, - skill_key_rejection_reason, -) +from .types_validation import is_valid_skill_version, skills_field_rejection_reason logger = logging.getLogger(__name__) @@ -208,46 +204,28 @@ def skill_refs(config: AiConfigRep | None) -> list[SkillReference]: Returns the skill references attached to a resolved AI Config. Pure: no network, store, or telemetry. Returns ``[]`` when the config has no - skills. Typical use: ``await get_skills(skill_refs(config))``. + ``skills`` field, or when *config* is not a dict (for example ``None`` from a + failed ``inspect_config``). Typical use: ``await get_skills(skill_refs(config))``. - Invalid entries (possible only in a hand-built dict; ``parse_ai_config`` - rejects them) are dropped with a warning, because ``write_skills`` with - ``prune=True`` would delete a dropped skill's files. - """ - if not isinstance(config, dict): - return [] + The config parser does not validate ``skills``, so a malformed field does + not fail core config calls. It is validated here instead, and rejected + whole: ``write_skills`` with ``prune=True`` would delete the files of any + skill missing from the list, so a partial or empty list is never returned + for a field that is present. - raw = config.get("skills") - if not isinstance(raw, list): + Raises: + ValueError: If ``skills`` is present but is not a list of ``{key, + version}`` objects with a valid key and an integer version >= 1. + This includes ``skills: null``. + """ + if not isinstance(config, dict) or "skills" not in config: return [] - refs: list[SkillReference] = [] - for index, entry in enumerate(raw): - if not isinstance(entry, dict): - logger.warning( - "skills[%d] is not a {key, version} object; it was dropped " - "from the projection", - index, - ) - continue - key = entry.get("key") - version = entry.get("version") - # Branch on the TypeGuard so ``key`` narrows to ``str``. - if not is_valid_skill_key(key): - logger.warning( - "skills[%d].key %s; it was dropped from the projection", - index, - skill_key_rejection_reason(key), - ) - elif not is_valid_skill_version(version): - logger.warning( - "skills[%d].version must be an integer >= 1; it was dropped " - "from the projection", - index, - ) - else: - refs.append(SkillReference(key=key, version=version)) - return refs + raw = config["skills"] + rejection = skills_field_rejection_reason(raw) + if rejection is not None: + raise ValueError(f"Invalid skills field in AI Config: {rejection}") + return [SkillReference(key=entry["key"], version=entry["version"]) for entry in raw] # --------------------------------------------------------------------------- diff --git a/packages/client/src/launchdarkly_ai_server/types.py b/packages/client/src/launchdarkly_ai_server/types.py index faf34e66..44c876fc 100644 --- a/packages/client/src/launchdarkly_ai_server/types.py +++ b/packages/client/src/launchdarkly_ai_server/types.py @@ -93,8 +93,9 @@ class Message: """ Raw AI config dict as returned by ``parse_ai_config``. Fields include ``model``, ``provider``, at least one of ``instructions`` / ``messages``, and an -optional ``skills`` array of ``{key, version}`` references (see -``launchdarkly_ai_server.experimental.skills.skill_refs``). +optional ``skills`` array of ``{key, version}`` references. Parsing does not +validate ``skills``; read it with +``launchdarkly_ai_server.experimental.skills.skill_refs``, which does. """ VariationMeta = dict[str, Any] diff --git a/packages/client/src/launchdarkly_ai_server/types_validation.py b/packages/client/src/launchdarkly_ai_server/types_validation.py index c70771e2..71702693 100644 --- a/packages/client/src/launchdarkly_ai_server/types_validation.py +++ b/packages/client/src/launchdarkly_ai_server/types_validation.py @@ -63,12 +63,13 @@ def _parse_tool(raw: Any, key: str) -> str | None: return None -def _parse_skills(raw: Any) -> str | None: +def skills_field_rejection_reason(raw: Any) -> str | None: """ - Validates the optional ``skills`` array. Returns an error message or ``None``. + Why a present ``skills`` field is malformed, or ``None`` when it is valid. - Fails closed: one malformed reference fails the whole config, rather than - silently materializing a partial skill set. + Used by ``skill_refs``, not by ``parse_ai_config``: Agent Skills is + experimental, and a malformed field must not fail a core config call. + One malformed reference rejects the whole field, never a partial list. """ if not isinstance(raw, list): return "skills must be an array of {key, version} objects" @@ -148,11 +149,8 @@ def parse_ai_config(raw: Any) -> ParseResult: error={"message": "outputFormat must be an object (JSON Schema)"}, ) - # ``in``, not ``is not None``: ``skills: null`` must fail the parse. Read as - # "no skills", a ``prune=True`` reconcile would delete skill files on disk. - if "skills" in raw: - err = _parse_skills(raw["skills"]) - if err: - return ParseFailure(success=False, error={"message": err}) + # ``skills`` is passed through unvalidated. Agent Skills is experimental, so + # a malformed field must not fail a core config call (TESTING.md §0.3); + # ``skill_refs`` rejects it where the references are used. return ParseSuccess(success=True, data=raw) diff --git a/packages/client/tests/test_lifecycle.py b/packages/client/tests/test_lifecycle.py index 7ad6a5ba..c1469868 100644 --- a/packages/client/tests/test_lifecycle.py +++ b/packages/client/tests/test_lifecycle.py @@ -785,6 +785,33 @@ async def test_returns_enabled_true_and_config_when_variation_is_enabled( assert result["config"]["model"]["name"] == "claude-3-5" # type: ignore[index] assert result["meta"] is not None + async def test_a_malformed_skills_field_does_not_fail_core_calls(self) -> None: + """Agent Skills is experimental, so its field cannot break a core call + (TESTING.md §0.3). ``skill_refs`` rejects it instead.""" + stub = _make_stub_client() + stub.variation = AsyncMock( + return_value={ + "_ldMeta": {"enabled": True, "variationKey": "v1", "version": 1}, + "model": {"name": "claude-3-5"}, + "provider": {"name": "Anthropic"}, + "instructions": "You are helpful.", + "skills": [{"key": "My_Skill", "version": 0}], + } + ) + with patch.object(lifecycle_module, "_setup_telemetry", return_value=None): + await init_client(client=stub) + ctx = {"kind": "user", "key": "user-1"} + with patch( + "launchdarkly_ai_server.utils.to_ld_context", + side_effect=lambda _c, ctx: ctx, + ): + result = await inspect_config("my-flag", ctx) + extracted = await lifecycle_module.extract_variation("my-flag", ctx) + + assert result["enabled"] is True + assert result["config"] is not None + assert extracted["config"]["model"]["name"] == "claude-3-5" + async def test_preserves_model_key_and_version_on_meta(self) -> None: stub = _make_stub_client() stub.variation = AsyncMock( diff --git a/packages/client/tests/test_schema.py b/packages/client/tests/test_schema.py index 359a8904..7c4d978b 100644 --- a/packages/client/tests/test_schema.py +++ b/packages/client/tests/test_schema.py @@ -86,7 +86,11 @@ def test_output_format_accepted(self) -> None: class TestParseAiConfigSkills: """ - Fail-closed validation of the optional ``skills`` array. + ``skills`` is passed through unvalidated. + + Agent Skills is experimental, so a malformed ``skills`` field must not fail + a core config call (TESTING.md §0.3). ``skill_refs`` validates the field + where the references are used; see ``TestSkillRefs`` in test_skills.py. """ def _base(self, **extra: Any) -> dict[str, Any]: @@ -101,105 +105,31 @@ def _base(self, **extra: Any) -> dict[str, Any]: def test_absent_skills_is_valid(self) -> None: assert parse_ai_config(self._base()).success is True - def test_empty_skills_is_valid(self) -> None: - assert parse_ai_config(self._base(skills=[])).success is True - - def test_valid_entries_accepted(self) -> None: + def test_valid_entries_pass_through(self) -> None: raw = self._base(skills=[{"key": "pdf-extraction", "version": 2}]) result = parse_ai_config(raw) assert result.success is True assert result.data["skills"] == [{"key": "pdf-extraction", "version": 2}] - def test_multiple_valid_entries_accepted(self) -> None: - raw = self._base( - skills=[{"key": "a", "version": 1}, {"key": "b-2", "version": 10}] - ) - assert parse_ai_config(raw).success is True - - def test_key_at_length_bound_accepted(self) -> None: - raw = self._base(skills=[{"key": "a" * 256, "version": 1}]) - assert parse_ai_config(raw).success is True - - @pytest.mark.parametrize("bad_skills", ["pdf", {"key": "a"}, 3, True]) - def test_non_array_skills_fails(self, bad_skills: Any) -> None: - assert parse_ai_config(self._base(skills=bad_skills)).success is False - - def test_an_explicit_null_skills_fails_rather_than_reading_as_absent( - self, - ) -> None: - """``skills: null`` is the non-array that reads as "no skills". - - It must not be treated that way. Read as absent it makes ``skill_refs`` - return ``[]``, and a ``prune=True`` reconcile then *deletes* previously - materialized skill files on the strength of a field the SDK could not - parse — the same hazard the store path refuses, where reading a - malformed object as absent would let prune delete the last known-good - copy on disk. Failing the whole parse is the louder and safer outcome, - and it is why this case is pinned apart from the other non-arrays. - """ - result = parse_ai_config(self._base(skills=None)) - assert result.success is False - assert "skills" in result.error["message"] - - def test_an_absent_skills_key_is_still_valid(self) -> None: - """The other half of the bullet above: *absent* is not *null*. - - Rejecting ``null`` must not cost backward compatibility for the - configs that simply have no ``skills`` field. - """ - raw = self._base() - assert "skills" not in raw - assert parse_ai_config(raw).success is True - - @pytest.mark.parametrize("entry", ["pdf-extraction", 1, None, ["a", 1]]) - def test_non_object_entry_fails(self, entry: Any) -> None: - assert parse_ai_config(self._base(skills=[entry])).success is False - - @pytest.mark.parametrize("bad_key", [None, 1, True, {"a": 1}, ["a"]]) - def test_missing_or_non_string_key_fails(self, bad_key: Any) -> None: - raw = self._base(skills=[{"key": bad_key, "version": 1}]) - assert parse_ai_config(raw).success is False - - def test_absent_key_fails(self) -> None: - assert parse_ai_config(self._base(skills=[{"version": 1}])).success is False - @pytest.mark.parametrize( - "bad_key", + "malformed", [ - "", - "Evil", - "-leading-dash", - ".hidden", - "_underscore", - "has space", - "a/b", - "a\\b", - "../escape", - "trailing-space ", - "under_score", - "a" * 257, + None, + "pdf", + {"key": "a"}, + 3, + ["pdf-extraction"], + [{"version": 1}], + [{"key": "Evil", "version": 1}], + [{"key": "../escape", "version": 1}], + [{"key": "a", "version": 0}], + [{"key": "a"}], + [{"key": "good", "version": 1}, {"key": "My_Skill", "version": 1}], ], ) - def test_pattern_and_length_violations_fail(self, bad_key: str) -> None: - raw = self._base(skills=[{"key": bad_key, "version": 1}]) - assert parse_ai_config(raw).success is False - - @pytest.mark.parametrize("bad_version", [0, -1, 2.5, "2", None, True, [1]]) - def test_invalid_version_fails(self, bad_version: Any) -> None: - raw = self._base(skills=[{"key": "a", "version": bad_version}]) - assert parse_ai_config(raw).success is False - - def test_absent_version_fails(self) -> None: - assert parse_ai_config(self._base(skills=[{"key": "a"}])).success is False - - def test_one_bad_entry_fails_the_whole_config(self) -> None: - raw = self._base( - skills=[{"key": "good", "version": 1}, {"key": "../bad", "version": 1}] - ) - assert parse_ai_config(raw).success is False - - def test_error_message_mentions_skills(self) -> None: - raw = self._base(skills=[{"key": "../bad", "version": 1}]) - result = parse_ai_config(raw) - assert result.success is False - assert "skills" in result.error["message"] + def test_a_malformed_skills_field_does_not_fail_the_parse( + self, malformed: Any + ) -> None: + result = parse_ai_config(self._base(skills=malformed)) + assert result.success is True + assert result.data["skills"] == malformed diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 18c3fa12..41a288ab 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -286,31 +286,59 @@ def test_emits_no_telemetry(self, recording_emitter: Any) -> None: 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. - - ``parse_ai_config`` fails the whole config closed on a malformed - reference, so a config that reached here through it cannot contain one. - A hand-built dict can, and feeding the shortened list to - ``write_skills`` would prune the dropped skill's on-disk copy — so the - drop is observable rather than silent. + def test_a_non_dict_config_returns_empty_list(self) -> None: + assert skill_refs(None) == [] + + @pytest.mark.parametrize( + "malformed", + [ + pytest.param(None, id="null"), + pytest.param("pdf", id="string"), + pytest.param({"key": "a", "version": 1}, id="object"), + pytest.param(3, id="number"), + pytest.param(["pdf-extraction"], id="non-object-entry"), + pytest.param([None], id="null-entry"), + pytest.param([{"version": 1}], id="absent-key"), + pytest.param([{"key": 1, "version": 1}], id="non-string-key"), + pytest.param([{"key": "", "version": 1}], id="empty-key"), + pytest.param([{"key": "Evil", "version": 1}], id="uppercase-key"), + pytest.param([{"key": "under_score", "version": 1}], id="underscore-key"), + pytest.param([{"key": "../escape", "version": 1}], id="traversal-key"), + pytest.param([{"key": "a" * 257, "version": 1}], id="long-key"), + pytest.param([{"key": "a"}], id="absent-version"), + pytest.param([{"key": "a", "version": 0}], id="zero-version"), + pytest.param([{"key": "a", "version": 2.5}], id="float-version"), + pytest.param([{"key": "a", "version": "2"}], id="string-version"), + pytest.param([{"key": "a", "version": True}], id="bool-version"), + ], + ) + def test_a_malformed_skills_field_raises(self, malformed: Any) -> None: + """The parser passes ``skills`` through, so it is validated here. + + Returning ``[]`` or a shortened list instead would authorize + ``write_skills(..., prune=True)`` to delete skills the config still + references. ``skills: null`` is included: read as "no skills" it would + prune everything. """ + with pytest.raises(ValueError, match=r"skills"): + skill_refs(self._config(skills=malformed)) + + def test_one_bad_entry_rejects_the_whole_field(self) -> None: config = self._config( - skills=[ - {"key": "good", "version": 1}, - {"key": "bad", "version": 0}, - {"key": "Bad-Key", "version": 1}, - "not-an-object", - ] + skills=[{"key": "good", "version": 1}, {"key": "../bad", "version": 1}] ) - - with caplog.at_level("WARNING", logger="launchdarkly_ai_server.skills"): - refs = skill_refs(config) - - assert refs == [SkillReference(key="good", version=1)] - assert len(caplog.records) == 3 - # The body is never echoed, and neither is the invalid key. - assert all("skills[" in r.getMessage() for r in caplog.records) + with pytest.raises(ValueError, match=r"skills\[1\]\.key"): + skill_refs(config) + + def test_the_error_does_not_echo_the_rejected_key(self) -> None: + config = self._config(skills=[{"key": "Secret-Name", "version": 1}]) + with pytest.raises(ValueError) as excinfo: + skill_refs(config) + assert "Secret-Name" not in str(excinfo.value) + + def test_key_at_length_bound_is_accepted(self) -> None: + refs = skill_refs(self._config(skills=[{"key": "a" * 256, "version": 1}])) + assert refs == [SkillReference(key="a" * 256, version=1)] def test_requires_no_client_or_store(self, mock_ld_client: Any) -> None: """No store configured, no client initialized — still a pure projection.""" diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index e23c5527..1ecdeefc 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -3563,11 +3563,11 @@ def test_a_reserved_name_is_still_a_valid_key_to_every_pure_layer( """The layer choice, asserted — this is the whole point of it. The constraint lives in the filesystem layer and must not migrate into - the key grammar. At the grammar level a rejection would fail the *entire* - AI Config — model, provider, instructions, tools — for a Linux customer - over a Windows-only constraint, and would shrink ``skill_refs``, which is - what authorizes a prune: "this skill fails to write on Windows" would - become "this skill gets deleted on Linux". + the key grammar. At the grammar level a rejection would make + ``skill_refs`` reject *every* reference for a Linux customer over a + Windows-only constraint, and a grammar that dropped entries instead + would shrink the list that authorizes a prune: "this skill fails to + write on Windows" would become "this skill gets deleted on Linux". """ assert is_valid_skill_key(reserved) is True