Skip to content

Adding VoyageAI integration - #1

Open
fzowl wants to merge 9 commits into
mainfrom
voyageai_integration
Open

Adding VoyageAI integration#1
fzowl wants to merge 9 commits into
mainfrom
voyageai_integration

Conversation

@fzowl

@fzowl fzowl commented Aug 16, 2026

Copy link
Copy Markdown
Owner

What

Adds a VoyageAI integration module (aiservices/voyageai) providing text embedding, multimodal embedding, contextualized embedding, and reranking services, plus a small reranking abstraction (TextRerankingService, RerankResult) in semantickernel-api.

Why this round (CI repair)

The upstream PR (microsoft#345) was red on merge-gatekeeper, which was blocked by the Java CI build failing under the bug-check profile. Root causes and fixes:

  • SpotBugs CT_CONSTRUCTOR_THROW: RerankResult and the four VoyageAI service classes plus VoyageAIClient throw validation exceptions from non-final constructors (finalizer-attack vector). Marked these classes final (behavior unchanged; validation preserved). This was the failure that stopped the CI build at semantickernel-api.
  • SpotBugs EI_EXPOSE_REP / EI_EXPOSE_REP2: the VoyageAIModels Jackson DTO accessors expose mutable list/array fields. Annotated the individual getters/setters/constructor with @SuppressFBWarnings, matching the existing pattern in the HuggingFace/OpenAI/Google DTOs. (This surfaced only after the first fix let the build reach the VoyageAI module.)
  • Version alignment: merged current upstream/main (which advanced the repo to 1.5.1-SNAPSHOT); the VoyageAI module still pinned 1.4.4-RC3-SNAPSHOT, which made the semantickernel-bom import unresolvable. Bumped the module's parent version to match.

Validation

Full reactor build with the CI command ./mvnw -Pbug-check -DskipTests -Pcompile-jdk17 clean install passes the semantickernel-api and aiservices/voyageai modules (SpotBugs clean). The VoyageAI unit tests pass (33 run; 6 live-API integration tests skipped without credentials).

Diff scope vs upstream/main is limited to the VoyageAI module, the two new reranking API files, PACKAGES.md, and the root pom.xml module entry.

fzowl and others added 9 commits November 17, 2025 10:54
…CONSTRUCTOR_THROW

SpotBugs (bug-check profile) flagged CT_CONSTRUCTOR_THROW on classes whose
constructors throw validation exceptions while being non-final, which the
Java CI build treats as an error and fails merge-gatekeeper. Marking these
value/service classes final removes the finalizer-attack vector SpotBugs
warns about while preserving the existing constructor validation.
After merging upstream/main, the repository version advanced to
1.5.1-SNAPSHOT while the VoyageAI module still pinned the old
1.4.4-RC3-SNAPSHOT parent, which made the semantickernel-bom import
unresolvable and broke the reactor build.
The VoyageAIModels request/response DTOs expose their mutable list and
array fields directly through getters, setters and one constructor, which
SpotBugs (bug-check profile) flags as EI_EXPOSE_REP/EI_EXPOSE_REP2. These
are plain Jackson-mapped data holders, so annotate the individual accessors
with @SuppressFBWarnings, matching the pattern already used by the other
aiservices DTOs (e.g. HuggingFace, OpenAI, Google).
@fzowl

fzowl commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

VERDICT:CHANGES_NEEDED

The CI-repair objective this round was aimed at is met, and commit hygiene is clean — but there are correctness and convention issues that should be fixed before merge.

What's solid

  • CT_CONSTRUCTOR_THROW fixed correctly. RerankResult and all four VoyageAI service classes plus VoyageAIClient are now final, so the throwing constructors are no longer finalizer-attack vectors. This is exactly the failure @karianna flagged on Adding VoyageAI integration microsoft/semantic-kernel-java#345.
  • EI_EXPOSE_REP / EI_EXPOSE_REP2 applied correctly. VoyageAIModels getters returning mutable List/float[]/nested DTOs carry @SuppressFBWarnings("EI_EXPOSE_REP"); setters and the MultimodalInput(List) ctor carry EI_EXPOSE_REP2; primitive/String accessors correctly omit it. Mirrors the HuggingFace/OpenAI DTO pattern — no over/under-application.
  • Wiring correct: parent bumped to 1.5.1-SNAPSHOT, module registered in root pom.xml, PACKAGES.md documented.
  • Commit hygiene clean: no Co-Authored-By, no "Generated with", no .claude/ files anywhere in the branch.
  • Solid unit coverage of the service layer (happy path, builder, validation, rerank descending-sort, contextualized insertion-order).

Should fix before merge

  1. [Medium] Blocking I/O on a Reactor thread. VoyageAIClient.sendRequestAsync wraps a synchronous newCall(...).execute() in Mono.fromCallable with no .subscribeOn(Schedulers.boundedElastic()). In a reactive library this blocks a non-blocking thread. Use OkHttp's async enqueue (Mono.create) or add subscribeOn(boundedElastic()).
  2. [Medium] New HTTP stack diverges from every other connector. Every existing connector (openai/google/huggingface) uses com.azure.core.http.HttpClient; this module pulls in com.squareup.okhttp3:okhttp with a hardcoded 4.12.0 — the other deps here are correctly unversioned via the parent BOM. Either align on azure-core, or at minimum centralize the OkHttp version in semantickernel-bom/parent dependencyManagement.
  3. [Medium] FAIL_ON_UNKNOWN_PROPERTIES not disabled globally. Only the wrapper DTOs carry @JsonIgnoreProperties(ignoreUnknown=true); leaf DTOs (EmbeddingDataItem, RerankDataItem, EmbeddingUsage, MultimodalContentItem) don't, so any new field VoyageAI returns will throw. Set it on the ObjectMapper instead of relying on unevenly-applied annotations.
  4. [Medium] VoyageAIClient has zero tests — the riskiest code (URL/auth header, non-2xx to AIException mapping, null/empty body, deserialization guards). OkHttp ships mockwebserver; a MockWebServer test here would be high value.
  5. [Low] rerankAsync throws synchronously for the null-query check instead of returning Mono.error(...), breaking reactive composition (the null-documents branch just below does it right).
  6. [Low] Inconsistent @JsonInclude(NON_NULL)EmbeddingRequest and RerankRequest lack it, so unset optionals serialize as explicit nulls ("top_k":null, etc.); other request DTOs have it.
  7. [Low] Possible NPE on response.getData().stream() if a 200 body deserializes with null data; and documents.get(item.getIndex()) trusts the API index is in range.
  8. [Low] Unused import org.junit.jupiter.api.Disabled in VoyageAIIntegrationTest — likely a checkstyle failure in this project.

Items 1–4 are the ones I'd want addressed; the CI fixes themselves are correct and can stay as-is.

@fzowl

fzowl commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

CI-repair round — merge-gatekeeper status

Investigated the merge-gatekeeper red on upstream microsoft#345 (head 5139ecb). Findings:

Root cause is the fork-PR workflow-approval gate, not a code defect. All five upstream workflow runs on the head commit are stuck at action_required (never executed):

Workflow State
Build Java Semantic Kernel completed/action_required
Spell Check completed/action_required
Check .md links completed/action_required
Run Java Integration Tests and Samples completed/action_required
Merge Gatekeeper completed/action_required

Combined commit status is pending; PR is MERGEABLE but BLOCKED. merge-gatekeeper aggregates the other checks, and since none have been allowed to run, it can never report green. Clearing this requires a Microsoft maintainer to click Approve and run workflows on microsoft#345 (external-contributor gate). No push from this side changes it.

The actual build is verified green locally on the current head (5139ecb), so the checks will pass once approved:

  • ./mvnw -B -Pbug-check -DskipTests -Pcompile-jdk17 clean installsemantickernel-api + aiservices/voyageai BUILD SUCCESS, SpotBugs clean (CT_CONSTRUCTOR_THROW + EI_EXPOSE_REP fixes hold).
  • ./mvnw -B -Pbug-check -Pcompile-jdk17 test -pl aiservices/voyageai33 run, 0 failures, 6 skipped (live-API integration tests skip without credentials).
  • typos -c .github/_typos.toml — clean (Spell Check).
  • Branch is 0 commits behind upstream/main; git diff --stat upstream/main...voyageai_integration is limited to the VoyageAI module, the two reranking API files, PACKAGES.md, and the root pom.xml entry — no unrelated files.

The only unrelated local build noise is semantickernel-data-oracle testcontainers failing because Docker is absent on this machine — that module is outside this diff and passes in CI where Docker is available.

No code change is warranted for this scoped round (merge-gatekeeper only). The separate review items (blocking I/O, OkHttp version, etc.) are out of scope here and left untouched.

@fzowl

fzowl commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

VERDICT:CHANGES_NEEDED

Thanks for this — it's a substantial, well-organized contribution. The diff is cleanly scoped (16 files vs the merge-base with upstream/main), the Javadoc is thorough, the builder patterns and constructor validation are consistent across the four services, and the tests (33) are meaningful. The SpotBugs CI-repair work is correct and well-reasoned: marking the value/service classes final for CT_CONSTRUCTOR_THROW, and annotating the DTO accessors with @SuppressFBWarnings to match the existing HuggingFace/OpenAI/Google pattern. The change is purely additive (no breaking changes) and the 1.5.1-SNAPSHOT version alignment is right.

Commit hygiene: clean. No Co-Authored-By, no "Generated with" trailers, no .claude/ files, correct authorship, and descriptive commit messages.

That said, a few items should be addressed before merge:

1. Module missing from semantickernel-bom (convention gap — should fix).
The module is added to the reactor in the root pom.xml, but unlike huggingface, openai, and google it is not registered in semantickernel-bom/pom.xml. Consumers importing the BOM won't get a managed version for semantickernel-aiservices-voyageai. Add the matching <dependency> entry (${project.version}) alongside the other aiservices modules (~line 96).

2. HTTP client diverges from project convention (design — please justify or align).
Every other aiservices module uses com.azure.core.http.HttpClient. This module introduces okhttp3 as a brand-new dependency to the whole repo, with a hardcoded 4.12.0 version not managed by the parent/BOM. That adds a new transitive-dependency surface and a version that won't track centrally. Prefer reusing azure-core's HttpClient (as HuggingFace does) — or, if okhttp is intentional, at minimum move the version into dependency management and flag the new dependency for the maintainers.

3. Blocking I/O on the subscriber thread (reactive correctness).
VoyageAIClient.sendRequestAsync wraps a blocking okhttp .execute() in Mono.fromCallable(...) with no .subscribeOn(Schedulers.boundedElastic()). This blocks whatever thread subscribes — a real problem in a fully reactive pipeline. By contrast, HuggingFaceClient uses azure-core's non-blocking HttpClient.send() returning a Mono. Either switch to a non-blocking client or offload with boundedElastic().

4. Reactive contract violation (minor).
VoyageAITextRerankingService.rerankAsync throws IllegalArgumentException synchronously for a null/empty query, whereas a method returning Mono should signal via Mono.error(...) (the empty-documents case already returns Mono.just(...)). Same eager-throw pattern is worth auditing across the services.

Minor / optional:

  • Unit tests mock VoyageAIClient, so the client's own serialize → HTTP → deserialize → error-handling path is only exercised by the live integration tests (skipped without credentials). A MockWebServer-based test would close that gap.
  • response.getData() is dereferenced (.size(), .stream()) without a null guard; a malformed response body would NPE rather than surface a clean AIException.

None of these are blockers in spirit — (1) and (3) are the ones I'd want resolved, and (2) is worth a conscious decision from the maintainers. Happy to re-review once addressed.

@karianna

Copy link
Copy Markdown

@johnoliver - Can you review this with a view to future direction of Microsoft Agent Framework. Happy to accept extra functionality in principle.

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