Skip to content

fix(evals): type the node-exists rows as ResultRow and type-check evals in CI - #3437

Merged
vitali87 merged 1 commit into
mainfrom
fix/evals-ty-result-row
Oct 24, 2026
Merged

vitali87 merged 1 commit into
mainfrom
fix/evals-ty-result-row

Conversation

@vitali87

@vitali87 vitali87 commented Oct 24, 2026 •

Copy link
Copy Markdown
Owner

Summary

Closes #3436.

The CYPHER_GRAPH_NODE_EXISTS branch of _StatefulIngestor.fetch_all in evals/cgr_graph.py builds {cs.KEY_QUALIFIED_NAME: wanted} rows inline. ty infers those rows as dict[str, str]. Because dict is invariant in its value type, that is not a ResultRow, so the local ty pre-commit hook failed on every commit. The rows are now annotated list[ResultRow]. The returned value is unchanged.

CI's two ty steps checked only codebase_rag, so the error never showed up there. Both now check codebase_rag evals.

Tests

codebase_rag/tests/test_eval_graph_node_exists.py covers the branch. test_mcp_input_bounds.py and test_gloss_tools.py drive the unknown-target refusals, but both stayed green with a raise inside the branch, so neither reaches it.

  • test_a_name_any_node_carries_is_found_as_one_row: a name carried by two nodes (a Function and a Method) comes back as exactly one row, and node_exists reports it as present.
  • test_an_unknown_name_is_not_found (negative): a name that no node carries returns no rows, and node_exists reports it as absent.

Both tests pass on the original code and on the annotated code. Each also fails under at least one mutation:

  • Raising inside the branch fails both.
  • Dropping [:1] or returning [] fails the first.
  • Removing the name filter fails the second.

Verification

  • Before: the hook's own ty command and ty check codebase_rag evals --exclude codebase_rag/tests/ each report 1 diagnostic, at evals/cgr_graph.py:1379.
  • After: both report "All checks passed!". The same holds with the milvus extra installed, and with milvus plus neo4j, which is CI's second step.

Summary by CodeRabbit

  • Chores
    • Automated type checks now cover the evaluation code as well as the main codebase, including configurations with and without the optional Neo4j support.
  • Tests
    • Added coverage for finding names shared across graph nodes and confirming that unknown names are not found.

@greptile-apps

This comment has been minimized.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T12:54:04.848292Z e750dd8 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 88cd9e6 and e750dd8.

Files selected for processing: 3
  • .github/workflows/ci.yml
  • codebase_rag/tests/test_eval_graph_node_exists.py
  • evals/cgr_graph.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review.

Walkthrough

The graph-node existence case now builds matching rows before returning at most one row. Tests cover present and absent names. Both CI type-check steps now include evals alongside codebase_rag.

Changes: Graph node existence

Layer / File(s) Summary
Graph-node result construction and tests
evals/cgr_graph.py, codebase_rag/tests/test_eval_graph_node_exists.py
The existence case builds matching rows in a local list and returns at most one row. Tests cover a qualified name shared by two node labels and an unknown name.
CI type-check coverage
.github/workflows/ci.yml
Both the standard and neo4j-extra type-check steps now include evals.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: claude

Merge Risk: Minimal · up to e750d

No actionable merge-blocking risk remains from the reviewed changes.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the type annotation fix and the CI scope update. It uses a concise Conventional Commits format.
Description check Passed The description explains the problem, the implementation, the tests, and the verification results. It omits some template sections, but it provides the critical information for this change.
Linked Issues check Passed Issue #3436 requires fixing the invalid-return-type diagnostic in evals/cgr_graph.py and extending CI type checking to evals/. The PR annotates the local rows as list[ResultRow] and preserves …
Out of Scope Changes check Passed The changes stay within issue #3436. The CI updates prevent hook and CI type-checking drift. The tests exercise the changed CYPHER_GRAPH_NODE_EXISTS behavior. No unrelated production behavior or unr…
Full details: Linked Issues check

Explanation

Issue #3436 requires fixing the invalid-return-type diagnostic in evals/cgr_graph.py and extending CI type checking to evals/. The PR annotates the local rows as list[ResultRow] and preserves the existing filter and [:1] limit. Both CI ty steps now check codebase_rag evals. The added tests cover a present qualified name and an unknown qualified name.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #3436. The CI updates prevent hook and CI type-checking drift. The tests exercise the changed CYPHER_GRAPH_NODE_EXISTS behavior. No unrelated production behavior or unrelated files are changed.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no blocking issues were found.

Summary

The PR fixes the inferred row type without changing lookup results.

  • The in-memory graph lookup returns typed name matches.
  • CI checks evals code alongside the main package.

No actionable issues found. Tests and type checks were not run during this review.

Reviews (1) · Last reviewed commit: "fix(evals): type the node-exists rows as..." · Reviewed by Greptile

@sonarqubecloud

Copy link
Copy Markdown

@vitali87
vitali87 merged commit fbccf83 into main Oct 24, 2026
41 checks passed
@vitali87
vitali87 deleted the fix/evals-ty-result-row branch October 24, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ty pre-commit hook fails on evals/cgr_graph.py:1379 (invalid-return-type), which CI never type-checks

1 participant