feat(mcp): wave 7b — editing-convention config-CRUD tools - #342
Conversation
Expose the /conventions REST surface as MCP tools (ADR-044): list_conventions & get_library_conventions (read), set_library_conventions & clone_conventions (write). All four move MCP_UNEXPOSED → OP_TO_TOOL with tiers. get_library_conventions surfaces inherited=true when a library falls back to the built-in industry default (ADR-022). set_library_conventions upserts one profile per library and rejects unsafe/catastrophic regex (ConventionValidation Error → tool error, mirroring REST 422). clone_conventions seeds from a built-in or another library's profile by sourceId. Handlers reuse the REST body schemas verbatim (PutConventionBodySchema/CloneConventionBodySchema). New convention.integration.test.ts: built-in listing, fresh-library inheritance, set→get round-trip, clone-from-built-in, and missing-library / bad-source errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex (GPT-5.5, xhigh) adversarial review — ran against |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Wave 7b editing-convention CRUD MCP tools: capability-tier and contract-map entries, handler implementations, registration wiring, and integration tests for listing, reading, updating, cloning, and validation. ChangesConvention Config MCP Tools
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mcp/convention.integration.test.ts (1)
42-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for unsafe/catastrophic-regex rejection.
The PR objectives call out that
set_library_conventionsrejects unsafe/catastrophic regex as a tool error (mirroring REST 422), viaConventionValidationErrorhandling inconvention-handlers.ts. This suite covers not-found errors but doesn't exercise that validation-rejection path — worth adding given it's a safety-relevant behavior explicitly promised by the PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/convention.integration.test.ts` around lines 42 - 89, Add test coverage for the unsafe/catastrophic-regex rejection path in convention MCP tools. Extend the convention integration suite around handleSetLibraryConventions to attempt setting a convention with a regex pattern that should be rejected, and assert the result is a tool error. Use the existing helpers in convention.integration.test.ts and the set/get flow to verify the ConventionValidationError path exposed by convention-handlers.ts is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp/convention-handlers.ts`:
- Around line 61-65: `handleGetLibraryConventions` is returning a fixed
validation message instead of the actual Zod validation details, unlike
`handleSetLibraryConventions` and `handleCloneConventions`. Update the
`ConventionLibraryIdArgs.safeParse` failure path in
`handleGetLibraryConventions` to return `issues(parsed.error)` so the error text
reflects the specific input problem, and keep the success path unchanged.
---
Nitpick comments:
In `@src/mcp/convention.integration.test.ts`:
- Around line 42-89: Add test coverage for the unsafe/catastrophic-regex
rejection path in convention MCP tools. Extend the convention integration suite
around handleSetLibraryConventions to attempt setting a convention with a regex
pattern that should be rejected, and assert the result is a tool error. Use the
existing helpers in convention.integration.test.ts and the set/get flow to
verify the ConventionValidationError path exposed by convention-handlers.ts is
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68eb48cc-94f4-4be7-a098-b9852f6d1258
📒 Files selected for processing (6)
src/mcp/capabilities.tssrc/mcp/contract-map.tssrc/mcp/convention-handlers.tssrc/mcp/convention-tools.tssrc/mcp/convention.integration.test.tssrc/mcp/tools.ts
…CodeRabbit) Adds a regression test asserting set_library_conventions rejects a catastrophic-backtracking regex in the rules (noteBanners: ['(a+)+$']) as a tool error — pinning the ConventionValidationError → toolError branch the PR description calls out but the suite didn't yet exercise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CodeRabbit nitpick addressed: added the missing unsafe/catastrophic-regex coverage. |
Why
Wave 7 (config-CRUD parity, ADR-044), sub-wave 7b. Exposes the
/conventionsREST endpoints as MCP tools so an agent can manage a library's editing-convention profiles (the editor-clue rule sets — colors, choice tokens, banners; ADR-022), not just consume them.What
Four tools, moved
MCP_UNEXPOSED→OP_TO_TOOLwith tiers:list_conventionsGET /conventionsget_library_conventionsGET /libraries/{id}/conventionsset_library_conventionsPUT /libraries/{id}/conventionsclone_conventionsPOST /libraries/{id}/conventions/cloneget_library_conventionssurfacesinherited: truewhen a library has no profile of its own and resolves to the built-in industry default.set_library_conventionsupserts one profile per library and rejects an unsafe/catastrophic regex in the rules (ConventionValidationError→ tool error, mirroring REST 422).clone_conventionsseeds a library's profile from a built-in default or another library's profile bysourceId.PutConventionBodySchema,CloneConventionBodySchema); noopenapi.yamlchange (wraps existing ops).No destructive tool in this sub-wave. No trim needed on the convention
name(the table has nolength(trim(name))>0CHECK, unlike templates).Review
Testing
convention.integration.test.ts(built-in listing, inheritance, set→get, clone, error branches)🤖 Co-authored by Claude Opus 4.8. Part of the Wave 7 config-CRUD parity series (ADR-044/045).
Summary by CodeRabbit
New Features
Bug Fixes
Tests