Repository navigation
fix(stack): pin MCP versions and document server pre-approval - #520
shaurya57-drona wants to merge 1 commit into
Conversation
Added section on MCP Servers and Permissions to README.
|
Addresses the audit findings reported in #514:
Closes #514 |
clark-cant
left a comment
There was a problem hiding this comment.
Summary: Docs-only addition to stack/README.md documenting the three bundled MCP servers and the enableAllProjectMcpServers: true pre-approval default (addresses the audit in #514).
Risk level: Low
Mandatory gates
- Duplicate / prior implementation: overlap — complementary to open PR #519 (same author, same issue #514; #519 carries the
.mcp.jsonversion pinning, this PR carries the README doc). No merged prior art documents MCP permissions instack/README.md. - Project standards: docs found (
CLAUDE.md,CONTRIBUTING.md); editingstack/directly matches the merged precedent #517 (also touchedstack/README.md). - Strategic necessity: clear value — documents a security-relevant default (project-wide MCP server pre-approval) and how to opt out.
Findings
Important
-
Doc ↔ config drift on the pinned versions. The new section states the three servers are pinned (
@playwright/mcp@0.0.83,chrome-devtools-mcp@1.10.1,shadcn@4.21.0), but this PR only touchesstack/README.md. Onmaintodaystack/.mcp.jsonstill uses@latestfor all three — the pinning lives in the separate, unmerged #519. Merging this PR on its own makes the README describe configuration that does not exist. Please land the pinning first (or together), or reword the section so the doc matchesmainuntil #519 merges. -
Missing blank line breaks rendering of existing content. In the diff the new
>blockquote is immediately followed by the existing closing lineBuilt as a starter for teams who want Claude Code to ship…with no blank line between them. In CommonMark that line becomes a lazy continuation of the blockquote paragraph, so the closing tagline renders inside the security note. Add a blank line after the blockquote. -
Title/body over-claim the scope. The title says “pin MCP versions and document server pre-approval” and the body repeats #519’s bullet list (“Pinned … in
stack/.mcp.json”), but this PR changes onlyREADME.md. Align the title/body with the doc-only scope, and keep the “pinned” claim in the PR that actually pins the versions.
Suggestion
-
Section placement. The new heading is inserted after the License section’s horizontal rule (
---), sitting between that rule and the closing tagline. Consider moving the block above## Requirements(or into “What’s in here”) so the License rule and closing tagline stay last. -
Heading level. Every other section in this file is
##; the new one is###. Use##for consistency (or deliberately nest it under an existing section).
Verdict: Request changes — Important: version/doc drift vs main, blockquote misrender of existing content, and title/body scope over-claim.
Posted by /ck:review-pr at 2026-10-08T20:07:09Z
Added section on MCP Servers and Permissions to README.
What does this PR change?
Why?
Checklist
Changes were made in
src/ui-ux-pro-max/(source of truth), not directly in.claude/or.factory/Ran
npm run sync:assets && npm run check:assetsincli/if data/scripts/templates changedAdded or updated tests if behavior changed (
.claude/skills/*/scripts/tests/,cli/tests/e2e/)Commit messages follow Conventional Commits (
feat:,fix:,docs:, etc.)This PR targets a feature branch, not pushed directly to
mainAddresses the audit findings reported in stack/ pre-approves every project MCP server (enableAllProjectMcpServers: true) #514:
Version Pinning: Pinned
@playwright/mcp,chrome-devtools-mcp, andshadcnto stable versions instack/.mcp.json.Permission Transparency: Documented
enableAllProjectMcpServers: trueinstack/README.mdso users understand the pre-approval behavior and know how to disable it.Closes #514