test(host-agent): add property coverage for command validators#27
test(host-agent): add property coverage for command validators#27
Conversation
There was a problem hiding this comment.
Pull request overview
Adds property-based tests to strengthen coverage for host-agent input validators that serve as security boundaries for host operations, and tightens ZFS + Git validation rules to reject additional unsafe edge cases.
Changes:
- Added
proptestas ahost-agentdev-dependency (via workspace dep). - Tightened ZFS dataset validation (reject empty /
.path components) and cached ZFS regexes withOnceLock. - Tightened Git branch/ref validation for additional
git check-ref-format-style edge cases and added new property tests across ZFS/Git/systemd/PCT validators.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/host-agent/src/zfs/mod.rs | Cache regexes with OnceLock, tighten dataset validation, add proptest-based properties |
| crates/host-agent/src/adapters/systemd.rs | Add proptest-based properties for unit-name validator |
| crates/host-agent/src/adapters/pct.rs | Add proptest-based properties for VMID validator |
| crates/host-agent/src/adapters/git.rs | Tighten branch validator edge cases; add proptest-based properties |
| crates/host-agent/Cargo.toml | Add proptest as dev-dependency (workspace-sourced) |
| Cargo.lock | Lockfile updates for proptest |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn accepted_dataset_names_never_have_empty_or_relative_components(name in "[\\x00-\\x7f]{0,80}") { | ||
| if is_valid_dataset_name(&name) { | ||
| prop_assert!(!name.is_empty()); | ||
| prop_assert!(!name.starts_with('/')); | ||
| prop_assert!(!name.ends_with('/')); | ||
| prop_assert!(!name.contains('@')); | ||
| prop_assert!(!name.contains("..")); | ||
| for part in name.split('/') { | ||
| prop_assert!(!part.is_empty(), "accepted dataset had empty component: {name:?}"); | ||
| prop_assert_ne!(part, ".", "accepted dataset had relative component: {:?}", name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepted_snapshot_names_never_contain_dataset_separators(name in "[\\x00-\\x7f]{0,80}") { | ||
| if is_valid_snapshot_name(&name) { | ||
| prop_assert!(!name.is_empty()); | ||
| prop_assert!(!name.contains('/')); | ||
| prop_assert!(!name.contains('@')); | ||
| prop_assert!(!name.chars().any(char::is_whitespace)); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepted_dataset_or_snapshot_requires_valid_parts(name in "[\\x00-\\x7f]{0,120}") { | ||
| if is_valid_dataset_or_snapshot(&name) { | ||
| if let Some((dataset, snap)) = name.split_once('@') { | ||
| prop_assert!(is_valid_dataset_name(dataset)); | ||
| prop_assert!(is_valid_snapshot_name(snap)); | ||
| prop_assert!(!snap.contains('@')); | ||
| } else { | ||
| prop_assert!(is_valid_dataset_name(&name)); | ||
| } |
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. The host-agent validator properties now generate accepted datasets/snapshots/units/VMIDs/branches directly and assert invariants on every generated case.
| #[test] | ||
| fn accepted_dataset_or_snapshot_requires_valid_parts(name in "[\\x00-\\x7f]{0,120}") { | ||
| if is_valid_dataset_or_snapshot(&name) { | ||
| if let Some((dataset, snap)) = name.split_once('@') { | ||
| prop_assert!(is_valid_dataset_name(dataset)); | ||
| prop_assert!(is_valid_snapshot_name(snap)); | ||
| prop_assert!(!snap.contains('@')); | ||
| } else { | ||
| prop_assert!(is_valid_dataset_name(&name)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. The host-agent validator properties now generate accepted datasets/snapshots/units/VMIDs/branches directly and assert invariants on every generated case.
| proptest! { | ||
| #[test] | ||
| fn accepted_unit_names_stay_single_token_and_known_suffix(name in "[\\x00-\\x7f]{0,100}") { | ||
| if is_valid_service_name(&name) { | ||
| prop_assert!(!name.is_empty()); | ||
| prop_assert!(!name.starts_with('.')); | ||
| prop_assert!(!name.starts_with('/')); | ||
| prop_assert!(!name.contains('/')); | ||
| prop_assert!(!name.chars().any(char::is_whitespace)); | ||
| let has_known_suffix = ["service", "socket", "timer", "target", "mount", "path"] | ||
| .iter() | ||
| .any(|suffix| name.ends_with(&format!(".{}", suffix))); | ||
| prop_assert!(has_known_suffix); | ||
| } |
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. The host-agent validator properties now generate accepted datasets/snapshots/units/VMIDs/branches directly and assert invariants on every generated case.
| fn accepted_vmids_are_plain_decimal_and_in_proxmox_range(id in "[\\x00-\\x7f]{0,32}") { | ||
| if is_valid_vmid(&id) { | ||
| prop_assert!(id.chars().all(|c| c.is_ascii_digit())); | ||
| let parsed = id.parse::<u32>().expect("valid vmid parses as u32"); | ||
| prop_assert!((100..=999999).contains(&parsed)); | ||
| } |
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. The host-agent validator properties now generate accepted datasets/snapshots/units/VMIDs/branches directly and assert invariants on every generated case.
| fn accepted_branch_names_preserve_git_ref_safety_invariants(name in "[\\x00-\\x7f]{0,100}") { | ||
| if is_valid_branch_name(&name) { | ||
| prop_assert!(!name.is_empty()); | ||
| prop_assert_ne!(name.as_str(), "@"); | ||
| prop_assert!(!name.starts_with('-')); | ||
| prop_assert!(!name.starts_with('/')); | ||
| prop_assert!(!name.ends_with('/')); | ||
| prop_assert!(!name.ends_with('.')); | ||
| prop_assert!(!name.contains("..")); | ||
| prop_assert!(!name.contains("//")); | ||
| let contains_at_brace = name.contains("@{"); | ||
| prop_assert!(!contains_at_brace); | ||
| prop_assert!(!name.chars().any(char::is_whitespace)); | ||
| for part in name.split('/') { | ||
| prop_assert!(!part.is_empty(), "accepted branch had empty component: {name:?}"); | ||
| prop_assert!(!part.starts_with('.'), "accepted branch had hidden component: {name:?}"); | ||
| prop_assert!(!part.ends_with(".lock"), "accepted branch had .lock component: {name:?}"); | ||
| } |
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. The host-agent validator properties now generate accepted datasets/snapshots/units/VMIDs/branches directly and assert invariants on every generated case.
| fn accepted_dataset_names_never_have_empty_or_relative_components(name in "[\\x00-\\x7f]{0,80}") { | ||
| if is_valid_dataset_name(&name) { | ||
| prop_assert!(!name.is_empty()); | ||
| prop_assert!(!name.starts_with('/')); | ||
| prop_assert!(!name.ends_with('/')); | ||
| prop_assert!(!name.contains('@')); | ||
| prop_assert!(!name.contains("..")); | ||
| for part in name.split('/') { | ||
| prop_assert!(!part.is_empty(), "accepted dataset had empty component: {name:?}"); | ||
| prop_assert_ne!(part, ".", "accepted dataset had relative component: {:?}", name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepted_snapshot_names_never_contain_dataset_separators(name in "[\\x00-\\x7f]{0,80}") { | ||
| if is_valid_snapshot_name(&name) { | ||
| prop_assert!(!name.is_empty()); | ||
| prop_assert!(!name.contains('/')); | ||
| prop_assert!(!name.contains('@')); | ||
| prop_assert!(!name.chars().any(char::is_whitespace)); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepted_dataset_or_snapshot_requires_valid_parts(name in "[\\x00-\\x7f]{0,120}") { | ||
| if is_valid_dataset_or_snapshot(&name) { | ||
| if let Some((dataset, snap)) = name.split_once('@') { | ||
| prop_assert!(is_valid_dataset_name(dataset)); | ||
| prop_assert!(is_valid_snapshot_name(snap)); | ||
| prop_assert!(!snap.contains('@')); | ||
| } else { | ||
| prop_assert!(is_valid_dataset_name(&name)); | ||
| } |
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. The host-agent validator properties now generate accepted datasets/snapshots/units/VMIDs/branches directly and assert invariants on every generated case.
|
Acknowledged from triage — host-agent property tests are useful coverage that complement the test-quality audit in #37. No conflicts with the security-proxy / fnox / MCP work happening in parallel; these are independent test files. |
Codex agent's contribution. Adds proptest coverage for is_valid_branch_name (git), is_valid_vmid (pct), is_valid_service_name (systemd) AND tightens the validators themselves to handle git ref-format edge cases (//, /., .lock suffix, trailing dot). 89 host-agent tests pass locally; CI green.
Summary
Adds property-based coverage for host-agent command validators that act as security boundaries around host operations.
proptestto host-agent dev-dependencies.tank//mediaandtank/./media.git check-ref-format-style edge cases://, leading/, trailing., hidden components, and.lockcomponents.OnceLockso property tests and hot-path validation do not recompile regexes per call.Tests
cargo test -p host-agentcargo clippy -p host-agent --all-targets -- -D warningsCoordination
This is an independent Codex-owned branch from
main; it does not touch the active secret-command/fnox PR branches.