Skip to content

sap-aicore: improve Gemini and Codex support - #1019

Draft
caikn wants to merge 2 commits into
magpie-community:mainfrom
caikn:feat/improve_gemini_and_codex_support
Draft

caikn wants to merge 2 commits into
magpie-community:mainfrom
caikn:feat/improve_gemini_and_codex_support

Conversation

@caikn

@caikn caikn commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

 sap-aicore account / deployment discovery
 ├─ GPT → native Responses
+│  ├─ forward client_metadata and native protocol extensions
+│  ├─ flatten Codex namespace tools; restore identities in JSON/SSE
+│  └─ drop custom/freeform tools and their orphaned history
 ├─ Claude → native Messages
+└─ Gemini → native Vertex generateContent / streamGenerateContent
+   ├─ reconcile gcp-vertexai deployment, active version and catalog
+   └─ preserve signed parts, function IDs, inline images and JSON schemas
  • Retain stateless Responses guards, account-owned credentials and token lifecycle.
  • Document Gemini host prerequisites and unsupported capabilities; keep the OpenCode 2 adapter identical to the canonical script.
  • Keep the already-set package version 0.4.1; no additional version bump.

Evidence

  • Before: Run the branch's regression test against isolated main sources: bun test test/responses.test.js --test-name-pattern 'Codex namespace and custom tools' → 0 pass, 1 fail; reply has no function-call output.
    After: Same regression on this branch → 1 pass, 0 fail; namespaced function calls and results complete the synthetic SAP tool loop.
  • Before: main has no native Gemini transport or gcp-vertexai discovery route.
    After: bun test test/gemini.test.js → 17 pass, 0 fail, covering native routing, signed numeric JSON, images, schemas, streaming/truncation, cancellation and credential redaction.
  • cd packages/sap-aicore && bun test → 371 pass, 0 fail across 18 files.
  • Before: CI adapter consistency check failed for sap-aicore.
    After: Restore canonical adapter; bun test scripts/opencode2.test.mjs scripts/check.test.mjs → 22 pass, 0 fail.
  • git diff --check main...HEAD → clean. No live SAP or Magpie acceptance rerun for this PR; tests use synthetic upstream replies.

Merge Danger

Door: two-way

Revert the changes to restore prior routing and validation. No deployment management, data migration or deletion.

Blast Radius: provider

Changes SAP AI Core discovery, GPT request/tool translation, shared JSON/SSE transport. GPT delegates generation/content validation to SAP; custom/freeform tools are omitted, not executed. Claude's native protocol remains unchanged but shares transport helpers. Gemini signed tool loops require the documented Magpie host fix; plugin changes alone do not fix incompatible host history handling.

- Discover and route Gemini deployments through native Vertex AI requests.
- Preserve signed Gemini tool history and streaming responses.
- Forward GPT fields to AI Core and translate Codex namespace/custom tools.

@yetone yetone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this. Native Gemini and the Codex namespace translation are both useful, and the break tests are good: I reverted 14 behaviours by hand, one at a time, and each one turned its test red. I can't merge it yet, though, because of three problems. The first one is a regression for accounts that use no Gemini at all.

1. Two deployed versions of one Gemini model make the whole discovery fail, GPT and Claude included

packages/sap-aicore/src/discovery.js:449 (resolveGemini) now resolves gcp-vertexai deployments. enrich at src/discovery.js:685-686 throws conflict ("distinct deployments resolved to the same model identity") when two entries share a canonical id with different versions, unless both are IDENTITY_AUTHORITATIVE. Only resolveAnthropic (src/discovery.js:440) ever sets that, so the exemption never applies to two Gemini entries. The result: an account with a GPT deployment plus gemini-2.5-flash deployed at versions 001 and 002 gets no models. On main, the same account lists its GPT model, because main ignored gcp-vertexai rows.

Probe (drop it into packages/sap-aicore/test/). It passes on main's sources and fails at 7f72a5d with SAP AI Core: distinct deployments resolved to the same model identity.:

import { test, afterEach } from "bun:test"
import assert from "node:assert/strict"
import { discover } from "../src/discovery.js"

const ROOT = "https://api.ai.eu10.hana.ondemand.com"
const original = globalThis.fetch
afterEach(() => { globalThis.fetch = original })
const ok = v => ({ ok: true, status: 200, redirected: false, url: "", body: new Response(JSON.stringify(v)).body })
const dep = (id, model, version, cid) => ({ id, status: "RUNNING", configurationId: cid, deploymentUrl: ROOT + "/v2/inference/deployments/" + id, details: { resources: { backendDetails: { model: { name: model, version } } } } })
const cfg = (cid, exe, model, version) => ({ id: cid, scenarioId: "foundation-models", executableId: exe, parameterBindings: [{ key: "modelName", value: model }, { key: "modelVersion", value: version }] })
const cat = (model, exe, provider, versions) => ({ model, provider, executableId: exe, allowedScenarios: [{ scenarioId: "foundation-models", executableId: exe }], versions: versions.map(name => ({ name, contextLength: 1000000, inputTypes: ["text"], deprecated: false, retirementDate: "", streamingSupported: true })) })

test("a GPT account that also has two Gemini versions deployed keeps its GPT model", async () => {
  const deployments = [dep("d1", "gpt-6-astra", "2026-09-03", "c1"), dep("d2", "gemini-2.5-flash", "001", "c2"), dep("d3", "gemini-2.5-flash", "002", "c3")]
  const configs = { c1: cfg("c1", "azure-openai", "gpt-6-astra", "2026-09-03"), c2: cfg("c2", "gcp-vertexai", "gemini-2.5-flash", "001"), c3: cfg("c3", "gcp-vertexai", "gemini-2.5-flash", "002") }
  const catalog = { resources: [cat("gpt-6-astra", "azure-openai", "OpenAI", ["2026-09-03"]), cat("gemini-2.5-flash", "gcp-vertexai", "Google", ["001", "002"])] }
  globalThis.fetch = async href => {
    const url = new URL(href)
    if (url.pathname === "/v2/lm/deployments") return ok({ resources: url.searchParams.get("$skip") === "0" ? deployments : [] })
    if (url.pathname.startsWith("/v2/lm/configurations/")) return ok(configs[url.pathname.split("/").pop()])
    if (url.pathname === "/v2/lm/scenarios/foundation-models/models") return ok(catalog)
    throw new Error("unexpected " + url.pathname)
  }
  const registry = await discover({ aiRoot: ROOT, resourceGroup: "default", bearer: async () => "tok" })
  assert.ok(registry["gpt-6-astra"], "GPT model missing: " + Object.keys(registry))
})

Running two versions side by side is normal while moving between versions. At the least, a conflict in one Gemini identity should not take down other vendors' models. Either pick one version, as the Claude path does, or leave just that identity out.

2. A reply cut off at MAX_TOKENS with no parts is turned into a 502

src/gemini.js:164-165 (responseShape) requires Array.isArray(candidate.content.parts) whenever content is present. Vertex answers {"content":{"role":"model"},"finishReason":"MAX_TOKENS"}, with no parts, when a thinking model spends the whole output budget on thoughts. That is a valid, complete reply. The plugin instead throws inference-response: a whole request fails with "the deployment returned an unexpected response", and a stream ends with an error frame {"error":{"code":502,...}}. The same check is reached through src/gemini.js:236.

Probe (same folder). It fails at 7f72a5d:

const truncated = { candidates: [{ content: { role: "model" }, finishReason: "MAX_TOKENS", index: 0 }], usageMetadata: { promptTokenCount: 3, totalTokenCount: 19, thoughtsTokenCount: 16 }, modelVersion: "gemini-3.8-flash" }
// whole: dispatch() with globalThis.fetch returning JSON.stringify(truncated) → expect status 200
// stream: "data: " + JSON.stringify(truncated) + "\r\n\r\n" as text/event-stream → expect no "error" frame

Please allow a missing parts on a candidate that has a finishReason, and add both cases to test/gemini.test.js.

3. The bytes sent upstream can differ from the request that was validated

src/transport.js:191-206 keeps the raw request text in NATIVE_REQUESTS, and src/gemini.js:202 forwards that text (nativeRequestJSON(body) ?? JSON.stringify(nativeBody)). validateRequest checks the parsed object. When a JSON body repeats a top-level key, the parsed object keeps only the last value, but the raw text still carries both. So this request passes validation, and the googleSearch tool, which the docs say is refused, reaches SAP:

{"contents":[{"role":"user","parts":[{"text":"hi"}]}],"tools":[{"googleSearch":{}}],"tools":[{"functionDeclarations":[{"name":"f"}]}]}

Probe: parse that text with readJSONRequest, run validateRequest, then dispatch with a stubbed fetch, and assert that the sent body has no googleSearch. It fails at 7f72a5d. What upstream does with a repeated key is up to the vendor, so this is minor. Still, a validator should only pass what it actually checked. Either forward the raw text only when re-serialising the parsed object gives the same value, or refuse repeated keys.

Smaller points

  • README.md:15 and docs/api-reference.md:129-131 say Gemini tool loops need a host patch that was "verified in an isolated build" against Magpie 0.1.2990. Current Magpie is far past that, and I couldn't find a magpie issue for the patch. Please file the host problem on yetone/magpie with a reproduction and link the issue, or drop the claim. As written, readers are pointed to an outside repo for a patched binary.
  • The GPT Responses path (src/responses.js) no longer validates fields locally, and the image capability gate is gone. Hosted and custom tools, and custom_tool_call history, are now dropped silently where main answered 400. If that's intended, please say so in the changelog. A client that sends web_search now gets an answer made without it and no error.
  • package.json goes 0.3.2 → 0.4.1 and skips 0.4.0. That's fine if intended.
  • main changed test/ in this package after your base (84ee9b8, #984). Please rebase, so the tests run on the tree that would merge.

What I ran (at 7f72a5d)

  • The package's tests: 371 pass, 0 fail. CI on the PR is green.
  • The full repo suite (bun test under a temp HOME) and bun scripts/check.mjs: 6941 pass, 19 skip, 0 fail across 502 files on Linux (WSL Ubuntu 24.04, Bun 1.4.2). check.mjs passes.
  • I broke each of these by hand, one at a time, and each turned the PR's tests red: forwarding the native bytes, the EOF complete hook, complete=true, the image gate, the stream query check, streamingSupported, restoreCall, dropped history, namespace aliases, the args.done restore, the body model check, content after a finish, stream error redaction, and the tool-choice translation.
  • I diffed src/responses.js against main's: for every request main accepted, the body sent upstream is the same apart from the namespace rewrite. src/messages.js (Claude) is unchanged.
  • Not tried: no live SAP AI Core run (I don't contact vendor APIs in review), no run through a magpie gateway end to end, and no run of the tree merged with main.

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