diff --git a/packages/client/README.md b/packages/client/README.md index ae572b3..d9c1db1 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -223,15 +223,17 @@ asyncio.run(main()) ### Agent Skills Skills are versioned `SKILL.md` documents managed in LaunchDarkly and attached to AI Config -variations by reference. The SDK surfaces which skills a config references and retrieves -their content. Materializing them onto disk, where agent runtimes discover them, follows. +variations by reference. The SDK surfaces which skills a config references, retrieves their +content, and materializes them onto disk where agent runtimes (Claude Agent SDK, and +anything else following the `//SKILL.md` convention) discover them. ```python import asyncio import hashlib +from pathlib import Path from launchdarkly_ai_server import ( - init_client, inspect_config, skill_refs, get_skill, get_skills, + init_client, inspect_config, skill_refs, get_skill, write_skills, InMemorySkillStore, ) @@ -260,13 +262,18 @@ async def main(): if skill is not None: print(skill.content) - # 3. Or resolve the config's references in one call. - for s in await get_skills(refs): - print(s.key, s.version) + # 3. Write them where the agent runtime will look. Only the leaf directory is + # created, so the parent must already exist. + Path(".claude").mkdir(exist_ok=True) + report = await write_skills(refs, ".claude/skills") + for action in report.errors: + print(f"skill {action.key or ''}: {action.error}") asyncio.run(main()) ``` +Pass `"*"` instead of a reference list to materialize every skill the store holds. + **`skills` is now 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 @@ -333,7 +340,22 @@ treat `expected_hash` / `observed_hash` as the evidence pair. **Versions are selected, not filtered.** A store may hold several versions of one key at once, because a delivery payload does: the newest version of every skill, plus every version a variation currently pins. `get_skill("k", version=1)` asks the store for version -1 and gets it even when a newer one is also held. +1 and gets it even when a newer one is also held. `all_skills()` and `write_skills("*")` +collapse to one skill per key at its newest version, since `//SKILL.md` is a +single path. + +**The root's parent must exist.** `write_skills` creates the root itself but never its +ancestors, so a typo cannot scatter a directory tree across your project. An absent parent, +a root that is an existing file, and a root that is a symlink each raise `ValueError` — +these are caller errors, distinct from the per-skill `error` actions in the report. + +**`write_skills` is deliberately conservative** about your filesystem. It writes only +`//SKILL.md`, tracks what it owns in a manifest at +`/.launchdarkly-skills.json`, and will overwrite or delete **only** paths that +manifest records. A file you placed yourself is reported as an error and left untouched; it +never writes through a symlink; writes are atomic (temp file, `fsync`, rename) at mode +`0644`; and if the manifest is unreadable it performs no destructive action at all. Removing +a skill from a variation is how revocation works — the next reconcile prunes it. | Export | Description | |---|---| @@ -341,11 +363,34 @@ version a variation currently pins. `get_skill("k", version=1)` asks the store f | `get_skill(key, *, version=None)` | One verified skill, or `None`. `version=None` means newest available; a specific `version` matches exactly. Raises only when no store is configured. | | `get_skills(refs)` | Batch form. Accepts `SkillReference` values and bare key strings (string = latest). Results follow input order; missing or unverifiable entries are omitted. | | `all_skills()` | Every verified skill the store holds, one per key at its newest version. | +| `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.** | | `SkillStore` | The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `add_listener(kind, fn)`. | | `InMemorySkillStore(objects=None)` | A dict-backed store with `put(raw)`, for local development and testing. Holds several versions of a key. | Configure the store with `init_client(options={"skillStore": store})`. With none configured, -the accessors raise `RuntimeError` explaining what to do. `shutdown()` clears it. +the accessors raise `RuntimeError` explaining what to do and `write_skills` reports the +failure in its report (or raises, with `on_unavailable="raise"`). `shutdown()` clears it. + +`ReconcileReport.actions` holds one `ReconcileAction` per outcome — `written`, `updated`, +`skipped_current`, `removed`, or `error` — each carrying `key`, `version`, the resolved +`path`, and `error`. `report.ok` is `True` when no action is an `error`, and +`report.errors` is just the `error` actions, so you rarely need to filter `actions` +yourself. A failure that belongs to the whole run rather than to one skill — an unreadable +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 +your own helpers: `ReconcileActionKind` (`written` / `updated` / `skipped_current` / +`removed` / `error`) and `OnUnavailable` (`keep` / `raise`). + +**`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, +so a large reconcile holds the event loop for its duration. Wrap it in +`asyncio.to_thread` if that matters. For the same reason `timeout` is checked between +steps rather than interrupting one already in progress. Reconcile one root at a time, +though: because nothing yields today, a run is atomic against the rest of your loop, and +wrapping it to run concurrently makes two runs against the same root race on the manifest. `all_objects` returns one entry per `(key, version)` under keys that are **opaque** to the SDK — identity is read from each object's own `key` and `version` fields, so a store is free @@ -389,3 +434,5 @@ 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` | +| `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 152f577..feb2e34 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -30,6 +30,7 @@ No other `launchdarkly-ai-*` package may define or duplicate these. They import | `src/launchdarkly_ai_server/types_validation.py` | `parse_ai_config` — validates flag variation shape; `is_valid_skill_key` / `is_valid_skill_version` / `skill_key_rejection_reason` (the canonical key-grammar explanation every layer quotes) | | `src/launchdarkly_ai_server/skills.py` | Agent Skills, retrieval half — `skill_refs`, `get_skill`/`get_skills`/`all_skills`, `InMemorySkillStore`, and the store/telemetry injection points `_set_store` / `_set_emitter_for_testing` | | `src/launchdarkly_ai_server/skills_core.py` | Shared skills internals — the `SkillStore` seam, module state, the telemetry seam and its three recorders, integrity verification, and store resolution. Imported by both `skills.py` and the materialization layer; imports neither | +| `src/launchdarkly_ai_server/skills_fs.py` | Agent Skills, materialization half — `write_skills`, request resolution, the manifest format and on-disk filenames, per-skill reconcile, and pruning | | `src/launchdarkly_ai_server/safe_fs.py` | Descriptor-pinned filesystem primitives — `atomic_write`, `unlink_file`, `pinned_directory`, `open_directory_nofollow`, `open_or_create_directory`, `SymlinkRefused`, and the `*at()` capability probe. Owns the descriptor-vs-path platform split; knows nothing about skills | | `src/launchdarkly_ai_server/utils.py` | `parse_template`, `parse_json_with_possible_fences`, `create_handler`, `parse_usage`, `make_track_data`, `to_ld_context` | | `src/launchdarkly_ai_server/registry.py` | `Registry`, `global_registry`, `compose`, `resolve_handlers`, `resolve_tools` | @@ -56,7 +57,7 @@ from launchdarkly_ai_server import ( TrackData, UsageDict, HandlerResult, HandlerStreamEvent, StreamEvent, StreamChunkEvent, StreamDoneEvent, ExecuteStreamEvent, ExecuteStreamDoneEvent, VariationMeta, InitClientOptions, JudgeResult, ParseResult, ParseSuccess, ParseFailure, - Skill, SkillReference, + Skill, SkillReference, ReconcileAction, ReconcileReport, ) # Utilities @@ -76,8 +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, + skill_refs, get_skill, get_skills, all_skills, write_skills, SkillStore, InMemorySkillStore, + SKILL_FILENAME, MANIFEST_FILENAME, MANIFEST_VERSION, + ReconcileActionKind, OnUnavailable, # the two closed-set unions ) ``` @@ -179,7 +182,7 @@ This is an OTel context value, not W3C baggage, so the id does not leak onto out Versioned `SKILL.md` documents attached to AI Config variations by reference, retrieved through an injectable store, and materialized onto disk for agent runtimes to discover. -Three layers, in increasing order of blast radius. Only the first is implemented here: +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. @@ -190,7 +193,8 @@ Three layers, in increasing order of blast radius. Only the first is implemented `init_client(options={"skillStore": store})`; with none configured the accessors raise an actionable `RuntimeError`. A delivery transport can be added behind the seam without touching the public API. -3. **Materialization** — writing skills onto disk under a manifest. +3. **Materialization** — `write_skills(skills, root)` writes `//SKILL.md` and + reconciles against a manifest at `/.launchdarkly-skills.json`. ### The store seam, and why version is part of the lookup @@ -241,6 +245,30 @@ Store data is **untrusted input**; the transport is not part of the trust bounda - **Attacker-controlled strings are never echoed into telemetry.** `contentHash` and `key` come off the wire, so a store could put the skill body in either; both are shape-checked and redacted before they reach a signal or a log line. +- **The key is re-validated inside `write_skills`**, regardless of upstream validation — a + key becomes a directory name. Rejection happens before any filesystem call. +- **Never write through a symlink**, in either the skill directory or the target file, on + the write path *and* the prune path. +- **Destructive operations only on manifest-listed paths whose `key` matches.** A file at a + managed path with no matching manifest entry is reported as `error` and left alone. +- **A corrupt manifest fails closed**: unreadable, unparseable, not an object, malformed + `entries`, or a `manifestVersion` this release cannot read means no overwrites and no + prunes, brand-new paths may still be written, an `error` action names the manifest, and + the manifest file itself is not rewritten. +- **An incomplete retrieval suppresses pruning.** Otherwise a transport outage would read + as "everything was revoked" and delete the customer's managed files. +- **Writes are atomic**: temp file created exclusively in the target's *own* directory, + mode `0644` set explicitly (never inherited from the umask, never executable), write, + fsync, `os.replace`, fsync the directory. `os.replace` is the single rename call site + and must not be swapped for `os.rename`. +- **Every operation under the root goes through a pinned descriptor, not a path.** See + "Descriptor-pinned filesystem access" below. Re-resolving `/` from its path at + write or unlink time reopens a swap window that the checks above cannot cover. +- **A key valid to the data model may still be unrepresentable on disk.** The model allows + 256 characters; `NAME_MAX` is 255 bytes. `write_skills` rejects an over-long key before + any filesystem call, and every per-skill filesystem failure is caught at the loop so it + becomes an `error` action — aborting the loop would skip the manifest rewrite and orphan + files already written in that run. - **A key is untrusted input everywhere it appears.** `skill_key_rejection_reason` is the single canonical explanation, so the config parser and the reference projection reject a key for the same stated reason — and so does every layer added later. A silently @@ -262,9 +290,6 @@ Exactly three signals exist, and the list is an **allowlist, not a floor**: | `AgentControl Skill Materialized` | each `written` / `updated` / `skipped_current` | `skill_key`, `content_bytes`, `content_hash`, `reconcile_action`, `language` | | `AgentControl Skill Revoked Received` | prune removes a formerly managed skill | `skill_key`, `version`, `removed_from_disk`, `language` | -The last two belong to the materialization layer and have no caller yet; they live here -with the first so the allowlist is one section of one file rather than three sites to audit. - ### The integrity-failure log record The signal above is product telemetry; the **log record** beside it is the customer-owned @@ -321,11 +346,12 @@ where they cannot see it. `AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` were considered and **deliberately excluded from SDK emission** — both are observable server-side. Do not add them. The skill body never appears in a signal, a log line, or -an error message, and no signal carries a filesystem path. An emitter that raises is caught -and logged; it never fails the operation. +an error message, and no signal carries a filesystem path (paths belong in the returned +`ReconcileReport`, which is user-facing API). An emitter that raises is caught and logged; +it never fails the operation. -Module state lives in `skills_core.py`, so there is exactly one store and one emitter -however the feature is entered. All three signals are emitted from the `record_*` functions +Module state lives in `skills_core.py`, the module `skills.py` and `skills_fs.py` share, +so there is exactly one store and one emitter however the feature is entered. All three signals are emitted from the `record_*` functions next to the seam there — nothing outside that module calls `emit`, so the allowlist is enforced in one place. @@ -366,7 +392,14 @@ primitives live in `safe_fs.py`, which knows nothing about skills: *trailing* symlink, but it does resolve the directory above it, so the same swap turns a removal into a delete of an attacker-chosen file. A symlink found where this SDK expects its own file raises `SymlinkRefused` rather than being tidied away: the state on disk is - not what the caller believes, and that is the caller's to report. + not what the caller believes, and that is the caller's to report. `_prune_one` goes + through it; `rmdir` stays path-based and is safe that way, since it fails `ENOTDIR` on a + symlink and only ever succeeds on an empty directory. + +Every `lstat`, `realpath` and containment check on the skills side lives in one shared +`_unsafe_path_reason`, so the write and prune paths cannot drift apart on what counts as +unsafe. `skills_fs._prune_one` spells its symlink check `os.stat(..., follow_symlinks=False)` +rather than `os.lstat`, matching the name the capability probe advertises. `safe_fs.SUPPORTS_DIR_FD` gates all of it, and the probe is not the obvious one. `os.supports_dir_fd` is populated per underlying syscall, and CPython registers `renameat` @@ -379,7 +412,43 @@ and silently turns the defense off, so the probe names the advertised twins (Windows) `open_directory_nofollow` returns `None` after an `lstat` check instead of attempting the descriptor open — `os.open` cannot open a directory there — and every caller falls back to the identical full-path sequence, the per-component `lstat` floor. The -residual window on those platforms is documented rather than closed. +residual window on those platforms is documented rather than closed; the TOCTOU tests skip +off this same flag, deliberately, so a probe that wrongly reports "unsupported" cannot also +silently skip the tests that would have caught it. + +Both call shapes are admitted by the test seam. `os.replace` remains the single +interceptable rename call site; under the descriptor-relative shape `dst` is the bare string +`"SKILL.md"`, so an `endswith("SKILL.md")` spy filter still matches, and the +same-directory requirement is proved by descriptor identity (`src_dir_fd == dst_dir_fd`, +resolving to the skill directory's `(st_dev, st_ino)`) instead of by comparing path strings. +A spy must `fstat` the descriptor **inside** the intercepted call — the implementation closes +it as soon as the write returns. + +### Deferred: bounded retries + +`timeout` is implemented — a monotonic deadline, checked before each retrieval, before +each write, and before each prune; only the final manifest rewrite runs past it, so files +already written are never orphaned. Bounded retries inside that deadline are **not** +implemented, and belong to the delivery transport, not to this layer. Three structural +reasons, all of which the transport changes: + +1. **There is nothing transient to retry.** `SkillStore.get_object` is a synchronous + in-process read against already-delivered data, modelled on the LaunchDarkly + data-store API. `InMemorySkillStore` reads a dict. A retry re-invokes customer code and + returns the same answer. +2. **The seam cannot classify a failure.** All it surfaces is "this raised". Retrying a + `PermissionError` or a malformed payload spends the caller's `timeout` on a certainty. + The transient/permanent taxonomy a retry policy needs is the transport's to define. +3. **Backoff has nowhere to sleep.** The retrieval path (`_resolve_requests`, + `_resolve_reference`, `_resolve_all`) is synchronous, called from an async + `write_skills`. Backoff would mean either `time.sleep` — blocking the event loop of every + caller — or async-ifying the whole path for a store that cannot benefit. + +Picking a bound and a backoff now would fix numbers in a cross-language contract with no +transport to calibrate them against, so there is **no** retry test and no assumable attempt +count. When the transport lands it owns the policy; keep both languages retry-free until +then, since the number of times a throwing store is invoked is observable and the two would +otherwise diverge. --- @@ -491,7 +560,7 @@ Install with `pip install "launchdarkly-ai-server[otel]"`; see [OTel Setup](#ote |---|---| | `launchdarkly-server-sdk>=9.0`, and the `otel` extra mirrored (`opentelemetry-sdk`, `opentelemetry-exporter-otlp-proto-http`) | Each dynamically-resolved or optional package is repeated in the dev group so the test suite can import it. Something that is *only* optional would not be installed in this workspace and the tests covering its present-and-working path could not run. | | `pytest>=8`, `pytest-asyncio>=0.24` | Test runner and the async support the whole suite relies on. `asyncio_mode = "auto"` is set at the workspace root, which is why no test in this package carries an `@pytest.mark.asyncio`. | -| `mypy>=1.10` (`strict`), `ruff>=0.15` | Type checker and linter/formatter. | +| `mypy>=1.10` (`strict`), `ruff>=0.15` | Type checker and linter/formatter. `mypy` strict mode is the only thing enforcing the `Literal[...]` closed set on `ReconcileAction.action` — unlike `write_skills`'s `on_unavailable`, which is also checked at runtime because the value can arrive from untyped code. | --- @@ -513,6 +582,22 @@ convenience accessor that reads meaning into it — no YAML/frontmatter parsing, verified verbatim byte buffer and nothing more; a consumer who wants structure parses it on their side of the boundary. +### 4. Assuming `write_skills` prunes on every run + +Pruning is suppressed when the manifest is corrupt or any retrieval was incomplete — both +mean the SDK cannot tell what it owns or what is still current, and deleting under that +uncertainty is data loss. A run whose report contains a manifest `error` will not have +pruned anything, so do not read "no `removed` actions" as "nothing is stale". + +### 5. Treating "absent from the resolved set" as always meaning revoked + +Revocation is pruning, but only for a skill the store genuinely no longer serves. An object +that is *present and unverifiable* is a different thing, and `_resolve_all` must emit a +failed `_PendingWrite` for it rather than filtering it out: dropping it silently leaves its +key out of the requested set, so prune deletes the last known-good copy on disk and reports +a routine `removed` with `report.ok` still true. Tampered content must never be able to +trigger deletion. + --- ## Adding a New Export @@ -532,4 +617,5 @@ on their side of the boundary. - Do not route skills telemetry through `client.track()`, and do not introduce an LD context anywhere in the skills path. Signals go through the `skills_core.py` emitter seam, whose default is a no-op, and only via its `record_*` functions. - Do not add a signal name outside the three in the Agent Skills table above — the list is an allowlist. `AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` were considered and deliberately excluded from SDK emission. - Do not rename `ld.skills.integrity_failure`, and do not add a ninth `reason_code` in one language only — both are documented compatibility surfaces. See "The integrity-failure log record" above. +- Do not relax any of the `write_skills` filesystem defenses (local key re-validation, symlink refusal, manifest-authorized destruction, corrupt-manifest fail-closed, atomic `0644` writes). Each is a deliberate security property with abuse-case tests attached. - Do not make `SkillStore` lookups key-only. Version is part of the lookup identity because a payload holds several versions of one key; a key-only seam cannot express a version-pinned reference. diff --git a/packages/client/src/launchdarkly_ai_server/__init__.py b/packages/client/src/launchdarkly_ai_server/__init__.py index 02c858f..37b1570 100644 --- a/packages/client/src/launchdarkly_ai_server/__init__.py +++ b/packages/client/src/launchdarkly_ai_server/__init__.py @@ -45,6 +45,13 @@ skill_refs, ) from .skills_core import SkillStore +from .skills_fs import ( + MANIFEST_FILENAME, + MANIFEST_VERSION, + SKILL_FILENAME, + OnUnavailable, + write_skills, +) from .tracking import execute_and_stream, execute_and_track, wrap_tool_handlers from .types import ( NATIVE_TOOL_KEY, @@ -74,6 +81,9 @@ ProviderGraphResponse, ProviderHandler, ProviderResponse, + ReconcileAction, + ReconcileActionKind, + ReconcileReport, Skill, SkillReference, StreamChunkEvent, @@ -136,6 +146,9 @@ "ProviderGraphResponse", "ProviderHandler", "ProviderResponse", + "ReconcileAction", + "ReconcileActionKind", + "ReconcileReport", "Skill", "SkillReference", "StreamChunkEvent", @@ -216,6 +229,14 @@ "get_skill", "get_skills", "all_skills", + "write_skills", "SkillStore", "InMemorySkillStore", + # skills — the two closed-set unions a typed consumer needs to name + "ReconcileActionKind", + "OnUnavailable", + # skills — on-disk constants, identical across languages + "SKILL_FILENAME", + "MANIFEST_FILENAME", + "MANIFEST_VERSION", ] diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index eb3946c..9d9b362 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -183,8 +183,8 @@ def skill_refs(config: AiConfigRep | None) -> list[SkillReference]: A config that came through ``parse_ai_config`` never contains an invalid entry — parsing fails closed on one. A hand-built dict can, and a silently - shortened projection would leave a caller materializing a skill set it - believes is complete, so every dropped entry is logged. + shortened projection would let ``write_skills`` prune the dropped skill's + on-disk copy, so every dropped entry is logged. """ if not isinstance(config, dict): return [] diff --git a/packages/client/src/launchdarkly_ai_server/skills_fs.py b/packages/client/src/launchdarkly_ai_server/skills_fs.py new file mode 100644 index 0000000..feb8fbc --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills_fs.py @@ -0,0 +1,960 @@ +""" +Agent Skills — filesystem materialization. + +The highest-blast-radius layer of the feature: this is the part that writes to a +customer's disk. Split out of ``skills.py`` on that boundary — everything here +takes already-verified content and reconciles it against a managed root, while +``skills.py`` owns retrieval and verification and knows nothing about the +filesystem. The dependency runs one way only, and the descriptor-pinned +primitives every destructive step goes through live in ``safe_fs.py``. + +The reconcile is manifest-driven and fails closed: destructive operations only +ever touch paths ``/.launchdarkly-skills.json`` records under a matching +key, a corrupt manifest suppresses every destructive action, and an incomplete +retrieval suppresses pruning. Content is re-verified immediately before the +write, because a ``Skill`` can also be constructed directly by a caller. +""" + +from __future__ import annotations + +import hashlib +import json +import logging +import os +import stat +import time +from collections.abc import Sequence +from dataclasses import dataclass +from datetime import UTC, datetime +from pathlib import Path +from typing import Any, Literal + +from .safe_fs import ( + SymlinkRefused, + atomic_write, + atomic_write_in, + pinned_directory, + unlink_file, +) +from .skills_core import ( + NO_STORE_MESSAGE, + Resolution, + SkillStore, + VerificationFailure, + get_store, + list_raw_objects, + log_withholding_summary, + newest_by_key, + record_materialized, + record_revoked, + reference_target, + resolve_from_store, + verified_bytes, + verify_raw_skill, +) +from .types import ( + ReconcileAction, + ReconcileActionKind, + ReconcileReport, + Skill, + SkillReference, +) +from .types_validation import ( + is_valid_skill_key, + is_valid_skill_version, + skill_key_rejection_reason, +) + +logger = logging.getLogger(__name__) + +MANIFEST_FILENAME = ".launchdarkly-skills.json" +"""The SDK's record of what it has written under a managed root.""" + +MANIFEST_VERSION = 1 +"""Manifest schema version this release writes, and the highest it can read.""" + +SKILL_FILENAME = "SKILL.md" +"""The single file each skill materializes to, under ``//``.""" + +OnUnavailable = Literal["keep", "raise"] +"""How ``write_skills`` reacts to content it could not retrieve.""" + +_UNAVAILABLE_PREFIX = "skill retrieval unavailable: " +""" +Prefix on every error describing content that could not be retrieved. Callers +assert on it, so it lives in one place. +""" + +_MAX_PATH_COMPONENT_BYTES = 255 +""" +NAME_MAX on Linux and macOS, and the component limit on Windows. A skill key +becomes a single directory name, and the data model permits keys up to 256 +characters — one byte longer than any of those filesystems can represent. Such a +key is rejected before any filesystem call so the caller gets a reported action +rather than an ENAMETOOLONG escaping from a stat deep inside the reconcile. +""" + + +# ------------------------------------------------------------------------- +# The reconcile entry point +# ------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class _PendingWrite: + """One skill queued for the reconcile: resolved content, or why there is none.""" + + key: str + skill: Skill | None = None + error: str | None = None + + +async def write_skills( + skills: Sequence[Skill | SkillReference | str] | str, + root: str | os.PathLike[str], + *, + prune: bool = True, + timeout: float = 10.0, + on_unavailable: OnUnavailable = "keep", +) -> ReconcileReport: + """ + Materializes skills under a managed root at ``//SKILL.md``. + + *skills* is a sequence of ``Skill`` / ``SkillReference`` / key strings, or + the literal ``"*"`` meaning everything ``all_skills()`` returns. ``Skill`` + values are used as-is; references and strings resolve through the accessors, + so they need a configured store. + + The reconcile is manifest-driven (``/.launchdarkly-skills.json``): + destructive operations only ever touch paths the manifest records under a + matching key, so a file the SDK did not write is never overwritten or + deleted. ``prune`` removes formerly-managed skills that are no longer in the + requested set — which is also how revocation takes effect. ``timeout`` + bounds retrieval, the writes, and pruning; the final manifest rewrite + always runs, so files already written are never orphaned. ``on_unavailable`` + chooses between reporting a failed retrieval (``"keep"``, leaving existing + managed files alone) and raising (``"raise"``). + + Returns a ``ReconcileReport`` in which every outcome is visible; raises + ``ValueError`` for a caller error such as an unusable root. + + **This call performs synchronous filesystem I/O and does not yield.** It is + ``async`` for signature parity with the other accessors and with the + TypeScript SDK, not because it awaits anything: every read, write, ``fsync`` + and rename runs inline, so a large reconcile blocks the event loop for its + duration. Wrap it in ``asyncio.to_thread`` if that matters on your loop. + ``timeout`` is checked between steps rather than interrupting one in + progress, for the same reason. + + **One root, one reconcile at a time.** Because nothing here yields, a whole + reconcile is atomic against every other task on the loop today. Wrapping it + to run concurrently makes that the caller's problem instead: two runs + against the same root interleave on the manifest, and the loser's entries + are lost — which leaves the files it wrote unmanaged, and a later reconcile + then refuses them as files the SDK did not write. + """ + # Both of these are annotated as closed sets, but the values can still arrive + # from untyped code, so they are checked rather than assumed. + if on_unavailable not in ("keep", "raise"): + raise ValueError( + f'on_unavailable must be "keep" or "raise", got {on_unavailable!r}' + ) + if timeout < 0: + raise ValueError(f"timeout must not be negative, got {timeout!r}") + + deadline = time.monotonic() + timeout + root_path = _resolve_root(root) + manifest, manifest_error = _load_manifest(root_path) + entries: dict[str, Any] = manifest.get("entries", {}) + + actions: list[ReconcileAction] = [] + if manifest_error is not None: + # Run-level failure: there is no single skill key to hang it off. + actions.append(_run_error(manifest_error)) + + requests, incomplete = _resolve_requests(skills, deadline, on_unavailable) + + written, write_timed_out = _write_all(root_path, requests, entries, deadline) + actions.extend(written) + incomplete = incomplete or write_timed_out + + # Pruning is destructive, so it needs a trustworthy picture of both sides: a + # corrupt manifest means we do not know what we own, and an incomplete run — + # a retrieval that failed, or a deadline that expired mid-write — means we do + # not know what is still current. Either way, deleting would be a guess. + if prune and manifest_error is None and not incomplete: + actions.extend( + _prune( + root_path, + entries, + {request.key for request in requests}, + deadline, + ) + ) + + if manifest_error is None: + actions.extend(_rewrite_manifest(root_path, manifest, entries)) + + return ReconcileReport(actions=actions) + + +_RUN_LEVEL_KEY = "" +""" +The documented sentinel for a failure that belongs to no single skill (see +``ReconcileAction``). Spelled once so every path that cannot attribute a +failure to a key agrees with the others. +""" + + +def _run_error(message: str) -> ReconcileAction: + """ + A failure belonging to the run rather than to one skill. + + Uses the run-level sentinel key; it is constructed here so every run-level + error agrees. + """ + return ReconcileAction(key=_RUN_LEVEL_KEY, action="error", error=message) + + +def _write_all( + root: Path, + requests: list[_PendingWrite], + entries: dict[str, Any], + deadline: float, +) -> tuple[list[ReconcileAction], bool]: + """ + Reconciles every pending write. Returns ``(actions, timed out mid-run)``. + + The loop never aborts: a per-skill failure becomes an ``error`` action and the + next skill is attempted, because returning early would skip the caller's + manifest rewrite and orphan every file already written in this run. + """ + actions: list[ReconcileAction] = [] + timed_out = False + + for request in requests: + if request.skill is None: + actions.append( + ReconcileAction( + key=request.key, + action="error", + error=request.error + or f"skill '{request.key}' could not be resolved", + ) + ) + continue + if time.monotonic() >= deadline: + timed_out = True + actions.append( + ReconcileAction( + key=request.key, + action="error", + error=( + "the timeout was exhausted before skill " + f"'{request.key}' could be written" + ), + ) + ) + continue + try: + actions.append(_write_one(root, request.skill, entries)) + except OSError as exc: + # A safety net, not the primary defense. pathlib's stat probes swallow + # only ENOENT/ENOTDIR/EBADF/ELOOP and re-raise every other errno, so an + # unexpected filesystem condition must not abort the loop. + actions.append( + ReconcileAction( + key=request.skill.key, + action="error", + version=request.skill.version, + error=f"skill '{request.skill.key}' could not be reconciled: {exc}", + ) + ) + + return actions, timed_out + + +def _rewrite_manifest( + root: Path, manifest: dict[str, Any], entries: dict[str, Any] +) -> list[ReconcileAction]: + """Writes the updated manifest. Returns an error action, or nothing.""" + manifest["manifestVersion"] = MANIFEST_VERSION + manifest["entries"] = entries + try: + # json.dumps is inside the guard: indent= selects the pure-Python encoder, + # and unknown fields must be round-tripped, so a deeply nested + # planted field can raise RecursionError here — after every skill file is + # already on disk. + serialized = json.dumps(manifest, indent=2, sort_keys=True).encode("utf-8") + atomic_write_in(root, MANIFEST_FILENAME, serialized) + except Exception as exc: + return [_run_error(f"the skills manifest could not be written: {exc}")] + return [] + + +# ------------------------------------------------------------------------- +# Request resolution — content in, or a reason there is none +# ------------------------------------------------------------------------- + + +def _unavailable(reason: str) -> str: + """Wraps *reason* as a retrieval-unavailable message.""" + return f"{_UNAVAILABLE_PREFIX}{reason}" + + +@dataclass(frozen=True) +class _RetrievalBlocked: + """Why retrieval must not be attempted. The reason is caller-facing.""" + + reason: str + + +def _available_store(deadline: float, subject: str) -> SkillStore | _RetrievalBlocked: + """ + The configured store, or why retrieval must not be attempted. + + Written once because this gate is what sets ``unavailable`` and therefore + suppresses pruning. If it were maintained in two places, a condition added + to one and not the other would not merely produce a wrong message — it + would delete the user's files. + """ + if time.monotonic() >= deadline: + return _RetrievalBlocked( + _unavailable( + f"the timeout was exhausted before {subject} could be retrieved" + ) + ) + store = get_store() + if store is None: + return _RetrievalBlocked(_unavailable(NO_STORE_MESSAGE)) + return store + + +def _resolve_requests( + skills: Sequence[Skill | SkillReference | str] | str, + deadline: float, + on_unavailable: OnUnavailable, +) -> tuple[list[_PendingWrite], bool]: + """ + Turns the caller's input into one request per skill. + + Returns the requests plus whether any retrieval was left incomplete — an + absent store, a raising store, or an exhausted timeout. That flag suppresses + pruning: deleting managed files because retrieval failed would turn a + transport outage into data loss. + """ + if isinstance(skills, str): + if skills != "*": + raise ValueError( + 'write_skills takes a sequence of skills or the literal "*"; ' + f"got {skills!r}" + ) + return _resolve_all(deadline, on_unavailable) + + requests: list[_PendingWrite] = [] + incomplete = False + for item in skills: + if isinstance(item, Skill): + requests.append(_PendingWrite(key=item.key, skill=item)) + continue + + key, wanted = reference_target(item) + resolved = _resolve_reference(key, wanted, deadline) + if resolved.unavailable: + incomplete = True + if on_unavailable == "raise": + raise RuntimeError(resolved.error) + requests.append( + _PendingWrite(key=key, skill=resolved.skill, error=resolved.error) + ) + + return requests, incomplete + + +def _resolve_reference( + key: str, wanted_version: int | None, deadline: float +) -> Resolution: + """ + Resolves one reference for the materialization path. + + Same core as the accessors, plus the two conditions only this path treats as + data rather than as an exception: an exhausted deadline and an absent store. + """ + store = _available_store(deadline, f"'{key}'") + if isinstance(store, _RetrievalBlocked): + return Resolution(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 resolved + + +def _unavailable_run( + error: str, on_unavailable: OnUnavailable +) -> tuple[list[_PendingWrite], bool]: + """ + One run-level retrieval failure — raised, or reported against the empty key. + + Always reports the run incomplete, which is what suppresses pruning: nothing + was retrieved, so every managed file on disk has to be assumed current. + """ + if on_unavailable == "raise": + raise RuntimeError(error) + return [_PendingWrite(key="", error=error)], True + + +def _pending_for_raw(object_key: str, raw: Any) -> _PendingWrite: + """ + One raw store object as a pending write — verified, or reported as failed. + + Present but unverifiable is NOT the same as revoked. Dropping it silently + would leave the key out of the requested set, so prune would delete the last + known-good copy already on disk and report a routine "removed" with + report.ok still true. A failed request instead gets the same treatment the + reference path already gives (see ``_resolve_reference``): the outcome is + surfaced, and the key stays in the requested set so nothing is pruned. + """ + skill = verify_raw_skill(raw) + if skill is not None: + return _PendingWrite(key=skill.key, skill=skill) + # The on-disk copy lives under the object's *own* key, which a custom store + # may key differently in ``all_objects``. The failure must be recorded under + # the object's key, or the copy written under it on an earlier run would + # fall out of the requested set and be pruned — the very deletion this + # function exists to prevent. + raw_key = raw.get("key") if isinstance(raw, dict) else None + key = raw_key if is_valid_skill_key(raw_key) else object_key + if not is_valid_skill_key(key): + # Neither key is usable, so this failure cannot be attributed to a skill + # — the run-level sentinel is the honest report. + return _PendingWrite( + key=_RUN_LEVEL_KEY, + error="the skill store served an object under an invalid key; " + "it was withheld", + ) + return _PendingWrite( + key=key, + error=f"skill '{key}' failed integrity verification and was " + "withheld; the copy already on disk was left alone", + ) + + +def _resolve_all( + deadline: float, on_unavailable: OnUnavailable +) -> tuple[list[_PendingWrite], bool]: + """Resolves the ``"*"`` form — everything the store currently holds.""" + store = _available_store(deadline, "the skill set") + if isinstance(store, _RetrievalBlocked): + return _unavailable_run(store.reason, on_unavailable) + + # Deliberately not via all_skills(), which reports a raising store as an + # empty result — that would look like "every skill was revoked" and let + # prune delete the lot. + objects, error = list_raw_objects(store) + if error is not None: + return _unavailable_run(_unavailable(error), on_unavailable) + + # One object per key, at its newest version. ``all_objects`` may hold several + # versions of one key, and //SKILL.md is a single path — writing it + # twice in one run is a bug rather than a policy. + candidates = newest_by_key(objects) + requests = [_pending_for_raw(key, raw) for key, raw in candidates] + log_withholding_summary( + "skills held by the store", + len(requests), + sum(1 for request in requests if request.skill is not None), + ) + return requests, False + + +# ------------------------------------------------------------------------- +# The managed root and its manifest +# ------------------------------------------------------------------------- + + +def _resolve_root(root: str | os.PathLike[str]) -> Path: + """ + Resolves the managed root once, up front. + + An unusable root is a caller error rather than a per-skill outcome, so this + raises. Only the leaf directory is ever created — recursively creating + missing ancestors would let a typo scatter a directory tree. + """ + path = Path(os.fspath(root)) + + # pathlib re-raises any errno outside ENOENT/ENOTDIR/EBADF/ELOOP, so an + # unreadable parent would surface as PermissionError where the docs + # promise ValueError. + try: + is_symlink = path.is_symlink() + exists = path.exists() + is_dir = path.is_dir() + except OSError as exc: + raise ValueError(f"the skills root could not be inspected: {exc}") from exc + + if is_symlink: + raise ValueError( + f"the skills root must be a real directory, not a symlink: {path}" + ) + + if exists: + if not is_dir: + raise ValueError(f"the skills root is not a directory: {path}") + else: + parent = path.parent + try: + parent_is_dir = parent.is_dir() + except OSError as exc: + raise ValueError( + f"the parent of the skills root could not be inspected: {exc}" + ) from exc + if not parent_is_dir: + raise ValueError( + f"the parent of the skills root does not exist: {parent}. " + "write_skills creates only the leaf directory." + ) + try: + path.mkdir() + except OSError as exc: + raise ValueError(f"the skills root could not be created: {exc}") from exc + + return Path(os.path.realpath(path)) + + +def _load_manifest(root: Path) -> tuple[dict[str, Any], str | None]: + """ + Loads the manifest. Returns ``(manifest, error)``. + + A manifest that cannot be read, cannot be parsed, is not an object, carries a + ``manifestVersion`` this release does not understand, or has a malformed + ``entries`` map is **corrupt**. The caller then performs no destructive + action and leaves the file itself alone: rewriting it would destroy the only + record of what the SDK owns, and acting on a manifest we cannot read would + mean guessing at which of the customer's files are ours. + + An absent manifest is not corrupt — that is simply a fresh root. + """ + path = root / MANIFEST_FILENAME + fresh: dict[str, Any] = {"manifestVersion": MANIFEST_VERSION, "entries": {}} + + if not path.exists(): + return fresh, None + + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + # UnicodeDecodeError is a ValueError, not an OSError: non-UTF-8 bytes in + # the manifest are corruption, and must fail closed like any other. + return {}, f"the skills manifest {MANIFEST_FILENAME} could not be read: {exc}" + + try: + data = json.loads(text) + except (ValueError, RecursionError) as exc: + return {}, ( + f"the skills manifest {MANIFEST_FILENAME} is not valid JSON ({exc}); " + "refusing every destructive action" + ) + + if not isinstance(data, dict): + return {}, ( + f"the skills manifest {MANIFEST_FILENAME} is not a JSON object; " + "refusing every destructive action" + ) + + version = data.get("manifestVersion") + if ( + not isinstance(version, int) + or isinstance(version, bool) + or version > MANIFEST_VERSION + ): + return {}, ( + f"the skills manifest {MANIFEST_FILENAME} declares manifestVersion " + f"{version!r}, which this SDK cannot read; refusing every destructive " + "action" + ) + + if not isinstance(data.get("entries"), dict): + return {}, ( + f"the skills manifest {MANIFEST_FILENAME} has a malformed 'entries' " + "map; refusing every destructive action" + ) + + return data, None + + +# ------------------------------------------------------------------------- +# Per-skill reconcile +# ------------------------------------------------------------------------- + + +def _unsafe_path_reason( + root: Path, skill_dir: Path, target: Path, key: str, *, require_directory: bool +) -> str | None: + """ + The path defenses, in one place. + + Returns why ``//SKILL.md`` must not be touched, or ``None``. + Shared by the write and prune paths: ``agents.md`` marks these checks + non-relaxable, and maintaining them twice is how they drift. + + *require_directory* is the one genuine difference between the two callers. A + write needs a real directory to write into. A prune only needs to not follow + a link — an entry whose directory has been replaced by a plain file has + already lost the file this SDK owned, so reporting ``removed`` is what lets + the stale manifest entry be dropped rather than pinned forever. + + Note that the containment check is unconditional even though ``skill_dir`` + may not exist yet: ``realpath`` resolves the existing prefix and appends the + rest, so a fresh key under a valid root passes. + """ + if skill_dir.is_symlink(): + return f"{key} is a symlink" + if require_directory and skill_dir.exists() and not skill_dir.is_dir(): + return f"{key} exists and is not a directory" + if target.is_symlink(): + return "the target file is a symlink" + if Path(os.path.realpath(skill_dir)).parent != root: + return f"it resolves outside the managed root {root}" + return None + + +def _key_rejection_reason(key: Any) -> str | None: + """ + Why *key* must not become a directory name under the managed root, or ``None``. + + Re-validated locally whatever any upstream layer already did, and + before any filesystem call, because a key becomes a path component. Shared by + the write and the prune paths so the two cannot disagree about which keys + this SDK could own; ``agents.md`` marks these checks non-relaxable, and + maintaining them twice is how they drift. + + ``key.encode`` is safe here only because it runs *after* the pattern check: + the key grammar admits no surrogate, so there is no unencodable key left to + raise on. Do not reorder these two. + """ + if not is_valid_skill_key(key): + return f"{key!r} is not a valid skill key: it {skill_key_rejection_reason(key)}" + # The data model allows 256 characters; no mainstream filesystem allows a + # 256-byte path component. Catch it here so it is a reported action rather + # than an ENAMETOOLONG raised from the first stat in the caller. + key_bytes = len(key.encode("utf-8")) + if key_bytes > _MAX_PATH_COMPONENT_BYTES: + return ( + f"skill key '{key[:32]}...' is {key_bytes} bytes, over the " + f"{_MAX_PATH_COMPONENT_BYTES}-byte limit for a single directory name" + ) + return None + + +def _write_one(root: Path, skill: Skill, entries: dict[str, Any]) -> ReconcileAction: + """Reconciles one verified skill against the managed root.""" + key = skill.key + + def failed(message: str) -> ReconcileAction: + return ReconcileAction( + key=key, action="error", version=skill.version, error=message + ) + + rejection = _key_rejection_reason(key) + if rejection is not None: + return failed(f"{rejection}; nothing was written") + if not is_valid_skill_version(skill.version): + return failed( + f"skill '{key}' has version {skill.version!r}, which is not an " + "integer >= 1; nothing was written" + ) + + skill_dir = root / key + target = skill_dir / SKILL_FILENAME + relative = f"{key}/{SKILL_FILENAME}" + + unsafe = _unsafe_path_reason(root, skill_dir, target, key, require_directory=True) + if unsafe is not None: + return failed(f"'{relative}' was refused: {unsafe}; nothing was written") + + # Re-verify immediately before writing, through the same core the accessors + # use: a Skill can also be constructed directly by a caller. + verified = verified_bytes(key, skill.content, skill.content_hash, skill.version) + if isinstance(verified, VerificationFailure): + return failed( + f"skill '{key}' failed verification immediately before writing: " + f"{verified.reason}; nothing was written" + ) + encoded, content_hash = verified.encoded, verified.content_hash + + # Overwrite only what the manifest records as ours under this key. + entry = entries.get(relative) + managed = isinstance(entry, dict) and entry.get("key") == key + exists = target.exists() + + if exists and not managed: + return failed( + f"'{relative}' exists but the manifest does not record it as managed " + f"under key '{key}'; refusing to overwrite a file this SDK did not write" + ) + + if exists: + try: + on_disk = _read_regular_file(target) + except OSError as exc: + return failed(f"'{relative}' could not be read: {exc}") + + if hashlib.sha256(on_disk).hexdigest() == content_hash: + _update_entry(entries, relative, skill, content_hash) + record_materialized(key, len(encoded), content_hash, "skipped_current") + return ReconcileAction( + key=key, + action="skipped_current", + version=skill.version, + path=str(target), + ) + # Stale version or local tampering — LD-resolved content wins. + action: ReconcileActionKind = "updated" + else: + action = "written" + + write_error = _write_through_descriptor(skill_dir, encoded, key, relative) + if write_error is not None: + return failed(write_error) + + _update_entry(entries, relative, skill, content_hash) + record_materialized(key, len(encoded), content_hash, action) + return ReconcileAction( + key=key, action=action, version=skill.version, path=str(target) + ) + + +def _read_regular_file(target: Path) -> bytes: + """ + Reads *target*, refusing anything that is not a regular file. + + A plain ``Path.read_bytes`` would ``open()`` by name — and opening a FIFO + with no writer blocks forever, so an attacker who can swap the managed file + for one (the same capability the symlink checks defend against) could hang + the whole reconcile, and the event loop with it. ``O_NONBLOCK`` makes that + open return immediately (it is a no-op for regular files), ``O_NOFOLLOW`` + refuses a trailing symlink, and the ``fstat`` on the descriptor — not the + path — is what the type check trusts. ``O_BINARY`` is what keeps these + bytes the *verbatim* bytes: it is 0 on POSIX, but on Windows a descriptor + without it translates CRLF on read, which would fail the hash comparison + against content that is actually current. + """ + flags = ( + os.O_RDONLY + | getattr(os, "O_NOFOLLOW", 0) + | getattr(os, "O_NONBLOCK", 0) + | getattr(os, "O_BINARY", 0) + ) + fd = os.open(target, flags) + try: + if not stat.S_ISREG(os.fstat(fd).st_mode): + raise OSError("the target file is not a regular file") + chunks: list[bytes] = [] + while True: + chunk = os.read(fd, 65536) + if not chunk: + return b"".join(chunks) + chunks.append(chunk) + finally: + os.close(fd) + + +def _write_through_descriptor( + skill_dir: Path, encoded: bytes, key: str, relative: str +) -> str | None: + """ + Performs the write itself. Returns a failure reason, or ``None`` on success. + + Split out of ``_write_one`` because everything above it decides *whether* to + write and this decides nothing: the directory is pinned to a descriptor and + every remaining step is relative to it, so none of the checks above can be + invalidated by a swap between here and the rename. + """ + try: + with pinned_directory(skill_dir, create=True) as dir_fd: + try: + atomic_write(skill_dir, SKILL_FILENAME, encoded, dir_fd=dir_fd) + except OSError as exc: + return f"'{relative}' could not be written: {exc}" + except OSError as exc: + return f"the directory for skill '{key}' could not be created: {exc}" + except ValueError as exc: + return f"'{relative}' was refused: {exc}" + return None + + +def _update_entry( + entries: dict[str, Any], relative: str, skill: Skill, content_hash: str +) -> None: + """ + Records a managed path in the manifest. + + Merges into any existing entry rather than replacing it, so fields written by + a future SDK release survive this one's rewrite. + + ``sha256`` and ``writtenAt`` are recorded for forensics only: the reconcile + decides currency by hashing the bytes on disk, precisely because the + manifest is untrusted, so neither field is ever read back as a decision + input. + """ + existing = entries.get(relative) + entry = dict(existing) if isinstance(existing, dict) else {} + entry["key"] = skill.key + entry["version"] = skill.version + entry["sha256"] = content_hash + entry["writtenAt"] = _utc_timestamp() + entries[relative] = entry + + +# ------------------------------------------------------------------------- +# Pruning — how revocation takes effect +# ------------------------------------------------------------------------- + + +def _prune_error(key: str, message: str, version: Any = None) -> ReconcileAction: + """ + A prune refusal. Mirrors ``_write_one``'s local ``failed`` helper. + + *version* comes off the manifest, which is untrusted, so it is validated here + rather than at each call site — the same guard the ``removed`` action applies, + so a refusal and a removal report the field identically. + Callers that genuinely do not know a version pass nothing; none of them may + invent one. + """ + return ReconcileAction( + key=key, + action="error", + version=version if is_valid_skill_version(version) else None, + error=message, + ) + + +def _prune( + root: Path, entries: dict[str, Any], requested: set[str], deadline: float +) -> list[ReconcileAction]: + """ + Removes managed skills that are no longer requested. + + This is also how revocation takes effect: a revoked skill is simply absent + from the resolved set, so the next reconcile removes it. There is + deliberately no opt-out. + + The deadline applies here just as it does to the writes: a skill left + unpruned is reported as an error and stays in the manifest, so the next + reconcile picks it up. + """ + actions: list[ReconcileAction] = [] + + for relative, entry in list(entries.items()): + if not isinstance(entry, dict): + continue + key = entry.get("key") + if not isinstance(key, str) or key in requested: + continue + + if time.monotonic() >= deadline: + actions.append( + _prune_error( + key, + f"the timeout was exhausted before '{relative}' could be " + "pruned; it was left in place", + entry.get("version"), + ) + ) + continue + + # Only a manifest path this SDK could have written is removable. + if ( + _key_rejection_reason(key) is not None + or relative != f"{key}/{SKILL_FILENAME}" + ): + actions.append( + _prune_error( + key, + f"manifest entry '{relative}' does not name a path this SDK " + f"could own under key '{key}'; it was left in place", + entry.get("version"), + ) + ) + continue + + try: + actions.append(_prune_one(root, relative, key, entries)) + except OSError as exc: + actions.append( + _prune_error( + key, + f"'{relative}' could not be removed: {exc}", + entry.get("version"), + ) + ) + + return actions + + +def _unlink_through_descriptor(skill_dir: Path, relative: str) -> str | None: + """ + Performs the removal itself. Returns a failure reason, or ``None`` on success. + + The mirror of ``_write_through_descriptor``, and split out for the same + reason: everything above it decides *whether* to remove, and this decides + nothing. The directory is pinned before the unlink because unlink never + follows a trailing symlink but does resolve the directory above it, so a + ``/`` swapped for a symlink between the checks and here would + otherwise delete a file outside the root. + """ + try: + with pinned_directory(skill_dir) as dir_fd: + try: + unlink_file(skill_dir, SKILL_FILENAME, dir_fd=dir_fd) + except SymlinkRefused: + return f"'{relative}' was not removed: the target file is a symlink" + except OSError as exc: + return f"'{relative}' could not be removed: {exc}" + except ValueError as exc: + return f"'{relative}' was not removed: {exc}" + return None + + +def _prune_one( + root: Path, relative: str, key: str, entries: dict[str, Any] +) -> ReconcileAction: + """Removes one managed skill file, and its directory when that empties it.""" + skill_dir = root / key + target = skill_dir / SKILL_FILENAME + version = entries[relative].get("version") + + unsafe = _unsafe_path_reason(root, skill_dir, target, key, require_directory=False) + if unsafe is not None: + return _prune_error(key, f"'{relative}' was not removed: {unsafe}", version) + + removed_from_disk = False + if target.exists(): + failure = _unlink_through_descriptor(skill_dir, relative) + if failure is not None: + return _prune_error(key, failure, version) + removed_from_disk = True + try: + # Path-based, and safe that way: rmdir never follows a trailing + # symlink (it fails ENOTDIR) and only ever succeeds on an empty + # directory. + skill_dir.rmdir() + except OSError: + pass # the customer keeps their own files here too + + entries.pop(relative, None) + + if removed_from_disk: + record_revoked(key, version) + + return ReconcileAction( + key=key, + action="removed", + version=version if is_valid_skill_version(version) else None, + path=str(target), + ) + + +def _utc_timestamp() -> str: + return datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ") diff --git a/packages/client/src/launchdarkly_ai_server/types.py b/packages/client/src/launchdarkly_ai_server/types.py index 4249d5c..10c90d8 100644 --- a/packages/client/src/launchdarkly_ai_server/types.py +++ b/packages/client/src/launchdarkly_ai_server/types.py @@ -452,6 +452,54 @@ class Skill: """Description from LaunchDarkly metadata; never parsed from the content.""" +ReconcileActionKind = Literal[ + "written", "updated", "skipped_current", "removed", "error" +] +"""The closed set of outcomes ``write_skills`` reports.""" + + +@dataclass(frozen=True) +class ReconcileAction: + """What ``write_skills`` did — or refused to do — for one skill.""" + + key: str + """ + The skill key, or the **empty string** for a failure that belongs to the run + rather than to one skill — a corrupt manifest, a manifest that could not be + rewritten, a retrieval that failed before any key was known. Callers grouping + a report by key need to expect that sentinel; a report may carry both kinds. + """ + action: ReconcileActionKind + version: int | None = None + path: str | None = None + """Canonical resolved path, when one was determined.""" + error: str | None = None + """Failure detail, set only when ``action == "error"``.""" + + +@dataclass(frozen=True) +class ReconcileReport: + """The result of a ``write_skills`` run — every outcome is visible here.""" + + actions: list[ReconcileAction] = field(default_factory=list) + + @property + def ok(self) -> bool: + """``True`` iff no action is an ``error``.""" + return not self.errors + + @property + def errors(self) -> list[ReconcileAction]: + """ + The ``error`` actions, in ``actions`` order. + + Exposed so callers never re-derive it — filtering ``actions`` is + boilerplate that otherwise reappears in every consumer. ``ok`` is defined + in terms of this, so the two can never disagree. + """ + return [a for a in self.actions if a.action == "error"] + + # --------------------------------------------------------------------------- # Model / graph options # --------------------------------------------------------------------------- diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 203eeb4..0c656ed 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -17,6 +17,8 @@ import launchdarkly_ai_server.skills as skills_module from launchdarkly_ai_server import ( InMemorySkillStore, + ReconcileAction, + ReconcileReport, Skill, SkillReference, all_skills, @@ -129,7 +131,7 @@ def _fabricated_hash_cases() -> list[Any]: class TestSkillTypes: - """Immutability and optional metadata.""" + """Immutability, optional metadata, and ``ReconcileReport.ok``.""" def test_skill_reference_is_immutable(self) -> None: ref = SkillReference(key="pdf-extraction", version=2) @@ -164,6 +166,70 @@ def test_skill_metadata_defaults_to_none(self) -> None: assert skill.name is None assert skill.description is None + def test_report_ok_true_when_no_error_action(self) -> None: + report = ReconcileReport( + actions=[ + ReconcileAction(key="a", action="written", version=1), + ReconcileAction(key="b", action="skipped_current", version=2), + ReconcileAction(key="c", action="removed"), + ReconcileAction(key="d", action="updated", version=3), + ] + ) + assert report.ok is True + + def test_report_ok_false_with_error_action(self) -> None: + report = ReconcileReport( + actions=[ + ReconcileAction(key="a", action="written", version=1), + ReconcileAction(key="b", action="error", error="nope"), + ] + ) + assert report.ok is False + + def test_empty_report_is_ok(self) -> None: + assert ReconcileReport(actions=[]).ok is True + + def test_report_errors_lists_error_actions_in_order(self) -> None: + """The report exposes its error actions itself.""" + first = ReconcileAction(key="b", action="error", error="first") + second = ReconcileAction(key="d", action="error", error="second") + report = ReconcileReport( + actions=[ + ReconcileAction(key="a", action="written", version=1), + first, + ReconcileAction(key="c", action="skipped_current", version=2), + second, + ReconcileAction(key="e", action="removed"), + ] + ) + assert report.errors == [first, second] + + def test_report_errors_empty_when_no_error_action(self) -> None: + report = ReconcileReport( + actions=[ + ReconcileAction(key="a", action="written", version=1), + ReconcileAction(key="b", action="removed"), + ] + ) + assert report.errors == [] + + def test_empty_report_has_no_errors(self) -> None: + assert ReconcileReport(actions=[]).errors == [] + + def test_report_ok_and_errors_always_agree(self) -> None: + """``ok`` is true iff ``errors`` is empty, on the same objects.""" + clean = ReconcileReport( + actions=[ReconcileAction(key="a", action="written", version=1)] + ) + failed = ReconcileReport( + actions=[ + ReconcileAction(key="a", action="written", version=1), + ReconcileAction(key="b", action="error", error="nope"), + ] + ) + for report in (clean, failed, ReconcileReport(actions=[])): + assert report.ok is (report.errors == []) + class TestSkillRefs: """Pure projection of the config's skills array.""" @@ -267,6 +333,59 @@ def test_object_kind_is_not_public_api(self) -> None: assert "SKILL_OBJECT_KIND" not in package.__all__ assert not hasattr(package, "SKILL_OBJECT_KIND") + def test_constants_are_exported_from_the_package_root(self) -> None: + import launchdarkly_ai_server as package + + assert package.SKILL_FILENAME == "SKILL.md" + assert package.MANIFEST_FILENAME == ".launchdarkly-skills.json" + assert package.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. + + Spelled out rather than imported from the implementation for the same + reason as the constants above: deriving the expectation from the thing + under test would make the assertion circular. + """ + import typing + + import launchdarkly_ai_server as package + + assert set(typing.get_args(package.ReconcileActionKind)) == { + "written", + "updated", + "skipped_current", + "removed", + "error", + } + assert set(typing.get_args(package.OnUnavailable)) == {"keep", "raise"} + 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 @@ -811,7 +930,7 @@ class TestWithholdingSummary: """ def _tampered(self, make_raw_skill: Any, key: str = "a") -> dict[str, Any]: - raw = make_raw_skill(key=key) + raw: dict[str, Any] = make_raw_skill(key=key) raw["contentHash"] = "0" * 64 return raw diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py new file mode 100644 index 0000000..62d01d5 --- /dev/null +++ b/packages/client/tests/test_skills_fs.py @@ -0,0 +1,582 @@ +""" +Tests for ``write_skills`` — filesystem materialization and manifest reconcile +semantics. + +Every test writes only inside pytest's ``tmp_path``. No network, no real +LaunchDarkly client, no real skill transport. + +The security abuse matrix — path traversal, symlink attacks, clobber +protection, corrupt manifests, atomicity under an injected crash, and the +materialization telemetry allowlist — is a separate module. +""" + +from __future__ import annotations + +import hashlib +import json +import os +from pathlib import Path +from typing import Any + +import pytest + +import launchdarkly_ai_server.skills as skills_module +from launchdarkly_ai_server import ( + InMemorySkillStore, + Skill, + SkillReference, + write_skills, +) + +MANIFEST_NAME = ".launchdarkly-skills.json" +SKILL_BODY = "---\nname: Test Skill\n---\nDo the thing.\n" + +INTEGRITY_SIGNAL = "AgentControl Skill Integrity Failure" + + +@pytest.fixture +def root(tmp_path: Path) -> Path: + r = tmp_path / "skills" + r.mkdir() + return r + + +pytestmark = pytest.mark.usefixtures("reset_skill_state") +"""Every test in this module runs against freshly cleared module state.""" + + +def _hash(content: str) -> str: + return hashlib.sha256(content.encode("utf-8")).hexdigest() + + +def _skill( + key: str = "test-skill", version: int = 1, content: str = SKILL_BODY +) -> Skill: + return Skill( + key=key, + version=version, + content=content.encode("utf-8"), + content_hash=_hash(content), + ) + + +def _manifest_path(root: Path) -> Path: + return root / MANIFEST_NAME + + +def _read_manifest(root: Path) -> dict[str, Any]: + return json.loads(_manifest_path(root).read_text(encoding="utf-8")) + + +def _write_manifest(root: Path, raw: Any) -> None: + root.mkdir(parents=True, exist_ok=True) + _manifest_path(root).write_text( + raw if isinstance(raw, str) else json.dumps(raw), encoding="utf-8" + ) + + +def _entry(key: str, version: int, content: str) -> dict[str, Any]: + return { + "key": key, + "version": version, + "sha256": _hash(content), + "writtenAt": "2026-08-14T19:00:00Z", + } + + +def _place_managed(root: Path, key: str, content: str, version: int = 1) -> Path: + """Pre-create a file AND its manifest entry — i.e. an SDK-managed path.""" + target = root / key / "SKILL.md" + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": {f"{key}/SKILL.md": _entry(key, version, content)}, + }, + ) + return target + + +def _actions_by_key(report: Any) -> dict[str, Any]: + return {a.key: a for a in report.actions} + + +def _error_messages(report: Any) -> list[str]: + """All ``error`` action messages, regardless of which key they hang off. + + Run-level (manifest) errors have no well-defined ``key`` yet, so assertions + about them scan every error action rather than looking one up by key. + """ + return [a.error or "" for a in report.actions if a.action == "error"] + + +class TestBasicWrites: + """Basic writes and the returned report.""" + + async def test_new_skill_is_written_verbatim(self, root: Path) -> None: + report = await write_skills([_skill("pdf-extraction", 2)], root) + + target = root / "pdf-extraction" / "SKILL.md" + assert target.read_text(encoding="utf-8") == SKILL_BODY + assert report.ok is True + action = _actions_by_key(report)["pdf-extraction"] + assert action.action == "written" + assert action.version == 2 + assert action.path is not None + assert Path(action.path).resolve() == target.resolve() + assert action.error is None + + async def test_skill_inputs_need_no_store(self, root: Path) -> None: + report = await write_skills([_skill("a")], root) + assert report.ok is True + assert (root / "a" / "SKILL.md").exists() + + async def test_reference_inputs_resolve_through_store( + self, root: Path, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=3)) + report = await write_skills([SkillReference(key="a", version=3)], root) + assert report.ok is True + assert (root / "a" / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + + async def test_string_inputs_resolve_latest( + self, root: Path, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=9)) + report = await write_skills(["a"], root) + assert _actions_by_key(report)["a"].version == 9 + + async def test_star_writes_everything_in_the_store( + self, root: Path, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + for k in ("a", "b", "c"): + store.put(make_raw_skill(key=k)) + report = await write_skills("*", root) + assert report.ok is True + assert len([a for a in report.actions if a.action == "written"]) == 3 + for k in ("a", "b", "c"): + assert (root / k / "SKILL.md").exists() + + async def test_one_action_per_requested_skill(self, root: Path) -> None: + report = await write_skills([_skill("a"), _skill("b")], root) + assert sorted(a.key for a in report.actions) == ["a", "b"] + + async def test_empty_request_on_empty_root_is_ok(self, root: Path) -> None: + report = await write_skills([], root) + assert report.ok is True + assert report.actions == [] + + +class TestManifest: + """Manifest format and forward compatibility.""" + + async def test_manifest_format_is_exact(self, root: Path) -> None: + await write_skills([_skill("pdf-extraction", 2)], root) + + manifest = _read_manifest(root) + assert manifest["manifestVersion"] == 1 + entry = manifest["entries"]["pdf-extraction/SKILL.md"] + assert entry["key"] == "pdf-extraction" + assert entry["version"] == 2 + assert entry["sha256"] == _hash(SKILL_BODY) + assert isinstance(entry["writtenAt"], str) + + async def test_entry_paths_are_forward_slash_relative(self, root: Path) -> None: + await write_skills([_skill("a")], root) + keys = list(_read_manifest(root)["entries"].keys()) + assert keys == ["a/SKILL.md"] + assert "\\" not in keys[0] + assert not keys[0].startswith("/") + + async def test_unknown_fields_are_preserved_on_rewrite(self, root: Path) -> None: + entry = _entry("a", 1, SKILL_BODY) + entry["futureEntryField"] = "keep-me" + _write_manifest( + root, + { + "manifestVersion": 1, + "futureTopLevelField": {"keep": True}, + "entries": {"a/SKILL.md": entry}, + }, + ) + (root / "a").mkdir() + (root / "a" / "SKILL.md").write_text(SKILL_BODY, encoding="utf-8") + + await write_skills([_skill("a", 2, SKILL_BODY + "more\n")], root) + + manifest = _read_manifest(root) + assert manifest["futureTopLevelField"] == {"keep": True} + assert manifest["entries"]["a/SKILL.md"]["futureEntryField"] == "keep-me" + + +class TestReconcileSemantics: + """The reconcile state table.""" + + async def test_unchanged_managed_file_is_skipped_current(self, root: Path) -> None: + target = _place_managed(root, "a", SKILL_BODY) + before = target.stat().st_mtime_ns + + report = await write_skills([_skill("a")], root) + + assert _actions_by_key(report)["a"].action == "skipped_current" + assert target.read_text(encoding="utf-8") == SKILL_BODY + assert target.stat().st_mtime_ns == before + + async def test_new_version_updates(self, root: Path) -> None: + _place_managed(root, "a", SKILL_BODY, version=1) + new_content = SKILL_BODY + "second version\n" + + report = await write_skills([_skill("a", 2, new_content)], root) + + action = _actions_by_key(report)["a"] + assert action.action == "updated" + assert action.version == 2 + assert (root / "a" / "SKILL.md").read_text(encoding="utf-8") == new_content + assert _read_manifest(root)["entries"]["a/SKILL.md"]["version"] == 2 + + async def test_local_tampering_is_overwritten(self, root: Path) -> None: + target = _place_managed(root, "a", SKILL_BODY) + target.write_text("locally tampered\n", encoding="utf-8") + + report = await write_skills([_skill("a")], root) + + assert _actions_by_key(report)["a"].action == "updated" + assert target.read_text(encoding="utf-8") == SKILL_BODY + + async def test_prune_removes_formerly_managed_skill(self, root: Path) -> None: + _place_managed(root, "gone", SKILL_BODY) + + report = await write_skills([], root) + + assert _actions_by_key(report)["gone"].action == "removed" + assert not (root / "gone" / "SKILL.md").exists() + assert not (root / "gone").exists() + assert _read_manifest(root)["entries"] == {} + + async def test_prune_false_keeps_the_file(self, root: Path) -> None: + target = _place_managed(root, "gone", SKILL_BODY) + + report = await write_skills([], root, prune=False) + + assert target.exists() + assert [a for a in report.actions if a.action == "removed"] == [] + assert "gone/SKILL.md" in _read_manifest(root)["entries"] + + async def test_prune_does_not_touch_unmanaged_files(self, root: Path) -> None: + _place_managed(root, "gone", SKILL_BODY) + bystander = root / "user-notes.md" + bystander.write_text("mine\n", encoding="utf-8") + user_dir_file = root / "user-skill" / "SKILL.md" + user_dir_file.parent.mkdir() + user_dir_file.write_text("hand written\n", encoding="utf-8") + + await write_skills([], root) + + assert bystander.read_text(encoding="utf-8") == "mine\n" + assert user_dir_file.read_text(encoding="utf-8") == "hand written\n" + + async def test_prune_refusal_for_unownable_path_reports_the_version( + self, root: Path + ) -> None: + """A prune refusal carries the manifest's version. + + A manifest entry whose path is not one this SDK could have written is + refused rather than removed. The entry is in hand at that point, so the + error action must carry its version — otherwise a prune *failure* is + strictly less informative than a prune *success*, which does report it. + """ + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": { + # Right key, wrong filename — not a path this SDK could own. + "orphan/NOTES.md": _entry("orphan", 7, SKILL_BODY), + }, + }, + ) + + report = await write_skills([], root) + + action = _actions_by_key(report)["orphan"] + assert action.action == "error" + assert action.version == 7 + + async def test_prune_refusal_for_symlinked_target_reports_the_version( + self, root: Path + ) -> None: + """Same contract on the symlink refusal path (prune side).""" + if not hasattr(os, "symlink"): + pytest.skip("platform has no symlink support") + (root / "a").mkdir() + outside_file = root.parent / "victim.md" + outside_file.write_text("victim content\n", encoding="utf-8") + (root / "a" / "SKILL.md").symlink_to(outside_file) + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": {"a/SKILL.md": _entry("a", 4, "victim content\n")}, + }, + ) + + report = await write_skills([], root) + + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert action.version == 4 + + async def test_unresolvable_request_still_reports_no_version( + self, root: Path + ) -> None: + """The other half of the contract: do not invent a version. + + A reference that could not be retrieved has neither a manifest entry + nor a ``Skill``, so there is no version to report and ``version`` stays + ``None``. Without this, "always populate version" would be satisfied by + fabricating one. + """ + report = await write_skills([SkillReference(key="ghost", version=3)], root) + + action = _actions_by_key(report)["ghost"] + assert action.action == "error" + assert action.version is None + + async def test_prune_keeps_directory_when_not_empty(self, root: Path) -> None: + _place_managed(root, "a", SKILL_BODY) + extra = root / "a" / "user-file.txt" + extra.write_text("keep\n", encoding="utf-8") + + report = await write_skills([], root) + + assert _actions_by_key(report)["a"].action == "removed" + assert not (root / "a" / "SKILL.md").exists() + assert extra.exists() + + +class TestRootHandling: + """Root resolution.""" + + async def test_absent_leaf_root_is_created(self, tmp_path: Path) -> None: + target_root = tmp_path / "skills" + report = await write_skills([_skill("a")], target_root) + assert report.ok is True + assert (target_root / "a" / "SKILL.md").exists() + + async def test_missing_ancestors_raise(self, tmp_path: Path) -> None: + with pytest.raises(ValueError): + await write_skills([_skill("a")], tmp_path / "a" / "b" / "c") + + async def test_root_that_is_a_file_raises(self, tmp_path: Path) -> None: + file_root = tmp_path / "not-a-dir" + file_root.write_text("x", encoding="utf-8") + with pytest.raises(ValueError): + await write_skills([_skill("a")], file_root) + + async def test_accepts_string_root(self, root: Path) -> None: + report = await write_skills([_skill("a")], str(root)) + assert report.ok is True + + +class TestSkillsArgumentErrors: + """A bare string that is not ``"*"`` raises. + + A ``ValueError``, not a ``TypeError``: a string *is* an accepted argument + type here, since ``"*"`` means "everything the store holds", so this is an + acceptable type carrying an invalid value. The accessors' equivalent guard + is a ``TypeError`` because a string is never a valid argument there. + """ + + async def test_bare_non_star_string_raises_value_error(self, root: Path) -> None: + with pytest.raises(ValueError) as excinfo: + await write_skills("pdf-extraction", root) + + # Naming the accepted forms is the actionable half of the message. + assert '"*"' in str(excinfo.value) + + async def test_star_is_accepted(self, root: Path) -> None: + """Positive control — otherwise the guard above could reject every string.""" + store = InMemorySkillStore() + store.put( + { + "key": "a", + "version": 1, + "content": SKILL_BODY, + "contentHash": _hash(SKILL_BODY), + } + ) + skills_module._set_store(store) + + report = await write_skills("*", root) + + assert report.ok is True + assert (root / "a" / "SKILL.md").exists() + + async def test_bare_string_writes_nothing(self, root: Path) -> None: + """The raise precedes any filesystem work. + + Asserting only the raise would also pass for an implementation that + created one directory per character before failing. + """ + with pytest.raises(ValueError): + await write_skills("abc", root) + + assert list(root.iterdir()) == [] + + +class TestResilience: + """Unavailable retrieval and timeout.""" + + async def test_keep_is_the_default_and_does_not_raise(self, root: Path) -> None: + existing = _place_managed(root, "a", SKILL_BODY) + + report = await write_skills([SkillReference(key="a", version=1)], root) + + assert report.ok is False + assert _actions_by_key(report)["a"].action == "error" + assert existing.read_text(encoding="utf-8") == SKILL_BODY + + async def test_raise_mode_propagates(self, root: Path) -> None: + with pytest.raises(RuntimeError, match=r"(?i)(unavailable|skill store)"): + await write_skills( + [SkillReference(key="a", version=1)], root, on_unavailable="raise" + ) + + async def test_store_error_is_reported_not_raised( + self, root: Path, exploding_store: Any + ) -> None: + report = await write_skills([SkillReference(key="a", version=1)], root) + + assert report.ok is False + assert _actions_by_key(report)["a"].action == "error" + + async def test_exhausted_timeout_behaves_as_unavailable( + self, root: Path, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a")) + report = await write_skills( + [SkillReference(key="a", version=1)], root, timeout=0 + ) + assert report.ok is False + assert not (root / "a" / "SKILL.md").exists() + + async def test_exhausted_timeout_raises_in_raise_mode( + self, root: Path, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a")) + with pytest.raises(RuntimeError, match=r"(?i)(unavailable|timeout|timed out)"): + await write_skills( + [SkillReference(key="a", version=1)], + root, + timeout=0, + on_unavailable="raise", + ) + + async def test_exhausted_timeout_stops_pruning( + self, root: Path, store: InMemorySkillStore + ) -> None: + """The deadline bounds pruning too, not just retrieval and the writes. + + A run whose writes all land just inside the deadline would otherwise go + on to stat, unlink and rmdir every stale manifest entry unbounded — the + opposite of what a small ``timeout`` asks for. + """ + existing = _place_managed(root, "stale", SKILL_BODY) + + report = await write_skills([], root, timeout=0) + + assert report.ok is False + assert existing.exists(), "prune ran past the exhausted deadline" + assert any("timeout was exhausted" in m for m in _error_messages(report)) + # The entry survives, so the next reconcile picks it up. + assert "stale/SKILL.md" in _read_manifest(root)["entries"] + + async def test_a_verification_failure_never_prunes_the_good_copy( + self, root: Path + ) -> None: + """A store may key ``all_objects`` differently from the object's own key. + + The on-disk copy lives under the object's own key, so a failure recorded + under the *store's* dict key would drop the real key out of the + requested set and let prune delete the last known-good copy. + """ + + class AliasKeyedStore: + """Keys objects by an internal id, not by the skill's own key.""" + + def __init__(self, raw: dict[str, Any]) -> None: + self._raw = raw + + def get_object(self, kind: str, key: str) -> dict[str, Any] | None: + return None + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + return {"internal-uuid-1": self._raw} + + existing = _place_managed(root, "pdf-extraction", SKILL_BODY) + tampered = { + "key": "pdf-extraction", + "version": 1, + "content": "tampered\n", + "contentHash": _hash(SKILL_BODY), # does not match the content + } + skills_module._set_store(AliasKeyedStore(tampered)) + + report = await write_skills("*", root) + + assert report.ok is False + assert existing.read_text(encoding="utf-8") == SKILL_BODY + assert [a.action for a in report.actions] == ["error"] + assert _actions_by_key(report)["pdf-extraction"].action == "error" + assert "pdf-extraction/SKILL.md" in _read_manifest(root)["entries"] + + async def test_unavailable_run_does_not_corrupt_manifest(self, root: Path) -> None: + _place_managed(root, "a", SKILL_BODY) + before = _read_manifest(root) + + await write_skills([SkillReference(key="b", version=1)], root) + + assert ( + _read_manifest(root)["entries"]["a/SKILL.md"] + == (before["entries"]["a/SKILL.md"]) + ) + + +class TestVerifyThenWrite: + """Hash re-verified immediately before writing.""" + + async def test_hash_mismatch_aborts_the_write( + self, root: Path, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + bad = Skill( + key="a", + version=1, + content=SKILL_BODY.encode("utf-8"), + content_hash="0" * 64, + ) + + report = await write_skills([bad], root) + + assert report.ok is False + assert _actions_by_key(report)["a"].action == "error" + assert not (root / "a" / "SKILL.md").exists() + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_oversize_skill_aborts_the_write(self, root: Path) -> None: + oversize = "x" * (64 * 1024 + 1) + report = await write_skills([_skill("a", 1, oversize)], root) + assert report.ok is False + assert not (root / "a" / "SKILL.md").exists() + + async def test_mismatch_does_not_disturb_existing_managed_file( + self, root: Path + ) -> None: + target = _place_managed(root, "a", SKILL_BODY) + bad = Skill(key="a", version=2, content=b"new content\n", content_hash="f" * 64) + + await write_skills([bad], root) + + assert target.read_text(encoding="utf-8") == SKILL_BODY