Skip to content

Refactor MCP config generation into SDK helper#1179

Open
weinong wants to merge 1 commit into
bradygaster:devfrom
weinong:refactor-mcp-config-helper
Open

Refactor MCP config generation into SDK helper#1179
weinong wants to merge 1 commit into
bradygaster:devfrom
weinong:refactor-mcp-config-helper

Conversation

@weinong

@weinong weinong commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • extract duplicated MCP server/config/frontmatter generation into @bradygaster/squad-sdk/config
  • update init and upgrade paths to use the shared MCP helper
  • align CLI SDK dependency minimum and lockfile metadata with the new SDK export
  • add behavioral package-export coverage for the shared MCP helper

Tests

  • npm test -- test/package-exports.test.ts test/cli/init.test.ts test/cli/upgrade.test.ts
  • npm run lint

Follow-up to #1166

Copilot AI review requested due to automatic review settings May 26, 2026 20:00

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

Note

Copilot was unable to run its full agentic suite in this review.

This PR extracts MCP (Model Context Protocol) configuration helpers into a shared SDK module, re-exports them from @bradygaster/squad-sdk/config, and updates the CLI to consume the shared helpers.

Changes:

  • Added mcp-config.ts to centralize MCP server spec generation and frontmatter/config rendering helpers.
  • Refactored SDK init.ts and CLI upgrade.ts to import MCP helpers from the shared module instead of duplicating logic.
  • Added export verification + behavior tests and bumped package versions to align CLI ↔ SDK.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/package-exports.test.ts Adds export checks and behavior tests for MCP helper functions exposed via the config barrel.
packages/squad-sdk/src/config/mcp-config.ts Introduces centralized MCP server spec + config/frontmatter helper implementations.
packages/squad-sdk/src/config/init.ts Removes inlined MCP helper implementations and uses the shared mcp-config module.
packages/squad-sdk/src/config/index.ts Re-exports the new MCP config module from the SDK config barrel.
packages/squad-sdk/package.json Bumps SDK version to 0.9.7-preview.
packages/squad-cli/src/cli/core/upgrade.ts Replaces duplicated MCP helper code with imports from @bradygaster/squad-sdk/config.
packages/squad-cli/package.json Updates CLI dependency constraint to require the new SDK version.

Comment on lines +77 to +95
export function injectMcpFrontmatter(content: string, servers: McpServerSpec[]): string {
const closingStart = content.indexOf('\n---', 4);
if (!content.startsWith('---') || closingStart === -1) {
return content;
}

return content.slice(0, closingStart)
+ '\n'
+ buildMcpFrontmatterBlock(servers)
+ content.slice(closingStart);
}

export function hasMcpFrontmatter(content: string): boolean {
const frontmatterEnd = content.indexOf('\n---', 4);
if (!content.startsWith('---') || frontmatterEnd === -1) {
return false;
}
return content.slice(0, frontmatterEnd).includes('mcp-servers:');
}
expect(injected).toContain('mcp-servers:\n squad_state:');
expect(injected).toContain('\n---\n\nBody mentions mcp-servers: only here.\n');
});

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

👋 Friendly nudge — this PR has had no activity for 12 days.

What needs attention:

  • 🔴 1 CI check(s) failing: Policy Gates. Fix these first.
  • 💬 2 unresolved review thread(s) (2 from Copilot). Address and resolve them.
  • 👀 No approving reviews yet. Request a review from a teammate.

If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows.
This is an automated check that runs on weekdays. It won't nudge the same PR more than once per week.

@bradygaster

Copy link
Copy Markdown
Owner

Hey @weinong — could you rebase this onto current dev when you get a chance? There are merge conflicts and a CI failure that likely resolve with a fresh rebase. Let me know if you need a hand! 🙏

@weinong weinong force-pushed the refactor-mcp-config-helper branch from 44c2d6b to ef9f418 Compare June 10, 2026 14:02
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🏗️ Architectural Review

⚠️ Architectural review: 1 warning(s).

Severity Category Finding Files
🟡 warning bootstrap-area 1 file(s) in the bootstrap area (packages/squad-cli/src/cli/core/) were modified. These files must maintain zero external dependencies. Review carefully. packages/squad-cli/src/cli/core/upgrade.ts

Automated architectural review — informational only.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🟡 Impact Analysis — PR #1179

Risk tier: 🟡 MEDIUM

📊 Summary

Metric Count
Files changed 6
Files added 2
Files modified 4
Files deleted 0
Modules touched 4
Critical files 1

🎯 Risk Factors

  • 6 files changed (6-20 → MEDIUM)
  • 4 modules touched (2-4 → MEDIUM)
  • Critical files touched: packages/squad-sdk/src/config/index.ts

📦 Modules Affected

root (1 file)
  • .changeset/fix-mcp-config-helpers.md
squad-cli (1 file)
  • packages/squad-cli/src/cli/core/upgrade.ts
squad-sdk (3 files)
  • packages/squad-sdk/src/config/index.ts
  • packages/squad-sdk/src/config/init.ts
  • packages/squad-sdk/src/config/mcp-config.ts
tests (1 file)
  • test/package-exports.test.ts

⚠️ Critical Files

  • packages/squad-sdk/src/config/index.ts

This report is generated automatically for every PR. See #733 for details.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🛫 PR Readiness Check

ℹ️ This comment updates on each push. Last checked: commit 6050f16

PR Scope: 📦🔧 Mixed (product + infrastructure)

⚠️ 3 item(s) to address before review

Status Check Details
Single commit 1 commit — clean history
Not in draft Ready for review
Branch up to date Up to date with dev
Copilot review No Copilot review yet — it may still be processing
Changeset present Changeset file found
Scope clean No .squad/ or docs/proposals/ files
No merge conflicts No merge conflicts
Copilot threads resolved 1 unresolved Copilot thread(s) — fix and resolve before merging
CI passing 6 check(s) still running

Files Changed (6 files, +203 −187)

File +/−
.changeset/fix-mcp-config-helpers.md +6 −0
packages/squad-cli/src/cli/core/upgrade.ts +3 −92
packages/squad-sdk/src/config/index.ts +1 −0
packages/squad-sdk/src/config/init.ts +1 −95
packages/squad-sdk/src/config/mcp-config.ts +115 −0
test/package-exports.test.ts +77 −0

Total: +203 −187


This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.

@weinong weinong force-pushed the refactor-mcp-config-helper branch from ef9f418 to 6050f16 Compare June 10, 2026 14:07
@weinong

weinong commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current dev and addressed the review feedback. Updates pushed in 6050f169:\n\n- Moved MCP config generation into the shared SDK helper while preserving the newer CLI version pinning behavior.\n- Fixed MCP frontmatter detection/injection for CRLF-delimited files.\n- Added regression coverage for CRLF frontmatter handling.\n- Added a changeset so Policy Gates pass.\n\nCI is green now, including Policy Gates, sdk-exports-validation, samples-build, and test.

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