[rhuss]

Summary

Fixes #12. Provider profiles and sandbox policies share the proto endpoint message, but the profile converter mapped only host, port, and protocol. Profiles with access, enforcement, rules, or credential settings couldn't be imported, and Profiles().Get followed by Update stripped those fields.

Changes

• types.NetworkEndpoint is now an alias of types.PolicyNetworkEndpoint, and NetworkEndpointFromProto/ToProto reuse the policy endpoint conversion, so every field is mapped in both directions.
• NetworkCredentialBinding and McpOptions are re-exported in openshell/v1; PolicyNetworkEndpoint uses both.
• Round-trip tests for a single endpoint (all fields) and for a profile with an access preset.

Compatibility

Keyed literals such as v1.NetworkEndpoint{Host: "api.example.com", Port: 443, Protocol: "rest"} keep compiling. The type is no longer comparable with ==, because the full endpoint contains slices; nothing in the SDK compares endpoints that way.

Testing

• mise run ci passes.
• The new tests don't compile against main: the old NetworkEndpoint has no Access, AllowUninspectedCredentials, or CredentialBinding.

The E2E suite in #6 still uses endpointless fixture profiles and skips TestAttachDetachProviderUpdatesCredentials; once this lands, a follow-up there gives the fixtures real endpoints and re-enables the test.

[rhuss]

cc-review Summary

What Went Well

• openshell/v1/internal/converter/profile.go NetworkEndpointFromProto/NetworkEndpointToProto (lines 88-103) now delegate to the shared policy endpoint converters instead of keeping a second hand-written field list. Profile and policy endpoints can't drift apart again, and the nil guard stays at the public entry point.
• openshell/v1/types/profile.go:146 uses a type alias (type NetworkEndpoint = PolicyNetworkEndpoint), so existing keyed literals with Host, Port and Protocol keep compiling and profile endpoints can be assigned straight into policy rules.
• openshell/v1/internal/converter/network_policy.go policyNetworkEndpointFromProto/ToProto copy Ports, AllowedIps, Rules, DenyRules, Mcp and CredentialBinding into fresh values, so the richer profile endpoints never alias proto-owned memory.
• openshell/v1/internal/converter/profile_test.go TestNetworkEndpointRoundTripKeepsAllFields (line 759) compares the whole round trip with proto.Equal(in, out) rather than spot-checking fields, and calls require.NotNil on CredentialBinding before reading it, so a broken mapping fails cleanly instead of panicking.
• openshell/v1/internal/converter/profile_test.go TestProviderProfileRoundTripKeepsEndpointAccess goes through the public ProviderProfileFromProto/ToProto path, which matches the lossy Get then Update scenario from #12.
• openshell/v1/types_reexport.go (lines 18-22) re-exports NetworkCredentialBinding and McpOptions, so every type reachable from PolicyNetworkEndpoint can be used from openshell/v1.
• openshell/v1/types/network_policy.go (lines 56-58) drops the now-false "distinct from NetworkEndpoint" wording, and coverage_test.go keeps the gateway-derived token_grant_owner explicitly skipped.

Findings

Severity	File	Description	Source
Notable	openshell/v1/internal/converter/profile.go:85	PR text and doc comment claim every field is mapped, but token_grant_owner is dropped	goal-alignment
Minor	openshell/v1/internal/converter/profile.go:87	Doc comments overstate the contract: shared helpers skip token_grant_owner on purpose	architecture
Minor	openshell/v1/internal/converter/profile_test.go:759	Round-trip test leaves 14 mapped fields at zero, so proto.Equal can't catch them being dropped	test-quality
Notable	openshell/v1/types/profile.go:146	Endpoints are unified via alias, binaries still use two separate SDK types	architecture
Minor	openshell/v1/types/profile.go:146	Alias change is source-breaking (no longer comparable); worth a release note	gator

All findings are posted as inline comments.

Review Details

• Mode: initial review (no earlier review conversations on this PR)
• Findings posted: 5
• Findings reviewed and not posted: 0
• Gate outcome: PASS (no Critical or Important findings)
• Participating reviewers: Architecture & Idioms, Goal Alignment, Test Quality, Gator (Principal Engineer)
• Also ran, no findings: Correctness, Production Readiness, Security, Codex
• Limitations: the CodeRabbit reviewer failed (exit 1), so coverage for this run is incomplete and it isn't recorded as a completed review baseline.

[rhuss]

Notable: The PR description and this new doc comment say every endpoint field is mapped in both directions. That isn't quite the case: proto field 27, token_grant_owner (sbv1.NetworkEndpoint.TokenGrantOwner), has no counterpart in PolicyNetworkEndpoint, so policyNetworkEndpointFromProto and policyNetworkEndpointToProto drop it.

Why this matters: A profile returned by Profiles().Get whose endpoint has token_grant_owner set comes back without it, and TestNetworkEndpointRoundTripKeepsAllFields never sets that field, so the "keeps all fields" claim isn't checked for it. The practical impact is low. The proto marks the field as gateway-derived and ignores user-authored values, so the goal of #12 (Get then Update no longer strips access, rules, enforcement or credential settings) is still met. What's off is the stated contract.

Suggested fix: Reword the comments and the PR text to say every user-authored field is mapped, and name TokenGrantOwner as intentionally left out because the gateway owns it. If read-back visibility is wanted, add the field to PolicyNetworkEndpoint and both converters instead. The comment on line 87 covers the same wording from the code side.

Source: goal-alignment agent

[rhuss]

Minor: The doc comments on NetworkEndpointFromProto (lines 85-87) and NetworkEndpointToProto (lines 96-97) say every field is mapped, but the shared policyNetworkEndpointFromProto/ToProto helpers skip proto field 27, token_grant_owner, on purpose. coverage_test.go (around lines 260-263) lists it as intentionally skipped because the gateway derives it.

Why this matters: The comments overstate the contract, and the new test comment at profile_test.go:756-758 ("every other field must survive both directions") repeats the claim. The round-trip test never sets TokenGrantOwner, so it can't catch the gap, and the next reader may assume a lossless conversion that doesn't exist.

Suggested fix: Reword both comments to something like "maps every authored field; the gateway-derived token_grant_owner is omitted (see coverage_test.go)", and adjust the test comment at profile_test.go:756-758 the same way.

Source: architecture agent

[rhuss]

Minor: TestNetworkEndpointRoundTripKeepsAllFields says every field must survive the round trip, but the input (lines 760-773) only sets Host, Port, Ports, Protocol, Tls, Enforcement, Access, AllowedIps, Path, AllowUninspectedCredentials, ProviderCredentialed and CredentialBinding. Fourteen mapped fields stay at their zero values: Rules, DenyRules, GraphqlPersistedQueries, Mcp, AllowEncodedSlash, PersistedQueries, GraphqlMaxBodyBytes, WebsocketCredentialRewrite, RequestBodyCredentialRewrite, AdvisorProposed, CredentialSigning, SigningService, SigningRegion and JsonRpcMaxBodyBytes.

Why this matters: If NetworkEndpointFromProto/ToProto ever stop delegating to the policy converters (say, a hand-written mapping like the one this PR removes that skips Rules, Mcp or SigningRegion), both sides of proto.Equal(in, out) still hold zero values for those fields and the test keeps passing. That's exactly the #12 regression the test comment says it guards against. TestNetworkPolicyRuleRoundTrip in network_policy_test.go covers these fields only through NetworkPolicyRuleFromProto, not through the profile entry points.

Suggested fix: Give every proto field a non-zero value in the input, including at least one L7Rule, one L7DenyRule, one GraphqlPersistedQueries entry and an McpOptions with StrictToolNames set. Reusing the fully populated endpoint fixture from network_policy_test.go would also work. Either way, the existing proto.Equal assertion would then cover all fields.

Source: test-quality agent

[rhuss]

Notable: Endpoints are now unified (NetworkEndpoint = PolicyNetworkEndpoint) because profiles and policies share sbv1.NetworkEndpoint. Binaries also share one proto message (sbv1.NetworkBinary), yet they keep two separate SDK types, NetworkBinary and PolicyNetworkBinary, so the type model ends up half unified.

Why this matters: This is informational, not a defect. Binaries lose no data, since Path is their only field and both converters map it. Still, a caller copying profile endpoints and binaries into a NetworkPolicyRule can now assign Endpoints directly but has to convert each binary by hand. The comment at types/network_policy.go:98 also still calls NetworkBinary the "simplified profile-level binary", though nothing about it is simplified.

Suggested fix: If the shared-message reasoning in this PR is the design rule, apply it to binaries too (type NetworkBinary = PolicyNetworkBinary) in a follow-up. Otherwise, document why the two cases differ and update the comment at network_policy.go:98. This can wait for a separate change.

Source: architecture agent

[rhuss]

Minor: Turning NetworkEndpoint from a three-field struct into an alias of PolicyNetworkEndpoint is a source-breaking change to the public API. The aliased struct holds slices, maps and pointers (Ports, Rules, GraphqlPersistedQueries, Mcp, CredentialBinding), so a == b, map[v1.NetworkEndpoint]T and unkeyed literals such as v1.NetworkEndpoint{"h", 443, "rest"} no longer compile. A type switch with both case v1.NetworkEndpoint and case v1.PolicyNetworkEndpoint now fails with a duplicate case.

Why this matters: Downstream builds break on upgrade, and nothing tells users why. For example, var a, b v1.NetworkEndpoint; _ = a == b compiles against base 7cc0b94 and fails against e12ab43 with invalid operation: a == b (struct containing []uint32 cannot be compared). The module is still pre-1.0 (v0.3.x tags), so Go semver allows the break, and nothing in-tree is affected (go vet ./... and go test ./openshell/v1/... pass). It just deserves a mention.

Suggested fix: Keep the alias, since it's the right fix for the data loss in #12, and add a breaking-change note to the PR description or release notes. Something like: "types.NetworkEndpoint is now an alias of PolicyNetworkEndpoint; it is no longer comparable, so use reflect.DeepEqual or compare fields."

Source: gator agent