fix: skip native build orchestrator for addon ≤3.8.0 and fix path bug#758
fix: skip native build orchestrator for addon ≤3.8.0 and fix path bug#758carlos-alm merged 8 commits intomainfrom
Conversation
The native build orchestrator in addon 3.8.0 has a path normalization bug: file_symbols keys use absolute paths but known_files uses relative paths, causing zero import/call edges. Additionally, the orchestrator ran even when engine: 'wasm' was explicitly requested. JS-side fixes (immediate): - Skip orchestrator when engine version is 3.8.0 (falls back to JS) - Skip orchestrator when engine is not 'native' (respects engine option) - Skip orchestrator on forceFullRebuild (version/schema mismatch) - Add 'No changes detected' logging on native earlyExit - Add 'Incremental: N changed' logging from native pipeline results - Write build_meta after native build for cross-engine consistency Rust-side fix (takes effect in next native addon release): - Use relative_path() instead of normalize_path() when building file_symbols map, and update sym.file to relative path - Add changedCount/removedCount/isFullBuild to BuildPipelineResult so JS side can emit incremental status logging
|
Claude finished @carlos-alm's task —— View job Root Cause Analysis ✅Problem Being Solved: Critical bug in native addon 3.8.0 where Why This Approach: The fix is well-justified:
Risk Assessment: ✅ LOW RISK - This is a legitimate bug fix addressing a critical regression, not a workaround that hides symptoms. Backlog Compliance
Code Quality Review ✅Rust Changes (crates/codegraph-core/src/build_pipeline.rs)Lines 257, 60-62: ✅ EXCELLENT
TypeScript Changes (src/domain/graph/builder/pipeline.ts)Lines 353-358: ✅ ROBUST DETECTION const orchestratorBuggy = ctx.engineVersion === '3.8.0';
const forceJs =
process.env.CODEGRAPH_FORCE_JS_PIPELINE === '1' ||
ctx.forceFullRebuild ||
orchestratorBuggy ||
ctx.engineName !== 'native';
Lines 378-389: ✅ ENHANCED LOGGING if (result.earlyExit) {
info('No changes detected');
} else if (!result.isFullBuild && (changed > 0 || removed > 0)) {
info(`Incremental: ${changed} changed, ${removed} removed`);
}
Lines 395-403: ✅ METADATA CONSISTENCY
Test Integrity ✅No test modifications - The PR doesn't weaken any test assertions or expectations. Per the description, 17/18 integration tests now pass (1 pre-existing bug), which demonstrates the fix works correctly. Technical Assessment ✅Path Resolution LogicThe core fix is sound:
Error Handling
Performance Implications
Minor Suggestions (Non-blocking)
Final AssessmentCritical ConcernsNone - This is a well-crafted fix for a critical regression. Final Recommendation
This PR demonstrates excellent engineering practice - identifying the root cause, implementing a precise fix, and providing robust safeguards during the transition period. |
Greptile SummaryThis PR fixes two root-cause bugs that caused 58 CI test failures: absolute paths in Confidence Score: 5/5Safe to merge — all changes are targeted bug fixes with no regressions introduced. Both root causes are correctly addressed: relative_path() fixes the Rust file_symbols key bug, and normalizePath is consistently applied across all batchResolved key construction sites on the JS side. Version guards for 3.8.0 are well-scoped (orchestrator uses semverCompare ≤ 3.8.0 which is safe because buildGraph didn't exist before 3.8.0; import-edge guard uses exact equality which is correct). Previous review concerns about the comment wording and setBuildMeta overwrite are resolved. No P0/P1 findings remain. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildGraph called] --> B{forceJs?}
B -->|env flag set| JS[JS Pipeline]
B -->|forceFullRebuild| JS
B -->|addon v3.8.0 buggy| JS
B -->|engine not native| JS
B -->|none match| C{nativeDb has buildGraph?}
C -->|no| JS
C -->|yes| D[Rust Orchestrator]
D --> E{earlyExit?}
E -->|yes| F[No changes detected, close DB, return]
E -->|no| G[log incremental stats, setBuildMeta, close DB, return result]
D -->|throws| JS
JS --> H{useNativeImportEdges?}
H -->|addon v3.8.0| I[JS import-edge builder]
H -->|fileSymbols size 3 or fewer| I
H -->|otherwise| J[Rust buildImportEdges NAPI, normalized forward-slash keys]
I --> K[Write to DB]
J --> K
Reviews (4): Last reviewed commit: "fix(native): normalize batch key separat..." | Re-trigger Greptile |
src/domain/graph/builder/pipeline.ts
Outdated
| // paths but known_files are relative, causing zero import/call edges. | ||
| // Skip the orchestrator for affected versions (fixed in source, will | ||
| // take effect in the next native addon release). | ||
| const orchestratorBuggy = ctx.engineVersion === '3.8.0'; |
There was a problem hiding this comment.
≤3.8.0 but check is strict equality
The inline comment (line 349), the PR title, and the orchestratorBuggy variable are all framed as "≤3.8.0", but the runtime check only matches exactly '3.8.0'. Any pre-release tag variant like '3.8.0-rc.1' or a hypothetical '3.7.5' that also ships buildGraph would bypass the guard and hit the buggy code path.
If buildGraph was truly first introduced in 3.8.0 (making earlier versions safe), tighten the comment to === '3.8.0' so future readers aren't tempted to widen the guard unnecessarily.
| const orchestratorBuggy = ctx.engineVersion === '3.8.0'; | |
| const orchestratorBuggy = ctx.engineVersion === '3.8.0'; // exactly 3.8.0 is the first and only released version with this path bug |
There was a problem hiding this comment.
Fixed — updated the comment to say "3.8.0" (exact match) instead of the misleading "≤3.8.0", making it clear this guards only against the single known-buggy release. Also added debug logging that reports which specific condition triggered the JS fallback (addresses the observability angle too).
src/domain/graph/builder/pipeline.ts
Outdated
| // Sync build_meta so JS-side version/engine checks work on next build | ||
| setBuildMeta(ctx.db, { | ||
| engine: ctx.engineName, | ||
| engine_version: ctx.engineVersion || '', | ||
| codegraph_version: CODEGRAPH_VERSION, | ||
| schema_version: String(ctx.schemaVersion), | ||
| node_count: String(result.nodeCount ?? 0), | ||
| edge_count: String(result.edgeCount ?? 0), | ||
| }); |
There was a problem hiding this comment.
codegraph_version; JS value silently wins
The native orchestrator already writes codegraph_version to the metadata table (using the Cargo crate version, env!("CARGO_PKG_VERSION")). This setBuildMeta call then overwrites it with the JS CODEGRAPH_VERSION. If the Rust crate and the npm package version numbers ever diverge — even briefly during a release cycle — every subsequent incremental build will detect a "version changed" event and force a full rebuild until the two are re-aligned.
This is low-risk while versions are kept in lockstep, but worth documenting explicitly or asserting that they match, since silent full-rebuild promotions are hard to diagnose.
There was a problem hiding this comment.
Fixed — added a comment block above setBuildMeta explaining that the JS overwrite is intentional: the npm package version is the authoritative one for the JS-side "version changed → full rebuild" detection, and the two versions are kept in lockstep by the release process.
Replace hardcoded `=== '3.8.0'` with `semverCompare(version, '3.8.0') <= 0` so the check automatically passes for 3.9.0+ without manual updates.
- Fix comment: say "3.8.0" (exact match) instead of misleading "≤3.8.0" - Log which condition triggered forceJs bypass for easier debugging - Document why JS overwrites Rust-written codegraph_version in build_meta
|
Addressed review feedback from both Greptile and Claude:
|
|
Claude finished @carlos-alm's task —— View job Re-reviewing updated changesTodo List
✅ Review Feedback Successfully AddressedAll review comments have been properly addressed: 1. Comment/Code Mismatch Fixed (Greptile feedback)
2. Enhanced Debug Logging (Claude suggestion)
3. Codegraph Version Overwrite Documentation (Greptile feedback)
4. Debug Import Added
🔍 Code Quality AssessmentRust Changes (
|
The native orchestrator path omitted built_at from setBuildMeta, leaving the build timestamp stale after native runs. The JS finalize stage already writes it — match that behavior so codegraph info shows the correct build time regardless of which engine path ran.
|
Addressing the two remaining findings from the Greptile summary:
|
|
Claude finished @carlos-alm's task —— View job ✅ Review Feedback Successfully AddressedAll review comments have been properly addressed: 1. Comment/Code Mismatch Fixed (Greptile feedback)
2. Enhanced Debug Logging (Claude suggestion)
3. Codegraph Version Overwrite Documentation (Greptile feedback)
4. Debug Import Added
🔍 Code Quality AssessmentRust Changes (
|
…esolution
The Rust build_import_edges function constructs lookup keys as
format!("{}/{}", root_dir, file), mixing backslashes in root_dir with
forward slashes. On Windows, the JS batch map keys use path.join()
which produces all-backslash paths, causing key mismatches and
silently dropping 5 import edges.
Fix:
- Normalize batch map keys to forward slashes (resolve.ts, resolve-imports.ts)
- Normalize lookup keys in getResolved and buildImportEdgesNative
- Normalize root_dir in Rust key construction (edge_builder.rs)
- Skip native import-edge builder for addon 3.8.0 (pre-compiled addon
still has the bug; Rust fix takes effect in 3.8.1+)
|
Addressing the remaining Windows CI failure (watcher-rebuild test — 5 missing import edges): Root cause: The Rust Fix (039affe):
All 574 integration+builder tests pass locally on Windows after this fix. |
Tests that look up resolveImportsBatch results must use normalizePath(fromFile) to match the normalized keys in the batch map.
Summary
The native build orchestrator in addon 3.8.0 stores absolute file paths instead of relative paths, causing zero import/call edges and 58 test failures on main CI.
Root cause 1 — orchestrator paths:
build_pipeline.rs:251usesnormalize_path()(slash normalization only) instead ofrelative_path()(strips root_dir prefix). This causesfile_symbolsto have absolute-path keys whileknown_fileshas relative paths, so import resolution finds no matches.Root cause 2 — Windows import edge keys: The Rust
build_import_edgesfunction constructs lookup keys asformat!("{}/{}", root_dir, file), mixing backslashes inroot_dirwith a forward slash separator. On Windows, the JS batch map keys usepath.join()which produces all-backslash paths, causing key mismatches that silently drop import edges.JS-side fixes (immediate — unblocks CI):
engine !== 'native'(previously ignoredengine: 'wasm'option)forceFullRebuild(version/schema mismatch was silently ignored)No changes detectedandIncremental: N changedlogging for native pipelinebuild_metaafter native build for cross-engine consistencyRust-side fixes (take effect in next native addon release):
relative_path()inbuild_pipeline.rswhen buildingfile_symbolsmaproot_dirbackslashes inedge_builder.rskey constructionchangedCount/removedCount/isFullBuildtoBuildPipelineResultTest plan
tests/integration/build.test.ts— 17/18 pass (1 pre-existing incremental edge bug)tests/integration/build-parity.test.ts— all passtests/integration/structure.test.ts— all passtests/integration/branch-compare.test.ts— all passtests/builder/pipeline.test.ts— all passtests/search/embedding-regression.test.ts— all passtests/integration/watcher-rebuild.test.ts— all pass (Windows parity fixed)