feat(mcp): wave 2b — design package tools (list / create / set specs / delete) - #349
Merged
Conversation
…/ delete)
Expose the design-package REST surface as MCP tools, continuing REST↔MCP parity
(ADR-044). Four tools: list_packages (read), create_package and set_package_specs
(write), delete_package (destructive). A design package is a subset of a project's
sections issued together. create is name-unique per project; set_package_specs is
a full ordered replacement whose members must be in the project TOC; delete is a
hard CASCADE (also destroys the package's issued revisions + frozen snapshots —
no guard), so it's destructive/off-by-default. Handlers reuse the REST body
schemas via `.shape` and never throw (return { isError } on failure). Four ops
move MCP_UNEXPOSED → OP_TO_TOOL; tiers registered. Parity + coverage stay green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Wave 2b "design packages" MCP tools: capability tiers and contract mappings for list/create/set-specs/delete package operations, corresponding Zod-validated handlers backed by DB calls, a registration module wiring these into the tool registrar, integration into ChangesDesign package MCP tools
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PackageHandler
participant Zod
participant DB
Client->>PackageHandler: handleCreatePackage(args)
PackageHandler->>Zod: validate projectId + name
Zod-->>PackageHandler: parsed args or validation error
PackageHandler->>DB: createPackage(projectId, name, pool)
DB-->>PackageHandler: package row or Postgres error (23503/23505)
PackageHandler-->>Client: ok(package) or toolError
Client->>PackageHandler: handleSetPackageSpecs(args)
PackageHandler->>Zod: validate packageId + specIds
Zod-->>PackageHandler: parsed args or validation error
PackageHandler->>DB: setPackageSpecs(packageId, specIds, pool)
DB-->>PackageHandler: specs or PackageNotFoundError/SpecNotInProjectError
PackageHandler-->>Client: ok({packageId, specs}) or toolError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
thewrz
marked this pull request as ready for review
July 3, 2026 15:26
Contributor
Author
|
Codex (GPT-5.5, xhigh) adversarial review against |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mcp/package.integration.test.ts (1)
117-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for order-preservation and empty-array clearing in
set_package_specs.The PR explicitly documents that
set_package_specs"preserves order" and that "an empty array clears" the package's specs, but this test only exercises a single-spec array. Neither the ordering contract nor the clear-with-[]behavior is verified here.♻️ Suggested additional assertions
it('sets package members from the project TOC and rejects out-of-project specs', async () => { const { projectId, specId } = await projectWithSection('09 91 00'); const pkg = parse<PackageSummary>(await handleCreatePackage({ projectId, name: 'Members' })); const res = await handleSetPackageSpecs({ packageId: pkg.packageId, specIds: [specId] }); expect(isToolError(res)).toBe(false); expect(parse<{ specs: unknown[] }>(res).specs.length).toBe(1); + // empty array clears membership + const cleared = await handleSetPackageSpecs({ packageId: pkg.packageId, specIds: [] }); + expect(isToolError(cleared)).toBe(false); + expect(parse<{ specs: unknown[] }>(cleared).specs).toHaveLength(0); + // a spec not in this project's TOC → not in project expect( isToolError(await handleSetPackageSpecs({ packageId: pkg.packageId, specIds: [MISSING] })) ).toBe(true);A separate multi-section fixture would be needed to assert order preservation (e.g., pass
[specB, specA]and confirmspecs[0].specId === specB).🤖 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/package.integration.test.ts` around lines 117 - 137, The `set_package_specs` coverage only verifies a single spec and misses the documented ordering and clear semantics. Extend the `handleSetPackageSpecs` test to use multiple specs from a multi-section fixture and assert the returned `specs` preserve the input order, then add a call with an empty `specIds` array and verify it clears the package’s specs. Keep the existing `handleSetPackageSpecs`/`PackageSummary` assertions and add checks around `parse(...)` for both the ordered result and the empty-array case.
🤖 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.
Nitpick comments:
In `@src/mcp/package.integration.test.ts`:
- Around line 117-137: The `set_package_specs` coverage only verifies a single
spec and misses the documented ordering and clear semantics. Extend the
`handleSetPackageSpecs` test to use multiple specs from a multi-section fixture
and assert the returned `specs` preserve the input order, then add a call with
an empty `specIds` array and verify it clears the package’s specs. Keep the
existing `handleSetPackageSpecs`/`PackageSummary` assertions and add checks
around `parse(...)` for both the ordered result and the empty-array case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb419675-fd26-48f8-8ac2-9daefa5fd1f6
📒 Files selected for processing (6)
src/mcp/capabilities.tssrc/mcp/contract-map.tssrc/mcp/package-handlers.tssrc/mcp/package-tools.tssrc/mcp/package.integration.test.tssrc/mcp/tools.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Wave 2b of the MCP contract build-out (ADR-044) — packages, the middle of the final Wave 2 remainder. Wraps the design-package REST surface so an agent can group a project's sections into issuable packages.
What
Four new tools, contract-bound to existing REST ops (no
openapi.yamlchange):list_packagesget /projects/{}/packagescreate_packagepost /projects/{}/packagesset_package_specsput /packages/{}/specsdelete_packagedelete /packages/{}create_package— name unique per project (409 on collision → tool error), position auto-assigned.set_package_specs— full ordered replacement (empty array clears); every specId must be in the package's own project TOC (else tool error).delete_packageis destructive — a hard CASCADE that also destroys the package's issued revisions and their frozen snapshots (there is no guard for issued revisions), so it's gated off by default..shapeand never throw (return{ isError: true }). Four ops moveMCP_UNEXPOSED→OP_TO_TOOL; tiers registered. Parity (INV-1/2/3) + coverage stay green.Wave 2c (revisions —
issue_package_revision+get_revision) is the last sub-wave; it completes full REST↔MCP parity.Testing
package.integration.test.ts, 9/9)contract.integration.test.ts, 5/5)pnpm test:coveragegreen (2233 pass; branches 80.39%)pnpm lintgreen🤖 Co-authored by Claude Opus 4.8 (1M context).
Summary by CodeRabbit
New Features
Bug Fixes