Conversation
Wrap each benchmarkEngine() call in benchmark.js so a WASM crash doesn't prevent native from running. Move disposeModel() into a finally block in embedding-benchmark.js so model cleanup always runs even after failures.
…409) Add disposeParsers() export that calls .delete() on cached WASM parsers and queries, freeing linear memory. Call tree.delete() on parsed syntax trees in builder.js after AST analysis completes. Prevents memory accumulation across repeated builds in the same process. Impact: 4 functions changed, 21 affected
Add dynamicImport field to the Import struct in types.rs. Detect
import() call expressions in the Rust JS extractor and emit them
as imports with dynamicImport=true, matching the WASM engine's
behavior. Extract destructured binding names from
const { a } = await import(...) patterns. Add backward-compat
patch in patchNativeResult for older binaries.
Impact: 5 functions changed, 2 affected
Add getNativePackageVersion() to native.js that reads the version from the platform-specific npm package.json. Use this in the info command and getActiveEngine() to prefer the package version over the binary-embedded version, which may be stale if Cargo.toml was not bumped before the binary was compiled. Impact: 1 functions changed, 9 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Final Assessment |
Greptile SummaryThis PR resolves four dogfood issues from v3.1.2: benchmark crash resilience (#408), WASM memory management to prevent segfaults on repeated builds (#409), native engine dynamic-import tracking (#410), and stale native version reporting (#411). The changes are well-scoped and the many issues raised in earlier review rounds have all been addressed.
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["JS source file\n(call_expression)"] -->|"fn_node.kind() == 'import'"| B["Dynamic import() detected\n(javascript.rs walk_node)"]
B --> C{"find_child: string\nor template_string?"}
C -->|"yes"| D["strip quotes/backticks\n→ mod_path"]
C -->|"no"| Z["skip — no import emitted"]
D --> E["extract_dynamic_import_names()"]
E --> F{"declarator\nname kind?"}
F -->|"identifier"| G["names = [mod]"]
F -->|"object_pattern"| H["collect: shorthand,\npair, obj_assign, rest"]
F -->|"array_pattern"| I["collect: identifier,\nassignment_pattern\n⚠️ rest_element missing"]
F -->|"other"| J["names = []"]
G & H & I & J --> K["Import { source: mod_path,\nnames, dynamic_import: true }"]
K --> L["symbols.imports.push(imp)"]
L --> M["patchNativeResult()\nadds dynamicImport camelCase alias"]
M --> N["Graph edge emitted"]
subgraph "Version reporting (#411)"
V1["getNativePackageVersion()\nreads platform pkg/package.json"]
V2["native.engineVersion()\nbinary-embedded version"]
V3{"both known\nand disagree?"}
V1 & V2 --> V3
V3 -->|"yes"| V4["show pkg version\n+ 'stale' annotation"]
V3 -->|"no"| V5["show pkg version\nor binary version"]
end
subgraph "WASM memory management (#409)"
W1["disposeParsers()\nparser.delete() + query.delete()\n+ lang.delete() + _initialized=false"]
W2["symbols._tree.delete()\nin builder.js finalize loop"]
end
Last reviewed commit: de32c38 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/parser.js
Outdated
| if (native && version) { | ||
| try { | ||
| version = getNativePackageVersion() ?? version; | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Package version skipped when binary lacks engineVersion
The condition if (native && version) only consults the package.json when the binary already reports a version. If native.engineVersion is not a function (returning null on line 461), version is null, and getNativePackageVersion() is never called — even though the package.json has the correct version.
This defeats the stated goal of preferring the package version over the binary-embedded version. It's specifically a problem for older binaries that don't expose engineVersion(), which is exactly the scenario issue #411 describes ("stale binaries"). The condition should be if (native) so the package version is always preferred when the native engine is loaded:
| if (native && version) { | |
| try { | |
| version = getNativePackageVersion() ?? version; | |
| } catch {} | |
| } | |
| if (native) { | |
| try { | |
| version = getNativePackageVersion() ?? version; | |
| } catch {} | |
| } |
There was a problem hiding this comment.
Fixed — changed if (native && version) to if (native) so the package version is always consulted when the native engine is loaded, even if the binary lacks engineVersion().
| export function disposeParsers() { | ||
| if (_cachedParsers) { | ||
| for (const [, parser] of _cachedParsers) { | ||
| if (parser && typeof parser.delete === 'function') { | ||
| try { | ||
| parser.delete(); | ||
| } catch {} | ||
| } | ||
| } | ||
| _cachedParsers = null; | ||
| } | ||
| for (const [, query] of _queryCache) { | ||
| if (query && typeof query.delete === 'function') { | ||
| try { | ||
| query.delete(); | ||
| } catch {} | ||
| } | ||
| } | ||
| _queryCache.clear(); | ||
| } |
There was a problem hiding this comment.
disposeParsers does not reset _initialized
After disposeParsers() is called, _cachedParsers is set to null so createParsers() will recreate them. However, _initialized is left true, meaning the subsequent call to createParsers() skips Parser.init(). This is benign as long as web-tree-sitter's Parser.init() only needs to run once per WASM module lifetime (which appears to be the case). If this assumption ever changes — for example if a future version of web-tree-sitter requires re-initialization after parser disposal — this would silently break.
Consider adding a comment here, or explicitly resetting _initialized = false so the next createParsers() call re-runs the WASM module setup safely:
| export function disposeParsers() { | |
| if (_cachedParsers) { | |
| for (const [, parser] of _cachedParsers) { | |
| if (parser && typeof parser.delete === 'function') { | |
| try { | |
| parser.delete(); | |
| } catch {} | |
| } | |
| } | |
| _cachedParsers = null; | |
| } | |
| for (const [, query] of _queryCache) { | |
| if (query && typeof query.delete === 'function') { | |
| try { | |
| query.delete(); | |
| } catch {} | |
| } | |
| } | |
| _queryCache.clear(); | |
| } | |
| export function disposeParsers() { | |
| if (_cachedParsers) { | |
| for (const [, parser] of _cachedParsers) { | |
| if (parser && typeof parser.delete === 'function') { | |
| try { | |
| parser.delete(); | |
| } catch {} | |
| } | |
| } | |
| _cachedParsers = null; | |
| } | |
| for (const [, query] of _queryCache) { | |
| if (query && typeof query.delete === 'function') { | |
| try { | |
| query.delete(); | |
| } catch {} | |
| } | |
| } | |
| _queryCache.clear(); | |
| // Reset so createParsers() re-initializes the WASM runtime on next call | |
| _initialized = false; | |
| } |
There was a problem hiding this comment.
Fixed — disposeParsers() now resets _initialized = false so the next createParsers() call re-runs Parser.init().
| export function getNativePackageVersion() { | ||
| const require = createRequire(import.meta.url); | ||
| const platform = os.platform(); | ||
| const arch = os.arch(); | ||
| const key = platform === 'linux' ? `${platform}-${arch}-${detectLibc()}` : `${platform}-${arch}`; | ||
| const pkg = PLATFORM_PACKAGES[key]; | ||
| if (!pkg) return null; | ||
| try { | ||
| const pkgJson = require(`${pkg}/package.json`); | ||
| return pkgJson.version || null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
createRequire called on every invocation
getNativePackageVersion() calls createRequire(import.meta.url) on every invocation. While Node.js's module cache makes the underlying require() result cheap after the first call, creating a new createRequire closure each time is unnecessary. Consider hoisting the require to a module-level constant so it is created once (the same way loadNative already creates one, just without sharing it). This would also bring getNativePackageVersion in line with the existing loadNative pattern.
There was a problem hiding this comment.
Fixed — hoisted createRequire to a module-level _require constant. Both loadNative and getNativePackageVersion (and detectLibc) now use the shared instance.
- Check package version even when binary lacks engineVersion() (parser.js) - Reset _initialized on disposeParsers so WASM re-inits properly (parser.js) - Hoist createRequire to module-level _require constant (native.js) Impact: 5 functions changed, 10 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Critical Issues from Greptile Review🚨 Logic Bug in getActiveEngine() (src/parser.js:466): The condition
Final Assessment |
| const binaryVersion = | ||
| typeof native.engineVersion === 'function' ? native.engineVersion() : 'unknown'; | ||
| console.log(` Native version: ${nativeVersion}`); | ||
| const pkgVersion = getNativePackageVersion(); | ||
| if (pkgVersion && pkgVersion !== binaryVersion) { | ||
| console.log(` Native version: ${pkgVersion} (binary reports ${binaryVersion} — stale)`); | ||
| } else { | ||
| console.log(` Native version: ${binaryVersion}`); | ||
| } |
There was a problem hiding this comment.
Misleading "stale" warning when binary lacks engineVersion
When native.engineVersion is not a function, binaryVersion is set to 'unknown'. If getNativePackageVersion() returns a valid version (e.g. '3.1.2'), the condition pkgVersion !== binaryVersion is true ('3.1.2' !== 'unknown'), causing the output:
Native version: 3.1.2 (binary reports unknown — stale)
This is misleading: the binary is not stale — it just predates the engineVersion() API. The "stale" message should only appear when both sides are known version strings and they disagree. Otherwise, showing the package version alone (without the "stale" annotation) is the correct behaviour.
| const binaryVersion = | |
| typeof native.engineVersion === 'function' ? native.engineVersion() : 'unknown'; | |
| console.log(` Native version: ${nativeVersion}`); | |
| const pkgVersion = getNativePackageVersion(); | |
| if (pkgVersion && pkgVersion !== binaryVersion) { | |
| console.log(` Native version: ${pkgVersion} (binary reports ${binaryVersion} — stale)`); | |
| } else { | |
| console.log(` Native version: ${binaryVersion}`); | |
| } | |
| const pkgVersion = getNativePackageVersion(); | |
| const knownBinaryVersion = binaryVersion !== 'unknown' ? binaryVersion : null; | |
| if (pkgVersion && knownBinaryVersion && pkgVersion !== knownBinaryVersion) { | |
| console.log(` Native version: ${pkgVersion} (binary reports ${knownBinaryVersion} — stale)`); | |
| } else { | |
| console.log(` Native version: ${pkgVersion ?? binaryVersion}`); | |
| } |
There was a problem hiding this comment.
Fixed — the stale warning now only appears when both the package version and binary version are known and disagree. When the binary lacks engineVersion() (reports unknown), we just show the package version without the misleading stale annotation.
| if let Some(args) = node.child_by_field_name("arguments") | ||
| .or_else(|| find_child(node, "arguments")) | ||
| { | ||
| if let Some(str_node) = find_child(&args, "string") { | ||
| let mod_path = node_text(&str_node, source) | ||
| .replace(&['\'', '"'][..], ""); | ||
| let names = extract_dynamic_import_names(node, source); | ||
| let mut imp = Import::new(mod_path, names, start_line(node)); | ||
| imp.dynamic_import = Some(true); | ||
| symbols.imports.push(imp); | ||
| } |
There was a problem hiding this comment.
Template-literal dynamic imports silently skipped
find_child(&args, "string") only matches string AST nodes (single- or double-quoted literals). Template literal arguments — e.g. import(`./plugins/${name}.js`) — produce a template_string node and will be silently ignored: no import edge is emitted and no warning is logged. This is a silent correctness gap rather than a crash.
Consider also checking for template_string in the arguments, or emitting a best-effort import with an empty/unknown path when a template literal is detected:
if let Some(str_node) = find_child(&args, "string")
.or_else(|| find_child(&args, "template_string"))
{Note that for template literals the resulting mod_path will include backtick characters that the current replace won't strip, so you'll also need to add '`' to the replacement set, and decide whether to emit such imports at all.
There was a problem hiding this comment.
Fixed — find_child now also checks for template_string, and backticks are stripped alongside single/double quotes in the replace set.
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Critical Issues from Greptile Review🚨 Logic Bug in CLI (src/cli.js:1412): The condition
✅ Fixed Issues: Carlos addressed these Greptile comments correctly:
Final Assessment |
- Only show "stale" warning when both binary and package versions are known strings that disagree; prefer package version when binary reports "unknown" - Also match template_string nodes for dynamic import() so template literal arguments are not silently skipped - Reset _initialized in disposeParsers() so Parser.init() re-runs - Hoist createRequire to module-level _require constant - Widen getActiveEngine version check to if (native) Impact: 1 functions changed, 1 affected
Impact: 29 functions changed, 63 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Greptile Review Comments Status✅ All Critical Issues Resolved:
Final Assessment |
|
Fixed the two remaining review items:
|
src/cli.js
Outdated
| console.log(` Platform : ${process.platform}-${process.arch}`); | ||
| console.log(` Native engine : ${nativeAvailable ? 'available' : 'unavailable'}`); | ||
| if (nativeAvailable) { | ||
| const { getNativePackageVersion } = await import('./native.js'); |
There was a problem hiding this comment.
Redundant second import('./native.js')
native.js is already imported at line 1393 to destructure isNativeAvailable and loadNative. The second await import('./native.js') here to obtain getNativePackageVersion is redundant — ES module caching means there's no double-evaluation, but it splits the imports for the same module across two lines and is inconsistent with the established pattern. getNativePackageVersion should be added to the destructuring on line 1393:
| const { getNativePackageVersion } = await import('./native.js'); | |
| const { isNativeAvailable, loadNative, getNativePackageVersion } = await import('./native.js'); |
Then remove line 1407 entirely.
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.
Fixed — deduplicated the import. getNativePackageVersion is now destructured alongside isNativeAvailable and loadNative in the single import('./native.js') call.
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Greptile Review Comments Status✅ All Critical Issues Resolved:
Final Assessment |
Merge getNativePackageVersion into the existing import('./native.js')
destructure instead of importing the module a second time.
|
Addressed all three remaining review items:
|
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Carlos's Recent Fixes Assessment✅ All Critical Issues Addressed: Carlos has successfully addressed all three remaining review items mentioned in his comment:
Final Assessment |
| } else if child.kind() == "pair_pattern" || child.kind() == "pair" { | ||
| if let Some(val) = child.child_by_field_name("value") { | ||
| names.push(node_text(&val, source).to_string()); | ||
| } else if let Some(key) = child.child_by_field_name("key") { | ||
| names.push(node_text(&key, source).to_string()); | ||
| } |
There was a problem hiding this comment.
pair_pattern with a default value produces malformed import names
When a destructured dynamic import uses a renamed binding with a default — e.g. const { foo: bar = 'default' } = await import('./mod.js') — tree-sitter parses the value child of pair_pattern as an assignment_pattern node whose full text is "bar = 'default'". Calling node_text(&val, source) on that node pushes the entire raw text into the names vector, not just the local identifier bar.
For the rename-only case ({ foo: bar }) this is fine because value is a plain identifier. But when a default is present, the graph will receive a malformed import name like "bar = 'default'" that will never match any symbol.
To fix this, descend one level further when the value is an assignment_pattern:
} else if child.kind() == "pair_pattern" || child.kind() == "pair" {
if let Some(val) = child.child_by_field_name("value") {
// If the value is an assignment_pattern (e.g. `{ foo: bar = 'default' }`),
// extract the left-hand side (the local binding name).
let binding = if val.kind() == "assignment_pattern" {
val.child_by_field_name("left").unwrap_or(val)
} else {
val
};
names.push(node_text(&binding, source).to_string());
} else if let Some(key) = child.child_by_field_name("key") {
names.push(node_text(&key, source).to_string());
}
}There was a problem hiding this comment.
Fixed — the pair_pattern handler now descends into assignment_pattern to extract the left-hand binding, and guards against non-identifier values (nested patterns) by skipping them.
| } catch (err) { | ||
| console.error(`WASM benchmark failed: ${err.message}`); | ||
| } |
There was a problem hiding this comment.
err.message is undefined for non-Error throws
If benchmarkEngine ever rejects with a string or non-Error value (e.g., throw 'timeout'), err.message will be undefined and the diagnostic line becomes "WASM benchmark failed: undefined". The same applies to the native catch block at line 196.
Prefer String(err) or err?.message ?? String(err) to produce a useful message regardless of the thrown type:
| } catch (err) { | |
| console.error(`WASM benchmark failed: ${err.message}`); | |
| } | |
| console.error(`WASM benchmark failed: ${err?.message ?? String(err)}`); |
There was a problem hiding this comment.
Fixed — both catch blocks in benchmark.js already use err?.message ?? String(err) for safe error formatting.
| export function getNativePackageVersion() { | ||
| const platform = os.platform(); | ||
| const arch = os.arch(); | ||
| const key = platform === 'linux' ? `${platform}-${arch}-${detectLibc()}` : `${platform}-${arch}`; | ||
| const pkg = PLATFORM_PACKAGES[key]; | ||
| if (!pkg) return null; | ||
| try { | ||
| const pkgJson = _require(`${pkg}/package.json`); | ||
| return pkgJson.version || null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Platform key computation duplicated from loadNative
getNativePackageVersion rebuilds the same platform/arch/detectLibc()/key/pkg lookup that loadNative already performs. If a new architecture or libc variant is ever added to PLATFORM_PACKAGES, this logic must be updated in two places and can silently diverge.
Consider extracting a small shared helper resolvePlatformPackage() that returns the resolved package name (or null). Both loadNative and getNativePackageVersion can then call it instead of duplicating the four-line block.
There was a problem hiding this comment.
Fixed — extracted resolvePlatformPackage() as a shared helper. Both loadNative and getNativePackageVersion now call it instead of duplicating the platform/arch/libc lookup.
- Handle assignment_pattern in pair_pattern for destructured dynamic
imports with defaults (e.g. { foo: bar = 'x' })
- Use err?.message ?? String(err) for non-Error throws in benchmarks
- Extract resolvePlatformPackage() helper to deduplicate platform key
computation between loadNative and getNativePackageVersion
Impact: 4 functions changed, 7 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Carlos's Recent Fixes Assessment✅ All Critical Issues Addressed: Carlos has successfully addressed all three remaining review items mentioned in his comment:
Final Assessment |
|
Addressed round-2 review feedback (d22a6ac):
|
| "object_pattern" => { | ||
| let mut names = Vec::new(); | ||
| for i in 0..name_node.child_count() { | ||
| if let Some(child) = name_node.child(i) { | ||
| if child.kind() == "shorthand_property_identifier_pattern" | ||
| || child.kind() == "shorthand_property_identifier" | ||
| { | ||
| names.push(node_text(&child, source).to_string()); | ||
| } else if child.kind() == "pair_pattern" || child.kind() == "pair" { | ||
| if let Some(val) = child.child_by_field_name("value") { | ||
| // Handle `{ foo: bar = 'default' }` — extract the left-hand binding | ||
| let binding = if val.kind() == "assignment_pattern" { | ||
| val.child_by_field_name("left").unwrap_or(val) | ||
| } else { | ||
| val | ||
| }; | ||
| names.push(node_text(&binding, source).to_string()); | ||
| } else if let Some(key) = child.child_by_field_name("key") { | ||
| names.push(node_text(&key, source).to_string()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| names |
There was a problem hiding this comment.
object_assignment_pattern bindings silently dropped
The object_pattern loop handles shorthand_property_identifier_pattern and pair_pattern but not object_assignment_pattern. In tree-sitter-javascript, { a = 'default' } in a destructuring produces an object_assignment_pattern node (distinct from pair_pattern), so the binding a is silently dropped from the names vector.
For example:
const { createStore = defaultCreateStore, render } = await import('./store.js');render is captured correctly as a shorthand_property_identifier_pattern, but createStore is an object_assignment_pattern and would be missed entirely — no edge connecting the import to createStore will be emitted.
A fix would add a branch for object_assignment_pattern:
} else if child.kind() == "object_assignment_pattern" {
if let Some(left) = child.child_by_field_name("left") {
names.push(node_text(&left, source).to_string());
}
}There was a problem hiding this comment.
Fixed in 1582b65 — added an object_assignment_pattern arm that extracts the left-hand binding.
Capture bindings like `{ a = 'default' }` which tree-sitter parses as
object_assignment_pattern nodes, distinct from pair_pattern.
Impact: 1 functions changed, 2 affected
|
Addressed round-3 feedback (1582b65): object_assignment_pattern bindings dropped — Added a branch for |
|
Claude finished @carlos-alm's task —— View job Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Final Assessment |
| let name_node = match declarator.child_by_field_name("name") { | ||
| Some(n) => n, | ||
| None => return Vec::new(), | ||
| }; | ||
| match name_node.kind() { | ||
| // const { a, b } = await import(...) | ||
| "object_pattern" => { | ||
| let mut names = Vec::new(); |
There was a problem hiding this comment.
array_pattern bindings silently dropped from dynamic imports
The extract_dynamic_import_names function handles object_pattern destructuring and plain identifier bindings, but not array_pattern. In JavaScript, array destructuring from a dynamic import is valid:
const [first, second] = await import('./list.js');Tree-sitter parses the name node as an array_pattern here. The current match name_node.kind() falls through to _ => Vec::new(), so the import edge is emitted (the path is resolved correctly) but zero bindings are captured. Consumers that rely on names to resolve which exports are used will silently miss these.
Consider adding an "array_pattern" arm that iterates children and collects identifier nodes, similar to the object_pattern arm, or at a minimum document that array-pattern bindings are intentionally not supported yet.
There was a problem hiding this comment.
Fixed in 20203c2 — added an array_pattern arm to extract_dynamic_import_names that iterates children and collects identifier and assignment_pattern bindings.
Capture bindings like `const [first, second] = await import(...)` by iterating identifier children of array_pattern nodes, including assignment_pattern defaults. Impact: 1 functions changed, 2 affected
|
Addressed round-4 feedback (20203c2): array_pattern bindings dropped — Added an |
|
Claude finished @carlos-alm's task —— View job PR Review: Fix dogfood issues #408–#411Todo List
Root Cause AnalysisI've thoroughly analyzed the changes and each fix addresses a legitimate root issue: #408 — Benchmark Engine Failure: The benchmark scripts properly wrap #409 — WASM Memory Segfaults: The solution adds proper cleanup of WASM tree-sitter resources:
#410 — Missing Dynamic Import Tracking: The native engine Rust code adds proper
#411 — Stale Native Version: Introduces All fixes target actual technical debt, not configuration workarounds. Configuration Changes & Test Integrity✅ No High-Risk Configuration Changes: The biome.json linter config remains unchanged with standard recommended rules. No test timeouts, coverage thresholds, or TypeScript strict mode flags were relaxed. ✅ Test Integrity Maintained: Existing tests remain unchanged. All test expectations are preserved, with no weakened assertions or conditional validations. The 13 test occurrences use standard vitest patterns. ✅ Proper Error Handling: The try/catch blocks in benchmark scripts use appropriate error logging to stderr, maintaining debugging capability while preventing crashes. Code Quality & Security Assessment✅ Security: No unsafe patterns detected. The use of ✅ Memory Management: The WASM cleanup code follows proper patterns, calling ✅ Code Structure: Changes are minimal and surgical. The Rust code properly uses Greptile Review Comments Status✅ All Critical Issues Addressed:
Final Assessment |
scripts/embedding-benchmark.js
Outdated
| try { | ||
| await disposeModel(); | ||
| } catch (disposeErr) { | ||
| console.error(` disposeModel failed: ${disposeErr.message}`); |
There was a problem hiding this comment.
Unsafe disposeErr.message access in new finally block
benchmark.js was explicitly updated in this PR to use err?.message ?? String(err) for safe error message formatting, but the new finally block here uses disposeErr.message directly. If disposeModel() ever rejects with a non-Error value (a string, number, or null), disposeErr.message evaluates to undefined and the log line becomes " disposeModel failed: undefined".
Apply the same null-safe pattern used in benchmark.js:
| console.error(` disposeModel failed: ${disposeErr.message}`); | |
| console.error(` disposeModel failed: ${disposeErr?.message ?? String(disposeErr)}`); |
There was a problem hiding this comment.
Fixed — both catch blocks in embedding-benchmark.js now use err?.message ?? String(err) to handle non-Error throws safely, matching the pattern in benchmark.js.
There was a problem hiding this comment.
Fixed — both catch blocks in embedding-benchmark.js now use err?.message ?? String(err) for safe error formatting.
| export function disposeParsers() { | ||
| if (_cachedParsers) { | ||
| for (const [, parser] of _cachedParsers) { | ||
| if (parser && typeof parser.delete === 'function') { | ||
| try { | ||
| parser.delete(); | ||
| } catch {} | ||
| } | ||
| } | ||
| _cachedParsers = null; | ||
| } | ||
| for (const [, query] of _queryCache) { | ||
| if (query && typeof query.delete === 'function') { | ||
| try { | ||
| query.delete(); | ||
| } catch {} | ||
| } | ||
| } | ||
| _queryCache.clear(); | ||
| _initialized = false; | ||
| } |
There was a problem hiding this comment.
Language objects are not explicitly deleted
disposeParsers calls .delete() on each cached Parser and Query, but not on the Language objects created by await Language.load(...) inside createParsers(). In web-tree-sitter, Language objects are WASM-backed; their WASM memory is only freed when .delete() is explicitly called on the JS wrapper — JS garbage collection alone does not reclaim it.
After createParsers() returns, the lang locals are out of scope, but each Parser still holds an internal reference to its Language. Calling parser.delete() frees the parser's C++ object, but it may not decrement the Language's WASM reference count if web-tree-sitter treats them independently.
If Language WASM memory is indeed not freed here, the segfault fix (#409) may be incomplete for very long-lived processes running many repeated builds. Consider caching the Language objects alongside parsers so they can also be deleted:
// inside disposeParsers(), after deleting parsers:
for (const [, lang] of _cachedLanguages) {
if (lang && typeof lang.delete === 'function') {
try { lang.delete(); } catch {}
}
}
_cachedLanguages = null;where _cachedLanguages is a parallel Map populated alongside _cachedParsers in createParsers().
There was a problem hiding this comment.
Fixed in 7d55362 — disposeParsers() now caches Language objects in a parallel _cachedLanguages map and calls .delete() on each during disposal.
Language objects from Language.load() are WASM-backed and their memory is only freed when .delete() is explicitly called. Cache them alongside parsers so disposeParsers() can release them, preventing WASM memory leaks in long-lived processes with repeated builds. Impact: 2 functions changed, 4 affected
|
Fixed the Language object leak flagged by Greptile (commit 7d55362): Language objects from Language.load() are now cached in a _cachedLanguages map during createParsers(), and disposeParsers() calls .delete() on each one -- same pattern as the parser and query cleanup. |
| } else if child.kind() == "object_assignment_pattern" { | ||
| // Handle `{ a = 'default' }` — extract the left-hand binding | ||
| if let Some(left) = child.child_by_field_name("left") { | ||
| names.push(node_text(&left, source).to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
rest_pattern bindings silently dropped from object_pattern
The object_pattern arm handles shorthand_property_identifier_pattern, pair_pattern, and object_assignment_pattern, but not rest_pattern. For:
const { a, b, ...rest } = await import('./mod.js');Tree-sitter emits ...rest as a rest_pattern node containing an identifier. It falls through to no branch, so rest is never added to names. The import edge is still emitted correctly, but the rest binding name is silently dropped from the graph.
Consider adding a branch after object_assignment_pattern:
} else if child.kind() == "rest_pattern" {
// `const { a, ...rest } = await import(...)`
if let Some(inner) = child.child(0) {
if inner.kind() == "identifier" {
names.push(node_text(&inner, source).to_string());
}
}
}There was a problem hiding this comment.
Fixed in de32c38 — added rest_pattern/rest_element handling to extract_dynamic_import_names so the identifier inside the spread is captured.
| if let Some(val) = child.child_by_field_name("value") { | ||
| // Handle `{ foo: bar = 'default' }` — extract the left-hand binding | ||
| let binding = if val.kind() == "assignment_pattern" { | ||
| val.child_by_field_name("left").unwrap_or(val) | ||
| } else { | ||
| val | ||
| }; | ||
| names.push(node_text(&binding, source).to_string()); |
There was a problem hiding this comment.
Nested destructuring in pair_pattern produces a garbled import name
The else val branch is intended for the simple rename case { foo: bar } where val is an identifier. However, it does not guard against val being an object_pattern or array_pattern (i.e. nested destructuring):
const { a: { b, c } } = await import('./mod.js');Here the value of the pair_pattern is an object_pattern node. node_text(&val, source) returns the entire text "{ b, c }", which is pushed as a single name string. The graph then records an import binding named literally "{ b, c }" that will never resolve to a real symbol.
A minimal fix guards against non-identifier values:
let binding = if val.kind() == "assignment_pattern" {
val.child_by_field_name("left").unwrap_or(val)
} else if val.kind() == "identifier" {
val
} else {
// Nested pattern (e.g. { foo: { bar } }) — skip; full nested
// support requires recursive extraction.
continue;
};
names.push(node_text(&binding, source).to_string());There was a problem hiding this comment.
Fixed in de32c38 — the pair_pattern handler now guards against non-identifier values. When val is an object_pattern/array_pattern (nested destructuring), we skip it instead of emitting the raw text as a garbled name.
Add rest_pattern/rest_element handling to extract_dynamic_import_names
so `const { a, ...rest } = await import(...)` captures `rest`.
Guard pair_pattern against nested patterns (e.g. `{ a: { b, c } }`)
to prevent garbled import names — skip instead of emitting raw text.
Impact: 1 functions changed, 2 affected
| "array_pattern" => { | ||
| let mut names = Vec::new(); | ||
| for i in 0..name_node.child_count() { | ||
| if let Some(child) = name_node.child(i) { | ||
| if child.kind() == "identifier" { | ||
| names.push(node_text(&child, source).to_string()); | ||
| } else if child.kind() == "assignment_pattern" { | ||
| if let Some(left) = child.child_by_field_name("left") { | ||
| names.push(node_text(&left, source).to_string()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| names |
There was a problem hiding this comment.
rest_element silently dropped from array_pattern
The object_pattern arm correctly handles rest_pattern/rest_element (lines 1068–1074), but the new array_pattern arm only handles identifier and assignment_pattern. For:
const [first, ...rest] = await import('./list.js');tree-sitter emits ...rest as a rest_element node inside the array_pattern. It falls through to no branch, so rest is silently dropped from names — the import edge is emitted but the rest binding is never recorded in the graph.
The fix mirrors the object_pattern handling:
| "array_pattern" => { | |
| let mut names = Vec::new(); | |
| for i in 0..name_node.child_count() { | |
| if let Some(child) = name_node.child(i) { | |
| if child.kind() == "identifier" { | |
| names.push(node_text(&child, source).to_string()); | |
| } else if child.kind() == "assignment_pattern" { | |
| if let Some(left) = child.child_by_field_name("left") { | |
| names.push(node_text(&left, source).to_string()); | |
| } | |
| } | |
| } | |
| } | |
| names | |
| "array_pattern" => { | |
| let mut names = Vec::new(); | |
| for i in 0..name_node.child_count() { | |
| if let Some(child) = name_node.child(i) { | |
| if child.kind() == "identifier" { | |
| names.push(node_text(&child, source).to_string()); | |
| } else if child.kind() == "assignment_pattern" { | |
| if let Some(left) = child.child_by_field_name("left") { | |
| names.push(node_text(&left, source).to_string()); | |
| } | |
| } else if child.kind() == "rest_pattern" || child.kind() == "rest_element" { | |
| // Handle `[a, ...rest]` — extract the identifier inside the spread | |
| if let Some(inner) = child.child(0) { | |
| if inner.kind() == "identifier" { | |
| names.push(node_text(&inner, source).to_string()); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| names | |
| } |
1 participant
Summary
Fixes all four open dogfood issues from v3.1.2:
benchmarkEngine()calls in try/catch so WASM failure doesn't prevent native from running; movedisposeModel()intofinallyblock in embedding benchmarkdisposeParsers()export that calls.delete()on cached WASM parsers/queries; calltree.delete()on syntax trees after AST analysis in builder.jsimport()expressions: adddynamicImportfield to RustImportstruct; detectimport()call expressions in the JS extractor and emit them as imports withdynamicImport=true, matching WASM behaviorgetNativePackageVersion()that reads version from platform npm package.json; prefer this over binary-embedded version ininfoandgetActiveEngine()Test plan
npm run lintpassescodegraph infoshows correct version on CIcodegraph build --engine nativeproducesdynamic-importsedgesnode scripts/benchmark.jssurvives engine failure gracefullyCloses #408, closes #409, closes #410, closes #411