Skip to content

fix(memory): preserve embedding proxy provider prefixes#66452

Merged
vincentkoc merged 3 commits intomainfrom
vk/pr-66190-main
Apr 14, 2026
Merged

fix(memory): preserve embedding proxy provider prefixes#66452
vincentkoc merged 3 commits intomainfrom
vk/pr-66190-main

Conversation

@vincentkoc
Copy link
Copy Markdown
Member

Summary

Describe the problem and fix in 2–5 bullets:

  • Problem: static embedding model refs routed through proxy providers could lose their provider prefix and get rewritten as raw OpenAI refs.
  • Why it matters: memory embedding requests can silently switch providers or break when users rely on proxy-prefixed model refs like spark/... or litellm/....
  • What changed: parse static embedding model refs with provider-aware logic so non-OpenAI prefixes survive, add focused regression coverage, and carry the missing changelog entry.
  • What did NOT change (scope boundary): no broader embedding provider selection or memory indexing policy changed.
  • Supersedes fix(memory): use native parser for openai embeddings proxy #66190.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: embedding model normalization treated static refs as OpenAI-native too early and discarded third-party provider prefixes.
  • Missing detection / guardrail: there was no focused test for prefixed static embedding refs flowing through the OpenAI embedding host path.
  • Contributing context (if known): memory setups frequently proxy embeddings through LiteLLM, Spark, and similar provider prefixes.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: src/memory-host-sdk/host/embeddings-openai.test.ts
  • Scenario the test should lock in: provider-prefixed static embedding refs are preserved instead of being collapsed to openai/....
  • Why this is the smallest reliable guardrail: the failure is contained in static model-ref parsing for the embedding host path.
  • Existing test that already covers this (if any): none.
  • If no new test is added, why not: N/A.

User-visible / Behavior Changes

Memory embeddings now preserve configured proxy-provider prefixes instead of silently rewriting them to OpenAI.

Diagram (if applicable)

N/A

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

  • OS: macOS
  • Runtime/container: Node 22 / pnpm test:serial
  • Model/provider: Memory embeddings via proxy providers
  • Integration/channel (if any): Memory host SDK
  • Relevant config (redacted): targeted fixture/test doubles only

Steps

  1. Configure memory embeddings with a provider-prefixed static model ref such as spark/text-embedding-3-large.
  2. Run pnpm test:serial src/memory-host-sdk/host/embeddings-openai.test.ts.
  3. Verify the resolved embedding model keeps the original provider prefix.

Expected

  • The provider-prefixed static ref remains provider-prefixed through embedding host resolution.

Actual

  • Before the fix, the prefix could be dropped and rewritten as an OpenAI ref.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: pnpm test:serial src/memory-host-sdk/host/embeddings-openai.test.ts.
  • Edge cases checked: non-OpenAI provider prefixes on static embedding refs.
  • What you did not verify: live remote embedding calls through every supported proxy provider.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: narrow fix may miss adjacent provider-specific edge cases not covered by the focused regression.
    • Mitigation: keep the patch scoped and ship targeted regression coverage for the implicated path only.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 14, 2026

Greptile Summary

Replaces the local normalizeEmbeddingModelWithPrefixes helper in embeddings-openai.ts with parseStaticModelRef from the shared agents utility, so proxy-provider prefixes like spark/… or litellm/… on static embedding model refs are no longer silently dropped. A focused regression test for the fixed path is also added. The only minor concern is that the CHANGELOG entry references (#66190) (the superseded PR) rather than this PR's number.

Confidence Score: 5/5

  • Safe to merge — the fix is narrow, correct, and backed by a focused regression test; only a P2 changelog nit remains.
  • All findings are P2 (changelog PR reference). The implementation correctly handles blank input, openai-prefixed refs, third-party prefixed refs, and bare model IDs. No logic or data-integrity issues found.
  • CHANGELOG.md — minor PR number reference question, no code files need special attention.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: CHANGELOG.md
Line: 30

Comment:
**Changelog cites superseded PR number**

The entry references `(#66190)`, but the PR description says this PR (#66452) *supersedes* #66190. If #66190 was never merged, the linked issue number is a dead end for readers tracing this fix in the changelog. Consider updating to reference the PR that actually lands the fix.

```suggestion
- Memory/embeddings: preserve non-OpenAI provider prefixes when normalizing OpenAI-compatible embedding model refs so proxy-backed memory providers stop failing with `Unknown memory embedding provider`. (#66452) Thanks @jlapenna and @vincentkoc.
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(memory): preserve embedding proxy pr..." | Re-trigger Greptile

Comment thread CHANGELOG.md Outdated
@vincentkoc vincentkoc merged commit 8820a43 into main Apr 14, 2026
10 checks passed
@vincentkoc vincentkoc deleted the vk/pr-66190-main branch April 14, 2026 10:05
steipete pushed a commit that referenced this pull request Apr 14, 2026
* fix(memory): preserve embedding proxy provider prefixes

* docs(changelog): fix embeddings entry

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer Maintainer-authored PR size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant