Skip to content

add native cosine fallback#826

Merged
rwmjhb merged 1 commit into
CortexReach:masterfrom
TurboTheTurtle:fix/native-cosine-fallback
May 26, 2026
Merged

add native cosine fallback#826
rwmjhb merged 1 commit into
CortexReach:masterfrom
TurboTheTurtle:fix/native-cosine-fallback

Conversation

@TurboTheTurtle

@TurboTheTurtle TurboTheTurtle commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add retrieval.disableNativeCosine and MEMORY_LANCEDB_DISABLE_NATIVE_COSINE as escape hatches for AVX-only hosts
  • skip LanceDB distanceType("cosine") when disabled, then score and sort vector candidates with JavaScript cosine
  • document the AVX-only SIGILL fallback and expose it in the plugin manifest
  • extend the vector-search cosine regression to prove the fallback avoids native cosine and reranks correctly

Closes #644.

Validation

  • node test/vector-search-cosine.test.mjs
  • npx tsc --noEmit
  • node scripts/verify-ci-test-manifest.mjs
  • git diff --check

@TurboTheTurtle
TurboTheTurtle marked this pull request as ready for review May 26, 2026 04:37
rwmjhb
rwmjhb requested changes May 26, 2026
View reviewed changes

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #826 Review: add native cosine fallback

Verdict: REQUEST-CHANGES | 7 rounds completed | Value: 74% | Size: MEDIUM | Author: TurboTheTurtle

Value Assessment

Problem: AVX-only Linux x64 users can hit SIGILL crashes when LanceDB executes native cosine vector search, making the plugin unstable on CPUs that otherwise meet the documented AVX requirement. The PR adds an opt-in escape hatch to avoid LanceDB native cosine and rerank candidates with JavaScript cosine similarity.

Dimension Assessment
Value Score 74%
Value Verdict priority-review
Issue Linked true
Project Aligned true
Duplicate false
AI Slop Score 0/6
User Impact high
Urgency urgent

Open Questions:

  • Because the base is marked stale, confirm the issue has not already been fixed or superseded on current main.
  • Validate on an actual AVX-only Linux x64 host that vectorSearch without distanceType("cosine") does not still enter the SIGILL path.
  • Confirm recall and ranking quality are acceptable when fallback candidate fetch uses LanceDB's default distance before JS cosine reranking.

Summary

AVX-only Linux x64 users can hit SIGILL crashes when LanceDB executes native cosine vector search, making the plugin unstable on CPUs that otherwise meet the documented AVX requirement. The PR adds an opt-in escape hatch to avoid LanceDB native cosine and rerank candidates with JavaScript cosine similarity.

Evaluation Signals

Signal Value
Blockers 0
Warnings 0
PR Size MEDIUM
Verdict Floor approve
Risk Level high
Value Model codex
Primary Model codex
Adversarial Model claude

Must Fix

  • F1: JS cosine fallback rejects LanceDB vectors
  • MR1: Disabling only distanceType("cosine") may not actually avoid the SIGILL crash the PR targets

Nice to Have

  • F2: Fallback can miss cosine-nearest rows before reranking
  • MR2: Regression test is structurally incapable of catching the real bug, making the PASS signal misleading

Recommended Action

Good direction — problem is worth solving. Author should address must-fix findings, then this is ready to merge.

Reviewed at 2026-05-26T05:22:03Z | 7 rounds | Value: codex | Primary: codex | Adversarial: claude

@TurboTheTurtle
TurboTheTurtle force-pushed the fix/native-cosine-fallback branch from 0238ab7 to 85e2b94 Compare May 26, 2026 05:37
@TurboTheTurtle

TurboTheTurtle commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I pushed 85e2b94 after this review, and the current head addresses the requested changes:

  • F1: the JS cosine path now normalizes LanceDB/Arrow-style vector-like row values with toNumberVector(...) before scoring, so fallback rows do not need to be plain arrays.
  • MR1: when retrieval.disableNativeCosine is enabled, the fallback no longer calls table.vectorSearch(...) or distanceType("cosine"); it scans scoped rows through table.query().select(...) and computes cosine in JavaScript.
  • F2: because the fallback ranks every scoped row before slicing to limit, it no longer depends on LanceDB default-distance candidates that could miss the cosine-nearest rows.
  • MR2: the regression test now uses vector-like row values and a fake vectorSearch() that throws if called, so it verifies both LanceDB-vector normalization and that native vector search is skipped.

Fresh validation on the current head:

node test/vector-search-cosine.test.mjs
npx tsc --noEmit
node scripts/verify-ci-test-manifest.mjs
git diff --check

@TurboTheTurtle
TurboTheTurtle requested a review from rwmjhb May 26, 2026 05:51
rwmjhb
rwmjhb approved these changes May 26, 2026
View reviewed changes

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #826 Review: add native cosine fallback

Verdict: APPROVE | 6 rounds completed | Value: 77% | Size: MEDIUM | Author: TurboTheTurtle

Value Assessment

Problem: AVX-only Linux x64 users can hit SIGILL crashes when LanceDB native vector search executes cosine search, causing OpenClaw to repeatedly crash despite the documented AVX requirement. The PR adds an opt-in path to avoid native cosine and rank scoped rows in JavaScript.

Dimension Assessment
Value Score 77%
Value Verdict priority-review
Issue Linked true
Project Aligned true
Duplicate false
AI Slop Score 0/6
User Impact high
Urgency urgent

Open Questions:

  • The PR context marks the base as stale; confirm current main has not already fixed or superseded #644.
  • Validate on an actual AVX-only Linux x64 host that the query().select() scan path does not still enter a LanceDB native code path that can SIGILL.
  • Confirm the full scoped scan performance is acceptable for large stores when the fallback is enabled.

Summary

AVX-only Linux x64 users can hit SIGILL crashes when LanceDB native vector search executes cosine search, causing OpenClaw to repeatedly crash despite the documented AVX requirement. The PR adds an opt-in path to avoid native cosine and rank scoped rows in JavaScript.

Evaluation Signals

Signal Value
Blockers 0
Warnings 0
PR Size MEDIUM
Verdict Floor approve
Risk Level high
Value Model codex
Primary Model codex
Adversarial Model claude

Nice to Have

  • F1: Fallback materializes every scoped vector at once
  • MR1: New vector-length guard silently drops rows on the DEFAULT native path, not just the fallback
  • MR2: Returned entry.vector for the native path changed from raw LanceDB value to an Array.from copy
  • MR3: Fallback warning uses raw console.warn instead of the plugin's logger/callback pattern

Recommended Action

Ready to merge.

Reviewed at 2026-05-26T06:35:02Z | 6 rounds | Value: codex | Primary: codex | Adversarial: claude

@rwmjhb
rwmjhb merged commit 2c7036f into CortexReach:master May 26, 2026
8 checks passed
@TurboTheTurtle
TurboTheTurtle deleted the fix/native-cosine-fallback branch May 26, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rwmjhb rwmjhb rwmjhb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SIGILL leads to openclaw crash on AVX-only CPU

2 participants

@TurboTheTurtle @rwmjhb