MCP: add upsert_entity and upsert_relationship tools (#4864)

[robfrank]

What does this PR do?

Adds two MCP tools, upsert_entity and upsert_relationship, that perform injection-safe match-or-create-by-key via parameterized Cypher MERGE:

• upsert_entity - match (or create) a vertex by matchKeys, then apply setProperties.
• upsert_relationship - match (or create) a directed edge between two MERGEd endpoints, keyed by endpoints + relType, then apply relProperties in a trailing SET (so repeated calls update the edge, never duplicate it).

Every caller value is a bound Cypher parameter; identifiers (type name, relationship type, property keys) are backtick-quoted via a new MCPToolUtils.quoteIdentifier that rejects embedded backticks so the quoting can't be escaped. A shared MCPToolUtils.executeParameterizedWrite runs the generated Cypher through the same engine.analyze() -> ExecuteCommandTool.checkPermission() gating execute_command uses, executes in a transaction, and serializes identically. No engine changes; registration is centralized in MCPDispatcher.

Motivation

Part of Wave 1 of the MCP GraphRAG & Agent-Memory epic (#4859). Models reliably build match-key logic wrong when hand-writing execute_command (string-concatenated WHERE clauses, missing normalization) and duplicate nodes/edges as a result - the most common agent-memory failure mode. Wrapping match-or-create behind a parameterized tool is a concrete correctness win and removes the injection vector that interpolated match keys carry.

Design and plan are committed on the branch: docs/superpowers/specs/2026-07-15-mcp-upsert-tools-4864-design.md and docs/superpowers/plans/2026-07-15-mcp-upsert-tools-4864.md.

Related issues

• Closes MCP: add upsert_entity and upsert_relationship tools #4864
• Part of epic Epic: MCP GraphRAG & Agent-Memory Surface #4859 (Wave 1)

Additional Notes

• Permission model: any MERGE analyzes to {CREATE, UPDATE} in ArcadeDB's OpenCypher engine (OpenCypherQueryEngine.getOperationTypes() adds both whenever hasMerge() is true, regardless of SET), so every upsert requires both allowInsert and allowUpdate. Matches the issue's "require both as a single unit" intent and the pre-existing MCPPermissionsTest.upsertRequiresBothCreateAndUpdate stub. Safe-by-default is unchanged (no new flag; write flags default false).
• Type auto-creation under allowInsert is intentional (ArcadeDB MERGE auto-creates the vertex/edge type), for incremental graph building - documented in the spec.
• Field naming: the entity type argument is typeName, not type, to avoid the JSON-Schema type keyword collision (same precedent as full_text_search MCP: add full_text_search tool #4862). Issue MCP: add upsert_entity and upsert_relationship tools #4864's schema text should be amended to match.
• Docs: user-facing MCP docs live out-of-tree in arcadedb-docs; a separate follow-up, as with MCP: add full_text_search tool #4862.
• Verification: mvn -pl server test -Dtest='MCP*' -> 114/114 green (a full mvn clean package was not run locally; CI will cover it). Tests exercise the running server end-to-end and cover idempotency, no-duplicate edges (incl. the edge property actually updating), endpoint auto-creation, injection inertness (x'}) DETACH DELETE n // stored as a literal, deletes nothing), permission denial naming the missing flag, empty-matchKeys rejection, and identifier validation. Tool count 11 -> 13 in both transports.
• Two cosmetic items deferred: argument-validation ordering; a small match-key loop duplicated across the two tools.

Checklist

• I have run the build using mvn clean package command (ran the targeted mvn -pl server test -Dtest='MCP*' suite, 114/114 green; full package build left to CI)
• My unit tests cover both failure and success scenarios

🤖 Generated with Claude Code

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 8 complexity

Metric	Results
Complexity	8

View in Codacy

🟢 Coverage 97.14% diff coverage · -6.94% coverage variation

Metric	Results
Coverage variation	✅ -6.94% coverage variation
Diff coverage	✅ 97.14% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (2f3ff7f)	140111	104363	74.49%
Head commit (89da34e)	172152 (+32041)	116289 (+11926)	67.55% (-6.94%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5301)	140	136	97.14%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[gemini-code-assist]

Code Review

This pull request implements two new MCP tools, upsert_entity and upsert_relationship, to perform injection-safe match-or-create operations using parameterized Cypher MERGE statements. It introduces a shared utility helper for parameterized writes and identifier quoting, registers the tools in the dispatcher, and adds comprehensive integration and unit tests. The review feedback correctly points out that the database parameter in both tools is retrieved without defensive validation, which could cause a JSONException instead of a clean IllegalArgumentException if omitted by the client.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The database parameter is retrieved using args.getString("database") without a default value or validation. If the client omits this parameter, it will throw a JSONException instead of a clean IllegalArgumentException. It is safer to retrieve it with a default value of null and validate it defensively, consistent with how typeName is handled.

Suggested change

	final String databaseName = args.getString("database");
	final String databaseName = args.getString("database", null);
	if (databaseName == null || databaseName.isBlank())
	throw new IllegalArgumentException("'database' is required");

[robfrank]

Applied. These tools already validate every other required arg (typeName/matchKeys) defensively, and resolveDatabase returns agent-friendly errors, so a raw getString throw on missing database was the odd one out. Now: getString("database", null) + null/blank guard -> clean IllegalArgumentException. (725ff4a)

[gemini-code-assist]

The database parameter is retrieved using args.getString("database") without validation. Since a require helper method is already defined in this class, we should reuse it to ensure consistent validation and throw a clean IllegalArgumentException if the parameter is missing or blank.

Suggested change

	final String databaseName = args.getString("database");
	final String databaseName = require(args, "database");

[robfrank]

Applied - reused the existing require() helper: final String databaseName = require(args, "database");. (725ff4a)

[claude]

Code Review: MCP upsert_entity / upsert_relationship

Nice, focused PR. The core design is exactly right: every caller-supplied value is a bound Cypher parameter, and the only things interpolated into the query text (type/rel/property identifiers) go through quoteIdentifier, which backtick-wraps and rejects embedded backticks so the quoting can't be escaped. Reusing engine.analyze() -> ExecuteCommandTool.checkPermission() keeps the permission axis identical to execute_command, and I confirmed the "requires both insert and update" claim against OpenCypherQueryEngine.getOperationTypes() (lines 127-130): any hasMerge() adds both CREATE and UPDATE regardless of a trailing SET, and MERGE never routes through the READ branch, so these tools correctly need allowInsert + allowUpdate and not allowReads. Good, server-layer-only footprint with centralized registration in MCPDispatcher.

Strengths

• Injection surface is genuinely closed: values bound, identifiers quoted + backtick-rejected, and the injection-inertness test (x'}) DETACH DELETE n // stored as a literal, deletes nothing) exercises it end-to-end.
• Parameter naming avoids collisions cleanly (m*/s* for entity; f*/t*/r* for relationship endpoints + edge props).
• Strong idempotency coverage: no-duplicate node, no-duplicate edge and the edge property actually updating on the second call (1843 -> 1844), endpoint auto-creation, no-SET path, permission denial naming the missing flag, empty-matchKeys rejection, plus quoteIdentifier unit tests.

Suggestions (all minor / non-blocking)

1. Non-scalar property values. matchKeys.get(key) / setProperties.get(key) return whatever the JSON holds - if an agent passes a nested object/array (e.g. setProperties: {"address": {"city":"Turin"}}), a JSONObject/JSONArray instance is placed into the bound-parameter map and handed to database.command("cypher", ...). Worth confirming the Cypher param binder converts these to a Map/List (vs. storing a toString() or throwing). Since these tools accept an arbitrary caller-supplied param map - new surface vs. execute_command - consider either documenting "values should be scalars" in the schema descriptions or normalizing nested JSON before binding. A matchKeys value of JSON null is a similar edge case (becomes JSONObject.NULL).

2. Duplicated match-key loop + inconsistent validation helpers (you already flagged the first). UpsertEntityTool inlines the merge-map loop and its own typeName/matchKeys checks, while UpsertRelationshipTool factors them into appendNodeMerge / require / requireKeys. Pulling the map-builder and the require/requireKeys helpers into MCPToolUtils would remove the duplication and make the two tools' error messages consistent.

3. Test gaps.

   • No coverage for upsert_relationship empty fromMatchKeys/toMatchKeys rejection (requireKeys) - only the entity's empty-matchKeys path is tested.
   • No tool-level test that a backtick-bearing typeName/relType/property key is rejected (only the quoteIdentifier unit test covers it); a single HTTP round-trip asserting isError would lock in that the guard is actually wired into the tools.
   • The stdio transport only gets its count bumped to 13 - no functional upsert test there, which is acceptable given HTTP covers the logic.

4. Checklist / build. The PR notes mvn clean package wasn't run locally (only the targeted MCP* suite). Per CLAUDE.md ("after every change in the backend, compile the project"), worth a full reactor build before merge - CI will catch it, but the box is unchecked.

Overall this is a clean, well-tested, security-conscious addition that matches the existing MCP tool conventions. The suggestions above are polish, not blockers.

[claude]

Review: MCP upsert_entity / upsert_relationship (#4864)

Solid, well-scoped PR. Server-layer only, reuses the existing analyze() -> checkPermission() gating, and the injection defense is the right shape: every caller value is a bound Cypher parameter and every identifier goes through quoteIdentifier, which backtick-quotes and rejects embedded backticks so the quoting can't be escaped. Test coverage is genuinely good - idempotency, no-duplicate edge (incl. the property actually updating), endpoint auto-creation, injection inertness, permission denial, empty-matchKeys, no-SET path, and identifier validation. Nice work.

A few things worth considering before merge:

1. Idempotency is not guaranteed without a unique index (correctness + performance)

This is the main one. MergeStep.findAllNodes() uses an index only if one covers the match keys, and otherwise falls back to a full type scan (iterateType(label, true)). The two tools auto-create the vertex/edge type on first write but never create an index on the match keys, which has two consequences:

• Performance: each upsert against a type with no matching index is an O(n) scan. Building N nodes via repeated upserts becomes O(N^2) - directly against the "performance and lightweight on GC" mantra in CLAUDE.md, and precisely the agent-memory hot path this epic targets.
• Concurrency / the core promise: without a unique index, MERGE is match-then-create and is not atomic across concurrent transactions - two overlapping upserts with identical matchKeys can both create, producing the very duplicate the tool exists to prevent. (The retry-on-collision logic in MergeStep only kicks in when a UNIQUE index actually exists.)

Suggestion: at minimum document that no-duplicate/idempotent behavior is only guaranteed when a unique index exists on the match keys (and that large types without one scan fully). Better would be to ensure a unique index on the match-key set on first use, though that interacts with the allowSchemaChange point below and deserves its own decision.

2. Type auto-creation bypasses allowSchemaChange

Documented as intentional in the spec, and it matches what a hand-written MERGE through execute_command already does, so it isn't a regression. Still worth surfacing explicitly in the user-facing tool docs: an agent holding only allowInsert+allowUpdate (no allowSchemaChange) can grow the schema with new vertex/edge types through these tools. Reasonable for incremental graph building, but operators should not have to read the engine to discover it.

3. Non-scalar match/set values

matchKeys/setProperties values are read via JSONObject.get(key) and bound directly. A nested object/array value becomes a JSONObject/JSONArray bound parameter, whose behavior as a Cypher param is undefined. Low priority (agents will almost always pass scalars), but consider validating scalars or documenting the constraint so a nested value fails cleanly rather than surfacing an opaque engine error.

4. Duplicated match-key merge loop (minor, already noted in the PR)

The match-key {k: $p, ...} builder is duplicated between UpsertEntityTool.execute and UpsertRelationshipTool.appendNodeMerge. Extracting a shared MCPToolUtils.appendNodeMerge(...) would remove the drift risk. The PR already flags this as deferred - just confirming it is worth a follow-up.

Minor / non-blocking

• executeParameterizedWrite parses twice (engine.analyze(cypher) then database.command(cypher, params)). This mirrors ExecuteCommandTool for the Cypher path (where analyzed.execute() returns null anyway), so it is consistent - just noting it is not free.
• Permission model reads correctly: getOperationTypes() classifies any MERGE as {CREATE, UPDATE} (no READ added for the trailing RETURN), so both allowInsert and allowUpdate are required and allowReads is not - matching the description. Good that the doc was corrected to reflect this.
• Tests assert contains("not allowed") against checkPermission's "... are not allowed by MCP configuration" - a light coupling to message text, acceptable.

Nothing here blocks the injection/permission fundamentals, which are sound. Item #1 is the one I'd most want addressed (or at least explicitly documented) before this ships, since it bears directly on the tool's stated correctness guarantee.

Reviewed against CLAUDE.md conventions. Automated review; please verify findings.

[robfrank]

Addressed in 6cc243d:

1. Non-scalar property values. Confirmed com.arcadedb.serializer.json.JSONObject implements Map<String,Object> (a nested object binds as a map), but JSONArray is only Iterable (not List) and JSON null becomes JSONObject.NULL - so arrays/null are the ambiguous cases. Rather than add nested-JSON normalization (unusual for match keys, YAGNI), I set the contract in the schema descriptions: matchKeys/setProperties/fromMatchKeys/toMatchKeys/relProperties now say "Values should be scalars".

2. Duplication + inconsistent helpers. Lifted requireString / requireNonEmptyObject and the match-key merge-map builder (appendNodeMerge) into MCPToolUtils. Both tools now use them, so validation, error messages, and the merge-map construction are shared and consistent. UpsertRelationshipTool's private require/requireKeys/appendNodeMerge are gone.

3. Test gaps. Added upsertRelationshipRejectsEmptyMatchKeys (empty fromMatchKeys -> isError, names the field) and upsertEntityRejectsBacktickIdentifier (a typeName of Bad`Type -> isError, "backtick"), the latter a single HTTP round-trip proving the quoteIdentifier guard is wired into the tool path. Left the stdio transport at count-only, as you noted HTTP covers the logic.

4. Full build. Left to CI as you note; locally verified the full MCP suite (116/116 green after these changes).

[robfrank]

Verified all four against the engine and addressed them (641f2f6, plus 6cc243d earlier):

1. Idempotency requires a UNIQUE index - confirmed and now documented. Checked MergeStep: the match uses tryFindByIndex only when an index covers the keys, else iterateType(label, true) (full scan, line ~1078), and the match-or-create atomicity comes solely from the catch (DuplicatedKeyException) -> re-match path (line ~270-285), which a UNIQUE index is what raises. So without one: O(n) per call and concurrent upserts with identical keys can both create. Both tool descriptions now state this, and the spec's Risks section documents the O(n^2)/concurrency consequences. Auto-creating a unique index on first use is left as a follow-up decision (it interacts with allowSchemaChange and with match-key-set variability), not folded into this PR.

2. Type auto-creation bypassing allowSchemaChange. Intentional, as you note. The tool descriptions already say the type is "Created automatically if it does not exist"; the operator-facing note belongs in the out-of-tree arcadedb-docs (tracked with the rest of the MCP docs follow-up, same as #4862).

3. Non-scalar values. Addressed in 6cc243d: confirmed JSONObject implements Map (binds) but JSONArray is only Iterable and JSON null becomes JSONObject.NULL, so the value fields' descriptions now say "Values should be scalars" rather than adding normalization (YAGNI).

4. Duplicated merge loop. Addressed in 6cc243d: appendNodeMerge plus requireString/requireNonEmptyObject now live in MCPToolUtils and both tools use them; the private copies are gone.

Minors noted: the double parse mirrors ExecuteCommandTool (Cypher's analyzed.execute() returns null, so the command re-parses either way) - left as-is for consistency; the contains("not allowed") coupling is accepted.

[claude]

Review: upsert_entity / upsert_relationship MCP tools (#4864)

Reviewed the full diff and the surrounding MCP code. Overall this is a well-designed, defensively-written change. The parameterized-value + backtick-quoted-identifier split is the right architecture, the permission gating correctly reuses ExecuteCommandTool.checkPermission, and test coverage is genuinely good (idempotency, no-duplicate edges with property update, endpoint auto-create, injection inertness, permission denial, empty-key rejection, backtick rejection). Nice work.

A few observations, mostly minor.

Security (looks solid)

• I confirmed the injection story end-to-end: every caller value is bound as a $param, and the only identifier path (quoteIdentifier) rejects embedded backticks - which is the sole escape character for a backtick-quoted Cypher identifier - so a clause genuinely cannot be injected. Newlines/braces/quotes inside an identifier are inert once backtick-wrapped. Good.
• I verified the permission claim against OpenCypherQueryEngine.getOperationTypes(): a MERGE ... RETURN analyzes to exactly {CREATE, UPDATE} (no READ is added), so the "Requires both insert and update permission" description is accurate and allowReads is correctly not required.

Design note worth surfacing (not a blocker)

• Type auto-creation bypasses allowSchemaChange. A MERGE (n:BrandNewType {...}) auto-creates the vertex/edge type, but that is classified as CREATE, not SCHEMA - so an operator who enabled allowInsert but deliberately left allowSchemaChange=false can still create brand-new types through these tools. This is consistent with how a raw MERGE behaves through execute_command, and the PR body flags it as intentional, so I'd just make sure it's called out in the user-facing docs follow-up, since the gate people expect (allowSchemaChange) is not the one that applies here.

Code quality (minor)

• Duplicated SET-clause loop. The trailing-SET builder is copy-pasted between UpsertEntityTool (lines 73-84) and UpsertRelationshipTool (lines 96-108) with only the variable/prefix differing. Since appendNodeMerge already lives in MCPToolUtils, a sibling appendSetClause(cypher, params, variable, properties, prefix) would remove the duplication and keep the quoting/param logic in one place. The PR already acknowledges this as deferred - flagging so it doesn't get lost.
• Non-scalar / non-object arg robustness. matchKeys/setProperties values are documented as "should be scalars" but not enforced; a nested object/array value would surface as a raw engine error rather than a clean self-correcting message. Similarly, if a caller passes matchKeys as a string instead of an object, requireNonEmptyObject's getJSONObject(field, null) behavior on a type mismatch is worth a quick check. Low priority, since the LLM-facing schema declares type: object.

Test coverage (small gaps)

• No test exercises a composite (multi-key) matchKeys - the appendNodeMerge loop's i > 0 branch and param-prefix disambiguation (m0, m1) are only covered for single-key inputs. A two-key upsert would lock that in.
• No direct unit test for appendNodeMerge / executeParameterizedWrite in MCPToolUtilsTest (only quoteIdentifier). The end-to-end tests cover the behavior, so this is optional.
• Injection is tested through a match-key value, but not through a setProperties value - same binding mechanism, so likely fine, but a one-line addition would close the loop.

Build

• PR notes the full mvn clean package was left to CI (only mvn -pl server test -Dtest='MCP*' ran locally). Worth confirming CI is green before merge, per the repo guideline to compile the whole project after backend changes.

Nothing here is blocking from a correctness or security standpoint. The duplicated-loop and composite-key test would be my top two nice-to-haves.

[robfrank]

Addressed the top items in 89da34e:

• Duplicated SET-clause loop. Extracted MCPToolUtils.appendSetClause(cypher, params, variable, properties, prefix) alongside appendNodeMerge; both tools now call it, so the SET construction lives in one place with the quoting/binding logic.
• Composite match keys - test added. upsertEntityCompositeMatchKeysIsIdempotent upserts with a two-key matchKeys (firstName+lastName) twice and asserts a single node, exercising the i > 0 branch and the m0/m1 param disambiguation.
• setProperties-value injection - test added. upsertEntityBindsSetPropertyValuesSoInjectionIsInert sets a note of '}) DETACH DELETE n // and asserts the node still exists with that literal value, proving the SET value is bound, not executed.

On the others: allowSchemaChange/type-auto-creation is documented in the descriptions + spec and the operator-facing note goes in the arcadedb-docs follow-up; a bad-type matchKeys (e.g. a string) hits getJSONObject(field, null) which returns null on a type mismatch, so requireNonEmptyObject already yields the clean "required and must contain at least one property" error; the double-parse mirrors ExecuteCommandTool (unavoidable on the Cypher path) and stays for consistency. Full MCP suite 118/118 green; full reactor build left to CI.

[claude]

Review: MCP upsert_entity / upsert_relationship tools (#4864)

Overall this is a clean, well-scoped, and well-tested PR. The parameterized-MERGE approach is the right design, the injection surface is correctly reduced to identifiers-only, and the identifier guard (reject any embedded backtick) is airtight since a backtick is the only character that could break out of a `...` quoted name in Cypher. Reuse of the existing analyze() -> checkPermission() gating keeps the security model consistent with execute_command. Nice work.

A few observations, mostly for discussion - none are blockers.

Security / permission model

• Type auto-creation bypasses allowSchemaChange. A MERGE on a non-existent typeName / relType implicitly creates the vertex/edge type, but analyze() classifies MERGE as {CREATE, UPDATE} (never SCHEMA). So an operator who granted allowInsert+allowUpdate but deliberately withheld allowSchemaChange can still cause new types/labels to be materialized in the schema through these tools. The PR documents this as intentional, and it is defensible for incremental graph building, but it's worth calling out explicitly in the tool description (not just the design spec) so operators configuring permissions understand the interaction. Right now the schema-change gate is silently not consulted.
• Create-only is impossible. Because any MERGE analyzes to both CREATE and UPDATE, a user with allowInsert but not allowUpdate cannot use upsert at all, even for a brand-new node where nothing is updated. This matches the issue's "both as a unit" intent, so fine - just noting the tradeoff.

Robustness

• No scalar validation on matchKeys / setProperties values. The schema descriptions say "Values should be scalars" but nothing enforces it. matchKeys.get(key) passes the raw value straight into the parameter map, so a nested object/array reaches the Cypher binder and can surface as a confusing low-level error rather than a clean "value must be scalar" message. Given the audience is LLM agents that will occasionally pass the wrong shape, a small validation with a self-correcting error message would improve the UX and is consistent with the clean-error philosophy you already applied to requireString / requireNonEmptyObject. Optional.

Performance

• Each call parses the generated Cypher twice: once in engine.analyze() and again in database.command("cypher", ...). This mirrors ExecuteCommandTool's cypher path (where analyzed.execute() returns null), so it's consistent, and MCP call frequency makes it immaterial - just flagging that the double-parse is inherent here.

Tests

Strong coverage: idempotency, no-SET path, injection inertness (with the stored-literal assertion), edge no-duplicate and property-update, endpoint auto-creation, both permission-denial directions, empty match-keys, and the quoteIdentifier unit tests including the backtick-injection case. Two minor gaps to consider:

• No test asserting a multi-key matchKeys (2+ properties) merges on the full composite key - the parameter-prefixing logic is the interesting part and is currently only exercised with single-key maps.
• No test for the non-scalar-value path (relates to the validation note above).

Nits (already acknowledged in the PR description)

• The SET-clause build loop is duplicated between the two tools; could fold into a shared MCPToolUtils helper alongside appendNodeMerge.
• The mvn clean package checklist item is unchecked - worth confirming CI runs the full build since only the targeted MCP* suite was run locally.

Nothing here blocks merge; the security note (schema-change bypass) is the one I'd most want reflected in the user-facing tool description before this ships.

[claude]

Review: MCP upsert_entity / upsert_relationship tools

Reviewed the final state on the branch (server module only, no engine changes). Overall this is a well-scoped, high-quality PR: parameterized values, backtick-guarded identifiers, permission gating reused from execute_command, single registration point in MCPDispatcher, and genuinely thorough end-to-end tests. Nice work extracting requireString / requireNonEmptyObject / appendNodeMerge / appendSetClause into MCPToolUtils - that closes the "duplicated match-key loop" item the description deferred.

Strengths

• Injection safety is sound. Every caller value is a bound parameter; identifiers go through quoteIdentifier, which rejects embedded backticks so the quoting can't be escaped. Tests exercise the classic breakout payload (x'}) DETACH DELETE n //) both as a match-key value and as a set-property value and confirm it is stored as a literal - good, since those are the two distinct binding paths.
• Permission ordering is correct. executeParameterizedWrite calls checkPermission(...) before opening the transaction, so a denied upsert never leaves partially-created endpoints behind.
• Param prefixes avoid collisions across the two node merges (f*/t*) and the SET clause (s*/r*) - verified there is no overlap.
• Test coverage is strong: idempotency, composite keys, no-duplicate edge (incl. the edge property actually updating to 1844), the no-SET execution path, endpoint auto-creation, permission denial, empty-matchKeys rejection, and backtick rejection wired through the real tool path.

Notes / minor points (non-blocking)

1. Pure create still requires allowUpdate. Because any MERGE analyzes to {CREATE, UPDATE}, an operator who grants only allowInsert (insert-only, never modify) cannot use these tools at all - even for a first-time create with no setProperties (see upsertEntityWithoutSetPropertiesCreatesNode, which needs both flags). This is intentional and documented in the PR body, but it is a real least-privilege constraint worth surfacing in the user-facing docs follow-up, since "require both as a unit" is not obvious to someone reasoning about permissions.

2. null match-key values produce a raw Cypher runtime error. If a caller passes matchKeys: {email: null}, the value binds as null and Cypher rejects MERGE on a null property with an engine-level message rather than the clean, self-correcting IllegalArgumentException the other validation paths give. The tool descriptions already say "Values should be scalars"; a cheap explicit guard in appendNodeMerge (reject null / JSON-null match-key values) would keep error messages consistent and agent-friendly. Optional.

3. Double parse per call. executeParameterizedWrite runs engine.analyze(cypher) and then database.command("cypher", cypher, params), so the statement is parsed twice. execute_command avoids this for SQL by reusing analyzed.execute(...), but that path returns null for Cypher, so there is no easy reuse here today. Negligible for a single-row write; flagging only so it is a conscious choice.

4. Concurrency / full-scan cost is now called out directly in the tool descriptions (recommend a UNIQUE index on the match keys) - good addition. Worth making sure the docs follow-up repeats it, since the "concurrent upserts can both create without a unique index" caveat is the exact failure mode this tool is meant to prevent.

Build

The checklist notes mvn clean package was not run locally (targeted MCP* suite green, full build left to CI). Please make sure CI's full reactor build is green before merge, per the repo's verify/install guidance - the targeted -Dtest='MCP*' run does not cover cross-module packaging.

Nothing here blocks; the correctness-critical pieces (binding, identifier quoting, permission-before-write) are all correct and tested.

[codecov]

Codecov Report

❌ Patch coverage is 93.57143% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.60%. Comparing base (2f3ff7f) to head (89da34e).

Files with missing lines	Patch %	Lines
...va/com/arcadedb/server/mcp/tools/MCPToolUtils.java	90.19%	2 Missing and 3 partials ⚠️
...om/arcadedb/server/mcp/tools/UpsertEntityTool.java	93.75%	1 Missing and 1 partial ⚠️
...adedb/server/mcp/tools/UpsertRelationshipTool.java	96.15%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5301      +/-   ##
============================================
+ Coverage     65.55%   65.60%   +0.04%     
- Complexity      968      971       +3     
============================================
  Files          1712     1714       +2     
  Lines        140111   140251     +140     
  Branches      30013    30023      +10     
============================================
+ Hits          91854    92012     +158     
+ Misses        35789    35777      -12     
+ Partials      12468    12462       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.