From eda4ae1f9219a8a3c3b363dcfc05fee566820825 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Mon, 31 Aug 2026 14:04:18 -0400 Subject: [PATCH] =?UTF-8?q?feat(client):=20Agent=20Skills=20=E2=80=94=20a?= =?UTF-8?q?=20distinguishable=20outcome=20for=20integrity=20failure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``get_skill`` returns ``None`` for four unrelated outcomes: no such skill, the store raised, the requested version is not the one held, and content that failed hash verification. A caller cannot fail closed on suspected tampering while tolerating a merely-absent skill, so no automated customer-side response is possible — finding LA-2 of the Agent Skills security design review. The information already existed internally, as prose in ``Resolution.error``. This gives it a token: ``Resolution`` grows a typed ``reason``, set explicitly at every construction site and declared without a default so a sixth outcome added later has to choose which public token it maps to. ``get_skill_result`` maps that straight through to a frozen ``SkillOutcome`` (``skill``, ``reason``, ``detail``). Deriving the public reason by matching the error string is the fragility LA-2 is about, so the mapping is readable in one table. ``get_skill`` is untouched — its ``None``-for-every-failure contract is documented in its docstring and in the README, and a test now pins that all four failures still collapse to ``None`` and still never raise. Nothing new is emitted: Gap 1's integrity record already fired inside verification before ``resolve_from_store`` returned, and a test asserts one failed retrieval still produces exactly one record and one signal. Co-Authored-By: Claude Opus 5 --- packages/client/README.md | 56 +++- packages/client/agents.md | 59 +++- .../src/launchdarkly_ai_server/__init__.py | 8 +- .../src/launchdarkly_ai_server/skills.py | 36 ++- .../src/launchdarkly_ai_server/skills_core.py | 39 ++- .../src/launchdarkly_ai_server/skills_fs.py | 10 +- .../src/launchdarkly_ai_server/types.py | 55 ++++ packages/client/tests/test_skills.py | 281 ++++++++++++++++++ 8 files changed, 527 insertions(+), 17 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 711ce5b..045180b 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -337,6 +337,55 @@ truncated payload; a mismatch means content was delivered whose bytes are not th LaunchDarkly hashed, which is a possible **active-tampering** signal. Alert on it, and treat `expected_hash` / `observed_hash` as the evidence pair. +#### Failing closed on tampering + +The log record above is the operator's surface. `get_skill_result` is the application's: +same retrieval, same verification, same telemetry as `get_skill`, and it reports which of +five outcomes happened instead of collapsing all of them to `None`. + +```python +from launchdarkly_ai_server import get_skill_result + +outcome = await get_skill_result("pdf-extraction") + +if outcome.reason == "integrity_failure": + # Content was delivered and did not verify. Do not degrade quietly. + raise SystemExit(f"refusing to start: {outcome.detail}") + +if outcome.reason == "store_unavailable": + # The store could not answer at all. Retry, alert, or carry on with what + # you already have — but this is an outage, not a revocation. + print(f"skill retrieval unavailable: {outcome.detail}") +elif outcome.reason in ("absent", "wrong_version"): + # Nothing was tampered with — this skill is simply not available to you. + print(f"continuing without a skill: {outcome.detail}") +elif outcome.skill is not None: + print(outcome.skill.content) +``` + +| `reason` | Meaning | +|---|---| +| `ok` | A verified skill was returned; `.skill` is set and `.detail` is `None`. | +| `absent` | The store answered, and does not hold that key. | +| `integrity_failure` | Content was delivered and failed verification, so it was withheld. **The one to fail closed on.** | +| `store_unavailable` | The store itself could not answer — it raised. An outage, not a deletion. | +| `wrong_version` | The store answered with a version other than the one asked for, so the answer was withheld. | + +`.detail` is human-readable and safe to log or show an operator — it names the key and the +failure mode, and never carries skill content or a filesystem path. Branch on `.reason`, +not on `.detail`. `.skill` is populated only when `.reason == "ok"`. `SkillOutcome` is +frozen, like every other value type here. + +**`get_skill` is unchanged.** It still returns `None` for all four failures and still never +raises for one, so no existing caller has to move. The two accessors run the same code path +and differ only in what they report — `get_skill_result` adds no second log record and no +second signal for a failure that already emitted one, so a caller can switch to it without +double-counting anything. + +`get_skills` and `all_skills` have no reported form: they still omit entries that could not +be resolved, and a run that omitted anything logs a count at WARN. Retrieve individually +with `get_skill_result` when you need the reason per key. + **Versions are selected, not filtered.** A store may hold several versions of one key at once, because a delivery payload does: the newest version of every skill, plus every version a variation currently pins. `get_skill("k", version=1)` asks the store for version @@ -387,6 +436,7 @@ Windows. |---|---| | `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when absent. | | `get_skill(key, *, version=None)` | One verified skill, or `None`. `version=None` means newest available; a specific `version` matches exactly. Raises only when no store is configured. | +| `get_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`. Use it to fail closed on tampering — 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. | | `all_skills()` | Every verified skill the store holds, one per key at its newest version. | | `write_skills(skills, root, *, prune=True, timeout=10.0, on_unavailable="keep")` | Materialize skills under `root`, returning a `ReconcileReport`. `prune` removes formerly-managed skills no longer requested. `on_unavailable="raise"` raises instead of reporting when content cannot be retrieved. Raises `ValueError` for an unusable root, a negative `timeout`, or an unrecognised `on_unavailable`. **Performs synchronous filesystem I/O — see the note below.** | @@ -406,9 +456,10 @@ manifest, for instance — carries the empty string as its `key`. The fixed on-disk values are exported too, so you do not have to hardcode them: `MANIFEST_FILENAME` (`.launchdarkly-skills.json`, handy for a `.gitignore`), -`SKILL_FILENAME`, and `MANIFEST_VERSION`. So are the two closed-set types, for annotating +`SKILL_FILENAME`, and `MANIFEST_VERSION`. So are the three closed-set types, for annotating your own helpers: `ReconcileActionKind` (`written` / `updated` / `skipped_current` / -`removed` / `error`) and `OnUnavailable` (`keep` / `raise`). +`removed` / `error`), `OnUnavailable` (`keep` / `raise`), and `SkillOutcomeReason` +(`absent` / `integrity_failure` / `ok` / `store_unavailable` / `wrong_version`). **`write_skills` blocks.** It is `async` for parity with the other accessors and with the TypeScript SDK, but it awaits nothing: every read, write, `fsync` and rename runs inline, @@ -460,5 +511,6 @@ All types are exported from this package. Handler packages import them from here | `GraphTopology` | The parsed graph flag shape (`root` + `edges`) | | `Skill` | A frozen skill document: `.key`, `.version`, `.content` (verified verbatim `bytes`), `.content_hash`, `.name?`, `.description?` | | `SkillReference` | A frozen version-pinned pointer to a skill: `.key`, `.version` | +| `SkillOutcome` | A frozen retrieval outcome: `.skill`, `.reason` (`SkillOutcomeReason`), `.detail` | | `ReconcileAction` | One `write_skills` outcome: `.key`, `.action`, `.version?`, `.path?`, `.error?` | | `ReconcileReport` | The `write_skills` result: `.actions`, `.ok`, and `.errors` | diff --git a/packages/client/agents.md b/packages/client/agents.md index cd99b28..a7b4ce1 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -77,10 +77,10 @@ from launchdarkly_ai_server import config, graph, resolve_graph # Agent Skills from launchdarkly_ai_server import ( - skill_refs, get_skill, get_skills, all_skills, write_skills, - SkillStore, InMemorySkillStore, + skill_refs, get_skill, get_skill_result, get_skills, all_skills, write_skills, + SkillStore, InMemorySkillStore, SkillOutcome, SKILL_FILENAME, MANIFEST_FILENAME, MANIFEST_VERSION, - ReconcileActionKind, OnUnavailable, # the two closed-set unions + ReconcileActionKind, OnUnavailable, SkillOutcomeReason, # the three closed-set unions ) ``` @@ -188,8 +188,8 @@ Three layers, in increasing order of blast radius: typed `SkillReference` values. Pure: no network, no client, no store, no telemetry. Validation of the array itself lives in `parse_ai_config` and is **fail closed** — one malformed reference fails the whole config parse. -2. **Content accessors** — `get_skill`, `get_skills`, `all_skills` read through the - `SkillStore` seam. Configure a store with +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 an actionable `RuntimeError`. A delivery transport can be added behind the seam without touching the public API. @@ -217,6 +217,55 @@ one place that collapses the result to one object per key, because both whole-st consumers need it — `all_skills`, since a list holding two versions of one key is not a set of skills, and the `"*"` reconcile, since `//SKILL.md` is a single path. +### The reported outcome vocabulary, and the `Resolution` mapping + +`get_skill` returns `Skill | None`; `get_skill_result` returns a frozen `SkillOutcome` +(`skill`, `reason`, `detail`) naming *which* outcome happened. Both are +`resolve_from_store` — one retrieval, one verification, one telemetry pass — and they differ +only in what they report. `get_skill`'s contract is load-bearing and **frozen**: `None` for +every failure, never raises for one, documented in its docstring and in the README. Change +it and every caller that treats `None` as "no skill" breaks silently. + +`SkillOutcomeReason` is five tokens, listed alphabetically for the same reason +`IntegrityReasonCode` is — so the vocabulary reads identically in the Python and TypeScript +SDKs, where the type name, the accessor name, and the tokens are all deliberately the same. +Do not rename one on one side. + +Internal `Resolution.reason` maps 1:1 onto it, set explicitly at every construction site: + +| `resolve_from_store` outcome | `reason` | +|---|---| +| the store raised (`unavailable=True`) | `store_unavailable` | +| `raw` is not a dict | `absent` | +| `verify_raw_skill` returned `None` | `integrity_failure` | +| `skill.version != wanted_version` | `wrong_version` | +| success | `ok` | + +**Adding a sixth internal outcome means choosing which public token it maps to.** +`Resolution.reason` has no default, so the compiler asks the question; answer it rather than +defaulting to `absent`, which claims the store does not hold the skill. If the new outcome +is genuinely neither of the five, the token set grows — on both sides, in the same commit. + +Two things the reason is deliberately *not*: + +- **Not derived from `Resolution.error`.** That string is prose for a human; recovering a + decision a caller fails closed on by matching it is the fragility the typed token exists + to remove. `detail` *is* that string, passed straight through — safe to surface (key and + failure mode only, never content, never a path), and not for matching on. +- **Not `Resolution.unavailable`.** The flag answers "may prune run?" and the token answers + "what does the caller learn?". They agree by construction — `unavailable` is `True` in + exactly the `store_unavailable` case — and both exist because `store_unavailable` must + stay distinct from `absent`: only a raising store suppresses pruning, since deleting + managed files after a failed lookup turns an outage into data loss. + +`get_skill_result` emits nothing of its own. The integrity log record and signal already +fired inside verification before `resolve_from_store` returned; recording anything here +would double-count one failure in a SIEM and in the product counter. + +There is no `get_skills_result` or `all_skills_result`. The batch accessors keep omitting +unresolved entries and keep logging the run-level WARN count, and a second accessor per +batch form would double the surface for a case nobody has asked for. + ### Security posture — do not relax any of this Store data is **untrusted input**; the transport is not part of the trust boundary. diff --git a/packages/client/src/launchdarkly_ai_server/__init__.py b/packages/client/src/launchdarkly_ai_server/__init__.py index 37b1570..753e1e9 100644 --- a/packages/client/src/launchdarkly_ai_server/__init__.py +++ b/packages/client/src/launchdarkly_ai_server/__init__.py @@ -41,6 +41,7 @@ InMemorySkillStore, all_skills, get_skill, + get_skill_result, get_skills, skill_refs, ) @@ -85,6 +86,8 @@ ReconcileActionKind, ReconcileReport, Skill, + SkillOutcome, + SkillOutcomeReason, SkillReference, StreamChunkEvent, StreamDoneEvent, @@ -150,6 +153,7 @@ "ReconcileActionKind", "ReconcileReport", "Skill", + "SkillOutcome", "SkillReference", "StreamChunkEvent", "StreamDoneEvent", @@ -227,14 +231,16 @@ # skills "skill_refs", "get_skill", + "get_skill_result", "get_skills", "all_skills", "write_skills", "SkillStore", "InMemorySkillStore", - # skills — the two closed-set unions a typed consumer needs to name + # skills — the three closed-set unions a typed consumer needs to name "ReconcileActionKind", "OnUnavailable", + "SkillOutcomeReason", # skills — on-disk constants, identical across languages "SKILL_FILENAME", "MANIFEST_FILENAME", diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index 9d9b362..cb21986 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -37,7 +37,7 @@ resolve_from_store, verify_raw_skill, ) -from .types import AiConfigRep, Skill, SkillReference +from .types import AiConfigRep, Skill, SkillOutcome, SkillReference from .types_validation import ( is_valid_skill_key, is_valid_skill_version, @@ -247,6 +247,40 @@ async def get_skill(key: str, *, version: int | None = None) -> Skill | None: return resolve_from_store(require_store(), key, version).skill +async def get_skill_result(key: str, *, version: int | None = None) -> SkillOutcome: + """ + Retrieves one verified skill, reporting *why* when there is none. + + Same retrieval, same verification, same telemetry as ``get_skill`` — the two + differ only in what they report. ``get_skill`` collapses "no such skill", + "the store raised", "that is not the version held", and "the content failed + integrity verification" to one ``None``; this returns a ``SkillOutcome`` + whose ``reason`` names which of them happened, so a caller can fail closed on + suspected tampering while tolerating a merely-absent skill: + + ```python + outcome = await get_skill_result("pdf-extraction") + if outcome.reason == "integrity_failure": + raise SystemExit(f"refusing to run: {outcome.detail}") + if outcome.skill is not None: + print(outcome.skill.content) + ``` + + ``detail`` is human-readable and safe to surface — it names the key and the + failure mode, never any skill content or filesystem path. Branch on + ``reason``, not on ``detail``. + + Emits nothing of its own: an integrity failure has already recorded its log + record and its signal inside verification, and recording a second here would + double-count one failure. Raises ``RuntimeError`` only when no skill store is + configured, exactly as ``get_skill`` does. + """ + resolved = resolve_from_store(require_store(), key, version) + return SkillOutcome( + skill=resolved.skill, reason=resolved.reason, detail=resolved.error + ) + + async def get_skills(refs: Sequence[SkillReference | str]) -> list[Skill]: """ Retrieves a batch of verified skills. diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py index 1e517e4..74cb706 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_core.py +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -44,7 +44,7 @@ from dataclasses import dataclass from typing import Any, Literal, Protocol, get_args -from .types import Skill, SkillReference +from .types import Skill, SkillOutcomeReason, SkillReference from .types_validation import is_valid_skill_key, is_valid_skill_version logger = logging.getLogger(__name__) @@ -672,6 +672,26 @@ def newest_by_key(objects: dict[str, dict[str, Any]]) -> list[tuple[str, Any]]: class Resolution: """One key resolved against a store: the skill, or why there is none.""" + reason: SkillOutcomeReason + """ + Which of the five public outcomes this resolution is. + + Declared first and **without a default**, so every construction site has to + state it. A default would be the wrong shape twice over: a contributor + adding a sixth internal outcome would inherit whichever token happened to be + the default rather than deciding which public token it maps to, and if that + default were ``"ok"`` a failure would publish ``ok`` with no skill attached. + + Carried as a token rather than derived from ``error`` on the way out: + ``get_skill_result`` publishes this value, and pattern-matching prose to + recover a decision a caller fails closed on is exactly the fragility the + typed outcome exists to remove. A reviewer can read the mapping here. + + Distinct from ``unavailable`` on purpose — that flag answers one question + (may prune run?) and this token answers a different one (what does the + caller learn?) — but the two can only disagree by a bug: ``unavailable`` is + ``True`` in exactly the ``store_unavailable`` case. + """ skill: Skill | None = None error: str | None = None unavailable: bool = False @@ -702,26 +722,33 @@ def resolve_from_store( raw = store.get_object(SKILL_OBJECT_KIND, key, wanted_version) except Exception as exc: logger.error("Skill store raised while retrieving '%s'", key, exc_info=True) - return Resolution(error=store_raised(exc), unavailable=True) + return Resolution( + reason="store_unavailable", + error=store_raised(exc), + unavailable=True, + ) if not isinstance(raw, dict): return Resolution( - error=f"skill '{key}' is not available from the configured skill store" + reason="absent", + error=f"skill '{key}' is not available from the configured skill store", ) skill = verify_raw_skill(raw) if skill is None: return Resolution( - error=f"skill '{key}' failed integrity verification and was withheld" + reason="integrity_failure", + error=f"skill '{key}' failed integrity verification and was withheld", ) if wanted_version is not None and skill.version != wanted_version: return Resolution( + reason="wrong_version", error=( f"skill '{key}' version {wanted_version} is not available " f"(the store holds version {skill.version})" - ) + ), ) - return Resolution(skill=skill) + return Resolution(reason="ok", skill=skill) def reference_target(item: SkillReference | str) -> tuple[str, int | None]: diff --git a/packages/client/src/launchdarkly_ai_server/skills_fs.py b/packages/client/src/launchdarkly_ai_server/skills_fs.py index 8a865dd..fb16379 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fs.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fs.py @@ -404,11 +404,17 @@ def _resolve_reference( """ store = _available_store(deadline, f"'{key}'") if isinstance(store, _RetrievalBlocked): - return Resolution(error=store.reason, unavailable=True) + return Resolution( + reason="store_unavailable", error=store.reason, unavailable=True + ) resolved = resolve_from_store(store, key, wanted_version) if resolved.unavailable and resolved.error is not None: - return Resolution(error=_unavailable(resolved.error), unavailable=True) + return Resolution( + reason="store_unavailable", + error=_unavailable(resolved.error), + unavailable=True, + ) return resolved diff --git a/packages/client/src/launchdarkly_ai_server/types.py b/packages/client/src/launchdarkly_ai_server/types.py index 10c90d8..65e4850 100644 --- a/packages/client/src/launchdarkly_ai_server/types.py +++ b/packages/client/src/launchdarkly_ai_server/types.py @@ -452,6 +452,61 @@ class Skill: """Description from LaunchDarkly metadata; never parsed from the content.""" +SkillOutcomeReason = Literal[ + "absent", "integrity_failure", "ok", "store_unavailable", "wrong_version" +] +""" +The closed set of outcomes ``get_skill_result`` reports. + +Alphabetical, as ``reason_code`` is in the integrity log record, so the token +list reads identically in every LaunchDarkly AI SDK. Each token is a distinct +*decision* a caller can make, which is the point of the type: ``absent`` is a +skill the store does not hold, ``integrity_failure`` is content that was +delivered and did not verify, and a caller that wants to fail closed on +suspected tampering while tolerating a merely-absent skill needs the two to be +told apart. + +- ``ok`` — a verified skill was returned. +- ``absent`` — the store answered, and does not hold the key. +- ``integrity_failure`` — content was delivered and failed verification; it was + withheld. The one token worth failing closed on. +- ``store_unavailable`` — the store itself could not answer: it raised. + Deliberately distinct from ``absent``, because an outage is not a deletion. +- ``wrong_version`` — the store answered with a version other than the one + asked for, so the answer was withheld. +""" + + +@dataclass(frozen=True) +class SkillOutcome: + """ + Why one retrieval returned what it did — the reported form of ``get_skill``. + + ``get_skill`` collapses every failure to ``None``, which is the right shape + for a caller that only wants content and cannot act on the difference. This + is the shape for a caller that can: ``reason`` names which of the five + outcomes happened, so an integrity failure is distinguishable from a skill + that simply is not configured. The two accessors differ only in what they + report — the retrieval, the verification, and the telemetry are the same + code path, run once. + + Instances are immutable. + """ + + skill: Skill | None + """The verified skill, and only ever populated when ``reason == "ok"``.""" + reason: SkillOutcomeReason + """Which outcome happened. A closed set — see ``SkillOutcomeReason``.""" + detail: str | None + """ + Human-readable detail, set for every reason except ``ok``. + + Safe to log or surface to an operator: it carries the skill key and the + failure mode, and never any skill content or filesystem path. Intended for a + human, not for matching on — branch on ``reason``. + """ + + ReconcileActionKind = Literal[ "written", "updated", "skipped_current", "removed", "error" ] diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 0c656ed..6470f0c 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -20,10 +20,12 @@ ReconcileAction, ReconcileReport, Skill, + SkillOutcome, SkillReference, all_skills, get_client, get_skill, + get_skill_result, get_skills, init_client, shutdown, @@ -143,6 +145,16 @@ def test_skill_is_immutable(self) -> None: with pytest.raises(dataclasses.FrozenInstanceError): skill.content = b"tampered" # type: ignore[misc] + def test_skill_outcome_is_immutable(self) -> None: + """A reported outcome is a value, like every other public skills type. + + Matters more here than for the others: a caller that fails closed on + ``reason`` must not be handed something a later layer can rewrite. + """ + outcome = SkillOutcome(skill=None, reason="integrity_failure", detail="nope") + with pytest.raises(dataclasses.FrozenInstanceError): + outcome.reason = "ok" # type: ignore[misc] + def test_skill_content_is_bytes(self) -> None: """Content is the verified verbatim bytes — opaque, never text.""" skill = _skill() @@ -385,6 +397,13 @@ def test_exported_action_union_admits_exactly_the_five_actions(self) -> None: "error", } assert set(typing.get_args(package.OnUnavailable)) == {"keep", "raise"} + assert set(typing.get_args(package.SkillOutcomeReason)) == { + "absent", + "integrity_failure", + "ok", + "store_unavailable", + "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.""" @@ -393,11 +412,14 @@ def test_retrieval_surface_is_exported_from_the_package_root(self) -> None: expected = { "skill_refs", "get_skill", + "get_skill_result", "get_skills", "all_skills", "SkillStore", "InMemorySkillStore", "Skill", + "SkillOutcome", + "SkillOutcomeReason", "SkillReference", } assert expected <= set(package.__all__) @@ -743,6 +765,265 @@ async def test_multibyte_content_verifies( assert skill.content == content.encode("utf-8") +class _RaisingStore: + """A store whose reads raise — the "the transport is down" case. + + Declared with the full ``get_object`` signature on purpose. A double missing + the ``version`` parameter would also produce a raise here, but a + ``TypeError`` from the call itself rather than from the store, and the test + would then pass without the store ever having been consulted. + """ + + def get_object( + self, kind: str, key: str, version: int | None = None + ) -> dict[str, Any] | None: + raise RuntimeError("transport failure") + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + raise RuntimeError("transport failure") + + +class _WrongVersionAnsweringStore: + """A store that answers a pinned lookup with some other version.""" + + def __init__(self, make_raw_skill: Any, answered_version: int = 99) -> None: + self._make = make_raw_skill + self._answered_version = answered_version + + def get_object( + self, kind: str, key: str, version: int | None = None + ) -> dict[str, Any] | None: + answer: dict[str, Any] = self._make(key=key, version=self._answered_version) + return answer + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + return {} + + +class TestGetSkillResult: + """ + The reported accessor — one token per outcome a retrieval can have. + + ``get_skill`` collapses four distinct failures to ``None``, which leaves a + caller unable to fail closed on suspected tampering while tolerating a skill + that is merely not configured. These tests pin that the five outcomes are + told apart, and that reporting them changed nothing about ``get_skill``. + """ + + def _tampered(self, make_raw_skill: Any, key: str = "a") -> dict[str, Any]: + raw: dict[str, Any] = make_raw_skill(key=key) + raw["contentHash"] = "0" * 64 + return raw + + async def test_ok_carries_the_skill_and_no_detail( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="pdf-extraction", version=2)) + + outcome = await get_skill_result("pdf-extraction") + + assert outcome.reason == "ok" + assert outcome.detail is None + assert outcome.skill is not None + assert outcome.skill.key == "pdf-extraction" + assert outcome.skill.version == 2 + assert outcome.skill.content == SKILL_BODY.encode("utf-8") + + async def test_absent_when_the_store_does_not_hold_the_key( + self, store: InMemorySkillStore + ) -> None: + outcome = await get_skill_result("nope") + + assert outcome.reason == "absent" + assert outcome.skill is None + assert outcome.detail + assert "'nope'" in outcome.detail + + async def test_integrity_failure_when_content_does_not_verify( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + """The one outcome a caller is expected to fail closed on.""" + store.put(self._tampered(make_raw_skill, key="a")) + + outcome = await get_skill_result("a") + + assert outcome.reason == "integrity_failure" + assert outcome.skill is None + assert outcome.detail + # The quoted form, so the assertion is about the key and not about a + # letter that appears in half the words in the message. + assert "'a'" in outcome.detail + + async def test_wrong_version_when_the_store_answers_with_another( + self, make_raw_skill: Any + ) -> None: + skills_module._set_store(_WrongVersionAnsweringStore(make_raw_skill)) + + outcome = await get_skill_result("a", version=1) + + assert outcome.reason == "wrong_version" + assert outcome.skill is None + assert outcome.detail + # The detail is what makes this actionable rather than merely negative: + # it names both the version asked for and the version held. + assert "version 1" in outcome.detail + assert "version 99" in outcome.detail + + async def test_store_unavailable_when_the_store_raises( + self, caplog: pytest.LogCaptureFixture + ) -> None: + skills_module._set_store(_RaisingStore()) + + with caplog.at_level("ERROR", logger="launchdarkly_ai_server.skills_core"): + outcome = await get_skill_result("a") + + assert outcome.reason == "store_unavailable" + assert outcome.skill is None + assert outcome.detail + assert "RuntimeError" in outcome.detail + + async def test_store_unavailable_is_distinct_from_absent( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """An outage is not a deletion, and the two must not read alike. + + This is the distinction ``write_skills`` already depends on to decide + whether pruning may run — only a raising store suppresses it — so + collapsing the two tokens here would put the public vocabulary at odds + with a policy the SDK already enforces internally. + """ + skills_module._set_store(_RaisingStore()) + with caplog.at_level("ERROR", logger="launchdarkly_ai_server.skills_core"): + raised = await get_skill_result("a") + + skills_module._set_store(InMemorySkillStore()) + empty = await get_skill_result("a") + + # Asserted as two named tokens rather than as an inequality: the type + # checker can already see that these two literals differ, so an + # inequality here would be dead weight. + assert raised.reason == "store_unavailable" + assert empty.reason == "absent" + + async def test_every_non_ok_outcome_carries_a_detail( + self, make_raw_skill: Any, caplog: pytest.LogCaptureFixture + ) -> None: + """A reason with no detail leaves an operator nothing to act on. + + Swept over all four failures in one test rather than asserted per case + only, so a fifth failure path added later without a message is caught by + a test whose name says what it is about. + """ + stores: list[tuple[str, Any]] = [ + ("absent", InMemorySkillStore()), + ( + "integrity_failure", + InMemorySkillStore({"a": self._tampered(make_raw_skill)}), + ), + ("wrong_version", _WrongVersionAnsweringStore(make_raw_skill)), + ("store_unavailable", _RaisingStore()), + ] + + outcomes: list[SkillOutcome] = [] + with caplog.at_level("ERROR", logger="launchdarkly_ai_server.skills_core"): + for _expected, store_double in stores: + skills_module._set_store(store_double) + outcomes.append(await get_skill_result("a", version=1)) + + assert [o.reason for o in outcomes] == [expected for expected, _ in stores] + assert all(o.skill is None for o in outcomes) + assert all(o.detail for o in outcomes) + + async def test_detail_never_carries_the_skill_content( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + """``detail`` is safe to log, so the body must not travel in it. + + Same rule the integrity log record follows, asserted separately here + because this string reaches the caller through a different surface. + """ + secret = "---\nname: Secret\n---\nSSN 000-00-0000 and an API key.\n" + store.put(make_raw_skill(key="a", content=secret, contentHash="0" * 64)) + + outcome = await get_skill_result("a") + + assert outcome.reason == "integrity_failure" + assert outcome.detail is not None + assert secret not in outcome.detail + assert "SSN" not in outcome.detail + assert "API key" not in outcome.detail + + async def test_raises_the_same_way_as_get_skill_with_no_store(self) -> None: + """Identical failure mode, down to the message. + + The two accessors differ only in what they report about a retrieval; a + missing store is a configuration error in both, so a caller cannot need + to handle it twice. + """ + with pytest.raises(RuntimeError, match="skill store") as reported: + await get_skill_result("a") + with pytest.raises(RuntimeError, match="skill store") as collapsed: + await get_skill("a") + + assert str(reported.value) == str(collapsed.value) + + async def test_records_no_second_integrity_signal( + self, + store: InMemorySkillStore, + make_raw_skill: Any, + recording_emitter: Any, + caplog: pytest.LogCaptureFixture, + ) -> None: + """One failed retrieval is one failure, on both surfaces. + + Verification already recorded the log record and the signal before + ``resolve_from_store`` returned, so reporting the reason must add + nothing: a second record would double-count one event in a SIEM and + inflate the product counter. ``_integrity_records`` is the shared parser + used by the log-record tests further down this module. + """ + skills_module._set_emitter_for_testing(recording_emitter) + store.put(self._tampered(make_raw_skill, key="a")) + + with caplog.at_level("ERROR", logger="launchdarkly_ai_server.skills_core"): + outcome = await get_skill_result("a") + + assert outcome.reason == "integrity_failure" + assert len(_integrity_records(caplog)) == 1 + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_get_skill_still_returns_none_for_every_failure( + self, make_raw_skill: Any, caplog: pytest.LogCaptureFixture + ) -> None: + """The no-behaviour-change guarantee. + + ``get_skill``'s contract — ``None`` for every failure, and it never + raises for one — is documented in its docstring and in the README, and + every existing caller treats ``None`` as "no skill". Adding a reported + accessor beside it must not move that line, so the four failures are + driven through both accessors in one test: the reason is distinguishable + *and* the collapsed form still collapses. + """ + cases: list[tuple[str, Any]] = [ + ("absent", InMemorySkillStore()), + ( + "integrity_failure", + InMemorySkillStore({"a": self._tampered(make_raw_skill)}), + ), + ("wrong_version", _WrongVersionAnsweringStore(make_raw_skill)), + ("store_unavailable", _RaisingStore()), + ] + + with caplog.at_level("ERROR", logger="launchdarkly_ai_server.skills_core"): + for expected_reason, store_double in cases: + skills_module._set_store(store_double) + reported = await get_skill_result("a", version=1) + assert reported.reason == expected_reason + # No pytest.raises wrapper: an escaping exception fails the test + # here, which is the "never raises" half of the contract. + assert await get_skill("a", version=1) is None + + class TestGetSkills: """Batch accessor."""