fix: restore silently-reverted parser fixes + repair _apply_tool_filter for fastmcp>=3 (closes #361, resolves #365)#442
Closed
tirth8205 wants to merge 15 commits into
Closed
fix: restore silently-reverted parser fixes + repair _apply_tool_filter for fastmcp>=3 (closes #361, resolves #365)#442tirth8205 wants to merge 15 commits into
tirth8205 wants to merge 15 commits into
Conversation
The parser gated CALLS edge emission on `enclosing_func` being set, so calls made from module scope (top-level script glue, CLI entrypoints, `if __name__ == "__main__"` blocks, and Jupyter/Databricks notebook cells) produced zero CALLS edges. Any function invoked only from those contexts was flagged as dead by `find_dead_code`, even when the function was the entire reason the script existed. Notebooks are particularly affected because every cell is module-scope by definition, so the existing notebook parser (PR #69) emitted nodes and IMPORTS_FROM edges but no CALLS edges — making the dead-code detector's notebook coverage vacuous. Fix: when `enclosing_func` is None, attribute the CALLS edge to the File node instead of dropping it. Matches the existing convention used by `_extract_value_references` and CONTAINS edges. Applied to all 5 gated emission sites: generic Python/JS/TS path, JSX components, Elixir, Solidity `emit`, and R. Downstream: `detect_entry_points` now filters File-sourced CALLS via `get_all_call_targets(include_file_sources=False)` so script-only callees remain detectable as entry points (otherwise `run_job()` called from `script.py` module scope would look "called" by `script.py` and disappear from flow analysis). Verified end-to-end against a Databricks `.ipynb` that calls `Predict.extract_data_from_sample_ids()` from cell-level code: edge count went from 0 to 14 CALLS edges, and `find_dead_code` no longer flags the method. Tests: - `test_module_scope_calls_attributed_to_file` — bare `.py` script - `test_module_scope_calls_in_notebook` — `.ipynb` file - `test_detect_entry_points_module_scope_caller_is_still_root` — flow analysis treats File-sourced CALLS correctly - `test_module_scope_caller_prevents_dead_code_flag` — end-to-end parse → store → find_dead_code - `test_if_main_block_caller_prevents_dead_code_flag` — same for `__main__` block (cherry picked from commit fe383c7)
…on-less scripts (#276) Two parser improvements that expand code-review-graph's file coverage to extension-less Unix scripts and Korn shell files. Feature 1: .ksh extension → bash parser (#235) ----------------------------------------------- Register .ksh (Korn shell) with tree-sitter-bash alongside the existing .sh / .bash / .zsh entries shipped in v2.3.0. Korn shell is close enough to bash syntactically that tree-sitter-bash handles the structural features the graph captures correctly. Context: in the close comment on PR #230, @tirth8205 explicitly flagged this as worth adding: "The .ksh extension in particular looks worth adding — I didn't include it in #227." Tests: test_detects_language extended with .ksh assertion; test_ksh_extension_parses_as_bash — end-to-end regression test that copies sample.sh to a temp .ksh file, parses it, and asserts identical function set and edge counts. Feature 2: shebang-based language detection (#237) -------------------------------------------------- detect_language() was extension-only — any file with no extension returned None and was silently skipped. This misses a huge category of production files: git hooks, CI scripts, bin/ entry points, installers. New SHEBANG_INTERPRETER_TO_LANGUAGE table maps common interpreter basenames to languages already registered: bash/sh/zsh/ksh/dash/ash -> bash python/python2/python3/pypy/pypy3 -> python node/nodejs -> javascript ruby, perl, lua, Rscript, php New _detect_language_from_shebang(path) static method reads the first 256 bytes, handles direct form (#!/bin/bash), env indirection (#!/usr/bin/env bash), env -S flags, trailing flags (#!/bin/bash -e), CRLF, binary content, and strict UTF-8 decoding. detect_language() now falls back to the shebang probe for files with no extension (suffix == ""). Files with a known extension are never re-read — extension-based detection stays authoritative. Tests (16 new in test_parser.py): every interpreter mapping, env -S flag, trailing flags, missing shebang, empty file, binary content, unknown interpreter, extension-does-not-get-overridden, and end-to-end parse_file producing function nodes from an extension-less bash script. Files changed ------------- - code_review_graph/parser.py — .ksh mapping + SHEBANG_INTERPRETER_TO_LANGUAGE table + _detect_language_from_shebang() + detect_language() fallback - tests/test_multilang.py — .ksh detection + end-to-end ksh parsing test - tests/test_parser.py — 16 shebang detection tests (cherry picked from commit e6e3144)
) Five root-cause fixes that together resolve every pre-existing Windows test failure on main. Each fix has targeted regression tests; the net effect is full green CI on Windows (8 failures -> 0). Bug 1: get_data_dir() writes non-UTF-8 .gitignore on Windows (#239) -------------------------------------------------------------------- write_text() called without encoding="utf-8". The em-dash in the header is U+2014 which Python encodes as cp1252 byte 0x97 on Windows. Any later UTF-8 read fails with UnicodeDecodeError. Fix: add encoding="utf-8" (matches sibling _ensure_repo_gitignore). Test: test_auto_gitignore_is_valid_utf8 — asserts UTF-8 byte sequence, rejects cp1252 byte. Bug 2: Databricks notebook detection fails on CRLF line endings (#239) ---------------------------------------------------------------------- source.startswith(b"# Databricks notebook source\n") hard-codes LF. Windows git checkout (core.autocrlf=true) produces CRLF. All Databricks handling silently bypassed — 4 tests fail. Fix: parse first line robustly, strip trailing \r before exact match. Tests: test_databricks_header_crlf_line_endings, test_databricks_header_lf_line_endings_still_work, test_databricks_header_prefix_false_positive_rejected. Bug 3: Stale FastMCP API in async regression guard (#239) --------------------------------------------------------- test_heavy_tools_are_coroutines called mcp.get_tools() which does not exist in fastmcp>=2.14.0 (pinned in pyproject.toml). The guard has been silently broken since it was written — the protection promised by PR #231 for #46/#136 was never actually enforced. Fix: resolve tools via getattr(crg_main, name) like the sibling test. Drop @pytest.mark.asyncio since no event loop is needed. Test: test_regression_guard_does_not_depend_on_fastmcp_internals — AST-walks the guard source to ensure no mcp internal API references. Bug 4-5: find_repo_root walks above test sandbox (#241) ------------------------------------------------------- test_returns_none_without_git and test_falls_back_to_start fail on any machine where tmp_path has a git-initialized ancestor (dotfiles repo at ~/.git — very common on developer machines). Fix: add optional stop_at parameter to find_repo_root() and find_project_root(). When set, the walk examines stop_at for .git and then stops. Default is None (existing walk-to-root behavior). Fully backward-compatible — all 7 production callers unchanged. Tests: test_stop_at_prevents_escape_to_outer_git, test_stop_at_finds_git_at_boundary, test_stop_at_forwarded_to_find_repo_root. Files changed ------------- - code_review_graph/incremental.py — encoding fix + stop_at API - code_review_graph/parser.py — CRLF-tolerant Databricks detection - tests/test_incremental.py — gitignore UTF-8 guard + stop_at tests - tests/test_main.py — fixed async guard + meta-guard - tests/test_notebook.py — CRLF + LF + false-positive guards (cherry picked from commit aa627fb)
(cherry picked from commit f092922)
(cherry picked from commit 425810b)
(cherry picked from commit 536fd4b)
…odes (#278) tree-sitter-java wraps extends/implements clauses in superclass and super_interfaces nodes whose .text includes the keyword. The _get_bases() function was storing the full text (e.g. "implements UserRepository") as the INHERITS edge target instead of just the type name. This caused inheritors_of queries to fail — the query looks up edges by the qualified class name or bare name, neither of which matches "implements UserRepository". Adds a Java-specific branch in _get_bases() that drills into the AST children to extract type_identifier nodes (including generic_type for parameterized interfaces like IBar<String>). C#/Kotlin are unaffected and retain the existing behavior. (cherry picked from commit 8104eb7)
tree-sitter-java places type_identifier (return type) before identifier (method name) in method_declaration nodes. The generic _get_name() loop matched type_identifier first, causing methods to be indexed under their return type instead of their actual name. For example: * `public String getName()` was indexed as "String" instead of "getName", and * `public ConfigBean getUtilityIngestionBean()`was indexed as "ConfigBean". This broke callers_of,callees_of, and children_of queries for any Java method with a non-void, non-generic return type. Adds a Java-specific branch in _get_name() that returns the first identifier child for method_declaration nodes, following the same pattern as the Go fix (field_identifier) from PR #166. Kotlin & Scala is unaffected — its syntax places the name before the return type. (cherry picked from commit 9a88f20)
Java imports like `import com.example.auth.User` were stored as raw dot-notation strings because _do_resolve_module() had no Java branch. This caused `importers_of` queries to return 0 — the query looks for file path targets, but the stored edges had raw import strings. Adds a Java branch that converts dot-notation to a relative path (com/example/auth/User.java) and walks up from the caller's directory to find the source root. This resolves same-source-root imports (the common case in Maven modules). Also handles: - Static imports (import static pkg.Class.member) — strips the member name and resolves to the class file - Wildcard imports (import pkg.*) — skipped, can't resolve to one file - JDK/library imports (java.util.*) — remain unresolved (no local file) (cherry picked from commit 088c281)
(cherry picked from commit 112a442)
Two pre-existing failures on main, both caused by the 'a3a043b'
feature landing against the fastmcp 2.x private API and never being
updated for the pinned fastmcp>=3:
1. _apply_tool_filter accessed mcp._tool_manager._tools which was
removed when fastmcp rewrote the tool registry. Replaced with
the public async mcp.list_tools() + mcp.local_provider.remove_tool(),
with a thread-pool fallback for callers that already have a
running event loop (tests, future integrations).
2. TestServeMainTransport.test_stdio_calls_mcp_run_stdio asserted
exact kwargs {transport: stdio} but PR #290 (cc169af) added
show_banner=False to the production call without refreshing the
test. Updated the assertion.
3. TestApplyToolFilter relied on mcp._tool_manager._tools for its
snapshot/restore fixture and on await mcp.get_tools() for its
assertions. Neither exists on fastmcp>=3. Rewrote the fixture
to snapshot via await mcp.list_tools() + restore via
mcp.add_tool(), and the assertions to use list_tools() directly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ions _get_name() returned wrong or missing names for C++ method definitions whose name is expressed via qualified_identifier (Class::method), destructor_name (~Class), or operator_name (operator==): - Ret Class::method() -> stored as "Ret" (return type) - void Class::method() -> silently dropped (no node emitted) - Class::~Class() -> silently dropped - bool Class::operator==() -> silently dropped Root cause: after recursing into function_declarator, the generic identifier loop only recognises identifier/name/type_identifier/ property_identifier/simple_identifier/constant. qualified_identifier, destructor_name, and operator_name are not in that list, so the recursive call returns None and the outer call falls through to the generic loop on function_definition, matching the sibling return-type type_identifier as the function name (or nothing, for void). Fix: handle qualified_identifier (peeling nested scopes for Outer::Inner::method), destructor_name, operator_name, and field_identifier explicitly inside function_declarator, before the generic fallthrough. Verified on a real-world Qt C++ codebase (~2,770 files): node count 6,903 -> 23,815 (3.4x), edges 42,997 -> 182,077 (4.2x). Caller-graph queries that returned empty before now return correct caller lists.
…uctor/operator definitions
Resolved conflicts in code_review_graph/parser.py and tests/test_parser.py: - parser.py: kept Julia parser block (from PR #365) and merged the field_identifier comment from main (both were about different sections of the same 'name child' generic loop). - test_parser.py: kept test_cpp_scoped_method_names inside TestModuleScopeCalls (from PR #365, covers destructor/operator/deep scoping) AND kept TestCppScopedFunctionName class (from main PR #403, covers qualified return types). Both test suites are complementary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tirth8205
changed the title
Owner
Author
|
Superseded by a cleaner branch that properly resolves all conflicts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is the conflict-resolved version of PR #365 by @npkriami18, which could not be directly merged due to conflicts with main.
Closes #361. Supersedes #365.
Changes from PR #365 (original)
_apply_tool_filterfor fastmcp>=3 API (usesmcp.list_tools()+mcp.local_provider.remove_tool())Conflict resolution
Two files conflicted between PR #365 and main:
code_review_graph/parser.py: Kept the full Julia parser block (from fix: restore 8 silently-reverted parser fixes + repair _apply_tool_filter for fastmcp>=3 (closes #361) #365) and merged thefield_identifiercomment from main's fix: extract correct function name for C++ scoped definitions #403 fix. Both the simple qualified_identifier fix (main fix: extract correct function name for C++ scoped definitions #403) and the comprehensive fix (fix: restore 8 silently-reverted parser fixes + repair _apply_tool_filter for fastmcp>=3 (closes #361) #365 — also handlesdestructor_name,operator_name, nested scopes) are present in the merged file.tests/test_parser.py: Kepttest_cpp_scoped_method_namesmethod insideTestModuleScopeCalls(from fix: restore 8 silently-reverted parser fixes + repair _apply_tool_filter for fastmcp>=3 (closes #361) #365, covers destructor/operator/deep scoping) AND keptTestCppScopedFunctionNameclass (from main fix: extract correct function name for C++ scoped definitions #403, covers qualified return types). Both test suites are complementary and test different C++ parsing scenarios.Original PR #365 description
Closes #361.
Two distinct bug classes addressed:
Class 1 — silently-reverted parser PRs (9 cherry-picks)
Two
chore: resolve merge conflicts with maincommits accepted the stale feature-branch side ofparser.py. Both feature branches forked from before the fixes below landed..kshClass 2 — fastmcp>=3 API drift
_apply_tool_filteraccessedmcp._tool_manager._toolswhich was removed in fastmcp 3.x. Replaced with publicawait mcp.list_tools()+mcp.local_provider.remove_tool().🤖 Generated with Claude Code