test(security): add integration tests for load_and_get_security_policy_info (#2688)

[staimoorulhassan]

Summary

• Adds two async integration tests for load_and_get_security_policy_info in src/openhuman/security/ops.rs, covering the full env-var → config load → RPC payload chain that was previously only exercised by the JSON-RPC smoke test (follow-up from issue Add direct integration test for load_and_get_security_policy_info #2688)
• load_and_get_security_policy_info_reflects_env_overlay — sets OPENHUMAN_MAX_ACTIONS_PER_HOUR=42 and asserts the RPC payload contains 42
• load_and_get_security_policy_info_ignores_zero_budget — sets the var to 0 (invalid value, must be ≥ 1) and asserts the config default is preserved instead

Both tests use TEST_ENV_LOCK for process-wide env serialisation, matching the pattern used across the test suite. Env vars are now restored via an RAII EnvRestore drop guard so cleanup is panic-safe (CodeRabbit review). Also aligned the env overlay zero-check with the RPC update path: apply_env_overlay now ignores OPENHUMAN_MAX_ACTIONS_PER_HOUR=0 with a warning, consistent with the RPC handler that already rejects zero.

Test plan

• cargo test -p openhuman security::ops passes
• load_and_get_security_policy_info_reflects_env_overlay asserts max_actions_per_hour == 42
• load_and_get_security_policy_info_ignores_zero_budget asserts max_actions_per_hour == <default> when env var is 0
• No regressions in existing security_policy_info_* unit tests

Closes #2688

Summary by CodeRabbit

• Tests

  • Added integration tests to validate that environment-based configuration overlays propagate correctly into security policy payloads and respect fallback behavior.

• Bug Fixes

  • Treat zero-value environment overrides for the max-actions-per-hour setting as invalid; retain the configured default instead of applying the invalid override.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b272e32e-461d-4ed3-97fa-ce21abb94699

📥 Commits

Reviewing files that changed from the base of the PR and between fb46e3e and cda114e.

📒 Files selected for processing (3)

• src/openhuman/config/schema/load.rs
• src/openhuman/config/schema/load_tests.rs
• src/openhuman/security/ops.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/openhuman/security/ops.rs

---

📝 Walkthrough

Walkthrough

Adds config validation rejecting OPENHUMAN_MAX_ACTIONS_PER_HOUR=0 and two async tests that set the env var, call load_and_get_security_policy_info(), and assert the RPC payload reflects a valid override (42) or preserves the default when the env override is invalid (0).

Changes

Security policy environment configuration tests

Layer / File(s)	Summary
Env overlay validation in config src/openhuman/config/schema/load.rs, src/openhuman/config/schema/load_tests.rs	apply_env_overlay_with now treats OPENHUMAN_MAX_ACTIONS_PER_HOUR=0 as invalid (logs a warning and does not assign); tests assert configured value remains when env is "0".
load_and_get_security_policy_info env overlay tests src/openhuman/security/ops.rs	Introduces EnvRestore test helper and two #[tokio::test] cases that lock env state, set OPENHUMAN_WORKSPACE and OPENHUMAN_MAX_ACTIONS_PER_HOUR, call the loader, and assert the RPC payload’s autonomy.max_actions_per_hour for valid (42) and invalid (0 → default preserved) overlays.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• tinyhumansai/openhuman#2201: Related env-overlay validation changes for config parsing and tests.
• tinyhumansai/openhuman#2499: Introduced the env-driven Action Budget feature that these tests and validation exercise.

Suggested labels

bug

Suggested reviewers

• graycyrus

Poem

🐰 In burrows of code I hop with glee,
I patch the env that others can't see.
Two tests I plant, one true, one sly,
So zero won't sneak and defaults stay nigh.
Hooray—config and payload sing, safe as can be!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: adding integration tests for load_and_get_security_policy_info, matching the primary objective of the PR.
Linked Issues check	✅ Passed	The PR fully implements the requirements from issue #2688: integration tests exercising env-var→config→RPC payload chain, coverage of valid overlay (42) and zero edge case, env mutation serialization with restoration.
Out of Scope Changes check	✅ Passed	All changes are within scope: env-overlay validation logic change to reject zero values is necessary to support the tests and aligns with RPC handler behavior per issue requirements.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/openhuman/security/ops.rs`:
- Around line 62-65: Introduce an RAII guard (e.g., RestoreEnvGuard) that
captures the previous values (Option<String>) of
"OPENHUMAN_MAX_ACTIONS_PER_HOUR" and "OPENHUMAN_WORKSPACE" when created and
restores them in its Drop impl, then use this guard at the start of both test
blocks (the places that currently call std::env::remove_var inside unsafe) so
cleanup always runs even if assertions panic; replace the direct
std::env::remove_var calls with instantiation of RestoreEnvGuard and let Drop
handle restoration.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1667608b-016e-4647-8629-518693ba6c49

📥 Commits

Reviewing files that changed from the base of the PR and between 3f2e2f2 and fb46e3e.

📒 Files selected for processing (1)

• src/openhuman/security/ops.rs

[staimoorulhassan]

Hi @graycyrus, thanks for the review! You're right that the Windows secrets ACL job fails on external contributor PRs because org secrets aren't injected for fork branches ("Secret source: None" in the runner log). I confirmed this is a fork limitation, not caused by our changes — the only files this PR touches are src/openhuman/security/ops.rs and src/openhuman/config/schema/load{,.rs,_tests.rs}, none of which are Windows-specific.

I pushed a re-trigger commit to get a fresh CI run. The job is currently pending and should finish clean shortly.

[graycyrus]

Clean integration test coverage for load_and_get_security_policy_info. The full env → config → RPC payload chain is now exercised directly via cargo test, not just the JSON-RPC smoke.

A few things worth noting:

load.rs zero-guard — the added Ok(0) arm in apply_env_overlay is a correct fix and matches how the RPC update path already handles zero. The warning log is the right call here.

Issue acceptance criteria drift — issue #2688 said the zero case should "surface as zero in the RPC payload", but the actual correct behavior (and what the PR implements) is to ignore the invalid value and preserve the default. The PR description explains the rationale clearly; the issue criteria were slightly off. The implementation is right.

RAII EnvRestore guard — properly restores both vars on drop regardless of panic path. The unsafe usage is justified: TEST_ENV_LOCK serialises the mutation, which is the correct pattern across the test suite. No issues.

Approved.

[sanil-23]

Closing as superseded by #2695 for issue #2688.

Both PRs add env-overlay coverage for load_and_get_security_policy_info, and the goals overlap closely (env-var → config load → RPC payload chain, =0 edge case, panic-safe env handling). #2695 is the in-flight winner here: it has 4 iterated commits (env-overlay path + hint:summarization follow-up + symlink-skip invariant rewrite + the =0 edge case with panic-safe env guards), all 32 CI checks green, and is currently being shepherded through the last reviewer pass.

Thanks for the contribution! The apply_env_overlay zero-check alignment you proposed here (mirroring the RPC handler's existing zero rejection) is a thoughtful touch — if you'd like to land that as a focused follow-up after #2695 merges, happy to look at a small follow-up PR scoped just to that delta in config/schema/load.rs.