Conversation
Add ~100 new unit tests covering: - security/request_signer.rs: HMAC signature computation, verification, timestamp validation, missing headers, stale requests, tampered bodies - security/scopes.rs: scope parsing from env, authorization logic, empty scopes, whitespace trimming, missing env var handling - security/audit_log.rs: all audit event methods with various parameter combinations (Some/None), clone safety, default trait - security/rate_limit.rs: rate limiting enforcement, window expiry, independent keys, zero limit, clone-safe shared state - commands/version_check.rs: RemoteVersion deserialization, missing env var handling, empty env var - commands/deploy.rs: rollback manifest serialization roundtrip, manifest load/save with mutex-guarded env vars, backup creation, empty manifest rollback - monitoring/mod.rs: MetricsSnapshot default/serialization, ControlPlane display/serialization/equality, CommandExecutionMetrics recording and serialization, MetricsCollector snapshot validation - transport/http_polling.rs: URL building, JSON extraction helpers, field parsing, command extraction from various JSON shapes Co-authored-by: vsilent <42473+vsilent@users.noreply.github.com> Agent-Logs-Url: https://github.com/trydirect/status/sessions/b8abdc87-ed3f-4f06-95e0-0dde0f62283d
Address code review feedback: protect env var manipulation in tests with mutex guards to prevent race conditions during parallel execution. Co-authored-by: vsilent <42473+vsilent@users.noreply.github.com> Agent-Logs-Url: https://github.com/trydirect/status/sessions/b8abdc87-ed3f-4f06-95e0-0dde0f62283d
There was a problem hiding this comment.
Pull request overview
Adds unit tests across transport, security, monitoring, and commands modules to increase confidence/coverage around request signing, scope parsing, rate limiting, polling JSON parsing, metrics snapshots, and deploy rollback manifest behavior.
Changes:
- Added/expanded unit tests for HTTP polling URL construction and JSON parsing helpers.
- Added unit tests for security components (scopes parsing, request signature verification, rate limiting, audit logging).
- Added unit tests for monitoring metrics structs/collector and update/deploy version + rollback manifest behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/transport/http_polling.rs | Adds unit tests for wait URL building and JSON parsing/extraction helpers. |
| src/security/scopes.rs | Adds tests validating AGENT_SCOPES parsing and allow/deny behavior. |
| src/security/request_signer.rs | Adds tests for signature computation, decoding, and verification paths. |
| src/security/rate_limit.rs | Adds async tests for rate limiter limits, key isolation, expiry, and clone behavior. |
| src/security/audit_log.rs | Adds “does not panic” tests for audit logger methods and traits. |
| src/monitoring/mod.rs | Adds tests for metrics structs (defaults/serde) and collector snapshot sanity checks. |
| src/commands/version_check.rs | Adds tests for RemoteVersion deserialization and env-var early returns. |
| src/commands/deploy.rs | Adds tests for rollback manifest serialization and file-based manifest operations. |
| fn scopes_from_env_parses_comma_separated() { | ||
| let _guard = env_lock().lock().expect("env lock poisoned"); | ||
| std::env::set_var("AGENT_SCOPES", "docker:restart,docker:logs,admin"); | ||
| let scopes = Scopes::from_env(); | ||
| assert!(scopes.is_allowed("docker:restart")); | ||
| assert!(scopes.is_allowed("docker:logs")); | ||
| assert!(scopes.is_allowed("admin")); | ||
| assert!(!scopes.is_allowed("docker:stop")); | ||
| std::env::remove_var("AGENT_SCOPES"); | ||
| } |
There was a problem hiding this comment.
These tests mutate AGENT_SCOPES and then unconditionally remove_var it, which can break other tests (or developer environments) when AGENT_SCOPES was set before the test run. Capture the previous value at the start of each test (or in a small helper) and restore it in a Drop guard after assertions.
| #[test] | ||
| fn scopes_from_env_missing_var_allows_all() { | ||
| let _guard = env_lock().lock().expect("env lock poisoned"); | ||
| std::env::remove_var("AGENT_SCOPES"); | ||
| let scopes = Scopes::from_env(); | ||
| assert!(scopes.is_allowed("anything")); | ||
| } |
There was a problem hiding this comment.
scopes_from_env_missing_var_allows_all removes AGENT_SCOPES but never restores its prior value, which can leak state into later tests. Save the original env value before modification and restore it at the end (even on panic) using a guard.
| #[tokio::test] | ||
| async fn check_remote_version_no_env_returns_none() { | ||
| let _guard = env_lock().lock().expect("env lock poisoned"); | ||
| std::env::remove_var("UPDATE_SERVER_URL"); | ||
| let result = check_remote_version().await.unwrap(); | ||
| assert!(result.is_none()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn check_remote_version_empty_env_returns_none() { | ||
| let _guard = env_lock().lock().expect("env lock poisoned"); | ||
| std::env::set_var("UPDATE_SERVER_URL", ""); | ||
| let result = check_remote_version().await.unwrap(); | ||
| assert!(result.is_none()); | ||
| std::env::remove_var("UPDATE_SERVER_URL"); | ||
| } |
There was a problem hiding this comment.
These tests modify UPDATE_SERVER_URL and either remove it or leave it empty without restoring any pre-existing value. To avoid leaking env changes across the test suite, capture the original value and restore it in a Drop guard (or use a small helper like with_env_var).
src/commands/deploy.rs
Outdated
| std::env::set_var("UPDATE_STORAGE_PATH", "/tmp/status-test-nonexistent-path"); | ||
| let manifest = load_manifest().await.unwrap(); | ||
| assert!(manifest.entries.is_empty()); |
There was a problem hiding this comment.
load_manifest_nonexistent_returns_default uses a hard-coded /tmp/status-test-nonexistent-path. If that path (or rollback.manifest under it) exists from a prior run, this test can become flaky. Prefer using tempfile::tempdir() (or a unique random subdir) and point UPDATE_STORAGE_PATH there to guarantee isolation.
| std::env::set_var("UPDATE_STORAGE_PATH", "/tmp/status-test-nonexistent-path"); | |
| let manifest = load_manifest().await.unwrap(); | |
| assert!(manifest.entries.is_empty()); | |
| let dir = tempfile::tempdir().unwrap(); | |
| std::env::set_var("UPDATE_STORAGE_PATH", dir.path().to_str().unwrap()); | |
| let manifest = load_manifest().await.unwrap(); | |
| assert!(manifest.entries.is_empty()); |
| #[tokio::test] | ||
| async fn save_and_load_manifest_roundtrip() { | ||
| let _guard = env_lock().lock().expect("env lock poisoned"); | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| std::env::set_var("UPDATE_STORAGE_PATH", dir.path().to_str().unwrap()); | ||
|
|
||
| let manifest = RollbackManifest { | ||
| entries: vec![RollbackEntry { | ||
| job_id: "test-job".to_string(), | ||
| backup_path: "/backup/test.bak".to_string(), | ||
| install_path: "/usr/bin/status".to_string(), | ||
| timestamp: Utc::now(), | ||
| }], | ||
| }; | ||
| save_manifest(&manifest).await.unwrap(); | ||
|
|
||
| let loaded = load_manifest().await.unwrap(); | ||
| assert_eq!(loaded.entries.len(), 1); | ||
| assert_eq!(loaded.entries[0].job_id, "test-job"); | ||
|
|
||
| std::env::remove_var("UPDATE_STORAGE_PATH"); | ||
| } |
There was a problem hiding this comment.
These tests set/remove UPDATE_STORAGE_PATH but don’t restore any pre-existing value, which can leak state into other tests (or affect local runs when the env var is set). Save the original value before mutation and restore it in a Drop guard so cleanup happens even if an assertion fails.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: vsilent <42473+vsilent@users.noreply.github.com> Agent-Logs-Url: https://github.com/trydirect/status/sessions/8a87702e-2218-4345-a395-0c5ee430b4c9
Fix env var state leakage in tests via Drop-based EnvGuard
|
@copilot test failed. Apply cargo fmt all and cargo clippy |
Co-authored-by: vsilent <42473+vsilent@users.noreply.github.com> Agent-Logs-Url: https://github.com/trydirect/status/sessions/c1f9ef47-5fc7-4ac8-bb43-5fbb46bfe5e2
Fixed in commit 8910642. Applied |
EnvGuarddrop-based restore helper tosrc/test_utils.rstest_utilsas a#[cfg(test)]module insrc/lib.rssrc/security/scopes.rstests to import and useEnvGuardsrc/commands/version_check.rstests to import and useEnvGuardsrc/commands/deploy.rstests to import and useEnvGuardload_manifest_nonexistent_returns_defaultto usetempfile::tempdir()instead of hard-coded/tmppathcargo fmt --allto fix formatting issues insrc/commands/deploy.rs,src/monitoring/mod.rs,src/security/request_signer.rscargo clippy -- -D warningspasses with no warnings✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.