Skip to content

test(security): add integration test for load_and_get_security_policy_info env override #2696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PranavAgarkar07 wants to merge 1 commit into from
+28 −0
Closed

test(security): add integration test for load_and_get_security_policy_info env override #2696

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 28 additions & 0 deletions src/openhuman/security/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,32 @@ mod tests {

assert_eq!(outcome.value["max_actions_per_hour"], json!(77));
}

Copy link
Copy Markdown
Contributor

@graycyrus graycyrus May 26, 2026

Choose a reason for hiding this comment

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

[major] The test / Rust Core Tests + Quality CI job fails, and your own checklist notes a pre-existing compilation error in apply_autonomy_settings_updates_action_budget blocks cargo test --lib. That means this test was never actually verified to pass.

I need one of:

  1. Fix the pre-existing compilation error in this PR (preferred — you're already in this file), or
  2. Show output from cargo test --lib -p openhuman -- load_and_get_security_policy_info_reflects_env_override proving the new test passes when run in isolation.

Shipping an unverified test defeats the purpose of the issue.

#[tokio::test]
async fn load_and_get_security_policy_info_reflects_env_override() {
// Serializes env-mutation with sibling tests that touch
// OPENHUMAN_WORKSPACE or OPENHUMAN_MAX_ACTIONS_PER_HOUR.
let _lock = crate::openhuman::config::TEST_ENV_LOCK
.lock()
.unwrap_or_else(|e| e.into_inner());

let tmp = tempfile::tempdir().unwrap();
let workspace = tmp.path().to_str().unwrap().to_string();

unsafe {
std::env::set_var("OPENHUMAN_WORKSPACE", &workspace);
std::env::set_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR", "42");
}

let outcome = load_and_get_security_policy_info()
.await
.expect("load_and_get_security_policy_info should succeed");

assert_eq!(outcome.value["max_actions_per_hour"], serde_json::json!(42));

unsafe {
std::env::remove_var("OPENHUMAN_WORKSPACE");
std::env::remove_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR");
}
Comment on lines +109 to +123
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make env cleanup panic-safe to prevent cross-test contamination.

Right now cleanup only runs at Line 120 after assertions. If the test fails earlier, OPENHUMAN_WORKSPACE / OPENHUMAN_MAX_ACTIONS_PER_HOUR can leak and cause flaky follow-up tests. Use a drop guard so cleanup always executes.

Suggested fix 
 #[tokio::test]
 async fn load_and_get_security_policy_info_reflects_env_override() {
@@
-    unsafe {
-        std::env::set_var("OPENHUMAN_WORKSPACE", &workspace);
-        std::env::set_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR", "42");
-    }
+    struct EnvCleanup;
+    impl Drop for EnvCleanup {
+        fn drop(&mut self) {
+            unsafe {
+                std::env::remove_var("OPENHUMAN_WORKSPACE");
+                std::env::remove_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR");
+            }
+        }
+    }
+    let _cleanup = EnvCleanup;
+
+    unsafe {
+        std::env::set_var("OPENHUMAN_WORKSPACE", &workspace);
+        std::env::set_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR", "42");
+    }
@@
-    unsafe {
-        std::env::remove_var("OPENHUMAN_WORKSPACE");
-        std::env::remove_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR");
-    }
 }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/security/ops.rs` around lines 109 - 123, The test sets
OPENHUMAN_WORKSPACE and OPENHUMAN_MAX_ACTIONS_PER_HOUR but only removes them
after assertions, so failures leak env state; create a small drop guard (e.g.,
struct EnvGuard) that captures the names (and optionally the previous values)
and implements Drop to remove/restore OPENHUMAN_WORKSPACE and
OPENHUMAN_MAX_ACTIONS_PER_HOUR, then instantiate that guard immediately after
setting the env vars in the test in src/openhuman/security/ops.rs so
load_and_get_security_policy_info() can run and the guard’s Drop will always run
even if the test panics.

}
Comment on lines +98 to +124
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add the missing OPENHUMAN_MAX_ACTIONS_PER_HOUR=0 edge-case assertion.

The linked issue acceptance criteria explicitly asks to verify the zero-value path as well. This test currently validates only 42, so the requested regression guard is still incomplete.

Suggested addition 
 #[tokio::test]
 async fn load_and_get_security_policy_info_reflects_env_override() {
@@
     assert_eq!(outcome.value["max_actions_per_hour"], serde_json::json!(42));
+
+    unsafe {
+        std::env::set_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR", "0");
+    }
+    let zero_outcome = load_and_get_security_policy_info()
+        .await
+        .expect("load_and_get_security_policy_info should succeed for zero override");
+    assert_eq!(
+        zero_outcome.value["max_actions_per_hour"],
+        serde_json::json!(0)
+    );
@@
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#[tokio::test]
async fn load_and_get_security_policy_info_reflects_env_override() {
// Serializes env-mutation with sibling tests that touch
// OPENHUMAN_WORKSPACE or OPENHUMAN_MAX_ACTIONS_PER_HOUR.
let _lock = crate::openhuman::config::TEST_ENV_LOCK
.lock()
.unwrap_or_else(|e| e.into_inner());
let tmp = tempfile::tempdir().unwrap();
let workspace = tmp.path().to_str().unwrap().to_string();
unsafe {
std::env::set_var("OPENHUMAN_WORKSPACE", &workspace);
std::env::set_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR", "42");
}
let outcome = load_and_get_security_policy_info()
.await
.expect("load_and_get_security_policy_info should succeed");
assert_eq!(outcome.value["max_actions_per_hour"], serde_json::json!(42));
unsafe {
std::env::remove_var("OPENHUMAN_WORKSPACE");
std::env::remove_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR");
}
}
#[tokio::test]
async fn load_and_get_security_policy_info_reflects_env_override() {
// Serializes env-mutation with sibling tests that touch
// OPENHUMAN_WORKSPACE or OPENHUMAN_MAX_ACTIONS_PER_HOUR.
let _lock = crate::openhuman::config::TEST_ENV_LOCK
.lock()
.unwrap_or_else(|e| e.into_inner());
let tmp = tempfile::tempdir().unwrap();
let workspace = tmp.path().to_str().unwrap().to_string();
unsafe {
std::env::set_var("OPENHUMAN_WORKSPACE", &workspace);
std::env::set_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR", "42");
}
let outcome = load_and_get_security_policy_info()
.await
.expect("load_and_get_security_policy_info should succeed");
assert_eq!(outcome.value["max_actions_per_hour"], serde_json::json!(42));
unsafe {
std::env::set_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR", "0");
}
let zero_outcome = load_and_get_security_policy_info()
.await
.expect("load_and_get_security_policy_info should succeed for zero override");
assert_eq!(
zero_outcome.value["max_actions_per_hour"],
serde_json::json!(0)
);
unsafe {
std::env::remove_var("OPENHUMAN_WORKSPACE");
std::env::remove_var("OPENHUMAN_MAX_ACTIONS_PER_HOUR");
}
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/security/ops.rs` around lines 98 - 124, The test
load_and_get_security_policy_info_reflects_env_override only asserts the 42
case; add a second edge-case assertion for OPENHUMAN_MAX_ACTIONS_PER_HOUR=0 by
setting the env var to "0", calling load_and_get_security_policy_info() again,
and asserting outcome.value["max_actions_per_hour"] equals serde_json::json!(0);
keep using the same TEST_ENV_LOCK and ensure you remove the env vars in the
existing cleanup block (or restore previous values) to avoid leaking the 0
override.

}
Loading