fix(core): enforce the a2a/mcp name and credential rules on every config path - #848
fix(core): enforce the a2a/mcp name and credential rules on every config path#848moonming wants to merge 3 commits into
Conversation
…fig path
The per-`auth_type` credential coupling and the name-shape rules lived
only in this gateway's Admin API write handlers. Declaratively configured
gateways never saw them, so a `resources.yaml` like
a2a_agents:
- name: bad/name
url: https://agents.example.com/a2a
auth_type: bearer
loaded clean on main (`OK: loaded 1 resource(s)`) even though the name is
the `/a2a/<name>` path segment and `bearer` has no secret to send.
Lifts four rules into the canonical schemas, following the
`model_one_of` pattern — defined once, injected into the generated
schema, so the published JSON Schema and every runtime validator share
one definition:
- a2a agent `name` must not contain `/` (it is the URL path segment)
- mcp server `name` must not contain `__` (tools are exposed as
`<name>__<tool>`, so the separator would make the split ambiguous)
- a2a `bearer`/`api_key` require a non-empty `secret`
- mcp `bearer`/`api_key` require `secret`; `oauth2` additionally
requires `client_id` and `token_url`
One assertion is deliberately inverted:
`mcp_server_schema_stays_permissive_on_credential_coupling` asserted
that an incomplete oauth2 row still validates, on the reasoning that the
write path would catch it and the runtime would degrade that server
gracefully. That reasoning depends on a write path existing. Rejecting at
load is also the more diagnosable failure — a rejected row is named in
`GET /status/config`'s `rejected` array, where a loaded-but-degraded
server silently serves no tools. The rationale is recorded at the test.
The write handlers' own checks now sit behind the schema gate, so their
tests see `AdminError::Schema` instead of `BadRequest`. Both map to 400,
so the wire contract is unchanged; those assertions now check the status.
Verified with a locally built `aisix validate`: each bad document is
rejected with a precise pointer, and valid ones — including a trailing
single underscore in an mcp name — still load.
📝 WalkthroughWalkthroughAdds conditional credential validation and stricter identifier patterns for A2A agents and MCP servers, wires these rules into generated and published schemas, updates admin tests to assert ChangesSchema validation
Export integrity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminRequest
participant decode
participant RootSchema
participant JSONSchemaValidator
AdminRequest->>decode: submit agent or MCP configuration
decode->>RootSchema: validate configuration
RootSchema->>JSONSchemaValidator: apply name and credential rules
JSONSchemaValidator-->>decode: return validation result
decode-->>AdminRequest: return 400 Bad Request on rejection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/aisix-admin/src/mcp_servers_handlers.rs (1)
95-132: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDead code: schema validation on line 96 already subsumes these checks.
validate_mcp_server(raw)?now enforces both the__-in-name rule and the per-auth_typecredential coupling via the injected schemaallOf. Since it runs first and returns early on any violation, lines 99-132 can never execute for the cases they guard — the test module's own comment above (lines 200-205) confirms "the schema gate now rejects these payloads beforedecode's own checks run." Leaving this in place is dead code and a second, now-divergence-prone source of truth for the same rule.🧹 Proposed removal of now-unreachable checks
let server: McpServer = serde_json::from_value(raw.clone()) .map_err(|e| AdminError::BadRequest(format!("malformed McpServer payload: {e}")))?; - if server.name.contains(TOOL_NAMESPACE_SEPARATOR) { - return Err(AdminError::BadRequest(format!( - "name must not contain the reserved separator `{TOOL_NAMESPACE_SEPARATOR}`" - ))); - } - // Per-auth_type credential coupling. The JSON schema stays flat and - // permissive on this (see the note on the McpServer struct); the write - // path is where an incomplete credential set is rejected outright. - let has_secret = !server.secret.as_deref().unwrap_or_default().is_empty(); - match server.auth_type { - McpAuthType::None => {} - McpAuthType::Bearer if !has_secret => { - return Err(AdminError::BadRequest( - "secret is required and must be non-empty when auth_type is `bearer`".to_string(), - )); - } - McpAuthType::ApiKey if !has_secret => { - return Err(AdminError::BadRequest( - "secret is required and must be non-empty when auth_type is `api_key`".to_string(), - )); - } - McpAuthType::OAuth2 => { - let has_client_id = !server.client_id.as_deref().unwrap_or_default().is_empty(); - let has_token_url = !server.token_url.as_deref().unwrap_or_default().is_empty(); - if !has_secret || !has_client_id || !has_token_url { - return Err(AdminError::BadRequest( - "client_id, token_url, and secret (the OAuth client secret) are required \ - and must be non-empty when auth_type is `oauth2`" - .to_string(), - )); - } - } - McpAuthType::Bearer | McpAuthType::ApiKey => {} - } // Per-type coupling: an openapi-backed server must carry a spec thatNote: verify
TOOL_NAMESPACE_SEPARATORisn't left unused elsewhere in this file after removal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-admin/src/mcp_servers_handlers.rs` around lines 95 - 132, Remove the redundant name-separator and auth_type credential checks from decode after validate_mcp_server(raw)?; retain deserialization and error mapping. Verify TOOL_NAMESPACE_SEPARATOR and any related imports or helpers remain used elsewhere, removing unused ones if necessary.crates/aisix-core/src/models/mcp_server.rs (1)
84-91: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale comment contradicts the credential-coupling function added right below it.
This comment still says the
oauth2/bearer/api_keycoupling "is deliberately NOT expressed in this flat schema" and is enforced by "the control plane... this gateway's own Admin API... at write time."mcp_server_credential_coupling()(added a few lines below in this same PR) now expresses exactly this coupling in the schema viaallOf/if/then, and per the admin test suite the Admin API write-path checks are now dead code the schema gate preempts (see the linked comment inmcp_servers_handlers.rs). This is the same doc drift the siblinga2a_agent.rscomment was correctly updated to avoid.📝 Proposed comment update (mirrors a2a_agent.rs's updated wording)
- // Cross-field coupling (`oauth2` requires `client_id` + `secret` + - // `token_url`; `bearer`/`api_key` require `secret`) is deliberately NOT - // expressed in this flat schema — that would force restructuring the - // resource into a oneOf. The control plane enforces the coupling strictly - // at write time, this gateway's own Admin API re-checks it on write, and - // the runtime degrades gracefully when a snapshot-loaded server is - // mis-configured: its credential exchange fails, its tools become - // unavailable, and the failure is logged like any other upstream failure. + // Cross-field coupling (`oauth2` requires `client_id` + `secret` + + // `token_url`; `bearer`/`api_key` require `secret`) is expressed as an + // injected `allOf` of `if`/`then` subschemas rather than in this flat + // struct — see `mcp_server_credential_coupling`. That keeps the resource + // flat (no oneOf restructuring) while giving the published schema and + // every runtime validator one shared definition, so a declarative + // `resources.yaml` and the control plane reject the same documents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/mcp_server.rs` around lines 84 - 91, Update the comment immediately above mcp_server_credential_coupling() to describe that oauth2, bearer, and api_key credential requirements are now enforced by the schema’s conditional allOf/if/then rules. Remove the outdated claims that coupling is intentionally absent and primarily enforced by write-time control-plane or Admin API checks, while retaining the runtime misconfiguration behavior if still applicable.
🧹 Nitpick comments (1)
crates/aisix-core/src/models/a2a_agent.rs (1)
141-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
thenblock could be factored out like the MCP sibling.
mcp_server_credential_coupling()extracts the repeated{"required": ["secret"], "properties": {...}}into asecret_requiredvariable; this function inlines the same block twice. Minor stylistic inconsistency across the two nearly-identical functions added in this PR.♻️ Optional dedup
pub fn a2a_agent_credential_coupling() -> Value { + let secret_required = json!({ + "required": ["secret"], + "properties": { "secret": { "type": "string", "minLength": 1 } } + }); json!([ { "if": { "properties": { "auth_type": { "const": "bearer" } }, "required": ["auth_type"] }, - "then": { - "required": ["secret"], - "properties": { "secret": { "type": "string", "minLength": 1 } } - } + "then": secret_required }, { "if": { "properties": { "auth_type": { "const": "api_key" } }, "required": ["auth_type"] }, - "then": { - "required": ["secret"], - "properties": { "secret": { "type": "string", "minLength": 1 } } - } + "then": secret_required } ]) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/a2a_agent.rs` around lines 141 - 158, Refactor a2a_agent_credential_coupling() to extract the repeated secret requirement and string validation object into a shared secret_required value, then reuse it in both bearer and api_key then clauses. Preserve the existing validation behavior and structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/aisix-admin/src/mcp_servers_handlers.rs`:
- Around line 95-132: Remove the redundant name-separator and auth_type
credential checks from decode after validate_mcp_server(raw)?; retain
deserialization and error mapping. Verify TOOL_NAMESPACE_SEPARATOR and any
related imports or helpers remain used elsewhere, removing unused ones if
necessary.
In `@crates/aisix-core/src/models/mcp_server.rs`:
- Around line 84-91: Update the comment immediately above
mcp_server_credential_coupling() to describe that oauth2, bearer, and api_key
credential requirements are now enforced by the schema’s conditional
allOf/if/then rules. Remove the outdated claims that coupling is intentionally
absent and primarily enforced by write-time control-plane or Admin API checks,
while retaining the runtime misconfiguration behavior if still applicable.
---
Nitpick comments:
In `@crates/aisix-core/src/models/a2a_agent.rs`:
- Around line 141-158: Refactor a2a_agent_credential_coupling() to extract the
repeated secret requirement and string validation object into a shared
secret_required value, then reuse it in both bearer and api_key then clauses.
Preserve the existing validation behavior and structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4369a94c-29a6-4692-9c69-9ca60a547f84
📒 Files selected for processing (7)
crates/aisix-admin/src/a2a_agents_handlers.rscrates/aisix-admin/src/mcp_servers_handlers.rscrates/aisix-core/src/models/a2a_agent.rscrates/aisix-core/src/models/mcp_server.rscrates/aisix-core/src/models/schema.rsschemas/resources/a2a_agent.schema.jsonschemas/resources/mcp_server.schema.json
…trailing `_`
Audit findings on the previous commit.
The mcp name pattern was wrong, and the previous commit message argued
for the wrong side of it. A trailing `_` is not cosmetic: tools are
advertised as `<name>__<tool>` and parsed back with `split_once("__")`,
so `github_` + `list` serializes to `github___list` and resolves to the
non-existent server `github` — every tool call against that server
fails. Worse, `gh_` + `x` and `gh` + `_x` produce the same wire name,
which is the ambiguity the rule exists to prevent. Pattern reverted to
reject both `__` and a trailing `_`, with the reasoning recorded.
Eight more write-handler rules were left behind, not one. All eight are
plain JSON Schema and are now lifted: `spec` is required for and limited
to `type: openapi`, must be an object, and must not be a Swagger 2.0
document; `api_key_header` is limited to `type: openapi` with
`auth_type: api_key` and must be a legal RFC 7230 header name. Only the
three rules that need to parse the OpenAPI document itself
(`aisix_mcp::validate_spec`, header-name typing, duplicate tool names)
remain, blocked by the aisix-mcp → aisix-core dependency direction.
The first draft of that lift silently did nothing: it used
`dependentSchemas`, a draft 2019-09 keyword, in a schema that declares
draft-07, so the validator ignored it as unknown. Unit tests passed.
Caught by running each case through a built `aisix validate`; now spelled
`dependencies` and covered by a regression test that says why.
Also from the audit:
- the a2a name pattern only excluded `/`, but the name is interpolated
into the advertised agent-card URL unencoded, so `a?b` advertises a URL
whose path is `/a2a/a`. Now excludes `/?#%`, whitespace and control
characters
- four comments asserted the schema stays permissive, directly above the
code that now enforces it
- restored the `client_id` doc comment an earlier comment rewrite
swallowed, and titled the new `anyOf` branches for the ReDoc tabs
- `decode_openapi_type_coupling` asserted on handler-authored messages
that the schema gate now pre-empts; it asserts the 400 and the failing
pointer instead
One diagnosability regression is accepted and recorded at the
constraint: rejecting Swagger 2.0 in the schema loses the write path's
"convert the spec to OpenAPI 3.x" hint, because a validator reports a
pointer and a constraint, not advice. A post-schema semantic hook in the
loaders would let the rule and the advice live together.
… rule
Findings from a second review pass focused on lifecycle, export round-trip
and mutation coverage.
`aisix export` omitted rows the loader rejected, printed a warning, and
exited 0. Export is the migration path off a store-backed deployment, so
a scripted migration would treat an incomplete file as a finished one and
lose those resources silently. A rejected row now fails the export the
same way a non-loadable file does; the written file stays for inspection.
`api_key_header: null` was rejected. The field is `Option<String>` and
an explicit null means absent — the comment right above the constraint
said so — but `dependencies` keys off presence alone, null included. It
is now a value-sensitive `if`/`then` on `type: string`, verified both
ways: null loads, a real header with non-api_key auth still fails.
The suite pinned only 5 of the 12 lifted rules at single-constraint
granularity. `spec` must-be-an-object was pinned by nothing — deleting it
left the string fixture rejected by the sibling swagger check, so the
test passed either way. Added mutation-pinning tests that start from a
fully valid document and violate exactly one obligation: each oauth2
field independently, empty and null for every credential, an explicit
`type: mcp` carrying openapi-only fields, a header with
`auth_type: none`, and every excluded a2a character under both the `name`
and `display_name` spellings.
One test fixture called `{"openapi": "3.0.0"}" a complete spec, but tool
generation needs a `paths` object — it now uses a spec that generates a
tool.
Second review pass — findings and dispositionA second reviewer went at this from angles the first pass did not cover: revert-fails-test (would deleting each constraint actually fail a test), the full ingestion lifecycle, export round-trip, and upgrade hazard. It found two things the first pass missed. I reproduced every claim before acting on it. Fixed in
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/aisix-core/src/models/mcp_server.rs (1)
412-414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: implementation no longer uses
dependencies.This comment says the
api_key_header/auth_typecoupling is "guarded by draft-07dependencies", but the actual schema (mcp_server_credential_coupling, lines 252-263) uses nestedallOf/if/then, not thedependencieskeyword. This looks like a leftover from an earlier iteration (per the PR's commit history) that was superseded by the value-sensitiveif/thenfix forapi_key_header: null.♻️ Suggested comment update
- // `api_key_header` only makes sense with `auth_type: api_key`, and has to - // be a legal header name. This one is guarded by draft-07 `dependencies` - // — the draft 2019-09 spelling would be ignored, so keep this test. + // `api_key_header` only makes sense with `auth_type: api_key`, and has to + // be a legal header name. This is guarded by a nested `allOf`/`if`/`then` + // (not `dependencies`, which is presence-based and would incorrectly + // reject an explicit `api_key_header: null`); keep this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/mcp_server.rs` around lines 412 - 414, Update the comment near the MCP server schema test to describe the current nested allOf/if/then coupling in mcp_server_credential_coupling, including its value-sensitive handling of api_key_header: null; remove the obsolete references to draft-07 dependencies and its spelling.crates/aisix-admin/src/mcp_servers_handlers.rs (1)
104-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: schema is no longer "flat and permissive" on credential coupling.
This comment says the JSON schema stays permissive and the write path enforces credential/type coupling. That's no longer accurate —
mcp_server.rs'smcp_server_credential_coupling()(wired into the schema inschema.rs) now rejects incomplete credentials and openapi-only-field violations beforevalidate_mcp_server(raw)?on line 96 even lets execution reach these checks. The updated test-module comment (lines 200-205) already reflects this; this comment indecode()doesn't.Most of lines 107-181 are effectively dead/unreachable now (the schema already rejects those payloads), except the
aisix_mcp-dependent parts (spec parsing, duplicate-tool detection) that must stay on the write path per the PR's design. Consider updating the comment and/or trimming the now-redundant per-field checks to avoid future drift between this function and the schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-admin/src/mcp_servers_handlers.rs` around lines 104 - 181, Update the credential-coupling comment in decode() to reflect that mcp_server_credential_coupling() in the schema already rejects incomplete credentials and openapi-only fields before these checks run. Remove the redundant per-authentication and field-coupling validation from decode(), while retaining aisix_mcp::validate_spec and other write-path checks that require OpenAPI parsing or tool-generation validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-core/src/models/a2a_agent.rs`:
- Around line 29-38: Update the regex pattern on the A2A agent name field in the
surrounding serde/schemars attributes to also reject DEL (U+007F), while
preserving the existing exclusions for separators, percent signs, whitespace,
and C0 controls.
In `@crates/aisix-server/src/export/mod.rs`:
- Around line 107-113: Update the export error flow around stats.rejections and
document.blocking so both rejection and blocking-issue diagnostics are reported
before returning failure. Move the rejection bail after the existing
blocking-reporting block, or otherwise aggregate both reports while preserving
one non-zero result and the separate handling of blocking export issues.
---
Nitpick comments:
In `@crates/aisix-admin/src/mcp_servers_handlers.rs`:
- Around line 104-181: Update the credential-coupling comment in decode() to
reflect that mcp_server_credential_coupling() in the schema already rejects
incomplete credentials and openapi-only fields before these checks run. Remove
the redundant per-authentication and field-coupling validation from decode(),
while retaining aisix_mcp::validate_spec and other write-path checks that
require OpenAPI parsing or tool-generation validation.
In `@crates/aisix-core/src/models/mcp_server.rs`:
- Around line 412-414: Update the comment near the MCP server schema test to
describe the current nested allOf/if/then coupling in
mcp_server_credential_coupling, including its value-sensitive handling of
api_key_header: null; remove the obsolete references to draft-07 dependencies
and its spelling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56395e86-2456-467b-8a64-54150859e507
📒 Files selected for processing (8)
crates/aisix-admin/src/a2a_agents_handlers.rscrates/aisix-admin/src/mcp_servers_handlers.rscrates/aisix-core/src/models/a2a_agent.rscrates/aisix-core/src/models/mcp_server.rscrates/aisix-core/src/models/schema.rscrates/aisix-server/src/export/mod.rsschemas/resources/a2a_agent.schema.jsonschemas/resources/mcp_server.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/aisix-admin/src/a2a_agents_handlers.rs
| /// be a single non-empty URL path segment. The name is interpolated into the | ||
| /// advertised agent-card URL without percent-encoding, so `/`, `?`, `#`, `%` | ||
| /// and whitespace are rejected: `a?b` would advertise a URL whose path is | ||
| /// just `/a2a/a`, and the lookup is an exact match on the stored name. | ||
| // `display_name` is the field's former name; stored documents and | ||
| // callers that still use it keep deserializing (schema-side acceptance | ||
| // lives in `schema::a2a_agent_root_schema`). Re-serialization always | ||
| // emits `name`. | ||
| #[serde(alias = "display_name")] | ||
| #[schemars(length(min = 1))] | ||
| #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f]+$"), length(min = 1))] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Regex omits DEL (0x7F), a control character.
The pattern ^[^/?#%\s\x00-\x1f]+$ only excludes C0 controls (0x00–0x1f). DEL (U+007F) is also a control character (Unicode category Cc) but isn't blocked here, so a name containing DEL would still validate despite the stated goal of rejecting "control characters" generically.
🐛 Proposed fix
- #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f]+$"), length(min = 1))]
+ #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f\\x7f]+$"), length(min = 1))]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// be a single non-empty URL path segment. The name is interpolated into the | |
| /// advertised agent-card URL without percent-encoding, so `/`, `?`, `#`, `%` | |
| /// and whitespace are rejected: `a?b` would advertise a URL whose path is | |
| /// just `/a2a/a`, and the lookup is an exact match on the stored name. | |
| // `display_name` is the field's former name; stored documents and | |
| // callers that still use it keep deserializing (schema-side acceptance | |
| // lives in `schema::a2a_agent_root_schema`). Re-serialization always | |
| // emits `name`. | |
| #[serde(alias = "display_name")] | |
| #[schemars(length(min = 1))] | |
| #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f]+$"), length(min = 1))] | |
| /// be a single non-empty URL path segment. The name is interpolated into the | |
| /// advertised agent-card URL without percent-encoding, so `/`, `?`, `#`, `%` | |
| /// and whitespace are rejected: `a?b` would advertise a URL whose path is | |
| /// just `/a2a/a`, and the lookup is an exact match on the stored name. | |
| // `display_name` is the field's former name; stored documents and | |
| // callers that still use it keep deserializing (schema-side acceptance | |
| // lives in `schema::a2a_agent_root_schema`). Re-serialization always | |
| // emits `name`. | |
| #[serde(alias = "display_name")] | |
| #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f\\x7f]+$"), length(min = 1))] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aisix-core/src/models/a2a_agent.rs` around lines 29 - 38, Update the
regex pattern on the A2A agent name field in the surrounding serde/schemars
attributes to also reject DEL (U+007F), while preserving the existing exclusions
for separators, percent signs, whitespace, and C0 controls.
| if !stats.rejections.is_empty() { | ||
| anyhow::bail!( | ||
| "export omitted {} rejected entr(ies); the resources file is incomplete. \ | ||
| Fix them in the source and re-export — the written file is for inspection only", | ||
| stats.rejections.len() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not suppress blocking-issue diagnostics when rejections also exist.
If stats.rejections and document.blocking are both non-empty, this bail exits before the collision/dangling-reference details at Lines 118-125 are printed. Report both classes first, then return one non-zero result (or move this bail after the existing blocking-reporting block). This preserves the stated separate handling for blocking export issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aisix-server/src/export/mod.rs` around lines 107 - 113, Update the
export error flow around stats.rejections and document.blocking so both
rejection and blocking-issue diagnostics are reported before returning failure.
Move the rejection bail after the existing blocking-reporting block, or
otherwise aggregate both reports while preserving one non-zero result and the
separate handling of blocking export issues.
1 participant
Prerequisite for removing the Admin API write path — and a bug fix in its own right.
The bug
The per-
auth_typecredential coupling, the name-shape rules, and thetype/spec/api_key_headercoupling are enforced only in this gateway's Admin API write handlers. A declaratively configured gateway never sees them. Onmaintoday:Both problems are real: a slash splits the agent's route into two segments, and
bearerwith no secret means the gateway authenticates upstream with nothing. The struct comments said this was deliberate — "this gateway's own Admin API re-checks it on write" — but that design leans on a write path that is about to be removed.What is lifted
Twelve rules, defined once and injected into the generated schema following the existing
model_one_of/mcp_policy_root_schemapattern, so the published JSON Schema and every runtime validator share one definition:nameexcludes/?#%, whitespace, control charsa2a.rs:237), soa?badvertises a URL whose path is just/a2a/anameexcludes__and a trailing_<name>__<tool>and parsed withsplit_once("__");github_+list→github___list→ resolves to the non-existent servergithub, andgh_+xcollides withgh+_xbearer/api_key⇒ non-emptysecretoauth2⇒secret+client_id+token_urlspecrequired for, and limited to,type: openapi; must be an object; must not be Swagger 2.0api_key_headerlimited totype: openapi+auth_type: api_key; must be a legal RFC 7230 header nameCross-field coupling is an injected
allOfofif/thensubschemas, so the resources stay flat — nooneOfrestructuring, which the original comments gave as the reason for leaving the rules out.Not lifted (3 rules)
aisix_mcp::validate_spec, thehttp::HeaderNametyping, and duplicate-tool-name detection all need to parse the OpenAPI document, andaisix-mcpdepends onaisix-core— lifting them would be a dependency cycle. They stay on the write path for now; their disposition belongs to the removal PR. (The header-name shape is covered by the RFC 7230 pattern above; only the exactHeaderNameparse is not.)One assertion is deliberately inverted.
mcp_server_schema_stays_permissive_on_credential_couplingasserted that an incompleteoauth2row still validates, so the loader keeps it and the runtime degrades that server gracefully. That reasoning depended on a write path existing to catch it. Rejecting at load is also the more diagnosable failure — a rejected row is named inGET /status/config'srejected[], whereas a loaded-but-degraded server silently serves no tools. The rationale is recorded at the test.Upgrade impact differs by configuration source:
/status/config'srejected[]; the gateway keeps serving (degraded).resources.yaml: the file source is all-or-nothing (filesource/status.rs:6). A single now-invalida2a_agents/mcp_serversentry rejects the whole file, so a gateway that boots with one exits at startup (aisix-server/src/main.rs:493) rather than starting degraded. Runaisix validate --resources <file>before rolling the image. OnSIGHUPthe last-good snapshot is retained and the entry appears inrejected[].One diagnosability regression, accepted and recorded at the constraint. Rejecting Swagger 2.0 in the schema loses the write path's targeted "convert the spec to OpenAPI 3.x" hint — a validator reports a pointer and a constraint, not advice. The rule is intact on every path. A post-schema semantic hook in the loaders would let the rule and the advice live together; worth doing, out of scope here.
Also
The write handlers' own checks now sit behind the schema gate, so their tests observe
AdminError::Schemainstead ofBadRequest. Both map to400(error.rs:46), so the wire contract is unchanged; those assertions now check the status and the failing pointer. Those handler branches are consequently unreachable — they are the thing being lifted, and the removal PR deletes them.Verification
cargo fmt,clippy --workspace --all-targetsclean, full workspace tests pass, schemas regenerated so the drift gate passes.Every rule was then exercised against a locally built
aisix validate, not just unit tests — which mattered: the first draft of theapi_key_headercoupling useddependentSchemas(draft 2019-09) in a schema declaring draft-07, so the validator silently ignored it while every unit test passed. Running the nine documents through the binary caught it. It is nowdependencies, with a regression test that says why.Summary by CodeRabbit
New Features
Bug Fixes
Documentation