Skip to content

feat(embeddings): forward dimensions param in HTTP embedding requests#1498

Merged
magyargergo merged 5 commits into
abhigyanpatwari:mainfrom
henry201605:feat/http-embedding-dimensions-passthrough
May 11, 2026
Merged

feat(embeddings): forward dimensions param in HTTP embedding requests#1498
magyargergo merged 5 commits into
abhigyanpatwari:mainfrom
henry201605:feat/http-embedding-dimensions-passthrough

Conversation

@henry201605

Copy link
Copy Markdown
Contributor

Summary

Forward GITNEXUS_EMBEDDING_DIMS as the dimensions field in the /v1/embeddings HTTP request body, enabling Matryoshka-capable models to return truncated vectors at the requested size.

Motivation / context

Models like OpenAI text-embedding-3-large (3072d native) support returning lower-dimensional vectors via the dimensions request parameter. Previously, GitNexus only used GITNEXUS_EMBEDDING_DIMS for local validation of the returned vector length — it was never sent to the endpoint. This meant users could not leverage Matryoshka truncation (e.g. requesting 1024d from a 3072d model) and would get a dimension-mismatch error instead.

Areas touched

  • gitnexus/ (CLI / core / MCP server)

Scope and constraints

In scope

  • Pass dimensions in the request body when GITNEXUS_EMBEDDING_DIMS is explicitly set
  • Omit the field entirely when unset (preserves backward compatibility for backends that reject unknown fields)
  • Unit tests for both paths (batch + single-query, with/without dimensions)

Explicitly out of scope / not done here

  • No changes to local ONNX embedding path (only HTTP path affected)
  • No changes to dimension validation logic (still checks returned vector matches expected)

Implementation notes

  • httpEmbedBatch gains an optional dimensions?: number parameter
  • Request body is conditionally extended: { input, model } or { input, model, dimensions }
  • Both httpEmbed (batch) and httpEmbedQuery (single) forward config.dimensions
  • Compatible with OpenAI, Cohere embed-v3, Voyage, and any endpoint that either supports or ignores the dimensions field

Testing and verification

  • cd gitnexus && npx tsc --noEmit (pass)
  • cd gitnexus && npm test (onnxruntime native binding incompatible with macOS 12.2.1 locally — CI will validate)

4 new unit tests added:

  1. omits dimensions from request body when GITNEXUS_EMBEDDING_DIMS is unset
  2. forwards GITNEXUS_EMBEDDING_DIMS as dimensions in request body
  3. forwards dimensions on the single-query path
  4. Existing sends correct request payload test unchanged (validates baseline)

Risk and rollout

  • No breaking change: when GITNEXUS_EMBEDDING_DIMS is unset, request body is identical to before
  • Users who set GITNEXUS_EMBEDDING_DIMS with a Matryoshka-capable model will now get correct truncated vectors instead of a dimension-mismatch error
  • Users with endpoints that reject unknown fields should leave GITNEXUS_EMBEDDING_DIMS unset (existing behavior preserved)

Checklist

  • PR body meets repo minimum length
  • No secrets, tokens, or machine-specific paths committed

…TP request body

When GITNEXUS_EMBEDDING_DIMS is set, include it as the `dimensions` field
in the /v1/embeddings request body. This enables Matryoshka-capable models
(OpenAI text-embedding-3-*, Cohere embed-v3, Voyage) to return truncated
vectors at the requested size.

When the env var is unset, the request body remains `{ input, model }` —
no breaking change for backends that reject unknown fields.

Adds 4 unit tests covering both paths (with/without dimensions) on both
the batch embed and single-query embed code paths.
@vercel

vercel Bot commented May 11, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@magyargergo magyargergo self-assigned this May 11, 2026
@github-actions

github-actions Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 5m 27s —— View job


Adversarial Production-Readiness Review — PR #1498

Tasks completed:

  • Read required docs (DoD.md, AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md)
  • Run branch hygiene git commands
  • Read changed files (http-client.ts, http-embedder.test.ts)
  • Read related files (embedder.ts, mcp/core/embedder.ts, schema.ts)
  • Trace dimensions through config → request body → vector validation
  • Run hidden Unicode / bidi scans
  • Check CI status on current head
  • Compile findings

Review bar for this PR

Applied against: DoD.md §2 (Correctness, Contracts, Security, Tests), §4.2 (gitnexus/ validation baseline), plus the PR-specific Definition of Done from the trigger comment. Key gates:

  • Unset dims → request body must be exactly { input, model }, no shape drift
  • Set dims → both batch and single-query paths must send dimensions
  • Returned vectors must still be validated before reaching FLOAT[N] storage
  • API key must not appear in errors, logs, or serialized bodies
  • No bidi/hidden Unicode in executable code
  • Branch diff scoped to the two embedding files
  • CI passes typecheck + unit tests on current head (after merge-from-main)

Current PR state

Item Value
Head SHA 199e662 (Merge branch 'main' into feat/http-embedding-dimensions-passthrough)
Feature commit bd6d82b
State Open
Changed files gitnexus/src/core/embeddings/http-client.ts (+33/−4), gitnexus/test/unit/http-embedder.test.ts (+67/0)
Typecheck ✅ Passed (quality / typecheck)
Lint ✅ Passed (quality / lint)
Format ✅ Passed (quality / format)
CodeQL ✅ Passed
Gitleaks ✅ Passed
Dependency Review ✅ Passed
Unit tests (Ubuntu/Windows/macOS) 🔄 In progress — no failed steps visible at review time
Docker Build 🔄 In progress
e2e Skipped (no web changes)

Cannot verify: Final unit test conclusion on all 3 OS. Merge is conditional on CI passing.


Branch hygiene assessment

Verdict: merge-from-main commit present but harmless and merge-safe.

Git evidence: the feature commit bd6d82b changes exactly 2 files (gitnexus/src/core/embeddings/http-client.ts, gitnexus/test/unit/http-embedder.test.ts). The merge commit 199e662 brought in fix(server): sanitize repo name (#1305) from main — no unrelated files appear in the PR diff beyond those already in main. The final diff remains scoped to the HTTP embedding runtime and its unit tests. Required checks ran on the current head after the merge commit (typecheck, lint, format all passed; unit tests in progress).


Understanding of the change

Before this PR, GITNEXUS_EMBEDDING_DIMS was used only for local returned-vector validation in http-client.ts — it was never sent to the embedding endpoint. This meant users requesting Matryoshka truncation (e.g., 1024d from a 3072d native model) would always get a dimension-mismatch error because the endpoint returned its native size.

The PR adds a dimensions?: number field to HttpConfig, extends readConfig() to parse GITNEXUS_EMBEDDING_DIMS into it (throwing on NaN or non-positive), adds an optional dimensions parameter to the private httpEmbedBatch(), and threads config.dimensions through both httpEmbed() (batch, called for core indexing) and httpEmbedQuery() (single-query, called for MCP search). When unset, the request body is unchanged ({ input, model } only). When set, { input, model, dimensions } is sent. The existing returned-vector length validation (config.dimensions ?? DEFAULT_DIMS) remains in place on both paths, so a provider that ignores the field and returns native dimensions will fail fast before any vector reaches the Kuzu FLOAT[N] column.

The MCP single-query path (gitnexus/src/mcp/core/embedder.ts) already imported httpEmbedQuery and calls it directly — it receives the dimensions forwarding transparently since httpEmbedQuery now reads config.dimensions internally. The getHttpDimensions() export is also consumed by both mcp/core/embedder.ts:154 (for getEmbeddingDims()) and core/embeddings/embedder.ts:305 (for getEmbeddingDimensions()), ensuring the reported dimension count is consistent with what is requested.


Findings

Finding 1 — parseInt accepts partial strings like "1024abc"

  • Severity: Minor / Low
  • File/line: http-client.ts:43const parsed = parseInt(rawDims, 10);
  • Evidence: parseInt("1024abc", 10) returns 1024. The NaN check on line 44 does not catch this. "1024abc" would silently become 1024.
  • Risk: A misconfigured env value could silently produce an unexpected dimension instead of an obvious startup error.
  • Context: This is consistent with the pre-existing pattern in schema.ts:446, which uses identical parseInt(process.env.GITNEXUS_EMBEDDING_DIMS ?? '384', 10) with the same loose check. The PR did not introduce this pattern; it replicated it. DoD does not explicitly require strict integer parsing.
  • Recommended fix (follow-up): Use /^\d+$/ or Number.isSafeInteger(Number(rawDims)) before parseInt in both http-client.ts and schema.ts for consistency. Add test for "1024abc" input.
  • Blocks merge: No — pre-existing pattern, consistent across codebase, low practical risk.

Finding 2 — Multi-batch test does not verify dimensions forwarding to each batch

  • Severity: Minor
  • File/line: http-embedder.test.ts:241–259 — "splits large inputs into batches" (70 texts → 2 fetch calls)
  • Evidence: This test does not set GITNEXUS_EMBEDDING_DIMS, so both fetch calls receive { input, model } only. No existing test verifies that when DIMS=1024 is set and inputs exceed 64 texts (triggering multiple httpEmbedBatch calls), every fetch call includes dimensions.
  • Risk: Code is correct — config.dimensions is read once and passed into the for loop at http-client.ts:193 for every batch. The code cannot omit it on batch 2+ without a logic bug. But the test doesn't prove it.
  • Recommended fix (follow-up): Add a test: set GITNEXUS_EMBEDDING_DIMS=512, provide 70 texts, mock 2 responses, and assert body.dimensions === 512 on both fetch calls via (fetch as any).mock.calls[0][1].body and mock.calls[1][1].body.
  • Blocks merge: No — code is verifiably correct; test gap only.

Finding 3 — Comment overstates backend compatibility

  • Severity: Info
  • File/line: http-client.ts:98"Endpoints that do not recognise the field are expected to ignore it."
  • Evidence: Some strict OpenAI-compatible self-hosted backends (e.g., older llama.cpp, custom VLLM configs) may return 400 Unknown field: dimensions. The comment implies "ignore" is the universal behavior.
  • Risk: User confusion if a backend rejects dimensions. However, the actual runtime behavior is safe: a 4xx response surfaces a clear sanitized error. And since the field is only sent when GITNEXUS_EMBEDDING_DIMS is explicitly set, users of non-Matryoshka backends who leave the env var unset are completely unaffected.
  • Recommended fix (follow-up): Soften comment to: "Endpoints that do not recognise the field may ignore it or return 400; leave GITNEXUS_EMBEDDING_DIMS unset for strict backends."
  • Blocks merge: No

PR-specific assessment sections

HTTP request contract

requestBody at http-client.ts:110–116 is constructed as { input: batch, model } first, then dimensions is conditionally added only when dimensions !== undefined. JSON.stringify on the resulting object produces exactly {"input":[...],"model":"..."} when unset — no undefined key, no extra fields. The test at line 118 asserts 'dimensions' in body === false (key absence, not just falsy value), which is the correct assertion. Request shape is preserved. ✅

NaN, negative, and non-numeric env values are caught at http-client.ts:44 and throw a clear error. 0 is caught by parsed <= 0. 1024abc passes — see Finding 1.

Batch/core embedding

httpEmbed() at http-client.ts:184–193 uses a for loop; config.dimensions (computed once outside the loop) is passed to each httpEmbedBatch() call as the 6th argument. Item count is checked before vector validation (line 196). Float32Array conversion and length validation at lines 207–218 prevent any wrong-length vector from proceeding. Multi-batch forwarding is correct in code but lacks a multi-batch test with dimensions set (Finding 2). ✅ with minor test gap.

Single-query / MCP embedding

httpEmbedQuery() at http-client.ts:237–243 passes config.dimensions to httpEmbedBatch(). Empty-response error at line 245–247 is unchanged. Dimension mismatch check at lines 252–261 is identical to the batch path. Return type remains number[] — compatible with mcp/core/embedder.ts:137 caller (embedQuery returns Promise<number[]>). Test at lines 144–163 directly exercises the MCP path (mod.embedQuery) and asserts body.dimensions === 512.

Schema/vector compatibility

schema.ts:446–452 computes EMBEDDING_DIMS = parseInt(process.env.GITNEXUS_EMBEDDING_DIMS ?? '384', 10) at module load time. The FLOAT[${EMBEDDING_DIMS}] column is baked into the schema DDL at DB creation. This means:

  • New DB, DIMS unset: Schema creates FLOAT[384], endpoint returns 384d → inserts correctly. ✅
  • New DB, DIMS=1024: Schema creates FLOAT[1024], endpoint is asked for 1024d, returns 1024d → inserts correctly. ✅
  • Existing DB (FLOAT[384]), user changes to DIMS=1024: Schema column stays FLOAT[384]; endpoint now returns 1024d; http-client.ts dimension check (expected=1024, got=1024) passes; Kuzu rejects 1024d into FLOAT[384] at insert time with a clear Kuzu error. Pre-existing behavior, not changed by this PR. GUARDRAILS.md documents the rebuild requirement for model/dimension changes.

This PR does not introduce new schema corruption risk.

Provider/error behavior

  • Provider rejects dimensions with 400: resilientFetch surfaces ResilientFetchExhaustedError; caught at http-client.ts:147–151, rethrown as "Embedding endpoint returned 400 (<safeUrl>, batch 0)" — no API key, no raw URL with query params. ✅
  • Provider ignores dimensions, returns native size: dimension mismatch error thrown at lines 208–215 before any DB insertion. ✅
  • Provider returns 503 (transient): resilientFetch retries with backoff; same request body (serialized string) is sent each time. ✅
  • Circuit breaker open: caught at http-client.ts:137–140, sanitized error with safeUrl. ✅
  • Timeout: DOMException TimeoutError caught at lines 142–145. ✅

Tests

Path Unset dims body Set dims body Mismatch detection
embedText (batch) ✅ line 99 ✅ line 121 ✅ line 311
embedQuery (MCP) ✅ line 144 ✅ line 376
Retry preserves body ✅ (body is pre-serialized string)
API key absent from errors ✅ line 206
Multi-batch, dims set ❌ missing ❌ missing
Invalid env string ❌ missing

vi.resetModules() + env cleanup in afterEach at lines 19–29 is sufficient to prevent module-level cache leakage between tests. Each test re-imports its module after setting env vars. Isolation is adequate.

CI / checks

  • TypeScript typecheck:quality / typecheck passed on current head 199e662
  • Lint + format: ✅ passed
  • Unit tests (Ubuntu/Windows/macOS): 🔄 in progress at review time, zero failed steps
  • CodeQL + Gitleaks + Dependency Review: ✅ all passed
  • The macOS onnxruntime local incompatibility noted in the PR body is the ONNX path, not the HTTP path. HTTP embedding tests mock fetch globally and do not touch ONNX/transformers.js, so they will pass on macOS CI. The PR body's "could not fully run locally" note is not a CI coverage gap.

Hidden Unicode

  • Bidi controls (U+202A–U+202E, U+2066–U+2069): None found in either changed file. ✅
  • Non-ASCII characters: 3 occurrences of em dash (U+2014) in http-client.ts comments at lines 8, 33, and 250. These are ordinary visible punctuation in comments, consistent with repo style (em dashes appear throughout the codebase). Not bidi controls, not in executable code or test assertions. No blocking Unicode issue.

Final verdict

Production-ready with minor follow-ups.

The core change is correctly implemented end-to-end: unset dimensions preserve the exact previous request shape (key-absence verified by test, not just undefined-value), set dimensions are threaded consistently through both the batch ingestion path and the MCP single-query path, returned-vector validation remains on both paths preventing bad vectors from reaching FLOAT[N] storage, API keys are absent from all error surfaces, Unicode is clean, and branch hygiene is acceptable. TypeScript typecheck, lint, and format all passed on the current head; unit tests are in progress with no failures visible. The two follow-up items — (1) strict parseInt validation for GITNEXUS_EMBEDDING_DIMS (pre-existing pattern, low risk) and (2) a multi-batch test verifying dimensions forwarding on each fetch call (code is correct, test gap only) — do not block merge but should be addressed before the next embedding-related change.

View CI run ·

@github-actions

github-actions Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
8678 8677 0 1 386s

✅ All 8677 tests passed

1 test(s) skipped — expand for details
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 77.99% 25797/33076 N/A% 🟢 ███████████████░░░░░
Branches 66.5% 16296/24503 N/A% 🟢 █████████████░░░░░░░
Functions 83.18% 2597/3122 N/A% 🟢 ████████████████░░░░
Lines 81.05% 23322/28773 N/A% 🟢 ████████████████░░░░

📋 View full run · Generated by CI

@magyargergo

Copy link
Copy Markdown
Collaborator

@henry201605 Could you please look into claude's findings? 🙏

…ch test, comment wording

1. Strict parseInt validation: reject non-numeric strings like '1024abc'
   by checking /^\d+$/ before parseInt (Finding 1).
2. Add multi-batch test asserting dimensions is forwarded in every fetch
   call when inputs exceed batch size (Finding 2).
3. Soften JSDoc comment: backends may ignore or reject the dimensions
   field rather than universally ignoring it (Finding 3).
4. Add test for invalid GITNEXUS_EMBEDDING_DIMS values.
@henry201605

Copy link
Copy Markdown
Contributor Author

@magyargergo Thanks for flagging these! All three findings have been addressed in be1073e:

  1. Strict parseInt validation — Added /^\d+$/ regex guard before parsing. Values like "1024abc" now throw immediately.
  2. Multi-batch dimensions test — Added a test that verifies dimensions is forwarded in every fetch call when inputs are split across batches.
  3. Comment wording — Softened to: "endpoints may ignore it or return 400", no longer implying universal compatibility.

Also added a test for invalid GITNEXUS_EMBEDDING_DIMS values to cover the new strict parsing.

@magyargergo magyargergo merged commit 622f98a into abhigyanpatwari:main May 11, 2026
44 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants