feat(mcp): wave 5 — apply_merge tool + extract applyMerge service - #339
Conversation
Burn down the last MCP_UNEXPOSED spec op — POST /specs/{}/merge — into the
apply_merge tool (write tier), stacked on wave 4.
The merge orchestration (one transaction: composed edit gate ADR-018 + optimistic
precondition, applyAccepted, content_version bump) was inlined in the REST
mergeHandler. Extracted it into a shared applyMerge(specId, accept, diff,
expectedVersion) service in src/merge, so the REST route and the MCP tool share
one code path instead of the MCP handler re-implementing raw transaction/SQL
(the boundary smell flagged in wave 3). The REST handler now just maps outcomes.
The nested DiffResultSchema + merge fields move to src/ast/merge-schemas.ts
(MergeFieldsShape), reused by the REST strict body schema and the MCP tool (which
also carries specId) — the big diff schema lives in one place. Edit-gate and merge
errors (stale/forbidden/InvalidAcceptedChange/MergeError) map to tool errors;
apply_merge returns { applied, rejected } like its REST sibling. Contract green
(INV-1/2/2b/3); REST merge tests unchanged (refactor is behavior-preserving).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR centralizes merge application logic into a new ChangesCentralize merge orchestration and expose apply_merge MCP tool
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant mergeHandler
participant handleApplyMerge
participant applyMerge
participant DB
alt REST request
Client->>mergeHandler: POST /specs/:id/merge
mergeHandler->>applyMerge: applyMerge(id, accept, diff, expectedVersion)
else MCP tool call
Client->>handleApplyMerge: apply_merge(args)
handleApplyMerge->>applyMerge: applyMerge(specId, accept, diff, expectedVersion)
end
applyMerge->>DB: find spec, begin transaction
applyMerge->>DB: assertSpecWritable, applyAccepted
applyMerge->>DB: update content_version, commit
DB-->>applyMerge: outcome
applyMerge-->>mergeHandler: outcome
applyMerge-->>handleApplyMerge: outcome
mergeHandler-->>Client: 200/404 response
handleApplyMerge-->>Client: success payload or toolError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Two P3 findings from the Codex adversarial review (no P1/P2; refactor confirmed behavior-equivalent + transaction-safe): - applyMerge accepted an injected `db` but ran the existence check on the global findSpecById/pool — a caller injecting another pool would get split read/write. Neither caller injects, so remove the param (YAGNI) and use the global pool throughout — the inconsistency is gone. - apply_merge tool description implied every accepted merge bumps contentVersion; reworded to say it bumps only when at least one change is actually applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex adversarial review (GPT-5.5 xhigh) — outcomesNo P1/P2. Codex confirmed the refactor is behavior-equivalent and transaction-safe: Two P3s, both fixed:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/merge/apply-merge.ts (1)
42-48: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSwallowed rollback failure has no observability.
If
ROLLBACKitself fails, the error is silently discarded with only a comment. Consider logging via the pino logger (per repo convention) so a failed rollback isn't invisible in production.As per path instructions,
src/**/*.ts: "Do not useconsole.*insrc/outside tests and scripts; use the pino logger atsrc/lib/logger.tsinstead." — while noconsole.*is used here, adding alogger.warnon rollback failure would align with that logging convention.🤖 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/merge/apply-merge.ts` around lines 42 - 48, The rollback failure in the `apply-merge` error path is silently swallowed, so add pino-based observability when `client.query('ROLLBACK')` throws inside `apply-merge.ts`. Update the nested `try/catch` in `apply-merge` to log a warning through the shared logger from `src/lib/logger.ts` before continuing with the original error rethrow, so a failed rollback is visible without changing the existing control flow.Source: Path instructions
🤖 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/merge/apply-merge.ts`:
- Around line 42-48: The rollback failure in the `apply-merge` error path is
silently swallowed, so add pino-based observability when
`client.query('ROLLBACK')` throws inside `apply-merge.ts`. Update the nested
`try/catch` in `apply-merge` to log a warning through the shared logger from
`src/lib/logger.ts` before continuing with the original error rethrow, so a
failed rollback is visible without changing the existing control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d199938f-a1e6-46c0-bc5e-f2dd6a80cc60
📒 Files selected for processing (11)
src/api/merge.tssrc/ast/index.tssrc/ast/merge-schemas.tssrc/mcp/capabilities.tssrc/mcp/contract-map.tssrc/mcp/merge-handlers.tssrc/mcp/merge-tool.integration.test.tssrc/mcp/merge-tools.tssrc/mcp/tools.tssrc/merge/apply-merge.tssrc/merge/index.ts
Why
Wave 5 of the MCP contract (ADR-044/045): make the last spec-level write — applying an accepted merge — reachable as an MCP tool, so an agent can run the full redline loop (
get_spec_diff→ review →apply_merge).What
apply_merge(write) ←POST /specs/{}/merge. PassspecId, theDiffResultfromget_spec_diff, andaccept(the change UUIDs to apply). Returns{ applied, rejected }.applyAccepted,content_versionbump) was inlined in the RESTmergeHandler. Extracted into a sharedapplyMerge()service insrc/merge, called by both the REST route and the MCP tool — so the MCP handler doesn't re-implement raw transaction/SQL (the boundary smell CodeRabbit flagged in Wave 3). The REST handler now just maps outcomes to HTTP.DiffResultSchema+ merge fields move tosrc/ast/merge-schemas.ts(MergeFieldsShape), reused by the REST strict body schema and the MCP tool (which also carriesspecId) — the big diff schema lives in one place.openapi.yamlchange. Contract green (INV-1/2/2b/3); REST merge tests unchanged (refactor is behavior-preserving).Testing
merge-tool.integration.test.ts(5 cases) + REST merge tests + contract pass (106 integration total); 1340 unitpnpm lintclean (eslint + tsc + prettier)🤖 Co-authored by Claude Opus 4.8. Stacked on #338.
Summary by CodeRabbit
New Features
Bug Fixes
Tests