feat: add pf port command preparation#252
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements non-privileged macOS PF preparation plus three CLI commands ( Changespf port command implementation
Sequence Diagram — ports install (high-level)sequenceDiagram
participant CLI as ports::install
participant Env as Environment
participant DB as Database
participant FS as FileSystem
participant Inspector as inspect_pf_anchor_file
participant Stdout as stdout
CLI->>Env: loopback_tcp_listener_ports()
Env-->>CLI: occupied_ports_set
CLI->>DB: assign_gateway_ports(is_available)
DB-->>CLI: GatewayPortAssignments
CLI->>FS: write prepared anchor & pf.conf
CLI->>Inspector: inspect prepared and system PF files
Inspector-->>CLI: PfFileState results
CLI->>Stdout: print mapping and install guidance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/state/tests/state_foundation.rs (1)
26-38: ⚡ Quick winUse an
instasnapshot for this new path test.This case switches to direct string equality checks, while the surrounding tests snapshot derived path output. Keeping it in the local snapshot style makes the test easier to extend with future path additions.
As per coding guidelines, `**/*.rs`: PREFER `insta` snapshots following patterns in nearby tests over substring assertions.♻️ Suggested update
#[test] fn pv_paths_include_prepared_pf_artifacts() { let paths = PvPaths::for_home("/Users/alice"); - assert_eq!( - paths.pf_anchor_config().as_str(), - "/Users/alice/.pv/config/pf/com.prvious.pv" - ); - assert_eq!( - paths.pf_conf_reference_config().as_str(), - "/Users/alice/.pv/config/pf/pf.conf" - ); + assert_debug_snapshot!(( + paths.pf_anchor_config(), + paths.pf_conf_reference_config(), + )); }🤖 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 `@crates/state/tests/state_foundation.rs` around lines 26 - 38, Replace the direct string equality asserts in the test pv_paths_include_prepared_pf_artifacts with an insta snapshot assertion: call PvPaths::for_home("/Users/alice") and produce a stable representation (e.g., a Vec of the two path strings or Debug display of paths.pf_anchor_config() and paths.pf_conf_reference_config()) and use insta::assert_snapshot! or insta::assert_debug_snapshot! to capture it; update the test name if desired, and ensure the snapshot key is generated by insta so future additions to PvPaths are captured by the snapshot comparison instead of inline assert_eq checks.crates/cli/tests/ports.rs (1)
342-349: ⚡ Quick winPrefer snapshots over the substring helper here.
assert_no_privileged_guidancere-encodes output expectations that the surroundinginstasnapshots already cover. Folding this into the snapshots would match the existing Rust test style and remove a second assertion path to maintain.As per coding guidelines,
**/*.rs: PREFERinstasnapshots following patterns in nearby tests over substring assertions.🤖 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 `@crates/cli/tests/ports.rs` around lines 342 - 349, The helper function assert_no_privileged_guidance is redundant with the existing insta snapshots; remove or stop calling assert_no_privileged_guidance and instead extend the relevant insta snapshot(s) to include the expectations about not containing privileged guidance (e.g., ensure the captured output snapshot for the test covering ports does not include "sudo", "pfctl", "sudo rm", or "sudo install"). Locate usages of assert_no_privileged_guidance in the ports tests and replace them by asserting/recording the full output with insta (e.g., assert_snapshot!/assert_display_snapshot!) so the absence of those substrings is validated via the snapshot.
🤖 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 `@crates/macos/src/lib.rs`:
- Around line 267-281: The current anchor detection (has_anchor_name) is too
loose and matches comments or unrelated text; update it to only consider active,
non-comment pf.conf directives by trimming lines, ignoring lines that start with
'#' and matching exact directive patterns such as anchor "com.prvious.pv" or
load anchor "com.prvious.pv" from "/etc/pf.anchors/com.prvious.pv" (or their
unquoted variants) instead of simple substring contains. Adjust the logic that
computes has_anchor_name (and keep PV_MARKER and the
PfFileState::Conflict/PfFileState::Missing branches) so it only returns Conflict
when a real anchor/load directive is present in a non-comment line.
- Around line 288-303: loopback_tcp_listener_ports currently only requests
netstat::AddressFamilyFlags::IPV4 and filters IpAddr::V4, so IPv6 loopback
listeners (::1 or dual-stack) are missed; update the function to query both IPV4
and IPV6 (or IPV6 only) and include a branch that matches
netstat::ProtocolSocketInfo::Tcp(tcp) where tcp.local_addr is IpAddr::V6(addr)
and addr.is_loopback() to insert tcp.local_port. Also tighten
inspect_pf_conf_reference PfFileState::Conflict detection by replacing fragile
substring checks of PV_MARKER (e.g., line.contains("com.prvious.pv")) with exact
directive parsing—call PfConfReference::parse_block(&content) or perform exact
anchor/directive matching to determine presence of the PV anchor marker so
commented or unrelated lines are not treated as conflicts.
---
Nitpick comments:
In `@crates/cli/tests/ports.rs`:
- Around line 342-349: The helper function assert_no_privileged_guidance is
redundant with the existing insta snapshots; remove or stop calling
assert_no_privileged_guidance and instead extend the relevant insta snapshot(s)
to include the expectations about not containing privileged guidance (e.g.,
ensure the captured output snapshot for the test covering ports does not include
"sudo", "pfctl", "sudo rm", or "sudo install"). Locate usages of
assert_no_privileged_guidance in the ports tests and replace them by
asserting/recording the full output with insta (e.g.,
assert_snapshot!/assert_display_snapshot!) so the absence of those substrings is
validated via the snapshot.
In `@crates/state/tests/state_foundation.rs`:
- Around line 26-38: Replace the direct string equality asserts in the test
pv_paths_include_prepared_pf_artifacts with an insta snapshot assertion: call
PvPaths::for_home("/Users/alice") and produce a stable representation (e.g., a
Vec of the two path strings or Debug display of paths.pf_anchor_config() and
paths.pf_conf_reference_config()) and use insta::assert_snapshot! or
insta::assert_debug_snapshot! to capture it; update the test name if desired,
and ensure the snapshot key is generated by insta so future additions to PvPaths
are captured by the snapshot comparison instead of inline assert_eq checks.
🪄 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 Plus
Run ID: 41a884ed-cc95-4913-8e23-09855ac2169d
⛔ Files ignored due to path filters (13)
Cargo.lockis excluded by!**/*.lockcrates/cli/tests/snapshots/ports__ports_install_fails_on_low_port_conflict_before_writing_prepared_artifacts.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/ports__ports_install_prepares_pf_artifacts_without_touching_system_paths.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/ports__ports_status_reports_prepared_and_system_pf_states_without_mutating_state.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/ports__ports_uninstall_removes_prepared_artifacts_and_defers_system_removal.snapis excluded by!**/*.snapcrates/macos/tests/snapshots/resolver_config__pf_anchor_inspection_reports_missing_current_stale_conflict_and_unreadable.snapis excluded by!**/*.snapcrates/macos/tests/snapshots/resolver_config__pf_conf_reference_inspection_reports_missing_current_stale_conflict_and_unreadable.snapis excluded by!**/*.snapcrates/macos/tests/snapshots/resolver_config__pf_config_renders_pv_owned_anchor_and_pf_conf_reference.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__gateway_port_allocator_persists_distinct_http_and_https_assignments.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_bash_script.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_fish_script.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_zsh_script.snapis excluded by!**/*.snapit/snapshots/cli__daemon_run_is_hidden_from_top_level_help.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
Cargo.tomlIMPLEMENTATION.mdcrates/cli/src/args.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/ports.rscrates/cli/src/environment.rscrates/cli/src/error.rscrates/cli/src/lib.rscrates/cli/tests/ports.rscrates/macos/Cargo.tomlcrates/macos/src/lib.rscrates/macos/tests/resolver_config.rscrates/state/src/database.rscrates/state/src/lib.rscrates/state/src/paths.rscrates/state/tests/state_foundation.rsdocs/superpowers/plans/2026-06-03-pr-11-pf-port-commands.mddocs/superpowers/specs/2026-06-03-pr-11-pf-port-commands-design.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/macos/tests/resolver_config.rs (1)
196-228: 💤 Low valueConsider snapshot pattern consistency.
These tests verify runtime listener detection using
assert!with membership checks, while all other tests in this file useassert_debug_snapshot!. The deviation appears justified because the ephemeral port assignment makes traditional snapshots impractical. However, given the file's established pattern, consider whether there's value in supplementing these runtime tests with snapshot-based tests for the underlying parsing logic (e.g., testingparse_netstat_tcp_listener_portsdirectly with fixed inputs).The current approach is sound for integration-level verification, but a hybrid approach might better align with the file's snapshot convention while preserving runtime coverage.
Based on learnings: PREFER
instasnapshots following patterns in nearby tests over substring assertions.🤖 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 `@crates/macos/tests/resolver_config.rs` around lines 196 - 228, The tests use runtime membership asserts for loopback_tcp_listener_ports() but the file prefers insta snapshots; add supplemental snapshot tests that exercise the underlying parsing logic (e.g., parse_netstat_tcp_listener_ports) with deterministic, fixed netstat output fixtures and assert_debug_snapshot! on the parsed port set, while keeping the existing integration-style runtime tests (loopback_tcp_listener_ports and its IPv6 variant) for ephemeral-port coverage; locate parse_netstat_tcp_listener_ports and write one or more unit tests that feed fixed input strings and call assert_debug_snapshot! on the resulting Vec/HashSet so the parsing behavior is snapshot-covered and consistent with nearby tests.
🤖 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.
Nitpick comments:
In `@crates/macos/tests/resolver_config.rs`:
- Around line 196-228: The tests use runtime membership asserts for
loopback_tcp_listener_ports() but the file prefers insta snapshots; add
supplemental snapshot tests that exercise the underlying parsing logic (e.g.,
parse_netstat_tcp_listener_ports) with deterministic, fixed netstat output
fixtures and assert_debug_snapshot! on the parsed port set, while keeping the
existing integration-style runtime tests (loopback_tcp_listener_ports and its
IPv6 variant) for ephemeral-port coverage; locate
parse_netstat_tcp_listener_ports and write one or more unit tests that feed
fixed input strings and call assert_debug_snapshot! on the resulting Vec/HashSet
so the parsing behavior is snapshot-covered and consistent with nearby tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bfb69be1-e319-47e4-8d99-a17c18c40c7b
⛔ Files ignored due to path filters (3)
crates/cli/tests/snapshots/ports__ports_install_uses_fallback_gateway_ports_when_preferred_ports_are_busy.snapis excluded by!**/*.snapcrates/macos/tests/snapshots/resolver_config__pf_anchor_inspection_reports_missing_current_stale_conflict_and_unreadable.snapis excluded by!**/*.snapcrates/macos/tests/snapshots/resolver_config__pf_conf_reference_inspection_reports_missing_current_stale_conflict_and_unreadable.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
crates/cli/tests/ports.rscrates/macos/src/lib.rscrates/macos/tests/resolver_config.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/macos/src/lib.rs
- crates/cli/tests/ports.rs
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pfanchor and pf.conf reference artifactspv ports:status,pv ports:install, andpv ports:uninstallScope
/etc/pf.confor/etc/pf.anchors/com.prvious.pvpfctlsudoTests
cargo nextest run --workspacecargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo shearcargo fmt --all -- --checkgit diff --checkSummary by CodeRabbit