Skip to content

fix(profiles): keep every endpoint field in provider profiles #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rhuss wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(profiles): keep every endpoint field in provider profiles #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions openshell/v1/internal/converter/profile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,29 +82,24 @@ func CredentialTokenGrantTypeToProto(t types.CredentialTokenGrantType) pb.Provid

// --- NetworkEndpoint ---

// NetworkEndpointFromProto converts a proto NetworkEndpoint to an SDK NetworkEndpoint.
// Only Host, Port, and Protocol are mapped; additional proto fields are ignored.
// NetworkEndpointFromProto converts a proto NetworkEndpoint to an SDK

@rhuss rhuss Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// NetworkEndpoint. Profile endpoints carry the same fields as policy
// endpoints, so every field is mapped.

@rhuss rhuss Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

func NetworkEndpointFromProto(ep *sbv1.NetworkEndpoint) *types.NetworkEndpoint {
if ep == nil {
return nil
}
return &types.NetworkEndpoint{
Host: ep.GetHost(),
Port: ep.GetPort(),
Protocol: ep.GetProtocol(),
}
result := policyNetworkEndpointFromProto(ep)
return &result
}

// NetworkEndpointToProto converts an SDK NetworkEndpoint to a proto NetworkEndpoint.
// NetworkEndpointToProto converts an SDK NetworkEndpoint to a proto
// NetworkEndpoint, mapping every field.
func NetworkEndpointToProto(ep *types.NetworkEndpoint) *sbv1.NetworkEndpoint {
if ep == nil {
return nil
}
return &sbv1.NetworkEndpoint{
Host: ep.Host,
Port: ep.Port,
Protocol: ep.Protocol,
}
return policyNetworkEndpointToProto(ep)
}

// --- NetworkBinary ---
Expand Down
52 changes: 52 additions & 0 deletions openshell/v1/internal/converter/profile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
sbv1 "github.com/ai-openshell/sdk-go/proto/sandboxv1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/durationpb"
)

Expand Down Expand Up @@ -751,3 +752,54 @@ func TestProviderProfileRoundTrip(t *testing.T) {

assert.Equal(t, original.Discovery.Credentials, back.Discovery.Credentials)
}

// Profile endpoints share the policy endpoint message; access, enforcement,
// credential settings, and every other field must survive both directions
// (ai-openshell/sdk-go#12).
func TestNetworkEndpointRoundTripKeepsAllFields(t *testing.T) {

@rhuss rhuss Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

in := &sbv1.NetworkEndpoint{
Host: "api.anthropic.com",
Port: 443,
Ports: []uint32{443, 8443},
Protocol: "rest",
Tls: sbv1.NetworkTlsMode_NETWORK_TLS_MODE_SKIP,
Enforcement: sbv1.NetworkEnforcementMode_NETWORK_ENFORCEMENT_MODE_ENFORCE,
Access: sbv1.NetworkAccessPreset_NETWORK_ACCESS_PRESET_FULL,
AllowedIps: []string{"10.0.0.0/8"},
Path: "/v1",
AllowUninspectedCredentials: true,
ProviderCredentialed: true,
CredentialBinding: &sbv1.NetworkCredentialBinding{Provider: "anthropic"},
}

ep := NetworkEndpointFromProto(in)
require.NotNil(t, ep)
assert.Equal(t, v1.NetworkAccessPresetFull, ep.Access)
assert.True(t, ep.AllowUninspectedCredentials)
require.NotNil(t, ep.CredentialBinding)
assert.Equal(t, "anthropic", ep.CredentialBinding.Provider)

out := NetworkEndpointToProto(ep)
assert.True(t, proto.Equal(in, out), "round trip changed the endpoint:\nin: %v\nout: %v", in, out)
}

func TestProviderProfileRoundTripKeepsEndpointAccess(t *testing.T) {
in := &pb.ProviderProfile{
Id: "claude-code",
Endpoints: []*sbv1.NetworkEndpoint{{
Host: "api.anthropic.com",
Port: 443,
Protocol: "rest",
Access: sbv1.NetworkAccessPreset_NETWORK_ACCESS_PRESET_READ_WRITE,
Enforcement: sbv1.NetworkEnforcementMode_NETWORK_ENFORCEMENT_MODE_ENFORCE,
}},
}

profile := ProviderProfileFromProto(in)
require.Len(t, profile.Endpoints, 1)
assert.Equal(t, v1.NetworkAccessPresetReadWrite, profile.Endpoints[0].Access)

out := ProviderProfileToProto(profile)
require.Len(t, out.GetEndpoints(), 1)
assert.True(t, proto.Equal(in.GetEndpoints()[0], out.GetEndpoints()[0]))
}
6 changes: 3 additions & 3 deletions openshell/v1/types/network_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ const (
NetworkAccessPresetFull NetworkAccessPreset = 3
)

// PolicyNetworkEndpoint describes a full network endpoint with its access controls
// as used in sandbox network policy rules. This is distinct from [NetworkEndpoint]
// which is the simplified profile-level endpoint (Host, Port, Protocol only).
// PolicyNetworkEndpoint describes a network endpoint with its access controls,
// as used in sandbox network policy rules and, as [NetworkEndpoint], in
// provider profiles.
type PolicyNetworkEndpoint struct {
Host string
Port uint32
Expand Down
11 changes: 5 additions & 6 deletions openshell/v1/types/profile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,11 @@ type TokenGrantAudienceOverride struct {
Scopes []string
}

// NetworkEndpoint describes a network endpoint provided by a profile.
type NetworkEndpoint struct {
Host string
Port uint32
Protocol string
}
// NetworkEndpoint describes a network endpoint provided by a profile. Profiles
// and sandbox policies share one endpoint message, so it is the same type as
// [PolicyNetworkEndpoint], with access, enforcement, rules, and credential
// settings.
type NetworkEndpoint = PolicyNetworkEndpoint

@rhuss rhuss Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rhuss Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// NetworkBinary describes a binary artifact provided by a profile.
type NetworkBinary struct {
Expand Down
6 changes: 6 additions & 0 deletions openshell/v1/types_reexport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ type NetworkPolicyRule = types.NetworkPolicyRule
// PolicyNetworkEndpoint describes a full network endpoint in a sandbox network policy rule.
type PolicyNetworkEndpoint = types.PolicyNetworkEndpoint

// NetworkCredentialBinding binds an endpoint to static credentials from an attached provider.
type NetworkCredentialBinding = types.NetworkCredentialBinding

// McpOptions configures MCP inspection for a policy endpoint.
type McpOptions = types.McpOptions

// NetworkTLSMode controls TLS handling for a policy endpoint.
type NetworkTLSMode = types.NetworkTLSMode

Expand Down
Loading