diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 490cd7705..e547cace8 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -2358,6 +2358,7 @@ pub fn run() { if openhuman_core::core::observability::is_transient_backend_api_failure(&event) || openhuman_core::core::observability::is_transient_integrations_failure(&event) || openhuman_core::core::observability::is_updater_transient_event(&event) + || openhuman_core::core::observability::is_skill_install_user_fetch_failure(&event) { return None; } diff --git a/docs/agent-workflows/cursor-cloud-agents.md b/docs/agent-workflows/cursor-cloud-agents.md index 5fc4bbfcf..04bd7eee1 100644 --- a/docs/agent-workflows/cursor-cloud-agents.md +++ b/docs/agent-workflows/cursor-cloud-agents.md @@ -1,6 +1,6 @@ # Cursor Cloud Agents — parallel workflow -Operator playbook for running 15–20 [Cursor Cloud Agents](https://docs.cursor.com/agents/cloud) in parallel against OpenHuman. Companion to [`codex-pr-checklist.md`](codex-pr-checklist.md); the same merge gates apply. +Operator playbook for running 15–20 [Cursor Cloud Agents](https://cursor.com/docs/cloud-agent) in parallel against OpenHuman. Companion to [`codex-pr-checklist.md`](codex-pr-checklist.md); the same merge gates apply. This doc closes [`tinyhumansai/openhuman#1480`](https://github.com/tinyhumansai/openhuman/issues/1480). diff --git a/src/core/observability.rs b/src/core/observability.rs index 061c8bdc4..8edf52279 100644 --- a/src/core/observability.rs +++ b/src/core/observability.rs @@ -2665,6 +2665,29 @@ pub fn is_transient_backend_api_failure(event: &sentry::protocol::Event<'_>) -> is_transient_domain_failure(event, "backend_api") } +/// Defense-in-depth `before_send` filter for skill-install fetch 4xx statuses. +/// +/// A user/catalog-supplied `SKILL.md` URL returning 4xx means the remote skill +/// path is missing, private, or otherwise unavailable to that user. The install +/// RPC still returns the error so the UI can surface it, but Sentry should keep +/// reporting server-side and transport failures only. +pub fn is_skill_install_user_fetch_failure(event: &sentry::protocol::Event<'_>) -> bool { + let tags = &event.tags; + if tags.get("domain").map(String::as_str) != Some("skills") { + return false; + } + if tags.get("operation").map(String::as_str) != Some("install_fetch") { + return false; + } + if tags.get("failure").map(String::as_str) != Some("non_2xx") { + return false; + } + + tags.get("status") + .and_then(|status| status.parse::().ok()) + .is_some_and(|status| (400..500).contains(&status)) +} + /// Transient integrations / Composio failures (timeout, connection reset, /// gateway hiccups). /// @@ -6104,6 +6127,65 @@ mod tests { ); } + #[test] + fn skills_install_fetch_filter_drops_client_error_statuses() { + for status in ["400", "401", "403", "404", "410", "499"] { + let event = event_with_tags(&[ + ("domain", "skills"), + ("operation", "install_fetch"), + ("failure", "non_2xx"), + ("status", status), + ]); + assert!( + is_skill_install_user_fetch_failure(&event), + "skills install_fetch status {status} must be treated as user/catalog state" + ); + } + } + + #[test] + fn skills_install_fetch_filter_keeps_server_and_wrong_shape_failures() { + for status in ["500", "502", "503"] { + let event = event_with_tags(&[ + ("domain", "skills"), + ("operation", "install_fetch"), + ("failure", "non_2xx"), + ("status", status), + ]); + assert!( + !is_skill_install_user_fetch_failure(&event), + "skills install_fetch status {status} must remain reportable" + ); + } + + for tags in [ + [ + ("domain", "skills"), + ("operation", "install_fetch"), + ("failure", "transport"), + ("status", "404"), + ], + [ + ("domain", "skills"), + ("operation", "run"), + ("failure", "non_2xx"), + ("status", "404"), + ], + [ + ("domain", "backend_api"), + ("operation", "install_fetch"), + ("failure", "non_2xx"), + ("status", "404"), + ], + ] { + let event = event_with_tags(&tags); + assert!( + !is_skill_install_user_fetch_failure(&event), + "only skills.install_fetch non_2xx 4xx events may be filtered: {tags:?}" + ); + } + } + #[test] fn integrations_filter_drops_transient_statuses() { for status in TRANSIENT_HTTP_STATUSES { diff --git a/src/main.rs b/src/main.rs index 5727b27c8..45324defe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -129,6 +129,7 @@ fn main() { if openhuman_core::core::observability::is_transient_backend_api_failure(&event) || openhuman_core::core::observability::is_transient_integrations_failure(&event) || openhuman_core::core::observability::is_updater_transient_event(&event) + || openhuman_core::core::observability::is_skill_install_user_fetch_failure(&event) { return None; } diff --git a/src/openhuman/workflows/ops.rs b/src/openhuman/workflows/ops.rs index db3116557..d6ad88310 100644 --- a/src/openhuman/workflows/ops.rs +++ b/src/openhuman/workflows/ops.rs @@ -52,6 +52,7 @@ pub(crate) use super::ops_discover::discover_workflows_inner; #[cfg(test)] pub(crate) use super::ops_install::{ derive_install_slug, install_workflow_from_url_with_home, normalize_install_url, + should_report_install_fetch_status, }; #[cfg(test)] pub(crate) use super::ops_types::{ diff --git a/src/openhuman/workflows/ops_install.rs b/src/openhuman/workflows/ops_install.rs index faa990e99..be2f81e5c 100644 --- a/src/openhuman/workflows/ops_install.rs +++ b/src/openhuman/workflows/ops_install.rs @@ -122,6 +122,10 @@ pub async fn install_workflow_from_url( install_workflow_from_url_with_home(workspace_dir, params, home.as_deref()).await } +pub(crate) fn should_report_install_fetch_status(status: reqwest::StatusCode) -> bool { + !status.is_success() && !status.is_client_error() +} + pub(crate) async fn install_workflow_from_url_with_home( workspace_dir: &Path, params: InstallWorkflowFromUrlParams, @@ -196,8 +200,6 @@ pub(crate) async fn install_workflow_from_url_with_home( let status = response.status(); if !status.is_success() { - let code = status.as_u16(); - let msg = format!("fetch failed: {fetch_url} returned status {code}"); // A 4xx (esp. 404/410) means the requested SKILL.md is gone or the URL // is wrong — expected user/catalog input state, surfaced to the UI as // "skill not found". Don't page Sentry for it (TAURI-RUST-CGE: ~1,446 @@ -205,9 +207,16 @@ pub(crate) async fn install_workflow_from_url_with_home( // reporting 5xx — a genuine remote failure is still Sentry-actionable. // The `Err(msg)` return is unchanged in both cases so the UI always // surfaces the failure. - if !status.is_client_error() { - let status_str = code.to_string(); - let report_msg = format!("fetch failed: {redacted_fetch_url} returned status {code}"); + let status_str = status.as_u16().to_string(); + let msg = format!( + "fetch failed: {fetch_url} returned status {}", + status.as_u16() + ); + let report_msg = format!( + "fetch failed: {redacted_fetch_url} returned status {}", + status.as_u16() + ); + if should_report_install_fetch_status(status) { crate::core::observability::report_error( report_msg.as_str(), "skills", @@ -218,6 +227,12 @@ pub(crate) async fn install_workflow_from_url_with_home( ("failure", "non_2xx"), ], ); + } else { + tracing::debug!( + fetch_url = %redacted_fetch_url, + status = status.as_u16(), + "[skills] install_workflow_from_url: skipped Sentry report for user/catalog fetch status" + ); } return Err(msg); } diff --git a/src/openhuman/workflows/ops_tests.rs b/src/openhuman/workflows/ops_tests.rs index 24e5a4017..c102a62a0 100644 --- a/src/openhuman/workflows/ops_tests.rs +++ b/src/openhuman/workflows/ops_tests.rs @@ -1244,6 +1244,23 @@ async fn install_workflow_from_url_is_idempotent_when_skill_already_exists() { assert!(second.stdout.contains("already installed"), "{second:?}"); } +#[test] +fn install_fetch_status_reporting_suppresses_client_errors_only() { + assert!(!should_report_install_fetch_status(reqwest::StatusCode::OK)); + assert!(!should_report_install_fetch_status( + reqwest::StatusCode::NOT_FOUND + )); + assert!(!should_report_install_fetch_status( + reqwest::StatusCode::GONE + )); + assert!(should_report_install_fetch_status( + reqwest::StatusCode::INTERNAL_SERVER_ERROR + )); + assert!(should_report_install_fetch_status( + reqwest::StatusCode::BAD_GATEWAY + )); +} + /// Happy path: install a SKILL.md under a synthetic user home, verify /// discovery sees it, uninstall, verify discovery no longer sees it and /// the on-disk dir is gone. diff --git a/tests/observability_smoke.rs b/tests/observability_smoke.rs index 304694f4e..151f11f66 100644 --- a/tests/observability_smoke.rs +++ b/tests/observability_smoke.rs @@ -10,8 +10,9 @@ use openhuman_core::core::observability::{ is_all_transient_provider_exhaustion_event, is_budget_event, is_session_expired_event, - is_transient_backend_api_failure, is_transient_integrations_failure, - is_transient_provider_http_failure, is_updater_transient_event, + is_skill_install_user_fetch_failure, is_transient_backend_api_failure, + is_transient_integrations_failure, is_transient_provider_http_failure, + is_updater_transient_event, }; use sentry::protocol::Event; use std::collections::BTreeMap; @@ -63,6 +64,7 @@ fn count_captured(events: Vec>) -> usize { || is_transient_integrations_failure(&event) || is_budget_event(&event) || is_updater_transient_event(&event) + || is_skill_install_user_fetch_failure(&event) || is_session_expired_event(&event) { None @@ -119,6 +121,36 @@ fn drops_backend_api_transient_statuses() { ); } +#[test] +fn drops_skills_install_fetch_404() { + let event = event_with_tags(&[ + ("domain", "skills"), + ("operation", "install_fetch"), + ("failure", "non_2xx"), + ("status", "404"), + ]); + assert_eq!( + count_captured(vec![event]), + 0, + "user/catalog skill install 4xx failures must be filtered in before_send" + ); +} + +#[test] +fn keeps_skills_install_fetch_500() { + let event = event_with_tags(&[ + ("domain", "skills"), + ("operation", "install_fetch"), + ("failure", "non_2xx"), + ("status", "500"), + ]); + assert_eq!( + count_captured(vec![event]), + 1, + "skill install server failures must still reach Sentry" + ); +} + #[test] fn drops_integrations_transient_transport_timeout() { let event = event_with_tags_and_message(