From 46e2e752b22a5a7ca5a476a54299003affee79c5 Mon Sep 17 00:00:00 2001 From: sadlilas <11658960+sadlilas@users.noreply.github.com> Date: Tue, 18 Aug 2026 18:52:05 -0700 Subject: [PATCH] fix: withhold binary command output to prevent context exhaustion Binary payloads returned by bash commands cost tens of thousands of tokens of noise the model cannot use. Per-call truncation bounds a single call but not a session -- a few dozen such calls exhaust even a 1M-token context -- so binary streams are now withheld entirely and replaced with a short, actionable placeholder. Detection runs on the RAW BYTES from the subprocess, in two stages: 1. If the bytes decode as strict UTF-8, the stream is text. This covers ASCII, UTF-8, and the NUL-delimiter idioms (`find -print0`, `grep -z`, `xargs -0`), since NUL is valid UTF-8. NUL is deliberately not used as a binary marker the way tool-web does: in shell output it is a legitimate delimiter, and keying off it would break the standard safe-filename idiom. 2. Otherwise the stream is either binary or text in a legacy 8-bit encoding. These separate cleanly on the proportion of C0/C1 control bytes, which are pervasive in binary and absent from text. Detecting on the decoded string's U+FFFD ratio was implemented first, then measured and rejected: it is inverted on both sides. Real executables carry large ASCII string tables and NUL padding that decode cleanly (/bin/cat 3.6%, python3 2.3%) and would slip past, while text in legacy encodings is high-bit on nearly every character (cp1251 Russian 80.9%, shift_jis Japanese 64.8%) and would be destroyed. On the control-byte measure every real binary tested scored >= 6.0% and every text sample scored 0.0%. Also fixes reported output sizes: stdout_bytes/stderr_bytes now come from the raw subprocess bytes rather than a re-encode of the decoded string, which inflated them (U+FFFD re-encodes to 3 bytes for what was a 1-byte input). Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_module_tool_bash/__init__.py | 112 ++++++++++- tests/test_binary_output_guard.py | 255 +++++++++++++++++++++++++ tests/test_max_concurrent.py | 16 +- 3 files changed, 376 insertions(+), 7 deletions(-) create mode 100644 tests/test_binary_output_guard.py diff --git a/amplifier_module_tool_bash/__init__.py b/amplifier_module_tool_bash/__init__.py index bdaeae7..f2f19a8 100644 --- a/amplifier_module_tool_bash/__init__.py +++ b/amplifier_module_tool_bash/__init__.py @@ -953,6 +953,46 @@ class BashTool: # Default output limit: ~100KB (roughly 25k tokens) DEFAULT_MAX_OUTPUT_BYTES = 100_000 + # Binary output detection. + # + # Binary output costs tens of thousands of tokens of noise the model + # cannot use. Per-call truncation bounds one call but not a session, so + # binary streams are withheld entirely rather than truncated. + # + # Detection runs on the RAW BYTES, in two stages: + # + # 1. If the bytes decode as strict UTF-8, the stream is text. This covers + # ASCII, UTF-8 (accents, CJK, emoji), and -- importantly -- the NUL + # delimiter idioms `find -print0`, `grep -z` and `xargs -0`, since NUL + # is valid UTF-8. Keying off NUL the way tool-web does would break the + # standard safe-filename idiom; in shell output NUL is a delimiter, + # not a binary marker. + # + # 2. Otherwise the stream is not UTF-8, which means either binary or text + # in a legacy 8-bit encoding. These are separated by the proportion of + # C0/C1 control bytes, which are pervasive in binary and essentially + # absent from text of any encoding. + # + # Do NOT re-key this on the U+FFFD ratio of the DECODED string. That was + # measured and rejected: it is inverted on both sides. Real executables + # are full of ASCII string tables and NUL padding that decode cleanly + # (/bin/cat 3.6%, python3 2.3%), so they slip past; while legitimate text + # in legacy encodings is high-bit on nearly every character (cp1251 + # Russian 80.9%, shift_jis Japanese 64.8%), so it gets destroyed. On the + # control-byte measure the two populations separate cleanly: every real + # binary tested scored >= 6.0%, every text sample scored 0.0%. + BINARY_CONTROL_BYTE_RATIO = 0.05 + + # C0/C1 control bytes excluding tab (0x09), LF (0x0A) and CR (0x0D), + # which are legitimate in text. + _BINARY_CONTROL_BYTES = frozenset( + set(range(0x09)) | {0x0B, 0x0C} | set(range(0x0E, 0x20)) | {0x7F} + ) + + # Below this many bytes the ratio is too noisy to be meaningful -- a short + # stream containing one control byte would otherwise trip the guard. + MIN_BINARY_SAMPLE_BYTES = 64 + def __init__(self, config: dict[str, Any]): """ Initialize bash tool. @@ -1205,14 +1245,28 @@ async def execute(self, input: dict[str, Any]) -> ToolResult: # Execute command and wait for completion result = await self._run_command(command, timeout=timeout) - # Apply output truncation to prevent context overflow - stdout, stdout_truncated, stdout_bytes = self._truncate_output( - result["stdout"] + # Decode, withholding binary BEFORE truncating. Truncation + # bounds a single call but not a session: a binary payload + # still costs tens of thousands of tokens of noise per call + # after truncation, so a few dozen such calls exhaust even a + # 1M-token context. + stdout, stdout_binary = self._guard_binary_output( + result["stdout_raw"], stream="stdout" ) - stderr, stderr_truncated, stderr_bytes = self._truncate_output( - result["stderr"] + stderr, stderr_binary = self._guard_binary_output( + result["stderr_raw"], stream="stderr" ) + # True sizes as produced by the subprocess. Taken before + # truncation and independently of decoding, so a lossy decode + # (U+FFFD is 3 bytes for a 1-byte input) cannot inflate them. + stdout_bytes = len(result["stdout_raw"]) + stderr_bytes = len(result["stderr_raw"]) + + # Apply output truncation to prevent context overflow + stdout, stdout_truncated, _ = self._truncate_output(stdout) + stderr, stderr_truncated, _ = self._truncate_output(stderr) + output = { "stdout": stdout, "stderr": stderr, @@ -1227,6 +1281,10 @@ async def execute(self, input: dict[str, Any]) -> ToolResult: if stderr_truncated: output["stderr_total_bytes"] = stderr_bytes + # Include binary metadata if either stream was withheld + if stdout_binary or stderr_binary: + output["binary_output_withheld"] = True + return ToolResult( success=result["returncode"] == 0, output=output, @@ -1308,6 +1366,42 @@ def _extract_tail_bytes(self, output: str, budget: int) -> str: # Fallback: decode with error replacement (shouldn't normally happen) return truncated_bytes.decode("utf-8", errors="ignore") + def _guard_binary_output(self, raw: bytes, stream: str) -> tuple[str, bool]: + """Decode subprocess output, withholding it if it is binary. + + Detection runs on the raw bytes, never on the decoded string (see + BINARY_CONTROL_BYTE_RATIO for why the decoded-string measure was + measured and rejected). + + Text -- in any encoding -- is returned decoded with errors="replace", + matching prior behaviour. Binary is replaced with a short placeholder, + because returning it (even truncated) spends tens of thousands of + tokens on noise the model cannot use. + + Returns: + Tuple of (decoded output or placeholder, was_withheld) + """ + try: + return raw.decode("utf-8"), False + except UnicodeDecodeError: + pass + + if len(raw) < self.MIN_BINARY_SAMPLE_BYTES: + return raw.decode("utf-8", errors="replace"), False + + control_bytes = sum(1 for b in raw if b in self._BINARY_CONTROL_BYTES) + control_ratio = control_bytes / len(raw) + if control_ratio < self.BINARY_CONTROL_BYTE_RATIO: + return raw.decode("utf-8", errors="replace"), False + + placeholder = ( + f"[binary output withheld: {stream} contained {len(raw)} bytes " + f"of non-text data ({control_ratio:.0%} control bytes). " + f"Redirect to a file and inspect it with a suitable tool, " + f"e.g. `command > out.bin` then `file out.bin` or `xxd out.bin | head`.]" + ) + return placeholder, True + def _truncate_output(self, output: str) -> tuple[str, bool, int]: """Truncate output if it exceeds max_output_bytes. @@ -1673,9 +1767,17 @@ async def _run_command( process.communicate(), timeout=effective_timeout ) + # The decoded strings remain the contract for existing callers. + # The raw bytes are carried alongside because binary detection + # must run on the bytes, not on a lossy decode of them (see + # `_guard_binary_output`), and because the true byte count is + # needed for size reporting -- a lossy decode inflates it, since + # U+FFFD re-encodes to 3 bytes for what was a 1-byte input. return { "stdout": stdout.decode("utf-8", errors="replace"), "stderr": stderr.decode("utf-8", errors="replace"), + "stdout_raw": stdout, + "stderr_raw": stderr, "returncode": process.returncode, } diff --git a/tests/test_binary_output_guard.py b/tests/test_binary_output_guard.py new file mode 100644 index 0000000..37684e4 --- /dev/null +++ b/tests/test_binary_output_guard.py @@ -0,0 +1,255 @@ +"""Regression test: binary command output must not be pushed into context. + +## Why this test exists + +Truncation bounds a *single* call at ``max_output_bytes``, but it does not +bound a *session*: a binary payload still costs tens of thousands of tokens of +noise per call after truncation, so a few dozen such calls exhaust even a +1M-token context. The guard withholds binary streams entirely, replacing them +with a short placeholder naming the byte count and pointing at a file-redirect +workflow. + +## Why detection runs on raw bytes + +An earlier revision keyed off the U+FFFD ratio of the *decoded* string. That +measure is inverted on both sides and was rejected on evidence: + +* Real executables are full of ASCII string tables and NUL padding that decode + cleanly -- ``/bin/cat`` scores 3.6% U+FFFD, ``python3`` 2.3% -- so genuine + binaries slip past. +* Text in legacy 8-bit encodings is high-bit on nearly every character -- + cp1251 Russian 80.9%, shift_jis Japanese 64.8% -- so genuine text is + destroyed. + +Detection therefore runs on the raw bytes: strict-UTF-8-decodable is text; +otherwise the C0/C1 control-byte ratio separates binary from legacy-encoded +text. On that measure the populations separate cleanly -- every real binary +tested scored >= 6.0%, every text sample 0.0%. The tests below pin both sides. + +## The NUL trap + +The obvious binary marker -- a NUL byte -- is the wrong signal for a *shell* +tool. ``find -print0``, ``grep -z`` and ``xargs -0`` emit NUL as a legitimate +record delimiter. NUL is valid UTF-8, so those payloads pass stage 1 untouched. +Keying off NUL (as tool-web does, where it genuinely does mean binary) would +break the standard safe-filename idiom. +""" + +from __future__ import annotations + +import shutil +from pathlib import Path + +import pytest +from amplifier_module_tool_bash import BashTool + + +def _tool(**config) -> BashTool: + return BashTool(config) + + +def _synthetic_binary() -> bytes: + """A payload shaped like real binary: dense control bytes, not valid UTF-8.""" + return bytes(range(256)) * 50 + + +# -------------------------------------------------------------------------- +# Binary output is withheld +# -------------------------------------------------------------------------- + + +def test_binary_output_is_replaced_with_placeholder() -> None: + """Binary is withheld and the payload never reaches context.""" + payload = _synthetic_binary() + + guarded, withheld = _tool()._guard_binary_output(payload, stream="stdout") + + assert withheld is True + assert "binary output withheld" in guarded + assert "stdout" in guarded + # The placeholder must be tiny compared to the payload it replaces. + assert len(guarded) < 300 + assert len(guarded) < len(payload) / 10 + + +def test_placeholder_reports_byte_count_and_ratio() -> None: + """The placeholder must tell the model what it lost, not just that it lost.""" + payload = _synthetic_binary() + + guarded, _ = _tool()._guard_binary_output(payload, stream="stdout") + + assert f"{len(payload)} bytes" in guarded + assert "control bytes" in guarded + # And it must point at a workflow that actually works. + assert "file" in guarded or "xxd" in guarded + + +def test_stderr_placeholder_names_stderr() -> None: + """Each stream is guarded independently and names itself.""" + guarded, withheld = _tool()._guard_binary_output( + _synthetic_binary(), stream="stderr" + ) + + assert withheld is True + assert "stderr" in guarded + + +@pytest.mark.parametrize("name", ["cat", "ls", "python3"]) +def test_real_executables_are_withheld(name: str) -> None: + """The case that broke the U+FFFD detector: actual compiled binaries. + + ``cat`` and ``python3`` decode with only 3.6% / 2.3% U+FFFD because they + are dense with ASCII string tables, so a decoded-string measure lets them + through. On raw control bytes they are unambiguous. + """ + path = shutil.which(name) + if path is None: + pytest.skip(f"{name} not on PATH") + + payload = Path(path).read_bytes()[:200_000] + + _, withheld = _tool()._guard_binary_output(payload, stream="stdout") + + assert withheld is True, f"{name} was not detected as binary" + + +# -------------------------------------------------------------------------- +# The NUL trap: legitimate NUL-delimited output must pass through untouched +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("label", "payload"), + [ + # find . -print0 + ("find -print0", b"./a.txt\x00./b.txt\x00./c with spaces.txt\x00" * 10), + # grep -z pattern file + ("grep -z", b"match one\x00match two\x00match three\x00" * 10), + # find -print0 | xargs -0 grep -l + ("xargs -0", b"./src/main.py\x00./src/util.py\x00" * 20), + ], +) +def test_nul_delimited_output_passes_through(label: str, payload: bytes) -> None: + """NUL is a legitimate shell delimiter, not a binary marker. + + A NUL-keyed guard (as used by tool-web, where NUL genuinely does mean + binary) would break the standard safe-filename idiom. NUL is valid UTF-8, + so these payloads pass the strict-decode stage and never reach the ratio. + """ + guarded, withheld = _tool()._guard_binary_output(payload, stream="stdout") + + assert withheld is False, f"{label} output was wrongly withheld" + assert guarded == payload.decode("utf-8") + + +def test_nul_payload_is_valid_utf8() -> None: + """Pins the premise stage 1 rests on: NUL-delimited output is valid UTF-8.""" + raw = b"./a.txt\x00./b.txt\x00" + + decoded = raw.decode("utf-8") # must not raise + + assert "\x00" in decoded + + +# -------------------------------------------------------------------------- +# Ordinary text must not be touched +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("label", "payload"), + [ + ("plain ascii", b"hello world\n" * 100), + ("utf-8 accents", "café résumé naïve\n".encode() * 100), + ("cjk", "日本語のテキスト\n".encode() * 100), + ("emoji", "shipped 🚀 done ✅\n".encode() * 100), + ("json", b'{"key": "value", "n": 42}\n' * 100), + ("ansi colours", b"\x1b[31mERROR\x1b[0m something failed\n" * 100), + ("base64", b"aGVsbG8gd29ybGQgdGhpcyBpcyBiYXNlNjQ=\n" * 100), + ("tabs and crlf", b"col1\tcol2\r\nval1\tval2\r\n" * 100), + ], +) +def test_utf8_text_passes_through(label: str, payload: bytes) -> None: + """Anything that decodes as strict UTF-8 is text. The guard must not eat it.""" + guarded, withheld = _tool()._guard_binary_output(payload, stream="stdout") + + assert withheld is False, f"{label} was wrongly withheld" + assert guarded == payload.decode("utf-8") + + +@pytest.mark.parametrize( + ("label", "text", "encoding"), + [ + ("cp1251 russian", "Привет мир, это текст\n" * 50, "cp1251"), + ("iso-8859-7 greek", "Γειά σου κόσμε\n" * 50, "iso-8859-7"), + ("shift_jis japanese", "日本語のテキストです\n" * 50, "shift_jis"), + ("latin-1 accents", "café résumé naïve\n" * 50, "latin-1"), + ], +) +def test_legacy_encoded_text_passes_through( + label: str, text: str, encoding: str +) -> None: + """The case that broke the U+FFFD detector in the other direction. + + Legacy 8-bit text is not valid UTF-8, so it reaches the ratio stage -- but + it carries no control bytes, so it passes. A decoded-string measure would + have scored these 64-81% "undecodable" and destroyed them. + """ + payload = text.encode(encoding) + + guarded, withheld = _tool()._guard_binary_output(payload, stream="stdout") + + assert withheld is False, f"{label} was wrongly withheld" + # Not valid UTF-8, so it is returned lossily -- but returned, not withheld. + assert guarded == payload.decode("utf-8", errors="replace") + + +def test_short_output_is_never_withheld() -> None: + """Below the sample floor the ratio is too noisy to act on.""" + tool = _tool() + short = b"\xff\xfe\x00\x01" + + assert len(short) < tool.MIN_BINARY_SAMPLE_BYTES + guarded, withheld = tool._guard_binary_output(short, stream="stdout") + + assert withheld is False + assert guarded == short.decode("utf-8", errors="replace") + + +def test_empty_output_is_never_withheld() -> None: + """Empty output must not divide by zero.""" + guarded, withheld = _tool()._guard_binary_output(b"", stream="stdout") + + assert withheld is False + assert guarded == "" + + +def test_sparse_undecodable_bytes_pass_through() -> None: + """A few bad bytes in mostly-text output is text, not binary.""" + payload = (b"normal log line here\n" * 100) + b"\xff\xfe" + + guarded, withheld = _tool()._guard_binary_output(payload, stream="stdout") + + assert withheld is False + assert guarded == payload.decode("utf-8", errors="replace") + + +# -------------------------------------------------------------------------- +# Threshold +# -------------------------------------------------------------------------- + + +def test_threshold_is_documented_value() -> None: + """The measured separation -- text 0.0%, binary >= 6.0% -- sets the bar.""" + assert _tool().BINARY_CONTROL_BYTE_RATIO == 0.05 + + +def test_tab_lf_cr_are_not_control_bytes() -> None: + """Whitespace control characters are legitimate in text output.""" + control = _tool()._BINARY_CONTROL_BYTES + + assert 0x09 not in control # tab + assert 0x0A not in control # LF + assert 0x0D not in control # CR + assert 0x00 in control + assert 0x1B in control # ESC -- dense in binary, sparse in ANSI text diff --git a/tests/test_max_concurrent.py b/tests/test_max_concurrent.py index 5510a07..32bb5ff 100644 --- a/tests/test_max_concurrent.py +++ b/tests/test_max_concurrent.py @@ -60,7 +60,13 @@ async def test_allows_sequential_calls(self): tool = BashTool({"max_concurrent": 1, "safety_profile": "unrestricted"}) with patch.object(tool, "_run_command", new_callable=AsyncMock) as mock_run: - mock_run.return_value = {"stdout": "hello", "stderr": "", "returncode": 0} + mock_run.return_value = { + "stdout": "hello", + "stderr": "", + "stdout_raw": b"hello", + "stderr_raw": b"", + "returncode": 0, + } result1 = await tool.execute({"command": "echo hello"}) result2 = await tool.execute({"command": "echo hello"}) @@ -107,7 +113,13 @@ async def test_no_limit_when_none(self): tool._active_commands = 999 with patch.object(tool, "_run_command", new_callable=AsyncMock) as mock_run: - mock_run.return_value = {"stdout": "hello", "stderr": "", "returncode": 0} + mock_run.return_value = { + "stdout": "hello", + "stderr": "", + "stdout_raw": b"hello", + "stderr_raw": b"", + "returncode": 0, + } result = await tool.execute({"command": "echo test"})