feat(minimald-rpc,minimald,minimal2): policy types, GetSessionPolicy RPC, and session policy CLI - #521
Conversation
…RPC, and session policy CLI Add EgressPolicy, IngressPolicy, PortMapping structs to minimald-rpc (all #[non_exhaustive]) and IpProto re-export; add GetSessionPolicy and DynamicPortMap RPCs. Wire a stub GetSessionPolicy handler into minimald that returns default empty policy for any found session. Add `minimal session policy <id>` subcommand that calls the RPC and prints structured JSON. Closes #498
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds networking policy data types ( ChangesSession Policy RPC and CLI
Sequence Diagram(s)sequenceDiagram
participant User
participant minimal2 CLI
participant minvmd Daemon
participant serve_get_session_policy
User->>minimal2 CLI: session policy <id-or-name>
minimal2 CLI->>minvmd Daemon: ensure running, connect via SSH socket
minimal2 CLI->>minvmd Daemon: GetSessionPolicy RPC (Id or Name variant)
minvmd Daemon->>serve_get_session_policy: dispatch handler
serve_get_session_policy->>serve_get_session_policy: lookup session by id/name
alt session found
serve_get_session_policy-->>minvmd Daemon: Errorable::Ok(SessionPolicy)
minvmd Daemon-->>minimal2 CLI: SessionPolicy payload
minimal2 CLI-->>User: print JSON (egress/ingress)
else not found
serve_get_session_policy-->>minvmd Daemon: Errorable::Err("no session found")
minvmd Daemon-->>minimal2 CLI: error response
minimal2 CLI-->>User: Err(())
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
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 `@crates/minimal2/src/main.rs`:
- Around line 416-425: The current code uses ok() on the response match which
discards error details from the server, and then reports all None cases as "No
session found" even when the actual failure is from the daemon. Replace the ok()
call with a direct pattern match on the resp variable to handle both success
cases (when policy data exists) and error cases (Errorable::Err variants),
preserving and reporting the actual server error message when it occurs, rather
than defaulting to the "No session found" message for all failures.
🪄 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: b0ae62ca-6f0b-4d15-9a29-eb21b0c02500
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/minimal2/src/main.rscrates/minimald-rpc/Cargo.tomlcrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rs
|
Revise claim for head 21ab668. |
|
Auto-revise 1 of 3. |
| /// The effective networking policy for a named session, as returned by | ||
| /// [`GetSessionPolicy`]. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| pub struct SessionPolicy { |
There was a problem hiding this comment.
LOW · correctness
SessionPolicy is a public struct that will gain fields as the networking stack is built out (effective mode, assigned IP, etc.), but it is missing #[non_exhaustive]. The Rust coding standards require #[non_exhaustive] on public structs that may grow; the three companion types in this file (PortMapping, EgressPolicy, IngressPolicy) all carry it already.
#[non_exhaustive]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct SessionPolicy {|
|
||
| /// Request for the [`DynamicPortMap`] RPC. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct DynamicPortMapRequest { |
There was a problem hiding this comment.
LOW · correctness
DynamicPortMapRequest is a public struct missing #[non_exhaustive]. The Rust coding standards require #[non_exhaustive] on public structs that may grow — R2.4's description of the dynamic port-mapping API is still evolving (Open Questions item 3 in the spec), so additional fields are plausible.
#[non_exhaustive]
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct DynamicPortMapRequest {| } | ||
|
|
||
| /// Response for the [`DynamicPortMap`] RPC. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] |
There was a problem hiding this comment.
LOW · correctness
DynamicPortMapResponse is a public struct (currently a unit struct) missing #[non_exhaustive]. The Rust coding standards require #[non_exhaustive] on public structs that may grow. Even though it is currently empty, it is likely to gain fields (e.g., the assigned external port) when the handler is wired to live state in U2-T2.
#[non_exhaustive]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct DynamicPortMapResponse;
This comment has been minimized.
This comment has been minimized.
Replace resp.ok() with direct Errorable pattern match so the actual server error text is surfaced instead of the generic "No session found" message for all non-Ok outcomes.
|
Commit pushed:
|
sdd-validate — Implementation boundaryBoundary resolved: Implementation (all changed files are non-spec, non-architecture, non-spike source files) Gate 1 — Proof artifacts re-executed and passingProof artifact 1 — Test:
Infrastructure limit: the gh-aw validation container has no Rust toolchain and no access to crates.io. Note: both commit-status and check-runs endpoints returned Proof artifact 2 — CLI:
The CLI proof artifact requires a running Governing rule: a proof artifact blocked by an infrastructure limit that no consumer required status check covers is a Blocker — the proof is satisfied by no gate. A live CLI invocation against a running daemon is outside the scope of Transitive evidence (informational — does not clear the Blocker):
To resolve (either path):
Gate 2 — Changed files within task scopeTask scope from #498
No changes to protected paths ( Gate 3 — No real credentials in the diffClean. ✓ Summary
Blocker present →
|
|
|
||
| /// The effective networking policy for a named session, as returned by | ||
| /// [`GetSessionPolicy`]. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] |
There was a problem hiding this comment.
MEDIUM — correctness
SessionPolicy is missing #[non_exhaustive] while all three sibling types added in this same PR — PortMapping (line 285), EgressPolicy (line 300), and IngressPolicy (line 314) — carry it. The workspace coding standard (docs/rust-coding-standards.md) requires #[non_exhaustive] on public enums/structs that may grow.
SessionPolicy is a public RPC response type that is almost certain to grow as the networking stack matures (e.g. a network_mode field, DNS policy, or firewall zone could be added in Units 3–4). Without #[non_exhaustive], adding any field is a semver-breaking change to minimald-rpc.
Suggested fix: add #[non_exhaustive] on the line immediately above this #[derive]:
#[non_exhaustive]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct SessionPolicy {| pub struct DynamicPortMap; | ||
|
|
||
| /// Request for the [`DynamicPortMap`] RPC. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] |
There was a problem hiding this comment.
LOW — correctness
DynamicPortMapRequest is missing #[non_exhaustive]. Per R2.4, the daemon must validate the requested port against dynamic_allowed_ports and reject out-of-range requests with a typed error — the struct is likely to gain additional context fields (e.g. a ttl or protocol_hint) as the full R2.4 implementation lands.
For consistency with the other new structs in this PR that correctly carry #[non_exhaustive], add the attribute here too:
#[non_exhaustive]
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct DynamicPortMapRequest {|
Commit pushed:
|
|
Addressed the three Changes:
To allow external crates to keep constructing Verification:
|
…PortMap RPC types Add missing `#[non_exhaustive]` to the three new public structs so they can gain fields as the networking stack evolves without breaking external callers. The three companion types already carry it. Add `SessionPolicy::new` and `DynamicPortMapRequest::new` constructors so external crates are not blocked from constructing these types after the attribute is applied. Update `minimald`'s stub handler to use the constructor.
8671a98 to
33e5144
Compare
Closes #498
Summary
minimald-rpc: AddEgressPolicy,IngressPolicy,PortMappingstructs (all#[non_exhaustive]), re-exportIpProtofrom sessions. AddGetSessionPolicyRPC (R2.6 read-only policy RPC) andDynamicPortMapRPC (R2.4 runtime port-mapping).minimald: Wire a stubserve_get_session_policyhandler — looks up the session, returns{egress: null, ingress: {port_mappings: [], dynamic_allowed_range: null}}for any found session. Live state wired in U2-T2.minimal2: Addminimal session policy <id>subcommand that callsGetSessionPolicyand prints structured JSON to stdout.Proof artifacts
Test —
cargo test -p minimald-rpc(1 test passed):Verifies
EgressPolicy,IngressPolicy,PortMapping,IpProtoare present andSessionPolicyserializes to the expected JSON shape including"egress":null,"port_mappings":[],"dynamic_allowed_range":null.Test —
cargo test -p minimald(38 tests passed, 0 failed):All existing RPC round-trip tests continue to pass with the new handler wired in.
Build —
cargo build -p minimal2: Succeeded. Theminimal session policysubcommand compiles, parses the session identifier (UUID or name), callsGetSessionPolicy, and prints the JSON response.Next step
Merging this PR closes #498. Once every task sub-issue of the tracking issue #478 is closed, the pipeline advances to
sdd:donefor final human review.Summary by CodeRabbit
null/empty outputs.