Refactor GitHub MCP transport options to share a single common option struct

docs/adr/54714-share-github-mcp-common-options-via-embedded-struct.md

@@ -0,0 +1,44 @@
+# ADR-54714: Share GitHub MCP Common Options via Embedded Struct
+
+**Date**: 2026-08-22
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+### Context
+
+The GitHub MCP renderer supports two transport modes — Docker (local) and Remote (hosted). Each mode has its own configuration struct (`GitHubMCPDockerOptions` and `GitHubMCPRemoteOptions`). Both structs independently declared the same 8 fields: `ReadOnly`, `Lockdown`, `LockdownFromStep`, `GuardPoliciesFromStep`, `Toolsets`, `Features`, `AllowedTools`, and `GuardPolicies`. These fields control shared MCP behaviour (read-only access, lockdown enforcement, guard policies, toolset selection, feature flags, and allowed-tool filtering) regardless of transport. The duplication meant that any change to shared behaviour required coordinated edits in two places, with no compiler-level guarantee that the structs remained in sync, creating ongoing drift risk.
+
+### Decision
+
+We will introduce `GitHubMCPCommonOptions` as a new shared struct containing all 8 transport-agnostic fields, and update `GitHubMCPDockerOptions` and `GitHubMCPRemoteOptions` to embed it anonymously. A regression test (`TestGitHubMCPOptionsEmbedCommonOptions`) uses reflection to assert that both transport structs embed `GitHubMCPCommonOptions`, enforcing the constraint at compile/test time. All construction sites in `mcp_renderer_github.go` initialise shared fields via the embedded struct literal.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep duplicated fields (status quo)
+
+Each transport struct retains its own independent copy of the 8 shared fields. Behaviour is identical to the new approach at runtime. Rejected because: there is no mechanism to prevent the structs from diverging independently — the problem that motivated this PR — and future changes to shared fields must always be made twice with no compiler enforcement.
+
+#### Alternative 2: Shared builder function instead of struct embedding
+
+A helper function (e.g., `newCommonOptions(...)`) could accept the common arguments and populate each transport struct's fields individually. This avoids anonymous embedding and keeps field access flat. Rejected because: it does not create a named type boundary visible in struct literals, making it harder to see at a glance which fields are shared; and it provides no compile-time or test-time guarantee that both transport structs expose the same set of shared fields.
+
+### Consequences
+
+#### Positive
+- Single definition for all shared GitHub MCP configuration fields; any new shared field is added once.
+- Compile/test-time enforcement via `TestGitHubMCPOptionsEmbedCommonOptions` prevents future structs from silently omitting the embedded type.
+- Reduced field count in transport-specific structs; transport-specific fields are clearly distinguished from shared ones.
+
+#### Negative
+- Call sites must use the explicit embedded struct key (`GitHubMCPCommonOptions: GitHubMCPCommonOptions{...}`) in keyed struct literals, which is more verbose than flat field assignment.
+- Go's field promotion means shared fields appear on the transport struct's surface, which can obscure their origin for readers unfamiliar with the embedding relationship.
+
+#### Neutral
+- All existing test cases required mechanical updates to use the embedded struct literal syntax — no test logic changed, only struct initialisation syntax.
+- The regression test uses `reflect.Type.FieldByName` + `Anonymous` check, which is a non-zero dependency on reflection in the test suite.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/engine_helpers_github_test.go

@@ -17,12 +17,14 @@ func TestRenderGitHubMCPDockerConfig(t *testing.T) {
 		{
 			name: "Claude engine configuration (no type field, with effective token)",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           false,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Toolsets:     "default",
+					AllowedTools: nil,
+				},
 				DockerImageVersion: "latest",
 				CustomArgs:         nil,
 				IncludeTypeField:   false,
-				AllowedTools:       nil,
 				EffectiveToken:     "${{ secrets.GITHUB_TOKEN }}",
 			},
 			expected: []string{
@@ -42,12 +44,14 @@ func TestRenderGitHubMCPDockerConfig(t *testing.T) {
 		{
 			name: "Copilot engine configuration (with type field, no effective token)",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           false,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Toolsets:     "default",
+					AllowedTools: []string{"create_issue", "issue_read"},
+				},
 				DockerImageVersion: "latest",
 				CustomArgs:         nil,
 				IncludeTypeField:   true,
-				AllowedTools:       []string{"create_issue", "issue_read"},
 				EffectiveToken:     "",
 			},
 			expected: []string{
@@ -67,12 +71,14 @@ func TestRenderGitHubMCPDockerConfig(t *testing.T) {
 		{
 			name: "Read-only mode enabled",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           true,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     true,
+					Toolsets:     "default",
+					AllowedTools: nil,
+				},
 				DockerImageVersion: "v1.0.0",
 				CustomArgs:         nil,
 				IncludeTypeField:   false,
-				AllowedTools:       nil,
 				EffectiveToken:     "",
 			},
 			expected: []string{
@@ -90,12 +96,14 @@ func TestRenderGitHubMCPDockerConfig(t *testing.T) {
 		{
 			name: "Custom args provided",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           false,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Toolsets:     "default",
+					AllowedTools: nil,
+				},
 				DockerImageVersion: "latest",
 				CustomArgs:         []string{"--verbose", "--debug"},
 				IncludeTypeField:   false,
-				AllowedTools:       nil,
 				EffectiveToken:     "",
 			},
 			expected: []string{
@@ -111,12 +119,14 @@ func TestRenderGitHubMCPDockerConfig(t *testing.T) {
 		{
 			name: "Copilot with wildcard tools (no allowed tools specified)",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           false,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Toolsets:     "default",
+					AllowedTools: nil, // When nil, should default to wildcard
+				},
 				DockerImageVersion: "latest",
 				CustomArgs:         nil,
 				IncludeTypeField:   true,
-				AllowedTools:       nil, // When nil, should default to wildcard
 				EffectiveToken:     "",
 			},
 			expected: []string{
@@ -131,12 +141,14 @@ func TestRenderGitHubMCPDockerConfig(t *testing.T) {
 		{
 			name: "Custom toolsets",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           false,
-				Toolsets:           "repos,issues,pull_requests",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Toolsets:     "repos,issues,pull_requests",
+					AllowedTools: nil,
+				},
 				DockerImageVersion: "latest",
 				CustomArgs:         nil,
 				IncludeTypeField:   false,
-				AllowedTools:       nil,
 				EffectiveToken:     "",
 			},
 			expected: []string{
@@ -176,12 +188,14 @@ func TestRenderGitHubMCPDockerConfig_OutputStructure(t *testing.T) {
 	// Test that the output has the expected JSON structure
 	var yaml strings.Builder
 	RenderGitHubMCPDockerConfig(&yaml, GitHubMCPDockerOptions{
-		ReadOnly:           true,
-		Toolsets:           "default",
+		GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+			ReadOnly:     true,
+			Toolsets:     "default",
+			AllowedTools: []string{"tool1", "tool2"},
+		},
 		DockerImageVersion: "latest",
 		CustomArgs:         []string{"--test"},
 		IncludeTypeField:   true,
-		AllowedTools:       []string{"tool1", "tool2"},
 		EffectiveToken:     "",
 	})
 

pkg/workflow/github_lockdown_test.go

@@ -114,12 +114,14 @@ func TestRenderGitHubMCPDockerConfigWithLockdown(t *testing.T) {
 		{
 			name: "Docker mode with lockdown enabled",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           false,
-				Lockdown:           true,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Lockdown:     true,
+					Toolsets:     "default",
+					AllowedTools: nil,
+				},
 				DockerImageVersion: "latest",
 				IncludeTypeField:   true,
-				AllowedTools:       nil,
 			},
 			expected: []string{
 				`"type": "stdio"`,
@@ -132,12 +134,14 @@ func TestRenderGitHubMCPDockerConfigWithLockdown(t *testing.T) {
 		{
 			name: "Docker mode with lockdown disabled",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           false,
-				Lockdown:           false,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Lockdown:     false,
+					Toolsets:     "default",
+					AllowedTools: nil,
+				},
 				DockerImageVersion: "latest",
 				IncludeTypeField:   true,
-				AllowedTools:       nil,
 			},
 			expected: []string{
 				`"type": "stdio"`,
@@ -151,12 +155,14 @@ func TestRenderGitHubMCPDockerConfigWithLockdown(t *testing.T) {
 		{
 			name: "Docker mode with lockdown and read-only both enabled",
 			options: GitHubMCPDockerOptions{
-				ReadOnly:           true,
-				Lockdown:           true,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     true,
+					Lockdown:     true,
+					Toolsets:     "default",
+					AllowedTools: nil,
+				},
 				DockerImageVersion: "v1.0.0",
 				IncludeTypeField:   false,
-				AllowedTools:       nil,
 			},
 			expected: []string{
 				`"GITHUB_READ_ONLY": "1"`,
@@ -200,12 +206,14 @@ func TestRenderGitHubMCPRemoteConfigWithLockdown(t *testing.T) {
 		{
 			name: "Remote mode with lockdown enabled",
 			options: GitHubMCPRemoteOptions{
-				ReadOnly:           false,
-				Lockdown:           true,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Lockdown:     true,
+					Toolsets:     "default",
+					AllowedTools: []string{"*"},
+				},
 				AuthorizationValue: "Bearer test-token",
 				IncludeToolsField:  true,
-				AllowedTools:       []string{"*"},
 				IncludeEnvSection:  false,
 			},
 			expected: []string{
@@ -221,12 +229,14 @@ func TestRenderGitHubMCPRemoteConfigWithLockdown(t *testing.T) {
 		{
 			name: "Remote mode with lockdown disabled",
 			options: GitHubMCPRemoteOptions{
-				ReadOnly:           false,
-				Lockdown:           false,
-				Toolsets:           "default",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     false,
+					Lockdown:     false,
+					Toolsets:     "default",
+					AllowedTools: []string{"*"},
+				},
 				AuthorizationValue: "Bearer test-token",
 				IncludeToolsField:  true,
-				AllowedTools:       []string{"*"},
 				IncludeEnvSection:  false,
 			},
 			expected: []string{
@@ -242,12 +252,14 @@ func TestRenderGitHubMCPRemoteConfigWithLockdown(t *testing.T) {
 		{
 			name: "Remote mode with lockdown and read-only both enabled",
 			options: GitHubMCPRemoteOptions{
-				ReadOnly:           true,
-				Lockdown:           true,
-				Toolsets:           "repos,issues",
+				GitHubMCPCommonOptions: GitHubMCPCommonOptions{
+					ReadOnly:     true,
+					Lockdown:     true,
+					Toolsets:     "repos,issues",
+					AllowedTools: nil,
+				},
 				AuthorizationValue: "Bearer test-token",
 				IncludeToolsField:  false,
-				AllowedTools:       nil,
 				IncludeEnvSection:  false,
 			},
 			expected: []string{