From fcb3a86f2f5c2eb92c3b6cef61cf273884ddb58a Mon Sep 17 00:00:00 2001 From: cdeust Date: Fri, 31 Jul 2026 09:28:44 +0200 Subject: [PATCH] fix(ast-parser): degrade gracefully on tree-sitter grammar DownloadError mcp_server/core/ast_parser.py::_get_extractor_and_tree already treated a missing tree-sitter-language-pack (ImportError) as a handled degraded mode, but had no handling for a pack that imports fine and then fails to fetch a grammar over the network (DownloadError) -- an offline install, air-gapped environment, proxy, or upstream outage crashed codebase_analyze with an uncaught third-party exception instead of degrading (main-red, CI run 30592244731, 2026-07-31: Test (Python 3.10)). Catches DownloadError specifically around get_parser().parse() (not widened to the pack's broader Error base -- only DownloadError is evidenced), returns the same None degraded-mode signal parse_file_ast already reads to pick the regex fallback, and logs one actionable warning naming the language and reason on every occurrence (not suppressed after the first, since this runs once per file, not once per process like a singleton model load). Three regression tests in test_ast_parser_language_contract.py force the failure deterministically (monkeypatch on the pack's own get_parser, matching the file's existing ImportError-probe convention) and assert the log emission itself, not just crash-absence -- all three verified to fail against the pre-fix code. Makes the test suite hermetic rather than papering over the flake: ci.yml's test and test-sqlite jobs (the two that install the codebase extra) gain a Resolve-cache-dir + Cache + Prefetch step trio for tree-sitter grammars, same cache-then-retry-with-backoff shape as the existing HF-embedding/FlashRank steps, reading the language set from AST_SUPPORTED so it cannot drift from what ast_parser.py actually uses. Also fixes pyproject.toml's [tool.mutmut] also_copy list, discovered while mutation-testing this change: it was missing "scripts", and tests_py/conftest.py -- the root conftest every test collects through -- imports scripts.check_venv_lock_parity at module level, so every scoped mutation run in this repo (not just this one) failed collection before scoring a single mutant. No other unguarded get_parser/get_language call site exists in mcp_server/ (swept repo-wide). Co-Authored-By: Claude --- .github/workflows/ci.yml | 54 ++++++++++++ CHANGELOG.md | 1 + mcp_server/core/ast_parser.py | 38 ++++++++- pyproject.toml | 9 ++ .../core/test_ast_parser_language_contract.py | 82 +++++++++++++++++++ 5 files changed, 181 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d8bb56cc..48fb4922 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -154,6 +154,37 @@ jobs: echo "FlashRank pre-download failed after 5 attempts" >&2 exit 1 + # tree-sitter-language-pack fetches each grammar's shared library lazily + # over the network at `get_parser()` call time, not at install time — a + # cold cache mid-suite is a real, previously-uncaught `DownloadError` + # (main-red, CI run 30592244731, 2026-07-31: + # tests_py/core/test_ast_extractors.py + benchmarks/test_codebase_ + # alteration.py). Needs the package importable, so — unlike the HF/ + # FlashRank cache steps above — this must come after "Install + # dependencies"; same cache-then-prefetch-with-retries shape otherwise, + # for the same reason: fetch once here so `pytest` itself never touches + # the network for a grammar. + - name: Resolve tree-sitter cache directory + run: echo "TREE_SITTER_CACHE_DIR=$(python -c 'from tree_sitter_language_pack import cache_dir; print(cache_dir())')" >> "$GITHUB_ENV" + + - name: Cache tree-sitter grammars + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ${{ env.TREE_SITTER_CACHE_DIR }} + key: ${{ runner.os }}-tree-sitter-grammars-${{ hashFiles('pyproject.toml') }} + + # The language set is read from AST_SUPPORTED (not hand-copied here) so + # this step cannot drift from the languages ast_parser.py actually uses. + - name: Prefetch tree-sitter grammars + run: | + for attempt in 1 2 3 4 5; do + python -c "from mcp_server.core.ast_parser import AST_SUPPORTED; from tree_sitter_language_pack import prefetch; prefetch(sorted(AST_SUPPORTED))" && exit 0 + echo "tree-sitter grammar prefetch attempt ${attempt} failed; retrying in $((attempt * 10))s" >&2 + sleep $((attempt * 10)) + done + echo "tree-sitter grammar prefetch failed after 5 attempts" >&2 + exit 1 + # Offline: both models are already cached by the steps above, so tests must # never reach out to huggingface.co mid-suite — deterministic and flake-free. - name: Run tests @@ -270,6 +301,29 @@ jobs: echo "HF pre-download failed after 5 attempts" >&2 exit 1 + # Same rationale as the `test` job's identically-named steps (CI run + # 30592244731, 2026-07-31): this job also installs the `codebase` extra + # (comment above), so it is equally exposed to a cold-cache mid-suite + # `DownloadError` without this. + - name: Resolve tree-sitter cache directory + run: echo "TREE_SITTER_CACHE_DIR=$(python -c 'from tree_sitter_language_pack import cache_dir; print(cache_dir())')" >> "$GITHUB_ENV" + + - name: Cache tree-sitter grammars + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ${{ env.TREE_SITTER_CACHE_DIR }} + key: ${{ runner.os }}-tree-sitter-grammars-${{ hashFiles('pyproject.toml') }} + + - name: Prefetch tree-sitter grammars + run: | + for attempt in 1 2 3 4 5; do + python -c "from mcp_server.core.ast_parser import AST_SUPPORTED; from tree_sitter_language_pack import prefetch; prefetch(sorted(AST_SUPPORTED))" && exit 0 + echo "tree-sitter grammar prefetch attempt ${attempt} failed; retrying in $((attempt * 10))s" >&2 + sleep $((attempt * 10)) + done + echo "tree-sitter grammar prefetch failed after 5 attempts" >&2 + exit 1 + - name: Run SQLite backend tests env: HF_HUB_OFFLINE: "1" diff --git a/CHANGELOG.md b/CHANGELOG.md index d1fba61f..98187ecf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] ### Fixed +- **`codebase_analyze` crashed with an uncaught `tree_sitter_language_pack.DownloadError` when a grammar could not be fetched** (main-red, CI run 30592244731, 2026-07-31, `Test (Python 3.10)`: `tests_py/benchmarks/test_codebase_alteration.py` (5 tests) and `tests_py/core/test_ast_extractors.py::test_decorated_function`). `tree-sitter-language-pack` is a declared dependency, but it resolves each grammar's shared library **lazily over the network at `get_parser()` call time**, not at install time — confirmed by measurement: an already-cached language returns in <50ms with no network attempt, an uncached one takes a real round trip against `https://github.com/xberg-io/tree-sitter-language-pack/releases`. `mcp_server/core/ast_parser.py::_get_extractor_and_tree` already treated a **missing** pack (`ImportError`) as a handled degraded mode, but had no handling for a pack that imports fine and then fails to **fetch** a grammar — an offline install, an air-gapped environment, a proxy, or an upstream outage reached `get_parser(language).parse(content)` (line 99) uncaught, so `codebase_analyze` raised a third-party exception type to its caller instead of degrading. Same defect class this repo has already been bitten by twice (the FlashRank silent-absence incident; the MCP stdio response-loss above): a degraded path existed and did not cover the failure that actually happens. Fixed by catching `DownloadError` specifically (not widened to the pack's broader `Error` base — only `DownloadError` is evidenced) around the `get_parser().parse()` call, returning the same `None` degraded-mode signal `parse_file_ast` already reads to select the regex fallback, and logging one actionable warning naming the language and the reason on every occurrence (not suppressed after the first — this runs once per file, not once per process). Regression tests force the failure deterministically (`monkeypatch.setattr` on the pack's own `get_parser`, matching the file's existing `ImportError`-probe convention) rather than by disabling real network: `tests_py/core/test_ast_parser_language_contract.py` gains `test_download_error_falls_back_instead_of_raising`, `test_download_error_logs_language_and_reason` (asserts the log emission itself, not merely the absence of a crash), and `test_download_error_degrades_through_parse_file_ast` (the public entry point `codebase_analyze` calls) — all three verified to fail against the pre-fix code. Made the test suite hermetic rather than papering over the flake: `.github/workflows/ci.yml`'s `test` and `test-sqlite` jobs (the two that install the `codebase` extra) gain a `Resolve tree-sitter cache directory` + `Cache tree-sitter grammars` + `Prefetch tree-sitter grammars` step trio — same cache-then-retry-with-backoff shape as the existing HF-embedding/FlashRank steps, for the same reason: fetch every language in `AST_SUPPORTED` (read from the module, not hand-copied, so the step cannot drift from what `ast_parser.py` actually uses) once, with retries, before `pytest` starts, so the suite's existing direct `get_parser(...)` calls in `test_ast_extractors.py`/`test_ast_parser_language_contract.py` never touch the network mid-run. No other unguarded `get_parser`/`get_language` call site exists in `mcp_server/` (swept repo-wide; `ast_parser.py` line 99 was the only production call). - **"Docker Smoke" intermittently reported no `tools/list` response with no exception and no JSON-RPC error frame** — reproduced on `main` (CI run 30504042295: attempt 1 failed, attempt 2 succeeded, same commit `56f2f4f`, no code change), on PR #254 and PR #266. Root cause is upstream: `mcp` 1.29.0's `BaseSession._receive_loop` (`mcp/shared/session.py`) closes the write stream unconditionally the instant stdin reaches EOF, even when a request dispatched from an earlier line in the same batch (`tools/list` after `initialize`) is still running in its own task and has not called `respond()` yet — `mcp.server.lowlevel.server.Server._handle_request` catches the resulting `ClosedResourceError` and logs it via `logger.debug()` on a logger with zero handlers by default, so the drop is completely silent. All three JSON-RPC lines are always fully read (measured `parsed_count == 3` on every trial, pass and fail alike) — nothing is ever left unread; only the already-computed response is lost. `fastmcp` 3.4.5's `LowLevelServer.run` override removes the base SDK's own `finally: tg.cancel_scope.cancel()` mitigation with nothing in its place, so Cortex's stdio entry point inherited the hazard unmitigated. Fixed at Cortex's composition root: `mcp_server/infrastructure/stdio_transport.py` interposes a write-stream proxy that no-ops the SDK's premature `aclose()` and closes the real stream only after the low-level server's `run()` call has returned — which, by anyio task-group join semantics, is only once every dispatched handler has had its own chance to respond. `mcp_server/__main__.py::main()` now drives stdio through this wrapper instead of `mcp.run(transport="stdio")` directly. Regression test at the SDK boundary (`tests_py/infrastructure/test_stdio_transport.py`): one test reproduces the drop against the bare upstream call directly (a permanent characterization of the upstream defect), a second drives the identical race through the fix and asserts the response survives — verified to fail against the pre-fix code (bypassing the guard reproduces the exact original symptom: `initialize` answered, `tools/call` silently missing). A scoped mutation run (`scripts/mutation_check.sh`) against the new module found 9 further survivors and 22 uncovered mutants; hardened in `tests_py/infrastructure/test_stdio_transport_wiring.py` (the `stateless` parameter's actual MCP-lifecycle effect, and the outer `run_stdio_drained` wrapper's banner/transport-context-var/log-message wiring) and `_stdio_transport_helpers.py` (shared fixtures, split out to keep both files under the 500-line cap) — final scoped mutation score: 42/43 killed, 1 documented-equivalent (`typing.cast`'s type argument is never read at runtime, same argument as `json_native.py`'s below). `scripts/docker_smoke.sh` gains a second, independent hardening: its `timeout`/`gtimeout` wrapper was itself measured (2026-07-30) not to reliably stop a genuinely hung container (SIGTERM to the `docker run` CLIENT process does not reliably reach the CONTAINER) — the container could outlive its supposed 60s bound indefinitely. A `--cidfile`-based watchdog now `docker kill`s the actual container ID after the same 60s budget, verified against a deliberately hanging test image (fails in exactly 60s, no leaked container) and a deliberately broken/exiting one (fails immediately) — both directions proven stable across repeated runs, on both the `timeout`-available and no-timeout-binary code paths. Boy-scout: `MIN_TOOL_COUNT`'s default and source comment had drifted to 49 (citing a test name, `test_standalone_baseline_is_49_tools`, that no longer exists) against the true current baseline of 52 (`tests_py/test_main.py::test_standalone_baseline_is_52_tools`) — the gate's floor was silently weaker than it should have been by three tools' worth of regression headroom; corrected in the same change. Review found one more divergence before merge: `run_stdio_drained`'s `show_banner` defaulted to a hardcoded `True` rather than resolving `fastmcp.settings.show_server_banner` the way the composition root's replaced call (`mcp.run(transport="stdio")`, via `TransportMixin.run_async`) does — a user who disabled the banner via `FASTMCP_SHOW_SERVER_BANNER=false` got it printed on stderr on every stdio launch regardless. Fixed by defaulting `show_banner` to `None` and resolving the setting at that point, exactly where `run_async` does, so an explicit argument still overrides it; pinned in both directions by `tests_py/infrastructure/test_stdio_transport_wiring.py`. Review found a second, size-only finding: the banner fix's docstring/citation additions pushed `run_stdio_drained` to 54 lines, over both the hard `§4.2` 50-line cap and this repo's own 40-line/method `CLAUDE.md` convention. Behavior-preserving refactor (Fowler 2018 Ch. 6, Extract Function): the banner resolution and its sourced citation move into a new `_resolve_show_banner()` helper (27 lines); the same pattern is applied to `_run_low_level_drained`, which the same measurement pass found already at 57 lines, by extracting the `mcp._mcp_server.run()` call and its `cast()`-equivalence citation into `_run_mcp_with_guarded_stream()` (37 lines) — both public functions land at 39 lines, no test added or modified, same 14/14 targeted + 732/732 (5 skipped) infrastructure-suite pass counts before and after. While relocating the citation, the two upstream line-number references it carried (`fastmcp/server/mixins/transport.py` `L56-57`/`L184-186`) were verified against the actually-installed `fastmcp==3.4.5` in `.venv` and found to be a consistent −32-line offset from the real `run_async`/`run_stdio_async` locations (`L88-89`/`L216-218`); corrected in place rather than carried forward unchecked. ### Security diff --git a/mcp_server/core/ast_parser.py b/mcp_server/core/ast_parser.py index 598c5e9e..1d45e3e2 100644 --- a/mcp_server/core/ast_parser.py +++ b/mcp_server/core/ast_parser.py @@ -15,6 +15,7 @@ from __future__ import annotations import hashlib +import logging from typing import TYPE_CHECKING, TypeGuard from mcp_server.core.ast_extractor_registry import build_extra_extractors @@ -47,6 +48,8 @@ from mcp_server.core.ast_extractor_registry import Extractor +logger = logging.getLogger(__name__) + def is_available() -> bool: """Check if tree-sitter is installed.""" @@ -69,15 +72,44 @@ def _is_ast_language(language: str) -> TypeGuard[SupportedLanguage]: def _get_extractor_and_tree(language: str, content: bytes) -> tuple | None: - """Get tree-sitter extractor and parsed tree, or None for fallback.""" + """Get tree-sitter extractor and parsed tree, or None for fallback. + + Precondition: `language` is a detected file language name; `content` + is that file's raw bytes. + Postcondition: returns `(extractor, tree)` once a parser was obtained + and used to parse `content`. Returns `None` — the degraded-mode signal + `parse_file_ast` reads to pick the regex fallback — when `language` + is unsupported, the pack is not installed (`ImportError`), or the + pack IS installed but could not fetch `language`'s grammar right now + (`DownloadError`: offline, air-gapped, proxied, or an upstream outage + — grammars resolve lazily over the network at `get_parser()` call + time). Never propagates a third-party exception. + """ if not _is_ast_language(language): return None try: - from tree_sitter_language_pack import get_parser # noqa: PLC0415 — optional-feature probe: ImportError here is a handled degraded mode + from tree_sitter_language_pack import ( # noqa: PLC0415 — optional-feature probe: ImportError here is a handled degraded mode + DownloadError, + get_parser, + ) except ImportError: return None - tree = get_parser(language).parse(content) + try: + tree = get_parser(language).parse(content) + except DownloadError as exc: + # Same degraded mode as ImportError above, reached differently: the + # pack imported fine but could not fetch/verify this grammar right + # now. Logged every call, not just the first — this runs once per + # file, not once per process like a singleton model load, so + # suppressing repeats would hide a mid-run outage. + logger.warning( + "tree-sitter grammar for %r could not be obtained (%s); " + "falling back to the regex parser for this file.", + language, + exc, + ) + return None return _EXTRACTORS[language], tree diff --git a/pyproject.toml b/pyproject.toml index eb96926a..64e25ae5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -666,12 +666,21 @@ only_mutate = ["mcp_server/shared/json_native.py"] # generate_repo_badges.py fails EVERY mutant on "assets/badge-version.svg: # missing" before it can score a single one — reproduced empirically # (issue #293) attempting exactly that run. +# scripts: tests_py/conftest.py — the ROOT conftest every test in the suite +# collects through — does `from scripts.check_venv_lock_parity import +# postgresql_extra_drift` at module level. Without this entry, ANY scoped +# run (any `only_mutate`/test selection) fails collection at the conftest +# itself with `ModuleNotFoundError: No module named 'scripts'` before a +# single mutant can be scored — reproduced empirically (2026-07-31) while +# scoping a run at mcp_server/core/ast_parser.py; not specific to that +# file, since every run shares this same conftest. also_copy = [ ".github", "tests_py", "deps", "docs", "assets", + "scripts", "README.md", "CONTRIBUTING.md", "SECURITY.md", diff --git a/tests_py/core/test_ast_parser_language_contract.py b/tests_py/core/test_ast_parser_language_contract.py index 3b833adc..6b9d70eb 100644 --- a/tests_py/core/test_ast_parser_language_contract.py +++ b/tests_py/core/test_ast_parser_language_contract.py @@ -28,6 +28,7 @@ _get_extractor_and_tree, _is_ast_language, is_available, + parse_file_ast, ) pytestmark = pytest.mark.skipif(not is_available(), reason="tree-sitter not installed") @@ -100,6 +101,87 @@ def test_missing_pack_falls_back_instead_of_raising( assert _get_extractor_and_tree("python", b"def f():\n pass\n") is None +# ── DownloadError: the pack imports fine but can't fetch the grammar ────── +# +# Reached a different way than the two tests above: `tree-sitter-language- +# pack` resolves a grammar lazily over the network at `get_parser()` call +# time (not at import time), so an offline install, an air-gapped network, +# a proxy, or an upstream outage raises `DownloadError` from an installed +# pack instead of `ImportError` from a missing one. Forced deterministically +# here (`monkeypatch.setattr` on the pack's own `get_parser`) rather than by +# disabling real network, per CI run 30592244731 (2026-07-31, main) where +# this exact exception escaped `_get_extractor_and_tree` uncaught. + + +def _make_download_error_parser(message: str): + """A `get_parser` replacement that always raises `DownloadError`.""" + from tree_sitter_language_pack import DownloadError + + def _raise(_name: str) -> None: + raise DownloadError(message) + + return _raise + + +def test_download_error_falls_back_instead_of_raising( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import tree_sitter_language_pack as tslp + + monkeypatch.setattr( + tslp, "get_parser", _make_download_error_parser("Failed to fetch manifest") + ) + assert _get_extractor_and_tree("python", b"def f():\n pass\n") is None + + +def test_download_error_logs_language_and_reason( + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + """The signal itself is asserted, not just the absence of a crash + (coding-standards.md §13.1 F1): an operator must be able to tell which + language failed and why from the log alone.""" + import tree_sitter_language_pack as tslp + + monkeypatch.setattr( + tslp, + "get_parser", + _make_download_error_parser( + "Failed to fetch manifest from https://example.invalid" + ), + ) + with caplog.at_level("WARNING", logger="mcp_server.core.ast_parser"): + result = _get_extractor_and_tree("python", b"def f():\n pass\n") + assert result is None + # Pins the message's static wording (both halves), not just the + # dynamic language/reason substitutions — a wording change that drops + # either half would otherwise still pass on the substrings alone. + assert any( + rec.message == "tree-sitter grammar for 'python' could not be obtained " + "(Failed to fetch manifest from https://example.invalid); " + "falling back to the regex parser for this file." + for rec in caplog.records + ) + + +def test_download_error_degrades_through_parse_file_ast( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The public entry point `codebase_analyze` calls: never propagates + the third-party exception, and still returns the same valid + `FileAnalysis` shape the regex fallback produces for a missing + install.""" + import tree_sitter_language_pack as tslp + + monkeypatch.setattr( + tslp, "get_parser", _make_download_error_parser("Failed to fetch manifest") + ) + result = parse_file_ast("demo.py", b"def f():\n pass\n") + assert result.path == "demo.py" + assert result.language == "python" + assert isinstance(result.definitions, list) + + def test_supported_language_with_an_extractor_parses() -> None: result = _get_extractor_and_tree("python", b"def f():\n pass\n") assert result is not None