Sync to server 0.8.1 and add branch-merge deleteBranch support#15
Sync to server 0.8.1 and add branch-merge deleteBranch support#15ragnorc wants to merge 3 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23843e8534
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "packageManager": "pnpm@9.15.0", | ||
| "omnigraph": { | ||
| "serverVersion": "0.8.0" | ||
| "serverVersion": "0.8.1" |
There was a problem hiding this comment.
Because this change also adds the untagged delete_branch / branch_deleted branch-merge fields to spec/openapi.json, leaving omnigraph.serverVersion at 0.8.1 makes the generated SERVER_VERSION claim compatibility with the 0.8.1 release even though that release's drift source will not match. In any release/CI path that runs pnpm run check-drift (which fetches v${serverVersion}/openapi.json), this pin fails and users can send deleteBranch to a 0.8.1 server that does not implement it; please keep the spec at 0.8.1 or bump this to the first tagged server containing the new fields.
Useful? React with 👍 / 👎.
| target: z.string().optional(), | ||
| deleteBranch: z.boolean().optional(), | ||
| }, | ||
| annotations: { readOnlyHint: false, destructiveHint: true, idempotentHint: true, openWorldHint: false }, |
There was a problem hiding this comment.
When callers set deleteBranch: true, retrying the same branches_merge after a successful response no longer has a source branch to merge, so it can fail with a missing source instead of returning already_up_to_date. Because the MCP tool advertises idempotentHint: true unconditionally, hosts may safely retry a call that is not safe under this option; please drop the hint or split cleanup into a separate non-idempotent path.
Useful? React with 👍 / 👎.
The spec carries the branch-merge delete_branch request field, the branch_deleted / branch_delete_error response fields, and the read_set_conflict / recovery_required structured error fields — all on the server's main branch but not yet in a tagged release, so check-drift stays red until the next release is pinned (see the PR checklist).
ConflictError gains readSetConflict (the coarse OCC read-set rejection, alongside the existing manifestConflict), and a code-less 503 with a recovery_required body maps to a typed ServiceUnavailableError carrying the blocking recovery operationId. branches.merge() documents deleteBranch and its non-fatal branchDeleted / branchDeleteError result fields; the types flow from the regenerated spec.
The tool passes deleteBranch through to the SDK and the workflow instructions collapse the branch loop's merge + delete steps into one branches_merge call; a deletion refusal is reported in the result, never as a tool error. The stub transport gains a /branches/merge route so the passthrough is pinned.
23843e8 to
ee4ca14
Compare
1 participant
What & why
Adds the merge-and-delete-source surface from ModernRelay/omnigraph#354 —
deleteBranchonog.branches.merge()and on thebranches_mergeMCP tool, with the non-fatalbranchDeleted/branchDeleteErrorresult fields — plus the other post-0.8.1 server additions the regenerated spec brings in:ConflictError.readSetConflictand a typedServiceUnavailableError(code-less 503) carryingrecoveryRequired.operationId.Based on #16 (the green 0.8.1 version catch-up), so this diff is exactly the unreleased overlay.
merge()docs. Tests: 89 passed (new: deleteBranch wire test, readSetConflict / 503 mapping).branches_mergeacceptsdeleteBranch; the workflow instructions collapse the branch loop's merge + delete into one call. Tests: 15 passed (new: passthrough).check-versionsandcheck-coveragepass; typecheck green.Draft until
check-driftis deliberately red: the vendored spec carries fields from the server's main branch (ModernRelay/omnigraph#354 and the structured-error work) that are not in any tagged release. Before undrafting:omnigraph.serverVersion(and package versions) to that release, runpnpm sync-spec && pnpm generate, confirmcheck-driftis green byte-for-byteNote
Medium Risk
Touches branch merge/delete behavior and client retry semantics for writes (409 read-set, 503 recovery); mostly additive API surface with tests, but callers must handle new non-fatal merge delete fields and error types.
Overview
Adds post-merge source branch cleanup end-to-end:
deleteBranchonog.branches.merge()and the MCPbranches_mergetool, withbranchDeleted/branchDeleteErroron success when deletion is refused. Agent workflow text now recommends merge + delete in one step instead of a separate delete call.Regenerates the vendored OpenAPI/SDK types for the newer server contract: merge request/response fields, 503 on several write/branch paths, and richer 409 bodies (
read_set_conflict, load “prepared authority” wording).The SDK error layer now surfaces
ConflictError.readSetConflictand maps code-less 503 responses toServiceUnavailableErrorwithrecoveryRequired.operationId, exported from the public API. MCP and SDK tests coverdeleteBranchpassthrough and the new error shapes.Reviewed by Cursor Bugbot for commit ee4ca14. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR syncs the SDK and MCP surfaces to the newer server API overlay. The main changes are:
deleteBranchsupport for SDK and MCP branch merges.Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
deleteBranchto the MCP merge tool and updates the workflow guidance.Reviews (2): Last reviewed commit: "Add deleteBranch to the branches_merge M..." | Re-trigger Greptile