Add GitHubMCPMode string enum and replace githubTool any with map[string]any#38945
Conversation
Co-authored-by: pelikhan <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request strengthens the GitHub MCP configuration surface by introducing a typed GitHubMCPMode enum (instead of bare strings) and by narrowing many githubTool parameters from any to map[string]any, removing repeated type assertions across the workflow/MCP rendering and compilation codepaths.
Changes:
- Add
GitHubMCPModestring enum and updateGitHubToolConfig.ModefromstringtoGitHubMCPMode. - Replace repeated
githubTool any→.(map[string]any)assertions by acceptingmap[string]anyin helpers/renderers and asserting once at call sites. - Update affected workflow compilation, MCP rendering, environment, and test code to use the narrower type and typed mode comparisons.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/tools_validation_test.go | Updates test inputs to use map[string]any for GitHub tool config. |
| pkg/workflow/tools_types.go | Introduces GitHubMCPMode and updates GitHubToolConfig.Mode type. |
| pkg/workflow/tools_parser.go | Casts parsed mode string into GitHubMCPMode at ingestion. |
| pkg/workflow/safeoutputs_guard_policy_test.go | Adjusts guard-policy tests for narrowed GitHub tool type. |
| pkg/workflow/mcp_setup_generator.go | Asserts GitHub tool config once and uses typed mode comparisons for gateway setup behavior. |
| pkg/workflow/mcp_renderer.go | Narrows GitHub renderer input to map[string]any via single assertion at dispatch. |
| pkg/workflow/mcp_renderer_types.go | Updates renderer function signatures to accept map[string]any for GitHub. |
| pkg/workflow/mcp_renderer_github.go | Updates GitHub MCP rendering to accept map[string]any and typed mode comparisons. |
| pkg/workflow/mcp_renderer_factory.go | Updates renderer factory closures to match narrowed GitHub signature. |
| pkg/workflow/mcp_github_config.go | Refactors GitHub tool helpers to accept map[string]any and return typed GitHubMCPMode. |
| pkg/workflow/mcp_environment.go | Uses map[string]any GitHub tool config when deriving MCP env vars. |
| pkg/workflow/mcp_config_test.go | Updates MCP config tests for narrowed GitHub tool config type. |
| pkg/workflow/github_toolset_test.go | Updates toolset tests for narrowed GitHub tool config type. |
| pkg/workflow/github_toolset_expansion_test.go | Updates toolset expansion tests for narrowed GitHub tool config type. |
| pkg/workflow/github_readonly_test.go | Simplifies read-only test to match getGitHubReadOnly() no-arg behavior. |
| pkg/workflow/github_lockdown_test.go | Updates lockdown test to assert GitHub tool config once. |
| pkg/workflow/engine_helpers_shared_test.go | Updates test renderer stubs for new GitHub renderer signature. |
| pkg/workflow/docker.go | Updates GitHub docker-image collection to use typed GitHub mode constants. |
| pkg/workflow/copilot_github_mcp_test.go | Updates Copilot GitHub MCP tests for narrowed GitHub tool config type. |
| pkg/workflow/copilot_engine_test.go | Updates Copilot engine tests for narrowed GitHub tool config type. |
| pkg/workflow/copilot_engine_execution.go | Updates token derivation to use asserted map[string]any GitHub config. |
| pkg/workflow/compiler_yaml.go | Updates lockdown env emission path to assert GitHub tool map once. |
| pkg/workflow/compiler_github_mcp_steps.go | Updates guard-policy checks to operate on asserted GitHub tool map. |
| pkg/workflow/compiler_difc_proxy.go | Narrows GitHub tool config plumbing for DIFC/CLI proxy policy derivation. |
| pkg/workflow/compiler_difc_proxy_test.go | Updates DIFC proxy policy tests for narrowed GitHub tool config type. |
| pkg/workflow/codex_mcp.go | Updates Codex MCP rendering path to assert GitHub tool map once. |
| pkg/workflow/claude_tools.go | Uses typed GitHub mode comparisons when selecting default tool lists. |
| pkg/workflow/agent_validation.go | Updates pi-engine mode validation to compare against typed constant. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 28/28 changed files
- Comments generated: 2
| if rawGithubTool, hasGitHub := tools["github"]; hasGitHub { | ||
| githubTool, _ := rawGithubTool.(map[string]any) | ||
| githubType := getGitHubType(githubTool) | ||
| // Only add if using local (Docker) mode | ||
| if githubType == "local" { | ||
| if githubType == GitHubMCPModeLocal { |
| if tools == nil || tools.GitHub == nil || tools.GitHub.Mode != GitHubMCPModeGHProxy { | ||
| return errors.New("engine 'pi' requires tools.github.mode: gh-proxy") | ||
| } |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (225 new lines in 📄 Draft ADR committed: 📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. A type-safety refactor like this one encodes a convention (push type assertions to the boundary; name the mode values) that future contributors will need to understand and uphold. Recording the decision now prevents the convention from silently eroding later. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /tdd, and /grill-with-docs — commenting with refinement suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Type-safety gap:
GitHubToolConfig.TyperemainsstringwhileModewas upgraded toGitHubMCPMode— thetypefield accepts the samelocal/remotesubset and should match - Silent mode fallthrough:
getGitHubTypesilently defaults tolocalwhenmode: gh-proxyormode: cliis set — intentional but not logged, unlike thetypefield path - Parse-time validation:
GitHubMCPMode(mode)intools_parser.goaccepts any string without checking against the enum constants, relying on use-site log warnings instead of early detection - Missing boundary tests: No test asserts that
gh-proxy/cliin themodefield →localtransport ingetGitHubType; this looks identical to the "invalid input" case without the test - Unenforced deprecation:
GitHubMCPModeCLIcarries the "legacy alias" label but emits no runtime warning; users have no signal to migrate togh-proxy
Positive Highlights
- ✅ Excellent reduction: 19 functions cleaned up, each
.(map[string]any)assertion now at exactly one call site per function - ✅
getGitHubReadOnly()removing the unused parameter is the right call — the parameter was dead weight - ✅
github_readonly_test.gosimplification is clean and correct - ✅ The single ingestion point in
tools_parser.gofor theGitHubMCPModecast is the right pattern - ✅ All 11 test files updated with consistent narrowed types
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 514.8 AIC · ⌖ 14.3 AIC · ⊞ 27.8K
| Allowed GitHubAllowedTools `yaml:"allowed,omitempty"` | ||
| Mode string `yaml:"mode,omitempty"` | ||
| Mode GitHubMCPMode `yaml:"mode,omitempty"` | ||
| Type string `yaml:"type,omitempty"` |
There was a problem hiding this comment.
[/improve-codebase-architecture] Type was not updated alongside Mode — the type field accepts only local and remote, which are a subset of GitHubMCPMode. Leaving it as string is an incomplete type safety improvement.
💡 Suggested change
// Before
Type string `yaml:"type,omitempty"`
// After
Type GitHubMCPMode `yaml:"type,omitempty"`With this change, getGitHubType and normalizeGitHubType would benefit from consistent types across both the raw-map and the parsed-struct paths. The type field is restricted to local/remote at validation anyway — give the type system a chance to enforce it.
| @@ -124,10 +121,10 @@ func isGitHubCLIModeEnabled(data *WorkflowData) bool { | |||
|
|
|||
| // normalizeGitHubType normalizes and validates GitHub MCP transport values. | |||
| // Supported values are `local` and `remote`. | |||
There was a problem hiding this comment.
[/improve-codebase-architecture] The comment says "Supported values are local and remote" without explaining that gh-proxy and cli are valid GitHubMCPMode constants intentionally excluded here. A future reader may think this is an omission rather than a design choice.
💡 Suggested comment update
// normalizeGitHubType normalizes and validates GitHub MCP transport values
// for the Docker/hosted transport path.
// Supports `local` (Docker) and `remote` (hosted) only.
// CLI-proxy modes (gh-proxy, cli) are a separate concern handled
// by isGitHubCLIModeEnabled and always use the local transport type.
func normalizeGitHubType(value string) (GitHubMCPMode, bool) {The current comment actively misleads anyone adding a new GitHubMCPMode constant: they might wonder why their new value isn\u2019t handled here.
| githubConfigLog.Printf("GitHub MCP type read from legacy mode field: %s", normalizedValue) | ||
| return normalizedValue | ||
| } | ||
| } |
There was a problem hiding this comment.
[/improve-codebase-architecture] When mode: gh-proxy or mode: cli is set, normalizeGitHubType returns ("", false) and this block silently falls through to the local default — no log message. The type field path (line 143) already logs "Unrecognized tools.github.type value" for unknown values. Consistent logging would help when debugging unexpected local mode selection.
💡 Suggested addition
After the normalizeGitHubType call in the mode path, add a log:
// gh-proxy/cli are valid modes but use local transport; CLI path is handled by isGitHubCLIModeEnabled
githubConfigLog.Printf("GitHub MCP mode %q uses local Docker transport (CLI proxy routing handled separately)", stringValue)This prevents silent confusion when a user sets mode: gh-proxy and wonders why Docker mode is active.
|
|
||
| if mode, ok := configMap["mode"].(string); ok { | ||
| config.Mode = mode | ||
| config.Mode = GitHubMCPMode(mode) |
There was a problem hiding this comment.
[/improve-codebase-architecture] The cast GitHubMCPMode(mode) converts any arbitrary string — including misspellings like "gh_proxy" or "locall" — to the type without validation. The enum gives compile-time safety for code, but doesn't protect against runtime YAML input.
💡 Suggested validation
Add a simple validity check at the ingestion point:
parsedMode := GitHubMCPMode(mode)
switch parsedMode {
case GitHubMCPModeLocal, GitHubMCPModeRemote, GitHubMCPModeGHProxy, GitHubMCPModeCLI:
config.Mode = parsedMode
default:
// Log and leave Mode at zero value; validation errors are surfaced later.
toolsParserLog.Printf("Unknown tools.github.mode value: %q", mode)
}This surfaces misconfiguration early and in one place, consistent with how other fields handle unknown values via the githubConfigLog pattern.
| // GitHubMCPModeGHProxy routes GitHub operations through the gh CLI proxy. | ||
| GitHubMCPModeGHProxy GitHubMCPMode = "gh-proxy" | ||
| // GitHubMCPModeCLI is a legacy alias for GitHubMCPModeGHProxy. | ||
| GitHubMCPModeCLI GitHubMCPMode = "cli" |
There was a problem hiding this comment.
[/grill-with-docs] GitHubMCPModeCLI is documented as a "legacy alias" but there's no active deprecation mechanism — no validation warning is emitted when mode: cli is used in a workflow. Users won't know to migrate to mode: gh-proxy.
💡 Suggestion
In isGitHubCLIModeEnabled (or at parse time), emit a deprecation notice when cli is detected:
case GitHubMCPModeCLI:
githubConfigLog.Print("tools.github.mode: \"cli\" is deprecated; use \"gh-proxy\" instead")
// Optionally surface this as a compiler warning, not just a debug log
return trueWithout any signal, the "legacy" label is aspirational rather than actionable for users.
There was a problem hiding this comment.
Review: GitHubMCPMode enum + map[string]any narrowing refactor
Solid cleanup — the PR consistently pushes type assertions to call sites and introduces a well-named enum type. No correctness regressions found; Go nil-map reads are safe, and the old any-based helpers had identical behaviour when passed non-map values.
Non-blocking findings (3)
-
normalizeGitHubTypeerror sentinel (mcp_github_config.go:130) — returnsGitHubMCPMode("")with no exported constant for the zero/unknown case. See inline comment. -
isGitHubCLIModeEnabledis the only remaining inline assertion (mcp_github_config.go:104) — all other helpers were refactored but this function still doesgithubTool.(map[string]any)internally. See inline comment. -
GitHubToolConfig.Typeis stillstring(tools_types.go:308) —Modewas upgraded toGitHubMCPModebutType(same domain, same valid values) was not. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 846.4 AIC · ⌖ 13.6 AIC · ⊞ 16.7K
| case GitHubMCPModeLocal, GitHubMCPModeRemote: | ||
| return normalizedValue, true | ||
| default: | ||
| return "", false |
There was a problem hiding this comment.
normalizeGitHubType returns GitHubMCPMode("") as its error sentinel — the zero value of the new enum type — with no exported constant for it.
💡 Suggested fix
Add a sentinel constant so the error return is self-documenting and exhaustive switch statements can cover it:
// In tools_types.go, add:
GitHubMCPModeUnknown GitHubMCPMode = ""Then the return becomes:
default:
return GitHubMCPModeUnknown, falseAll current callers guard on the bool, so this is not a runtime bug, but introducing a type without a defined zero-value constant invites callers to silently compare == "" or use uninitialized GitHubMCPMode vars without realising they match an unintended state.
| @@ -107,13 +104,13 @@ func isGitHubCLIModeEnabled(data *WorkflowData) bool { | |||
| if toolConfig, ok := githubTool.(map[string]any); ok { | |||
There was a problem hiding this comment.
isGitHubCLIModeEnabled was only half-refactored — it still does its own .(map[string]any) assertion internally while every other helper function in this file was updated to accept map[string]any directly.
💡 Details
Every other helper (getGitHubType, getGitHubToken, getGitHubLockdown, getGitHubGuardPolicies, etc.) now takes map[string]any and delegates the type assertion to the call site. isGitHubCLIModeEnabled has a different signature (*WorkflowData) so it can’t be refactored the same way, but the internal assertion on line 104:
if toolConfig, ok := githubTool.(map[string]any); ok {...is now the only place in mcp_github_config.go that does this pattern inline. Introduce a helper to extract the map (or extract it once at the top of the function alongside the existing githubTool == false guard) to be consistent with the rest of the file and avoid repeating the pattern if a future caller needs to handle a validated map[string]any.
| type GitHubToolConfig struct { | ||
| Allowed GitHubAllowedTools `yaml:"allowed,omitempty"` | ||
| Mode string `yaml:"mode,omitempty"` | ||
| Mode GitHubMCPMode `yaml:"mode,omitempty"` |
There was a problem hiding this comment.
GitHubToolConfig.Mode is now typed as GitHubMCPMode but Type (line 309) remains string, even though both fields encode the same domain concept and getGitHubType() reads from both.
💡 Details
getGitHubType() in mcp_github_config.go reads the "type" key first and falls back to "mode". Both fields describe the MCP transport mode (local/remote), yet one is GitHubMCPMode and the other is plain string. A future reader will not know why these differ or which is canonical. Either:
- Type the
Typefield asGitHubMCPMode(preferred, enforces the same validation at parse time), or - Add a comment explaining that
Typeis retained asstringintentionally (e.g., if it accepts valuesGitHubMCPModedoes not).
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics & Test Classification (19 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 92/100. Test quality is excellent — 0% of modified tests are implementation tests (threshold: 30%). All 19 test changes are mechanical type-safety refactors preserving existing behavioral contracts. Full analysis posted in the comment above.
|
@copilot run pr-finisher skill |
…cli as pi engine mode Co-authored-by: pelikhan <[email protected]>
Addressed both review threads in commit
Local |
pkg/workflow/mcp_github_config.gohad ~19 functions acceptinggithubTool anythat each immediately asserted tomap[string]any, and MCP mode values were passed as bare strings with no compile-time checking.Type enum
Added
GitHubMCPModestring type with named constants totools_types.go:GitHubToolConfig.Modeupdated fromstring→GitHubMCPMode.tools_parser.gocasts the parsed string at the single ingestion point.Parameter type narrowing
All
githubTool anyparams acrossmcp_github_config.goand its callers changed tomap[string]any. The single.(map[string]any)assertion now happens once at each call site (fromtools["github"]) rather than inside every helper. Nil maps are safe — Go returns zero values on reads.getGitHubReadOnly()had its unused parameter removed (always returnstrue).normalizeGitHubType/getGitHubTypereturnGitHubMCPModeinstead ofstring. All bare string comparisons (== "remote",!= "gh-proxy") replaced with the typed constants.Files changed
tools_types.go,mcp_github_config.gomcp_renderer_github.go,mcp_renderer_types.go,mcp_renderer_factory.go,mcp_renderer.go,docker.go,compiler_yaml.go,compiler_difc_proxy.go,compiler_github_mcp_steps.gomcp_environment.go,mcp_setup_generator.go,copilot_engine_execution.go,codex_mcp.go,claude_tools.gotools_parser.go,agent_validation.gomap[string]any, non-map test inputs replaced withnil