Refactor MCP JSON section helpers to share rendering skeleton

[Copilot]

The semantic clustering pass found the codebase to be well-factored, with one minor cosmetic duplication in the MCP renderer: writeJSONStringMapSection and writeJSONStringMapSectionRaw repeated the same section scaffolding and only differed in how entries were written. This change removes that duplication without changing the raw placeholder-unescaping behavior.

• What changed

  • Added a small private helper in pkg/workflow/mcp_renderer_section_helpers.go to own the shared:
    • section open
    • entry emission
    • trailing-comma / close handling
  • Kept the two public call paths split:
    • writeJSONStringMapSection still uses normal JSON entry encoding
    • writeJSONStringMapSectionRaw still uses the raw entry writer for unquoted heredoc env/header sections

• Behavior preserved

  • The raw path continues to undo the json.Marshal double-escape for shell placeholders like \${VAR}.
  • No changes were made to the documented escaping semantics or to call sites.

• Result

  • Removes the duplicated section-writing skeleton while keeping the raw/non-raw distinction explicit and localized.

func writeJSONStringMapSection(yaml *strings.Builder, indent, name string, values map[string]string, trailingComma bool) {
	writeJSONStringMapSectionWithEntries(yaml, indent, name, values, trailingComma, writeJSONStringMapEntries)
}

func writeJSONStringMapSectionRaw(yaml *strings.Builder, indent, name string, values map[string]string, trailingComma bool) {
	writeJSONStringMapSectionWithEntries(yaml, indent, name, values, trailingComma, writeJSONStringMapEntriesRaw)
}

[Copilot]

Pull request overview

Refactors MCP JSON section rendering to share common scaffolding while preserving raw placeholder handling.

Changes:

• Adds a shared entry-writer callback helper.
• Retains distinct standard and raw JSON entry paths.

Show a summary per file

File	Description
pkg/workflow/mcp_renderer_section_helpers.go	Deduplicates JSON section rendering logic.

Review details

Tip

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

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Medium

[github-actions]

🤖 PR Triage

Field	Value
Category	refactor
Risk	low
Score	28/100 (impact:10 urgency:8 quality:10)
Action	✅ auto_merge

Rationale: Single-file refactor deduplicating MCP JSON section helper skeleton (11 add / 13 del). Pure DRY improvement, not draft, no behavior change. Auto-merge when CI passes.

Generated by 🔧 PR Triage Agent · 208.1 AIC · ⌖ 4.98 AIC · ⊞ 5.6K · ◷

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. This PR (#45410) refactors MCP JSON section helpers to share rendering skeleton—a production code change only. Test Quality Sentinel analysis skipped.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #45410 does not have the 'implementation' label and has only 11 new lines of code in business logic directories (threshold: 100).

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

Skills-Based Review\n\nApplied /codebase-design — one minor suggestion, otherwise a clean refactor.\n\nThe only finding: writeJSONStringMapSectionWithEntries has no doc comment. Future readers won't know what the writeEntries parameter contract is. A brief comment matching the one already on writeJSONStringMapSectionRaw would complete the picture.\n\nPositive highlights: clean strategy-pattern extraction, zero behaviour change, doc comment preserved, net line reduction.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 23 AIC · ⌖ 4.38 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[github-actions]

[/codebase-design] The new private helper writeJSONStringMapSectionWithEntries is the load-bearing piece of this refactor but has no doc comment — future readers won't know what the writeEntries parameter contract is (e.g. the expected indentation it receives).

💡 Suggested doc comment

// writeJSONStringMapSectionWithEntries writes a JSON object section, delegating
// per-entry serialisation to writeEntries.  writeEntries receives the builder,
// the values map, and the child indent string (indent+"  ").

Adding this makes the abstraction self-documenting and consistent with the existing comment on writeJSONStringMapSectionRaw.

@copilot please address this.

[github-actions]

Clean structural refactor — no blocking issues.

The extraction of writeJSONStringMapSectionWithEntries correctly eliminates the duplicated section-scaffolding without changing any observable behavior. Both delegating wrappers preserve their original semantics, the raw escape-handling path remains intact, and the doc comment was correctly migrated to writeJSONStringMapSectionRaw.

Minor note (non-blocking)

writeJSONStringMapSectionWithEntries has no doc comment. Since it is unexported and the two wrappers have docs, this is acceptable, but a one-liner explaining the writeEntries callback contract would help future readers.

🔎 Code quality review by PR Code Quality Reviewer · 22.7 AIC · ⌖ 4.31 AIC · ⊞ 5.4K
_{Comment /review to run again}

[github-actions]

Clean refactor — the shared writeJSONStringMapSectionWithEntries helper correctly captures the repeated section scaffold, both public functions delegate to it properly, and the raw-path doc comment is preserved. No behavioral changes detected.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.1 AIC · ⌖ 4.89 AIC · ⊞ 4.8K