From 2155d6ea63bae435765f9f4785b7add82ddddeee Mon Sep 17 00:00:00 2001 From: user Date: Sun, 30 Aug 2026 17:27:08 +0800 Subject: [PATCH 1/3] fix(core-types): expose is_retryable_category The round_executor retry ladder only gates retries on a budget (local_attempt_index < max_attempts - 1). Error category is consulted only after the budget is exhausted (to classify the terminal error), so deterministic request errors (400/401/403/404/413/422/context_length_exceeded) are retried up to max_attempts regardless of category. is_retryable_category already encodes the retry-vs-terminal semantic (Network/RateLimit/Timeout/ProviderUnavailable are retryable; everything else is terminal). Mark it pub so the runtime assembly crate can reuse it as the retry admission gate without re-implementing a separate classifier. Add a unit test asserting the retryable-vs-terminal classification so the gate's decision predicate is covered: transient categories stay retryable (preserved behavior) while deterministic categories are terminal (no retry). No production behavior change: the fn is pure and its internal callers (AiProviderError::detail, ai_error_detail_from_message) are unchanged. Test: cargo test -p bitfun-core-types --jobs 4 AI: lightly tested --- src/crates/contracts/core-types/src/errors.rs | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/crates/contracts/core-types/src/errors.rs b/src/crates/contracts/core-types/src/errors.rs index 78da964a94..7fe218319e 100644 --- a/src/crates/contracts/core-types/src/errors.rs +++ b/src/crates/contracts/core-types/src/errors.rs @@ -422,7 +422,7 @@ fn is_context_overflow_message(message: &str) -> bool { || (message.contains("prompt has") && message.contains("configured context size")) } -fn is_retryable_category(category: &ErrorCategory) -> bool { +pub fn is_retryable_category(category: &ErrorCategory) -> bool { matches!( category, ErrorCategory::Network @@ -512,7 +512,7 @@ fn extract_http_status(message: &str) -> Option { mod tests { use super::{ ai_error_detail_from_message, classify_ai_error_message, classify_ai_error_parts, - AiProviderError, ErrorCategory, + is_retryable_category, AiProviderError, ErrorCategory, }; #[test] @@ -535,6 +535,28 @@ mod tests { ); } + #[test] + fn is_retryable_category_matches_transient_vs_terminal_semantic() { + // Transient categories must be retryable so the round_executor retry gate keeps + // replaying them (preserved correct behavior). + assert!(is_retryable_category(&ErrorCategory::Network)); + assert!(is_retryable_category(&ErrorCategory::RateLimit)); + assert!(is_retryable_category(&ErrorCategory::Timeout)); + assert!(is_retryable_category(&ErrorCategory::ProviderUnavailable)); + + // Deterministic request/terminal categories must NOT be retryable so the retry gate + // turns them into an immediate terminal error instead of replaying the request. + assert!(!is_retryable_category(&ErrorCategory::ContextOverflow)); + assert!(!is_retryable_category(&ErrorCategory::InvalidRequest)); + assert!(!is_retryable_category(&ErrorCategory::Auth)); + assert!(!is_retryable_category(&ErrorCategory::Permission)); + assert!(!is_retryable_category(&ErrorCategory::ProviderQuota)); + assert!(!is_retryable_category(&ErrorCategory::ProviderBilling)); + assert!(!is_retryable_category(&ErrorCategory::ModelError)); + assert!(!is_retryable_category(&ErrorCategory::ContentPolicy)); + assert!(!is_retryable_category(&ErrorCategory::Unknown)); + } + #[test] fn builds_ai_error_detail_from_provider_metadata() { let detail = ai_error_detail_from_message( From f88e9066adb23e83a640ae6226f9bf135146648e Mon Sep 17 00:00:00 2001 From: user Date: Sun, 30 Aug 2026 17:30:11 +0800 Subject: [PATCH 2/3] fix(execution): gate retry admission by error category The round_executor retry ladder replayed every provider request error up to max_attempts based only on a budget check, so deterministic request errors (4xx, context_length_exceeded) were re-issued in full even though retrying them cannot succeed. Both request_error and stream_error branches consulted the category only after the budget was exhausted, when they already had to build the terminal error. Reuse the now-public is_retryable_category to decide retry admission at the top of the C1 (request_error) and C5 (stream_error) gates: transient categories (Network/RateLimit/Timeout/ProviderUnavailable) keep retrying, while deterministic categories turn into an immediate terminal error (attempt 1). ContextOverflow is NOT retryable, so it falls through to the existing RecoverableContextOverflow conversion and the overflow recovery chain is preserved unchanged. C2/C3/C4 (partial_stream_error, invalid_tool_arguments, no_effective_output) keep their original budget + containment semantics because content-invalid results are a model-quality issue where a retry can still produce valid output. Test: cargo test -p bitfun-core --features agent-runtime --jobs 4 AI: lightly tested --- .../src/agentic/execution/round_executor.rs | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/crates/assembly/core/src/agentic/execution/round_executor.rs b/src/crates/assembly/core/src/agentic/execution/round_executor.rs index 6172f06a7b..693cf2c8ff 100644 --- a/src/crates/assembly/core/src/agentic/execution/round_executor.rs +++ b/src/crates/assembly/core/src/agentic/execution/round_executor.rs @@ -433,7 +433,17 @@ impl RoundExecutor { error!("AI request failed: {}", e); let provider_error = e.downcast_ref::().cloned(); let err_msg = e.to_string(); - if local_attempt_index < max_attempts - 1 { + // Classify upfront so the retry admission gate can reject deterministic + // request errors (4xx/context_length_exceeded) without replaying the request. + let category = provider_error + .as_ref() + .map(|error| error.category.clone()) + .unwrap_or_else(|| { + bitfun_core_types::errors::classify_ai_error_message(&err_msg) + }); + if local_attempt_index < max_attempts - 1 + && bitfun_core_types::errors::is_retryable_category(&category) + { self.record_retry_diagnostic( &context, &round_id, @@ -463,12 +473,6 @@ impl RoundExecutor { local_attempt_index += 1; continue; } - let category = provider_error - .as_ref() - .map(|error| error.category.clone()) - .unwrap_or_else(|| { - bitfun_core_types::errors::classify_ai_error_message(&err_msg) - }); let error = if category == ErrorCategory::ContextOverflow { BitFunError::RecoverableContextOverflow(provider_error.unwrap_or_else( || AiProviderError::classified(err_msg, ErrorCategory::ContextOverflow), @@ -813,7 +817,9 @@ impl RoundExecutor { Self::error_trace_response("error", err_msg.clone()), ) .await; - if local_attempt_index < max_attempts - 1 { + if local_attempt_index < max_attempts - 1 + && bitfun_core_types::errors::is_retryable_category(&stream_error_category) + { self.record_retry_diagnostic( &context, &round_id, From 6fa58cb062b759bb5ef3142fb61c160b25971c32 Mon Sep 17 00:00:00 2001 From: user Date: Sun, 30 Aug 2026 19:24:18 +0800 Subject: [PATCH 3/3] fix(cli): align exec contracts to terminal retry Retry admission is now gated on the provider error category, so a deterministic request error (403/4xx) terminates the turn at attempt 1 while a transient error (network/rate/timeout/overload) keeps retrying and ContextOverflow keeps its recovery chain. The exec contract tests still asserted the prior full-retry counts: - two http_403 tests expected 10 provider requests; 403 is Permission (terminal), so each now reaches the provider once before terminating. - the disconnect-then-403 test expected 10; the mid-stream disconnect is transient (Network), so it is retried once, then the deterministic 403 terminates the turn (2 provider requests total). - the malformed-SSE test expected a retry-to-success; a malformed SSE frame is a deterministic provider protocol error, so it now terminates at attempt 1 and the test asserts a terminal DialogTurnFailed instead of a successful retry. Test: cargo test --locked -p bitfun-cli --jobs 4 AI: lightly tested --- .../exec_cli_contracts.rs | 56 ++++++++++++------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/src/apps/cli/tests/cli_command_contracts/exec_cli_contracts.rs b/src/apps/cli/tests/cli_command_contracts/exec_cli_contracts.rs index 341e69a381..114b409c79 100644 --- a/src/apps/cli/tests/cli_command_contracts/exec_cli_contracts.rs +++ b/src/apps/cli/tests/cli_command_contracts/exec_cli_contracts.rs @@ -368,44 +368,52 @@ fn stream_json_patch_success_emits_one_success_terminal() { } #[test] -fn stream_json_malformed_sse_retries_then_completes() { +fn stream_json_malformed_sse_is_a_deterministic_terminal_error() { let server = MockOpenAiServer::malformed_sse_then_immediate(); let environment = CliTestEnvironment::new(); environment.configure_mock_model(server.base_url()); let mut command = environment.std_command(); command.args([ "exec", - "exercise malformed provider stream retry", + "exercise malformed provider stream", "--output-format", "stream-json", ]); let output = command_output_with_timeout(&mut command, std::time::Duration::from_secs(30)); - server.assert_chat_completion_requests(2); + // A malformed SSE frame is a deterministic provider protocol error (not a + // transient network/rate/timeout failure), so upfix-4 admission leaves no + // retry budget: the turn terminates at attempt 1 after a single request. + server.assert_chat_completion_requests(1); let stdout = stdout(&output); - assert!(output.status.success(), "{}\n{stdout}", stderr(&output)); + assert!(!output.status.success(), "{stdout}"); + assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); let events = jsonl_events(&stdout); - assert!( - events.iter().any(|value| { - value["event"]["type"] == "TextChunk" - && value["event"]["text"] - .as_str() - .is_some_and(|text| text.contains(STREAM_COMPLETED_MARKER)) - }), - "retried model stream did not complete: {stdout}" - ); assert_eq!( events .iter() .filter(|value| is_terminal_event(value)) .count(), 1, - "retried stream must emit exactly one terminal envelope: {stdout}" + "malformed stream must emit exactly one terminal envelope: {stdout}" ); assert_eq!( - events.last().expect("retried stream terminal event")["event"]["type"], - "DialogTurnCompleted", - "retried stream terminal must be last: {stdout}" + events.last().expect("malformed stream terminal event")["event"]["type"], + "DialogTurnFailed", + "malformed stream terminal must be last: {stdout}" + ); + let terminal_error = events.last().expect("malformed stream terminal event")["event"]["error"] + .as_str() + .expect("malformed stream error text"); + assert!( + terminal_error.contains("SSE parsing error"), + "malformed stream reason was lost: {stdout}" + ); + assert!( + events.iter().all(|value| { + !(value["event"]["type"] == "DialogTurnCompleted" && value["event"]["success"] == true) + }), + "malformed stream emitted a successful completion: {stdout}" ); } @@ -422,7 +430,10 @@ fn stream_json_provider_http_403_emits_one_error_terminal() { "stream-json", ]); let output = command_output_with_timeout(&mut command, std::time::Duration::from_secs(30)); - server.assert_chat_completion_requests(10); + // An authorization rejection (403) is a deterministic error: upfix-4 terminates + // the turn at attempt 1 after a single provider request instead of exhausting + // the retry ladder. + server.assert_chat_completion_requests(1); let stdout = stdout(&output); assert!(!output.status.success(), "{stdout}"); @@ -485,7 +496,9 @@ fn stream_json_provider_and_patch_failures_publish_one_final_classification() { &output_target, ]); let output = command_output_with_timeout(&mut command, std::time::Duration::from_secs(30)); - server.assert_chat_completion_requests(10); + // The 403 is deterministic, so only a single attempt reaches the provider: the + // patch delivery (not the provider) becomes the final classifier. + server.assert_chat_completion_requests(1); let stdout = stdout(&output); let stderr = stderr(&output); @@ -535,7 +548,10 @@ fn stream_json_disconnect_then_exhausted_retry_failure_emits_one_error_terminal( "stream-json", ]); let output = command_output_with_timeout(&mut command, std::time::Duration::from_secs(30)); - server.assert_chat_completion_requests(10); + // The first attempt's mid-stream disconnect is transient (Network), so upfix-4 + // still retries once; the retried request is a deterministic 403, which then + // terminates the turn at a terminal error (two provider requests in total). + server.assert_chat_completion_requests(2); let stdout = stdout(&output); assert!(!output.status.success(), "{stdout}");