From f492493601c4c23e9810771aa4403fe39d8b0064 Mon Sep 17 00:00:00 2001 From: CodeGhost21 <164498022+CodeGhost21@users.noreply.github.com> Date: Tue, 7 Jul 2026 00:14:15 +0530 Subject: [PATCH] fix(skills): thread local-http install flag as param, not process env (#4567) (#4584) --- docs/README.de.md | 2 +- docs/README.ja-JP.md | 2 +- docs/README.ko.md | 2 +- docs/README.ur-pk.md | 2 +- docs/README.zh-CN.md | 2 +- src/openhuman/skills/ops_install.rs | 51 +++++++++++++++++++---------- src/openhuman/skills/ops_tests.rs | 47 +++++++++----------------- 7 files changed, 54 insertions(+), 54 deletions(-) diff --git a/docs/README.de.md b/docs/README.de.md index ccafae844..950b057c0 100644 --- a/docs/README.de.md +++ b/docs/README.de.md @@ -43,7 +43,7 @@

Frühe Beta Aktuellste Version - GitHub Stars + GitHub Stars Lizenz

diff --git a/docs/README.ja-JP.md b/docs/README.ja-JP.md index 78ecafb5c..0bcd7f73a 100644 --- a/docs/README.ja-JP.md +++ b/docs/README.ja-JP.md @@ -43,7 +43,7 @@

Early Beta 最新リリース - GitHub Stars + GitHub Stars ライセンス

diff --git a/docs/README.ko.md b/docs/README.ko.md index f4c5657ab..348340772 100644 --- a/docs/README.ko.md +++ b/docs/README.ko.md @@ -43,7 +43,7 @@

얼리 베타 최신 릴리스 - GitHub Stars + GitHub Stars 라이선스

diff --git a/docs/README.ur-pk.md b/docs/README.ur-pk.md index be00f30fd..20fbfe6ac 100644 --- a/docs/README.ur-pk.md +++ b/docs/README.ur-pk.md @@ -53,7 +53,7 @@

ابتدائی آزمائشی نسخہ تازہ ترین نسخہ - ستارے + ستارے لائسنس

diff --git a/docs/README.zh-CN.md b/docs/README.zh-CN.md index 8ec203f11..55c0eb95d 100644 --- a/docs/README.zh-CN.md +++ b/docs/README.zh-CN.md @@ -43,7 +43,7 @@

早期测试版 最新版本 - GitHub Stars + GitHub Stars 许可证

diff --git a/src/openhuman/skills/ops_install.rs b/src/openhuman/skills/ops_install.rs index be2f81e5c..39eecef0f 100644 --- a/src/openhuman/skills/ops_install.rs +++ b/src/openhuman/skills/ops_install.rs @@ -119,7 +119,9 @@ pub async fn install_workflow_from_url( params: InstallWorkflowFromUrlParams, ) -> Result { let home = dirs::home_dir(); - install_workflow_from_url_with_home(workspace_dir, params, home.as_deref()).await + let allow_local_http = read_allow_local_http_env(); + install_workflow_from_url_with_home(workspace_dir, params, home.as_deref(), allow_local_http) + .await } pub(crate) fn should_report_install_fetch_status(status: reqwest::StatusCode) -> bool { @@ -130,9 +132,10 @@ pub(crate) async fn install_workflow_from_url_with_home( workspace_dir: &Path, params: InstallWorkflowFromUrlParams, home: Option<&Path>, + allow_local_http: bool, ) -> Result { let raw_url = params.url.trim().to_string(); - validate_install_url(&raw_url)?; + validate_install_url_with_config(&raw_url, allow_local_http)?; let timeout_secs = params .timeout_secs @@ -148,7 +151,7 @@ pub(crate) async fn install_workflow_from_url_with_home( // resolver may see different answers than ours. Closing that gap requires // pinning a `SocketAddr` and passing it to reqwest via a custom resolver, // tracked separately. - if !allow_local_http_install_url(&fetch_url) { + if !(allow_local_http && is_loopback_http_url(&fetch_url)) { validate_resolved_host(&fetch_url).await?; } @@ -698,6 +701,19 @@ pub(crate) fn derive_install_slug(fm: &WorkflowFrontmatter) -> Result Result<(), String> { + validate_install_url_with_config(raw, read_allow_local_http_env()) +} + +/// Same as [`validate_install_url`] but takes an explicit `allow_local_http` +/// flag instead of reading `OPENHUMAN_SKILL_INSTALL_ALLOW_LOCAL_HTTP` from the +/// process environment. Callers below the public entry point should always use +/// this variant — the env-var read in `validate_install_url` is racy across +/// threads under the parallel test runner (#4567), and the escape hatch is +/// only meaningful at a single boundary. +pub(crate) fn validate_install_url_with_config( + raw: &str, + allow_local_http: bool, +) -> Result<(), String> { let trimmed = raw.trim(); if trimmed.is_empty() { return Err("url must not be empty".to_string()); @@ -710,7 +726,7 @@ pub fn validate_install_url(raw: &str) -> Result<(), String> { } let parsed = url::Url::parse(trimmed).map_err(|e| format!("invalid url {trimmed:?}: {e}"))?; if parsed.scheme() != "https" { - if allow_local_http_install_url(trimmed) { + if allow_local_http && is_loopback_http_url(trimmed) { return Ok(()); } return Err(format!( @@ -732,10 +748,15 @@ pub fn validate_install_url(raw: &str) -> Result<(), String> { Ok(()) } -fn allow_local_http_install_url(raw: &str) -> bool { - if std::env::var(ALLOW_LOCAL_HTTP_ENV).ok().as_deref() != Some("1") { - return false; - } +/// Reads the local-HTTP install escape hatch env var. Kept as the single +/// boundary point where env is inspected so downstream validation never +/// touches process-global state — required to stop the parallel-test race +/// described in #4567. +fn read_allow_local_http_env() -> bool { + std::env::var(ALLOW_LOCAL_HTTP_ENV).ok().as_deref() == Some("1") +} + +fn is_loopback_http_url(raw: &str) -> bool { let Ok(parsed) = url::Url::parse(raw) else { return false; }; @@ -907,15 +928,12 @@ mod install_fetch_tests { /// for both a 4xx (which is NOT reported to Sentry) and a 5xx (which is). /// Exercises both branches of the non-2xx handling; the report-suppression /// polarity itself is asserted by `is_skills_install_client_error_event` in - /// observability. Uses the local-HTTP install escape hatch so the loopback - /// mock passes URL validation. + /// observability. Passes the local-HTTP install escape hatch as an + /// explicit param so the loopback mock passes URL validation — the env-var + /// path is process-global and races with other env-touching tests under + /// parallel execution (#4567). #[tokio::test] async fn non_2xx_install_fetch_returns_err_for_4xx_and_5xx() { - let _env = crate::openhuman::config::TEST_ENV_LOCK - .lock() - .unwrap_or_else(|e| e.into_inner()); - std::env::set_var(ALLOW_LOCAL_HTTP_ENV, "1"); - let tmp = TempDir::new().unwrap(); for status in [StatusCode::NOT_FOUND, StatusCode::INTERNAL_SERVER_ERROR] { @@ -928,6 +946,7 @@ mod install_fetch_tests { timeout_secs: Some(5), }, None, + true, ) .await .expect_err("a non-2xx fetch must return Err so the UI surfaces it"); @@ -936,7 +955,5 @@ mod install_fetch_tests { "error must surface the status to the UI: {err}" ); } - - std::env::remove_var(ALLOW_LOCAL_HTTP_ENV); } } diff --git a/src/openhuman/skills/ops_tests.rs b/src/openhuman/skills/ops_tests.rs index a38004b86..d49ccfba7 100644 --- a/src/openhuman/skills/ops_tests.rs +++ b/src/openhuman/skills/ops_tests.rs @@ -7,30 +7,6 @@ fn write(path: &Path, content: &str) { std::fs::write(path, content).unwrap(); } -struct EnvVarGuard { - key: &'static str, - previous: Option, -} - -impl EnvVarGuard { - fn set(key: &'static str, value: &str) -> Self { - let previous = std::env::var_os(key); - unsafe { std::env::set_var(key, value) }; - Self { key, previous } - } -} - -impl Drop for EnvVarGuard { - fn drop(&mut self) { - unsafe { - match &self.previous { - Some(value) => std::env::set_var(self.key, value), - None => std::env::remove_var(self.key), - } - } - } -} - /// Workspace-only variant of [`load_workflow_metadata`] used by tests that care only /// about project-scope semantics. The production [`load_workflow_metadata`] now /// consults `dirs::home_dir()`; in unit tests that would non-deterministically @@ -1214,7 +1190,6 @@ async fn install_workflow_from_url_is_idempotent_when_skill_already_exists() { use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; - let _guard = EnvVarGuard::set("OPENHUMAN_SKILL_INSTALL_ALLOW_LOCAL_HTTP", "1"); let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/SKILL.md")) @@ -1231,15 +1206,23 @@ async fn install_workflow_from_url_is_idempotent_when_skill_already_exists() { timeout_secs: Some(5), }; - let first = - install_workflow_from_url_with_home(workspace.path(), params.clone(), Some(home.path())) - .await - .unwrap(); + // Pass the local-HTTP escape hatch as an explicit param — the env-var + // path is process-global and races other env-touching tests under + // parallel execution (#4567). + let first = install_workflow_from_url_with_home( + workspace.path(), + params.clone(), + Some(home.path()), + true, + ) + .await + .unwrap(); assert_eq!(first.new_skills, vec!["apple-notes"]); - let second = install_workflow_from_url_with_home(workspace.path(), params, Some(home.path())) - .await - .unwrap(); + let second = + install_workflow_from_url_with_home(workspace.path(), params, Some(home.path()), true) + .await + .unwrap(); assert!(second.new_skills.is_empty(), "{second:?}"); assert!(second.stdout.contains("already installed"), "{second:?}"); }