fix: parser extraction bugs exposed by workspace-wide test expansion#9
Open
anvanster wants to merge 491 commits into
Open
fix: parser extraction bugs exposed by workspace-wide test expansion#9anvanster wants to merge 491 commits into
anvanster wants to merge 491 commits into
Conversation
… 58 tests and fixed two real visitor bugs the new tests exposed (use-alias import path and interface extends parents).
…o 52 tests and fixed two real visitor bugs (interface-extends parent traits and enum-body method extraction) that the new tests exposed.
…to 49 tests, covering doc-comment prefixes, catch/match complexity accumulation, trait-method parenting, extends-with-constructor-args parsing, and call attribution scoping.
… 49 tests, covering doc comments on type/alias/port declarations, exposed-operator symbols, nested-case complexity, and pinning three latent visitor gaps.
…to 52 tests, covering protocol conformance splitting, required-method abstractness, doc/body_prefix on classes, additional visibility modifiers, required init, nested types, optional return types, and pinning three real visitor gaps.
…53 tests, covering struct/interface entity gaps, type aliases, unnamed params, type-switch complexity, qualified return types, and pinning three latent visitor gaps.
…o 53 tests, covering category interfaces, keyword-selector name/param handling, for-in complexity, switch-case call attribution, and pinning five real visitor gaps.
… 54 tests, covering import-relation shapes, foreach loop complexity, doc-comment joining, and pinning several real tree-sitter-tcl ERROR-parse quirks.
…o 57 tests, covering visibility/params/static detection, supertraits, tuple/array/dyn type refs, while-let complexity, skip_private config, and pinning the latent doc-comment gap.
…o 31 tests, covering multiline-value section extension, the truncation boundary just above the cap, sub-tables, final-section flush, multi-pair array-of-tables, and empty/comment-only documents.
…uite from 5 to 13 tests, covering ImportsFrom edges, the external-property unresolved fallback, direct-module-node call matching, name-based path fallback, non-call usage edge variants, mixed dead/used imports, and import-line reporting.
…te from 8 to 15 tests, covering multi-level caller depth, mutual-recursion dedup, both-direction callee-only tagging, diagnostic edge-count reporting, missing requested_line, depth-zero, and the empty-language branch.
…t suite from 7 to 14 tests, covering Imports-edge deps, Both-direction get_edges, max_symbols capping, oversized-dep source omission, one-dep-per-primary, shared-dep dedup, and non-import/call edge exclusion.
…e from 8 to 14 tests, covering signature/path propagation, default fields on bare nodes, diamond depth-2 dedup, direct-and-transitive caller single-counting, direct-caller tie-break, and limit exceeding function count.
…suite from 8 to 15 tests, covering stage-2 non-test exclusion, stage-1 precedence over same-file, transitive callee depth, path-based test detection, stage-3 non-function exclusion, the .spec pattern, and limit=0.
…st suite from 8 to 15 tests, covering the reachable-set edge guard, depth-0 bailout, diamond dedup, duplicate edges, bare-node defaults, transitive importedBy, and self-import edges.
…uite from 8 to 16 tests, covering the line_end==0 range collapse, the unknown-language fallback, the source-unavailable placeholder, code-less callers, multiple callers, stage-1 non-calling-test exclusion, Class node-type rendering, and the stage-2 non-Function guard.
…rom 9 to 16 tests, covering the Extends/Implements/Contains edge-type mappings, rename risk elevation, line/column propagation, the no-requested-line fallback, and multi-edge-between-pair impacts.
… 14 to 30 tests, adding 16 net-new end-to-end tests exercising the previously-untested get_ai_context entry point and its intent-driven traversal, dependency, sibling, architecture, imports, usage-example, and debug-hint helpers.
…m 10 to 22 tests, adding 12 end-to-end tests exercising the previously-untested find_unused_code async entry point.
…o 15 tests, covering the previously-untested project-slug, ephemeral-workspace, home-dir, and telemetry-id helpers.
…uite from 9 to 17 tests, covering previously-untested builder setters, truncate_optional, and the serde rename_all wire-format contracts.
…to 19 tests, adding 12 net-new tests covering the previously-untested hash_content, index_file, index_directory, and index_workspace runtime entry points.
…23 tests, adding 10 net-new tests covering the previously-untested HTTP handler detection, call-edge resolution, complexity propagation, import-edge props, and fallback file-node paths.
… 26 tests, adding 12 net-new tests covering the previously-untested Spring/JAX-RS HTTP annotation detection, annotation-value extraction, HTTP-handler node props, fallback file node, complexity propagation, import-edge props, method qualification/containment, and interface required_methods.
…to 23 tests, adding 9 net-new tests covering previously-untested fallback file node, complexity propagation, import-edge props, import-node dedup, method qualification/containment, interface required_methods, class type_parameters, function doc/return_type, and inheritance order prop.
…20 tests, adding 8 net-new tests covering previously-untested complexity propagation, module/function/class/method/trait doc and prop branches, orphaned parent_class containment, and the is_direct call-edge prop.
…o 30 tests, adding 10 net-new tests covering line/signature/path/visibility props, the method-vs-function prop distinction, negative inheritance/implementation edges, and import-node reuse of trait/function/file nodes.
…1 to 31 tests, adding 10 net-new tests covering free-function path/line/signature/optional/flag props, method path/line bounds, the interface-method narrower-prop divergence, trait Contains edge, direct-call is_direct, and alias-only/wildcard-only import edge props.
… 33 tests, adding 10 net-new tests covering free-function/class/trait/method path-visibility-line props, the method narrower-prop divergence, negative inheritance/implementation edges, in-file function import reuse, module-name self-loop import, and multi-function file containment.
…s (22→27) pinning the previously-untested success-path dispatch arms of handle_custom_request across the traverseGraph, getCallers, getCallees, findBySignature, and analyzeComplexity handlers.
…s (27→29) covering the last two previously-untested success-path dispatch arms of handle_custom_request: getAIContext and getDetailedSymbolInfo over a populated graph.
…t.rs (16→17) covering the previously-untested Section 5 recent-git-changes path of get_edit_context via a real temp git repo.
…text.rs (14→15) covering the previously-untested Step 2 symbol token-budget break in get_curated_context.
…erver mcp/transport.rs and added 4 net-new tests (2→6) pinning its valid-JSON, whitespace-trim, blank-line, and malformed-JSON branches.
…rs in codegraph-harness jsonrpc.rs and added 5 net-new tests (0→5) covering the previously-untested-because-IO-coupled framing and line-parse logic.
…raph-server branch_watcher.rs and added 3 net-new tests (7→10) pinning the git diff status-character classification branches.
…raph-server domain/impact.rs and added 3 net-new tests (18→21) pinning the change-type→severity mapping that was previously unreachable in unit tests.
…t_type, base_risk_level, escalate_risk_level) from the IO-coupled analyze_impact fn in codegraph-server domain/impact.rs and added 7 net-new tests (21→28) pinning all their branches.
…g the Int/Float/Bool/type-mismatch comparison arms of QueryBuilder::property() that only had String-arm coverage.
…func_map-miss (unknown-name) branches of apply_kernel_macros that prior all-match/empty-list tests never reached.
….rs (8→9) pinning the line_end_opt None branch of get_symbol_source that the existing both-fields-missing test never reached.
… pinning detect_entry_type's path-based TestEntry branch and its None fallback, both previously unreached.
… pinning symbol_weight's inverted-line-bounds saturating_sub branch that the two existing tests (end>=start or both zero) never reached.
…visit_for_complexity's finally_clause exception-handler branch that the existing catch-only test never reached.
…2) pinning visit_for_complexity's return_statement arm via the early_returns metric that no existing test asserted.
…7) pinning visit_for_complexity's elvis_expression branch that no existing complexity test reached.
…pinning visit_for_complexity's while_statement arm that only had repeat-while and for-in loop coverage.
… pinning visit_for_complexity's binary-arm word-operator sub-branch (`and`/`or`) that the existing logical-operators test (&&/|| only) never reached.
… pinning visit_for_complexity's `&&` and word-`or` operator sub-branches that the existing logical-operator tests (`and` and `||` only) never reached.
…inning five previously-untested visit_for_complexity arms: word and/or logical operators, do_statement, finally_clause, switch case/default, and match_conditional_expression.
…pinning visit_for_complexity's untested `||` and `and` operator-spelling sub-branches plus two grammar-shape gaps (pure-`and` unary_expression parse and C-style for_statement_1).
… operator sub-branches: codegraph-objc's || spelling (53→54) and codegraph-r's vectorized-| negative path (46→47).
…ning visit_for_complexity's else_clause arm and the enter_scope/exit_scope max_nesting_depth tracking, neither of which any existing test reached.
🔍 CodeGraph PR Review286 files changed (+69775/−1326, 4732 functions) · Risk: 🔴 high Blast radius8094 direct callers affected across
|
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
1 participant
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.
What Changed
Risk Assessment
✅ Low: The remaining unreviewed delta is 10 lines of test-only env-lock guards that correctly and completely close the round-3 finding; a full sweep of both codegraph-server test binaries and the codegraph-memory crate found no residual unguarded HOME/USERPROFILE readers or mutators.
Testing
Ran the full cargo workspace test suite covering all 43 crates that received net-new tests (121 binaries, 6,261 tests, all passing), then stress-tested the env-lock flakiness fix by running the codegraph-server lib and bin test binaries 10 times in a row at 16 threads with zero failures; transcripts, a per-crate breakdown of the 4,118 new tests, and a sample of the new tests passing are saved as evidence, and the worktree was left clean. No visual evidence applies since the change is test-only with no UI surface.
Evidence: Full workspace test transcript (121 binaries, 6,261 passed, 0 failed)
Evidence: Env-lock stress: 10 consecutive codegraph-server runs at 16 threads, all green
Evidence: Per-crate count of 4,118 net-new test attributes (base→target diff)
Evidence: Sample of net-new tests from recent commits shown passing
test visitor::tests::test_visitor_complexity_max_nesting_depth ... ok test visitor::tests::finally_clause_raises_complexity ... ok test visitor::tests::test_complexity_elvis_expression ... ok test visitor::tests::test_vectorized_and_not_counted ... ok test visitor::tests::test_vectorized_or_not_counted ... ok test visitor::tests::test_complexity_max_nesting_depth_grows_with_braced_bodies ... okPipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed (3) ✅
crates/codegraph-server/src/main.rs:581- Cross-module env-var race on HOME/USERPROFILE in the codegraph-server test binary: crash_phase.rs tests guard mutations with a module-local ENV_LOCK (crash_phase.rs:75), mcp/engine.rs tests use a different module-local ENV_LOCK (engine.rs:401), and main.rs's breadcrumb_roundtrip_writes_parseable_json mutates HOME/USERPROFILE with no lock at all. All three modules compile into one test binary and cargo runs tests in parallel threads, so a test can observe another test's temp HOME or restore a stale value (e.g. restoring HOME to another test's already-deleted temp dir), causing flaky failures and cross-test pollution. Fix: route all HOME/USERPROFILE mutations in this binary through one shared lock (a small pub(crate) test-support helper) instead of per-module statics.crates/codegraph-cobol/src/visitor.rs.dbg:1- Committed debug artifact: crates/codegraph-cobol/src/visitor.rs.dbg is a zero-byte leftover file (added in gnhf 224). Per the project's file-tracking policy debug artifacts must not be tracked; delete it.crates/codegraph-tcl/src/bin_probe_test.rs:1- Committed dead file: crates/codegraph-tcl/src/bin_probe_test.rs is zero-byte and referenced by nomoddeclaration anywhere in the crate (added in gnhf 250), so it is never compiled and is pure repository noise; delete it.crates/codegraph-lua/src/visitor.rs:125- This branch is not test-only despite most commit messages reading 'Added N net-new tests': ~1,400 production lines changed, including real parser behavior fixes in 16 language crates (e.g. Lua local-assignment function extraction rewrite here, Java interface-extends/enum-body extraction, Kotlin class_body/wildcard_import handling, Swift async/param/return-type detection, C#returnsfield, Python match-statement complexity, TOML nested-pair visiting and a metrics double-count fix) plus behavior-preserving seam refactors in codegraph-server and codegraph-harness. Each fix was disclosed in its own commit message and is pinned by new tests; I verified the riskiest rewrites in context and found no logic errors. Flagging so reviewers know parser extraction output and parse metrics change with this merge.🔧 Fix: Share one test env lock; drop two dead files
1 warning still open:
crates/codegraph-server/src/memory.rs:708- Residual reader-side env race the new shared test_env lock does not cover: codegraph_home_dir_ends_with_dot_codegraph calls codegraph_home_dir().unwrap() without taking crate::test_env::lock(). If it runs concurrently with crash_phase.rs's mark_without_home_is_noop (which removes both HOME and USERPROFILE while holding the lock), codegraph_home_dir() returns Err and the unwrap panics, making the test flaky. Fix: addlet _guard = crate::test_env::lock();at the top of this test. This is the only unguarded HOME reader in the lib test binary (index_state tests use ephemeral roots; the engine's HOME-reading test already locks).🔧 Fix: Guard HOME-reading memory test with shared env lock
1 warning still open:
crates/codegraph-server/src/daemon.rs:416- Residual HOME-reader race the shared test_env lock still does not cover: four daemon.rs tests (heartbeat_path_is_slug_json_under_daemons_dir at line 413, write_read_remove_round_trip, live_daemon_for_returns_fresh_and_prunes_stale, no_daemon_when_heartbeat_absent) call heartbeat_path()/daemons_dir(), which read HOME/USERPROFILE, without taking crate::test_env::lock(). They run in the same lib test binary as crash_phase's mark_without_home_is_noop (removes both vars while holding the lock) and with_isolated_home tests (repoint HOME at a temp dir, then delete it). Concurrent interleaving makes daemons_dir() return Err so the .expect("home is set in test env")/.unwrap() panics, or splits a write/read pair across two different HOME values so read() returns None and .expect("heartbeat should be readable") panics - flaky failures, and heartbeat files can leak into the deleted temp home. Fix: addlet _guard = crate::test_env::lock();at the top of these four tests (the other six daemon tests are pure in-memory and need no guard). Round 2's memory.rs fix asserted it was the only unguarded HOME reader; daemon.rs was missed.🔧 Fix: Guard all remaining HOME-reading tests with shared env lock
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
cargo test --workspace— full suite over all 43 crates: 121 test binaries, 6,261 tests passed, 0 failed, exit code 0cargo test -p codegraph-server --lib --bins -- --test-threads=16run 10 consecutive times to stress the shared test-env-lock fix for HOME-reading daemon/memory/crash-breadcrumb tests: 1,006 lib + 5 bin tests green on every runVerified specific net-new tests from recent gnhf commits (else_clause/max_nesting_depth, elvis_expression, while_statement, vectorized|negative path, finally_clause) appear asokin the workspace transcriptCounted new#[test]/#[tokio::test]attributes per crate in the base→target diff (4,118 total across all 43 crates) to confirm the coverage intentgit status --porcelainafter testing — worktree clean, no transient artifacts left behind✅ **Document** - passed
✅ No issues found.
crates/codegraph-server/src/domain/unused_code.rs:52- Pre-existing dead_code warnings: find_unused_code, FindUnusedCodeParams/Result, UnusedCodeCandidate, and six helper functions are never used outside #[cfg(test)] since the find_unused_code tool moved to the pro edition. Fixing requires deleting the staged feature code or adding #[allow(dead_code)] - a product decision, not a safe mechanical fix. Not introduced by this branch (it only added tests for these items).✅ **Push** - passed
✅ No issues found.