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