Skip to content

chore: fix CI lint and type errors on main#342

Open
gzenz wants to merge 17 commits intotirth8205:mainfrom
gzenz:chore/fix-upstream-lint-v2
Open

chore: fix CI lint and type errors on main#342
gzenz wants to merge 17 commits intotirth8205:mainfrom
gzenz:chore/fix-upstream-lint-v2

Conversation

@gzenz
Copy link
Copy Markdown
Contributor

@gzenz gzenz commented Apr 19, 2026

Summary

Main branch CI has been failing since PR #94. This fixes it.

  • Remove 10 duplicate macOS-copy module files (analysis 2.py, enrich 2.py, enrich 3.py, exports 2.py, exports 3.py, graph_diff 2.py, jedi_resolver 2.py, memory 2.py, memory 3.py, token_benchmark 2.py) that tripped ruff N999.
  • Sort imports in main.py (ruff I001).
  • Drop unused parse_git_diff_ranges import in tools/review.py (ruff F401).
  • Add # type: ignore[attr-defined] for FastMCP._tool_manager private attribute access in main.py.

Verified locally: uv run ruff check code_review_graph/ and uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional both pass.

Context

This is PR 0/16 in a stack that ports closed #158. Merging this first turns main green so the 16 feature PRs (#326-#341) can show clean CI after rebase. See any of those PRs for the full stack description.

gzenz added 17 commits April 19, 2026 09:54
- Remove duplicate macOS-copy module files (analysis 2.py, enrich 2.py,
  enrich 3.py, exports 2.py, exports 3.py, graph_diff 2.py,
  jedi_resolver 2.py, memory 2.py, memory 3.py, token_benchmark 2.py)
  that tripped ruff N999.
- Sort imports in main.py (ruff I001).
- Drop unused parse_git_diff_ranges import in tools/review.py (ruff F401).
- Add type-ignore for FastMCP._tool_manager private attribute access
  in main.py.

CI on main has been red since PR tirth8205#94. This fixes it.
MCP tool calls were opening a fresh GraphStore (and SQLite connection)
on every invocation. Cache one GraphStore per db_path and reuse it,
falling back to a fresh connection if the cached one is dead.

Thread-safe via a module-level lock.
Exact known-answer assertions for compute_risk_score and trace_flows, plus
error-path coverage for parser on malformed input and module-cache eviction.

- TestRiskScoreExact: 6 tests pinning risk score math (untested, tested,
  security keyword, caller fractions, 20-caller cap)
- TestFlowExact: 4 tests for linear chain depth, cycles, single-file
  criticality, and non-test neighbors
- TestParserErrorPaths: 8 tests for binary files, unknown extensions,
  syntax errors, empty files, CRLF, deeply nested AST, unicode
- TestCacheEviction: oldest-half module cache eviction
Add three helpers:

- `_resolve_star_imports`: walks top-level `import_from_statement`
  nodes, detects `wildcard_import`, resolves the source module to a
  file, and merges the exported names into the caller's import_map.
- `_get_exported_names`: returns the public names a module exports,
  respecting `__all__` when present. Caches results per resolved path
  with a threading lock so concurrent parses share work safely.
- `_extract_dunder_all`: parses `__all__ = [...]` at module scope.

Wires star-import expansion into `parse_bytes` and the notebook
concat path. Enables resolution of calls that come in via wildcard
imports (e.g. `from constants import *` then `use_constant()`).
When receiver filtering emits "Foo.bar" (uppercase receiver), resolve it
to "/path/file::Foo.bar" by looking up the class entry in the file
symbol table. Closes a resolution gap where class-qualified bare calls
stayed unresolved after the receiver filter rewrite.
Add _resolve_cross_file_class_methods as a postprocess step that scans
Class nodes, builds a name -> file::ClassName map (dropping colliding
names), and rewrites bare ClassName.method edge targets to the qualified
file::ClassName.method form. Complements the per-file resolver in
parser.py which only sees symbols in the calling file.
The cross-file ClassName.method resolver dropped any class whose name
appeared in more than one file. That was too strict: codebases commonly
reuse short class names across feature packages (util/ClassA.kt and
test/ClassA.kt, multiple platform-specific Impl files, etc.). The call
site almost always means the version in the closest package.

Rebuild class_candidates as a name -> [qn, ...] map. For each edge with
a bare ClassName.method target, pick the class qn whose file path shares
the deepest common directory prefix with the edge's calling file. If the
top match is not unique, fall back to leaving the target bare (rather
than mis-resolving).
…odes

The parser already extracted decorator tuples for test detection but
discarded them before emitting NodeInfo. flows._has_framework_decorator
and refactor._is_entry_point expect this data on nodes[...].extra; with
no source data, entry-point detection never fires and framework-exposed
callables (FastAPI routes, Click commands, Spring handlers) are flagged
as dead.

Wire class-level decorators (Python decorated_definition, Java/Kotlin
modifiers.annotation/marker_annotation) and function-level decorators
(existing tuple) into NodeInfo.extra["decorators"]. flows.py patterns
already cover FastAPI/Click/Spring/Angular/Celery/pytest.
_resolve_call_targets only consulted the same-file symbol table, so every
call to an imported library symbol (Column, useState, Depends, etc.) stayed
bare. For Python-heavy repos this is the dominant unresolved class and
halves the CALLS resolution rate.

Pass the file's import_map through to _resolve_call_targets and add a
third resolution arm: bare target -> imported module -> {module}::{name}.
Also matches ClassName.method when ClassName is imported. Same-file
symbols still take precedence.

Notebook parse path merges import_maps across per-language cell groups
before resolving, so Python/R notebooks see all imports.
- Dedupe callers/callees/inheritors by qualified_name (repeated CALLS
  edges were surfacing the same node multiple times).
- tests_for: use store.get_transitive_tests (direct TESTED_BY + one CALLS
  hop) instead of only direct TESTED_BY edges.
- Disambiguation: when multiple candidates match a bare name and exactly
  one is a production node (not is_test), prefer it silently instead of
  returning 'ambiguous'.
Webpack/rollup bundle outputs and AWS CDK synth artifacts are generated
code that pollutes the graph. Add them to DEFAULT_IGNORE_PATTERNS.
Log duration of each postprocess stage (signatures, FTS, flows, communities,
summaries) and include them in the build result under 'postprocess_timing'.
Also log which summary table skipped on sqlite OperationalError (was silent
rollback).
Add `_INSTANCE_METHOD_BLOCKLIST` (48 ubiquitous JS/TS/Python method
names: push, map, toString, etc.) and receiver filtering for CALLS
edges on member expressions (obj.method()).

In production code, only emit CALLS when:
- receiver is self/cls/this/super (always resolved to enclosing class)
- receiver starts with uppercase (ClassName.method → "ClassName.method")
- receiver is a namespace import (import * as X from Y)
- receiver's leftmost ident is an imported module (module-qualified path)

Other instance calls emit bare method names unless the name is in the
blocklist, in which case the edge is dropped as unresolvable noise.

Test files are exempted -- they still emit all method calls so TESTED_BY
edges continue to resolve correctly.
Add `_enrich_typed_var_calls` plus per-language walkers for Python,
Kotlin, Java, and JS/TS. Each walker tracks typed variables within a
scope and, when a call is made against one, emits a CALLS edge to
`Type::method` (or to the resolved cross-file target when the type
is importable).

Handles:
- Python `service: AuthService = ...` then `service.method()`
- Kotlin `val service: AuthService` and primary-constructor params
- Java `FooService svc = ...` and `var x = new Foo()`
- TS/JS typed locals, `new X()` inference, and constructor parameter
  properties (`constructor(private svc: AuthService)` →
  `this.svc.method()`)

Invoked after `_extract_from_tree` and before `_resolve_call_targets`
in `parse_bytes`, so the synthetic edges flow through the normal
resolution pass.
Add two enrichers that catch function/class references that aren't
invoked at the reference site but still establish a call relationship:

`_enrich_func_ref_args`: scans call argument lists for identifiers
that match a locally-defined function or class name, plus JSX
expression attributes (`onClick={handler}`) and Kotlin callable
references (`::agentThread`).

`_enrich_func_ref_returns`: scans return statements and assignments
for identifiers that match a defined name. Skips self-referential
assignments (`x = x`).

These prevent dead-code false positives for callbacks, thread
targets, and registered handlers (e.g.
`Thread(target=worker)`, `HTTPServer(addr, Handler)`,
`return myCallback`).
- `build`/`update`/`status` now accept `-q`/`--quiet` to suppress
  progress output (useful for Claude Code PostToolUse hooks).
- `status --json` emits the full status block as a single JSON object
  (nodes, edges, files, languages, last_updated, branch info).
- New `enrich` subcommand wires the existing `enrich.run_hook()`
  module so Claude Code PreToolUse hooks can invoke
  `code-review-graph enrich` directly.
Expose find_dead_code via 'code-review-graph dead-code' with --kind,
--file-pattern, --limit, and --json flags. Prints a compact table by
default, JSON with --json for scripting.
@gzenz gzenz force-pushed the chore/fix-upstream-lint-v2 branch from 8148d4f to 1296f3f Compare April 19, 2026 07:55
@gzenz
Copy link
Copy Markdown
Contributor Author

gzenz commented Apr 19, 2026

Heads-up on pre-existing test failures this PR surfaces (but does not introduce):

Before this PR, lint failed on main, so test jobs never got to run on PRs. Now that lint passes, four test failures show up on every PR in the stack. I believe all of them are pre-existing on main:

  1. AttributeError: 'FastMCP' object has no attribute '_tool_manager' — upstream fastmcp dropped the private _tool_manager attribute. The code in main.py:_apply_tool_filter and the --tools test in test_main.py both need updating. Touches: a3a043b feat(serve): add --tools flag.
  2. AssertionError: [{'show_banner': ..., 'transport': 'stdio'}] == [{'transport': 'stdio'}]fastmcp.run() signature drift; test expects the old signature.
  3. Parser regressions on the main tree (unrelated to this stack):
    • Godot _ready top-level function not detected
    • SQL sqlQuery target missing (now returns \dirname)
    • Generic class detection returning empty set for SampleManager, Item
    • Java imports[].target returning FQN (com.example.auth.User) instead of resolved file path
    • Java getName method missing from extracted names
    • INHERITS target leaking implements / extends prefix

Happy to fix (1) and (2) in this PR if you want — they're small (update _tool_manager usage to the new fastmcp API, align the test signature). (3) is a larger surface and probably belongs in a separate PR once this stack lands.

Tell me how you'd like to proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant