diff --git a/docs/api.md b/docs/api.md index cf3afd5..efe67f0 100644 --- a/docs/api.md +++ b/docs/api.md @@ -219,7 +219,7 @@ Python and the JS library use the same rules: - The decision is the leading token after whitespace, one `` block, one code fence, markdown `**` / `__` / backticks, straight or curly quotes, and an optional `Answer:` or `Decision:` prefix. That token must be exactly `yes` or `no`, then the end of the string, whitespace, or one of `. , ! : ; )`. A quoted or bold decision followed by a reason (`**no** unrelated`, `"no" unrelated`, `“no” — unrelated`) is that decision. Anything else is malformed: `failed_open=true`, the file is included, and the decision is not cached. - A missing `choices` list, an empty list, a missing message or content field, non-string message content (such as `["no"]`), and a non-JSON body are failed open inside that same handling and are not cached. - A cache read error is a miss: the model is called normally and the run does not abort. A cache write error is logged and does not abort the run. When the OpenAI client rejects `reasoning_effort`, the retry omits it and the decision is cached under the empty effort. -- Valid UTF-8 bytes are decoded to text, including a leading U+FEFF BOM, and are not replaced with an empty string. The 8192-byte probe tolerates only an incomplete trailing sequence of at most 3 bytes. Invalid UTF-8 anywhere else in the payload, including after that window, takes the binary path. A NUL is binary. A non-empty buffer that decodes to empty text is binary. `on_binary=include` returns the original bytes unchanged. +- Valid UTF-8 bytes are decoded to text, including a leading U+FEFF BOM, and are not replaced with an empty string. An incomplete trailing sequence of at most 3 bytes keeps the valid prefix at any length (`b"a\xc3"` decodes to `"a"`). Any other invalid UTF-8, including a bad byte after the first 8192 bytes, takes the binary path. A NUL is binary. A non-empty buffer that decodes to empty text is binary. `on_binary=include` returns the original bytes unchanged. Ignored paths are excluded before those bytes are decoded. - Token truncation keeps the first `maxTokens * 4` Unicode code points and never splits a code point or a surrogate pair. The byte cap cuts on a code-point boundary in UTF-8 and never emits U+FFFD. The shared fixture is `tests/fixtures/parity/relevance.json`. - The token estimate is `ceil(code points / 4)`. Empty text is 0. tiktoken is not used for this check. - Skip globs are case-insensitive. `**` crosses directories and `*` does not. `skipped_reason` for a skip is `ignored`. The list includes `**/*.min.js` and `**/*.min.mjs`. diff --git a/docs/development/testing.md b/docs/development/testing.md index 7aa40d0..3c62779 100644 --- a/docs/development/testing.md +++ b/docs/development/testing.md @@ -43,7 +43,7 @@ assert "Updated" in updated["file.py"] ## Relevance filter -`tests/test_relevance.py` mocks the LLM. It covers the shared 1024-token / `reasoning_effort=low` request, the length-prefixed cache key (mode, note, and the effort actually sent, including a legacy SDK that rejects the parameter), the leading-token parser (a quoted decision followed by a reason), non-string message content failed open, malformed answers counted as failed open, a cache read treated as a miss, cache write errors, transient retries, invalid UTF-8 after the 8192-byte probe, a preserved leading BOM, and the canonical skip list (`ignored`, including `*.min.mjs`). `tests/fixtures/parity/relevance.json` is a byte-identical copy of the JS fixture and is checked against the library functions, including its sha256. `tests/test_relevance_live.py` is skipped unless `LIVE_LLM=1`, in which case it reads `GPTDIFF_LLM_API_KEY` and calls the configured endpoint. CI does not set `LIVE_LLM`, so it never makes that call. The benchmark in `bench/relevance/` uses the same gate. `tests/test_relevance_bench.py` checks that a single `generate_diff` failure is stored in the JSON report with the other pipeline's diff and applied files. +`tests/test_relevance.py` mocks the LLM. It covers the shared 1024-token / `reasoning_effort=low` request, the length-prefixed cache key (mode, note, and the effort actually sent, including a legacy SDK that rejects the parameter), the leading-token parser (a quoted decision followed by a reason), non-string message content failed open, malformed answers counted as failed open, a cache read treated as a miss, cache write errors, transient retries, invalid UTF-8 after the 8192-byte probe, a truncated UTF-8 tail that keeps its valid prefix, ignored binaries excluded before decode, a preserved leading BOM, and the canonical skip list (`ignored`, including `*.min.mjs`). `tests/fixtures/parity/relevance.json` is a byte-identical copy of the JS fixture and is checked against the library functions, including its sha256. `tests/test_relevance_live.py` is skipped unless `LIVE_LLM=1`, in which case it reads `GPTDIFF_LLM_API_KEY` and calls the configured endpoint. CI does not set `LIVE_LLM`, so it never makes that call. The benchmark in `bench/relevance/` uses the same gate. `tests/test_relevance_bench.py` checks that a single `generate_diff` failure is stored in the JSON report with the other pipeline's diff and applied files. ## Running the Test Suite diff --git a/patchling/relevance.py b/patchling/relevance.py index b433b69..fed7bb3 100644 --- a/patchling/relevance.py +++ b/patchling/relevance.py @@ -16,9 +16,11 @@ Anything else is malformed: the file is included, ``failed_open`` is set, and the decision is not cached. Non-string message content is that same failure. A cache read error is a miss and the model is called; a cache write error is -logged and does not abort the run. Invalid UTF-8 anywhere in a byte payload +logged and does not abort the run. A truncated UTF-8 tail keeps the valid +prefix (``b"a\\xc3"`` is ``"a"``). Any other invalid UTF-8 in a byte payload is binary. ``on_binary="include"`` returns those original bytes unchanged. -A leading UTF-8 BOM (U+FEFF) stays in decoded text. +A leading UTF-8 BOM (U+FEFF) stays in decoded text. Ignored paths are +excluded before their bytes are decoded. """ from __future__ import annotations @@ -631,11 +633,12 @@ def _utf8_text_or_none(sample: bytes) -> Optional[str]: def is_binary_content(content) -> bool: """True when the payload is not text. - The first 8192 bytes are decoded as UTF-8. An incomplete sequence of at - most 3 bytes at that probe boundary is still text. A NUL or any other - invalid byte inside the window is binary, and so is a non-empty window - that decodes to empty text. Bytes past the window are checked too: any - invalid UTF-8 there is binary. A leading BOM is text. + The whole payload is validated as UTF-8. An incomplete trailing sequence + of at most 3 bytes keeps the valid prefix and is still text, at any + length (``b"a\\xc3"`` is ``"a"``). A NUL or any other invalid byte is + binary, and so is a non-empty buffer that decodes to empty text. The + first 8192 bytes are also checked for control-character density. A + leading BOM is text. """ if isinstance(content, str): if "\x00" in content: @@ -661,11 +664,10 @@ def _bytes_are_binary(raw: bytes) -> bool: return True if len(raw) <= 8192: return False - # The probe may end inside a character. The rest of the file has to be - # well-formed UTF-8; an invalid byte after the window is not dropped. - try: - raw.decode("utf-8") - except UnicodeDecodeError: + # An incomplete trailing sequence of at most 3 bytes keeps the prefix. + # Any other invalid byte, including one past the probe window, is binary. + rest = _utf8_text_or_none(raw) + if rest is None or rest == "": return True return False @@ -694,15 +696,13 @@ def _coerce_text(content) -> str: def _decode_utf8_text(raw: bytes) -> Optional[str]: """Decode UTF-8, keeping a leading BOM (U+FEFF). - ``utf-8-sig`` is not used. A buffer of at most 8192 bytes may end on an - incomplete sequence; those trailing bytes are dropped. A longer buffer - must be entirely well-formed, so a bad byte after the probe is not cut off. + ``utf-8-sig`` is not used. An incomplete trailing sequence of at most + 3 bytes is dropped and the valid prefix is kept, at any length. Any + other invalid byte returns ``None`` so the caller takes the binary path. """ try: return raw.decode("utf-8") except UnicodeDecodeError: - if len(raw) > 8192: - return None return _utf8_text_or_none(raw) @@ -710,9 +710,10 @@ def _prepare_content(content) -> tuple[bool, str]: """Return ``(is_binary, text)``. Valid UTF-8 bytes are decoded to text, including a leading U+FEFF, and - are never replaced with an empty string. Invalid UTF-8 anywhere, or a - NUL byte, takes the binary path. Callers that include a binary file pass - the original bytes through instead of this text. + are never replaced with an empty string. A truncated trailing sequence + keeps the valid prefix. Any other invalid UTF-8, or a NUL byte, takes + the binary path. Callers that include a binary file pass the original + bytes through instead of this text. """ if isinstance(content, str): return is_binary_content(content), content @@ -977,8 +978,9 @@ def filter_relevant( the empty effort. Binary files and files over ``max_bytes`` / ``max_tokens`` are not sent in - full. Invalid UTF-8 after the first 8192 bytes is still binary. - ``on_binary="include"`` keeps the original bytes. ``on_binary`` and + full. A truncated UTF-8 tail keeps the valid prefix. Any other invalid + UTF-8 is binary. Ignored paths are excluded before their bytes are + decoded. ``on_binary="include"`` keeps the original bytes. ``on_binary`` and ``on_oversize`` are ``include`` (keep, no model call), ``exclude`` (drop, no model call), or ``truncate`` (ask the model about a truncated or redacted preview). Pass ``None`` for ``max_bytes`` or diff --git a/tests/fixtures/parity/relevance.json b/tests/fixtures/parity/relevance.json index 54c0340..29545a4 100644 --- a/tests/fixtures/parity/relevance.json +++ b/tests/fixtures/parity/relevance.json @@ -163,13 +163,13 @@ { "text": "a€b", "maxBytes": 4, "expected": "a€" } ], "bytes": { - "rule": "Valid UTF-8 bytes decode to text and a non-empty buffer is never decoded as empty. Invalid UTF-8 takes the binary path. Different bytes at the same path do not share a key.", + "rule": "Valid UTF-8 bytes decode to text and a non-empty buffer is never decoded as empty. A truncated trailing sequence keeps the valid prefix only when those bytes are a legal partial character (0x61 0xC3 and 0x61 0xE0 0xA0 decode to \"a\"). Any other invalid UTF-8 takes the binary path. Different bytes at the same path do not share a key.", "validUtf8": "café", "changed": ["alpha", "beta"] }, "binary": { "sampleBytes": 8192, - "rule": "Validate the whole payload as UTF-8. An incomplete trailing sequence is tolerated only when it ends on the 8192-byte probe boundary. A NUL or any other invalid byte anywhere is binary. A non-empty buffer that decodes to empty text is binary. Included binary files keep their original bytes.", + "rule": "Validate the whole payload as UTF-8. Tolerate a tail only when it is a truncated but valid partial sequence at the exact end of the sample: the lead must be able to start a character (not C0, C1, or F5-FF), every continuation must be 80-BF, and the second byte must be in range (E0 is A0-BF, ED is 80-9F, F0 is 90-BF, F4 is 80-8F). That partial keeps the valid prefix at any length, including 0x61 0xC3 and 0x61 0xE0 0xA0 decoding to \"a\". Bytes 0x61 0xE0 0x80, 0x61 0xED 0xA0, 0x61 0xF0 0x80, and 0x61 0xF4 0x90 are binary. A NUL or any other invalid byte anywhere is binary. A non-empty buffer that decodes to empty text is binary. Included binary files keep their original bytes.", "cases": [ { "name": "2-byte cut", "pad": 8191, "tail": [195], "binary": false }, { "name": "3-byte cut", "pad": 8190, "tail": [226, 130], "binary": false }, @@ -177,7 +177,25 @@ { "name": "invalid lead", "pad": 8191, "tail": [255], "binary": true }, { "name": "nul", "pad": 0, "tail": [104, 105, 0], "binary": true }, { "name": "incomplete only", "pad": 0, "tail": [195], "binary": true }, - { "name": "8192 ASCII + 0xff + TAIL", "pad": 8192, "tail": [255, 84, 65, 73, 76], "binary": true } + { "name": "truncated tail keeps prefix", "pad": 0, "tail": [97, 195], "binary": false, "text": "a" }, + { "name": "valid e0 partial keeps prefix", "pad": 0, "tail": [97, 224, 160], "binary": false, "text": "a" }, + { "name": "8192 ASCII + 0xff + TAIL", "pad": 8192, "tail": [255, 84, 65, 73, 76], "binary": true }, + { "name": "e0 second byte 80", "pad": 1, "tail": [224, 128], "binary": true }, + { "name": "ed second byte a0", "pad": 1, "tail": [237, 160], "binary": true }, + { "name": "f0 second byte 80", "pad": 1, "tail": [240, 128], "binary": true }, + { "name": "f4 second byte 90", "pad": 1, "tail": [244, 144], "binary": true }, + { "name": "e0 second byte 80 at 8192", "pad": 8192, "tail": [224, 128], "binary": true }, + { "name": "ed second byte a0 at 8192", "pad": 8192, "tail": [237, 160], "binary": true }, + { "name": "f0 second byte 80 at 8192", "pad": 8192, "tail": [240, 128], "binary": true }, + { "name": "f4 second byte 90 at 8192", "pad": 8192, "tail": [244, 144], "binary": true }, + { "name": "e0 second byte 80 at 9000", "pad": 9000, "tail": [224, 128], "binary": true }, + { "name": "ed second byte a0 at 9000", "pad": 9000, "tail": [237, 160], "binary": true }, + { "name": "f0 second byte 80 at 9000", "pad": 9000, "tail": [240, 128], "binary": true }, + { "name": "f4 second byte 90 at 9000", "pad": 9000, "tail": [244, 144], "binary": true }, + { "name": "lead c0", "pad": 0, "tail": [97, 192], "binary": true }, + { "name": "lead c1", "pad": 0, "tail": [97, 193], "binary": true }, + { "name": "lead f5", "pad": 0, "tail": [97, 245], "binary": true }, + { "name": "continuation below 80", "pad": 0, "tail": [97, 195, 127], "binary": true } ] }, "bom": { diff --git a/tests/test_relevance.py b/tests/test_relevance.py index 0d3c329..dc71018 100644 --- a/tests/test_relevance.py +++ b/tests/test_relevance.py @@ -1373,6 +1373,97 @@ def fake_call_llm(api_key, base_url, model, messages, max_tokens, temperature, b assert included.files["a.bin"].endswith(b"\xffTAIL") +def test_truncated_utf8_tail_keeps_the_valid_prefix(): + from patchling.relevance import _decode_utf8_text, _prepare_content, is_binary_content + + short = b"a\xc3" + assert is_binary_content(short) is False + assert _decode_utf8_text(short) == "a" + assert _prepare_content(short) == (False, "a") + assert _prepare_content(b"hello\xe2\x82") == (False, "hello") + assert _prepare_content(b"a\xf0\x9f\x98") == (False, "a") + # A lone incomplete sequence decodes to empty text and stays binary. + assert is_binary_content(b"\xc3") is True + assert _prepare_content(b"\xc3") == (True, "") + # A bad continuation is not a truncated tail. + assert is_binary_content(b"a\xc3X") is True + assert _decode_utf8_text(b"a\xc3X") is None + + long = b"a" * 8192 + b"\xc3" + assert len(long) > 8192 + assert is_binary_content(long) is False + binary, text = _prepare_content(long) + assert binary is False + assert text == "a" * 8192 + assert "\ufffd" not in text + + bodies = [] + + def fake_call_llm(api_key, base_url, model, messages, max_tokens, temperature, budget_tokens=None): + bodies.append(messages[-1]["content"]) + return _Response("yes") + + result = filter_relevant( + {"a.py": short}, + "task", + model="m", + cache=False, + api_key="k", + call_llm=fake_call_llm, + ) + assert result.decisions[0].skipped_reason is None + assert result.files["a.py"] == "a" + assert "File contents:\na\n\nIs this file relevant" in bodies[0] + assert "\ufffd" not in bodies[0] + + +def test_ignored_binary_is_not_decoded(monkeypatch): + import patchling.relevance as relevance + + payload = bytes([0x61, 0xC3, 0xFF, 0x00]) + decoded = [] + + def _spy(name, fn): + def wrapped(content, *args, **kwargs): + raw = content + if isinstance(raw, (bytes, bytearray)) and bytes(raw) == payload: + decoded.append(name) + return fn(content, *args, **kwargs) + return wrapped + + for name in ( + "_prepare_content", + "is_binary_content", + "_decode_utf8_text", + "_utf8_text_or_none", + "_coerce_text", + ): + monkeypatch.setattr(relevance, name, _spy(name, getattr(relevance, name))) + + def fake_call_llm(api_key, base_url, model, messages, max_tokens, temperature, budget_tokens=None): + user = messages[-1]["content"] + assert "dist/app.min.js" not in user + return _Response("yes") + + result = filter_relevant( + { + "dist/app.min.js": payload, + "src/keep.js": "export const n = 1;\n", + }, + "task", + model="m", + cache=False, + api_key="k", + call_llm=fake_call_llm, + ) + ignored = next(item for item in result.decisions if item.path == "dist/app.min.js") + assert ignored.decision == "no" + assert ignored.skipped_reason == "ignored" + assert "dist/app.min.js" not in result.files + assert decoded == [] + assert result.files["src/keep.js"] == "export const n = 1;\n" + + def test_leading_bom_is_preserved_in_decoded_text(): from patchling.relevance import PROMPT_VERSION, _prepare_content, _truncate_text @@ -1710,9 +1801,10 @@ def fake_apply(file_path, original_content, file_diff, model, api_key=None, base # sha256 of tests/fixtures/parity/relevance.json. That file is a byte-identical -# copy of test/fixtures/parity/relevance.json on 255BITS/patchling branch -# cursor/relevance-filter-followup-915b. A mismatch fails CI. -RELEVANCE_FIXTURE_SHA256 = "6976ad71714be0138a6043d9bdeb724abe34a65515f8389913dca10a0c48c4f2" +# copy of test/fixtures/parity/relevance.json on 255BITS/patchling commit +# f26c770a026f2554da3972c10b7f7918d075fd56 (branch cursor/main-green-utf8-915b). +# A mismatch fails CI. +RELEVANCE_FIXTURE_SHA256 = "dbe5c7033b279be2a0b923d82dffb1349039b051e18a3650b224570374b8914b" def test_vendored_relevance_fixture_matches_library(): @@ -1722,6 +1814,7 @@ def test_vendored_relevance_fixture_matches_library(): PROMPT_VERSION, _SYSTEM_PROMPT, _build_user_prompt, + _prepare_content, _truncate_text, count_tokens, is_binary_content, @@ -1843,6 +1936,11 @@ def test_vendored_relevance_fixture_matches_library(): sample = b"a" * case["pad"] + tail expect = case.get("binary", case.get("expect_binary")) assert is_binary_content(sample) is expect, case + if "text" in case: + binary_flag, text = _prepare_content(sample) + assert binary_flag is False, case + assert text == case["text"], case + assert "\ufffd" not in text bom = data.get("bom") if bom: