From d6b0d77188c822caf8f6d5c4a4eab9d85fc2d1b5 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Wed, 30 Sep 2026 16:30:58 -0400 Subject: [PATCH] test(client): pin the adoption comparison to the same content-hash digests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TestContentHashContract` covers `verified_bytes`, which the accessor and the pre-write re-verify share. It does not reach the other place the rule is written down: adoption hashes the bytes it reads off disk with an expression of its own and compares that to `content_hash`. That one decides whether an unmanaged file is claimed or refused, so a comparison that normalized would adopt a planted file whose bytes merely *normalize* to the delivered content, leave those foreign bytes in place, and report `skipped_current` — an action documented to mean the opposite. Pins it against the same literal digest, with the same three alterations asserted to be refused rather than adopted. The fixture writes bytes rather than `write_text`, whose text mode translates `\n` to `os.linesep`: a no-op on POSIX, but on Windows it would rewrite the CRLF these cases are about before the test ran. Also documents the three fixture `_hash` helpers. Each is the expression the implementation uses, which is what makes it useless as an oracle, and the docstring says so and names where the rule is actually pinned. Matches the TypeScript SDK, which carries the same two suites, so neither implementation is the only one holding the cross-service rule. Verified by mutation on the adoption comparison: normalizing line endings and appending a final newline each refuse the exact match that should adopt; NFC-normalizing adopts the NFD-planted file as `skipped_current`, which only the refusal cases catch. Co-Authored-By: Claude Opus 5 --- packages/client/tests/test_skills.py | 10 +- packages/client/tests/test_skills_fdv2.py | 8 ++ packages/client/tests/test_skills_fs.py | 140 ++++++++++++++++++++++ 3 files changed, 157 insertions(+), 1 deletion(-) diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index fa720992..acee37ae 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -60,7 +60,15 @@ def _hash(content: str) -> str: - """sha256, lowercase hex, over verbatim utf-8 bytes.""" + """Convenience for building fixtures whose ``content_hash`` is correct. + + Deliberately the same expression the implementation hashes with, which is + what makes it useless as an oracle: a change to the hashing rule moves + every fixture built here along with it, and nothing in this file would + fail. The rule is pinned independently, against the digests LaunchDarkly's + delivery service computes, in ``TestContentHashContract`` below — whose literal digests + must never be replaced by a call to this helper. + """ return hashlib.sha256(content.encode("utf-8")).hexdigest() diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index b9caf25e..f5df109b 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -80,6 +80,14 @@ def _hash(content: str) -> str: + """Convenience for building fixtures whose ``content_hash`` is correct. + + Deliberately the same expression the implementation hashes with, which is + what makes it useless as an oracle: a change to the hashing rule moves + every fixture built here along with it, and nothing in this file would + fail. The rule is pinned independently, against the digests LaunchDarkly's + delivery service computes, in ``TestContentHashContract`` in ``test_skills.py``. + """ return hashlib.sha256(content.encode("utf-8")).hexdigest() diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 443f9abd..2c54ba21 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -12,6 +12,7 @@ import json import os import stat +import unicodedata from pathlib import Path from typing import Any, NamedTuple @@ -367,6 +368,17 @@ def root(tmp_path: Path) -> Path: def _hash(content: str) -> str: + """Convenience for building fixtures whose ``content_hash`` is correct. + + Deliberately the same expression the implementation hashes with, which is + what makes it useless as an oracle: a change to the hashing rule moves + every fixture built here along with it, and nothing in this file would + fail. The rule is pinned independently, against the digests LaunchDarkly's + delivery service computes, in ``TestContentHashContract`` in ``test_skills.py`` for the + accessor and write paths, and in + ``TestAdoptionContentHashContract`` below for the adoption + comparison, which hashes through an expression of its own. + """ return hashlib.sha256(content.encode("utf-8")).hexdigest() @@ -3293,6 +3305,134 @@ async def test_a_byte_identical_file_under_a_mismatched_entry_is_re_keyed( assert [e["key"] for e in entries.values()] == ["a"] +# --------------------------------------------------------------------------- +# The adoption comparison hashes the delivered rule +# --------------------------------------------------------------------------- + +# The same vector and digest pinned in ``test_skills.py``, deliberately copied +# rather than imported: the whole value of a literal is that it is not derived +# from anything. Both copies are the digest LaunchDarkly's delivery service +# computes for this content, so neither may be replaced by a call to ``_hash``, +# and the two only ever change together — and only if the service's rule changed. +# +# ``é`` written as an escape rather than as the character, so a tool that +# re-normalized this source file could not quietly change the input. +_ADOPTION_HAZARD_CONTENT = "# héllo\r\n\ntrailing no newline" +_ADOPTION_HAZARD_DIGEST = ( + "2b7c050d94135e5e947263053ebde1c988c2bb90bcc037e8f3d1a2d340b9a558" +) + +# The same three fixups the accessor-side contract test applies, here against +# the file on disk: an editor rewriting line endings, a lint step adding the +# final newline, a form decomposing the accent. None changes what the document +# means and all three change its bytes, so none of them is the content +# LaunchDarkly delivered. +_ADOPTION_NORMALIZATIONS = [ + pytest.param(lambda text: text.replace("\r\n", "\n"), id="crlf_to_lf"), + pytest.param(lambda text: text + "\n", id="trailing_newline_added"), + pytest.param( + lambda text: unicodedata.normalize("NFD", text), id="nfd_decomposition" + ), +] + + +def _plant_unmanaged(root: Path, key: str, content: str) -> Path: + """``_place_unmanaged``, but writing verbatim bytes. + + ``write_text`` opens in text mode, where ``newline=None`` translates ``\\n`` + to ``os.linesep``. That is a no-op on POSIX and on Windows it would rewrite + the CRLF these cases are about, turning the planted file into something + other than what the test meant to plant. Adoption decides from bytes, so the + fixture places bytes. + """ + target = root / key / "SKILL.md" + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(content.encode("utf-8")) + return target + + +class TestAdoptionContentHashContract: + """ + The verbatim-bytes rule where adoption writes it down a second time. + + ``TestContentHashContract`` in ``test_skills.py`` pins ``verified_bytes``, + which the accessor and the pre-write re-verify share. Adoption does not go + through it: it hashes the bytes it read off disk with an expression of its + own and compares that to ``content_hash``, so it is a second statement of + the same rule and the accessor-side test cannot reach it. + + It is also the statement that decides whether an unmanaged file is claimed + or refused. A comparison that normalized would adopt a planted file whose + bytes merely *normalize* to the delivered content, leave those foreign bytes + on disk, and report ``skipped_current`` — an action documented to mean the + bytes on disk already are the resolved content. + """ + + @staticmethod + def _pinned() -> Skill: + """A skill carrying the pinned digest as a literal. + + ``write_skills`` re-verifies through ``verified_bytes`` before it looks + at the disk at all, so this pins the write path's copy of the rule too: + a skill built this way only survives as far as the adoption comparison + while the digest still matches its content's exact bytes. + """ + return Skill( + key="a", + version=1, + content=_ADOPTION_HAZARD_CONTENT.encode("utf-8"), + content_hash=_ADOPTION_HAZARD_DIGEST, + ) + + async def test_a_file_matching_the_pinned_digest_is_adopted( + self, root: Path + ) -> None: + """The positive control, and the half that fails if either side drifts. + + The bytes on disk are byte-for-byte what the pinned digest is the digest + of, which is the one file adoption is for. + """ + target = _plant_unmanaged(root, "a", _ADOPTION_HAZARD_CONTENT) + + report = await write_skills([self._pinned()], root) + + assert report.ok is True, _error_messages(report) + assert _actions_by_key(report)["a"].action == "skipped_current" + # Adopted, not rewritten, and now recorded. + assert target.read_bytes() == _ADOPTION_HAZARD_CONTENT.encode("utf-8") + entry = _read_manifest(root)["entries"]["a/SKILL.md"] + assert entry["key"] == "a" + assert entry["sha256"] == _ADOPTION_HAZARD_DIGEST + + @pytest.mark.parametrize("normalize", _ADOPTION_NORMALIZATIONS) + async def test_a_normalized_file_is_refused_rather_than_adopted( + self, root: Path, normalize: Any + ) -> None: + """The teeth. Adoption is only safe while disagreeing is detectable. + + Each of these is content LaunchDarkly never delivered, so adopting one + would claim a foreign file into the manifest and report it as current. + """ + planted = normalize(_ADOPTION_HAZARD_CONTENT) + assert planted != _ADOPTION_HAZARD_CONTENT, ( + "the transform left this input alone" + ) + target = _plant_unmanaged(root, "a", planted) + + report = await write_skills([self._pinned()], root) + + assert report.ok is False + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert "refusing to overwrite a file this SDK did not write" in ( + action.error or "" + ) + # Neither adopted nor overwritten: the planted bytes are still the + # planted bytes, and nothing was claimed into the manifest. + assert target.read_bytes() == planted.encode("utf-8") + assert "a/SKILL.md" not in _read_manifest(root)["entries"] + + # --------------------------------------------------------------------------- # Orphaned temp files # ---------------------------------------------------------------------------