Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 47 additions & 1 deletion src/openhuman/agent_orchestration/tools/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,30 @@ pub(crate) async fn dispatch_subagent(
agent_id: definition.id.clone(),
error: message.clone(),
});
Ok(ToolResult::error(format!("{tool_name} failed: {message}")))
// Make the failure unmistakable to the orchestrator: the delegated
// task did NOT run, so it must not be reported as success or have
// its output fabricated. Without this guardrail a weak orchestrator
// can narrate a plausible success from the bare error text — the
// "hallucinated success" half of #3193 (e.g. claiming `run_code`
// wrote a file when the coding model 404'd and nothing executed).
Ok(ToolResult::error(format_subagent_failure(
tool_name, &message,
)))
}
}
}

/// Format a subagent-delegation failure so the orchestrator cannot mistake it
/// for success. Kept as a standalone, side-effect-free fn so the exact wording
/// is unit-testable without standing up a registry + failing model (#3193).
fn format_subagent_failure(tool_name: &str, message: &str) -> String {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In-band prompt-engineering preamble now applied to every subagent failure.

format_subagent_failure is invoked on the generic run_subagent error path — so timeouts, budget exhaustion, transport errors, and any other transient failure now also carry the ~200-char "do NOT treat this as success or fabricate an output" preamble in transcripts, logs, and observability. That's intentional given #3193's "hallucinated success" symptom, but it's a behavior change broader than the issue title suggests ("completion-only 404"). Worth either:

  • calling out in the PR body's Behavior Changes section that the envelope is applied to all subagent failures, or
  • gating the envelope to a narrower class of "hard, non-retryable" failures so retry/budget cases keep their shorter error string.

Not blocking — flagging so it's a deliberate decision.

format!(
"{tool_name} failed and did not complete — no work was performed and no \
results were produced. Do NOT treat this as success or fabricate an \
output; report the failure to the user. Error: {message}"
)
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -162,4 +181,31 @@ mod tests {
"unexpected graceful-failure message: {out}"
);
}

#[test]
fn subagent_failure_envelope_forbids_fabricated_success() {
// #3193: a hard delegation failure (e.g. run_code's coding model
// 404ing) must be surfaced so the orchestrator cannot narrate a
// plausible success. The envelope states the task did not run, tells
// the model not to fabricate output, and preserves the root error.
let msg = format_subagent_failure(
"run_code",
"openhuman API error (404): model 'davinci-002' does not support \
the chat-completions API",
);
assert!(msg.contains("run_code failed"), "names the tool: {msg}");
assert!(
msg.contains("did not complete"),
"states no completion: {msg}"
);
assert!(
msg.to_lowercase().contains("do not treat this as success")
&& msg.contains("fabricate"),
"warns against fabricated success: {msg}"
);
assert!(
msg.contains("davinci-002") && msg.contains("404"),
"preserves the root error: {msg}"
);
}
}
111 changes: 99 additions & 12 deletions src/openhuman/inference/provider/compatible.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,43 @@ impl OpenAiCompatibleProvider {
}
}

/// Build an actionable error for a completion-only model that was routed
/// to `/v1/chat/completions`. OpenHuman only speaks the chat-completions
/// API (with an optional `/v1/responses` fallback) — a completion-only /
/// base model 404s here and the responses fallback cannot rescue it, so we
/// surface the model name and concrete remediation instead of an opaque
/// "responses fallback failed" chain. See issue #3193.
fn completion_only_model_message(&self, model: &str, sanitized: &str) -> String {
format!(
"{name} API error (404): model '{model}' does not support the \
chat-completions API that OpenHuman uses — it appears to be a \
completion-only / base model. Assign a chat-capable model to this \
provider (e.g. in Settings → AI), or pick a different model. \
Provider detail: {sanitized}",
name = self.name,
)
}

/// Guard shared by every chat-completions 404 handler: if the body shows a
/// completion-only model, return the actionable error so the caller can
/// fail fast instead of attempting the futile `/v1/responses` fallback.
/// `None` means "not this case — proceed with normal fallback/enrich".
/// See issue #3193.
fn completion_only_404_guard(
&self,
status: reqwest::StatusCode,
sanitized: &str,
model: &str,
) -> Option<anyhow::Error> {
if Self::is_completion_only_model_404(status, sanitized) {
Some(anyhow::anyhow!(
self.completion_only_model_message(model, sanitized)
))
} else {
None
}
}

/// Create a provider with a custom User-Agent header.
///
/// Some providers (for example Kimi Code) require a specific User-Agent
Expand Down Expand Up @@ -798,6 +835,24 @@ impl OpenAiCompatibleProvider {
Self::is_native_tool_schema_unsupported(reqwest::StatusCode::BAD_REQUEST, error)
}

/// Detect a 404 whose body says the model is completion-only and cannot be
/// served from `/v1/chat/completions` (OpenAI: "This is not a chat model
/// and thus not supported in the v1/chat/completions endpoint. Did you
/// mean to use v1/completions?"). When this fires, attempting the
/// `/v1/responses` fallback is futile, so callers should fail fast with an
/// actionable message via [`completion_only_model_message`]. The match is
/// deliberately tight so ordinary "model does not exist" 404s are NOT
/// caught (those should keep their existing fallback / enrich behaviour).
/// See issue #3193.
fn is_completion_only_model_404(status: reqwest::StatusCode, error: &str) -> bool {
if status != reqwest::StatusCode::NOT_FOUND {
return false;
}
let lower = error.to_lowercase();
lower.contains("not a chat model")
|| (lower.contains("v1/chat/completions") && lower.contains("v1/completions"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Second disjunct is correct today but fragile to OpenAI rewording.

The string "v1/chat/completions" does not contain "v1/completions" as a continuous substring (the /chat/ infix breaks it), so the two contains calls really are independent matches — but that's only obvious if you stop and parse it. The whole clause currently fires only because OpenAI's body happens to mention both endpoint paths ("...the v1/chat/completions endpoint. Did you mean to use v1/completions?"). If OpenAI ever drops the second URL from the wording, this branch silently stops matching and we fall back to the opaque path.

Suggest a one-line comment on line 853 making the "two independent substrings" intent explicit, e.g.:

// Defensive fallback: OpenAI's current phrasing references BOTH endpoint paths
// as two separate substrings (`v1/chat/completions` does not contain `v1/completions`).

}

/// Streaming variant of the native-tools chat path.
///
/// Sends the request with `stream: true`, consumes the upstream SSE
Expand Down Expand Up @@ -1343,6 +1398,12 @@ impl Provider for OpenAiCompatibleProvider {
let error = response.text().await?;
let sanitized = super::sanitize_api_error(&error);

// A completion-only model 404s here and the /v1/responses fallback
// cannot rescue it — fail fast with actionable guidance (#3193).
if let Some(err) = self.completion_only_404_guard(status, &sanitized, model) {
return Err(err);
}

if status == reqwest::StatusCode::NOT_FOUND && self.supports_responses_fallback {
return self
.chat_via_responses(credential, &fallback_messages, model)
Expand Down Expand Up @@ -1510,18 +1571,38 @@ impl Provider for OpenAiCompatibleProvider {
if !response.status().is_success() {
let status = response.status();

// Mirror chat_with_system: 404 may mean this provider uses the Responses API
if status == reqwest::StatusCode::NOT_FOUND && self.supports_responses_fallback {
return self
.chat_via_responses(credential, &effective_messages, model)
.await
.map_err(|responses_err| {
let fb = super::format_anyhow_chain(&responses_err);
anyhow::anyhow!(
"{} API error (chat completions unavailable; responses fallback failed: {fb})",
self.name
)
});
// A 404 may mean this provider uses the Responses API, OR that the
// model is completion-only. Read the body once so we can tell the
// two apart (#3193) — only the 404 branch needs it; the response is
// not used again here, so `api_error` below still owns the rest.
if status == reqwest::StatusCode::NOT_FOUND {
let error = response.text().await?;
let sanitized = super::sanitize_api_error(&error);

// Completion-only model: the responses fallback can't help —
// fail fast with actionable guidance.
if let Some(err) = self.completion_only_404_guard(status, &sanitized, model) {
return Err(err);
}

if self.supports_responses_fallback {
return self
.chat_via_responses(credential, &effective_messages, model)
.await
.map_err(|responses_err| {
let fb = super::format_anyhow_chain(&responses_err);
anyhow::anyhow!(
"{} API error ({status}): {sanitized} (chat completions unavailable; responses fallback failed: {fb})",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Undocumented behavior change on the non-completion-only fallback path.

Before this PR, chat_with_history's responses-fallback failure read:

{name} API error (chat completions unavailable; responses fallback failed: {fb})

After this PR it reads:

{name} API error ({status}): {sanitized} (chat completions unavailable; responses fallback failed: {fb})

That's a strict improvement (now includes the original 404 body and status), but it's a behavior change to a path unrelated to completion-only detection — any ordinary 404 that triggers the responses fallback will now have a different error string. The PR body's Behavior Changes section only lists the completion-only and subagent-envelope changes, not this. Suggest adding a one-liner there so downstream log parsers / Sentry classifiers aren't surprised.

(Cross-reference: the same widening was applied at the native-tool-calling chat 404 path — consistent with this one, which is good.)

self.name
)
});
}

let enriched = self.enrich_404_message(
format!("{} API error ({status}): {sanitized}", self.name),
status,
);
return Err(anyhow::anyhow!("{enriched}"));
}

let err = super::api_error(&self.name, response).await;
Expand Down Expand Up @@ -1859,6 +1940,12 @@ impl Provider for OpenAiCompatibleProvider {
});
}

// A completion-only model 404s here and the /v1/responses fallback
// cannot rescue it — fail fast with actionable guidance (#3193).
if let Some(err) = self.completion_only_404_guard(status, &sanitized, model) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adjacent code paths share the same 404 surface but skip the new guard.

The PR body claims "completion-only detection is shared across all three chat entrypoints", but two more entrypoints on this same provider can hit the same OpenAI completion-only 404:

  1. chat_with_tools (line ~1697) — if !response.status().is_success() { return Err(super::api_error(&self.name, response).await); }. No guard, no responses fallback. If a run_code-style flow ever passes through the native tool-calling non-streaming path (and tools is non-empty), the user gets the opaque "not a chat model" 404 again. Transport-error path does fall back to chat_with_history (which has the guard), but the 404-status path does not.

  2. Streaming paths — stream_chat_with_system (~line 2138) and stream_chat_with_history (~line 2298). Both already sanitize the error body, so adding a completion_only_404_guard branch is a one-liner that matches the pattern used here.

Proposal: either replicate the 3-line guard at all four additional sites (preferred — they're 1-liners and the helper is already shared), or document in the PR body why streaming + chat_with_tools are intentionally excluded. Otherwise #3193 can re-emerge through any of these paths.

return Err(err);
}

if status == reqwest::StatusCode::NOT_FOUND && self.supports_responses_fallback {
return self
.chat_via_responses(credential, &effective_messages, model)
Expand Down
120 changes: 120 additions & 0 deletions src/openhuman/inference/provider/compatible_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2109,3 +2109,123 @@ fn convert_tool_specs_dedups_many_duplicates() {
.collect();
assert_eq!(names, vec!["x", "y", "z"]);
}

// ── #3193: completion-only model 404 detection + actionable message ──────────

#[test]
fn completion_only_model_404_detected_from_openai_signature() {
// The exact body OpenAI returns when a completion-only/base model is sent
// to /v1/chat/completions.
let body = "This is not a chat model and thus not supported in the \
v1/chat/completions endpoint. Did you mean to use v1/completions?";
assert!(OpenAiCompatibleProvider::is_completion_only_model_404(
reqwest::StatusCode::NOT_FOUND,
body
));
}

#[test]
fn completion_only_model_404_ignores_ordinary_not_found() {
// A "model does not exist" 404 must NOT be misclassified — it should keep
// its existing fallback / enrich behaviour, not get the completion-only
// message.
let body = "The model `gpt-9o` does not exist or you do not have access to it.";
assert!(!OpenAiCompatibleProvider::is_completion_only_model_404(
reqwest::StatusCode::NOT_FOUND,
body
));
}

#[test]
fn completion_only_model_404_requires_404_status() {
// Same phrasing under a non-404 status is not the completion-only case.
let body = "not a chat model";
assert!(!OpenAiCompatibleProvider::is_completion_only_model_404(
reqwest::StatusCode::BAD_REQUEST,
body
));
}

#[test]
fn completion_only_message_names_model_and_remediation() {
let p = make_provider("openhuman", "https://api.example.com/v1", Some("k"));
let msg = p.completion_only_model_message(
"davinci-002",
"This is not a chat model ... Did you mean to use v1/completions?",
);
assert!(
msg.contains("davinci-002"),
"names the offending model: {msg}"
);
assert!(
msg.contains("completion-only") && msg.contains("chat-completions"),
"explains the capability mismatch: {msg}"
);
assert!(
msg.contains("chat-capable model"),
"states the remediation: {msg}"
);
}

#[test]
fn completion_only_404_guard_fires_only_on_signature() {
let p = make_provider("openhuman", "https://api.example.com/v1", Some("k"));
// Matches → Some(actionable error).
let hit = p.completion_only_404_guard(
reqwest::StatusCode::NOT_FOUND,
"This is not a chat model. Did you mean to use v1/completions?",
"davinci-002",
);
let err = hit.expect("guard should fire on the completion-only signature");
assert!(err.to_string().contains("davinci-002"));
// Ordinary not-found → None (normal fallback/enrich path is preserved).
assert!(p
.completion_only_404_guard(
reqwest::StatusCode::NOT_FOUND,
"The model `gpt-9o` does not exist.",
"gpt-9o"
)
.is_none());
}

#[tokio::test]
async fn completion_only_404_fails_fast_without_responses_fallback() {
// End-to-end over the wire: a completion-only 404 must short-circuit with
// the actionable message and NOT attempt /v1/responses (not mounted here —
// if the guard regressed, the error would instead read "responses fallback
// failed"). Provider has the fallback ENABLED (default `new`), proving the
// guard pre-empts it. #3193.
let server = MockServer::start().await;
Mock::given(method("POST"))
.and(path("/v1/chat/completions"))
.respond_with(ResponseTemplate::new(404).set_body_json(serde_json::json!({
"error": {
"message": "This is not a chat model and thus not supported in the \
v1/chat/completions endpoint. Did you mean to use v1/completions?",
"type": "invalid_request_error"
}
})))
.mount(&server)
.await;

let provider = OpenAiCompatibleProvider::new(
"openhuman",
&format!("{}/v1", server.uri()),
Some("key"),
AuthStyle::Bearer,
);

let err = provider
.chat_with_history(&[ChatMessage::user("write a file")], "davinci-002", 0.0)
.await
.expect_err("completion-only model must error");
let msg = err.to_string();
assert!(
msg.contains("davinci-002") && msg.contains("chat-capable model"),
"expected actionable completion-only message, got: {msg}"
);
assert!(
!msg.contains("responses fallback failed"),
"guard must pre-empt the responses fallback, got: {msg}"
);
}
Loading