fix: class-scope field annotation typeMap keys to prevent cross-class collision#1495
Conversation
… diff Adds snapshot-pre-bash.sh (PreToolUse Bash) + track-bash-writes.sh (PostToolUse Bash): the pre-hook captures git status --porcelain to a per-worktree temp file before each Bash call; the post-hook diffs the before/after state and appends newly modified or created files to .claude/session-edits.log. This closes the gap where files written by sed -i, printf redirects, tee, heredocs, or build tools (Cargo.lock, lockfiles) were never recorded, causing guard-git.sh to emit false-positive BLOCKED errors. Closes #1457
- clojure.rs: annotate lifetime-anchor assignment to silence false-positive - cfg.rs: remove never-called start_line_of method - complexity.rs: remove never-constructed NotHandled variant; convert irrefutable if-let patterns to plain let destructures - dataflow.rs: remove never-read callee fields from CallReturn/Destructured - incremental.rs: remove never-read lang field from CacheEntry cargo check and cargo clippy both clean after these changes.
Adds .github/workflows/perf-canary.yml — a path-filtered workflow that fires on PRs touching src/extractors/, src/domain/graph/, or crates/** and runs only the incremental-benchmark suite (full build + no-op + 1-file rebuild, both engines). Catches the class of regressions that accumulated invisibly across the Phase 8.x PRs and were only detected at v3.12.0 publish time. The regression guard gains BENCH_CANARY=1 mode: raises thresholds to 50%/100%/150% (standard/noisy/WASM) and skips the build, query, and resolution suites — only incremental checks run. This absorbs shared- runner timing variance while still blocking catastrophic regressions (+98% full build, +1827% 1-file rebuild from v3.12.0). Closes #1433
On incremental builds, runPostNativeCha previously scanned all call→qualified-method edges in the DB (~12ms flat, O(graph size)), even for 1-file changes where no hierarchy or RTA evidence changed. Add two cheap indexed gate queries. Gate A checks whether any changed file introduced a class/interface/trait/struct/record node (hierarchy may have new implementors reachable from unchanged call sites). Gate B checks whether any changed file added a call edge to a class-kind target (RTA set may have grown, enabling previously filtered expansions in unchanged callers). If neither gate fires, restrict the candidate query to src.file IN changedFiles — safe because the hierarchy and instantiated set are unchanged for all other files. Full builds (isFullBuild=true) and cases where either gate fires retain the existing full-scan behaviour. Mirrors the changed-files scoping pattern of runPostNativeThisDispatch. Closes #1441
Times each JS post-pass in tryNativeOrchestrator and exposes the
measurements in BuildResult.phases:
- gapDetectMs — dropped-language gap detection + backfill
- chaMs — CHA expansion (interface dispatch)
- thisDispatchMs — this/super dispatch WASM re-parse (was already
tracked but now properly named alongside the rest)
- reclassifyMs — scoped role re-classification after edge insertion
- techniqueBackfillMs — technique-column UPDATE on native-written edges
Previously only thisDispatchMs was reported, causing wall-clock vs
phaseSum to diverge by 1.1s+ on 1-file rebuilds and making benchmark
regressions undiagnosable from committed history.
Updates update-incremental-report.ts to render the new phases in a
collapsible details block under each engine's 1-file rebuild section.
Closes #1434
…ld for required-tier grammars The docstring claimed pool cost was "amortised over enough parse work" — measurements show IPC overhead scales linearly (~55–64ms/file pool vs ~8–10ms/file inline). The real motivation is crash safety for exotic WASM grammars (#965); JS/TS/TSX (required-tier, used in all this-dispatch backfill calls) have never triggered the V8 fatal crash class and are safe to run inline. Raise threshold 16 → 32 to keep typical this-dispatch batches (≤ 18 files on the codegraph corpus) on the inline fast path. Exotic-language drops are almost always well under 32 files and also benefit from the inline path without meaningful crash risk increase. Closes #1435
…e incremental rebuilds On 1-file native incremental builds, two JS post-passes ran unconditionally even when they had no work to do: - `backfillNativeDroppedFiles`: called whenever changedCount > 0, even when detectDroppedLanguageGap returned an empty gap. Gate now checks gap.missingAbs.length > 0 || gap.staleRel.length > 0 directly, matching backfillNativeDroppedFiles's own internal early-exit guard. - Node/edge COUNT(*) re-count: ran unconditionally after all post-passes even when none of them wrote any edges. COUNT(*) over 50K+ edge tables is non-trivial, especially via the NativeDbProxy napi-rs round-trip. Now gated on postPassWroteData (backfill | CHA edges | this-dispatch edges). Closes #1454
The post-pass it timed (runPostNativePrototypeMethods) was deleted in b5c03a2 when func-prop extraction moved to Rust (#1432). The optional field was never set by any code path that survived the deletion. Also remove the stale reference to "prototype-methods post-pass" from the parseFilesWasmForBackfill docstring — only the this-dispatch post-pass uses symbolsOnly now. Closes #1432
… collision Field type annotations (`private repo: OrderRepository`) were seeded as bare file-wide typeMap keys, causing `this.repo` inside `UserService` to resolve to `OrderRepository` when both classes had a `repo` field (issue #1458). Both extractors (TS `handleFieldDefTypeMap` and Rust `field_definition` branch) now seed `ClassName.field` keys at confidence 0.9, matching the `CallerClass.X` resolver fallback added in PR #1382. Bare keys are kept at confidence 0.6 as fallbacks for single-class files or class expressions where no enclosing class name is available. Both engines change identically — parity preserved.
|
Claude finished @carlos-alm's task —— View job PR Review: fix: class-scope field annotation typeMap keys to prevent cross-class collision
Root Cause AnalysisProblem Being Solved: Two classes with identically-named fields (e.g. Why This Approach: The fix is architecturally correct. The resolver already has a Risk Assessment: Low. The change is additive — it inserts new keys and demotes existing ones rather than removing functionality. The Phase 2: Configuration Change DetectionNo linting rules relaxed, no TypeScript flags disabled, no thresholds lowered. Clean. Phase 3: Test IntegrityTests at
No removed assertions, no skipped tests, no weakened expectations. Phase 4: Code QualityTS extractor ( The logic is clean. One minor observation: the Rust extractor ( There is a parity gap between TS and Rust in the
This means the Rust engine, unlike the TS engine, can produce duplicate typeMap entries for the same bare key when multiple class expressions appear in a file. The edge builder presumably deduplicates by taking the highest-confidence entry, but this is a latent difference in extraction behavior worth tracking. This is not introduced by this PR — it's a pre-existing gap in how
Correctly returns Test for the class-expression Backlog Compliance
Critical Concerns
Final Recommendation
|
Greptile SummaryThis PR fixes a cross-class
Confidence Score: 5/5Safe to merge — the fix is surgical and well-contained, with both code paths (TS and Rust) updated symmetrically and covered by new unit and integration tests. Both extractors and both resolvers are updated in lock-step. The HashMap conversion in Rust already implements higher confidence wins with first wins on tie, so the new 0.9/0.6 split works correctly at resolution time without any additional changes. The class-expression fallback path (bare keys at 0.9) is preserved, preventing regressions in single-class files. The integration test validates end-to-end correctness for the reported bug scenario. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Field annotation\n`private repo: OrderRepository`"] --> B{enclosing class\ndetected?}
B -- "Yes (class_declaration)" --> C["Seed ClassName.field\n@ confidence 0.9"]
B -- "No (class expression\nor no class)" --> D["Seed bare field + this.field\n@ confidence 0.9"]
C --> E["Also seed bare field + this.field\n@ confidence 0.6 fallback"]
subgraph Resolver ["Resolver — this.repo.save() lookup"]
F{receiver starts\nwith this. AND\ncallerName available?}
F -- Yes --> G["Look up ClassName.prop\nin typeMap FIRST"]
G -- hit --> H["Resolve to correct\nper-class type ✓"]
G -- miss --> I["Fall back to bare keys\n(effectiveReceiver, receiver,\nrest-param key)"]
F -- No --> I
end
E --> F
D --> F
Reviews (6): Last reviewed commit: "fix: resolve merge conflicts with main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('prevents cross-class collision for same-named fields (issue #1458)', () => { | ||
| const symbols = parseTS(` | ||
| class OrderService { | ||
| private repo: OrderRepository; | ||
| } | ||
| class UserService { | ||
| private repo: UserRepository; | ||
| } | ||
| `); | ||
| // Each class gets its own scoped key — no collision. | ||
| expect(symbols.typeMap.get('OrderService.repo')).toEqual({ type: 'OrderRepository', confidence: 0.9 }); | ||
| expect(symbols.typeMap.get('UserService.repo')).toEqual({ type: 'UserRepository', confidence: 0.9 }); | ||
| // Bare "repo" key should hold the first class's type at 0.6 (second write is same confidence, no overwrite). | ||
| expect(symbols.typeMap.get('repo')?.confidence).toBe(0.6); | ||
| }); |
There was a problem hiding this comment.
The new prevents cross-class collision test verifies that the correct keys are seeded in typeMap, but it doesn't exercise the resolver. Because the resolver checks bare repo before ClassName.repo, an end-to-end test that calls resolveCallEdges with callerName = "UserService.run" and asserts the resolved type is UserRepository (not OrderRepository) would immediately expose the gap described above. Without a resolver-level assertion, this test can pass while the original bug remains active in production.
There was a problem hiding this comment.
Fixed. The resolver (call-resolver.ts) now checks the class-scoped key (ClassName.prop) before bare fallback keys for this. receivers. Added integration test issue-1458-cross-class-field-typemap.test.ts that calls buildGraph with both classes in the same file and asserts OrderService.run → OrderRepository.save and UserService.run → UserRepository.save, with no false cross-class edges.
| match enclosing_type_map_class(node, source) { | ||
| Some(class_name) => { | ||
| // Primary: class-scoped key prevents cross-class collision. | ||
| push_type_map_entry( | ||
| symbols, | ||
| format!("{}.{}", class_name, field_name), | ||
| type_name.to_string(), | ||
| ); | ||
| // Fallback bare keys at lower confidence. | ||
| symbols.type_map.push(TypeMapEntry { | ||
| name: field_name.clone(), | ||
| type_name: type_name.to_string(), | ||
| confidence: 0.6, | ||
| }); | ||
| symbols.type_map.push(TypeMapEntry { | ||
| name: format!("this.{}", field_name), | ||
| type_name: type_name.to_string(), | ||
| confidence: 0.6, | ||
| }); | ||
| } | ||
| None => { | ||
| // No enclosing class declaration (e.g. class expression) | ||
| // — use bare keys only at full confidence. | ||
| push_type_map_entry(symbols, field_name.clone(), type_name.to_string()); | ||
| push_type_map_entry(symbols, format!("this.{}", field_name), type_name.to_string()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The TS test file gets two new cases for the collision scenario, but no parallel Rust unit test is added. The existing tests in javascript.rs verify the ClassName.field key is seeded (e.g., line 3671 A.t, line 3722 C.helper), but none cover the multi-class scenario where two classes define the same field name. A minimal test mirroring the new TS prevents cross-class collision case would confirm parity between the two extractors.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Added field_annotation_multi_class_seeds_separate_scoped_keys Rust unit test in crates/codegraph-core/src/extractors/javascript.rs. It parses two classes with identically-named fields and asserts OrderService.repo → OrderRepository @ 0.9 and UserService.repo → UserRepository @ 0.9 are both present as separate entries — mirroring the TS collision test.
There was a problem hiding this comment.
The test was using parse_js() (tree-sitter-javascript) but the fixture uses TypeScript-specific syntax (private field declarations with type annotations). The JS grammar doesn't support type annotations so the type_map came back empty, causing the assertion to fail on CI. Fixed by adding parse_ts() using LANGUAGE_TYPESCRIPT and switching the test to it. All 396 Rust unit tests now pass locally.
Codegraph Impact Analysis8 functions changed → 33 callers affected across 6 files
|
…nce threshold constant
The secondary docstring still described the old 16-value rationale
("engine-parity drop sizes"). Replace with a pointer to
INLINE_BACKFILL_THRESHOLD where the full rationale now lives.
…r this. receivers When handleFieldDefTypeMap seeds ClassName.field at 0.9 and bare field/ this.field at 0.6, the resolver was still finding the bare 0.6 entry first (effectiveReceiver lookup) and skipping the class-scoped check entirely. The result: cross-class collision from issue #1458 persisted at runtime even though the extractor emitted the right keys. Fix: for this. receivers, check the class-scoped key (ClassName.prop) first, then fall back to bare keys. Both TS (call-resolver.ts) and Rust (build_edges.rs) resolvers are updated identically. Fixes #1458.
…-end integration test - Rust: field_annotation_multi_class_seeds_separate_scoped_keys confirms that two classes with identically-named fields produce separate class-scoped typeMap keys at confidence 0.9 (mirrors the TS prevents-cross-class-collision test). - Integration: issue-1458-cross-class-field-typemap.test.ts exercises the full buildGraph → resolver path (WASM engine) and asserts that OrderService.run resolves to OrderRepository.save and UserService.run to UserRepository.save, with no cross-class false edges.
|
Addressed Greptile's findings: Root cause confirmed: The resolver checked bare Fixes pushed:
Lint: was failing due to Biome line-length formatting on the new test assertions — already fixed in the original commit; confirmed passing now. All 4 new integration test assertions pass. Rust compiles clean ( |
The field_annotation_multi_class_seeds_separate_scoped_keys test used parse_js() (tree-sitter-javascript), but the test fixture uses TypeScript syntax (private field declarations with type annotations). The JS grammar does not support type annotations, so the type_map came back empty and the assertion failed. Add parse_ts() helper using LANGUAGE_TYPESCRIPT and switch the test to it. All 396 Rust unit tests pass.
|
Fix for CI failures: The Fixed by adding a Out-of-scope findings from the Claude review filed as GitHub issues:
|
|
CI fix (commit 76a4380): the Rust multi-class collision test was using |
1 participant
Field type annotations (
private repo: OrderRepository) were seeded as bare file-wide typeMap keys, causingthis.repoinsideUserServiceto resolve toOrderRepositorywhen both classes had arepofield.Both extractors (TS
handleFieldDefTypeMapand Rustfield_definitionbranch) now seedClassName.fieldkeys at confidence 0.9, matching theCallerClass.Xresolver fallback added in PR #1382. Bare keys are kept at confidence 0.6 as fallbacks for single-class files or class expressions where no enclosing class name is available.Both engines change identically — parity preserved.
Closes #1458