Skip to content

fix: close engine parity gap between native and WASM#309

Merged
carlos-alm merged 1 commit intomainfrom
fix/engine-parity-292
Mar 4, 2026
Merged

fix: close engine parity gap between native and WASM#309
carlos-alm merged 1 commit intomainfrom
fix/engine-parity-292

Conversation

@carlos-alm
Copy link
Contributor

Summary

Closes the engine parity gap reported in the v3.0.0 dogfood report (Section 5). Native produced +89 nodes / +75 edges more than WASM due to two independent bugs:

  • WASM missed constants: The query-based fast path (extractSymbolsQuery) had no pattern for lexical_declaration/variable_declaration, so const X = <literal> was never detected. Added extractConstantsWalk() — a targeted walk of top-level children that fills this gap without a full tree traversal.
  • Native missed destructured TS params: extract_js_parameters in Rust only checked pattern.kind() == "identifier", missing shorthand_property_identifier_pattern (e.g. { name, age }). Added fallback chain (patternleftchild(0)) and the missing kind check.
  • Native is_js_literal incomplete: Was missing array, object, unary_expression, binary_expression, new_expression — now aligned with WASM's isConstantValue().

Also removed the EXTENDED kind filter from build-parity.test.js that was masking these bugs — all node kinds are now compared directly.

Test plan

  • npx vitest run tests/engines/parity.test.js — 10 passed (new constant + destructured param cases)
  • npx vitest run tests/integration/build-parity.test.js — 2 passed (no filtered kinds)
  • npx vitest run tests/integration/build.test.js — 16 passed
  • npx vitest run tests/parsers/javascript.test.js — 20 passed
  • npm run lint — clean (1 pre-existing warning)

WASM query path missed constants (no query pattern for
lexical_declaration) — add targeted extractConstantsWalk() to
extractSymbolsQuery(). Native missed destructured TS params
(only checked "identifier", not "shorthand_property_identifier_pattern")
— add fallback chain and kind check in extract_js_parameters(). Also
align is_js_literal() with WASM's isConstantValue() (add array, object,
unary/binary/new_expression).

Remove EXTENDED kind filters from build-parity test so all node kinds
are compared directly. Add constant and destructured-param parity test
cases.

Impact: 4 functions changed, 4 affected
@claude
Copy link

claude bot commented Mar 4, 2026

Claude finished @carlos-alm's task —— View job


PR Review: Engine Parity Fix

Reviewing this PR using the 4-phase methodology for codegraph engine parity fixes.

Todo List

  • Read CLAUDE.md for repo-specific guidelines
  • Phase 1: Root Cause Analysis - understand the problem being solved
  • Phase 2: Configuration Change Detection - check for high-risk changes
  • Phase 3: Test Integrity - verify tests aren't weakened
  • Phase 4: Code Quality Review - examine implementation details
  • Provide final rating and recommendation

Root Cause Analysis ✅

Problem Being Solved: Legitimate engine parity gaps between native Rust and WASM implementations. The v3.0.0 dogfood report showed native produced +89 nodes / +75 edges more than WASM due to three distinct bugs:

  1. WASM missed constants: Query-based extraction (extractSymbolsQuery) had no patterns for lexical_declaration/variable_declaration, so const X = literal was never detected
  2. Native missed destructured TS params: Only checked pattern.kind() == "identifier", missing shorthand_property_identifier_pattern for { name, age } patterns
  3. Native is_js_literal incomplete: Missing array, object, unary_expression, binary_expression, new_expression compared to WASM's isConstantValue()

Why This Approach: The fixes are targeted and surgical:

  • WASM: Add extractConstantsWalk() - targeted walk of top-level children only, not full tree traversal
  • Native: Add fallback chain for parameter extraction and expand literal detection
  • Tests: Remove masking filters to enable direct validation

Risk Assessment: ✅ Low risk. This fixes actual bugs rather than masking symptoms.

Configuration Change Detection ✅

Analysis: No high-risk configuration changes detected:

  • ❌ No biome/linting rule modifications
  • ❌ No test timeout increases or coverage threshold reductions
  • ❌ No build configuration changes
  • ❌ No TypeScript strict mode disabling

The only "configuration" change is removing the EXTENDED kind filter in build-parity.test.js, which strengthens rather than weakens validation by comparing all node kinds directly instead of filtering some out.

Test Integrity ✅

Analysis: Tests are strengthened, not weakened:

Positive Changes:

  • Removed masking filters: build-parity.test.js previously filtered out parameter, property, constant kinds with comment "WASM extracts these as children but native engine defers child extraction for now" - this was masking the actual bugs
  • Direct comparison: Now compares all node kinds and edges without exclusions: expect(nativeGraph.nodes).toEqual(wasmGraph.nodes)
  • New test cases added: parity.test.js gains specific test cases for constants and destructured parameters

Validation:

  • Tests now validate the actual fixes rather than working around known gaps
  • PR description shows comprehensive test coverage: 5 test suites, 37 total test cases
  • No weakened assertions or conditional logic

Conclusion: This is exactly what good testing looks like - removing workarounds when the underlying issue is fixed.

Code Quality Review ✅

JavaScript WASM Extractor (src/extractors/javascript.js):

extractConstantsWalk() - Well-designed:

  • Efficient: Only walks top-level children, not full tree traversal
  • Precise: Handles export const by unwrapping to declaration child
  • Safe: Multiple validation checks (!node, type checks, startsWith('const '))
  • Consistent: Uses same nodeEndLine() and line numbering as rest of codebase
  • No false positives: Explicitly skips functions already captured by query patterns

Implementation Quality:

// Handle `export const …` — unwrap the export_statement to its declaration child
if (node.type === 'export_statement') {
  const inner = node.childForFieldName('declaration');
  if (!inner) continue;
  declNode = inner;
}

This correctly handles both const X = 1 and export const X = 1 cases.

Rust Native Extractor (crates/codegraph-core/src/extractors/javascript.rs):

✅ Parameter extraction fix - Robust:

let name_node = child.child_by_field_name("pattern")
    .or_else(|| child.child_by_field_name("left"))
    .or_else(|| child.child(0));
if let Some(name_node) = name_node {
    if name_node.kind() == "identifier"
        || name_node.kind() == "shorthand_property_identifier_pattern"
  • Defensive: Proper fallback chain for edge cases
  • Targeted: Specific fix for shorthand_property_identifier_pattern
  • Safe: Maintains existing identifier check

is_js_literal() expansion - Correct alignment:

matches!(node.kind(),
    "number" | "string" | "true" | "false" | "null" | "undefined"
    | "template_string" | "regex" | "array" | "object"
    | "unary_expression" | "binary_expression" | "new_expression"
)

Now perfectly matches WASM's isConstantValue() function.

Security & Performance:

  • No security issues: No command injection, SQL injection, or unsafe operations
  • No performance regressions: extractConstantsWalk() is O(n) where n = top-level children only
  • No memory issues: Proper Option/Result handling in Rust, safe node access in JS

Critical Concerns

None identified. This is high-quality, well-tested code that fixes real bugs without introducing technical debt.

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: Three legitimate engine parity bugs identified through dogfooding
**Why This Approach**: Surgical fixes targeting root causes, not symptoms  
**Risk Assessment**: Low risk - fixes real bugs, strengthens test validation

## Critical Concerns (if any)
None. Exemplary implementation quality.

## Final Recommendation
- Rating: ⭐⭐⭐⭐⭐ (5/5)
- Action: APPROVE
- Reasoning: This PR demonstrates best practices - identified real bugs through dogfooding, implemented targeted fixes, removed test workarounds when underlying issues were resolved, and added comprehensive test coverage. The implementation quality is excellent with defensive programming and proper edge case handling.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Greptile Summary

Closed three independent parity bugs between native (Rust) and WASM (JavaScript) engines that caused native to produce +89 nodes / +75 edges more than WASM.

Fixes implemented:

  • WASM: Added extractConstantsWalk() to detect const X = <literal> declarations that query patterns missed
  • Native: Added support for shorthand_property_identifier_pattern in parameter extraction to handle destructured TypeScript params like { name, age }
  • Native: Expanded is_js_literal() to include array, object, unary_expression, binary_expression, new_expression — matching WASM's isConstantValue()
  • Tests: Removed the EXTENDED kind filter from build-parity.test.js that was masking these bugs

All test suites pass, including new test cases for constants and destructured parameters.

Confidence Score: 5/5

  • This PR is safe to merge with no concerns
  • Three targeted bug fixes with comprehensive test coverage, no breaking changes, and all tests passing. The changes achieve exact parity between engines by aligning their logic
  • No files require special attention

Important Files Changed

Filename Overview
crates/codegraph-core/src/extractors/javascript.rs Fixed two native engine bugs: added shorthand_property_identifier_pattern for destructured TS params, and expanded is_js_literal to match WASM's isConstantValue
src/extractors/javascript.js Added extractConstantsWalk to capture const declarations with literal values that query patterns missed
tests/engines/parity.test.js Added test cases for constants and destructured parameters to verify the fixes
tests/integration/build-parity.test.js Removed EXTENDED kind filter that was masking bugs, now compares all node kinds directly

Last reviewed commit: ce91999

@carlos-alm carlos-alm merged commit 52d6dcc into main Mar 4, 2026
20 checks passed
@carlos-alm carlos-alm deleted the fix/engine-parity-292 branch March 4, 2026 02:48
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant