feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat(core): add Adapter enum (skeleton for issue #302 Phase A) - #297

Merged
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton
May 16, 2026
Merged

feat(core): add Adapter enum (skeleton for issue #302 Phase A)#297
moonming merged 2 commits into
mainfrom
feat/adapter-enum-skeleton

Conversation

@moonming

@moonmingmoonming commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

First sub-PR of api7/AISIX-Cloud#302 Phase A (DP-side Provider→Adapter refactor).

Adds a new closed Adapter enum on aisix-core alongside the existing Provider enum, plus a From<Provider> for Adapter mapping.

This PR is intentionally zero-behavior-change — it only introduces new types. Nothing in the gateway dispatches off Adapter yet, no entity field references it, and Provider continues to drive 100% of runtime behavior. Follow-up sub-PRs in Phase A migrate entities, schema, the Hub, and Bridges to consume Adapter directly.

What this PR does

  1. Adds Adapter enum in crates/aisix-core/src/models/model.rs:
    • Variants: Openai, Anthropic, Bedrock, Vertex, AzureOpenai
    • Same derive set as Provider (Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)
    • #[serde(rename_all = "kebab-case")] so AzureOpenai serializes as "azure-openai"
  2. Adds From<Provider> for Adapter mapping:
  3. Re-exports Adapter from aisix_core::models and the crate root, mirroring Provider.

What this PR does NOT do

  • ❌ Does not remove or modify Provider
  • ❌ Does not change ProviderKey struct
  • ❌ Does not change schema.rs / JSON Schema
  • ❌ Does not change Hub, Bridges, or any dispatch path
  • ❌ Does not change any fixture or wire payload

Serde casing note

The Adapter uses kebab-case while Provider uses lowercase. This is intentional: Provider's values are all single tokens (no hyphens or underscores to disambiguate), but Adapter needs azure-openai to remain readable on the wire. Both are pinned by tests so future edits are surfaced loudly.

Test plan

  • cargo test -p aisix-core — 136 passed, 0 failed (includes 4 new Adapter tests)
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • All 6 Provider variants covered by From<Provider> mapping test
  • All 5 Adapter variants pinned by serialize/deserialize round-trip tests
  • Unknown variant strings (e.g. "gemini", "azureopenai", "azure_openai") rejected by deserialize

References

Summary by CodeRabbit

  • New Features

    • Added a new Adapter API allowing upstream protocol representation and automatic conversion from existing providers.
  • Tests

    • Added comprehensive unit tests covering Adapter serialization/deserialization and provider-to-Adapter mapping.

Review Change Stack

Introduces a new closed `Adapter` enum (Openai, Anthropic, Bedrock,
Vertex, AzureOpenai) alongside the existing `Provider` enum, plus a
`From<Provider> for Adapter` mapping. This is the first sub-PR of issue
api7/AISIX-Cloud#302 Phase A — the broader effort renames DP's
`Provider` to `Adapter` (closed wire-shape set) and adds a separate
open-string vendor identity on the control plane.
Behavior is unchanged in this PR:
- `Provider` still exists, still drives all dispatch.
- No entity field references `Adapter`.
- No schema, no Hub, no Bridge changes.
The Adapter uses `kebab-case` so AzureOpenai serializes as
"azure-openai"; Provider keeps `lowercase` because all its values are
single tokens.
From<Provider> mapping rationale (also documented inline):
- Openai → Openai (direct)
- Anthropic → Anthropic (direct)
- Google → Vertex (Vertex AI wire shape is the production target;
no separate AI Studio adapter)
- Deepseek → Openai (OpenAI-compatible chat completions)
- Cohere → Openai (gateway uses Cohere OpenAI-compat endpoints; #213
Phase 1 — rerank-only)
- Jina → Openai (rerank identity-mapped to OpenAI-compat shape;
#213 Phase 2)
Tests:
- Every Provider variant has its mapped Adapter pinned.
- Adapter serialize/deserialize pinned for all 5 variants, with
azure-openai as the load-bearing kebab-case case.
- Unknown variant strings (e.g. "gemini", "azureopenai") rejected.
CopilotAI review requested due to automatic review settings May 16, 2026 06:35
@coderabbitai

coderabbitaiBot commented May 16, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: ba810998-49f7-4e21-9409-aa9bf5a9336e

📥 Commits

Reviewing files that changed from the base of the PR and between eecdc90 and 5d76cc0.

📒 Files selected for processing (1)
  • crates/aisix-core/src/models/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/aisix-core/src/models/model.rs

📝 Walkthrough

Walkthrough

Adds a new public Adapter enum (kebab-case serde), implements From<Provider> mapping, adds unit tests validating serialization/deserialization and mappings, and re-exports Adapter through the models module and crate root.

Changes

Adapter type and public export

Layer / File(s)Summary
Adapter type definition and conversion
crates/aisix-core/src/models/model.rs
New Adapter enum maps Provider variants (e.g., GoogleVertex, Deepseek/Cohere/JinaOpenai) with kebab-case serde representation for upstream protocol routing.
Adapter test coverage
crates/aisix-core/src/models/model.rs
Unit tests verify every Provider maps to a defined Adapter, validate JSON wire-string serialization including azure-openai, confirm deserialization from kebab-case strings, and reject unknown values.
Public API re-exports
crates/aisix-core/src/models/mod.rs, crates/aisix-core/src/lib.rs
Adapter is added to the models module's multi-line pub use block and re-exported at the crate root, making it publicly accessible.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

Comment @coderabbitai help to get the list of available commands and usage tips.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent audit (CLAUDE.md §8)

Cold review by an audit pass with no shared context. Brief: skeleton PR adding Adapter enum + From<Provider> mapping; PR claims zero behavior change.

Verdict per angle

AngleResult
CorrectnessOK — purely additive types; no runtime path touched
ReliabilityN/A — no I/O, no async, no concurrency surface
SecurityOK — closed-enum deserialize rejects unknown variants (test covers); no auth/secret surface
Sensitive-info leakageOK — wire strings are public protocol names
Breaking changesNone — no existing public symbol removed/changed
E2E coverageN/A for this PR (no user-visible behavior); unit pinning is appropriate

Findings

MEDIUM-1: Provider::Google → Adapter::Vertex may not reflect current runtime behavior

Provider::Google's default_base_url() today is https://generativelanguage.googleapis.com/v1beta/openai — that is the Gemini Generative Language API via its OpenAI-compatibility endpoint, not Vertex AI. So the wire shape Provider::Google actually speaks at runtime is the OpenAI wire shape, not the Vertex AI wire shape.

If Adapter represents "wire shape the gateway encodes against" (as the new doc comment says: "the closed set of upstream protocols the gateway knows how to encode against"), then the consistent mapping is:

Provider::Google => Adapter::Openai,

The current Provider::Google => Adapter::Vertex mapping appears to assume a future migration of Provider::Google from the AI Studio compat endpoint to native Vertex. That assumption is reasonable as a forward-looking choice for the Phase A refactor target, but it is not the current wire shape and downstream PRs that flip dispatch onto Adapter will silently change Google's request encoding unless the mapping is revisited.

Suggested action (either is acceptable, but pick one explicitly):

  • (a) Change the mapping to Provider::Google => Adapter::Openai to match the current default_base_url, and let a separate PR introduce a future Adapter::Vertex migration with its own e2e coverage.
  • (b) Keep Provider::Google => Adapter::Vertex but expand the inline doc comment to say explicitly: "this intentionally diverges from the current runtime behavior, which speaks OpenAI-compat against /v1beta/openai; downstream Phase A PRs must migrate the dispatch path before this mapping is applied at runtime." — and link to the tracking issue's Phase A migration step where the dispatch flip is gated.

Either way, this needs to be explicit so the next PR that wires Adapter into dispatch doesn't silently change wire shape for Google traffic.

LOW-1: Provider described as "legacy" in doc-comment is premature

The From<Provider> for Adapter impl doc-comment calls Provider "the legacy Provider enum" — but Provider is still the source of truth in this PR (and across Phase A until the migration completes). "Legacy" reads as "deprecated, do not use" which a current reader of the codebase might be confused by, since they will continue to add Provider-driven code throughout Phase A.

Suggested code: change line 84 (in the patched file):

- /// Best-effort mapping from the legacy `Provider` enum onto the+ /// Mapping from the current `Provider` enum onto the

LOW-2: No as_str() / default_base_url() on Adapter

Provider exposes as_str() and default_base_url(). Adapter exposes neither. This is intentional for a skeleton PR (no caller exists yet), but should be tracked so the next PR in Phase A doesn't quietly skip the helpers. Suggest noting in the PR description test-plan section that helpers are deferred to the entity-migration sub-PR. (The PR body already broadly says follow-ups will migrate entities/Hub/Bridges, so this is borderline NIT.)

Merge gate

  • MEDIUM-1 is the only blocking item per §8. Please either (a) change the mapping or (b) expand the inline doc to gate it explicitly. Once that is done, the rest is LOW/NIT and not blocking.

…dback
Address audit MEDIUM-1 and LOW-1 on PR #297 without changing the
mapping itself (per Phase A plan):
- LOW-1: drop the word 'legacy' on Provider — Provider is still the
source of truth across Phase A; 'legacy' was misleading.
- MEDIUM-1: spell out that Provider::Google → Adapter::Vertex is a
forward-looking mapping that intentionally diverges from current
runtime behavior (Provider::Google today speaks OpenAI-compat
against /v1beta/openai). Any downstream PR that flips dispatch
onto Adapter must either also migrate the Google bridge to native
Vertex AI encoding or revisit this arm before merging.
No code/runtime change; only the doc-comment for the From impl.
@moonming

Copy link
Copy Markdown
MemberAuthor

Audit response

Addressed in 5d76cc0:

  • MEDIUM-1 — kept the Provider::Google → Adapter::Vertex mapping (this is the forward-looking Phase A target per the tracking issue), but expanded the inline doc-comment on the From<Provider> impl to spell out the divergence from current runtime behavior explicitly: any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI encoding or revisit this arm before merging. The mapping is not wired into dispatch in this PR, so no runtime change.
  • LOW-1 — dropped the word "legacy" on Provider in the doc-comment; Provider is still the source of truth across Phase A.
  • LOW-2 — accepted as deferred to the entity-migration sub-PR (no as_str() / default_base_url() on Adapter until a caller exists).

cargo fmt / cargo test / cargo clippy all still clean.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@moonming

Copy link
Copy Markdown
MemberAuthor

Independent third-party audit (separate from author self-review)

Cold review pass by an audit agent with no shared context. Brief: skeleton PR adding closed Adapter enum + From<Provider> mapping under issue #302 Phase A; claim is zero-behavior-change, type-only addition.

Note: I have read the author self-review (the "Independent audit (CLAUDE.md §8)" comment above) and the follow-up commit 5d76cc0. This audit is independent of that pass and verifies the mitigation actually landed plus checks the angles the self-review may not have covered.

Verdict per §8 angle

AngleResult
CorrectnessOK with one note (see LOW-1) — closed-enum match exhaustive, every Provider arm pinned by test, serde casing pinned by serialize+deserialize round-trip
ReliabilityN/A — pure types, no I/O / async / concurrency surface introduced
SecurityOK — closed enum, unknown variant rejected (pinned by adapter_rejects_unknown_variant_strings); no auth or secret surface
Sensitive-info leakageN/A — wire strings are public protocol identifiers
Breaking changesNone — Adapter is a net-new symbol, no existing symbol removed or renamed; verified no name collision (see below)
E2E coverageN/A — type-only, not yet consumed by any dispatch path; unit pinning is the correct test level
Scope creepStrictly within stated scope — Provider untouched, ProviderKey untouched, schema/Hub/Bridge untouched, only re-export blocks updated minimally

Independent verification steps I ran

  1. Name-collision sweep: rg "(struct|enum|trait|pub use).*Adapter" across crates/ — zero existing type/trait named Adapter. The word appears only in two doc-comments (aisix-etcd/src/etcd_provider.rs:187, aisix-guardrails/src/build.rs:149) describing unrelated adapter patterns in prose; no symbol conflict.
  2. No live consumer: rg "Adapter::from|<Adapter as From|use.*::Adapter" — only the new tests in model.rs reference Adapter::from. No downstream crate has been wired to consume the enum yet, which matches the PR's "zero behavior change" claim.
  3. From<Provider> exhaustiveness: the match provider { ... } arm covers all 6 current Provider variants. Compiler will reject any future Provider addition without an explicit From arm — matched by adapter_from_provider_covers_every_variant which pins each chosen mapping (so a future silent edit also fails the test).
  4. Casing choice: kebab-case vs Provider's lowercase is correctly justified — AzureOpenai needs disambiguation (azure-openai vs azureopenai); the rejection test explicitly rejects both \"azureopenai\" and \"azure_openai\", locking the wire contract.
  5. Re-export placement: alphabetical-ish slot in both lib.rs:34 and models/mod.rs:37 matches existing pattern. Minimal diff.
  6. MEDIUM-1 mitigation verification: the doc-comment in 5d76cc0 (lines 101–110) now says explicitly that Provider::Google → Adapter::Vertex diverges from current runtime behavior and that "any downstream PR that flips dispatch onto Adapter MUST either also migrate the Google bridge to native Vertex AI request encoding, or revisit this arm before merging." This is acceptable per §8's "explicitly justified" gate, because the mapping is not consumed by any dispatch path in this PR and the next-PR contract is recorded inline.

Findings

MEDIUM-1 (author-disclosed, mitigation verified)

Author already disclosed and addressed in commit 5d76cc0. I confirm:

  • The mitigation is in place: the inline doc-comment on From<Provider> for Adapter now explicitly states the divergence between Adapter::Vertex and the current Provider::Google wire shape (/v1beta/openai), and gates any downstream dispatch flip behind a Google-bridge migration.
  • The mitigation is sufficient for this PR because:
    • No code currently consumes Adapter::from(Provider::Google). Search confirms zero downstream callers.
    • The next-PR contract is recorded at the point of failure (the From impl itself), so the engineer who wires Adapter into dispatch cannot miss it.
  • Status: resolved. No further action required in this PR. Tracking-wise, the Phase A migration PR(s) that flip dispatch onto Adapter should cite this doc-comment in the design notes per §7 (reference-implementation discipline).

LOW-1: From<Provider> for an enum maps Cohere/Jina to Adapter::Openai, but they have distinct upstreams — confirm this is the intended "wire shape" semantics

Per the docstring on Adapter, the type represents "the closed set of upstream protocols the gateway knows how to encode against — distinct from a vendor identity (which is captured separately on ProviderKey)." Mapping Cohere → Openai and Jina → Openai is therefore correct under that semantics — the gateway currently calls Cohere's and Jina's OpenAI-compatible rerank endpoints with OpenAI wire shape (confirmed via crates/aisix-proxy/src/rerank.rs lines 113–124 and 255–261). The vendor identity remains on ProviderKey / Provider, exactly as the doc-comment promises.

This is a note, not a defect: the mapping is consistent with the stated semantics. The reason I flag it as LOW is that the next-PR contract for any future native-Cohere or native-Jina adapter implementation is not recorded in this PR — if Adapter::Cohere is later introduced, the migration will need to revisit the From<Provider> arms similarly to the Google → Vertex case. Suggest tracking that in the issue #302 Phase A plan so it doesn't get re-discovered the hard way.

Optional suggested action: none required for this PR. If you want belt-and-suspenders, append one sentence to the existing doc-comment on the Cohere/Jina arms:

 /// - `Cohere` → `Openai`: the gateway currently talks to Cohere's
/// OpenAI-compatible endpoints (#213 Phase 1 — rerank-only),
/// so the wire adapter is `openai`. A native Cohere adapter is
- /// not part of this skeleton.+ /// not part of this skeleton; a future `Adapter::Cohere` would+ /// require revisiting this arm alongside a native-Cohere bridge,+ /// following the same downstream-migration discipline as+ /// `Provider::Google → Adapter::Vertex` above.

Not blocking. Pure forward-compat note.

LOW-2: Doc-comment word choice — "wire-shape" vs "protocol" consistency

The new doc-comment uses both "wire-shape adapter" (line 65) and "upstream protocols the gateway knows how to encode against" (line 66) in the same paragraph. Both are clear in context, but downstream readers grepping for "wire shape" or "protocol" will get partial matches. Suggest picking one and threading it consistently. Pure NIT, not blocking.

LOW-3: No as_str() / default_base_url() on Adapter (author-acknowledged)

Author already acknowledged this as "deferred to entity-migration sub-PR" in the response comment. I agree this is the right call for a skeleton PR — adding helpers without a caller would violate §2 (no speculative code). Resolved.

Merge gate (per §8)

  • All previously raised MEDIUM/LOW are either resolved in commit 5d76cc0 or are NITs that don't block.
  • My independent pass surfaced no NEW HIGH or MEDIUM findings.
  • The LOW-1 forward-compat note is optional and can be addressed in a follow-up if you want symmetry across From<Provider> arms.

Verdict: PR #297 passes independent audit. Recommend merge.

The Provider::Google → Adapter::Vertex mitigation is the load-bearing part — the inline doc-comment is precise about (a) the current runtime wire shape, (b) why this PR keeps the forward-looking mapping, and (c) what the next PR's contract is. That's exactly the kind of explicit justification §8 requires.

Sign up for freeto 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

@moonming