fix(native): strip pre-release suffix in semverCompare

[carlos-alm]

Summary

• semverCompare('3.9.3-dev.6', '3.9.1') incorrectly returned -1 because Number('3-dev') is NaN, and NaN || 0 collapsed to 0, making the patch comparison 0 < 1
• This caused shouldSkipNativeOrchestrator to flag all pre-release builds as "buggy addon", disabling the native orchestrator fast path from perf(native): use single rusqlite connection for entire build pipeline #897
• Fix: strip -<prerelease> suffix before splitting on . so 3.9.3-dev.6 correctly compares as 3.9.3

Impact

Every dev build (e.g. 3.9.3-dev.6) was silently falling back to the JS pipeline instead of using the Rust orchestrator. Benchmarking confirmed the native orchestrator was skipped:

[codegraph DEBUG] Skipping native orchestrator: buggy addon 3.9.3-dev.6

Test plan

• Existing semverCompare tests pass (equal, less-than, greater-than across major/minor/patch)
• New test: pre-release suffixes stripped correctly (3.9.3-dev.6 > 3.9.1, 3.9.3-dev.6 == 3.9.3, 2.0.0-beta.1 > 1.9.9-alpha.3)
• All 23 tests in update-check.test.ts pass

[claude]

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

---

PR Review Complete ✅

Todo List

• Phase 1: Root cause analysis - understand the semver comparison bug
• Phase 2: Configuration change detection - check for risky changes
• Phase 3: Test integrity - verify test quality and coverage
• Phase 4: Code quality review - analyze implementation approach
• Phase 5: Backlog compliance assessment - rate against project principles
• Final: Provide comprehensive review rating and recommendation

Phase 1: Root Cause Analysis ✅

Problem Being Solved: Pre-release versions like 3.9.3-dev.6 were incorrectly compared in semverCompare, causing Number('3-dev') to return NaN, which collapsed to 0 via the || 0 fallback. This made 3.9.3-dev.6 appear as 0.0.3 vs 3.9.1, returning -1 (less than) instead of 1 (greater than).

Why This Approach: The fix strips pre-release suffixes with replace(/-.*$/, '') before splitting on ., so 3.9.3-dev.6 correctly compares as 3.9.3. This is a minimal, targeted change that preserves the existing logic while fixing the core issue.

Impact Verification: Found that shouldSkipNativeOrchestrator in src/domain/graph/builder/pipeline.ts:483 uses semverCompare(ctx.engineVersion, '3.9.1') < 0 to gate the native Rust orchestrator. The bug caused ALL pre-release builds to fall back to the slower JS pipeline instead of using the fast Rust path introduced in #897.

Phase 2: Configuration Change Detection ✅

✅ No high-risk configuration changes detected

• No linting rule modifications
• No test timeout increases or coverage threshold lowering
• No build configuration changes
• No TypeScript strict mode relaxation
• This is a pure bug fix with targeted behavior change

Phase 3: Test Integrity ✅

✅ Strong test coverage and quality

• New test cases properly validate the fix: Lines 55-62 in update-check.test.ts specifically test pre-release suffix stripping
• Comprehensive scenarios covered:
  • 3.9.3-dev.6 > 3.9.1 (the original failing case) ✅
  • 3.9.3-dev.6 == 3.9.3 (suffix stripped correctly) ✅
  • 3.9.3-dev.6 < 3.9.4 (still works with higher versions) ✅
  • 2.0.0-beta.1 > 1.9.9-alpha.3 (both sides with suffixes) ✅
• All existing tests preserved: 23 tests in the file, no assertions weakened
• Test demonstrates actual problem: The comment shows this was the exact failing case from production

Phase 4: Code Quality Review ✅

✅ Implementation is sound and minimal

• Regex approach is correct: replace(/-.*$/, '') strips everything after first hyphen, handling all pre-release formats (-dev.6, -beta.1, -alpha.3)
• No performance concerns: Regex is simple and only runs during version checks (infrequent)
• No security issues: Pure string manipulation, no injection vectors
• Backwards compatible: All existing semver comparisons continue to work identically
• Well-documented: JSDoc updated to reflect new behavior: "Strips pre-release suffixes (e.g. "3.9.3-dev.6" → "3.9.3")"
• Single responsibility: Function still does exactly one thing (semver compare), just handles edge case correctly

Phase 5: Backlog Compliance Assessment ✅

Backlog Compliance

• Zero-dep: ✓ — No new dependencies added, uses only built-in regex
• Foundation-aligned: ✓ — Pure utility fix, no violation of core principles
• Problem-fit: 4 — Directly improves native engine accuracy, prevents performance regression from dev builds incorrectly falling back to JS pipeline
• Breaking: No — Purely additive fix, no existing API changes
• Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Critical Concerns

None. This is a well-executed bug fix with proper testing and clear impact.

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: Clean, targeted fix for a real performance regression affecting all development builds. Strong test coverage validates the solution. No configuration risks or breaking changes. Only missing the 5th star because it's a bug fix rather than a feature that advances the core mission.

---

[greptile-apps]

Greptile Summary

Fixes a bug in semverCompare where pre-release version strings like 3.9.3-dev.6 caused Number('3-dev') → NaN, which collapsed to 0 via || 0, making 3.9.3-dev.6 compare as less than 3.9.1 and silently disabling the native Rust orchestrator for all dev builds. The fix is minimal: strip the -<prerelease> suffix with a regex replace before splitting on ..

Confidence Score: 5/5

Safe to merge — minimal, targeted fix with no regressions and adequate test coverage.

The change is two lines in one function, the root cause is clearly explained, the fix is correct, and the new tests directly exercise the previously broken cases. No pre-existing behavior is altered for callers that don't pass pre-release strings, and checkForUpdates already short-circuits before reaching semverCompare for pre-release inputs.

No files require special attention.

Vulnerabilities

No security concerns identified.

Important Files Changed

Filename	Overview
src/infrastructure/update-check.ts	Two-line fix strips pre-release suffix before splitting on ., eliminating the NaN-collapse bug; jsdoc updated to match.
tests/unit/update-check.test.ts	Adds a new it block with four assertions covering both one-sided and two-sided pre-release comparisons; existing suite unchanged.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["semverCompare(a, b)"] --> B["Strip pre-release suffix\na.replace(/-.*$/, '')"]
    B --> C["Split on '.' and map to Number\n['3','9','3'] → [3, 9, 3]"]
    C --> D{"Compare each segment\n(major → minor → patch)"}
    D -- "na < nb" --> E["return -1"]
    D -- "na > nb" --> F["return 1"]
    D -- "all equal" --> G["return 0"]

    H["Old behavior: '3.9.3-dev.6'"] --> I["split('.') → ['3', '9', '3-dev', '6']"]
    I --> J["Number('3-dev') → NaN\nNaN || 0 → 0"]
    J --> K["0 < 1 → returns -1 ❌"]

    L["New behavior: '3.9.3-dev.6'"] --> M["replace(/-.*$/, '') → '3.9.3'"]
    M --> N["split('.') → ['3','9','3']"]
    N --> O["[3, 9, 3] — all valid numbers"]
    O --> P["3 > 1 → returns 1 ✅"]

Loading

_{Reviews (1): Last reviewed commit: "fix(native): strip pre-release suffix in..." | Re-trigger Greptile}

[github-actions]

Codegraph Impact Analysis

1 functions changed → 5 callers affected across 3 files

• semverCompare in src/infrastructure/update-check.ts:23 (5 transitive callers)