refactor: consolidate duplicated policy YAML struct hierarchies in navigator-policy

[johntmyers]

Problem Statement

The sandbox policy has three separate Rust struct hierarchies representing the same YAML shape: Deserialize-only types in navigator-policy for parsing user-authored YAML, prost-generated types from sandbox.proto for wire/storage, and Serialize-only types in navigator-cli for ncl sandbox policy get --full output. These were written independently and have silently diverged — the input YAML uses filesystem_policy but the output YAML writes filesystem (matching the proto field name), breaking round-tripping. The output path also silently drops allowed_ips and the name field on network policy rules.

Technical Context

The proto layer in the middle is unavoidable (it serves as the wire/storage format), but the Input and Output struct hierarchies are mirrors of each other — one implements Deserialize, the other Serialize — with manual field-by-field conversion functions on each side. A single set of structs in navigator-policy that derive both Serialize and Deserialize would eliminate the duplication and make round-tripping correct by construction. The filesystem_policy naming also flows into the OPA/Rego layer (data.filesystem_policy in rego rules and proto_to_opa_data_json()), so the canonical YAML key needs to be preserved.

Affected Components

Component	Key Files	Role
navigator-policy	crates/navigator-policy/src/lib.rs	Input YAML structs (PolicyFile, FilesystemDef, etc.) and YAML→proto conversion
navigator-cli	crates/navigator-cli/src/run.rs	Output YAML structs (PolicyYaml, FilesystemYaml, etc.) and proto→YAML conversion
proto	proto/sandbox.proto	Wire format — field named filesystem (not filesystem_policy)
navigator-sandbox (OPA)	crates/navigator-sandbox/src/opa.rs	Manual proto→JSON with "filesystem_policy" key for Rego evaluation
Rego rules	dev-sandbox-policy.rego	References data.filesystem_policy

Proposed Approach

Consolidate the Input (Hierarchy A) and Output (Hierarchy C) structs into a single set of types in navigator-policy that derive both Serialize and Deserialize. Use #[serde(rename = "filesystem_policy")] (or keep the struct field named filesystem_policy) so the canonical YAML key is preserved. Move the proto→YAML conversion (policy_to_yaml) into navigator-policy alongside the existing YAML→proto conversion, so both directions live in one place. The CLI then just calls the shared serializer. The OPA JSON builder (proto_to_opa_data_json) is a separate concern and should not be changed in this work — it already uses the correct key name.

Scope Assessment

• Complexity: Medium
• Confidence: High — clear path, no design ambiguity
• Estimated files to change: 3-4 (navigator-policy/src/lib.rs, navigator-cli/src/run.rs, Cargo.toml if needed, tests)
• Issue type: refactor

Risks & Open Questions

• allowed_ips and name field data loss: The output structs silently drop these. The consolidated types must include all fields from both the input and proto definitions. Verify no other fields are missing.
• Stale checked-in generated proto code: crates/navigator-core/src/proto/navigator.sandbox.v1.rs is completely out of date (old schema with NetworkPolicy/NetworkMode that no longer exist). Not used at runtime but misleads developers. Consider deleting or regenerating as a follow-up.
• InferencePolicy.api_patterns: The proto has this field but it has no YAML representation in either direction today. The consolidated types should include it for completeness, but it can be #[serde(default, skip_serializing_if)] for now.
• BTreeMap vs HashMap: Output uses BTreeMap for deterministic ordering; input uses HashMap. The consolidated type should use BTreeMap for stable YAML output.
• deny_unknown_fields: The input types use #[serde(deny_unknown_fields)]. This is strict and good for input validation but means adding new optional fields requires a version bump or feature flag. Keep this behavior.

Test Considerations

• Round-trip test: parse dev-sandbox-policy.yaml → proto → YAML string → parse again → assert equality
• Ensure allowed_ips and name fields survive the round-trip
• Existing OPA tests in navigator-sandbox/src/opa.rs should continue passing (they use inline YAML with filesystem_policy key)
• CLI policy get --full output should be parseable by parse_sandbox_policy()

---

Created by spike investigation. Use build-from-issue to plan and implement.

[johntmyers]

🔬 spike-agent

Technical Investigation

Architecture Overview

The sandbox policy flows through four layers:

User YAML file
    ↓  serde_yaml::from_str (Deserialize)
Input structs (PolicyFile, FilesystemDef, ...)     [navigator-policy]
    ↓  convert_policy()
Proto structs (SandboxPolicy, FilesystemPolicy, ...)  [prost-generated from sandbox.proto]
    ↓  stored in gateway DB as protobuf bytes
    ↓  retrieved via gRPC GetSandboxPolicyStatus
    ↓  policy_to_yaml()
Output structs (PolicyYaml, FilesystemYaml, ...)   [navigator-cli]
    ↓  serde_yaml::to_string (Serialize)
YAML string (written to stdout or file)

The Input and Output struct hierarchies are structural mirrors — same fields, same nesting — but were written independently with no shared types. Each has its own manual conversion function mapping field-by-field to/from the proto types. This means any rename, addition, or removal in one hierarchy must be manually replicated in the other, with no compiler enforcement.

A fourth hierarchy exists in navigator-sandbox/src/policy.rs for sandbox-side runtime use. This one is intentionally different (uses PathBuf, Option<String>, Rust enums) and should NOT be merged — it's a semantic transformation, not a naming concern.

Code References

Location	Description
crates/navigator-policy/src/lib.rs:25-131	Input structs: PolicyFile, FilesystemDef, LandlockDef, ProcessDef, InferenceDef, NetworkPolicyRuleDef, NetworkEndpointDef, L7RuleDef, L7AllowDef, NetworkBinaryDef (10 structs, Deserialize-only)
crates/navigator-policy/src/lib.rs:137-207	convert_policy(): Input → Proto conversion, field-by-field
crates/navigator-cli/src/run.rs:1372-1457	Output structs: PolicyYaml, FilesystemYaml, LandlockYaml, ProcessYaml, InferenceYaml, NetworkPolicyRuleYaml, NetworkEndpointYaml, L7RuleYaml, L7AllowYaml, NetworkBinaryYaml (10 structs, Serialize-only)
crates/navigator-cli/src/run.rs:1460-1537	policy_to_yaml(): Proto → Output conversion, field-by-field
proto/sandbox.proto:9-127	Proto message definitions
crates/navigator-sandbox/src/opa.rs:552-663	proto_to_opa_data_json(): Proto → JSON for OPA engine (manually builds JSON, uses "filesystem_policy" key)
dev-sandbox-policy.rego:10	Rego rule: data.filesystem_policy
crates/navigator-sandbox/src/policy.rs:14-167	Runtime sandbox structs (Hierarchy D — intentionally different, not in scope)
crates/navigator-core/src/proto/navigator.sandbox.v1.rs	Stale checked-in generated code — old schema, not used at runtime

Current Behavior

Input path (ncl sandbox create --policy ./file.yaml):

1. load_sandbox_policy() reads YAML file
2. serde_yaml::from_str() deserializes into PolicyFile (field: filesystem_policy)
3. convert_policy() maps to proto SandboxPolicy (field: filesystem)
4. Proto is sent to gateway via gRPC, stored as protobuf bytes

Output path (ncl sandbox policy get test --full):

1. CLI calls GetSandboxPolicyStatus gRPC
2. Gateway decodes protobuf bytes into proto SandboxPolicy (field: filesystem)
3. policy_to_yaml() maps proto to PolicyYaml (field: filesystem — no rename attribute)
4. serde_yaml::to_string() serializes — outputs filesystem: not filesystem_policy:

Result: Output YAML cannot be fed back as input. Also, allowed_ips and the network policy name field are silently dropped.

Complete Field Name Mismatch Inventory

#	Field	Input (YAML key)	Proto field	Output (YAML key)	OPA JSON key	Status
1	filesystem_policy	filesystem_policy	filesystem	filesystem	filesystem_policy	BROKEN — round-trip fails
2	network_policies[].name	present	present	missing	present	DATA LOSS
3	endpoints[].allowed_ips	present	present	missing	present	DATA LOSS
4	inference.api_patterns	missing	present	missing	N/A	Consistent gap — no YAML representation
5	network_policies map type	HashMap	HashMap (prost)	BTreeMap	N/A	Benign — output is deterministically ordered

What Would Need to Change

Phase 1 — Consolidate Input + Output structs (this issue):

1. navigator-policy/src/lib.rs: Add Serialize derive to all existing input structs (PolicyFile → rename to Policy or SandboxPolicyYaml). Add missing fields (allowed_ips on NetworkEndpointDef's serialization equivalent). Switch HashMap to BTreeMap for network_policies. Export the types publicly.

2. navigator-policy/src/lib.rs: Add a new pub fn policy_to_yaml(policy: &SandboxPolicy) -> String function that converts proto → the shared types → YAML string. This is the inverse of the existing parse_sandbox_policy.

3. navigator-cli/src/run.rs: Delete the entire PolicyYaml hierarchy (lines 1372-1457) and policy_to_yaml() (lines 1460-1537). Replace with a call to navigator_policy::policy_to_yaml().

4. Tests: Add a round-trip test in navigator-policy that parses dev-sandbox-policy.yaml, converts to proto, converts back to YAML, parses again, and asserts structural equality.

Not in scope (follow-ups):

• Stale checked-in proto file cleanup
• proto_to_opa_data_json() in navigator-sandbox (already uses correct key names)
• Runtime sandbox policy structs (Hierarchy D — intentionally different)

Patterns to Follow

The existing code in navigator-policy already follows a clean pattern:

• Private serde structs with #[serde(deny_unknown_fields)] and #[serde(default)] on optional fields
• A conversion function that maps serde types ↔ proto types
• Public API returns proto types (callers don't see the serde structs)

The consolidation should preserve this pattern but make the serde structs pub (or expose them via public conversion functions) so the CLI can use them for output too.

Test Coverage Notes

• navigator-policy currently has zero tests — all testing is implicit via the include_str! of dev-sandbox-policy.yaml at compile time (panics if parsing fails)
• OPA tests in navigator-sandbox/src/opa.rs use inline YAML strings with filesystem_policy — these must keep passing
• The CLI has no tests for the --full YAML output
• A round-trip test would be the single most valuable addition

---

This investigation provides context for implementation. Next step: review the issue, refine if needed, then use build-from-issue to create a plan and implement.

[johntmyers]

🏗️ build-plan

Implementation Plan

Issue type: refactor
Complexity: Medium
Confidence: High — clear path, no design ambiguity

Summary

Consolidate the Deserialize-only input structs and Serialize-only output structs into a single set of types in navigator-policy that derive both Serialize and Deserialize, with bidirectional proto conversion. Delete the duplicate hierarchy from the CLI. Fixes the filesystem vs filesystem_policy round-trip bug and the silent data loss of allowed_ips and name fields.

Scope

• crates/navigator-policy/src/lib.rs: Add Serialize to existing structs, add missing fields (allowed_ips, api_patterns), switch HashMap → BTreeMap, add pub fn policy_to_yaml(&SandboxPolicy) -> Result<String> (proto → YAML), export types needed by CLI
• crates/navigator-cli/src/run.rs: Delete PolicyYaml hierarchy (~85 lines) and policy_to_yaml() function (~77 lines), replace with call to navigator_policy::policy_to_yaml()
• crates/navigator-policy/Cargo.toml: No new deps needed — already has serde and serde_yaml

Implementation Steps

1. Add Serialize to existing input structs in navigator-policy/src/lib.rs

   • Add #[derive(Serialize)] to PolicyFile, FilesystemDef, InferenceDef, LandlockDef, ProcessDef, NetworkPolicyRuleDef, NetworkEndpointDef, L7RuleDef, L7AllowDef, NetworkBinaryDef
   • Add #[serde(skip_serializing_if = "...")] to optional/empty fields so output YAML is clean
   • Switch network_policies from HashMap to BTreeMap for deterministic ordering
   • Add missing allowed_ips: Vec<String> serialization support (already on NetworkEndpointDef, just needs skip_serializing_if)
   • Add api_patterns field to InferenceDef with #[serde(default, skip_serializing_if = "Vec::is_empty")]
   • Ensure NetworkBinaryDef.harness has #[serde(skip_serializing)] so the deprecated field doesn't appear in output

2. Add pub fn policy_to_yaml(policy: &SandboxPolicy) -> Result<String> to navigator-policy

   • Converts proto SandboxPolicy → the shared serde structs → YAML string via serde_yaml::to_string
   • Inverse of existing convert_policy() (rename to to_proto() for symmetry)
   • Add pub fn from_proto(policy: &SandboxPolicy) -> PolicyFile for the proto → serde struct conversion

3. Delete duplicate hierarchy from navigator-cli/src/run.rs

   • Remove PolicyYaml, FilesystemYaml, LandlockYaml, ProcessYaml, InferenceYaml, NetworkPolicyRuleYaml, NetworkEndpointYaml, L7RuleYaml, L7AllowYaml, NetworkBinaryYaml (lines ~1372-1457)
   • Remove policy_to_yaml() function (lines ~1460-1537)
   • Replace usage in sandbox_policy_get with navigator_policy::policy_to_yaml(&policy)

4. Add round-trip test in navigator-policy

   • Parse dev-sandbox-policy.yaml → proto → YAML string → parse again → compare proto representations for equality
   • Test that allowed_ips survives the round-trip (add a policy with allowed_ips to test data)
   • Test that the name field on NetworkPolicyRule survives

Test Plan

• Unit tests: Round-trip test in navigator-policy/src/lib.rs — parse YAML → proto → YAML → proto → assert equality. Also test with allowed_ips and custom name fields.
• Integration: N/A — no cross-component interaction changes
• E2E: N/A — no e2e/ files modified, existing OPA tests in navigator-sandbox cover the YAML→proto→OPA path

Risks & Open Questions

• deny_unknown_fields on the serde structs means adding api_patterns (new to YAML) requires #[serde(default)] so existing YAML files without it still parse. Already planned.
• The harness field on NetworkBinaryDef is deprecated — needs #[serde(skip_serializing)] so it deserializes (backward compat) but doesn't appear in output.

Documentation Impact

• architecture/security-policy.md: No changes needed — the schema docs already use filesystem_policy which is the correct canonical name

---

Revision 1 — initial plan

[johntmyers]

🏗️ build-from-issue-agent

Implementation Complete

PR: #97

What was built

Consolidated the duplicate YAML struct hierarchies into a single set of types in navigator-policy that derive both Serialize and Deserialize. Deleted ~162 lines of duplicate code from the CLI. Fixed three round-trip bugs: filesystem_policy vs filesystem field name mismatch, silent allowed_ips data loss, and silent name field data loss.

Tests

• Unit: 5 tests added (round-trip default policy, canonical field name check, allowed_ips preservation, name preservation, api_patterns preservation)
• Integration: N/A
• E2E: N/A

Docs updated

• None needed

The issue will auto-close when the PR is merged.