From b84bb8ded7aeb0b2286c64be58c78ec2c560d9d1 Mon Sep 17 00:00:00 2001 From: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com> Date: Sat, 15 Aug 2026 10:14:00 +1000 Subject: [PATCH] fix(build-context): bound the per-file read into file_cache `_read_file_cache()` called `_read_text_no_follow()` on every discovered component, and that does an unbounded `source.read()`. A local directory scan therefore materialized each file whole before any analyzer looked at it, so a multi-GB file in a skill drove peak memory to its full size and was only then skipped downstream at `MAX_FILE_CHARS`. `INGEST_MAX_BYTES` does not cover this. Its own docstring scopes it to "Each remote/archive ingest path", and a local directory target reaches `build_context` through `validate_local_input_path()`, which does no sizing. `MAX_FILE_BYTES` is not a gate here either: in this module it is used only inside `_is_valid_oms_signature()` and for a `size_bytes` metadata field. The gate reuses the stat already taken for the `S_ISREG` check, so it costs no extra syscall, and it emits `LedgerOutcome.SKIPPED` with `LedgerReason.SIZE_LIMIT` rather than raising. That matches what the static, AST, taint, and YARA analyzers already do for oversized input, so the file is reported as not-inspected instead of silently vanishing, and it flows into `analysis_completeness` the same way. The bound is derived rather than picked. Every downstream consumer limits itself in characters, the largest being `MAX_PYTHON_AST_CACHE_SOURCE_CHARS`. UTF-8 uses at most 4 bytes per character, and `errors="replace"` yields one character per undecodable byte, so nothing above 4x that character budget can decode to a size any consumer accepts. The gate is outcome-preserving by construction: it cannot exclude content that would otherwise have been analyzed, it only declines to materialize bytes already guaranteed to be skipped. `test_cache_read_bound_cannot_exclude_content_any_consumer_accepts` fails if a consumer ever raises its budget past the bound. One behavior change worth calling out: the LLM `semantic_*` path has no character cap of its own, so a file above this bound previously would have been chunked and sent to the provider. It now reaches `get_batches()` absent from the cache. That is the intended direction for a scanner, and the ledger event makes it visible rather than silent. Tests cover the skip, the ledger event fields, the inclusive boundary, and the derivation. `test_build_context_never_reads_an_oversized_file` spies on `_read_text_no_follow` because asserting only that the path is missing from `file_cache` would still pass if the file were read in full and discarded, which would leave the peak-memory problem exactly where it was. Verified against three mutants: removing the gate, making it read before testing size, and making the bound exclusive. The read-then-discard mutant is caught by that spy test alone. 2189 passed, 17 skipped, 4 xfailed. Ruff clean. Signed-off-by: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com> --- src/skillspector/nodes/build_context.py | 29 +++++- tests/nodes/test_build_context.py | 112 +++++++++++++++++++++++- 2 files changed, 138 insertions(+), 3 deletions(-) diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index 0caa441f..09c3bb1f 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -46,7 +46,7 @@ ledger_event, ) from skillspector.logging_config import get_logger -from skillspector.python_ast import prewarm_python_ast_cache +from skillspector.python_ast import MAX_PYTHON_AST_CACHE_SOURCE_CHARS, prewarm_python_ast_cache from skillspector.state import SkillspectorState logger = get_logger(__name__) @@ -79,6 +79,20 @@ {".py", ".sh", ".bash", ".zsh", ".js", ".ts", ".rb", ".go", ".rs", ".pl"} ) +# Upper bound on a single file read into ``file_cache``. +# +# Every downstream consumer bounds itself in *characters*: the analyzers skip at +# ``static_runner.MAX_FILE_CHARS`` and the prewarmed AST cache rejects a single +# source above ``MAX_PYTHON_AST_CACHE_SOURCE_CHARS``, the larger of the two. +# UTF-8 needs at most 4 bytes per character, and ``errors="replace"`` yields one +# character per undecodable byte, so a file larger than 4x that character budget +# cannot decode to something any consumer would accept. Reading it would only +# materialize bytes that are guaranteed to be skipped, which is why the gate is +# outcome-preserving rather than a new policy: it bounds peak memory for a local +# directory scan, which ``input_handler.INGEST_MAX_BYTES`` does not cover because +# that budget applies only to remote and archive ingest. +MAX_CACHE_READ_BYTES = MAX_PYTHON_AST_CACHE_SOURCE_CHARS * 4 + _OMS_SIGNATURE_PATH = "skill.oms.sig" _SIGSTORE_BUNDLE_MEDIA_TYPE = "application/vnd.dev.sigstore.bundle.v0.3+json" _IN_TOTO_PAYLOAD_TYPE = "application/vnd.in-toto+json" @@ -416,6 +430,19 @@ def _read_file_cache( ) ) continue + if file_stat.st_size > MAX_CACHE_READ_BYTES: + ledger_events.append( + ledger_event( + outcome=LedgerOutcome.SKIPPED, + record_type=LedgerRecordType.SYSTEM, + phase="cache", + path=path, + reason=LedgerReason.SIZE_LIMIT, + observed_bytes=file_stat.st_size, + limit_bytes=MAX_CACHE_READ_BYTES, + ) + ) + continue try: content = _read_text_no_follow(full) file_cache[path] = content diff --git a/tests/nodes/test_build_context.py b/tests/nodes/test_build_context.py index 19c4ca65..5e600e1d 100644 --- a/tests/nodes/test_build_context.py +++ b/tests/nodes/test_build_context.py @@ -30,9 +30,16 @@ from langgraph.checkpoint.serde.jsonplus import JsonPlusSerializer from skillspector.constants import MODEL_CONFIG -from skillspector.nodes.build_context import build_context +from skillspector.inspection_ledger import LedgerOutcome, LedgerReason +from skillspector.nodes import build_context as build_context_module +from skillspector.nodes.analyzers.static_runner import MAX_FILE_CHARS +from skillspector.nodes.build_context import MAX_CACHE_READ_BYTES, build_context from skillspector.providers import reset_provider, use_provider -from skillspector.python_ast import ParsedPythonFile, get_python_ast +from skillspector.python_ast import ( + MAX_PYTHON_AST_CACHE_SOURCE_CHARS, + ParsedPythonFile, + get_python_ast, +) from skillspector.state import SkillspectorState _OMS_FIXTURE = Path(__file__).parents[1] / "fixtures" / "oms" / "mcore-split-pr.skill.oms.sig" @@ -667,3 +674,104 @@ def test_build_context_rejects_symlinked_manifest(tmp_path: Path) -> None: assert result["manifest"] == {} assert "SKILL.md" not in result["components"] assert "SKILL.md" not in result["file_cache"] + + +def _skill_with_large_file(tmp_path: Path, size_bytes: int) -> Path: + """Lay out a minimal skill whose scripts/ holds a sparse file of *size_bytes*.""" + skill = tmp_path / "skill" + (skill / "scripts").mkdir(parents=True) + (skill / "SKILL.md").write_text("---\nname: big\n---\nbody\n", encoding="utf-8") + huge = skill / "scripts" / "huge.py" + with huge.open("wb") as handle: + handle.truncate(size_bytes) # sparse: costs no disk blocks + return skill + + +def test_build_context_skips_a_file_over_the_cache_read_bound(tmp_path: Path) -> None: + """An oversized file is recorded as skipped instead of being read into memory. + + `_read_file_cache` previously called `_read_text_no_follow`, which does an + unbounded `source.read()`, on every discovered component. `INGEST_MAX_BYTES` + does not help here: it bounds remote and archive ingest only, so a local + directory scan materialized the whole file before any analyzer skipped it. + """ + skill = _skill_with_large_file(tmp_path, MAX_CACHE_READ_BYTES + 1) + + result = build_context({"skill_path": str(skill)}) + + assert "scripts/huge.py" not in result["file_cache"] + assert "SKILL.md" in result["file_cache"] + + events = [ + event + for event in result["inspection_ledger"] + if event.get("path") == "scripts/huge.py" and event.get("phase") == "cache" + ] + assert len(events) == 1, events + event = events[0] + assert event["outcome"] == LedgerOutcome.SKIPPED.value + assert event["reason_code"] == LedgerReason.SIZE_LIMIT.value + assert event["observed_bytes"] == MAX_CACHE_READ_BYTES + 1 + assert event["limit_bytes"] == MAX_CACHE_READ_BYTES + + +def test_build_context_never_reads_an_oversized_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The gate must prevent the read, not merely discard its result. + + Asserting only that the path is absent from `file_cache` would still pass if + the file were read in full and then dropped, which leaves the peak-memory + problem exactly where it was. This pins the read itself. + """ + skill = _skill_with_large_file(tmp_path, MAX_CACHE_READ_BYTES + 1) + read_paths: list[str] = [] + real_read = build_context_module._read_text_no_follow + + def spy(path: Path) -> str: + read_paths.append(path.name) + return real_read(path) + + monkeypatch.setattr(build_context_module, "_read_text_no_follow", spy) + + build_context({"skill_path": str(skill)}) + + assert "huge.py" not in read_paths + assert "SKILL.md" in read_paths + + +def test_build_context_still_reads_a_file_at_the_cache_read_bound( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The bound is inclusive, so a file exactly at the limit is still analyzed. + + Guards the off-by-one in the other direction: an exclusive `>=` here would + silently drop content every analyzer would have accepted. + """ + monkeypatch.setattr(build_context_module, "MAX_CACHE_READ_BYTES", 64) + skill = tmp_path / "skill" + (skill / "scripts").mkdir(parents=True) + (skill / "SKILL.md").write_text("---\nname: edge\n---\nbody\n", encoding="utf-8") + (skill / "scripts" / "edge.py").write_bytes(b"x" * 64) + + result = build_context({"skill_path": str(skill)}) + + assert result["file_cache"]["scripts/edge.py"] == "x" * 64 + assert not [ + event + for event in result["inspection_ledger"] + if event.get("path") == "scripts/edge.py" + and event.get("reason_code") == LedgerReason.SIZE_LIMIT.value + ] + + +def test_cache_read_bound_cannot_exclude_content_any_consumer_accepts() -> None: + """The bound is derived from the largest character budget downstream. + + UTF-8 uses at most 4 bytes per character, and `errors="replace"` yields one + character per undecodable byte, so nothing above this byte count can decode + to a character count any consumer would accept. If a consumer ever raises + its character budget, this assertion fails and the bound must move with it. + """ + assert MAX_CACHE_READ_BYTES == MAX_PYTHON_AST_CACHE_SOURCE_CHARS * 4 + assert MAX_CACHE_READ_BYTES >= MAX_FILE_CHARS * 4