diff --git a/packages/client/README.md b/packages/client/README.md index 8691bab..1123e9f 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -220,6 +220,126 @@ 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, 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, write_skills, + InMemorySkillStore, +) + +SKILL_MD = "---\nname: PDF Extraction\n---\nExtract text from PDFs.\n" + +async def main(): + # A store supplies skill content. InMemorySkillStore is the dict-backed + # store for local development, testing, and bring-your-own-content use. + store = InMemorySkillStore() + store.put({ + "key": "pdf-extraction", + "version": 2, + "content": SKILL_MD, + # sha256, lowercase hex, over the verbatim utf-8 bytes. Content whose hash + # does not match is withheld, so this is not optional. + "contentHash": hashlib.sha256(SKILL_MD.encode("utf-8")).hexdigest(), + }) + await init_client(options={"skillStore": store}) + + # 1. Which skills does this config reference? Pure projection — no I/O. + info = await inspect_config("doc-agent", {"kind": "user", "key": "user-123"}) + refs = skill_refs(info["config"]) # [SkillReference(key='pdf-extraction', version=2)] + + # 2. Fetch content. Returns None rather than raising when a skill is unavailable. + skill = await get_skill("pdf-extraction") + if skill is not None: + print(skill.content) + + # 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 +`extract_variation` raises. A variation that previously carried its own custom `skills` +field of a different shape must rename it before upgrading. + +**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. + +**Integrity is not optional.** Content is only returned after its sha256 (lowercase hex, +over the verbatim UTF-8 bytes) matches the delivered `contentHash`, its key and version +revalidate, and its size is within 64 KiB. Anything that fails is withheld and treated as +missing — no unverified content ever reaches your code. + +**`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 | +|---|---| +| `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when absent. | +| `get_skill(key, *, version=None)` | One verified skill, or `None`. `version=None` means newest available; a specific `version` matches exactly. Raises only when no store is configured. | +| `get_skills(refs)` | Batch form. Accepts `SkillReference` values and bare key strings (string = latest). Results follow input order; missing or unverifiable entries are omitted. | +| `all_skills()` | Every verified skill the store holds. | +| `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)`, `all_objects(kind)`, optional `add_listener(kind, fn)`. | +| `InMemorySkillStore(objects=None)` | A dict-backed store with `put(raw)`, for local development and testing. | + +Configure the store with `init_client(options={"skillStore": store})`. With none configured, +the three accessors raise `RuntimeError` explaining what to do; `write_skills` reports the +failure in its report (or raises, with `on_unavailable="raise"`). + +`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 and on-the-wire values are exported too, so you do not have to hardcode +them: `MANIFEST_FILENAME` (`.launchdarkly-skills.json`, handy for a `.gitignore`), +`SKILL_FILENAME`, `MANIFEST_VERSION`, and `SKILL_OBJECT_KIND`. 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. + +> `Skill.frontmatter()` parses the leading YAML block as a convenience. It needs `pyyaml`, +> which is **not** a dependency of this package — install it yourself if you want that +> method to return anything. It returns `None` rather than raising when the library is +> missing, the block is absent or oversize, or the YAML is hostile. + +--- + ### Utility Helpers ```python @@ -250,3 +370,7 @@ All types are exported from this package. Handler packages import them from here | `GraphNode` / `GraphEdge` | A dataclass node (`.key`, `.config`, `.meta`, `.edges`, `.is_terminal`) and a dataclass directed edge (`.key`, `.source_key`, `.target_key`, `.handoff`) | | `ProviderGraphResponse` | A dataclass returned by `graph(...).invoke()`: `.response`, `.usage`, `.judge_results` | | `GraphTopology` | The parsed graph flag shape (`root` + `edges`) | +| `Skill` | A frozen `SKILL.md` document: `.key`, `.version`, `.content`, `.content_hash`, `.name?`, `.description?`, and `.frontmatter()` | +| `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 61d99cf..c6845db 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -27,7 +27,12 @@ No other `launchdarkly-ai-*` package may define or duplicate these. They import | `src/launchdarkly_ai_server/tracking.py` | `execute_and_track`, `execute_and_stream`, `wrap_tool_handlers`, `parse_usage` | | `src/launchdarkly_ai_server/graph.py` | `graph()`, `resolve_graph()`, `GraphInstance` | | `src/launchdarkly_ai_server/types.py` | All shared Python types — `AiConfigRep`, `ProviderHandler`, `LDContext`, `NativeTool`, etc. | -| `src/launchdarkly_ai_server/types_validation.py` | `parse_ai_config` — validates flag variation shape | +| `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 `skills_fs.py`; 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/frontmatter.py` | Bounded, safe YAML parse of a `SKILL.md` frontmatter block, for `Skill.frontmatter()`. Never in the integrity path | | `src/launchdarkly_ai_server/utils.py` | `parse_template`, `parse_json_with_possible_fences`, `create_handler`, `parse_usage`, `make_track_data`, `to_ld_context` | | `src/launchdarkly_ai_server/registry.py` | `Registry`, `global_registry`, `compose`, `resolve_handlers`, `resolve_tools` | | `src/launchdarkly_ai_server/judges.py` | `run_judges`, `build_judge_tasks`, `run_judge` | @@ -53,6 +58,7 @@ from launchdarkly_ai_server import ( TrackData, UsageDict, HandlerResult, HandlerStreamEvent, StreamEvent, StreamChunkEvent, StreamDoneEvent, ExecuteStreamEvent, ExecuteStreamDoneEvent, VariationMeta, InitClientOptions, JudgeResult, ParseResult, ParseSuccess, ParseFailure, + Skill, SkillReference, ReconcileAction, ReconcileReport, ) # Utilities @@ -69,8 +75,20 @@ from launchdarkly_ai_server import execute_and_track, execute_and_stream, wrap_t # Entry points from launchdarkly_ai_server import config, graph, resolve_graph + +# Agent Skills +from launchdarkly_ai_server import ( + skill_refs, get_skill, get_skills, all_skills, write_skills, + SkillStore, InMemorySkillStore, + SKILL_OBJECT_KIND, SKILL_FILENAME, MANIFEST_FILENAME, MANIFEST_VERSION, + ReconcileActionKind, OnUnavailable, # the two closed-set unions +) ``` +`MAX_SKILL_CONTENT_BYTES` is deliberately *not* among them: it is a local enforcement +bound on content the platform produces, not a value this SDK defines, so exporting it +would semver-lock a number this side does not own. Keep it internal to `skills_core`. + When adding a new export, add it to `__init__.py`'s imports and `__all__`. Handler packages must never import from sub-paths (e.g. `launchdarkly_ai_server.client`). --- @@ -155,6 +173,191 @@ This is an OTel context value, not W3C baggage, so the id does not leak onto out --- +## Agent Skills + +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: + +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. +2. **Content accessors** — `get_skill`, `get_skills`, `all_skills` read through the + `SkillStore` seam. Configure a store with + `init_client(options={"skillStore": store})`; with none configured the accessors raise + an actionable `RuntimeError`. A delivery transport can be added behind the seam + without touching the public API. +3. **Materialization** — `write_skills(skills, root)` writes `//SKILL.md` and + reconciles against a manifest at `/.launchdarkly-skills.json`. + +### Security posture — do not relax any of this + +Store data is **untrusted input**; the transport is not part of the trust boundary. + +- **Integrity is mandatory and doubled, through one implementation.** Both passes call + `skills_core.verified_bytes`, so the integrity signal's property set cannot depend on which + layer caught the defect. Every raw object is verified at the accessor + boundary (key pattern and length, integer version >= 1, content at most 64 KiB, sha256 + lowercase hex over the verbatim UTF-8 bytes against `contentHash`), and the hash is + re-verified immediately before writing. A `Skill` is only ever constructed from content + that passed. Nothing unverified reaches user code. +- **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. +- **`frontmatter()` is never in the integrity or write path.** It is a lazy convenience: + `yaml` is imported *inside* the method (`pyyaml` is a dev-only dependency and must not + become a runtime one), the block is bounded to 8 KB and depth 10, aliases are disabled + outright, and it returns `None` on any failure rather than raising. The import, the + loader subclass, and the parse all sit inside **one** `except Exception` — `import yaml` + succeeding does not prove PyYAML is what was imported. +- **Delimiters are anchored at column 0** (`rstrip()`, never `strip()`). Left-stripping + would let an indented `---` inside a block scalar terminate the block early and return a + *truncated but valid* mapping — silently wrong data the caller cannot detect. +- **No unencodable string ever reaches an encode.** `json.loads` turns a `\ud800` escape + into an unpaired surrogate with no UTF-8 representation; all three `.encode("utf-8")` + sites treat that as a verification failure. Never reach for `errors="surrogatepass"` — + fabricating bytes could satisfy the hash comparison. +- **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. +- **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. + +### Telemetry seam + +Skills telemetry goes through a private emitter with one method, +`record(signal, properties)`, whose default implementation is a **no-op** — nothing leaves +the process in this release. `client.track()` is deliberately *not* used: it needs an LD +context, spends the customer's event volume, lands in their data export, and is silenced by +offline mode. No LD context is involved anywhere in this feature. + +Exactly three signals exist, and the list is an **allowlist, not a floor**: + +| Signal | When | Properties | +|---|---|---| +| `AgentControl Skill Integrity Failure` | any hash/size/shape verification failure | `skill_key`, `version?`, `expected_hash?`, `observed_hash?`, `language` | +| `AgentControl Skill Materialized` | each `written` / `updated` / `skipped_current` | `skill_key`, `content_bytes`, `content_hash`, `reconcile_action`, `language` | +| `AgentControl Skill Revoked Received` | prune removes a formerly managed skill | `skill_key`, `version`, `removed_from_disk`, `language` | + +`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 (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`, 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. + +The injection path is deliberately narrower than the state's location: `skills.py` owns +`_set_store`, `_set_emitter_for_testing` and `_clear_state`, which delegate to +`skills_core`. `init_client` and `shutdown` use those, tests inject through those +(`skills._set_store(store)` is the same setter `init_client` uses), and neither should +reach into `skills_core` directly. + +### Descriptor-pinned filesystem access — the `/` swap window + +A path check is only as good as the last path resolution after it. Every `lstat`, +`realpath`, and containment check — all of them in one shared `_unsafe_path_reason`, so +the write and prune paths cannot drift — validates an *inode*, but +a following `os.replace(tmp, root / key / "SKILL.md")` re-resolves `/` from its +*name* — so an attacker holding write permission on the managed root can move the validated +directory aside, leave a symlink in its place, and redirect the write (or, on the prune +path, the unlink) outside the root. Narrowing that window is not a fix; the race is +winnable at any width. + +So the checks hand off to a descriptor and nothing re-resolves a path afterwards. The +primitives live in `safe_fs.py`, which knows nothing about skills: + +- `open_directory_nofollow` opens the directory with `O_RDONLY | O_DIRECTORY | O_NOFOLLOW` + and confirms `S_ISDIR` on the `fstat` (the explicit check is what covers platforms with no + `O_DIRECTORY`). `open_or_create_directory` wraps it with `os.mkdir` plus an `lstat` on the + `FileExistsError` path — `Path.mkdir(exist_ok=True)` accepts a symlink-to-directory as + "already there", which would reopen the hole the caller's check just closed. +- `atomic_write` creates the temp file with `O_CREAT | O_EXCL | O_NOFOLLOW` **at** that + descriptor (`_mkstemp_at`, since `tempfile` has no `dir_fd` form), `fchmod`s the + descriptor rather than `chmod`ing a path, and renames with + `os.replace(tmp, "SKILL.md", src_dir_fd=fd, dst_dir_fd=fd)`. The manifest takes the same + route through `atomic_write_in`, against a descriptor on the root. +- `_prune_one` unlinks descriptor-relative too. `unlink` never follows a *trailing* symlink, + but it does resolve the directory above it, so the same swap turns a prune into a delete + of an attacker-chosen file. `rmdir` stays path-based and is safe that way: it fails + `ENOTDIR` on a symlink and only ever succeeds on an empty directory. + +`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` +under `os.rename` only and `fstatat` under `os.stat` only — even though `os.replace` is the +same `renameat`-backed function and `os.lstat` is `fstatat` with `AT_SYMLINK_NOFOLLOW`. +Probing the names this module actually calls reports "unsupported" on every POSIX platform +and silently turns the defense off, so the probe names the advertised twins +(`{os.rename, os.open, os.unlink, os.stat}`) and `skills_fs._prune_one` spells its symlink check +`os.stat(..., follow_symlinks=False)` rather than `os.lstat`. Where the family is absent +(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; 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 the `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. The + shipped default returns "unavailable"; `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 that 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. + +--- + ## OTel Setup The core client owns all OTel initialization. `init_client()` configures a `TracerProvider` with `ConversationIdSpanProcessor` and a `BatchSpanProcessor` plus an OTLP HTTP exporter when the optional OTel packages are installed. @@ -230,6 +433,44 @@ When `enabled` is `False`, `config` is always `None`. When `enabled` is `True` b --- +## Dependencies + +Tier 0, so the runtime surface is deliberately tiny: **one** hard dependency, and everything else either an optional extra, resolved dynamically at runtime, or dev-only. Nothing here may grow without a reason recorded in this table. + +### Runtime (`[project] dependencies`) + +| Package | Why | +|---|---| +| `opentelemetry-api>=1.25` | The tracer/span API used on every instrumented path (`tracking.py`, `graph.py`, `content.py`, `conversation.py`, `utils.py`). API-only — the *SDK* half is an optional extra, so a consumer that never configures OTel gets no-op spans rather than an `ImportError`. `conversation.py` imports `opentelemetry.sdk.trace.SpanProcessor` under `TYPE_CHECKING` only, for exactly this reason. | + +There is deliberately **no** `python-dotenv` here: `lifecycle.py` reads `os.environ` directly, so loading a `.env` file is the application's job rather than the SDK's. `python-dotenv` is in the workspace dev group for the examples only. + +### Optional extra (`[project.optional-dependencies] otel`) + +| Package | Why | +|---|---| +| `opentelemetry-sdk>=1.25` | Tracer provider, resources, and the batch span processor, imported inside `_setup_telemetry()` in `lifecycle.py`. Optional so telemetry is opt-in; absent ⇒ a `logger.warning` and no spans, never a raise. | +| `opentelemetry-exporter-otlp-proto-http>=1.25` | OTLP/HTTP span export and its compression enum. Same optionality, same loader. | + +Install with `pip install "launchdarkly-ai-server[otel]"`; see [OTel Setup](#otel-setup) for the endpoint variables. + +### Resolved dynamically, declared nowhere + +| Package | Why | +|---|---| +| `launchdarkly-server-sdk` | The LaunchDarkly server SDK, reached by `importlib.import_module("ldclient")` (falling back to `launchdarkly_server_sdk`) inside `init_client()`'s options path. Undeclared on purpose: the BYOC path (`init_client(client=...)`) targets environments that supply their own client, and a hard dependency would force an unused SDK into every such install. So it is imported late and raises actionably when missing — absent ⇒ a `RuntimeError` naming the `pip install`, and only on the path that needs it. | + +### Dev-only (workspace root `[dependency-groups] dev`) — the ones with a contract attached + +| Package | Why | +|---|---| +| `pyyaml>=6` | Parses `SKILL.md` frontmatter for `Skill.frontmatter()`. **Dev-only on purpose, and it must stay that way** — installing this SDK must never pull in a YAML library, so `frontmatter.py` does `import yaml` *inside* `parse_block()` and returns `None` when it is absent. Promoting it to `[project] dependencies` or to an extra breaks that contract. The guard is `test_yaml_is_not_imported_at_module_scope`, which fails if `skills.py` binds a `yaml` attribute at import time. See pitfall 3. | +| `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` 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. | + +--- + ## Common Pitfalls ### 1. Calling `get_client()` before `init_client()` resolves @@ -240,6 +481,31 @@ When `enabled` is `False`, `config` is always `None`. When `enabled` is `True` b `execute_and_track` expects the handler to return a plain `dict` with at least `output` and `usage` keys. Do not return a custom class — `parse_usage` and the telemetry pipeline both access dict keys. +### 3. Importing `yaml` at module scope anywhere + +`pyyaml` is a **dev-only** dependency. A module-level `import yaml` makes it a de-facto +runtime dependency and breaks every install that does not happen to have it. It belongs +inside `frontmatter.parse_block()`, which is reached only from `Skill.frontmatter()` and +returns `None` when the library is absent. `types.py` must stay free of it in particular: +it is the shared declarative type surface every handler package imports, which is why the +parser lives in `frontmatter.py` and `Skill.frontmatter()` imports it lazily. + +### 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 @@ -255,3 +521,8 @@ When `enabled` is `False`, `config` is always `None`. When `enabled` is `True` b - Handler packages must import `LDContext` from `launchdarkly-ai-server` — not directly from any LD SDK. - Do not weaken the `parse_ai_config` validation — handler packages rely on `config` being valid when they receive it. - `parse_usage` must continue to accept `input_tokens/output_tokens`, `inputTokens/outputTokens`, and `input/output` as all existing handlers return one of these variants. +- Do not add `pyyaml` (or any YAML library) to this package's runtime dependencies. It is dev-only and must stay imported lazily inside `Skill.frontmatter()`. +- 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 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. diff --git a/packages/client/src/launchdarkly_ai_server/__init__.py b/packages/client/src/launchdarkly_ai_server/__init__.py index 80b959b..00ab6fd 100644 --- a/packages/client/src/launchdarkly_ai_server/__init__.py +++ b/packages/client/src/launchdarkly_ai_server/__init__.py @@ -37,6 +37,24 @@ resolve_handlers, resolve_tools, ) +from .skills import ( + InMemorySkillStore, + all_skills, + get_skill, + get_skills, + skill_refs, +) +from .skills_core import ( + SKILL_OBJECT_KIND, + 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, @@ -66,6 +84,11 @@ ProviderGraphResponse, ProviderHandler, ProviderResponse, + ReconcileAction, + ReconcileActionKind, + ReconcileReport, + Skill, + SkillReference, StreamChunkEvent, StreamDoneEvent, StreamEvent, @@ -126,6 +149,11 @@ "ProviderGraphResponse", "ProviderHandler", "ProviderResponse", + "ReconcileAction", + "ReconcileActionKind", + "ReconcileReport", + "Skill", + "SkillReference", "StreamChunkEvent", "StreamDoneEvent", "StreamEvent", @@ -199,4 +227,20 @@ "graph", "resolve_graph", "GraphInstance", + # skills + "skill_refs", + "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-the-wire and on-disk constants, identical across languages + "SKILL_OBJECT_KIND", + "SKILL_FILENAME", + "MANIFEST_FILENAME", + "MANIFEST_VERSION", ] diff --git a/packages/client/src/launchdarkly_ai_server/frontmatter.py b/packages/client/src/launchdarkly_ai_server/frontmatter.py new file mode 100644 index 0000000..41fa8e8 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/frontmatter.py @@ -0,0 +1,121 @@ +""" +``SKILL.md`` frontmatter parsing. + +A lazy convenience for ``Skill.frontmatter()``, and deliberately nothing more: +this is **never** part of the integrity path. It lives in its own module rather than in ``types.py`` because +``types.py`` is the package's shared declarative type surface — imported by every +handler package — and a YAML loading strategy has no business riding along with +``LDContext``. + +Parsing is bounded on every axis a hostile document could exploit: the block is +at most 8 KB, nesting at most 10 levels deep, alias/anchor resolution is disabled +outright, and only a safe loader is used so no object can be constructed. Every +failure degrades to ``None``; nothing here raises. +""" + +from __future__ import annotations + +from typing import Any + +_FRONTMATTER_MAX_BYTES = 8 * 1024 +"""Upper bound on the leading frontmatter block handed to the YAML parser.""" + +_FRONTMATTER_MAX_DEPTH = 10 +"""Upper bound on frontmatter nesting depth.""" + + +def extract_block(content: str) -> str | None: + """ + Returns the body of the leading ``---`` block, or ``None``. + + Delimiters are anchored at column 0 and compared with ``rstrip()``, not + ``strip()``. Every convention this format follows (Jekyll, gray-matter, + python-frontmatter, the agentskills.io ``SKILL.md`` layout) anchors them + that way, and YAML itself only recognises a document marker at column 0. + Stripping the *left* side would let an indented ``---`` or ``...`` — which + is ordinary text inside a block scalar — terminate the block early and + return a truncated mapping the caller could not distinguish from the real + one. ``rstrip()`` still tolerates a trailing ``\r`` (CRLF files) and + trailing spaces. + + Scanned newline by newline rather than over ``rest.split("\\n")``: splitting + materializes every line of a document that may be 64 KB to find a delimiter + that can only matter in the first 8 KB. The offset bail below is what makes + that bound explicit — past it, any block found would be rejected as oversize + anyway, so a document with no delimiter at all stops costing more to reject + the larger it gets. + """ + first_newline = content.find("\n") + if first_newline == -1 or content[:first_newline].rstrip() != "---": + return None + + rest = content[first_newline + 1 :] + offset = 0 + while True: + newline = rest.find("\n", offset) + end = len(rest) if newline == -1 else newline + if rest[offset:end].rstrip() in ("---", "..."): + block = rest[:offset] + try: + oversize = len(block.encode("utf-8")) > _FRONTMATTER_MAX_BYTES + except UnicodeEncodeError: + # Unpaired surrogates: not authentic content, and not something + # this convenience accessor may raise over. + return None + return None if oversize else block + if newline == -1: + return None # unterminated block + offset = newline + 1 + # A block ending at this offset is `offset` characters long, and UTF-8 is + # never shorter than one byte per character, so every delimiter from here + # on yields a block the size check would reject. + if offset > _FRONTMATTER_MAX_BYTES: + return None + + +def parse_block(block: str) -> dict[str, Any] | None: + """ + Safe, bounded YAML parse of an already-size-checked frontmatter block. + + Everything — the import, the loader subclass, and the parse — sits inside + one guard. ``import yaml`` succeeding does not prove PyYAML is what was + imported: a shadowing ``yaml.py``, a partial install, or an unrelated module + of the same name would make ``yaml.SafeLoader`` an ``AttributeError`` at + class-creation time. This accessor is documented to return ``None`` rather + than raise, so every one of those degrades to ``None``. + """ + try: + # Imported here, not at module scope: pyyaml is a development-only + # dependency and must never become a runtime one. + import yaml # type: ignore[import-untyped] + + class _BoundedSafeLoader(yaml.SafeLoader): # type: ignore[misc] + """ + ``SafeLoader`` that refuses aliases and bounds nesting depth. + + Aliases are disabled outright rather than counted: PyYAML resolves + them as shared references, so the classic billion-laughs document + parses in about a millisecond and no size or depth bound would + reject it. Making the presence of an alias itself disqualifying is + also what keeps the Python and TypeScript implementations in + agreement on the same input. + """ + + _depth = 0 + + def compose_node(self, parent: Any, index: Any) -> Any: + if self.check_event(yaml.AliasEvent): + raise yaml.YAMLError("alias nodes are not permitted in frontmatter") + self._depth += 1 + try: + if self._depth > _FRONTMATTER_MAX_DEPTH: + raise yaml.YAMLError("frontmatter nesting is too deep") + return super().compose_node(parent, index) + finally: + self._depth -= 1 + + parsed = yaml.load(block, Loader=_BoundedSafeLoader) + except Exception: + return None + + return parsed if isinstance(parsed, dict) else None diff --git a/packages/client/src/launchdarkly_ai_server/lifecycle.py b/packages/client/src/launchdarkly_ai_server/lifecycle.py index 8969f2e..28b7e81 100644 --- a/packages/client/src/launchdarkly_ai_server/lifecycle.py +++ b/packages/client/src/launchdarkly_ai_server/lifecycle.py @@ -6,6 +6,7 @@ import os from typing import Any +from . import skills from .types import InitClientOptions logger = logging.getLogger(__name__) @@ -130,13 +131,43 @@ async def init_client( - Pass *client* directly (BYOC) to skip the LaunchDarkly Python SDK path. - Otherwise, reads ``LD_SDK_KEY`` from env or ``options['sdkKey']``. + - ``options['skillStore']`` configures the store the Agent Skills accessors + read from. Absent by default, in which case they raise an actionable error. + + This function is idempotent for the client singleton: a second call returns + the existing client without re-initializing, and every option is ignored — + **except** ``skillStore``, which is applied on every successful call. That + asymmetry is deliberate, and it is what lets a client that was lazily + auto-initialized, or initialized without a store, be given one afterwards. + A ``skillStore`` of ``None`` (or absent) never clears an already-configured + store; use ``shutdown()`` for that. The store is installed only once + initialization has succeeded: a call that raises leaves no global state + behind, so a failed init cannot leave the skill accessors working against a + store the application believes was never installed. Returns the initialized ``LDClientInterface`` instance. """ - global _client - opts = options or {} + ld_client = await _resolve_client(opts, client) + + # The single success point: every path that raises returns before here, so + # "installed only on success" is one statement rather than a copy per exit. + skill_store = opts.get("skillStore") + if skill_store is not None: + skills._set_store(skill_store) + return ld_client + + +async def _resolve_client(opts: InitClientOptions, client: Any) -> Any: + """ + Returns the singleton client, initializing it on first call. + + Split from ``init_client`` so that function has exactly one success point to + hang the ``skillStore`` carve-out on. + """ + global _client + # Idempotent — if already initialized, return the existing client if _client is not None: return _client @@ -198,12 +229,18 @@ async def shutdown() -> None: """ Shuts down the singleton client. Idempotent — safe to call multiple times even if the client was never initialized or already shut down. + + Also clears the configured skill store (and telemetry emitter): after a + shutdown, re-pass ``skillStore`` to the next ``init_client`` if the skill + accessors should keep working. """ global _client, _tracer_provider local_client = _client local_provider = _tracer_provider + skills._clear_state() + # Null the singleton before any awaits so a second call is a no-op _client = None _tracer_provider = None @@ -240,6 +277,7 @@ def _reset_for_testing() -> None: global _client, _tracer_provider _client = None _tracer_provider = None + skills._clear_state() async def inspect_config( diff --git a/packages/client/src/launchdarkly_ai_server/safe_fs.py b/packages/client/src/launchdarkly_ai_server/safe_fs.py new file mode 100644 index 0000000..3605399 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/safe_fs.py @@ -0,0 +1,311 @@ +""" +Descriptor-pinned filesystem primitives. + +Split out because none of this knows what a skill is: it is the "write a file +under a directory an attacker may be racing you for" problem, solved once. +``skills_fs.py`` is the only caller today. + +The whole point is that a path check is only as good as the last path +resolution after it. Every operation here therefore runs relative to a +descriptor pinned to a directory the caller has already validated, rather than +re-resolving a name — which is what closes the swap window rather than merely +narrowing it. Where the platform has no ``*at()`` syscall family (Windows) the +identical sequence runs against full paths, the per-component ``lstat`` floor. +""" + +from __future__ import annotations + +import errno +import os +import secrets +import stat +import tempfile +from collections.abc import Iterator +from contextlib import contextmanager +from pathlib import Path + +_FILE_MODE = 0o644 +"""Mode set explicitly on every written file — never inherited from the umask, +and never executable.""" + +SUPPORTS_DIR_FD = os.supports_dir_fd.issuperset( + # renameat, openat, unlinkat, fstatat — the four this module needs. + {os.rename, os.open, os.unlink, os.stat} +) +""" +Whether the ``*at()`` syscall family is available, so every operation under the +managed root can be performed relative to a descriptor pinned to a directory +this module has already verified rather than re-resolved from its path. + +That is what closes the swap window rather than merely narrowing it: +a descriptor refers to the inode that was checked, so replacing ``/`` +with a symlink after the check cannot redirect a write or an unlink out of the +root. POSIX has these calls; Windows does not, and there the per-component +``lstat`` floor the spec permits applies instead. + +The probe deliberately names ``os.rename`` and ``os.stat`` rather than the +``os.replace`` and ``os.lstat`` this module actually calls. ``os.supports_dir_fd`` +is populated per underlying syscall, and CPython registers ``renameat`` under +``rename`` only and ``fstatat`` under ``stat`` only — even though ``os.replace`` +is the same ``renameat``-backed function and ``os.lstat`` is ``fstatat`` with +``AT_SYMLINK_NOFOLLOW``, and both accept the descriptor keywords wherever their +advertised twin does (verified on CPython 3.12 and 3.13, macOS). Probing the +names this module calls would report "unsupported" on every POSIX platform and +silently disable the defense. +""" + + +def open_directory_nofollow(directory: Path) -> int | None: + """ + Opens *directory* without following a final symlink, and pins it. + + Everything the caller does afterwards goes through the returned descriptor + instead of the path, which is what turns the "narrow window" into no + window at all: the descriptor names the inode that was checked, so swapping + the path for a symlink between the check and the write cannot redirect the + write out of the managed root. + + On a platform without the ``*at()`` family (Windows) this returns ``None`` + after verifying via ``lstat`` that the path is a real, non-symlink + directory — the per-component floor. It must not attempt the descriptor + open there: ``os.open`` goes through the CRT on Windows, which cannot open + a directory at all, so the descriptor path would fail every operation + rather than fall back. + + Raises ``ValueError`` when the path will not open (or inspect) as a real + directory — the caller reports that as a refusal rather than letting it + escape. + """ + if not SUPPORTS_DIR_FD: + try: + mode = os.lstat(directory).st_mode + except OSError as exc: + raise ValueError(f"the directory could not be inspected: {exc}") from exc + if stat.S_ISLNK(mode): + raise ValueError("the directory is a symlink") + if not stat.S_ISDIR(mode): + raise ValueError("the path is not a directory") + return None + + flags = os.O_RDONLY | getattr(os, "O_DIRECTORY", 0) | getattr(os, "O_NOFOLLOW", 0) + try: + fd = os.open(directory, flags) + except OSError as exc: + raise ValueError( + f"the directory could not be opened without following links: {exc}" + ) from exc + try: + # O_DIRECTORY already guarantees this wherever the platform defines it; + # the explicit check is what covers the platforms that do not. + if not stat.S_ISDIR(os.fstat(fd).st_mode): + raise ValueError("the path is not a directory") + except BaseException: + os.close(fd) + raise + return fd + + +def open_or_create_directory(directory: Path) -> int | None: + """ + Creates *directory* if absent and returns a descriptor pinned to it. + + ``Path.mkdir(exist_ok=True)`` treats an existing symlink-to-directory as + "already there", which would re-open the very hole the caller's ``lstat`` + check just closed. ``os.mkdir`` plus an ``lstat`` on the ``FileExistsError`` + path does not: a link reports as a link, and is refused. + """ + try: + os.mkdir(directory, 0o755) + except FileExistsError: + mode = os.lstat(directory).st_mode + if stat.S_ISLNK(mode): + raise ValueError("the directory is a symlink") from None + if not stat.S_ISDIR(mode): + raise ValueError("the path is not a directory") from None + return open_directory_nofollow(directory) + + +@contextmanager +def pinned_directory(directory: Path, *, create: bool = False) -> Iterator[int | None]: + """ + Holds *directory* pinned for the duration of the block, then releases it. + + Yields what the two openers above return — a descriptor, or ``None`` on the + ``lstat`` floor — so the caller states the platform split once, as + ``if dir_fd is not None``, and cannot forget the ``os.close``. Raises + ``ValueError`` for a directory that will not pin, exactly as they do. + """ + dir_fd = ( + open_or_create_directory(directory) + if create + else open_directory_nofollow(directory) + ) + try: + yield dir_fd + finally: + if dir_fd is not None: + os.close(dir_fd) + + +class SymlinkRefused(OSError): + """ + Raised instead of removing a symlink found where a real file was expected. + + An ``OSError`` subclass so a caller that only cares that the removal failed + keeps its single ``except``; a distinct type so one that must report *this* + refusal specifically does not have to match on a message. + """ + + +def unlink_file(directory: Path, name: str, *, dir_fd: int | None) -> None: + """ + Removes ``/``, refusing to follow a symlink at *name*. + + The mirror of ``atomic_write``, and descriptor-relative for the same reason: + ``unlink`` never follows a *trailing* symlink, but it does resolve the + directory above it, so a ```` swapped for a symlink after the + caller's checks would otherwise turn this into a delete of an + attacker-chosen file. Given a *dir_fd* the probe and the unlink both run + against it; without one the identical sequence runs against full paths. + + Raises ``SymlinkRefused`` when *name* is a symlink. Note that this refuses + rather than removes: ``unlink`` would happily delete the link itself, but a + link where this SDK expects its own file means the state on disk is not what + the manifest describes, and that is the caller's to report rather than to + tidy away. + """ + if dir_fd is None: + # No ``*at()`` family: the trailing-symlink check and the unlink are both + # path-based, the per-component floor. + target = directory / name + if target.is_symlink(): + raise SymlinkRefused(f"{name} is a symlink") + target.unlink() + return + + # os.stat(follow_symlinks=False), not os.lstat: identical result, and it is + # the spelling os.supports_dir_fd actually advertises. + probe = os.stat(name, dir_fd=dir_fd, follow_symlinks=False) + if stat.S_ISLNK(probe.st_mode): + raise SymlinkRefused(f"{name} is a symlink") + os.unlink(name, dir_fd=dir_fd) + + +def _mkstemp_at(dir_fd: int, prefix: str) -> tuple[int, str]: + """ + ``tempfile.mkstemp`` for a directory descriptor. + + ``tempfile`` has no ``dir_fd`` form, so this reproduces the part that + matters: ``O_CREAT | O_EXCL`` against an unpredictable name, retried on + collision, so an existing temp path is never reused and a planted one is + never written through. + """ + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_NOFOLLOW", 0) + for _ in range(tempfile.TMP_MAX): + name = f"{prefix}{secrets.token_hex(8)}.tmp" + try: + return os.open(name, flags, 0o600, dir_fd=dir_fd), name + except FileExistsError: + continue + raise OSError(errno.EEXIST, "no usable temporary file name was found") + + +def atomic_write( + directory: Path, name: str, data: bytes, *, dir_fd: int | None = None +) -> None: + """ + Writes *data* to ``/`` so no partial file is ever + observable. + + The temp file is created exclusively in the target's *own* directory — one + anywhere else would make the rename cross-device, and therefore not atomic — + written, fsynced, renamed over the target, and the directory fsynced so the + rename itself survives a crash. Mode is set explicitly rather than left to + the process umask, and the execute bit is never set. + + Given a *dir_fd* on a platform with the ``*at()`` family, every one of those + steps runs relative to that descriptor and both names are bare filenames. + Without one (Windows) the identical sequence runs against full paths, which + is the per-component ``lstat`` floor. + + ``os.replace`` is the one and only rename call site, reached by attribute + lookup on the ``os`` module so tests can intercept it; ``os.rename`` must + not be substituted for it (it is also the only one with defined overwrite + semantics on Windows). + """ + at_fd = dir_fd if dir_fd is not None and SUPPORTS_DIR_FD else None + prefix = f".{name}." + target: str | Path + + if at_fd is not None: + fd, temp = _mkstemp_at(at_fd, prefix) + target = name + else: + # mkstemp opens with O_CREAT|O_EXCL, so an existing temp path is never + # reused. + fd, temp = tempfile.mkstemp(dir=directory, prefix=prefix, suffix=".tmp") + target = directory / name + + try: + try: + # fchmod, not chmod: operating on the descriptor cannot be redirected + # by anything that swaps the temp path underneath us, and it makes the + # mode independent of the process umask (both creation paths open 0600). + os.fchmod(fd, _FILE_MODE) + view = memoryview(data) + while view: + view = view[os.write(fd, view) :] + os.fsync(fd) + finally: + os.close(fd) + if at_fd is not None: + os.replace(temp, target, src_dir_fd=at_fd, dst_dir_fd=at_fd) + else: + os.replace(temp, target) + except BaseException: + try: + if at_fd is not None: + os.unlink(temp, dir_fd=at_fd) + else: + os.unlink(temp) + except OSError: + pass + raise + + if at_fd is not None: + _fsync_directory_fd(at_fd) + else: + _fsync_directory(directory) + + +def atomic_write_in(directory: Path, name: str, data: bytes) -> None: + """ + ``atomic_write`` against a directory this module does not already hold open. + + Used for the skills manifest, whose directory is the managed root. The + descriptor is taken with ``O_NOFOLLOW``, so a root swapped for a symlink after + ``_resolve_root`` validated it fails the write instead of redirecting it — + the caller turns that into a run-level ``error`` action. + """ + with pinned_directory(directory) as dir_fd: + atomic_write(directory, name, data, dir_fd=dir_fd) + + +def _fsync_directory_fd(fd: int) -> None: + """Best effort — not every platform allows fsync on a directory descriptor.""" + try: + os.fsync(fd) + except OSError: + pass + + +def _fsync_directory(directory: Path) -> None: + """Best effort — not every platform lets a directory be opened for fsync.""" + try: + fd = os.open(directory, os.O_RDONLY) + except OSError: + return + try: + _fsync_directory_fd(fd) + finally: + os.close(fd) diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py new file mode 100644 index 0000000..b4366ea --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -0,0 +1,239 @@ +""" +Agent Skills — reference discovery and content accessors. + +The public retrieval surface: projecting the skill references a resolved AI +Config carries, and retrieving skill content through an injectable store seam. + +The three layers of the feature sit in three modules, and the dependencies run +one way only: + +- ``skills_core.py`` — the store and telemetry seams, module state, integrity + verification, and store resolution. Shared, and imports neither of the others. +- ``skills.py`` (this file) — ``skill_refs``, the accessors, and + ``InMemorySkillStore``. +- ``skills_fs.py`` — the highest-blast-radius layer, the one that writes to a + customer's disk. It owns the manifest format and the on-disk filenames; + nothing here knows about the filesystem. + +``_set_store``, ``_set_emitter_for_testing`` and ``_clear_state`` live here +because this module is the documented injection path; the +state they mutate lives in ``skills_core``. +""" + +from __future__ import annotations + +import logging +from collections.abc import Callable, Sequence +from typing import Any + +from . import skills_core +from .skills_core import ( + SKILL_OBJECT_KIND, + list_raw_objects, + reference_target, + require_store, + resolve_from_store, + verify_raw_skill, +) +from .types import AiConfigRep, Skill, SkillReference +from .types_validation import ( + is_valid_skill_key, + is_valid_skill_version, + skill_key_rejection_reason, +) + +logger = logging.getLogger(__name__) + +# --------------------------------------------------------------------------- +# Injection points +# --------------------------------------------------------------------------- +# +# These three names are the documented seam: ``init_client`` and ``shutdown`` +# call them, and tests inject through them. They delegate to ``skills_core``, +# which owns the state, so that there is exactly one store and one emitter no +# matter which layer reaches for them. + + +# Bound directly to the implementations rather than wrapped: a one-line +# delegation per name would give every state mutation two definitions and two +# docstrings to keep in agreement, which is the drift these names exist to +# avoid. ``_set_emitter_for_testing`` keeps its distinct name because it has no +# production caller. +_set_store = skills_core.set_store +_set_emitter_for_testing = skills_core.set_emitter +_clear_state = skills_core.clear_state + + +class InMemorySkillStore: + """ + A skill store backed by a plain dict. + + Ships for local development, tests, and bring-your-own-content injection. + Holds raw wire objects verbatim and performs no validation of its own — + verification belongs at the accessor boundary, where it applies to every + store equally. + """ + + def __init__(self, objects: dict[str, dict[str, Any]] | None = None) -> None: + self._objects: dict[str, dict[str, Any]] = dict(objects or {}) + self._listeners: dict[str, list[Callable[[dict[str, Any]], Any]]] = {} + + def put(self, raw: dict[str, Any]) -> None: + """ + Adds or replaces a raw skill object, keyed by its own ``key`` field. + + Notifies every skill-kind listener with the raw object as a single + positional argument. No validation happens here — verification belongs at + the accessor boundary, where it applies to every store equally — so a + listener sees exactly what was put, unverified. + """ + key = raw.get("key") + if not isinstance(key, str): + raise ValueError("a raw skill object must carry a string 'key'") + self._objects[key] = raw + for listener in self._listeners.get(SKILL_OBJECT_KIND, []): + listener(raw) + + def get_object(self, kind: str, key: str) -> dict[str, Any] | None: + if kind != SKILL_OBJECT_KIND: + return None + return self._objects.get(key) + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + if kind != SKILL_OBJECT_KIND: + return {} + return dict(self._objects) + + def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: + """ + Registers *fn* to be called with each raw object ``put`` under *kind*. + + Only ``kind == SKILL_OBJECT_KIND`` is ever notified, because ``put`` only + accepts skill objects; a listener registered under any other kind is + recorded and never fires. + """ + self._listeners.setdefault(kind, []).append(fn) + + +# --------------------------------------------------------------------------- +# Reference discovery +# --------------------------------------------------------------------------- + + +def skill_refs(config: AiConfigRep | None) -> list[SkillReference]: + """ + Projects a resolved AI Config's ``skills`` array into typed references. + + A pure projection — no network, no client, no store, no telemetry. Returns + ``[]`` when the config carries no skills. Compose it with the accessors for + per-context resolution: ``await get_skills(skill_refs(config))``. + + A config that came through ``parse_ai_config`` never contains an invalid + entry — parsing fails closed on one. A hand-built dict can, and a silently + 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 [] + + raw = config.get("skills") + if not isinstance(raw, list): + 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 predicate (not the reason string) so the type + # checker narrows ``key`` to ``str`` for the reference below. + 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 + + +# --------------------------------------------------------------------------- +# Content accessors +# --------------------------------------------------------------------------- + + +async def get_skill(key: str, *, version: int | None = None) -> Skill | None: + """ + Retrieves one verified skill by key. + + ``version=None`` means the newest version the store holds; a specific + ``version`` returns the skill only when that exact version is available. + Returns ``None`` — never raises — when the skill is missing, the requested + version is not the one held, or verification fails. Raises ``RuntimeError`` + only when no skill store is configured. + + There is no context parameter: skills have no targeting, so the SDK + credentials fully determine availability. Compose per-context resolution + explicitly with ``get_skills(skill_refs(config))``. + """ + return resolve_from_store(require_store(), key, version).skill + + +async def get_skills(refs: Sequence[SkillReference | str]) -> list[Skill]: + """ + Retrieves a batch of verified skills. + + Accepts a mixed sequence of ``SkillReference`` values and bare key strings, + where a string means "the latest version". Results follow input order for + the skills that were found; entries that are missing, are the wrong version, + or fail verification are omitted rather than returned as placeholders. + """ + if isinstance(refs, str): + # str satisfies Sequence[str], so this type-checks; iterating it would + # silently look up one skill per character. + raise TypeError( + "get_skills takes a sequence of references; pass [key] rather than a " + f"bare string. Got {refs!r}." + ) + + store = require_store() + + skills: list[Skill] = [] + for ref in refs: + key, wanted = reference_target(ref) + skill = resolve_from_store(store, key, wanted).skill + if skill is not None: + skills.append(skill) + return skills + + +async def all_skills() -> list[Skill]: + """ + Retrieves every verified skill the store currently holds. + + Skills that fail verification are omitted. Raises ``RuntimeError`` only when + no skill store is configured. + """ + objects, error = list_raw_objects(require_store()) + if error is not None: + return [] + + skills: list[Skill] = [] + for raw in objects.values(): + skill = verify_raw_skill(raw) + if skill is not None: + skills.append(skill) + return skills diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py new file mode 100644 index 0000000..df0ba8c --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -0,0 +1,509 @@ +""" +Agent Skills — the internals ``skills`` and ``skills_fs`` both need. + +Extracted so the two layers above it share one implementation through an +explicit surface instead of reaching into each other's privates. Everything +here is package-internal — nothing in this module is exported from +``launchdarkly_ai_server`` except the two constants that are public API — and +the dependency runs one way: this module imports neither ``skills`` nor +``skills_fs``. + +What lives here, and why it has to be one copy: + +- **The store seam and the configured store.** One place holds the store, so + the accessors and the materialization path cannot disagree about whether one + is configured. +- **The telemetry seam.** Every signal the feature can emit is constructed by a + ``record_*`` function in this file and nowhere else, which is what makes the + three-signal allowlist enforceable by reading one section. ``emit`` is never + called from outside this module. +- **Integrity verification.** ``verified_bytes`` runs twice per skill by design + — once at the accessor boundary, and again immediately before a write, since a + ``Skill`` can also be constructed directly by a caller. Sharing the + implementation is what keeps the two passes from drifting — the signal's + property keys must match whichever layer caught the defect. +- **Store resolution.** ``resolve_from_store`` is the fetch-and-verify sequence + the accessors and the reconcile share, so its call sites cannot drift apart — + in particular on how a raising store is handled. + +Everything the store hands back is untrusted input; the transport is not part of +the trust boundary. Key, version, size, and content hash are revalidated here on +every pass. + +The store and emitter are injected through ``skills._set_store`` and +``skills._set_emitter_for_testing`` — those names are the documented seam, +and they delegate here. +""" + +from __future__ import annotations + +import hashlib +import logging +import re +from dataclasses import dataclass +from typing import Any, Protocol + +from .types import Skill, SkillReference +from .types_validation import is_valid_skill_key, is_valid_skill_version + +logger = logging.getLogger(__name__) + +SKILL_OBJECT_KIND = "skill" +"""The object kind under which skill content is delivered.""" + +MAX_SKILL_CONTENT_BYTES = 64 * 1024 +""" +Hard cap on skill content. Legitimately delivered skills are well under this +bound, so anything larger is withheld regardless of whether its hash checks out. + +Deliberately **not** exported from the package root, unlike the on-disk and +on-the-wire constants beside it. Those are values this SDK defines and a caller +may need to agree with; this one is a local enforcement bound on content the +platform produces, so publishing it would semver-lock a number this side does +not own — and a caller pre-flighting "will my skill fit?" against it would be +reading the client's guess rather than the real limit. The reason string from +``verified_bytes`` already reports the bound when it is what withheld content. +""" + +_LANGUAGE = "python" + +_SHA256_HEX = re.compile(r"\A[0-9a-f]{64}\Z") +"""What a legitimate content hash looks like. Anything else is redacted before +it reaches telemetry — ``contentHash`` is attacker-controlled, and a store that +put the skill body there would otherwise leak it into a signal.""" + +_SIGNAL_INTEGRITY_FAILURE = "AgentControl Skill Integrity Failure" +_SIGNAL_MATERIALIZED = "AgentControl Skill Materialized" +_SIGNAL_REVOKED = "AgentControl Skill Revoked Received" + +NO_STORE_MESSAGE = ( + "No skill store is configured, so skill content cannot be retrieved. Configure " + 'one with init_client(options={"skillStore": store}) — InMemorySkillStore is ' + "available for local development and testing." +) + + +# --------------------------------------------------------------------------- +# The store seam +# --------------------------------------------------------------------------- + + +class SkillStore(Protocol): + """ + Structural interface every source of skill content satisfies. + + Duck-typed on purpose, mirroring how the LaunchDarkly client interface works + in this package: pass any object carrying these methods. + + ``add_listener(kind, fn)`` is part of the seam but + **optional**, which is why it is deliberately not declared here: a Protocol + member is required for structural compatibility, so declaring it would reject + every store that does not implement it. Nothing in this module calls it — it + exists for the delivery transport to push updates through. + + The raw objects a store serves are wire-shaped, with camelCase field names + identical across language implementations:: + + {"key": "pdf-extraction", "version": 2, "content": "---\\n...", + "contentHash": "9f3a...", "name": "PDF Extraction", "description": "..."} + """ + + def get_object(self, kind: str, key: str) -> dict[str, Any] | None: ... + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: ... + + +# --------------------------------------------------------------------------- +# Telemetry seam +# --------------------------------------------------------------------------- + + +class _TelemetryEmitter(Protocol): + def record(self, signal: str, properties: dict[str, Any]) -> None: ... + + +class _NoOpEmitter: + """ + The default emitter. + + No skills telemetry leaves the process in this release: ``client.track()`` is + the wrong channel (it needs an LD context, spends the customer's event + volume, and lands in their data export), and the diagnostic-event channel + that would be right has no wrapper-SDK extension point yet. Signals are + recorded through this seam so the eventual transport drops in behind it + without touching a single call site. + """ + + def record(self, signal: str, properties: dict[str, Any]) -> None: + return None + + +_NOOP_EMITTER: _TelemetryEmitter = _NoOpEmitter() + +# --------------------------------------------------------------------------- +# Module state +# --------------------------------------------------------------------------- + +_store: SkillStore | None = None +_emitter: _TelemetryEmitter = _NOOP_EMITTER +"""Never ``None``: "no emitter installed" is spelled as the no-op, so ``emit`` +has one code path instead of re-deciding on every signal.""" + + +def set_store(store: Any) -> None: + """ + Replaces the configured store. + + Reached through ``skills._set_store``, which is the documented seam; see that + function for who calls it and why it has no test-only twin. + """ + global _store + _store = store + + +def set_emitter(emitter: Any) -> None: + """Replaces the telemetry emitter. Reached through + ``skills._set_emitter_for_testing``.""" + global _emitter + _emitter = emitter + + +def clear_state() -> None: + """Drops both the store and the emitter. Reached through ``skills._clear_state``.""" + global _store, _emitter + _store = None + _emitter = _NOOP_EMITTER + + +def get_store() -> SkillStore | None: + """The configured store, or ``None``. The only reader of the global.""" + return _store + + +def require_store() -> SkillStore: + store = get_store() + if store is None: + raise RuntimeError(NO_STORE_MESSAGE) + return store + + +def emit(signal: str, properties: dict[str, Any]) -> None: + """ + Records one signal. Never raises into the calling operation — a broken + emitter must not be able to fail a retrieval or a reconcile. + """ + try: + _emitter.record(signal, properties) + except Exception: + logger.warning("Skills telemetry emitter raised; ignoring", exc_info=True) + + +def record_integrity_failure( + skill_key: str, + reason: str, + *, + version: Any = None, + expected_hash: Any = None, + observed_hash: str | None = None, +) -> None: + """ + Records an integrity failure. Carries hashes and byte counts only — the skill + body never appears in a signal, a log line, or an error message. + """ + # Both of these come off the wire, so neither may be echoed verbatim: a store + # that set contentHash (or key) to the skill body would otherwise publish the + # body itself. Shape-check, then redact. + safe_key = skill_key if is_valid_skill_key(skill_key) else "" + properties: dict[str, Any] = {"skill_key": safe_key, "language": _LANGUAGE} + if is_valid_skill_version(version): + properties["version"] = version + if isinstance(expected_hash, str): + properties["expected_hash"] = ( + expected_hash + if _SHA256_HEX.match(expected_hash) + else "" + ) + if observed_hash is not None: + properties["observed_hash"] = observed_hash + + logger.error("Skill '%s' failed integrity verification: %s", safe_key, reason) + emit(_SIGNAL_INTEGRITY_FAILURE, properties) + + +def record_materialized( + skill_key: str, content_bytes: int, content_hash: str, reconcile_action: str +) -> None: + """ + Records a materialization. Deliberately carries no ``target_path`` and no + filesystem path of any kind — the same reasoning that keeps the skill body + out of telemetry keeps the customer's directory layout out. Paths live in the + returned ``ReconcileReport``, which is user-facing API rather than telemetry. + """ + emit( + _SIGNAL_MATERIALIZED, + { + "skill_key": skill_key, + "content_bytes": content_bytes, + "content_hash": content_hash, + "reconcile_action": reconcile_action, + "language": _LANGUAGE, + }, + ) + + +def record_revoked(skill_key: str, version: Any) -> None: + """ + Records a revocation — a prune that removed a formerly managed skill. + + Lives here with the other two recorders rather than at the prune site so the + signal allowlist is maintained in one place: every signal this SDK can emit + is visible in this section of this module, and nothing outside it touches + ``emit``. + """ + # Both fields come off the manifest, which is untrusted — same rule as + # ``record_integrity_failure``: shape-check, then redact, so a hand-edited + # manifest cannot plant an arbitrary string in a signal. + safe_key = skill_key if is_valid_skill_key(skill_key) else "" + properties: dict[str, Any] = { + "skill_key": safe_key, + "removed_from_disk": True, + "language": _LANGUAGE, + } + if is_valid_skill_version(version): + properties["version"] = version + emit(_SIGNAL_REVOKED, properties) + + +# --------------------------------------------------------------------------- +# Integrity verification +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class VerifiedContent: + """Content that passed integrity verification.""" + + encoded: bytes + """The verbatim UTF-8 bytes, exactly as hashed.""" + content_hash: str + """The locally computed sha256 — never the caller's expected value.""" + + +@dataclass(frozen=True) +class VerificationFailure: + """Why content did not pass. The reason is safe to show a caller.""" + + reason: str + + +def verified_bytes( + key: str, content: str, expected_hash: str, version: int +) -> VerifiedContent | VerificationFailure: + """ + The whole content half of integrity verification: encode, size, hash. + + Returns the verbatim bytes and their locally computed sha256, or a + human-readable reason — having already recorded the integrity signal, so the + signal's property set cannot depend on which caller noticed. The hash handed + back is the one computed here, never the caller's expected value: the two are + equal on this path by construction, and returning the locally derived one + keeps an attacker-supplied string out of ``Skill``. + + The two outcomes are distinct types rather than a ``tuple | str`` union so a + call site reads as "verification failed" instead of "the result is a string", + and so a future success payload carrying a ``str`` cannot silently invert the + discrimination. + + This runs twice per skill by design: once at the accessor boundary, and again + immediately before a write, because a ``Skill`` can also be constructed + directly by a caller. Sharing the implementation is what keeps those two + passes from drifting — the property keys must match. + + The second pass re-encodes and re-hashes content the first pass already + hashed. That redundancy is deliberate: it is negligible next to the write + it guards, and carrying the first pass's bytes forward would put a "trust + the value computed upstream" branch inside the one function whose entire + job is not to. + """ + try: + encoded = content.encode("utf-8") + except UnicodeEncodeError: + # json.loads turns a "\ud800" escape into an unpaired surrogate, which has + # no UTF-8 encoding. There are no bytes the server could have hashed, so + # this is not authentic content. Never use errors="surrogatepass" here: + # that would fabricate bytes and could satisfy the hash comparison. + reason = "content is not encodable as UTF-8" + record_integrity_failure( + key, reason, version=version, expected_hash=expected_hash + ) + return VerificationFailure(reason) + + if len(encoded) > MAX_SKILL_CONTENT_BYTES: + reason = ( + f"content is {len(encoded)} bytes, over the " + f"{MAX_SKILL_CONTENT_BYTES} byte cap" + ) + record_integrity_failure( + key, reason, version=version, expected_hash=expected_hash + ) + return VerificationFailure(reason) + + # sha256, lowercase hex, over the verbatim UTF-8 bytes — no canonicalization + # and no frontmatter parsing anywhere in the integrity path. + observed_hash = hashlib.sha256(encoded).hexdigest() + if observed_hash != expected_hash: + record_integrity_failure( + key, + "content hash mismatch", + version=version, + expected_hash=expected_hash, + observed_hash=observed_hash, + ) + return VerificationFailure("content hash mismatch") + + return VerifiedContent(encoded=encoded, content_hash=observed_hash) + + +def verify_raw_skill(raw: Any) -> Skill | None: + """ + Turns one untrusted raw store object into a ``Skill``, or withholds it. + + On any failure the skill is treated as missing, the integrity signal is + recorded, and an error is logged. No unverified content is ever returned to + user code. + """ + if not isinstance(raw, dict): + record_integrity_failure("", "raw skill object is not an object") + return None + + key = raw.get("key") + if not is_valid_skill_key(key): + record_integrity_failure( + key if isinstance(key, str) else "", + "key is not a valid skill key", + ) + return None + + version = raw.get("version") + if not is_valid_skill_version(version): + record_integrity_failure(key, "version is not an integer >= 1") + return None + + content = raw.get("content") + if not isinstance(content, str): + record_integrity_failure( + key, "content is missing or not a string", version=version + ) + return None + + expected_hash = raw.get("contentHash") + if not isinstance(expected_hash, str): + record_integrity_failure( + key, "contentHash is missing or not a string", version=version + ) + return None + + verified = verified_bytes(key, content, expected_hash, version) + if isinstance(verified, VerificationFailure): + return None + + name = raw.get("name") + description = raw.get("description") + return Skill( + key=key, + version=version, + content=content, + content_hash=verified.content_hash, + name=name if isinstance(name, str) else None, + description=description if isinstance(description, str) else None, + ) + + +def store_raised(exc: Exception) -> str: + """The one wording for "the store could not answer", used by every path.""" + return f"the skill store raised {type(exc).__name__}: {exc}" + + +def list_raw_objects( + store: SkillStore, +) -> tuple[dict[str, dict[str, Any]], str | None]: + """ + Every raw object the store holds, or the reason it could not answer. + + Returns the reason rather than raising, because both callers need the + distinction between "no skills" and "the store is broken" — and they need it + worded identically. Letting the exception out instead would make each of + them re-derive the log line and the message, which is the drift this module + exists to prevent. + """ + try: + objects = store.all_objects(SKILL_OBJECT_KIND) + except Exception as exc: + logger.error("Skill store raised while listing skills", exc_info=True) + return {}, store_raised(exc) + return (objects if isinstance(objects, dict) else {}), None + + +# --------------------------------------------------------------------------- +# Resolution internals — shared with the materialization path +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class Resolution: + """One key resolved against a store: the skill, or why there is none.""" + + skill: Skill | None = None + error: str | None = None + unavailable: bool = False + """ + ``True`` when the *store* could not answer — it raised — rather than when it + answered "no". Only the former suppresses pruning: deleting managed files + because a lookup failed would turn an outage into data loss. + """ + + +def resolve_from_store( + store: SkillStore, key: str, wanted_version: int | None +) -> Resolution: + """ + Fetches one key and verifies it — the sequence the accessors and the + materialization path share. + + Written once on purpose, so the call sites cannot drift apart — in + particular on the policy for a raising store. + """ + try: + raw = store.get_object(SKILL_OBJECT_KIND, key) + except Exception as exc: + logger.error("Skill store raised while retrieving '%s'", key, exc_info=True) + return Resolution(error=store_raised(exc), unavailable=True) + + if not isinstance(raw, dict): + return Resolution( + error=f"skill '{key}' is not available from the configured skill store" + ) + + skill = verify_raw_skill(raw) + if skill is None: + return Resolution( + error=f"skill '{key}' failed integrity verification and was withheld" + ) + if wanted_version is not None and skill.version != wanted_version: + return Resolution( + error=( + f"skill '{key}' version {wanted_version} is not available " + f"(the store holds version {skill.version})" + ) + ) + return Resolution(skill=skill) + + +def reference_target(item: SkillReference | str) -> tuple[str, int | None]: + """Normalises a reference-or-key into ``(key, wanted version)``. + + A bare string means "the latest version the store holds". + """ + if isinstance(item, str): + return item, None + return item.key, item.version diff --git a/packages/client/src/launchdarkly_ai_server/skills_fs.py b/packages/client/src/launchdarkly_ai_server/skills_fs.py new file mode 100644 index 0000000..1a88307 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills_fs.py @@ -0,0 +1,948 @@ +""" +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, + 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) + + return [_pending_for_raw(key, raw) for key, raw in objects.items()], 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 57f8134..2cf8713 100644 --- a/packages/client/src/launchdarkly_ai_server/types.py +++ b/packages/client/src/launchdarkly_ai_server/types.py @@ -92,7 +92,8 @@ class Message: AiConfigRep = dict[str, Any] """ Raw AI config dict as returned by ``parse_ai_config``. Fields include -``model``, ``provider``, and at least one of ``instructions`` / ``messages``. +``model``, ``provider``, at least one of ``instructions`` / ``messages``, and an +optional ``skills`` array of ``{key, version}`` references (see ``skill_refs``). """ VariationMeta = dict[str, Any] @@ -412,6 +413,116 @@ class ProviderGraphResponse: """Results from a graph-level judge, if configured.""" +# --------------------------------------------------------------------------- +# Agent Skills +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class SkillReference: + """A version-pinned pointer to a skill, as attached to an AI Config variation.""" + + key: str + """Immutable skill key — ``^[a-z0-9][a-z0-9-]*$``, at most 256 characters.""" + version: int + """Immutable skill version — an integer >= 1.""" + + +@dataclass(frozen=True) +class Skill: + """ + A single verbatim ``SKILL.md`` document. + + Only ever constructed after integrity verification passes, so ``content`` + is the exact byte sequence LaunchDarkly delivered and ``content_hash`` is + its sha256. Instances are immutable. + """ + + key: str + version: int + content: str + """Verbatim ``SKILL.md`` — YAML frontmatter plus markdown body.""" + content_hash: str + """sha256, lowercase hex, over the verbatim UTF-8 bytes of ``content``.""" + name: str | None = None + """Display name from LaunchDarkly metadata; never parsed from the markdown.""" + description: str | None = None + """Description from LaunchDarkly metadata; never parsed from the markdown.""" + + def frontmatter(self) -> dict[str, Any] | None: + """ + Parses the leading ``---`` frontmatter block, if any. + + A lazy convenience, never part of the integrity path. The YAML library + is imported inside this method so it stays a development-only + dependency. Parsing is bounded on every axis a hostile document could + exploit: the block must be at most 8 KB, nesting at most 10 levels + deep, alias/anchor resolution is disabled outright, and only a safe + loader is used so no object can be constructed. + + Returns ``None`` — never raises — when the block is absent, + unterminated, oversize, too deeply nested, not a mapping, unparseable, + or when no safe YAML parser is available. + """ + # Imported here, not at module scope: this module is the package's + # shared declarative type surface, and the parser it delegates to is + # only ever reached through this one accessor. + from .frontmatter import extract_block, parse_block + + block = extract_block(self.content) + if block is None: + return None + return parse_block(block) + + +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/src/launchdarkly_ai_server/types_validation.py b/packages/client/src/launchdarkly_ai_server/types_validation.py index acdcce4..37cd56b 100644 --- a/packages/client/src/launchdarkly_ai_server/types_validation.py +++ b/packages/client/src/launchdarkly_ai_server/types_validation.py @@ -1,16 +1,62 @@ from __future__ import annotations -from typing import Any +import re +from typing import Any, TypeGuard from .types import ParseFailure, ParseResult, ParseSuccess _VALID_ROLES = {"user", "assistant", "system"} +SKILL_KEY_GRAMMAR = "^[a-z0-9][a-z0-9-]*$" +""" +The skill key grammar, as a string, so every message that has to explain a +rejection quotes the rule rather than restating it. Tightening the pattern below +then cannot leave an error message describing the old grammar. +""" + +_SKILL_KEY_PATTERN = re.compile(r"\A[a-z0-9][a-z0-9-]*\Z") +""" +``SKILL_KEY_GRAMMAR``, anchored with ``\\A``/``\\Z`` rather than ``^``/``$`` +because ``$`` also matches immediately before a trailing newline, which would +let ``"pdf-extraction\\n"`` through as a directory name. +""" + +SKILL_KEY_MAX_LENGTH = 256 +"""Longest key the data model permits. Note that no mainstream filesystem allows +a 256-byte path component, so ``write_skills`` applies a tighter bound of its own.""" + def _is_object(v: Any) -> bool: return isinstance(v, dict) +def skill_key_rejection_reason(key: Any) -> str | None: + """ + Why *key* is not a valid skill key, or ``None`` when it is. + + The canonical explanation, so the config parser, the filesystem layer and + the reference projection all reject a key for the same stated reason. + ``is_valid_skill_key`` is this predicate with the reason discarded. + """ + if not isinstance(key, str): + return "must be a string" + if len(key) > SKILL_KEY_MAX_LENGTH: + return f"must be at most {SKILL_KEY_MAX_LENGTH} characters" + if _SKILL_KEY_PATTERN.match(key) is None: + return f"must match {SKILL_KEY_GRAMMAR}" + return None + + +def is_valid_skill_key(key: Any) -> TypeGuard[str]: + """Skill keys are untrusted input everywhere they appear — validate every time.""" + return isinstance(key, str) and skill_key_rejection_reason(key) is None + + +def is_valid_skill_version(version: Any) -> TypeGuard[int]: + """Skill versions are integers >= 1. ``bool`` is not an acceptable integer.""" + return isinstance(version, int) and not isinstance(version, bool) and version >= 1 + + def _parse_tool(raw: Any, key: str) -> str | None: """Returns an error message string or ``None`` on success.""" if not _is_object(raw): @@ -24,6 +70,28 @@ def _parse_tool(raw: Any, key: str) -> str | None: return None +def _parse_skills(raw: Any) -> str | None: + """ + Validates the optional ``skills`` array. Returns an error message or ``None``. + + Fail closed: a malformed reference makes the whole config malformed, because + an SDK that silently dropped a bad reference would materialize a partial + skill set without telling anyone. + """ + if not isinstance(raw, list): + return "skills must be an array of {key, version} objects" + + for index, entry in enumerate(raw): + if not _is_object(entry): + return f"skills[{index}] must be an object with key and version" + key_rejection = skill_key_rejection_reason(entry.get("key")) + if key_rejection is not None: + return f"skills[{index}].key {key_rejection}" + if not is_valid_skill_version(entry.get("version")): + return f"skills[{index}].version must be an integer >= 1" + return None + + def parse_ai_config(raw: Any) -> ParseResult: """ Validates a raw LaunchDarkly flag variation as an ``AiConfigRep``. @@ -88,4 +156,10 @@ def parse_ai_config(raw: Any) -> ParseResult: error={"message": "outputFormat must be an object (JSON Schema)"}, ) + skills = raw.get("skills") + if skills is not None: + err = _parse_skills(skills) + if err: + return ParseFailure(success=False, error={"message": err}) + return ParseSuccess(success=True, data=raw) diff --git a/packages/client/tests/conftest.py b/packages/client/tests/conftest.py index 6c14020..17fbb68 100644 --- a/packages/client/tests/conftest.py +++ b/packages/client/tests/conftest.py @@ -1,7 +1,14 @@ +import hashlib +from collections.abc import Iterator +from typing import Any from unittest.mock import AsyncMock, MagicMock import pytest +import launchdarkly_ai_server.lifecycle as lifecycle_module +import launchdarkly_ai_server.skills as skills_module +from launchdarkly_ai_server import InMemorySkillStore + @pytest.fixture def mock_ld_client() -> MagicMock: @@ -36,3 +43,108 @@ def mock_tracer(mock_span: MagicMock) -> MagicMock: tracer.start_as_current_span.return_value.__exit__ = MagicMock(return_value=False) tracer.start_span.return_value = mock_span return tracer + + +# --------------------------------------------------------------------------- +# Agent Skills helpers +# +# Exposed as fixtures rather than importable module-level helpers: pytest runs +# with --import-mode=importlib and the tests directory is not a package, so +# sibling imports from conftest are not reliable. +# --------------------------------------------------------------------------- + +SKILL_BODY = "---\nname: Test Skill\n---\nDo the thing.\n" + + +class _RecordingEmitter: + """Telemetry seam double — records (signal, properties) pairs.""" + + def __init__(self) -> None: + self.records: list[tuple[str, dict[str, Any]]] = [] + + def record(self, signal: str, properties: dict[str, Any]) -> None: + self.records.append((signal, properties)) + + def signals(self, name: str) -> list[dict[str, Any]]: + return [props for sig, props in self.records if sig == name] + + +class _ThrowingEmitter: + """Telemetry seam double whose record() always raises.""" + + def record(self, signal: str, properties: dict[str, Any]) -> None: + raise RuntimeError("emitter exploded") + + +@pytest.fixture +def make_raw_skill() -> Any: + """Factory for wire-shaped raw store objects with a correct contentHash.""" + + def _make( + key: str = "test-skill", + version: int = 1, + content: str = SKILL_BODY, + **overrides: Any, + ) -> dict[str, Any]: + obj: dict[str, Any] = { + "key": key, + "version": version, + "content": content, + "contentHash": hashlib.sha256(content.encode("utf-8")).hexdigest(), + "name": "Test Skill", + "description": "A skill used in tests.", + } + obj.update(overrides) + return obj + + return _make + + +@pytest.fixture +def store() -> InMemorySkillStore: + """An in-memory store, wired in as the configured store for the test.""" + s = InMemorySkillStore() + skills_module._set_store(s) + return s + + +class _ExplodingStore: + """Store double whose every read raises — the "transport is down" case.""" + + def get_object(self, kind: str, key: str) -> dict[str, Any] | None: + raise RuntimeError("transport failure") + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + raise RuntimeError("transport failure") + + +@pytest.fixture +def exploding_store() -> _ExplodingStore: + """A raising store, wired in as the configured store for the test.""" + s = _ExplodingStore() + skills_module._set_store(s) + return s + + +@pytest.fixture +def reset_skill_state() -> Iterator[None]: + """Clears client, store, and emitter module state around one test. + + Opted into per module with ``pytestmark = pytest.mark.usefixtures(...)`` + rather than being autouse here: autouse would newly reset lifecycle state + for every test in every module in this directory, which is a behaviour change + well outside the skills tests. + """ + lifecycle_module._reset_for_testing() + yield + lifecycle_module._reset_for_testing() + + +@pytest.fixture +def recording_emitter() -> _RecordingEmitter: + return _RecordingEmitter() + + +@pytest.fixture +def throwing_emitter() -> _ThrowingEmitter: + return _ThrowingEmitter() diff --git a/packages/client/tests/test_schema.py b/packages/client/tests/test_schema.py index fad75cf..40118b5 100644 --- a/packages/client/tests/test_schema.py +++ b/packages/client/tests/test_schema.py @@ -3,6 +3,10 @@ Reference: TESTING.md §3.5 """ +from typing import Any + +import pytest + from launchdarkly_ai_server import parse_ai_config @@ -81,3 +85,97 @@ def test_output_format_accepted(self) -> None: raw["outputFormat"] = {"type": "object", "properties": {}} result = parse_ai_config(raw) assert result.success is True + + +class TestParseAiConfigSkills: + """ + Fail-closed validation of the optional ``skills`` array. + """ + + def _base(self, **extra: Any) -> dict[str, Any]: + raw: dict[str, Any] = { + "model": {"name": "claude-3"}, + "provider": {"name": "Anthropic"}, + "instructions": "You are helpful.", + } + raw.update(extra) + return raw + + 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: + 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 + + @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", + [ + "", + "Evil", + "-leading-dash", + ".hidden", + "_underscore", + "has space", + "a/b", + "a\\b", + "../escape", + "trailing-space ", + "under_score", + "a" * 257, + ], + ) + 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"] diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py new file mode 100644 index 0000000..f21d0ac --- /dev/null +++ b/packages/client/tests/test_skills.py @@ -0,0 +1,1124 @@ +""" +Tests for Agent Skills types, frontmatter, reference discovery, content +accessors, integrity verification, and the telemetry seam. +""" + +from __future__ import annotations + +import dataclasses +import hashlib +import json +import sys +import time +from typing import Any +from unittest.mock import MagicMock + +import pytest + +import launchdarkly_ai_server.lifecycle as lifecycle_module +import launchdarkly_ai_server.skills as skills_module +from launchdarkly_ai_server import ( + InMemorySkillStore, + ReconcileAction, + ReconcileReport, + Skill, + SkillReference, + all_skills, + get_client, + get_skill, + get_skills, + init_client, + shutdown, + skill_refs, +) + +SKILL_BODY = "---\nname: Test Skill\n---\nDo the thing.\n" + +INTEGRITY_SIGNAL = "AgentControl Skill Integrity Failure" +MATERIALIZED_SIGNAL = "AgentControl Skill Materialized" +REVOKED_SIGNAL = "AgentControl Skill Revoked Received" + +# The three signal names are an allowlist, not a floor: any +# other name reaching the emitter is a regression. +APPROVED_SIGNALS = frozenset({INTEGRITY_SIGNAL, MATERIALIZED_SIGNAL, REVOKED_SIGNAL}) + +# These two were considered and deliberately excluded from SDK emission — +# named explicitly rather than relying on the subset check to be read as +# covering them. +REMOVED_SIGNALS = frozenset( + { + "AgentControl Skill SDK Reference Returned", + "AgentControl Skill Content Retrieved", + } +) + + +pytestmark = pytest.mark.usefixtures("reset_skill_state") +"""Every test in this module runs against freshly cleared module state.""" + + +def _hash(content: str) -> str: + """sha256, lowercase hex, over verbatim utf-8 bytes.""" + return hashlib.sha256(content.encode("utf-8")).hexdigest() + + +def _skill( + key: str = "test-skill", + version: int = 1, + content: str = SKILL_BODY, + content_hash: str | None = None, +) -> Skill: + """Build a verified-shaped Skill directly (bypasses the accessors).""" + return Skill( + key=key, + version=version, + content=content, + content_hash=content_hash if content_hash is not None else _hash(content), + ) + + +# --------------------------------------------------------------------------- +# Unencodable content +# +# A lone surrogate has no UTF-8 encoding, so there are no bytes LaunchDarkly +# could have hashed and the object is not authentic. ``json.loads`` is how one +# actually arrives: a payload carries the escape, and the parser hands back a +# real unpaired surrogate. +# --------------------------------------------------------------------------- + +UNENCODABLE_BODIES = ( + json.loads(r'"hi \ud800 there"'), # lone high surrogate + # A lone *low* surrogate, which is the one range errors="surrogateescape" + # smuggles through (as a raw 0x80 byte) while raising on everything else. + json.loads(r'"hi \udc80 there"'), +) + +NON_STRICT_HANDLERS = ( + "surrogatepass", + "surrogateescape", + "replace", + "ignore", + "backslashreplace", + "xmlcharrefreplace", + "namereplace", +) +"""Every ``str.encode`` error handler that is not ``strict``. + +``verified_bytes`` must use none of them: each one *fabricates* bytes for input +that has no encoding, and fabricated bytes can satisfy the hash comparison. +""" + + +def _fabricated_hash_cases() -> list[Any]: + """One case per (body, handler) pair the handler can actually encode. + + Each carries the sha256 of the bytes *that* handler would have produced, so + the case is not vacuous: an implementation that reached for the handler + would encode successfully, match the pinned hash, and return content + LaunchDarkly never delivered. Handlers that raise on a given body are + skipped — for that input they are as strict as ``strict``, so there is + nothing to detect. + """ + cases: list[Any] = [] + for index, body in enumerate(UNENCODABLE_BODIES): + for handler in NON_STRICT_HANDLERS: + try: + fabricated = body.encode("utf-8", errors=handler) + except UnicodeEncodeError: + continue + cases.append( + pytest.param( + body, + hashlib.sha256(fabricated).hexdigest(), + id=f"body{index}-{handler}", + ) + ) + return cases + + +FABRICATED_HASH_CASES = _fabricated_hash_cases() + + +class TestSkillTypes: + """Immutability and ReconcileReport.ok.""" + + def test_skill_reference_is_immutable(self) -> None: + ref = SkillReference(key="pdf-extraction", version=2) + with pytest.raises(dataclasses.FrozenInstanceError): + ref.version = 3 # type: ignore[misc] + + def test_skill_is_immutable(self) -> None: + skill = _skill() + with pytest.raises(dataclasses.FrozenInstanceError): + skill.content = "tampered" # type: ignore[misc] + + def test_skill_carries_optional_metadata(self) -> None: + skill = Skill( + key="a", + version=1, + content=SKILL_BODY, + content_hash=_hash(SKILL_BODY), + name="A Skill", + description="does things", + ) + assert skill.name == "A Skill" + assert skill.description == "does things" + + def test_skill_metadata_defaults_to_none(self) -> None: + skill = _skill() + 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 TestPackageExports: + """ + The on-the-wire / on-disk constants are public API and + must be reachable from the package root, not only from the sub-path module. + + The literal values are spelled out here on purpose: this is the one place + the constants themselves are asserted, so importing them to build the + expectation would make the assertion circular. Every other filesystem test + keeps writing the literals by hand for the same reason. + """ + + def test_constants_are_exported_from_the_package_root(self) -> None: + import launchdarkly_ai_server as package + + assert package.SKILL_OBJECT_KIND == "skill" + 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_OBJECT_KIND", + "SKILL_FILENAME", + "MANIFEST_FILENAME", + "MANIFEST_VERSION", + } + assert expected <= set(package.__all__) + + def test_content_cap_is_not_public_api(self) -> None: + """The content cap stays internal to ``skills_core`` — see the + ``MAX_SKILL_CONTENT_BYTES`` docstring there for why it is not exported.""" + import launchdarkly_ai_server as package + from launchdarkly_ai_server import skills_core + + assert skills_core.MAX_SKILL_CONTENT_BYTES == 65536 + assert "MAX_SKILL_CONTENT_BYTES" not in package.__all__ + assert not hasattr(package, "MAX_SKILL_CONTENT_BYTES") + + def test_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"} + + +class TestFrontmatter: + """Bounded, safe, lazy YAML frontmatter parsing.""" + + def test_valid_frontmatter_parses(self) -> None: + content = "---\nname: test\nversion: 1\n---\nBody text\n" + result = _skill(content=content).frontmatter() + assert result == {"name": "test", "version": 1} + + def test_absent_frontmatter_returns_none(self) -> None: + content = "# Just markdown\n\nNo frontmatter here.\n" + assert _skill(content=content).frontmatter() is None + + def test_unterminated_block_returns_none(self) -> None: + content = "---\nname: test\nnever closed\n" + assert _skill(content=content).frontmatter() is None + + def test_malformed_yaml_returns_none(self) -> None: + content = "---\nname: [unclosed\n bad: : :\n---\nBody\n" + assert _skill(content=content).frontmatter() is None + + def test_non_mapping_frontmatter_returns_none(self) -> None: + content = "---\n- one\n- two\n---\nBody\n" + assert _skill(content=content).frontmatter() is None + + def test_oversize_block_returns_none(self) -> None: + big = "\n".join(f"key{i}: {'x' * 80}" for i in range(200)) + content = f"---\n{big}\n---\nBody\n" + assert len(big) > 8 * 1024 + assert _skill(content=content).frontmatter() is None + + def test_deep_nesting_returns_none(self) -> None: + block = "".join(f"{' ' * i}k{i}:\n" for i in range(14)) + f"{' ' * 14}v: 1\n" + content = f"---\n{block}---\nBody\n" + started = time.monotonic() + result = _skill(content=content).frontmatter() + assert result is None + assert time.monotonic() - started < 5.0 + + def test_single_alias_returns_none(self) -> None: + """Alias resolution is *disabled*, not bounded, so one alias is + already disqualifying. This minimal case is the actual boundary the rule + draws; the billion-laughs bomb below is only a corollary of it.""" + content = "---\nname: test\nanchored: &a 1\naliased: *a\n---\nBody\n" + assert _skill(content=content).frontmatter() is None + + def test_billion_laughs_does_not_hang_or_crash(self) -> None: + """The classic bomb, which the alias rule rejects on sight. + + The threat here is *not* memory blow-up: PyYAML resolves aliases as + shared references, so plain ``yaml.safe_load`` parses this in about a + millisecond and returns a 7-key dict. Nor does any other bound catch it + — it is ~300 bytes and 6 levels deep, inside both the 8 KB and depth-10 + limits. The contract asserted is the one from + ``test_single_alias_returns_none``: an alias is present ⇒ None. The + elapsed-time bound below only guards a parser that *does* expand. + """ + bomb = ( + "---\n" + "a: &a ['x','x','x','x','x','x','x','x','x']\n" + "b: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a]\n" + "c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b]\n" + "d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c]\n" + "e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d]\n" + "f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e]\n" + "g: [*f,*f,*f,*f,*f,*f,*f,*f,*f]\n" + "---\nBody\n" + ) + started = time.monotonic() + result = _skill(content=bomb).frontmatter() + elapsed = time.monotonic() - started + assert result is None + assert elapsed < 5.0 + + def test_python_object_tag_is_inert(self) -> None: + content = "---\nevil: !!python/object/apply:os.system ['echo pwned']\n---\nB\n" + assert _skill(content=content).frontmatter() is None + + def test_custom_tag_is_inert(self) -> None: + content = "---\nevil: !SomeType {a: 1}\n---\nBody\n" + assert _skill(content=content).frontmatter() is None + + def test_returns_none_when_yaml_unavailable( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """pyyaml is dev-only; absence must not raise.""" + monkeypatch.setitem(sys.modules, "yaml", None) + content = "---\nname: test\n---\nBody\n" + assert _skill(content=content).frontmatter() is None + + def test_yaml_is_not_imported_at_module_scope(self) -> None: + """The import must live inside frontmatter(); a module-level ``import + yaml`` would bind a ``yaml`` attribute on the skills module and make + pyyaml a de-facto runtime dependency.""" + assert not hasattr(skills_module, "yaml") + + +class TestSkillRefs: + """Pure projection of the config's skills array.""" + + def _config(self, **extra: Any) -> dict[str, Any]: + base: dict[str, Any] = { + "model": {"name": "claude-3"}, + "provider": {"name": "Anthropic"}, + "instructions": "hi", + } + base.update(extra) + return base + + def test_absent_skills_returns_empty_list(self) -> None: + assert skill_refs(self._config()) == [] + + def test_empty_skills_returns_empty_list(self) -> None: + assert skill_refs(self._config(skills=[])) == [] + + def test_returns_typed_references_in_order(self) -> None: + config = self._config( + skills=[{"key": "a", "version": 1}, {"key": "b", "version": 3}] + ) + refs = skill_refs(config) + assert refs == [ + SkillReference(key="a", version=1), + SkillReference(key="b", version=3), + ] + assert all(isinstance(r, SkillReference) for r in refs) + + def test_emits_no_telemetry(self, recording_emitter: Any) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + skill_refs(self._config(skills=[{"key": "a", "version": 1}])) + assert recording_emitter.records == [] + + def test_dropped_entries_are_logged(self, caplog: pytest.LogCaptureFixture) -> None: + """A shortened projection is never silent. + + ``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. + """ + config = self._config( + skills=[ + {"key": "good", "version": 1}, + {"key": "bad", "version": 0}, + {"key": "Bad-Key", "version": 1}, + "not-an-object", + ] + ) + + 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) + + def test_requires_no_client_or_store(self, mock_ld_client: Any) -> None: + """No store configured, no client initialized — still a pure projection.""" + refs = skill_refs(self._config(skills=[{"key": "a", "version": 2}])) + assert refs == [SkillReference(key="a", version=2)] + mock_ld_client.track.assert_not_called() + + +class TestInMemorySkillStore: + """The public in-memory store implementation.""" + + def test_get_object_round_trips(self, make_raw_skill: Any) -> None: + raw = make_raw_skill(key="pdf-extraction", version=2) + s = InMemorySkillStore({"pdf-extraction": raw}) + assert s.get_object("skill", "pdf-extraction") == raw + + def test_get_object_unknown_key_returns_none(self) -> None: + assert InMemorySkillStore().get_object("skill", "nope") is None + + def test_put_then_get(self, make_raw_skill: Any) -> None: + s = InMemorySkillStore() + raw = make_raw_skill(key="a") + s.put(raw) + assert s.get_object("skill", "a") == raw + + def test_all_objects_returns_everything(self, make_raw_skill: Any) -> None: + s = InMemorySkillStore() + s.put(make_raw_skill(key="a")) + s.put(make_raw_skill(key="b")) + assert set(s.all_objects("skill").keys()) == {"a", "b"} + + def test_all_objects_unknown_kind_is_empty(self, make_raw_skill: Any) -> None: + s = InMemorySkillStore() + s.put(make_raw_skill(key="a")) + assert s.all_objects("flag") == {} + + def test_put_notifies_skill_kind_listeners(self, make_raw_skill: Any) -> None: + """``add_listener`` is part of the seam, so its one + implementation carries a smoke test for the callback contract: the raw + object, verbatim and unverified, as a single positional argument.""" + s = InMemorySkillStore() + seen: list[dict[str, Any]] = [] + s.add_listener("skill", seen.append) + raw = make_raw_skill(key="a") + + s.put(raw) + + assert seen == [raw] + + def test_put_does_not_notify_other_kind_listeners( + self, make_raw_skill: Any + ) -> None: + s = InMemorySkillStore() + seen: list[dict[str, Any]] = [] + s.add_listener("flag", seen.append) + + s.put(make_raw_skill(key="a")) + + assert seen == [] + + +class TestStoreConfiguration: + """Store wiring on the lifecycle layer.""" + + async def test_configured_via_init_client_option( + self, make_raw_skill: Any, mock_ld_client: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + await init_client(options={"skillStore": store}, client=mock_ld_client) + skill = await get_skill("a") + assert skill is not None + assert skill.key == "a" + + async def test_get_skill_raises_actionably_when_no_store(self) -> None: + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + async def test_get_skills_raises_actionably_when_no_store(self) -> None: + with pytest.raises(RuntimeError, match="skill store"): + await get_skills([SkillReference(key="a", version=1)]) + + async def test_all_skills_raises_actionably_when_no_store(self) -> None: + with pytest.raises(RuntimeError, match="skill store"): + await all_skills() + + async def test_shutdown_clears_the_store( + self, make_raw_skill: Any, mock_ld_client: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + await init_client(options={"skillStore": store}, client=mock_ld_client) + assert await get_skill("a") is not None + + await shutdown() + + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + async def test_skill_store_is_applied_on_every_init_client_call( + self, make_raw_skill: Any + ) -> None: + """``skillStore`` is the one option a second call applies. + + ``init_client`` is idempotent for the client singleton, and on a second + call every other option is ignored. ``skillStore`` is applied anyway, + on purpose: it is what lets a client that was lazily auto-initialized, + or initialized without a store, be given one afterwards. Both halves are + asserted on the same pair of calls, because each is meaningless without + the other. + """ + first_store = InMemorySkillStore() + first_store.put(make_raw_skill(key="first")) + second_store = InMemorySkillStore() + second_store.put(make_raw_skill(key="second")) + + first_client = MagicMock() + second_client = MagicMock() + + await init_client(options={"skillStore": first_store}, client=first_client) + await init_client(options={"skillStore": second_store}, client=second_client) + + # Half one: the client singleton is unchanged — the second call is a + # no-op for it, so the second client was discarded. + assert get_client() is first_client + + # Half two: the store was nevertheless swapped. + assert await get_skill("second") is not None + assert await get_skill("first") is None + + async def test_init_client_without_a_store_leaves_the_configured_one( + self, make_raw_skill: Any + ) -> None: + """Only a non-None ``skillStore`` replaces the configured store. + + Otherwise a bare ``init_client()`` from an unrelated code path — the + lazy auto-init, say — would silently unconfigure skills. + """ + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + await init_client(options={"skillStore": store}, client=MagicMock()) + + await init_client(client=MagicMock()) + + assert await get_skill("a") is not None + + async def test_failed_init_client_leaves_no_store_configured( + self, monkeypatch: pytest.MonkeyPatch, make_raw_skill: Any + ) -> None: + """A raising ``init_client`` must not leave global state behind. + + Installing the store before the SDK-key check would leave the accessors + working against a store the application believes was never installed, + masking a failed initialization. + """ + monkeypatch.delenv("LD_SDK_KEY", raising=False) + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + + with pytest.raises(RuntimeError, match="No LaunchDarkly SDK key"): + await init_client(options={"skillStore": store}) + + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + async def test_reset_for_testing_clears_the_store( + self, make_raw_skill: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + skills_module._set_store(store) + assert await get_skill("a") is not None + + lifecycle_module._reset_for_testing() + + with pytest.raises(RuntimeError, match="skill store"): + await get_skill("a") + + +class TestAccessorArgumentErrors: + """A bare string is a type error, not a reference. + + ``str`` satisfies ``Sequence[str]``, so the annotation on ``get_skills`` + admits a bare string and only this runtime guard catches it; iterating one + would look up a skill per character. Deliberately a *different* class from + ``write_skills``'s bare-string rejection, which is a ``ValueError`` because + a string is an accepted argument type there. + """ + + async def test_bare_string_raises_type_error( + self, store: InMemorySkillStore + ) -> None: + with pytest.raises(TypeError) as excinfo: + await get_skills("pdf-extraction") # type: ignore[arg-type] + + # The message has to name the fix, not merely reject the input. + assert "[key]" in str(excinfo.value) + + async def test_bare_string_is_rejected_before_the_store_is_consulted( + self, make_raw_skill: Any + ) -> None: + """The guard is an argument check, so it precedes store resolution. + + Asserting the raise alone would also pass if the string were iterated + into single-character lookups that all missed, so pin that no lookup + happened at all. + """ + looked_up: list[str] = [] + + class _RecordingStore: + def get_object(self, kind: str, key: str) -> dict[str, Any] | None: + looked_up.append(key) + return None + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + return {} + + skills_module._set_store(_RecordingStore()) + + with pytest.raises(TypeError): + await get_skills("abc") # type: ignore[arg-type] + + assert looked_up == [] + + +class TestGetSkill: + """Single-skill accessor.""" + + async def test_returns_verified_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="pdf-extraction", version=2)) + skill = await get_skill("pdf-extraction") + assert skill is not None + assert skill.key == "pdf-extraction" + assert skill.version == 2 + assert skill.content == SKILL_BODY + assert skill.content_hash == _hash(SKILL_BODY) + assert skill.name == "Test Skill" + assert skill.description == "A skill used in tests." + + async def test_version_omitted_returns_newest_available( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=7)) + skill = await get_skill("a") + assert skill is not None + assert skill.version == 7 + + async def test_exact_version_match_returns_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=3)) + skill = await get_skill("a", version=3) + assert skill is not None + assert skill.version == 3 + + async def test_other_version_returns_none( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=3)) + assert await get_skill("a", version=2) is None + assert await get_skill("a", version=4) is None + + async def test_missing_key_returns_none(self, store: InMemorySkillStore) -> None: + assert await get_skill("nope") is None + + async def test_multibyte_content_verifies( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + content = "---\nname: emoji\n---\n🚀 unicode ✅ body\n" + store.put(make_raw_skill(key="a", content=content)) + skill = await get_skill("a") + assert skill is not None + assert skill.content == content + + +class TestGetSkills: + """Batch accessor.""" + + async def test_mixed_refs_and_strings( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=1)) + store.put(make_raw_skill(key="b", version=5)) + result = await get_skills([SkillReference(key="a", version=1), "b"]) + assert [s.key for s in result] == ["a", "b"] + + async def test_preserves_input_order( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + for k in ("a", "b", "c"): + store.put(make_raw_skill(key=k)) + result = await get_skills(["c", "a", "b"]) + assert [s.key for s in result] == ["c", "a", "b"] + + async def test_missing_entries_are_omitted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a")) + result = await get_skills(["a", "missing"]) + assert [s.key for s in result] == ["a"] + + async def test_version_mismatch_is_omitted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", version=2)) + result = await get_skills([SkillReference(key="a", version=1)]) + assert result == [] + + async def test_empty_input_returns_empty_list( + self, store: InMemorySkillStore + ) -> None: + assert await get_skills([]) == [] + + async def test_integrity_failure_omitted_and_signalled( + self, + store: InMemorySkillStore, + make_raw_skill: Any, + recording_emitter: Any, + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="good-a")) + store.put(make_raw_skill(key="bad", contentHash="0" * 64)) + store.put(make_raw_skill(key="good-b")) + + result = await get_skills(["good-a", "bad", "good-b"]) + + assert [s.key for s in result] == ["good-a", "good-b"] + failures = recording_emitter.signals(INTEGRITY_SIGNAL) + assert len(failures) == 1 + assert failures[0]["skill_key"] == "bad" + + +class TestAllSkills: + """All_skills accessor.""" + + async def test_returns_every_verified_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + for k in ("a", "b", "c"): + store.put(make_raw_skill(key=k)) + result = await all_skills() + assert {s.key for s in result} == {"a", "b", "c"} + + async def test_empty_store_returns_empty_list( + self, store: InMemorySkillStore + ) -> None: + assert await all_skills() == [] + + async def test_omits_skills_that_fail_verification( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="good")) + store.put(make_raw_skill(key="bad", contentHash="deadbeef")) + result = await all_skills() + assert {s.key for s in result} == {"good"} + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + +class TestIntegrityVerification: + """Mandatory verification at the accessor boundary.""" + + async def test_hash_mismatch_withholds_skill( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="a", contentHash="a" * 64)) + assert await get_skill("a") is None + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_tampered_content_withholds_skill( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + raw = make_raw_skill(key="a") + raw["content"] = raw["content"] + "x" # hash now stale by one byte + store.put(raw) + assert await get_skill("a") is None + + async def test_oversize_content_rejected( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + oversize = "x" * (64 * 1024 + 1) + store.put(make_raw_skill(key="a", content=oversize)) + assert await get_skill("a") is None + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_content_at_size_cap_is_accepted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + at_cap = "x" * (64 * 1024) + store.put(make_raw_skill(key="a", content=at_cap)) + skill = await get_skill("a") + assert skill is not None + assert len(skill.content) == 64 * 1024 + + async def test_key_at_length_bound_from_store_accepted( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + """The accepting side of the <= 256 bound. + + ``write_skills`` cannot reach this bound (a key is one directory name and + NAME_MAX is 255), so config validation and this accessor-side + revalidation are the only two layers where 256 is observable at all. The + rejecting side is ``test_invalid_key_from_store_rejected["x" * 257]``. + """ + key = "a" * 256 + store.put(make_raw_skill(key=key)) + skill = await get_skill(key) + assert skill is not None + assert skill.key == key + + @pytest.mark.parametrize( + "bad_key", + [ + "Evil", + "-leading-dash", + ".hidden", + "has space", + "a/b", + "../escape", + "", + "x" * 257, + ], + ) + async def test_invalid_key_from_store_rejected( + self, make_raw_skill: Any, bad_key: str + ) -> None: + """A hostile store may serve any key — the accessor revalidates.""" + raw = make_raw_skill(key="placeholder") + raw["key"] = bad_key + skills_module._set_store(InMemorySkillStore({bad_key: raw})) + assert await get_skill(bad_key) is None + + @pytest.mark.parametrize("bad_version", [0, -1, 2.5, "2", None, True]) + async def test_invalid_version_from_store_rejected( + self, make_raw_skill: Any, bad_version: Any + ) -> None: + raw = make_raw_skill(key="a", version=bad_version) + skills_module._set_store(InMemorySkillStore({"a": raw})) + assert await get_skill("a") is None + + async def test_missing_content_rejected(self, make_raw_skill: Any) -> None: + raw = make_raw_skill(key="a") + del raw["content"] + skills_module._set_store(InMemorySkillStore({"a": raw})) + assert await get_skill("a") is None + + async def test_missing_content_hash_rejected(self, make_raw_skill: Any) -> None: + raw = make_raw_skill(key="a") + del raw["contentHash"] + skills_module._set_store(InMemorySkillStore({"a": raw})) + assert await get_skill("a") is None + + async def test_uppercase_hash_rejected( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + """Hashes are lowercase hex; a non-canonical hash is not authentic.""" + store.put(make_raw_skill(key="a", contentHash=_hash(SKILL_BODY).upper())) + assert await get_skill("a") is None + + @pytest.mark.parametrize(("body", "fabricated_hash"), FABRICATED_HASH_CASES) + async def test_unencodable_content_withheld( + self, recording_emitter: Any, body: str, fabricated_hash: str + ) -> None: + """Content with no UTF-8 encoding is withheld, and the signal recorded. + + ``str.encode`` raises on a lone surrogate, so ``verified_bytes`` has an + exception to catch and never sees bytes for this content at all. + + The parametrization is what makes that observable. The guard must never + pass ``errors="surrogatepass"``, or any other non-strict handler: each + of them fabricates bytes for input that has no encoding, and fabricated + bytes can satisfy the hash comparison. Every case here supplies the + sha256 of the bytes one such handler would have produced, so an + implementation that reached for one would verify this object + successfully and hand back content LaunchDarkly never sent. An + arbitrary wrong hash would not catch that — the mismatch check would + reject the input before the encoder guard was reached. + """ + with pytest.raises(UnicodeEncodeError): + body.encode("utf-8") # the premise: there is no encoding to hash + + skills_module._set_emitter_for_testing(recording_emitter) + skills_module._set_store( + InMemorySkillStore( + { + "a": { + "key": "a", + "version": 1, + "content": body, + "contentHash": fabricated_hash, + } + } + ) + ) + + assert await get_skill("a") is None + assert len(recording_emitter.signals(INTEGRITY_SIGNAL)) == 1 + + async def test_store_error_does_not_leak_content( + self, exploding_store: Any + ) -> None: + assert await get_skill("a") is None + assert await get_skills(["a"]) == [] + assert await all_skills() == [] + + +class TestTelemetrySeam: + """Internal emitter seam, no client.track, no context.""" + + async def test_default_emitter_is_noop( + self, store: InMemorySkillStore, make_raw_skill: Any + ) -> None: + store.put(make_raw_skill(key="a", contentHash="0" * 64)) + assert await get_skill("a") is None # no emitter injected, no raise + + async def test_integrity_signal_properties( + self, + store: InMemorySkillStore, + make_raw_skill: Any, + recording_emitter: Any, + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="a", version=4, contentHash="b" * 64)) + + await get_skill("a") + + props = recording_emitter.signals(INTEGRITY_SIGNAL)[0] + assert props["skill_key"] == "a" + assert props["version"] == 4 + assert props["expected_hash"] == "b" * 64 + assert props["observed_hash"] == _hash(SKILL_BODY) + assert props["language"] == "python" + + async def test_skill_body_never_appears_in_signals( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="a", contentHash="c" * 64)) + + await get_skill("a") + + for _signal, props in recording_emitter.records: + for value in props.values(): + assert "Do the thing." not in str(value) + + # ``skill_key`` and ``expected_hash`` are copied off the wire, so a hostile + # store can smuggle the body through either one and publish it in a signal + # that is otherwise body-free. The sweep above cannot see that: it serves a + # well-formed 64-character digest under a valid key, so neither replacement + # branch ever runs, and it passes even against an implementation that copies + # both fields verbatim. These two cases are what make the rule observable. + # Both assert the body's *absence* rather than the placeholder's exact + # spelling, which is not part of the contract. + + async def test_body_smuggled_through_content_hash_is_redacted( + self, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + body = "UNIQUE-SECRET-BODY-VIA-HASH" + skills_module._set_store( + InMemorySkillStore( + {"a": {"key": "a", "version": 1, "content": body, "contentHash": body}} + ) + ) + + assert await get_skill("a") is None + + signals = recording_emitter.signals(INTEGRITY_SIGNAL) + assert len(signals) == 1 + for value in signals[0].values(): + assert body not in str(value) + + async def test_body_smuggled_through_the_key_is_redacted( + self, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + # Uppercase and a path separator, so this is not a valid skill key and + # the invalid-key branch is the one that has to redact it. + body = "UNIQUE-SECRET-BODY-VIA-KEY/../x" + skills_module._set_store( + InMemorySkillStore( + {body: {"key": body, "version": 1, "content": "x", "contentHash": "y"}} + ) + ) + + assert await get_skill(body) is None + + signals = recording_emitter.signals(INTEGRITY_SIGNAL) + assert len(signals) == 1 + for value in signals[0].values(): + assert body not in str(value) + + async def test_no_ld_track_calls_from_accessors( + self, make_raw_skill: Any, mock_ld_client: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="a")) + store.put(make_raw_skill(key="bad", contentHash="0" * 64)) + await init_client(options={"skillStore": store}, client=mock_ld_client) + + await get_skill("a") + await get_skill("bad") + await get_skills(["a"]) + await all_skills() + + mock_ld_client.track.assert_not_called() + + async def test_throwing_emitter_never_breaks_the_operation( + self, store: InMemorySkillStore, make_raw_skill: Any, throwing_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(throwing_emitter) + store.put(make_raw_skill(key="bad", contentHash="0" * 64)) + store.put(make_raw_skill(key="good")) + + assert await get_skill("bad") is None + good = await get_skill("good") + assert good is not None + assert good.key == "good" + + async def test_accessors_record_no_signal_outside_the_approved_set( + self, store: InMemorySkillStore, make_raw_skill: Any, recording_emitter: Any + ) -> None: + """The three names are an allowlist, not a floor. + + Asserted over the recorded strings, so nothing here mandates a + particular module-level constant. The write-side half of this sweep is + ``test_write_skills_records_no_signal_outside_the_approved_set`` in + test_skills_fs.py, where all four reconcile actions can be staged. + + Guards the most likely regression: an implementation that also emits + ``AgentControl Skill Content Retrieved`` from ``get_skill``, or + ``AgentControl Skill SDK Reference Returned`` from ``skill_refs``, + passes every other test in this class. + """ + skills_module._set_emitter_for_testing(recording_emitter) + store.put(make_raw_skill(key="good")) + store.put(make_raw_skill(key="tampered", contentHash="0" * 64)) + + assert await get_skill("good") is not None + assert await get_skill("tampered") is None + await get_skills(["good", "tampered"]) + await all_skills() + skill_refs({"skills": [{"key": "good", "version": 1}]}) + + recorded = {signal for signal, _props in recording_emitter.records} + assert recorded <= APPROVED_SIGNALS, ( + f"unapproved signal(s): {sorted(recorded - APPROVED_SIGNALS)}" + ) + assert not recorded & REMOVED_SIGNALS + # Positive control: a subset assertion is satisfied vacuously by an + # implementation that records nothing at all. + assert INTEGRITY_SIGNAL in recorded diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py new file mode 100644 index 0000000..60adc6f --- /dev/null +++ b/packages/client/tests/test_skills_fs.py @@ -0,0 +1,1618 @@ +""" +Tests for ``write_skills`` — filesystem materialization, manifest reconcile +semantics, and the full security abuse matrix. + +Every test writes only inside pytest's ``tmp_path``. No network, no real +LaunchDarkly client, no real skill transport. +""" + +from __future__ import annotations + +import hashlib +import json +import os +import stat +from pathlib import Path +from typing import Any, NamedTuple + +import pytest + +import launchdarkly_ai_server.safe_fs as safe_fs_module +import launchdarkly_ai_server.skills as skills_module +import launchdarkly_ai_server.skills_fs as skills_fs_module +from launchdarkly_ai_server import ( + InMemorySkillStore, + Skill, + SkillReference, + get_skill, + init_client, + write_skills, +) + +MANIFEST_NAME = ".launchdarkly-skills.json" +SKILL_BODY = "---\nname: Test Skill\n---\nDo the thing.\n" + +MATERIALIZED_SIGNAL = "AgentControl Skill Materialized" +REVOKED_SIGNAL = "AgentControl Skill Revoked Received" +INTEGRITY_SIGNAL = "AgentControl Skill Integrity Failure" + +# The three signal names are an allowlist, not a floor. +APPROVED_SIGNALS = frozenset({MATERIALIZED_SIGNAL, REVOKED_SIGNAL, INTEGRITY_SIGNAL}) + +# Considered and deliberately excluded from SDK emission — named explicitly +# so the regression is unmissable. +REMOVED_SIGNALS = frozenset( + { + "AgentControl Skill SDK Reference Returned", + "AgentControl Skill Content Retrieved", + } +) + + +pytestmark = pytest.mark.usefixtures("reset_skill_state") +"""Every test in this module runs against freshly cleared module state.""" + + +def _hash(content: str) -> str: + 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, 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"] + + +_INJECTED = "simulated crash between write and rename" + + +def _dir_id(path: Path) -> tuple[int, int]: + """``(st_dev, st_ino)`` — a directory's identity, independent of its name.""" + info = os.stat(path) + return (info.st_dev, info.st_ino) + + +class _RenameCall(NamedTuple): + """One intercepted ``os.replace`` of a ``SKILL.md``. + + ``src``/``dst`` are exactly what the implementation passed. Where the rename + is ``dir_fd``-relative they are bare filenames and the location lives in the + descriptors, so ``*_dir_id`` carries each descriptor's ``(st_dev, st_ino)`` + resolved *at call time* — the implementation closes the descriptors as soon + as the write returns, so they cannot be resolved from the assertions. + """ + + src: str + dst: str + src_dir_fd: int | None + dst_dir_fd: int | None + src_dir_id: tuple[int, int] | None + dst_dir_id: tuple[int, int] | None + + +class _ReplaceSpy: + """Records — and optionally fails — every atomic rename of a ``SKILL.md``. + + Write/rename interception hook: the implementation performs + the final rename through a single ``os.replace`` call site, so patching the + attribute on the ``os`` module observes it. Destinations other than + ``SKILL.md`` (i.e. the manifest's own atomic write) pass straight through — + the filter holds for both call shapes, since the ``dir_fd``-relative form + passes ``"SKILL.md"`` itself as ``dst``. + + Used two ways: to prove an injected failure is what produced an ``error`` + action (atomicity), and to prove no write was *attempted* for a + rejected key — the OS would reject several hostile keys on its + own, so a failed write is not evidence of a defense. + """ + + def __init__(self, fail: bool = False) -> None: + self.calls: list[_RenameCall] = [] + self._fail = fail + self._real = os.replace + + def __call__(self, src: Any, dst: Any, **kwargs: Any) -> None: + if str(dst).endswith("SKILL.md"): + src_dir_fd = kwargs.get("src_dir_fd") + dst_dir_fd = kwargs.get("dst_dir_fd") + self.calls.append( + _RenameCall( + src=str(src), + dst=str(dst), + src_dir_fd=src_dir_fd, + dst_dir_fd=dst_dir_fd, + src_dir_id=None if src_dir_fd is None else _fd_id(src_dir_fd), + dst_dir_id=None if dst_dir_fd is None else _fd_id(dst_dir_fd), + ) + ) + if self._fail: + raise OSError(_INJECTED) + self._real(src, dst, **kwargs) + + def install(self, monkeypatch: pytest.MonkeyPatch) -> _ReplaceSpy: + # The attribute is set on the shared ``os`` module, so the + # single ``os.replace`` call site in safe_fs is intercepted wherever it is + # reached from. Named through the calling module rather than an arbitrary + # one so the hook documents which code it covers. + monkeypatch.setattr(safe_fs_module.os, "replace", self) + return self + + +def _fd_id(fd: int) -> tuple[int, int]: + info = os.fstat(fd) + return (info.st_dev, info.st_ino) + + +def _assert_atomic_rename_of(spy: _ReplaceSpy, skill_dir: Path) -> None: + """Assert the one recorded rename put ``SKILL.md`` into *skill_dir*. + + The temp file must be created in the target's own + directory, so the rename is atomic rather than cross-device. Two call + shapes prove it. Where the platform has ``renameat`` + the rename is ``dir_fd``-relative and the property is asserted by descriptor + identity — one descriptor for both sides, resolving to *skill_dir*'s inode — + which is stronger than comparing path strings, because it also rules out the + descriptor having been redirected between the check and the rename. On the + ``lstat`` floor (Windows) the names are full paths and share a parent. + """ + assert len(spy.calls) == 1 + call = spy.calls[0] + + if safe_fs_module.SUPPORTS_DIR_FD: + assert call.dst == "SKILL.md" + assert call.src != "SKILL.md" + assert call.src_dir_fd is not None + assert call.src_dir_fd == call.dst_dir_fd + assert call.dst_dir_id == _dir_id(skill_dir) + else: + assert Path(call.dst) == skill_dir / "SKILL.md" + assert Path(call.src).parent == skill_dir + assert Path(call.src).name != "SKILL.md" + + +class _SwapDirectoryDuring: + """Fires the directory-swap race at the exact instant of an operation. + + Renames ``/`` aside and leaves a symlink to *outside* in its + place, then lets the intercepted call proceed — the narrowest possible + version of the window an attacker with write access to the managed root + would otherwise have to hit by timing. Both hooks are the + interception points (``os.replace`` for the write, ``os.unlink`` for the + prune), so no implementation internals are touched. + """ + + def __init__(self, attribute: str, skill_dir: Path, outside: Path) -> None: + self.attribute = attribute + self.skill_dir = skill_dir + self.moved_to = skill_dir.parent / f"{skill_dir.name}.real" + self.outside = outside + self.swapped = False + self._real = getattr(os, attribute) + + def __call__(self, first: Any, *args: Any, **kwargs: Any) -> Any: + named = args[0] if args else first + if str(named).endswith("SKILL.md") and not self.swapped: + os.rename(self.skill_dir, self.moved_to) + os.symlink(self.outside, self.skill_dir, target_is_directory=True) + self.swapped = True + return self._real(first, *args, **kwargs) + + def install(self, monkeypatch: pytest.MonkeyPatch) -> _SwapDirectoryDuring: + # ``os.replace`` is called from safe_fs, ``os.unlink`` from skills_fs; both + # resolve to the same module object, so either name reaches both. + module = safe_fs_module if self.attribute == "replace" else skills_fs_module + monkeypatch.setattr(module.os, self.attribute, self) + return self + + +_needs_dir_fd = pytest.mark.skipif( + not safe_fs_module.SUPPORTS_DIR_FD, + reason="no *at() family on this platform; the per-component lstat floor applies", +) + + +@pytest.fixture +def root(tmp_path: Path) -> Path: + r = tmp_path / "skills" + r.mkdir() + return r + + +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 TestAtomicityAndPermissions: + """Atomic writes, no partial files, 0644.""" + + async def test_written_file_is_0644_and_not_executable(self, root: Path) -> None: + await write_skills([_skill("a")], root) + mode = stat.S_IMODE((root / "a" / "SKILL.md").stat().st_mode) + assert mode == 0o644 + assert not mode & stat.S_IXUSR + assert not mode & stat.S_IXGRP + assert not mode & stat.S_IXOTH + + async def test_write_goes_through_a_single_atomic_rename( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Positive control for the interception hook. + + Without this, the ``spy.calls == []`` assertions in the failure tests + below and in the traversal matrix could pass in a suite where the hook + is never reachable at all. + """ + spy = _ReplaceSpy().install(monkeypatch) + + report = await write_skills([_skill("a")], root) + + assert report.ok is True + # The temp file is created in the *same* directory + # as the target, so the rename is atomic rather than cross-device. + _assert_atomic_rename_of(spy, root / "a") + assert (root / "a" / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + + async def test_rename_failure_leaves_prior_content_intact( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + target = _place_managed(root, "a", SKILL_BODY) + spy = _ReplaceSpy(fail=True).install(monkeypatch) + + report = await write_skills([_skill("a", 2, "brand new content\n")], root) + + # The injected failure — not an unrelated rejection, and not an + # implementation that attempted nothing — is what produced the error. + _assert_atomic_rename_of(spy, target.parent) + + assert report.ok is False + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert _INJECTED in (action.error or "") + + assert target.read_text(encoding="utf-8") == SKILL_BODY + # No temp artifact survives the failed run. + assert sorted(p.name for p in target.parent.iterdir()) == ["SKILL.md"] + + async def test_no_partial_file_at_target_after_failure( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + spy = _ReplaceSpy(fail=True).install(monkeypatch) + + report = await write_skills([_skill("a")], root) + + _assert_atomic_rename_of(spy, root / "a") + + assert report.ok is False + assert _INJECTED in (_actions_by_key(report)["a"].error or "") + assert not (root / "a" / "SKILL.md").exists() + # Neither a partial target nor a leaked temp file. + skill_dir = root / "a" + leftovers = ( + sorted(p.name for p in skill_dir.iterdir()) if skill_dir.exists() else [] + ) + assert leftovers == [] + + async def test_manifest_is_valid_json_after_a_run_with_errors( + self, root: Path + ) -> None: + report = await write_skills([_skill("a"), _skill("../evil")], root) + assert report.ok is False + assert isinstance(_read_manifest(root), dict) + + +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, 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="new content\n", content_hash="f" * 64) + + await write_skills([bad], root) + + assert target.read_text(encoding="utf-8") == SKILL_BODY + + +# --------------------------------------------------------------------------- +# Security abuse matrix +# --------------------------------------------------------------------------- + +HOSTILE_KEYS = [ + "../evil", + "..", + ".", + "", + "/etc/cron.d/x", + "..\\evil", + "c:evil", + "skill:ads", + "sk\0ill", + "-skill", + "Evil", + "a/b", + "x" * 257, + "a/../../b", + "./a", + " leading-space", + "trailing-space ", +] + + +class TestPathTraversal: + """Nothing is ever written outside the root.""" + + @pytest.mark.parametrize("hostile_key", HOSTILE_KEYS) + async def test_hostile_key_is_rejected( + self, tmp_path: Path, hostile_key: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + root = tmp_path / "skills" + root.mkdir() + outside_before = sorted(p.name for p in tmp_path.iterdir()) + spy = _ReplaceSpy().install(monkeypatch) + + report = await write_skills([_skill(hostile_key)], root) + + assert report.ok is False + assert [a.action for a in report.actions if a.key == hostile_key] == ["error"] + + # The SDK's key validation — not the operating system — must be what + # stopped this. An overlong key exceeds NAME_MAX, a null byte raises in + # the path API, and an absolute path outside the root usually fails on + # permissions, so "an error was reported" is not evidence of a defense + # (and the absolute-path verdict would flip on a privileged runner). + # Assert instead that no write was ever attempted. + assert spy.calls == [] + + # Nothing created outside the root, and no skill directory inside it. + assert sorted(p.name for p in tmp_path.iterdir()) == outside_before + assert [p.name for p in root.iterdir() if p.name != MANIFEST_NAME] == [] + assert list(root.rglob("SKILL.md")) == [] + + async def test_interception_hook_fires_for_a_valid_key( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Positive control for the ``spy.calls == []`` assertion above.""" + spy = _ReplaceSpy().install(monkeypatch) + + report = await write_skills([_skill("ok-key")], root) + + assert report.ok is True + assert [Path(call.dst).name for call in spy.calls] == ["SKILL.md"] + + async def test_long_but_filesystem_legal_key_is_written(self, root: Path) -> None: + """The ≤ 256 length bound cannot be exercised through ``write_skills``. + + A key becomes a single directory name and NAME_MAX is 255 bytes on Linux + and macOS, so the longest key the data model permits cannot exist on + disk at all. Assert the accepting side at the largest writable length; + the bound itself is covered by the pure layers (config validation and + accessor revalidation). + """ + key = "k" * 255 + report = await write_skills([_skill(key)], root) + + assert report.ok is True + assert (root / key / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + + async def test_key_at_the_data_model_bound_is_reported_not_raised( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A 256-character key is valid to every pure layer but fits no filesystem. + + Config validation and the accessors must both accept exactly 256 + characters, yet NAME_MAX is 255 + on Linux and macOS, so this key reaches ``write_skills`` legitimately and + cannot become a directory. Every outcome must be visible in + the report, so it must surface as an ``error`` action rather than an + ``OSError`` escaping the call — which would also skip the manifest rewrite + and orphan any file already written in the same run. + """ + spy = _ReplaceSpy().install(monkeypatch) + long_key = "a" * 256 + + report = await write_skills([_skill("good"), _skill(long_key)], root) + + by_key = _actions_by_key(report) + assert by_key[long_key].action == "error" + assert by_key["good"].action == "written" + # The bare-filename ``dst`` of a ``dir_fd``-relative rename carries no + # directory, so "the path does not contain the hostile key" is no longer + # a meaningful check. Assert the stronger thing instead: the only rename + # that happened was into the valid skill's own directory. + assert [call.dst_dir_id for call in spy.calls] == [_dir_id(root / "good")] + # The valid skill is fully reconciled: written AND recorded, not orphaned. + assert (root / "good" / "SKILL.md").exists() + assert "good/SKILL.md" in _read_manifest(root)["entries"] + + async def test_valid_keys_still_write_alongside_rejected_ones( + self, root: Path + ) -> None: + report = await write_skills([_skill("good"), _skill("../evil")], root) + by_key = _actions_by_key(report) + assert by_key["good"].action == "written" + assert by_key["../evil"].action == "error" + assert (root / "good" / "SKILL.md").exists() + + async def test_traversal_key_does_not_create_parent_files( + self, tmp_path: Path + ) -> None: + root = tmp_path / "skills" + root.mkdir() + await write_skills([_skill("../../escaped")], root) + assert not (tmp_path / "escaped").exists() + assert not (tmp_path.parent / "escaped").exists() + + +@pytest.mark.skipif( + not hasattr(os, "symlink"), reason="platform has no symlink support" +) +class TestSymlinkAttacks: + """Never write through a symlink.""" + + async def test_symlinked_root_raises(self, tmp_path: Path) -> None: + real_dir = tmp_path / "real" + real_dir.mkdir() + link_root = tmp_path / "link" + link_root.symlink_to(real_dir, target_is_directory=True) + + with pytest.raises(ValueError): + await write_skills([_skill("a")], link_root) + + assert list(real_dir.iterdir()) == [] + + async def test_symlinked_skill_directory_is_refused(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (root / "a").symlink_to(outside, target_is_directory=True) + + report = await write_skills([_skill("a")], root) + + assert report.ok is False + assert _actions_by_key(report)["a"].action == "error" + assert list(outside.iterdir()) == [] + + async def test_symlinked_target_file_is_refused(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + root.mkdir() + outside_file = tmp_path / "victim.md" + outside_file.write_text("victim content\n", encoding="utf-8") + (root / "a").mkdir() + (root / "a" / "SKILL.md").symlink_to(outside_file) + # Manifest lists the path so clobber protection is not what saves us. + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": {"a/SKILL.md": _entry("a", 1, "victim content\n")}, + }, + ) + + report = await write_skills([_skill("a", 2, "attacker payload\n")], root) + + assert report.ok is False + assert _actions_by_key(report)["a"].action == "error" + assert outside_file.read_text(encoding="utf-8") == "victim content\n" + + async def test_symlinked_target_is_not_pruned(self, tmp_path: Path) -> None: + """A manifest-listed path that is a symlink is refused, not unlinked. + + Asserting only that the victim file survives proves nothing here: + unlinking a symlink never touches its target, so that assertion holds + for an implementation with no symlink check at all. The observable + contract is the refusal itself (prune path). + """ + root = tmp_path / "skills" + root.mkdir() + outside_file = tmp_path / "victim.md" + outside_file.write_text("victim content\n", encoding="utf-8") + (root / "a").mkdir() + link = root / "a" / "SKILL.md" + link.symlink_to(outside_file) + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": {"a/SKILL.md": _entry("a", 1, "victim content\n")}, + }, + ) + + report = await write_skills([], root) + + assert report.ok is False + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert action.error is not None + assert [a for a in report.actions if a.action == "removed"] == [] + # The symlink itself is left in place and stays managed. + assert link.is_symlink() + assert "a/SKILL.md" in _read_manifest(root)["entries"] + assert outside_file.read_text(encoding="utf-8") == "victim content\n" + + @_needs_dir_fd + async def test_directory_swapped_at_the_rename_cannot_redirect_the_write( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The swap window is closed, not merely narrowed. + + Every check in the world is worthless if the final rename re-resolves + ``/`` from its path: an attacker holding write permission on + the managed root can replace the validated directory with a symlink in + between and redirect the write out of the root. The rename is therefore + performed relative to a descriptor pinned to the directory that was + checked, so it follows the inode rather than the name. + """ + root = tmp_path / "skills" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + race = _SwapDirectoryDuring("replace", root / "a", outside).install(monkeypatch) + + report = await write_skills([_skill("a")], root) + + assert race.swapped is True, "the race never fired; the test proves nothing" + assert list(outside.iterdir()) == [] + assert (race.moved_to / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + assert report.ok is True + + @_needs_dir_fd + async def test_directory_swapped_at_the_prune_cannot_redirect_the_unlink( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The same window on the destructive side. + + ``unlink`` never follows a *trailing* symlink, but it does resolve the + directory above it, so the swap turns a prune into a delete of an + attacker-chosen outside file. The unlink is descriptor-relative for the + same reason the rename is. + """ + root = tmp_path / "skills" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + victim = outside / "SKILL.md" + victim.write_text("precious\n", encoding="utf-8") + _place_managed(root, "a", SKILL_BODY) + race = _SwapDirectoryDuring("unlink", root / "a", outside).install(monkeypatch) + + report = await write_skills([], root) + + assert race.swapped is True, "the race never fired; the test proves nothing" + assert victim.read_text(encoding="utf-8") == "precious\n" + assert not (race.moved_to / "SKILL.md").exists() + assert [a.action for a in report.actions if a.key == "a"] == ["removed"] + + +class TestWithoutDirFd: + """The full-path fallback for platforms with no ``*at()`` family. + + On Windows ``os.open`` cannot open a directory at all, so acquiring the + descriptor must not even be attempted there — a fallback reached only after + a descriptor open would leave every write, prune and manifest rewrite + failing rather than falling back. These tests force the flag off so the + fallback is exercised on POSIX too. + """ + + @pytest.fixture(autouse=True) + def _no_dir_fd(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Models Windows: no ``*at()`` family, and directories cannot be opened. + + Forcing the flag off alone would not reproduce the platform, because + ``os.open`` on a directory succeeds on POSIX — the fallback would be + reached either way. Making that call raise the ``PermissionError`` + Windows raises is what proves the descriptor open is never attempted. + """ + monkeypatch.setattr(safe_fs_module, "SUPPORTS_DIR_FD", False) + real_open = os.open + + def no_directory_open(path: Any, *args: Any, **kwargs: Any) -> int: + if os.path.isdir(path): + raise PermissionError(13, "Permission denied") + return real_open(path, *args, **kwargs) + + monkeypatch.setattr(safe_fs_module.os, "open", no_directory_open) + + async def test_write_prune_and_manifest_all_succeed(self, root: Path) -> None: + first = await write_skills([_skill("a"), _skill("b")], root) + assert first.ok is True, _error_messages(first) + assert (root / "a" / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + assert _manifest_path(root).exists() + assert stat.S_IMODE((root / "a" / "SKILL.md").stat().st_mode) == 0o644 + + second = await write_skills([_skill("a")], root) + + assert second.ok is True, _error_messages(second) + assert not (root / "b" / "SKILL.md").exists() + assert "b/SKILL.md" not in _read_manifest(root)["entries"] + + async def test_a_symlinked_skill_directory_is_still_refused( + self, root: Path, tmp_path: Path + ) -> None: + """The fallback keeps the ``lstat`` floor: no writing through a link.""" + outside = tmp_path / "outside" + outside.mkdir() + (root / "a").symlink_to(outside, target_is_directory=True) + + report = await write_skills([_skill("a")], root) + + assert report.ok is False + assert list(outside.iterdir()) == [] + + +class TestNonRegularFiles: + """A managed path that is not a regular file is refused, never read.""" + + @pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="no FIFOs on this platform") + async def test_a_fifo_at_the_managed_path_does_not_block(self, root: Path) -> None: + """Reading a FIFO with no writer blocks forever. + + Same attacker capability the symlink checks defend against: swapping a + managed ``SKILL.md`` for a FIFO would otherwise hang the whole reconcile + — and the caller's event loop with it — well past any ``timeout``, since + the deadline is only consulted between steps. + """ + skill_dir = root / "a" + skill_dir.mkdir() + os.mkfifo(skill_dir / "SKILL.md") + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": {"a/SKILL.md": _entry("a", 1, SKILL_BODY)}, + }, + ) + + report = await write_skills([_skill("a")], root) + + assert report.ok is False + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert action.error is not None + assert "regular file" in action.error + assert stat.S_ISFIFO(os.lstat(skill_dir / "SKILL.md").st_mode) + + +class TestClobberProtection: + """Destructive ops only on manifest-listed paths.""" + + async def test_unmanaged_file_is_never_overwritten(self, root: Path) -> None: + target = root / "a" / "SKILL.md" + target.parent.mkdir() + target.write_text("user authored\n", encoding="utf-8") + + report = await write_skills([_skill("a")], root) + + assert report.ok is False + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert action.error is not None + assert target.read_text(encoding="utf-8") == "user authored\n" + + async def test_unmanaged_file_is_never_deleted(self, root: Path) -> None: + target = root / "a" / "SKILL.md" + target.parent.mkdir() + target.write_text("user authored\n", encoding="utf-8") + + await write_skills([], root) + + assert target.read_text(encoding="utf-8") == "user authored\n" + + async def test_manifest_entry_with_mismatched_key_does_not_authorize( + self, root: Path + ) -> None: + target = root / "a" / "SKILL.md" + target.parent.mkdir() + target.write_text("user authored\n", encoding="utf-8") + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": { + "a/SKILL.md": _entry("different-key", 1, "user authored\n") + }, + }, + ) + + report = await write_skills([_skill("a")], root) + + assert report.ok is False + assert _actions_by_key(report)["a"].action == "error" + assert target.read_text(encoding="utf-8") == "user authored\n" + + +DIVERGENT_CONTENT = "existing content\n" + + +def _live_entries() -> dict[str, Any]: + """A parseable entries map that really does claim ``a/SKILL.md`` as managed.""" + return {"a/SKILL.md": _entry("a", 1, DIVERGENT_CONTENT)} + + +# The first six variants are unparseable: ``entries`` is missing, the wrong type, +# or the whole document is garbage. That makes "performed no destructive action" +# arithmetic rather than a defense — with no entries to act on, a file at a +# managed path is protected by clobber protection and there is nothing to prune, +# so those cases pass against an implementation that simply treats a corrupt +# manifest as an empty one. +# +# The ``*_live_entries`` variants are the ones that actually test round-tripping: corrupt +# ONLY in ``manifestVersion``, with a valid entries map listing the managed path +# under a matching key. The implementation has everything it needs to overwrite +# and to prune, and must refuse anyway. +CORRUPT_MANIFESTS: list[tuple[str, Any]] = [ + ("garbage", "{not json at all"), + ("empty", ""), + ("wrong_types", {"manifestVersion": 1, "entries": ["a/SKILL.md"]}), + ("entries_missing", {"manifestVersion": 1}), + ("future_version", {"manifestVersion": 2, "entries": {}}), + ("version_not_int", {"manifestVersion": "1", "entries": {}}), + ("future_version_live_entries", {"manifestVersion": 2, "entries": _live_entries()}), + ( + "version_not_int_live_entries", + {"manifestVersion": "1", "entries": _live_entries()}, + ), +] + +LIVE_ENTRY_MANIFESTS: list[tuple[str, Any]] = [ + case for case in CORRUPT_MANIFESTS if case[0].endswith("_live_entries") +] + + +class TestCorruptManifest: + """Corrupt manifest fails closed, non-destructively.""" + + @pytest.mark.parametrize( + "raw", + [case[1] for case in CORRUPT_MANIFESTS], + ids=[case[0] for case in CORRUPT_MANIFESTS], + ) + async def test_no_destructive_action_and_error_reported( + self, root: Path, raw: Any + ) -> None: + target = root / "a" / "SKILL.md" + target.parent.mkdir() + target.write_text(DIVERGENT_CONTENT, encoding="utf-8") + _write_manifest(root, raw) + + report = await write_skills([_skill("a", 2, "new content\n")], root) + + assert report.ok is False + # The error must name the manifest. For the unparseable variants the file + # at the managed path is also unmanaged, so a bare "some error happened" + # assertion is satisfied by clobber protection alone and says nothing + # about whether the manifest state was detected at all. + errors = _error_messages(report) + assert any("manifest" in e.lower() for e in errors), errors + assert target.read_text(encoding="utf-8") == DIVERGENT_CONTENT + + async def test_run_level_error_carries_the_empty_key_sentinel( + self, root: Path + ) -> None: + """A run-level error has no skill key to hang off. + + The empty string is public API surface: a caller grouping the report by + key has to know the sentinel exists. Asserted here rather than in the + parametrized cases above so it is a statement about the manifest error + specifically, not about whichever error happens to come first. + """ + _write_manifest(root, "{not json at all") + + report = await write_skills([_skill("a")], root) + + manifest_errors = [ + action + for action in report.errors + if "manifest" in (action.error or "").lower() + ] + assert manifest_errors, _error_messages(report) + assert all(action.key == "" for action in manifest_errors) + # A per-skill error in the same report still carries its real key, so the + # sentinel is not simply "every error action has an empty key". + assert all( + action.key != "" + for action in report.errors + if action not in manifest_errors + ) + + @pytest.mark.parametrize( + "raw", + [case[1] for case in LIVE_ENTRY_MANIFESTS], + ids=[case[0] for case in LIVE_ENTRY_MANIFESTS], + ) + async def test_managed_file_is_not_pruned_when_only_the_version_is_corrupt( + self, root: Path, raw: Any + ) -> None: + """The prune counterpart of the live-entries cases. + + Here the implementation can read the entries map and knows exactly which + file it owns, so refusing to remove it is a real decision rather than an + absence of information. + """ + target = root / "a" / "SKILL.md" + target.parent.mkdir() + target.write_text(DIVERGENT_CONTENT, encoding="utf-8") + _write_manifest(root, raw) + + report = await write_skills([], root) + + assert report.ok is False + assert target.read_text(encoding="utf-8") == DIVERGENT_CONTENT + assert [a for a in report.actions if a.action == "removed"] == [] + errors = _error_messages(report) + assert any("manifest" in e.lower() for e in errors), errors + + async def test_nothing_is_pruned_under_a_corrupt_manifest(self, root: Path) -> None: + target = root / "a" / "SKILL.md" + target.parent.mkdir() + target.write_text(DIVERGENT_CONTENT, encoding="utf-8") + _write_manifest(root, "{not json at all") + + report = await write_skills([], root) + + assert report.ok is False + assert target.exists() + assert [a for a in report.actions if a.action == "removed"] == [] + errors = _error_messages(report) + assert any("manifest" in e.lower() for e in errors), errors + + async def test_brand_new_paths_may_still_be_written(self, root: Path) -> None: + _write_manifest(root, "{not json at all") + + report = await write_skills([_skill("fresh")], root) + + actions = _actions_by_key(report) + assert actions["fresh"].action == "written" + assert (root / "fresh" / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + + async def test_corrupt_manifest_file_is_not_destroyed(self, root: Path) -> None: + _write_manifest(root, "{not json at all") + + await write_skills([], root) + + assert _manifest_path(root).exists() + assert _manifest_path(root).read_text(encoding="utf-8") == "{not json at all" + + +class TestWriteSkillsTelemetry: + """Materialized / revoked signals from write_skills.""" + + async def test_materialized_signal_per_action( + self, root: Path, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + _place_managed(root, "same", SKILL_BODY) + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": { + "same/SKILL.md": _entry("same", 1, SKILL_BODY), + "stale/SKILL.md": _entry("stale", 1, "old\n"), + }, + }, + ) + (root / "stale").mkdir() + (root / "stale" / "SKILL.md").write_text("old\n", encoding="utf-8") + + await write_skills( + [_skill("same"), _skill("stale", 2, "fresh\n"), _skill("brand-new")], + root, + ) + + signals = recording_emitter.signals(MATERIALIZED_SIGNAL) + by_key = {s["skill_key"]: s for s in signals} + assert len(signals) == 3 + assert by_key["same"]["reconcile_action"] == "skipped_current" + assert by_key["stale"]["reconcile_action"] == "updated" + assert by_key["brand-new"]["reconcile_action"] == "written" + + async def test_materialized_signal_properties( + self, root: Path, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + + await write_skills([_skill("a")], root) + + props = recording_emitter.signals(MATERIALIZED_SIGNAL)[0] + assert props["skill_key"] == "a" + assert props["content_bytes"] == len(SKILL_BODY.encode("utf-8")) + assert props["content_hash"] == _hash(SKILL_BODY) + assert props["reconcile_action"] == "written" + assert props["language"] == "python" + + async def test_no_filesystem_paths_in_telemetry( + self, root: Path, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + + await write_skills([_skill("a")], root) + + for _signal, props in recording_emitter.records: + assert "target_path" not in props + for value in props.values(): + assert str(root) not in str(value) + + async def test_no_skill_body_in_telemetry( + self, root: Path, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + + await write_skills([_skill("a")], root) + + for _signal, props in recording_emitter.records: + for value in props.values(): + assert "Do the thing." not in str(value) + + async def test_revoked_signal_on_prune( + self, root: Path, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + _place_managed(root, "gone", SKILL_BODY, version=4) + + await write_skills([], root) + + revoked = recording_emitter.signals(REVOKED_SIGNAL) + assert len(revoked) == 1 + assert revoked[0]["skill_key"] == "gone" + assert revoked[0]["version"] == 4 + assert revoked[0]["removed_from_disk"] is True + assert revoked[0]["language"] == "python" + + async def test_revoked_signal_redacts_an_untrusted_manifest_version( + self, root: Path, recording_emitter: Any + ) -> None: + """The manifest is untrusted, so its version is shape-checked first. + + Anything with write access to the managed root can plant an arbitrary + string here; echoing it verbatim would publish attacker-controlled + content — a skill body, or PII — as a signal property. + """ + skills_module._set_emitter_for_testing(recording_emitter) + target = root / "gone" / "SKILL.md" + target.parent.mkdir(parents=True) + target.write_text(SKILL_BODY, encoding="utf-8") + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": { + "gone/SKILL.md": { + "key": "gone", + "version": "Do the thing. " * 8, + "sha256": _hash(SKILL_BODY), + } + }, + }, + ) + + await write_skills([], root) + + revoked = recording_emitter.signals(REVOKED_SIGNAL) + assert len(revoked) == 1 + assert "version" not in revoked[0] + assert revoked[0]["skill_key"] == "gone" + for value in revoked[0].values(): + assert "Do the thing." not in str(value) + + async def test_no_revoked_signal_when_prune_disabled( + self, root: Path, recording_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(recording_emitter) + _place_managed(root, "gone", SKILL_BODY) + + await write_skills([], root, prune=False) + + assert recording_emitter.signals(REVOKED_SIGNAL) == [] + + async def test_write_skills_records_no_signal_outside_the_approved_set( + self, root: Path, recording_emitter: Any + ) -> None: + """Allowlist sweep over a run that exercises all four actions. + + The accessor-side half of this sweep is + ``test_accessors_record_no_signal_outside_the_approved_set`` in + test_skills.py. Asserted over recorded strings, so no module-level + signal-name constant is required of the implementation. + """ + skills_module._set_emitter_for_testing(recording_emitter) + for key, content in (("same", SKILL_BODY), ("stale", "old\n"), ("gone", "g\n")): + (root / key).mkdir() + (root / key / "SKILL.md").write_text(content, encoding="utf-8") + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": { + "same/SKILL.md": _entry("same", 1, SKILL_BODY), + "stale/SKILL.md": _entry("stale", 1, "old\n"), + "gone/SKILL.md": _entry("gone", 1, "g\n"), + }, + }, + ) + + report = await write_skills( + [_skill("same"), _skill("stale", 2, "fresh\n"), _skill("brand-new")], + root, + ) + + # Positive control: the subset assertion is vacuous unless the run + # really did produce all four actions and record for them. + assert {a.action for a in report.actions} == { + "skipped_current", + "updated", + "written", + "removed", + } + recorded = {signal for signal, _props in recording_emitter.records} + assert recorded <= APPROVED_SIGNALS, ( + f"unapproved signal(s): {sorted(recorded - APPROVED_SIGNALS)}" + ) + assert not recorded & REMOVED_SIGNALS + assert recorded == {MATERIALIZED_SIGNAL, REVOKED_SIGNAL} + + async def test_no_ld_track_calls_from_write_skills( + self, root: Path, mock_ld_client: Any + ) -> None: + await init_client(client=mock_ld_client) + + await write_skills([_skill("a"), _skill("../evil")], root) + + mock_ld_client.track.assert_not_called() + + async def test_throwing_emitter_never_breaks_the_reconcile( + self, root: Path, throwing_emitter: Any + ) -> None: + skills_module._set_emitter_for_testing(throwing_emitter) + + report = await write_skills([_skill("a")], root) + + assert report.ok is True + assert (root / "a" / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + + async def test_integrity_signal_property_keys_match_across_layers( + self, root: Path, recording_emitter: Any + ) -> None: + """The same defect, caught at either layer, records + the same property keys. + + Verification runs twice by design: once at the accessor boundary and + again immediately before a write. The signal contract marks ``expected_hash`` + optional, so an implementation that populates it on one path and omits + it on the other passes every other assertion here while making the + signal's shape depend on which internal code path noticed. Oversize + content is the case reachable from both layers with the expected hash in + hand throughout. + """ + skills_module._set_emitter_for_testing(recording_emitter) + oversize = "x" * (64 * 1024 + 1) + content_hash = _hash(oversize) + + # Layer 1 — the accessor boundary. + store = InMemorySkillStore() + store.put( + { + "key": "big", + "version": 1, + "content": oversize, + "contentHash": content_hash, + } + ) + skills_module._set_store(store) + assert await get_skill("big") is None + + # Layer 2 — verify-then-write, on a directly constructed Skill. + report = await write_skills( + [Skill(key="big", version=1, content=oversize, content_hash=content_hash)], + root, + ) + assert report.ok is False + + failures = recording_emitter.signals(INTEGRITY_SIGNAL) + assert len(failures) == 2, failures + accessor_keys, write_keys = (set(props) for props in failures) + assert accessor_keys == write_keys, ( + f"accessor-only keys: {sorted(accessor_keys - write_keys)}; " + f"write-only keys: {sorted(write_keys - accessor_keys)}" + ) + assert "expected_hash" in accessor_keys diff --git a/pyproject.toml b/pyproject.toml index f8ae52e..f84713e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -23,6 +23,7 @@ dev = [ "ruff>=0.15.20", "pre-commit>=4.6.0", "brotlicffi>=1.0", + "pyyaml>=6", ] [tool.pytest.ini_options] diff --git a/uv.lock b/uv.lock index de575a7..69d50b9 100644 --- a/uv.lock +++ b/uv.lock @@ -38,6 +38,7 @@ dev = [ { name = "pytest-asyncio", specifier = ">=0.24" }, { name = "pytest-mock", specifier = ">=3" }, { name = "python-dotenv", specifier = ">=1.2.2" }, + { name = "pyyaml", specifier = ">=6" }, { name = "ruff", specifier = ">=0.15.20" }, ] @@ -790,7 +791,7 @@ wheels = [ [[package]] name = "launchdarkly-ai-claude-agents" -version = "0.1.1" +version = "0.1.4" source = { editable = "packages/claude-agents" } dependencies = [ { name = "anthropic" }, @@ -809,7 +810,7 @@ requires-dist = [ [[package]] name = "launchdarkly-ai-claude-messages" -version = "0.1.1" +version = "0.1.4" source = { editable = "packages/claude-messages" } dependencies = [ { name = "anthropic" }, @@ -826,7 +827,7 @@ requires-dist = [ [[package]] name = "launchdarkly-ai-langchain-agents" -version = "0.1.1" +version = "0.1.4" source = { editable = "packages/langchain-agents" } dependencies = [ { name = "langchain-core" }, @@ -845,7 +846,7 @@ requires-dist = [ [[package]] name = "launchdarkly-ai-langchain-messages" -version = "0.1.1" +version = "0.1.4" source = { editable = "packages/langchain-messages" } dependencies = [ { name = "langchain-core" }, @@ -862,7 +863,7 @@ requires-dist = [ [[package]] name = "launchdarkly-ai-openai-agents" -version = "0.1.1" +version = "0.1.4" source = { editable = "packages/openai-agents" } dependencies = [ { name = "launchdarkly-ai-server" }, @@ -881,7 +882,7 @@ requires-dist = [ [[package]] name = "launchdarkly-ai-openai-messages" -version = "0.1.1" +version = "0.1.4" source = { editable = "packages/openai-messages" } dependencies = [ { name = "launchdarkly-ai-server" }, @@ -898,7 +899,7 @@ requires-dist = [ [[package]] name = "launchdarkly-ai-python" -version = "0.1.1" +version = "0.1.3" source = { editable = "packages/ai" } dependencies = [ { name = "launchdarkly-ai-server" }, @@ -918,7 +919,7 @@ provides-extras = ["otel"] [[package]] name = "launchdarkly-ai-server" -version = "0.1.1" +version = "0.1.3" source = { editable = "packages/client" } dependencies = [ { name = "opentelemetry-api" },