Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions packages/mcp/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Workflow norms (violating these breaks things or silently corrupts data):
4. \`load mode: "merge"\` upserts by @key (idempotent — use this for at-least-once pipelines). \`"overwrite"\` truncates the branch. \`"append"\` fails on key collision.
5. Verify every write. \`commits_list\` head BEFORE and AFTER. If identical, the write did not land. 504s do not mean failure — the server may have committed after the proxy dropped the response.
6. Append-only types (Signal, Claim, Decision, Event, Interaction, Policy, Outcome, MarketingElement) duplicate on blind retry. Pointer types (Org, Person, Opportunity, Channel, Actor, ActionItem, Artifact, Meeting, Technology, Campaign, UseCase) dedupe via @key.
7. Risky/large writes: \`branches_create\` from main → \`load\` onto the branch → verify → \`branches_merge\` → \`branches_delete\`.
7. Risky/large writes: \`branches_create\` from main → \`load\` onto the branch → verify → \`branches_merge\` with \`deleteBranch: true\` (merges and cleans up the branch in one step; a deletion refusal is reported in the result, never fails the merge).
8. Schema is read-only over this MCP. \`schema_get\` returns the active .pg source; there is no \`schema_apply\` tool. A cluster-managed graph rejects HTTP schema apply (409) — schema changes go through \`omnigraph cluster apply\` (an operator/CLI action), not an agent tool.

Date format: ISO strings on \`mutate\` params; integer days-since-epoch in load JSONL \`Date\` fields. \`DateTime\` is ISO on both.
Expand Down Expand Up @@ -323,15 +323,21 @@ export function createOmnigraphMcpServer(opts: CreateServerOptions): McpServer {
{
title: 'Merge branch',
description:
'Merge `source` into `target` (default `main`). Idempotent: re-merging an already-merged branch yields outcome=already_up_to_date.',
'Merge `source` into `target` (default `main`). Idempotent: re-merging an already-merged branch yields outcome=already_up_to_date. ' +
'Pass deleteBranch=true to delete the source branch after the merge lands — a deletion refusal is reported via branchDeleted/branchDeleteError in the result, never as an error.',
inputSchema: {
source: z.string().min(1),
target: z.string().optional(),
deleteBranch: z.boolean().optional(),
},
annotations: { readOnlyHint: false, destructiveHint: true, idempotentHint: true, openWorldHint: false },
Comment thread
greptile-apps[bot] marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not advertise merge-and-delete as idempotent

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 👍 / 👎.

},
async ({ source, target }) => {
const r = await og.branches.merge({ source, target: target ?? defaultBranch });
async ({ source, target, deleteBranch }) => {
const r = await og.branches.merge({
source,
target: target ?? defaultBranch,
deleteBranch,
});
return { content: jsonText(r) };
},
);
Expand Down
26 changes: 26 additions & 0 deletions packages/mcp/test/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,20 @@ function fakeFetch(): typeof globalThis.fetch {
if (method === 'GET' && path === '/branches') {
return respond(200, { branches: ['main', 'feature'] });
}
if (method === 'POST' && path === '/branches/merge') {
const body = JSON.parse(String(init?.body ?? '{}')) as {
source: string;
target?: string;
delete_branch?: boolean;
};
return respond(200, {
actor_id: null,
outcome: 'already_up_to_date',
source: body.source,
target: body.target ?? 'main',
...(body.delete_branch ? { branch_deleted: true } : {}),
});
}
if (method === 'GET' && path === '/schema') {
return respond(200, { schema_source: 'node Person { name: String @key }' });
}
Expand Down Expand Up @@ -138,6 +152,18 @@ describe('omnigraph-mcp server', () => {
expect(parsed.sdkServerVersion).toBe('0.8.1');
});

it('branches_merge passes deleteBranch through and returns the delete result', async () => {
const { client } = await setup();
const r = await client.callTool({
name: 'branches_merge',
arguments: { source: 'feature', target: 'main', deleteBranch: true },
});
const block = (r.content as Array<{ type: string; text: string }>)[0]!;
const parsed = JSON.parse(block.text);
expect(parsed.outcome).toBe('already_up_to_date');
expect(parsed.branchDeleted).toBe(true);
});

it('calls the query tool and preserves opaque param keys', async () => {
const { client } = await setup();
const r = await client.callTool({
Expand Down
36 changes: 35 additions & 1 deletion packages/sdk/src/errors.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import type { ErrorCode, ErrorOutput, ManifestConflict, MergeConflict } from './types';
import type {
ErrorCode,
ErrorOutput,
ManifestConflict,
MergeConflict,
ReadSetConflict,
RecoveryRequired,
} from './types';

export interface OmnigraphErrorContext {
status: number;
Expand Down Expand Up @@ -44,17 +51,43 @@ export class ConflictError extends OmnigraphError {
* Refresh and retry.
*/
readonly manifestConflict?: ManifestConflict;
/**
* Set when the conflict is a coarse read-set rejection: `member` (a branch
* identifier, graph head, or schema identity the write was prepared
* against) changed between preparation and commit. Refresh and retry.
*/
readonly readSetConflict?: ReadSetConflict;

constructor(ctx: OmnigraphErrorContext) {
super(ctx);
const body = ctx.body as ErrorOutput | undefined;
this.mergeConflicts = body?.mergeConflicts;
this.manifestConflict = body?.manifestConflict ?? undefined;
this.readSetConflict = body?.readSetConflict ?? undefined;
}
}

export class TooManyRequestsError extends OmnigraphError {}
export class InternalServerError extends OmnigraphError {}

/**
* 503 with a `recovery_required` body: a durable recovery intent overlaps the
* requested operation and must be resolved (typically by the server's next
* write or a read-write reopen) before a retry can succeed. Carries the
* blocking recovery `operationId`. The body deliberately has no `code` —
* `ErrorCode` is a closed wire contract, so the meaning rides in the
* structured field.
*/
export class ServiceUnavailableError extends OmnigraphError {
readonly recoveryRequired?: RecoveryRequired;

constructor(ctx: OmnigraphErrorContext) {
super(ctx);
const body = ctx.body as ErrorOutput | undefined;
this.recoveryRequired = body?.recoveryRequired ?? undefined;
}
}

export class NetworkError extends OmnigraphError {}

/**
Expand Down Expand Up @@ -88,6 +121,7 @@ const statusToClass: Record<number, new (ctx: OmnigraphErrorContext) => Omnigrap
409: ConflictError,
429: TooManyRequestsError,
500: InternalServerError,
503: ServiceUnavailableError,
};

export function fromResponse(args: {
Expand Down
2 changes: 2 additions & 0 deletions packages/sdk/src/generated/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ export {
type QueryRequest,
type ReadOutput,
type ReadRequest,
type ReadSetConflictOutput,
type ReadTargetOutput,
type RecoveryRequiredOutput,
type SchemaApplyOutput,
type SchemaApplyRequest,
type SchemaOutput,
Expand Down
82 changes: 79 additions & 3 deletions packages/sdk/src/generated/types.gen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,31 @@ export type BranchMergeOutcome =

export type BranchMergeOutput = {
actor_id?: string | null;
/**
* Why the requested source-branch deletion did not happen. Present iff
* `branch_deleted` is `false`.
*/
branch_delete_error?: string | null;
/**
* Result of the requested post-merge source-branch deletion. Absent when
* `delete_branch` was not requested; `true` when the source branch was
* deleted; `false` when the deletion was refused or failed (the merge
* itself still succeeded — see `branch_delete_error`).
*/
branch_deleted?: boolean | null;
outcome: BranchMergeOutcome;
source: string;
target: string;
};

export type BranchMergeRequest = {
/**
* Delete the source branch after a successful merge. The deletion runs
* under its own `branch_delete` policy check; a refusal or failure is
* reported via `branch_deleted` / `branch_delete_error` on the response
* and never fails the already-landed merge.
*/
delete_branch?: boolean;
/**
* Source branch whose commits will be merged.
*/
Expand Down Expand Up @@ -126,6 +145,8 @@ export type ErrorOutput = {
error: string;
manifest_conflict?: null | ManifestConflictOutput;
merge_conflicts?: Array<MergeConflictOutput>;
read_set_conflict?: null | ReadSetConflictOutput;
recovery_required?: null | RecoveryRequiredOutput;
};

export type ExportRequest = {
Expand Down Expand Up @@ -443,11 +464,26 @@ export type ReadRequest = {
snapshot?: string | null;
};

/**
* Structured authority mismatch for an RFC-022 prepared write. Values are
* strings because members include optional graph commit ids and future
* authority tokens, not only numeric table versions.
*/
export type ReadSetConflictOutput = {
actual?: string | null;
expected?: string | null;
member: string;
};

export type ReadTargetOutput = {
branch?: string | null;
snapshot?: string | null;
};

export type RecoveryRequiredOutput = {
operation_id: string;
};

export type SchemaApplyOutput = {
applied: boolean;
manifest_version: number;
Expand Down Expand Up @@ -597,6 +633,10 @@ export type ClusterCreateBranchErrors = {
* Per-actor admission cap exceeded; honor `Retry-After` header
*/
429: ErrorOutput;
/**
* An overlapping durable recovery intent must be resolved before retry
*/
503: ErrorOutput;
};

export type ClusterCreateBranchError =
Expand Down Expand Up @@ -645,6 +685,10 @@ export type ClusterMergeBranchesErrors = {
* Per-actor admission cap exceeded; honor `Retry-After` header
*/
429: ErrorOutput;
/**
* An overlapping durable recovery intent must be resolved before retry
*/
503: ErrorOutput;
};

export type ClusterMergeBranchesError =
Expand Down Expand Up @@ -693,6 +737,10 @@ export type ClusterDeleteBranchErrors = {
* Per-actor admission cap exceeded; honor `Retry-After` header
*/
429: ErrorOutput;
/**
* An overlapping durable recovery intent must be resolved before retry
*/
503: ErrorOutput;
};

export type ClusterDeleteBranchError =
Expand Down Expand Up @@ -734,13 +782,17 @@ export type ClusterChangeErrors = {
*/
403: ErrorOutput;
/**
* Merge conflict
* Write-authority conflict
*/
409: ErrorOutput;
/**
* Per-actor admission cap exceeded; honor `Retry-After` header
*/
429: ErrorOutput;
/**
* An overlapping durable recovery intent must be resolved before retry
*/
503: ErrorOutput;
};

export type ClusterChangeError = ClusterChangeErrors[keyof ClusterChangeErrors];
Expand Down Expand Up @@ -898,10 +950,18 @@ export type ClusterIngestErrors = {
* Forbidden
*/
403: ErrorOutput;
/**
* Prepared load authority changed before effects
*/
409: ErrorOutput;
/**
* Per-actor admission cap exceeded; honor `Retry-After` header
*/
429: ErrorOutput;
/**
* An overlapping durable recovery intent must be resolved before retry
*/
503: ErrorOutput;
};

export type ClusterIngestError = ClusterIngestErrors[keyof ClusterIngestErrors];
Expand Down Expand Up @@ -941,10 +1001,18 @@ export type ClusterLoadErrors = {
* Forbidden
*/
403: ErrorOutput;
/**
* Prepared load authority changed before effects
*/
409: ErrorOutput;
/**
* Per-actor admission cap exceeded; honor `Retry-After` header
*/
429: ErrorOutput;
/**
* An overlapping durable recovery intent must be resolved before retry
*/
503: ErrorOutput;
};

export type ClusterLoadError = ClusterLoadErrors[keyof ClusterLoadErrors];
Expand Down Expand Up @@ -985,13 +1053,17 @@ export type ClusterMutateErrors = {
*/
403: ErrorOutput;
/**
* Merge conflict
* Write-authority conflict
*/
409: ErrorOutput;
/**
* Per-actor admission cap exceeded; honor `Retry-After` header
*/
429: ErrorOutput;
/**
* An overlapping durable recovery intent must be resolved before retry
*/
503: ErrorOutput;
};

export type ClusterMutateError = ClusterMutateErrors[keyof ClusterMutateErrors];
Expand Down Expand Up @@ -1076,7 +1148,7 @@ export type ClusterInvokeQueryErrors = {
*/
404: ErrorOutput;
/**
* Merge conflict
* Stored mutation write-authority conflict
*/
409: ErrorOutput;
/**
Expand All @@ -1087,6 +1159,10 @@ export type ClusterInvokeQueryErrors = {
* Policy evaluation error (a denial is reported as 404, not 500)
*/
500: ErrorOutput;
/**
* A stored mutation is blocked by a durable recovery intent
*/
503: ErrorOutput;
};

export type ClusterInvokeQueryError =
Expand Down
3 changes: 3 additions & 0 deletions packages/sdk/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export {
ConflictError,
TooManyRequestsError,
InternalServerError,
ServiceUnavailableError,
NetworkError,
ConfigurationError,
} from './errors';
Expand Down Expand Up @@ -67,6 +68,8 @@ export type {
// Conflict / errors / shared
MergeConflict,
ManifestConflict,
ReadSetConflict,
RecoveryRequired,
ErrorOutput,
// Utility
Camelize,
Expand Down
6 changes: 6 additions & 0 deletions packages/sdk/src/resources/branches.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ export class BranchesResource {
* Merge `source` into `target` (defaults to `main`).
*
* **Idempotency**: a retry on the same merge yields `outcome: "already_up_to_date"`.
*
* **Cleanup**: `deleteBranch: true` deletes `source` after the merge lands
* (requires `branch_delete` permission in addition to `branch_merge`). A
* deletion refusal never fails the call — the merge is durable by then —
* so check `branchDeleted` / `branchDeleteError` on the result. It runs on
* every successful outcome, including `already_up_to_date`.
*/
merge(input: BranchMergeInput, opts: CallOptions = {}): Promise<BranchMerge> {
return this.t.request<BranchMerge>('POST', '/branches/merge', {
Expand Down
4 changes: 4 additions & 0 deletions packages/sdk/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ import type {
QueryCatalogEntry as QueryCatalogEntryOutput,
QueryRequest,
ReadOutput,
ReadSetConflictOutput,
ReadTargetOutput,
RecoveryRequiredOutput,
SchemaApplyOutput,
SchemaApplyRequest,
SchemaOutput,
Expand Down Expand Up @@ -81,6 +83,8 @@ export type Snapshot = Camelize<SnapshotOutput>;
export type SnapshotTable = Camelize<SnapshotTableOutput>;
export type MergeConflict = Camelize<MergeConflictOutput>;
export type ManifestConflict = Camelize<ManifestConflictOutput>;
export type ReadSetConflict = Camelize<ReadSetConflictOutput>;
export type RecoveryRequired = Camelize<RecoveryRequiredOutput>;

// Inputs (requests): camelCase from the caller, converted to snake_case on the wire.
export type BranchCreateInput = Camelize<BranchCreateRequest>;
Expand Down
Loading
Loading