Skip to content

Refactor GitHub MCP serialization into shared section helpers#41038

Merged
pelikhan merged 15 commits into
mainfrom
copilot/duplicate-code-fix
Jun 24, 2026
Merged

Refactor GitHub MCP serialization into shared section helpers#41038
pelikhan merged 15 commits into
mainfrom
copilot/duplicate-code-fix

Conversation

Copilot AI commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

GitHub MCP rendering was duplicating the same map-serialization and section-closing logic across local, remote, and custom paths. That duplication made env/header output and guard-policy handling easier to drift when one path changed without the others.

  • Shared serialization helpers

    • Add focused helpers for sorted JSON/TOML string-map rendering.
    • Reuse one path for comma placement and section closing instead of open-coded loops in each renderer.
  • Centralized GitHub MCP assembly

    • Move shared GitHub env/header construction behind common helpers.
    • Keep local Docker, remote HTTP, and TOML rendering aligned on:
      • token passthrough
      • GITHUB_HOST / toolset propagation
      • read-only / lockdown flags
  • Unified guard-policy footer handling

    • Route GitHub guard-policy rendering through a single helper.
    • Keep explicit guard policies and step-derived automatic policies emitted consistently across local and remote modes.
  • Custom MCP cleanup

    • Reuse the same section writers in custom MCP JSON/TOML rendering for env, headers, and http_headers.
    • Extract custom env-value normalization so format-specific substitution logic is defined once.
writeJSONStringMapSection(yaml, "                ", "env", envVars, hasGuardPolicies)
renderGitHubMCPGuardPolicies(yaml, options.GuardPolicies, options.GuardPoliciesFromStep, "                ")

``
pr-sous-chef: refresh branch requested.
Run: https://github.com/github/gh-aw/actions/runs/28055021664

Generated by 👨‍🍳 PR Sous Chef · 85.2 AIC · ⌖ 1.07 AIC · ⊞ 17.3K ·

Copilot AI and others added 4 commits June 23, 2026 14:34
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor duplicate GitHub MCP serialization logic Refactor GitHub MCP serialization into shared section helpers Jun 23, 2026
Copilot AI requested a review from pelikhan June 23, 2026 14:46
@pelikhan pelikhan marked this pull request as ready for review June 23, 2026 16:29
Copilot AI review requested due to automatic review settings June 23, 2026 16:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors repeated GitHub MCP config rendering logic by introducing shared section helpers for deterministic string-map serialization (JSON/TOML) and consolidating common GitHub env/header + guard-policy rendering across local Docker, remote HTTP, and custom MCP paths.

Changes:

  • Added shared helpers for writing sorted JSON/TOML string-map sections and centralized GitHub MCP env/header/guard-policy assembly.
  • Updated GitHub MCP renderers (Docker/remote/TOML) and custom MCP config rendering to reuse the shared section writers.
  • Removed the legacy header-to-YAML helper and added targeted unit tests for the new helper behavior.
Show a summary per file
File Description
pkg/workflow/yaml_env_helpers.go Removes the legacy header serialization helper (now superseded by shared MCP section writers).
pkg/workflow/mcp_renderer_section_helpers.go Adds shared JSON/TOML string-map section helpers plus GitHub MCP env/header/guard-policy helpers.
pkg/workflow/mcp_renderer_section_helpers_test.go Adds unit tests covering the new shared helpers.
pkg/workflow/mcp_renderer_github.go Switches GitHub MCP Docker/remote/TOML rendering to use the new shared helpers.
pkg/workflow/mcp_config_custom.go Reuses shared section writers for custom MCP env/headers/http_headers rendering and centralizes env normalization.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 5/5 changed files
  • Comments generated: 1

Comment on lines +3 to +26
import (
"fmt"
"strings"
)

func writeJSONStringMapEntries(yaml *strings.Builder, values map[string]string, indent string) {
for i, key := range sortedMapKeys(values) {
comma := ","
if i == len(values)-1 {
comma = ""
}
fmt.Fprintf(yaml, "%s\"%s\": \"%s\"%s\n", indent, key, values[key], comma)
}
}

func writeJSONStringMapSection(yaml *strings.Builder, indent, name string, values map[string]string, trailingComma bool) {
fmt.Fprintf(yaml, "%s\"%s\": {\n", indent, name)
writeJSONStringMapEntries(yaml, values, indent+" ")
if trailingComma {
fmt.Fprintf(yaml, "%s},\n", indent)
return
}
fmt.Fprintf(yaml, "%s}\n", indent)
}

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.

Fixed in f210dc2. writeJSONStringMapEntries now marshals keys/values via json.Marshal, and I added coverage in TestWriteJSONStringMapSectionEscapesKeysAndValues to assert valid JSON for quoted/newline-containing keys and values.

@github-actions

Copy link
Copy Markdown
Contributor

@copilot review all comments and address unresolved review feedback. Please fix the JSON escaping issue and rerun checks.

Generated by 👨‍🍳 PR Sous Chef · 36.5 AIC · ⌖ 1.06 AIC · ⊞ 17.3K ·

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot review all comments and address unresolved review feedback. Please fix the JSON escaping issue and rerun checks.

...

Addressed in f210dc2. I fixed JSON string escaping in the shared MCP section helpers and updated affected tests/goldens; local validation was rerun (make fmt, make lint, make test-unit, make test, make recompile).

@copilot run pr-finisher skill

Done. I ran the pr-finisher pass and pushed f210dc2 with the unresolved MCP JSON escaping fix plus updated tests/goldens and full local validation.

@github-actions github-actions Bot mentioned this pull request Jun 23, 2026
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

@copilot review all comments and address unresolved review feedback.

Generated by 👨‍🍳 PR Sous Chef · 39.7 AIC · ⌖ 1.05 AIC · ⊞ 17.3K ·

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot recompile

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot recompile

Recompiled and pushed in 882a212.

@github-actions

Copy link
Copy Markdown
Contributor

``
@copilot review all comments and address unresolved review feedback.
Please resolve the open thread so this PR can be re-reviewed.

Generated by 👨‍🍳 PR Sous Chef · 85.2 AIC · ⌖ 1.07 AIC · ⊞ 17.3K ·

@github-actions

Copy link
Copy Markdown
Contributor

``
@copilot refresh the branch, rerun checks, and summarize the remaining blockers.

Generated by 👨‍🍳 PR Sous Chef · 85.2 AIC · ⌖ 1.07 AIC · ⊞ 17.3K ·

@pelikhan

Copy link
Copy Markdown
Collaborator

Copilot AI and others added 3 commits June 23, 2026 22:29
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix https://github.com/github/gh-aw/actions/runs/28055646864/job/83069633363?pr=41038

Fixed in 4b93189. I investigated the failing job and removed the panic path in mustMarshalJSONString so custom linter panicinlibrarycode passes.

@github-actions

Copy link
Copy Markdown
Contributor

``
@copilot review all comments and address the unresolved review feedback.
Please also summarize any remaining blockers and the next merge-ready step.

Generated by 👨‍🍳 PR Sous Chef · 53.9 AIC · ⌖ 1.12 AIC · ⊞ 17.6K ·

@pelikhan pelikhan merged commit 1b8bc51 into main Jun 24, 2026
29 checks passed
@pelikhan pelikhan deleted the copilot/duplicate-code-fix branch June 24, 2026 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants