From 0f303f4bed53c4c597bb284e5414fcca4a831735 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Wed, 7 Oct 2026 11:20:58 -0400 Subject: [PATCH] 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