mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(observability): demote skill install 4xx fetches (#4238)
Co-authored-by: M3gA-Mind <megamind@mahadao.com>
This commit is contained in:
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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).
|
||||
|
||||
|
||||
@@ -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::<u16>().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 {
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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::{
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<Event<'static>>) -> 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(
|
||||
|
||||
Reference in New Issue
Block a user