feat(filter): add shared AI classifier and anthropic_messages_format filter#587
Conversation
…filter Extract AI request format classification into a shared classifier module and add the anthropic_messages_format filter for Anthropic Messages API detection. The classifier promotes format facts to internal headers for downstream routing. Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
873d487 to
76c3827
Compare
|
looks like the Coding Conventions are failing due to the
|
praxis-bot
left a comment
There was a problem hiding this comment.
Review: feat(filter): add shared AI classifier and anthropic_messages_format filter
Clean extraction of the classifier into a shared module, and a well-structured new filter. The code follows project conventions closely and the classifier disambiguation logic for Anthropic vs ChatCompletions is reasonable. Several test coverage gaps and one correctness concern below.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | Medium | classifier/mod.rs |
classify_format uses prompt object presence to classify as Responses, but old code did not. This silently changes behavior for existing openai_responses_format users. |
| 2 | Medium | messages_format/mod.rs |
Missing on_invalid: continue test for non-JSON/invalid-JSON. Only reject-mode negative tests exist. |
| 3 | Medium | messages_format/tests.rs |
run_filter utility always uses path /v1/messages, which triggers the path-based override. No test exercises body-only Anthropic classification. |
| 4 | Medium | messages_format/tests.rs |
No test for on_invalid: reject with NonJson or UnknownJson formats, only InvalidJson is tested. |
| 5 | Medium | Project | Per CLAUDE.md test requirements: no example config and no integration test for the new filter. |
| 6 | Small | messages_format/mod.rs |
max_tokens written to metadata but not promoted to filter results. Asymmetric with stream. |
| 7 | Small | classifier/mod.rs |
has_anthropic_signals doc comment incomplete re: system array coverage. |
| 8 | Small | messages_format/tests.rs |
No test for stream: false being promoted. |
| 9 | Small | messages_format/tests.rs |
No test for header suppression when config field is null. |
| 10 | Nit | messages_format/mod.rs |
is_messages_path lacks trailing-slash normalization unlike is_responses_path. |
| 11 | Nit | classifier/mod.rs |
Extraction adds behavioral changes (prompt object detection, new fields) that should be called out in PR description. |
See inline comments for details on findings 1-7, 10.
|
|
||
| /// Determine format from top-level keys. | ||
| /// | ||
| /// Precedence: `input` or `prompt` object → Responses, then |
There was a problem hiding this comment.
[Medium] This adds prompt object detection to classify_format, which the old classifier at openai/responses/classifier/mod.rs did not have. The old code only checked input and messages; now any payload with a prompt object (but no input) is classified as Responses instead of UnknownJson. This changes behavior for existing openai_responses_format users.
The addition is likely intentional and correct (Responses API does support prompt objects), but it is a behavioral change bundled into what the PR describes as an extraction/refactor of the classifier. Worth calling out explicitly in the PR description so reviewers are aware.
| /// Detected body format. | ||
| pub format: AiRequestFormat, | ||
| /// Whether `conversation` is present and non-null. | ||
| pub has_conversation: bool, |
There was a problem hiding this comment.
[Small] New fields has_tools, has_prompt_id, and max_tokens are added to ClassifiedRequest compared to the original. The existing openai_responses_format filter (via the updated responses/mod.rs) now receives these new fields but does not use them in its own promotion logic. Fine for forward-compatibility, but the struct now extracts data that only anthropic_messages_format consumes.
| None => &[], | ||
| }; | ||
|
|
||
| let mut classified = classify_request_body(bytes); |
There was a problem hiding this comment.
[Medium] The path override (is_messages_path) uses exact equality with "/v1/messages". Unlike is_responses_path in the shared classifier which strips trailing slashes, a request to /v1/messages/ would not trigger the Anthropic override. Consider normalizing the trailing slash for consistency, or add a test documenting that /v1/messages/ intentionally falls through to body-only classification.
| "{}", | ||
| r#"{"model":"claude-opus-4-8","max_tokens":1024,"messages":[{"role":"user","content":"Hi"}]}"#, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
[Medium] run_filter always sets the path to /v1/messages, which activates the is_messages_path override in on_request_body. This means promotes_anthropic_messages_format passes because of the path boost, not because the body alone classifies as AnthropicMessages.
Consider adding a test where the path is e.g. /v1/some-other-path and the body has messages + max_tokens + system to verify body-only classification promotes correctly without the path boost.
| } | ||
|
|
||
| // ----------------------------------------------------------------------------- | ||
| // Promotion Tests |
There was a problem hiding this comment.
[Medium] Only InvalidJson is tested in reject mode. Missing tests for:
NonJson(empty body) in reject modeUnknownJson(valid JSON withoutmessagesorinput) in reject modecontinuemode with invalid/non-JSON bodies (should confirm filter releases without rejecting)
These are distinct code paths in handle_invalid_format and each should have coverage.
|
|
||
| /// Write durable metadata. | ||
| fn write_metadata(ctx: &mut HttpFilterContext<'_>, classified: &ClassifiedRequest) { | ||
| ctx.set_metadata("anthropic_format.format", classified.format.as_str()); |
There was a problem hiding this comment.
[Small] max_tokens is written to metadata (anthropic_format.max_tokens) but not promoted to filter results in promote_filter_results. This is asymmetric with stream which is promoted to both. If downstream branch chains might need to condition on max_tokens, this would require a follow-up change.
|
|
||
| results.set("format", classified.format.as_str())?; | ||
|
|
||
| if let Some(model) = &classified.model |
There was a problem hiding this comment.
[Nit] The filter result key is "anthropic_messages_format" while the metadata prefix is "anthropic_format.". The inconsistency could cause confusion: a user might expect anthropic_messages_format.format in metadata. Consider aligning the metadata prefix to "anthropic_messages_format." to match, or document the difference.
|
PR too large: 1517 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add |
- Normalize trailing slash on /v1/messages/ path check - Add body-only classification test without path boost - Add NonJson, UnknownJson reject-mode and continue-mode tests - Promote max_tokens to filter results for symmetry with stream - Clarify has_anthropic_signals doc re: system string and array forms - Add stream:false promotion test - Add null header config suppression test Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
There was a problem hiding this comment.
Thanks! A few observations:
- Splitting into two PRs would have been easier (one for the reworked classifier and one for the message format
- we are missing integration tests for the new filters
- we are missing YAML examples for the new filters
EDIT: i just noticed #592 but i think it's better to have CI validating new config and tests in the same PR :)
Yeah, that makes sense that splitting would have been cleaner. As you saw the integration tests and examples are in subsequent PRs. I wanted to try to keep the changes setup in moderate scope to not overload the amount of stuff I was going to ask you to review, but LMK if you want me to include them here. |
|
CI failure: The CodeQL |
There was a problem hiding this comment.
PR Review - Shared AI Classifier and Anthropic Messages Format Filter
Summary: Extracts AI request body classification into a shared classifier module and adds the anthropic_messages_format filter.
Overall: Well-structured extraction. The shared classifier is sound for multi-provider format detection. Test coverage is solid with a few gaps noted in inline comments and below.
| Severity | Count |
|---|---|
| Medium | 4 |
| Small | 3 |
| Nit | 2 |
Findings without inline placement
-
[Medium] The run_filter test utility hardcodes path to /v1/messages, so path-boost is always active. No test verifies pure body-heuristic classification without path or header boost. Consider adding one.
-
[Small] No test for ChatCompletions not being rejected in reject mode via handle_invalid_format.
| /// Extracted `max_tokens` field value, if present. | ||
| pub max_tokens: Option<u64>, | ||
| /// Extracted `model` field value, if present. | ||
| pub model: Option<String>, |
There was a problem hiding this comment.
[Nit] Doc comment says prompt.id but the code checks prompt.prompt_id (line 137). Consider updating to prompt.prompt_id for consistency.
| fn classify_format(obj: &serde_json::Map<String, serde_json::Value>) -> AiRequestFormat { | ||
| if obj.contains_key("input") || obj.get("prompt").is_some_and(serde_json::Value::is_object) { | ||
| return AiRequestFormat::Responses; | ||
| } |
There was a problem hiding this comment.
[Small] The classify_format precedence: input or prompt-object takes absolute precedence over messages+max_tokens+system. A request with both a prompt object AND Anthropic signals would classify as Responses. This is probably intentional (Responses API uses prompt objects), but worth a test case to lock in this edge case behavior explicitly.
|
|
||
| let bytes = match body.as_ref() { | ||
| Some(b) => b.as_ref(), | ||
| None => &[], |
There was a problem hiding this comment.
[Medium] The boost logic reclassifies ChatCompletions to AnthropicMessages when anthropic-version header is present OR path is /v1/messages. However, it does not boost UnknownJson. If the body has max_tokens and system but no messages key, the classifier returns UnknownJson and the filter will not reclassify. This might be desired (let the backend reject), but worth documenting the intent since the boost only applies to ChatCompletions.
| ) { | ||
| if let Some(header) = &config.headers.format { | ||
| ctx.extra_request_headers | ||
| .push((Cow::Owned(header.clone()), classified.format.as_str().to_owned())); |
There was a problem hiding this comment.
[Medium] promote_filter_results does not promote has_tools, has_prompt_id, has_conversation, or has_previous_response_id to filter results, unlike the openai_responses_format filter which promotes all of these. If downstream branch conditions need to route Anthropic requests by tool usage or conversation state, these will be unavailable. write_metadata also does not promote has_tools. This may be intentional for the initial PR (fields not yet needed for Anthropic routing), but it creates an asymmetry between the two format filters that could surprise users.
| // ----------------------------------------------------------------------------- | ||
|
|
||
| /// Default maximum request body size for `StreamBuffer` mode (1 MiB). | ||
| const DEFAULT_MAX_BODY_BYTES: usize = 1_048_576; // 1 MiB |
There was a problem hiding this comment.
[Nit] The default is 1 MiB here vs. openai_responses_format default of 64 MiB (MAX_JSON_BODY_BYTES). This difference is presumably intentional (Anthropic messages are typically smaller), but there is no doc comment explaining the rationale for the 1 MiB default vs. the ceiling of 64 MiB.
| @@ -6,6 +6,10 @@ | |||
|
|
|||
| pub(crate) mod agentic; | |||
There was a problem hiding this comment.
[Medium] This mod anthropic declaration is not gated behind cfg(feature = ai-inference), but its internals import from crate::builtins::http::ai::classifier which is feature-gated. If ai-inference is disabled, compilation should fail. Either add the feature gate here (matching the pattern of the other AI filter modules) or verify the module compiles without the feature.
| @@ -189,7 +188,9 @@ fn handle_invalid_format(format: AiRequestFormat, config: &ResponsesFormatConfig | |||
| AiRequestFormat::InvalidJson => "invalid JSON body", | |||
| AiRequestFormat::NonJson => "request body is not JSON", | |||
| AiRequestFormat::UnknownJson => "unrecognized AI API format", | |||
There was a problem hiding this comment.
[Small] Good addition of AiRequestFormat::AnthropicMessages to the pass-through arm. This ensures the responses filter does not reject requests that the shared classifier now identifies as Anthropic format.
|
Unsigned commits: 7a50a84. Please sign your commits. |
Adds the unified-gateway example config and 4 integration tests to the classifier PR per review feedback. The config routes by classifier-promoted x-praxis-ai-format headers with the openai_responses_format header promotion explicitly suppressed to prevent overwriting. Signed-off-by: Francisco Arceo <farceo@redhat.com>
7a50a84 to
b6e09a4
Compare
Summary
classifiermodule atfilter/src/builtins/http/ai/classifier/mod.rs, adding theAnthropicMessagesvariant toAiRequestFormatanthropic_messages_formatfilter that classifies Anthropic Messages API requests and promotes format facts to internal headers for downstream routingopenai/responsesto import from the shared classifier instead of its local copyBehavioral changes from extraction: The shared classifier adds
promptobject detection (requests with apromptobject but noinputnow classify asResponsesinstead ofUnknownJson). New fields (has_tools,has_prompt_id,max_tokens) are extracted intoClassifiedRequestfor Anthropic consumption; existingopenai_responses_formatusers are unaffected.Example configs and integration tests are in #592 (PR 6 of this stack).
Part 1 of the Anthropic Messages API filter stack (epic #484).
Test plan
cargo test -p praxis-proxy-filter-- all unit tests pass including classifier and messages_format testsmake lint-- clippy and fmt cleanmake test-unit-- no regressions across workspace