diff --git a/app/src/providers/ChatRuntimeProvider.tsx b/app/src/providers/ChatRuntimeProvider.tsx index 16ea244e3..fbf69c01e 100644 --- a/app/src/providers/ChatRuntimeProvider.tsx +++ b/app/src/providers/ChatRuntimeProvider.tsx @@ -942,14 +942,14 @@ const ChatRuntimeProvider = ({ children }: { children: React.ReactNode }) => { const currentState = store.getState(); const threadMessages = currentState.thread.messagesByThreadId[event.thread_id] ?? []; const lastMsg = threadMessages[threadMessages.length - 1]; - // For the generic 'inference' type the server may send a raw internal error string; - // use the safe user-facing constant instead. For all other classified types - // (rate_limited, timeout, auth_error, etc.) the message comes from - // classify_inference_error() in web.rs and is already user-friendly. - const errorContent = - event.error_type === 'inference' - ? USER_FACING_AGENT_ERROR_MESSAGE - : event.message || USER_FACING_AGENT_ERROR_MESSAGE; + // Every error_type — including the generic 'inference' fallback — carries a + // user-facing `message` produced by classify_inference_error() in web_errors.rs. + // For 'inference' that message is the friendly summary PLUS the real, sanitized + // upstream provider error appended as a `> quote` block (secret-scrubbed and + // length-capped server-side via with_provider_detail()/sanitize_api_error()), so + // surfacing it tells the user *why* the turn failed instead of a blanket apology. + // The hardcoded constant is only a last-resort fallback for an empty/missing message. + const errorContent = event.message || USER_FACING_AGENT_ERROR_MESSAGE; if (!(lastMsg?.sender === 'agent' && lastMsg?.content === errorContent)) { void dispatch( addInferenceResponse({ content: errorContent, threadId: event.thread_id }) diff --git a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx index 165cf9b38..664927ed2 100644 --- a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx +++ b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx @@ -687,15 +687,20 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria expect(store.getState().chatRuntime.inferenceStatusByThread['t-err']).toBeUndefined(); }); - it('adds a sanitized user-facing error bubble with Discord report action on chat_error', async () => { + it('forwards the server-provided inference error message verbatim', async () => { const listeners = renderProvider(); + // Transport-level failures yield no provider `error.message` body, so + // `with_provider_detail()` in web_errors.rs returns just the friendly + // generic message with no raw URL appended — the FE forwards it as-is + // (backend owns sanitization; see web_errors_tests.rs). + const serverMessage = + 'Something went wrong. Please try again.\nThis error has been reported. You can also report it on Discord.\nReport on Discord'; act(() => { listeners.onError?.({ thread_id: 't-err-sanitized', request_id: 'r1', - message: - 'agent job failed: error sending request for url (https://staging-api.alphahuman.xyz/openai/v1/chat/completions)', + message: serverMessage, error_type: 'inference', round: 0, }); @@ -704,36 +709,20 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria await waitFor(() => expect(threadApi.appendMessage).toHaveBeenCalledWith( 't-err-sanitized', - expect.objectContaining({ - sender: 'agent', - content: expect.stringContaining('Something went wrong. Please try again.'), - }) + expect.objectContaining({ sender: 'agent', content: serverMessage }) ) ); - expect(threadApi.appendMessage).toHaveBeenCalledWith( - 't-err-sanitized', - expect.objectContaining({ - content: expect.stringContaining( - 'Report on Discord' - ), - }) - ); - expect(threadApi.appendMessage).not.toHaveBeenCalledWith( - 't-err-sanitized', - expect.objectContaining({ - content: expect.stringContaining('https://staging-api.alphahuman.xyz'), - }) - ); }); - it('does not append duplicate fallback error bubble when the previous message already matches', async () => { + it('does not append a duplicate error bubble when the previous message already matches', async () => { const listeners = renderProvider(); + const repeated = 'Your AI provider is temporarily unavailable. Please try again later.'; act(() => { listeners.onError?.({ thread_id: 't-err-dedupe', request_id: 'r1', - message: 'transport fail one', + message: repeated, error_type: 'inference', round: 0, }); @@ -742,9 +731,7 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria await waitFor(() => expect(threadApi.appendMessage).toHaveBeenCalledWith( 't-err-dedupe', - expect.objectContaining({ - content: expect.stringContaining('Something went wrong. Please try again.'), - }) + expect.objectContaining({ content: repeated }) ) ); @@ -752,7 +739,7 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria listeners.onError?.({ thread_id: 't-err-dedupe', request_id: 'r2', - message: 'transport fail two', + message: repeated, error_type: 'inference', round: 0, }); @@ -765,7 +752,7 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria call => call[0] === 't-err-dedupe' && typeof call[1]?.content === 'string' && - call[1].content.includes('Something went wrong. Please try again.') + call[1].content.includes(repeated) ); expect(matchingCalls).toHaveLength(1); }); @@ -987,12 +974,13 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria // Error classifier full set — Batch-5 coverage (#1506, pr#1566). // - // For the generic 'inference' error type the raw server message is - // replaced with USER_FACING_AGENT_ERROR_MESSAGE. For all classified - // types (rate_limited, auth_error, budget_exhausted, context_overflow, - // timeout, network, tool_error, provider_error, model_unavailable) the - // server already provides a user-friendly message, which is forwarded - // directly. 'cancelled' produces no bubble at all. + // Every error_type — including the generic 'inference' fallback — carries a + // user-friendly `message` from classify_inference_error() in web_errors.rs, + // which is forwarded directly so the user sees the real reason (for + // 'inference' that message is a friendly summary plus the sanitized upstream + // provider error as a `> quote` block). The USER_FACING_FALLBACK constant is + // only used when the server sends an empty/missing message. 'cancelled' + // produces no bubble at all. describe('inference error classifier — full type set', () => { const USER_FACING_FALLBACK = 'Something went wrong. Please try again.\nThis error has been reported. You can also report it on Discord.\nReport on Discord'; @@ -1029,15 +1017,42 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria ); }); - it('replaces raw internal message with user-facing constant for inference type', async () => { + it('surfaces the server inference message (friendly summary + sanitized provider detail)', async () => { const listeners = renderProvider(); - const threadId = 't-raw-inference'; + const threadId = 't-inference-detail'; + // Shape produced by web_errors.rs `with_provider_detail(generic, err)`: + // the friendly summary, then the real upstream reason as a `> quote` + // block (already secret-scrubbed and length-capped server-side). + const serverMessage = + 'Something went wrong. Please try again.\n\n> Project `proj_x` does not have access to model `gpt-5.5`.'; act(() => { listeners.onError?.({ thread_id: threadId, request_id: 'r1', - message: 'internal panic: channel closed unexpectedly at line 42', + message: serverMessage, + error_type: 'inference', + round: 0, + }); + }); + + await waitFor(() => + expect(threadApi.appendMessage).toHaveBeenCalledWith( + threadId, + expect.objectContaining({ content: serverMessage, sender: 'agent' }) + ) + ); + }); + + it('falls back to the constant when an inference error has no message', async () => { + const listeners = renderProvider(); + const threadId = 't-inference-empty'; + + act(() => { + listeners.onError?.({ + thread_id: threadId, + request_id: 'r1', + message: '', error_type: 'inference', round: 0, }); @@ -1049,11 +1064,6 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria expect.objectContaining({ content: USER_FACING_FALLBACK, sender: 'agent' }) ) ); - // The raw server string must NOT leak through. - expect(threadApi.appendMessage).not.toHaveBeenCalledWith( - threadId, - expect.objectContaining({ content: expect.stringContaining('channel closed unexpectedly') }) - ); }); it('produces no error bubble for cancelled turns', async () => { diff --git a/tests/inference_local_admin_raw_coverage_e2e.rs b/tests/inference_local_admin_raw_coverage_e2e.rs index 4828dedf2..2a9fb4032 100644 --- a/tests/inference_local_admin_raw_coverage_e2e.rs +++ b/tests/inference_local_admin_raw_coverage_e2e.rs @@ -5,7 +5,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, MutexGuard, OnceLock}; use axum::body::Body; use axum::extract::State; @@ -77,6 +77,22 @@ impl Drop for EnvVarGuard { } } +/// Process-wide lock serializing tests that mutate global environment +/// variables through [`EnvVarGuard`]. `cargo llvm-cov` runs integration tests +/// multi-threaded (it does not pass `--test-threads=1`), so without this guard +/// concurrent tests clobber each other's env — e.g. one test points +/// `OPENHUMAN_OLLAMA_BASE_URL` at an unreachable port and asserts Ollama is +/// unavailable while another points it at a mock and asserts it is available. +/// Each env-mutating test holds this guard for its whole body; declaring it +/// before any `EnvVarGuard` makes it drop last, after the env is restored. +fn env_lock() -> MutexGuard<'static, ()> { + static ENV_LOCK: OnceLock> = OnceLock::new(); + ENV_LOCK + .get_or_init(|| Mutex::new(())) + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} + #[tokio::test] async fn compatible_provider_covers_retry_headers_responses_and_parse_errors() { let (base, state) = serve_mock().await; @@ -251,6 +267,7 @@ async fn compatible_provider_covers_retry_headers_responses_and_parse_errors() { #[tokio::test] async fn local_admin_covers_assets_diagnostics_downloads_and_ops_errors() { + let _env_guard = env_lock(); let (base, state) = serve_mock().await; let tmp = tempdir().expect("tempdir"); let mut config = temp_config(&tmp); @@ -392,6 +409,7 @@ async fn local_admin_covers_assets_diagnostics_downloads_and_ops_errors() { #[tokio::test] async fn provider_model_listing_covers_local_synthesis_and_openrouter_failures() { + let _env_guard = env_lock(); let (base, _state) = serve_mock().await; let tmp = tempdir().expect("tempdir"); let mut config = temp_config(&tmp); @@ -465,6 +483,7 @@ async fn provider_model_listing_covers_local_synthesis_and_openrouter_failures() #[tokio::test] async fn local_admin_reports_unhealthy_runtime_and_lm_studio_issue_shapes() { + let _env_guard = env_lock(); let tmp = tempdir().expect("tempdir"); let mut config = temp_config(&tmp); config.local_ai.runtime_enabled = true;