refactor(callouts): normalize outbound failure-policy configuration - #845
refactor(callouts): normalize outbound failure-policy configuration#845r-papso wants to merge 10 commits into
Conversation
…odule Callout filters carried duplicate FailureMode enums and validators in the web-search and OpenAI Responses paths. Move them into a single apis/src/callout_policy.rs, along with OnMissing, CalloutSettings, and the timeout/status validators from the deleted config_validation module. Keep FailureMode (no answer from the callout) and OnMissing (a successful answer that the resource is absent) separate, and document `on_failure` as the canonical key so it does not collide with the pipeline-level `failure_mode`. Callers repointed, no behavior changes. Refs: praxis-proxy#697 Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Adopt the canonical `on_failure` key from the shared callout policy module in the OpenAI and Anthropic web-search filters, updating config parsing, tests, generated filter docs, examples, and the xtask doc check. Breaking change: `provider_failure_mode` is no longer accepted. Configs must use `on_failure`. Refs: praxis-proxy#697 Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Adopt the canonical `on_failure` key in the openai_responses_compact and openai_file_search_callout filters, updating config parsing, tests, generated filter docs, examples, and the vLLM integration config. Breaking change: `callout_failure_mode` is no longer accepted. Configs must use `on_failure`. Refs: praxis-proxy#697 Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Drop the filter-local FailureModeConfig enum and use praxis_ai_apis::callout_policy::FailureMode instead. The filter already exposed the canonical `on_failure` key, so this is an internal type consolidation with no behavior or configuration change. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Apply rustfmt to the import blocks touched by the callout policy move, realign a comment column in the file-search-callout example, and make xtask filter-docs to scan apis/src/callout_policy.rs as a shared config source so FailureMode and OnMissing resolve when generating filter docs. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Add an outbound-callout section to the 0.2 migration guide: a per-filter table mapping provider_failure_mode and callout_failure_mode to on_failure, and a note that the old keys are rejected at startup rather than aliased. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Drop web_search's local validate_timeout_ms and validate_status_on_error copies in favor of the equivalents in callout_policy, passing the existing DEFAULT_TIMEOUT_MS and DEFAULT_STATUS_ON_ERROR explicitly. Error messages and accepted ranges are identical, behavior is unchanged. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
P2 — apis/src/callout_policy.rs:6-19: the new “single source” documents semantics that its consumers do not follow. OnMissing::Continue also suppresses file-ID transport, timeout, non-2xx, parsing, and size failures. Conversely, http_callout ignores FailureMode for non-2xx and malformed successful JSON. Either describe these enums as vocabulary whose failure classification is filter-specific, or keep incompatible policies local.
P3 — apis/src/callout_policy.rs:114-125: http_callout retains an identical status validator and even contains a stale comment proposing its future promotion into praxis-ai-apis, where this PR already added it. That leaves #697’s validator-consolidation criterion incomplete.
Describe the enums as fixing only the accepted values and the default, with each filter's on_failure / on_missing field docs authoritative for classification. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
…callout Drop http_callout's local validate_status_on_error and reuse the equivalent validation function from praxis-ai-apis. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
| //! | ||
| //! | Question | Type | YAML key | Values | | ||
| //! | --- | --- | --- | --- | | ||
| //! | The callout did not produce a usable answer. Serve the request anyway? | [`FailureMode`] | `on_failure` | `closed`, `open` | |
There was a problem hiding this comment.
(ai): "The module table and FailureMode docstring state on_failure governs DNS, connect, timeout, TLS, non-2xx, and unparseable body for every callout filter. http_callout diverges: handle_response at filters/src/callout/mod.rs:327-329 always rejects completed non-2xx responses with the upstream status, ignoring on_failure: open (test non_2xx_callout_response_forwards_status_to_downstream documents this). handle_success at lines 236-244 logs and continues on malformed JSON without consulting on_failure. http_callout.md correctly scopes on_failure to transport/I/O only, but the shared canonical module contradicts that. Either reframe FailureMode as shared vocabulary whose failure-class mapping is filter-specific (with a per-filter table), or align http_callout behavior with the documented semantics."
There was a problem hiding this comment.
Makes sense - I went through all the callout filters and they indeed express slightly different behavior in case of failure. I think that forcing all existing (and potential future) callout filters adopting the same on_failure / on_missing behavior may be too restrictive.
I therefore removed the 'classification' table from the callout_policy.rs and made clear that the module is rather a vocabulary, and specific callout behavior is each filter's own concern.
What do you think?
|
|
||
| /// What happens when a callout succeeds and answers that the | ||
| /// requested resource does not exist. Configured as `on_missing`. | ||
| /// |
There was a problem hiding this comment.
(ai): "callout_policy.rs describes OnMissing as applying when a callout succeeds and reports a missing resource. openai_file_resolve resolve_reference at apis/src/openai/responses/file_resolve/resolve.rs:666-668 treats on_missing: continue as pass-through for any file_id resolution error except TooManyReferences, including transport, timeout, non-2xx, parsing, and size failures—not only a successful 404-style absence. Filter docs (openai_file_resolve.md line 24) say cannot be fetched, which is closer to behavior than the shared enum doc. The shared vocabulary should either document this widening explicitly or narrow the enum description to filter-specific semantics with a cross-reference to file_resolve."
There was a problem hiding this comment.
That's right. I updated the docs to state that OnMissing controls "What happens when a requested resource cannot be fetched". That indeed more closely maps to file_resolve.rs which is for now only filter using it. The doc description can be widened in the future, if needed.
| format!("http_callout: invalid header name '{}': {e}", h.name).into() | ||
| })?; | ||
| let value: http::HeaderValue = expanded.parse().map_err(|e| -> FilterError { | ||
| format!("http_callout: invalid header value for '{}': {e}", h.name).into() |
There was a problem hiding this comment.
(ai): "Issue #697 calls for consolidating duplicate validators where semantics match. web_search and openai_responses_compact now call callout_policy::validate_timeout_ms and validate_status_on_error (commit 45dc2c7), but filters/src/callout/mod.rs still defines a local validate_status_on_error at lines 431-439. The helper comment at lines 423-426 still proposes future promotion into praxis-ai-apis even though callout_policy.rs already exports it. Behavior is unchanged, but the consolidation criterion remains open for http_callout."
There was a problem hiding this comment.
This should be already adressed in f9d6d02.
| //! two different questions (the first listed value of each is | ||
| //! its default): | ||
| //! | ||
| //! | Question | Type | YAML key | Values | |
There was a problem hiding this comment.
(ai): "Acceptance criteria require inventorying every outbound AI callout filter and classifying transport, provider, missing-resource, partial-result, and parse failures. callout_policy.rs provides a two-row vocabulary table but does not map each filter (http_callout, web_search, compact, file_search_callout, file_resolve) to which failure classes its on_failure/on_missing settings actually govern. Operators cannot tell from the shared module alone that http_callout treats completed non-2xx responses outside on_failure, or that openai_file_resolve applies on_missing: continue to transport and parse errors for file_id, not only absent resources."
There was a problem hiding this comment.
I reviewed individual filters' fail-open / fail-close behaviors, and they indeed express slightly different behavior. Since the documentation is created out of individual filters' configs, and since I want to keep the PR free of [callout filters] behavior changes, I dropped the table from the callout_policy.rs and made it clear that the module is a vocabulary - each filter owns it's fail-open / fail-close behavior and docs.
Drop the question/answer table and the failure-class wording from callout_policy. The module only fixes the accepted values and the default. Which conditions route through on_failure or on_missing varies per filter, so each filter's field docs and behavior remain the authoritative reference. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
|
@leseb thanks! Addressed your remarks. I went through individual filters' fail-open / fail-close behaviors and they indeed express a slightly different behavior (e.g. http_callout always failing on non 2xx response). Since forcing all filters adopting the same behavior might be too restrictive IMO, and since I want to keep PR free from behavior changes, I made it clear that I also wired status on error validation in http_callout to praxis-ai-apis, reusing the one from What do you think? |
Summary
Outbound callout filters expressed the same fail-open/fail-closed choice three ways (
provider_failure_mode,callout_failure_mode,on_failure) and carried duplicateFailureModeenums and validators.The PR consolidates duplicate enums / validators into
apis/src/callout_policy.rsas the single source for outbound callout failure policy, and repoints all callout filters at it. The external key is nowon_failureeverywhere. The name avoids collision with the structural pipeline-levelfailure_modeand pairs withon_missing.FailureModeandOnMissingstay separate following a criterion to keep semantically distinct policies separate.No behavior changes beyond the key rename.
Closes #697
Validation
cargo test -p praxis-ai-apiscargo test -p praxis-ai-filtersmake lintChecklist
Signed-off-bytrailer.Breaking changes
provider_failure_mode(anthropic_web_search, openai_web_search) andcallout_failure_mode(openai_responses_compact, openai_file_search_callout) are renamed toon_failure. Values (open / closed) and the closed default are unchanged. http_callout already usedon_failureand is unaffected.The old keys are not accepted as aliases. Migration is documented in
docs/migrating-to-0.2.md.