diff --git a/packages/client/README.md b/packages/client/README.md index d9c1db1..711ce5b 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -357,6 +357,32 @@ never writes through a symlink; writes are atomic (temp file, `fsync`, rename) a `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. +**One exception, and it is what makes a crashed reconcile recoverable.** A file at a managed +path whose bytes are *already byte-identical* to the content LaunchDarkly resolved is +adopted — recorded in the manifest and reported `skipped_current` — rather than refused. +Without that, a process killed after a skill file lands but before the manifest is rewritten +leaves that file managed-but-unrecorded, which is indistinguishable from a file you wrote +yourself, so every later reconcile would refuse it and the skill would stay wedged until +someone intervened. Adoption cannot weaken the guarantee above, because bytes that differ in +any way are still refused and left untouched. Note that an adopted file becomes prunable +like any other managed file — which is the same outcome the crash pre-empted. + +**A few keys are legal to an AI Config but not to a filesystem.** A key becomes a single +directory name, so `write_skills` applies bounds of its own on top of the key grammar: no +mainstream filesystem allows a 256-byte path component, and Windows reserves 22 MS-DOS +device names (`con`, `prn`, `aux`, `nul`, `com1`–`com9`, `lpt1`–`lpt9`) that cannot be +directory names there. Either one is a reported `error` action for that skill, and the +rejection is unconditional rather than platform-gated — a managed root written from a Linux +container is routinely read from a Windows host, so the on-disk result must not depend on +which OS ran the write. The keys stay valid everywhere else: an AI Config referencing a skill +named `aux` parses, and its other fields are unaffected. If you have a skill named for a +device, rename it. + +**Total path length is yours to bound, not the SDK's.** The 255-byte bound above is per +*component*; the root is your path, so `` + `` + `/SKILL.md` can still exceed +Windows' 260-character `MAX_PATH` with a perfectly legal key. Choose a short managed root on +Windows. + | Export | Description | |---|---| | `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when absent. | diff --git a/packages/client/agents.md b/packages/client/agents.md index feb2e34..cd99b28 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -250,7 +250,26 @@ Store data is **untrusted input**; the transport is not part of the trust bounda - **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. + managed path with no matching manifest entry is reported as `error` and left alone — + *unless its bytes already are the resolved content*, in which case it is adopted (manifest + entry recorded, reported `skipped_current`). That single exception is what makes a + reconcile killed between the content writes and the final manifest rewrite recoverable + instead of permanently wedged, and it cannot be widened: the comparison is over the + verbatim bytes against the resolved `contentHash`, a read that fails is a refusal and + never an overwrite, and the read is bounded at `len(content) + 1` bytes so a file that + merely *begins* with the resolved content is refused too. Do not relax it to a prefix, a + length, an mtime, or the manifest's own recorded `sha256` — that field is untrusted and is + never a decision input. `skipped_current` is reused deliberately rather than adding an + `adopted` action kind; `ReconcileActionKind` is a public closed set. +- **Temp files are swept, within the same bounds as everything else.** `atomic_write` unlinks + its own temp file on any exception, but a `SIGKILL` leaves one behind that no manifest + entry records, and a non-empty directory defeats `_prune_one`'s `rmdir` — so one orphan + pins a skill directory forever. The sweep is the only place this SDK removes a file the + manifest does not list, and it is bounded on every axis: inside `//` only, for a + key that passes `_key_rejection_reason`; only names `safe_fs.is_temp_name` recognizes, + anchored at both ends and asked of `safe_fs` rather than re-spelled (a copy would drift + from the writer); only regular files, with the type read off the descriptor; unlinked + through the pinned descriptor. It never raises and never aborts a run. - **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 @@ -265,10 +284,28 @@ Store data is **untrusted input**; the transport is not part of the trust bounda "Descriptor-pinned filesystem access" below. Re-resolving `/` from its path at write or unlink time reopens a swap window that the checks above cannot cover. - **A key valid to the data model may still be unrepresentable on disk.** The model allows - 256 characters; `NAME_MAX` is 255 bytes. `write_skills` rejects an over-long key before - any filesystem call, and every per-skill filesystem failure is caught at the loop so it - becomes an `error` action — aborting the loop would skip the manifest rewrite and orphan - files already written in that run. + 256 characters; `NAME_MAX` is 255 bytes. Windows additionally reserves 22 MS-DOS device + names, none of which can be a directory name there: `con`, `prn`, `aux`, `nul`, + `com1`–`com9`, `lpt1`–`lpt9` (`com0` and `lpt0` are *not* reserved; do not add them). + `write_skills` rejects both 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. +- **Those two bounds live in `_key_rejection_reason`, not in the key grammar, and must not + move.** `is_valid_skill_key` / `skill_key_rejection_reason` keep admitting an over-long or + reserved key on purpose. `parse_ai_config` fails closed on a bad `skills` entry, so a + grammar-level rejection would invalidate the *entire* AI Config — model, provider, + instructions, tools — for a Linux customer over a Windows-only constraint; and it would + silently shrink `skill_refs`, which is what authorizes a prune, converting "this skill + fails to write on Windows" into "this skill gets deleted on Linux". `_key_rejection_reason` + is shared by the write and prune paths, so one edit covers both destructive paths. + The reserved-name check is unconditional rather than `os.name == "nt"`-gated: a root + written from a Linux container is routinely read from a Windows host, and neither + repository has a Windows CI runner (every matrix job is `ubuntu-latest`), so a gated branch + would be untestable — the exact condition that produced the gap. No suffix stripping and no + case folding are needed, because the grammar admits no `.` and no `$` (so `con.txt` and + `CONIN$` are unreachable) and is lowercase-only. The residual the SDK cannot check is total + path length: the 255-byte bound is per *component*, and the root belongs to the customer, + so `MAX_PATH` overflow is a README note rather than a check. - **A key is untrusted input everywhere it appears.** `skill_key_rejection_reason` is the single canonical explanation, so the config parser and the reference projection reject a key for the same stated reason — and so does every layer added later. A silently diff --git a/packages/client/src/launchdarkly_ai_server/safe_fs.py b/packages/client/src/launchdarkly_ai_server/safe_fs.py index 3605399..73eef87 100644 --- a/packages/client/src/launchdarkly_ai_server/safe_fs.py +++ b/packages/client/src/launchdarkly_ai_server/safe_fs.py @@ -17,6 +17,7 @@ import errno import os +import re import secrets import stat import tempfile @@ -191,6 +192,53 @@ def unlink_file(directory: Path, name: str, *, dir_fd: int | None) -> None: os.unlink(name, dir_fd=dir_fd) +_TEMP_SUFFIX = ".tmp" +"""Suffix on every temp file this module creates.""" + +_TEMP_TOKEN_BYTES = 8 +"""Bytes of randomness in a temp name, as ``secrets.token_hex`` takes them.""" + +_TEMP_TOKEN_PATTERN = re.compile( + # Two producers, one recognizer. The descriptor path below names its temp + # file with ``secrets.token_hex(_TEMP_TOKEN_BYTES)`` — twice that many + # lowercase hex characters. The fallback path hands naming to + # ``tempfile.mkstemp``, whose sequence is eight characters drawn from + # ``[a-z0-9_]``. Matched with ``fullmatch``, which anchors both branches at + # both ends, so nothing longer or otherwise-shaped is ever recognized. + rf"[0-9a-f]{{{_TEMP_TOKEN_BYTES * 2}}}|[a-z0-9_]{{8}}" +) + + +def temp_name_prefix(name: str) -> str: + """ + The prefix every temp file for *name* is created under. + + Spelled once because two callers need to agree on it: ``atomic_write`` + creates the name, and a caller sweeping orphaned temp files left by a crash + has to recognize it. A copy of the format string in the sweeper would be a + copy that can drift out of step with the writer. + """ + return f".{name}." + + +def is_temp_name(candidate: str, name: str) -> bool: + """ + Whether *candidate* is a name this module could have created for *name*. + + The recognizer for the orphan sweep: ``atomic_write`` unlinks its temp file + on any exception, but a ``SIGKILL`` between the create and the rename leaves + it behind, and nothing else on disk records that it exists. Deliberately + narrow — prefix, random token, and suffix must all match, with nothing + before or after — because the only thing a caller does with a ``True`` here + is delete the file. + """ + prefix = temp_name_prefix(name) + if not candidate.startswith(prefix) or not candidate.endswith(_TEMP_SUFFIX): + return False + token = candidate[len(prefix) : -len(_TEMP_SUFFIX)] + return _TEMP_TOKEN_PATTERN.fullmatch(token) is not None + + def _mkstemp_at(dir_fd: int, prefix: str) -> tuple[int, str]: """ ``tempfile.mkstemp`` for a directory descriptor. @@ -202,7 +250,7 @@ def _mkstemp_at(dir_fd: int, prefix: str) -> tuple[int, str]: """ 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" + name = f"{prefix}{secrets.token_hex(_TEMP_TOKEN_BYTES)}{_TEMP_SUFFIX}" try: return os.open(name, flags, 0o600, dir_fd=dir_fd), name except FileExistsError: @@ -234,7 +282,7 @@ def atomic_write( semantics on Windows). """ at_fd = dir_fd if dir_fd is not None and SUPPORTS_DIR_FD else None - prefix = f".{name}." + prefix = temp_name_prefix(name) target: str | Path if at_fd is not None: @@ -243,7 +291,7 @@ def atomic_write( 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") + fd, temp = tempfile.mkstemp(dir=directory, prefix=prefix, suffix=_TEMP_SUFFIX) target = directory / name try: diff --git a/packages/client/src/launchdarkly_ai_server/skills_fs.py b/packages/client/src/launchdarkly_ai_server/skills_fs.py index feb8fbc..8a865dd 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fs.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fs.py @@ -33,6 +33,7 @@ SymlinkRefused, atomic_write, atomic_write_in, + is_temp_name, pinned_directory, unlink_file, ) @@ -95,6 +96,27 @@ """ +_WINDOWS_RESERVED_NAMES = frozenset( + {"con", "prn", "aux", "nul"} + | {f"com{digit}" for digit in range(1, 10)} + | {f"lpt{digit}" for digit in range(1, 10)} +) +""" +The 22 MS-DOS device names Windows still reserves, which cannot be directory +names there. The key grammar admits every one of them, so a customer who names a +skill ``con`` gets a working reconcile on Linux and a broken one on Windows — +rejected here instead, on every platform, so the on-disk result never depends on +which OS ran the write. Neither repository has a Windows CI runner, which is the +condition that produced the gap in the first place. + +The bare names are the whole set: no suffix stripping is needed because the key +grammar admits no ``.``, so ``con.txt`` is unreachable, and ``CONIN$`` / +``CONOUT$`` are unreachable for want of a ``$``; no case folding is needed +because the grammar is lowercase-only. ``com0`` and ``lpt0`` are deliberately +absent — those are not reserved. +""" + + # ------------------------------------------------------------------------- # The reconcile entry point # ------------------------------------------------------------------------- @@ -644,6 +666,17 @@ def _key_rejection_reason(key: Any) -> str | None: f"skill key '{key[:32]}...' is {key_bytes} bytes, over the " f"{_MAX_PATH_COMPONENT_BYTES}-byte limit for a single directory name" ) + # Same reasoning as the byte bound above, and it lives at the same layer for + # the same reason: the grammar itself must keep admitting these, because + # rejecting them there would fail the whole AI Config over one skill, and + # would shrink ``skill_refs`` — which is what authorizes a prune, so a + # Windows-only constraint would delete the skill's file on Linux. + if key in _WINDOWS_RESERVED_NAMES: + return ( + f"skill key '{key}' is a name Windows reserves for a device and " + "cannot be a directory name there; it is rejected on every platform " + "so a managed root written on one OS is usable on the other" + ) return None @@ -683,24 +716,45 @@ def failed(message: str) -> ReconcileAction: ) encoded, content_hash = verified.encoded, verified.content_hash + # Sweep before writing rather than after, so a temp file this run is about + # to create can never be a candidate. + _sweep_orphan_temp_files(root, key) + # 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: + # Hash first, and decide from the bytes. The manifest check below is what + # protects a customer's own file, but it also refuses the file this SDK + # itself wrote and was killed before recording — the reconcile writes + # every skill and only then rewrites the manifest, so a crash in that + # window leaves a managed path with no entry, and every later reconcile + # takes the refusal branch forever. Comparing the bytes distinguishes the + # two cases without weakening anything: only content byte-identical to + # what LaunchDarkly resolved is ever adopted. try: - on_disk = _read_regular_file(target) + on_disk = _read_regular_file(target, max_bytes=len(encoded)) except OSError as exc: + if not managed: + # A read that failed proves nothing, and must never become an + # overwrite: it is the comparison below that would authorize one. + return failed( + f"'{relative}' exists, the manifest does not record it as " + f"managed under key '{key}', and it could not be read to " + f"compare against the resolved content: {exc}; refusing to " + "overwrite a file this SDK may not have written" + ) return failed(f"'{relative}' could not be read: {exc}") if hashlib.sha256(on_disk).hexdigest() == content_hash: + # ``skipped_current`` covers this deliberately, rather than a new + # action kind: its documented meaning is that the bytes on disk + # already are the resolved content, which is exactly as true for an + # adopted file as for one this SDK wrote and recorded. Adoption does + # add a manifest entry, so the file becomes prunable later — correct, + # because a prune then removes content LaunchDarkly delivered anyway. _update_entry(entries, relative, skill, content_hash) record_materialized(key, len(encoded), content_hash, "skipped_current") return ReconcileAction( @@ -709,6 +763,12 @@ def failed(message: str) -> ReconcileAction: version=skill.version, path=str(target), ) + + if 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" + ) # Stale version or local tampering — LD-resolved content wins. action: ReconcileActionKind = "updated" else: @@ -725,7 +785,7 @@ def failed(message: str) -> ReconcileAction: ) -def _read_regular_file(target: Path) -> bytes: +def _read_regular_file(target: Path, *, max_bytes: int) -> bytes: """ Reads *target*, refusing anything that is not a regular file. @@ -739,6 +799,12 @@ def _read_regular_file(target: Path) -> bytes: 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. + + Reads at most ``max_bytes + 1`` bytes. The only consumer compares a hash, and + anything longer than the resolved content cannot match it, so the one extra + byte is enough to prove inequality — which is what keeps a foreign file of + arbitrary size from being pulled into memory now that adoption reads files + the manifest does not list. """ flags = ( os.O_RDONLY @@ -751,15 +817,82 @@ def _read_regular_file(target: Path) -> bytes: 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) + remaining = max_bytes + 1 + while remaining > 0: + chunk = os.read(fd, min(remaining, 65536)) if not chunk: - return b"".join(chunks) + break chunks.append(chunk) + remaining -= len(chunk) + return b"".join(chunks) finally: os.close(fd) +def _sweep_orphan_temp_files(root: Path, key: str) -> None: + """ + Removes temp files a killed reconcile left behind under ``//``. + + ``atomic_write`` unlinks its own temp file on any exception, but a ``SIGKILL`` + between the create and the rename leaves one on disk, and nothing else + records that it exists: ``_prune`` walks manifest entries, and an orphan + never has one. The second-order effect is what makes this worth doing — + ``_prune_one``'s ``rmdir`` only succeeds on an empty directory, so a single + orphaned temp pins a skill's directory permanently. + + Bounded on every axis, because this is the one place the SDK removes a file + the manifest does not list: only inside a directory named by a key that + passes ``_key_rejection_reason``; only names ``safe_fs`` itself recognizes as + its own temp naming for ``SKILL.md``, anchored at both ends, and asked of + ``safe_fs`` rather than re-spelled here so the recognizer cannot drift from + the writer; only regular files; and every removal relative to a descriptor + pinned with ``O_NOFOLLOW``. It never raises and never aborts the run: the + reconcile itself has succeeded either way, so a sweep that cannot happen is + a warning. + """ + if _key_rejection_reason(key) is not None: + return + skill_dir = root / key + if not skill_dir.is_dir(): + return + + try: + with pinned_directory(skill_dir) as dir_fd: + # Listing by path is safe even though the removals are + # descriptor-relative: a name reaches the unlink only if it matches + # the anchored temp pattern, and the unlink resolves it inside the + # pinned directory, so a listing redirected between the pin and here + # can at worst name a file that is not in it. + for name in sorted(os.listdir(skill_dir)): + if is_temp_name(name, SKILL_FILENAME): + _remove_orphan_temp_file(skill_dir, name, dir_fd) + except (OSError, ValueError) as exc: + logger.warning( + "orphaned temp files under skill '%s' could not be swept: %s", key, exc + ) + + +def _remove_orphan_temp_file(skill_dir: Path, name: str, dir_fd: int | None) -> None: + """ + Removes one recognized orphan. A per-file failure warns and moves on. + + The type check is what keeps the temp naming from being a way to have this + SDK delete something it did not write: a symlink or a FIFO wearing that name + is not a file ``atomic_write`` left behind, so it is not this function's to + remove. It is read off the descriptor, not the path, wherever there is one. + """ + try: + if dir_fd is not None: + mode = os.stat(name, dir_fd=dir_fd, follow_symlinks=False).st_mode + else: + mode = os.lstat(skill_dir / name).st_mode + if not stat.S_ISREG(mode): + return + unlink_file(skill_dir, name, dir_fd=dir_fd) + except (OSError, ValueError) as exc: + logger.warning("an orphaned temp file could not be removed: %s", exc) + + def _write_through_descriptor( skill_dir: Path, encoded: bytes, key: str, relative: str ) -> str | None: @@ -929,6 +1062,10 @@ def _prune_one( if unsafe is not None: return _prune_error(key, f"'{relative}' was not removed: {unsafe}", version) + # Before the removal, so the ``rmdir`` below is not defeated by an orphaned + # temp file that nothing else on disk records. + _sweep_orphan_temp_files(root, key) + removed_from_disk = False if target.exists(): failure = _unlink_through_descriptor(skill_dir, relative) diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 5af4d19..223a326 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -26,8 +26,11 @@ SkillReference, get_skill, init_client, + parse_ai_config, + skill_refs, write_skills, ) +from launchdarkly_ai_server.types_validation import is_valid_skill_key MANIFEST_NAME = ".launchdarkly-skills.json" SKILL_BODY = "---\nname: Test Skill\n---\nDo the thing.\n" @@ -1640,3 +1643,388 @@ async def test_integrity_signal_property_keys_match_across_layers( f"write-only keys: {sorted(write_keys - accessor_keys)}" ) assert "expected_hash" in accessor_keys + + +# --------------------------------------------------------------------------- +# Self-healing partial reconciles +# --------------------------------------------------------------------------- + + +def _place_unmanaged(root: Path, key: str, content: str) -> Path: + """A file at a managed path with **no** manifest entry. + + Exactly the state a reconcile killed between the content writes and the + final manifest rewrite leaves behind — and, indistinguishably on disk, the + state a customer authoring their own file there creates. Which is why the + bytes are the only thing that may decide between them. + """ + target = root / key / "SKILL.md" + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + return target + + +class TestCrashMidReconcileRecovery: + """A crash between the writes and the manifest rewrite must not wedge a skill.""" + + async def test_byte_identical_unmanaged_file_is_adopted(self, root: Path) -> None: + """The whole point: the second reconcile repairs the first one's crash. + + Without adoption every later reconcile takes the clobber-refusal branch + forever, because the file is at a managed path with no manifest entry. + """ + target = _place_unmanaged(root, "a", SKILL_BODY) + + first = await write_skills([_skill("a")], root) + + assert first.ok is True, _error_messages(first) + action = _actions_by_key(first)["a"] + assert action.action == "skipped_current" + assert action.version == 1 + assert action.path == str(target) + # Adopted, not rewritten, and now recorded. + assert target.read_text(encoding="utf-8") == SKILL_BODY + entry = _read_manifest(root)["entries"]["a/SKILL.md"] + assert entry["key"] == "a" + assert entry["version"] == 1 + assert entry["sha256"] == _hash(SKILL_BODY) + + # And the run after it is an ordinary no-op, through the managed path. + second = await write_skills([_skill("a")], root) + assert second.ok is True, _error_messages(second) + assert _actions_by_key(second)["a"].action == "skipped_current" + + async def test_adoption_writes_nothing( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Adoption is a manifest edit, not a write. Nothing touches the bytes.""" + _place_unmanaged(root, "a", SKILL_BODY) + spy = _ReplaceSpy().install(monkeypatch) + + report = await write_skills([_skill("a")], root) + + assert report.ok is True, _error_messages(report) + assert spy.calls == [] + + async def test_adoption_reports_skipped_current_not_a_new_action_kind( + self, root: Path, recording_emitter: Any + ) -> None: + """``skipped_current`` is reused deliberately — no ``adopted`` kind exists.""" + skills_module._set_emitter_for_testing(recording_emitter) + _place_unmanaged(root, "a", SKILL_BODY) + + report = await write_skills([_skill("a")], root) + + assert {a.action for a in report.actions} == {"skipped_current"} + props = recording_emitter.signals(MATERIALIZED_SIGNAL)[0] + assert props["reconcile_action"] == "skipped_current" + assert props["skill_key"] == "a" + + async def test_an_adopted_file_is_prunable_afterwards(self, root: Path) -> None: + """The documented caveat, pinned. + + Adoption records a manifest entry, so a later reconcile may prune the + file. That is correct rather than a weakening: only content byte-identical + to what LaunchDarkly resolved is ever adopted, so the prune removes + content LaunchDarkly delivered — exactly what would have happened had the + crash never occurred. + """ + target = _place_unmanaged(root, "a", SKILL_BODY) + await write_skills([_skill("a")], root) + + report = await write_skills([], root) + + assert report.ok is True, _error_messages(report) + assert _actions_by_key(report)["a"].action == "removed" + assert not target.exists() + + async def test_differing_unmanaged_content_is_still_refused( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The clobber guarantee, restated against the adoption rule. + + Adoption compares bytes, so anything that is not byte-identical to the + resolved content falls through to the same refusal as before. + """ + target = _place_unmanaged(root, "a", "user authored\n") + spy = _ReplaceSpy().install(monkeypatch) + + 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 "did not write" in action.error + assert target.read_text(encoding="utf-8") == "user authored\n" + assert spy.calls == [] + + async def test_a_longer_file_sharing_the_content_prefix_is_not_adopted( + self, root: Path + ) -> None: + """The read is bounded at ``len(content) + 1``, and that one byte matters. + + A bound of exactly ``len(content)`` would make every file that merely + *begins* with the resolved content hash as current, adopting — and later + pruning — a customer file with the skill body at its head. + """ + longer = SKILL_BODY + "and my own notes below\n" + target = _place_unmanaged(root, "a", longer) + + 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") == longer + + @pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="no FIFOs on this platform") + async def test_an_unmanaged_fifo_is_refused_and_never_read( + self, root: Path + ) -> None: + """Adoption widened the read to foreign files, so this refusal is load-bearing. + + Opening a FIFO with no writer blocks forever; the descriptor-pinned read + opens ``O_NONBLOCK`` and rejects anything that is not a regular file, so + this returns rather than hanging the reconcile and the event loop with it. + """ + skill_dir = root / "a" + skill_dir.mkdir() + os.mkfifo(skill_dir / "SKILL.md") + + 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) + + async def test_a_read_failure_on_an_unmanaged_file_refuses( + self, root: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A failed read proves nothing, so it must never become an overwrite. + + The adoption comparison is what would otherwise authorize the write, and + a file whose bytes could not be read has not been shown to be ours. + """ + target = _place_unmanaged(root, "a", SKILL_BODY) + spy = _ReplaceSpy().install(monkeypatch) + real_open = os.open + + def refuse_the_target(path: Any, *args: Any, **kwargs: Any) -> int: + if isinstance(path, (str, os.PathLike)) and os.fspath(path) == str(target): + raise PermissionError(13, "Permission denied") + return real_open(path, *args, **kwargs) + + monkeypatch.setattr(skills_fs_module.os, "open", refuse_the_target) + + 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 + # Distinguishable from the byte-mismatch refusal: this one says the + # comparison could not be made at all. + assert "could not be read to compare" in action.error + assert spy.calls == [] + assert target.read_text(encoding="utf-8") == SKILL_BODY + assert "a/SKILL.md" not in _read_manifest(root)["entries"] + + +# --------------------------------------------------------------------------- +# Orphaned temp files +# --------------------------------------------------------------------------- + + +def _temp_name(token: str = "0123456789abcdef") -> str: + """A name ``atomic_write`` could have created for ``SKILL.md``. + + The prefix comes from ``safe_fs`` itself rather than a copy of its format + string, so a change to the naming breaks this helper instead of silently + making the sweep a no-op. + """ + return f"{safe_fs_module.temp_name_prefix('SKILL.md')}{token}.tmp" + + +class TestOrphanedTempFiles: + """A ``SIGKILL`` mid-write leaves a temp file nothing else records.""" + + async def test_an_orphan_is_swept_on_the_next_write(self, root: Path) -> None: + _place_managed(root, "a", SKILL_BODY) + orphan = root / "a" / _temp_name() + orphan.write_text("half-written body", encoding="utf-8") + + report = await write_skills([_skill("a")], root) + + assert report.ok is True, _error_messages(report) + assert not orphan.exists() + assert (root / "a" / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + + async def test_an_orphan_no_longer_blocks_directory_cleanup( + self, root: Path + ) -> None: + """The second-order effect: ``rmdir`` fails on a non-empty directory. + + One orphaned temp file would otherwise pin the skill's directory under + the managed root forever, long after the skill itself was revoked. + """ + _place_managed(root, "a", SKILL_BODY) + orphan = root / "a" / _temp_name() + orphan.write_text("half-written body", encoding="utf-8") + + report = await write_skills([], root) + + assert report.ok is True, _error_messages(report) + assert _actions_by_key(report)["a"].action == "removed" + assert not (root / "a").exists() + + @pytest.mark.parametrize( + "innocent", + [ + "notes.tmp", + "SKILL.md.tmp", + ".SKILL.md.tmp", + ".SKILL.md..tmp", + _temp_name("not-a-token"), + _temp_name("0123456789abcdef") + ".bak", + "x" + _temp_name(), + _temp_name("0123456789abcdefff"), + ], + ) + async def test_a_lookalike_name_is_left_alone( + self, root: Path, innocent: str + ) -> None: + """The recognizer is anchored at both ends, and the sweep deletes files. + + Anything that is not exactly the naming ``safe_fs`` produces belongs to + the customer, whatever it resembles. + """ + _place_managed(root, "a", SKILL_BODY) + bystander = root / "a" / innocent + bystander.write_text("mine\n", encoding="utf-8") + + report = await write_skills([_skill("a")], root) + + assert report.ok is True, _error_messages(report) + assert bystander.read_text(encoding="utf-8") == "mine\n" + + @pytest.mark.skipif( + not hasattr(os, "symlink"), reason="platform has no symlink support" + ) + async def test_a_symlink_wearing_the_temp_name_is_not_removed( + self, root: Path, tmp_path: Path + ) -> None: + """The temp naming must not become a way to have the SDK delete elsewhere. + + Only a regular file is ever swept, and the type comes off the descriptor + rather than a followed path. + """ + outside = tmp_path / "precious.txt" + outside.write_text("do not delete\n", encoding="utf-8") + _place_managed(root, "a", SKILL_BODY) + link = root / "a" / _temp_name() + link.symlink_to(outside) + + report = await write_skills([_skill("a")], root) + + assert report.ok is True, _error_messages(report) + assert outside.read_text(encoding="utf-8") == "do not delete\n" + assert link.is_symlink() + + +# --------------------------------------------------------------------------- +# Windows reserved device names +# --------------------------------------------------------------------------- + +# Spelled out independently of the implementation's own set, so a name dropped +# from that set fails here rather than agreeing with itself. +WINDOWS_RESERVED_KEYS = ( + ["con", "prn", "aux", "nul"] + + [f"com{digit}" for digit in range(1, 10)] + + [f"lpt{digit}" for digit in range(1, 10)] +) + + +class TestWindowsReservedNames: + """Keys Windows cannot hold as directory names, refused on every platform.""" + + def test_the_set_is_exactly_twenty_two_names(self) -> None: + assert len(WINDOWS_RESERVED_KEYS) == len(set(WINDOWS_RESERVED_KEYS)) == 22 + + @pytest.mark.parametrize("reserved", WINDOWS_RESERVED_KEYS) + async def test_a_reserved_key_is_refused_by_write_skills( + self, root: Path, reserved: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + spy = _ReplaceSpy().install(monkeypatch) + + report = await write_skills([_skill(reserved)], root) + + assert report.ok is False + action = _actions_by_key(report)[reserved] + assert action.action == "error" + assert action.error is not None + assert "Windows reserves" in action.error + assert reserved in action.error + # Rejected before any filesystem call, not by the OS. + assert spy.calls == [] + assert not (root / reserved).exists() + + @pytest.mark.parametrize("reserved", WINDOWS_RESERVED_KEYS) + async def test_a_reserved_key_is_refused_by_the_prune_path( + self, root: Path, reserved: str + ) -> None: + """``_key_rejection_reason`` gates both destructive paths, so both refuse. + + A manifest naming a reserved key is left in place rather than acted on: + the same key check that stops the write stops the delete. + """ + target = _place_managed(root, reserved, SKILL_BODY) + + report = await write_skills([], root) + + assert report.ok is False + action = _actions_by_key(report)[reserved] + assert action.action == "error" + assert action.error is not None + assert "left in place" in action.error + assert target.read_text(encoding="utf-8") == SKILL_BODY + + @pytest.mark.parametrize("not_reserved", ["com0", "lpt0", "con1", "nul2", "conx"]) + async def test_neighbouring_names_are_not_reserved( + self, root: Path, not_reserved: str + ) -> None: + """``com0`` and ``lpt0`` are not device names, and must still write.""" + report = await write_skills([_skill(not_reserved)], root) + + assert report.ok is True, _error_messages(report) + assert (root / not_reserved / "SKILL.md").exists() + + @pytest.mark.parametrize("reserved", WINDOWS_RESERVED_KEYS) + def test_a_reserved_name_is_still_a_valid_key_to_every_pure_layer( + self, reserved: str + ) -> None: + """The layer choice, asserted — this is the whole point of it. + + The constraint lives in the filesystem layer and must not migrate into + the key grammar. At the grammar level a rejection would fail the *entire* + AI Config — model, provider, instructions, tools — for a Linux customer + over a Windows-only constraint, and would shrink ``skill_refs``, which is + what authorizes a prune: "this skill fails to write on Windows" would + become "this skill gets deleted on Linux". + """ + assert is_valid_skill_key(reserved) is True + + parsed = parse_ai_config( + { + "model": {"name": "claude-3"}, + "provider": {"name": "Anthropic"}, + "instructions": "You are helpful.", + "skills": [{"key": reserved, "version": 1}], + } + ) + assert parsed.success is True + + refs = skill_refs(parsed.data) + assert [ref.key for ref in refs] == [reserved]