Skip to content

Commit 43d3f27

Browse files
jpgough-msrocketstack-matt
jpgough-ms
and
rocketstack-matt
authored
Missing MCP Tools: Update Architecture (#2426)
* feat(calm-hub): add updateArchitecture MCP tool * test(calm-hub): add integration tests for updateArchitecture MCP tool * chore: EasyCLA Failure * chore: EasyCLA Failure * fix(calm-hub): resolve MCP updateArchitecture regressions and security gaps Addresses 4 issues in the MCP updateArchitecture tool: 1. BLOCKER: name/description silently nulled on every update - Added optional name/description @ToolArgs (matches REST PUT shape) - Preserve existing values when caller omits them via findArchitectureSummary() - Added ArgumentCaptor verification in unit tests + post-update regression assertion in integration tests 2. Unsafe semantics: tool description said "publish" but implementation is upsert - Updated description to explicitly document upsert behavior - Documented legacy/backwards-compatibility intent - Clarified that overwrite can silently clobber published versions 3. Configuration gate bypass: allow.put.operations ignored by MCP tool - Added allowPutOperations @ConfigProperty (defaults to false) - Tool now refuses to execute when flag is false, matching REST PUT behavior - Prevents MCP from flipping deployments mutation posture by default - Integration profiles enable flag for test suite via getConfigOverrides() 4. Unbounded JSON payload on update AND pre-existing gap on create - Added validateMaxLength(architectureJson, MAX_JSON_PAYLOAD_LENGTH) to updateArchitecture - Closed pre-existing gap in createArchitecture with same validation - Tests cover both paths All 46 unit tests pass. --------- Co-authored-by: Matthew Bain <66839492+rocketstack-matt@users.noreply.github.com>
1 parent e7bb3ab commit 43d3f27

7 files changed

Lines changed: 405 additions & 47 deletions

File tree

‎calm-hub/src/integration-test/java/integration/IntegrationTestProfile.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.quarkus.test.common.QuarkusTestResource;
44
import io.quarkus.test.junit.QuarkusTestProfile;
55

6+
import java.util.Map;
67
import java.util.Set;
78

89
@QuarkusTestResource(EndToEndResource.class)
@@ -19,5 +20,12 @@ public String getConfigProfile() {
1920
// Optional: specify a custom profile name if needed
2021
return "integration-test";
2122
}
23+
24+
@Override
25+
public Map<String, String> getConfigOverrides() {
26+
// Enable the PUT/upsert paths so the MCP updateArchitecture tool and the
27+
// equivalent REST PUT endpoint can be exercised by the integration suite.
28+
return Map.of("allow.put.operations", "true");
29+
}
2230
}
2331

‎calm-hub/src/integration-test/java/integration/MongoMcpIntegration.java‎

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -368,10 +368,52 @@ void mcp_validation_rejects_decorator_type_filter_with_newline() {
368368
assertThat(text(result), containsString("Type filter"));
369369
}
370370

371-
// --- Control Tools (create paths) ---
371+
// --- updateArchitecture ---
372372

373373
@Test
374374
@Order(26)
375+
void mcp_update_architecture_publishes_new_version() {
376+
ToolResponse result = architectureTools.updateArchitecture(
377+
"finos", createdArchitectureId, "1.1.0", "{\"name\": \"mcp-test-architecture-updated\"}", null, null);
378+
assertThat(result.isError(), is(false));
379+
assertThat(text(result), containsString("updated successfully"));
380+
assertThat(text(result), containsString("1.1.0"));
381+
}
382+
383+
@Test
384+
@Order(27)
385+
void mcp_list_architecture_versions_includes_updated_version() {
386+
ToolResponse result = architectureTools.listArchitectureVersions("finos", createdArchitectureId);
387+
assertThat(result.isError(), is(false));
388+
assertThat(text(result), containsString("1.0.0"));
389+
assertThat(text(result), containsString("1.1.0"));
390+
}
391+
392+
@Test
393+
@Order(28)
394+
void mcp_list_architectures_preserves_name_after_update() {
395+
// Regression guard: prior to this change updateArchitecture silently nulled the
396+
// architecture's name and description, so listArchitectures would fall back to
397+
// "Architecture <id>" instead of the original "MCP Test Arch".
398+
ToolResponse result = architectureTools.listArchitectures("finos");
399+
assertThat(result.isError(), is(false));
400+
assertThat(text(result), containsString("MCP Test Arch"));
401+
assertThat(text(result), containsString("Integration test architecture"));
402+
}
403+
404+
@Test
405+
@Order(29)
406+
void mcp_update_architecture_returns_error_for_nonexistent_architecture() {
407+
ToolResponse result = architectureTools.updateArchitecture(
408+
"finos", 999999, "1.1.0", "{\"name\": \"ghost\"}", null, null);
409+
assertThat(result.isError(), is(true));
410+
assertThat(text(result), containsString("not found"));
411+
}
412+
413+
// --- Control Tools (create paths) ---
414+
415+
@Test
416+
@Order(30)
375417
void mcp_create_control_requirement() {
376418
ToolResponse result = controlTools.createControlRequirement(
377419
"security", "MCP Test Control", "Integration test control requirement", CONTROL_REQUIREMENT_JSON);
@@ -385,31 +427,31 @@ void mcp_create_control_requirement() {
385427
}
386428

387429
@Test
388-
@Order(27)
430+
@Order(31)
389431
void mcp_list_controls_contains_created() {
390432
ToolResponse result = controlTools.listControls("security");
391433
assertThat(result.isError(), is(false));
392434
assertThat(text(result), containsString("MCP Test Control"));
393435
}
394436

395437
@Test
396-
@Order(28)
438+
@Order(32)
397439
void mcp_list_control_versions_after_create() {
398440
ToolResponse result = controlTools.listControlVersions("security", createdControlId);
399441
assertThat(result.isError(), is(false));
400442
assertThat(text(result), containsString("1.0.0"));
401443
}
402444

403445
@Test
404-
@Order(29)
446+
@Order(33)
405447
void mcp_get_control_after_create() {
406448
ToolResponse result = controlTools.getControl("security", createdControlId, "1.0.0");
407449
assertThat(result.isError(), is(false));
408450
assertThat(text(result), containsString("mcp-test-control"));
409451
}
410452

411453
@Test
412-
@Order(30)
454+
@Order(34)
413455
void mcp_create_control_configuration() {
414456
ToolResponse result = controlTools.createControlConfiguration(
415457
"security", createdControlId, CONTROL_CONFIGURATION_JSON);
@@ -418,7 +460,7 @@ void mcp_create_control_configuration() {
418460
}
419461

420462
@Test
421-
@Order(31)
463+
@Order(35)
422464
void mcp_create_control_configuration_for_missing_control_returns_error() {
423465
ToolResponse result = controlTools.createControlConfiguration(
424466
"security", 99999, CONTROL_CONFIGURATION_JSON);
@@ -427,7 +469,7 @@ void mcp_create_control_configuration_for_missing_control_returns_error() {
427469
}
428470

429471
@Test
430-
@Order(32)
472+
@Order(36)
431473
void mcp_create_control_requirement_rejects_invalid_json() {
432474
ToolResponse result = controlTools.createControlRequirement(
433475
"security", "Bad", "desc", "not-json");

‎calm-hub/src/integration-test/java/integration/NitriteIntegrationTestProfile.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.quarkus.test.junit.QuarkusTestProfile;
44

55
import java.util.List;
6+
import java.util.Map;
67
import java.util.Set;
78

89
public class NitriteIntegrationTestProfile implements QuarkusTestProfile {
@@ -21,4 +22,9 @@ public String getConfigProfile() {
2122
public List<TestResourceEntry> testResources() {
2223
return List.of(new TestResourceEntry(NitriteEndToEndResource.class));
2324
}
25+
26+
@Override
27+
public Map<String, String> getConfigOverrides() {
28+
return Map.of("allow.put.operations", "true");
29+
}
2430
}

‎calm-hub/src/integration-test/java/integration/NitriteMcpIntegration.java‎

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -328,10 +328,52 @@ void mcp_validation_rejects_decorator_type_filter_with_newline() {
328328
assertThat(text(result), containsString("Type filter"));
329329
}
330330

331-
// --- Control Tools (create paths) ---
331+
// --- updateArchitecture ---
332332

333333
@Test
334334
@Order(26)
335+
void mcp_update_architecture_publishes_new_version() {
336+
ToolResponse result = architectureTools.updateArchitecture(
337+
"finos", createdArchitectureId, "1.1.0", "{\"name\": \"mcp-nitrite-architecture-updated\"}", null, null);
338+
assertThat(result.isError(), is(false));
339+
assertThat(text(result), containsString("updated successfully"));
340+
assertThat(text(result), containsString("1.1.0"));
341+
}
342+
343+
@Test
344+
@Order(27)
345+
void mcp_list_architecture_versions_includes_updated_version() {
346+
ToolResponse result = architectureTools.listArchitectureVersions("finos", createdArchitectureId);
347+
assertThat(result.isError(), is(false));
348+
assertThat(text(result), containsString("1.0.0"));
349+
assertThat(text(result), containsString("1.1.0"));
350+
}
351+
352+
@Test
353+
@Order(28)
354+
void mcp_list_architectures_preserves_name_after_update() {
355+
// Regression guard: prior to this change updateArchitecture silently nulled the
356+
// architecture's name and description, so listArchitectures would fall back to
357+
// "Architecture <id>" instead of the original "MCP Nitrite Arch".
358+
ToolResponse result = architectureTools.listArchitectures("finos");
359+
assertThat(result.isError(), is(false));
360+
assertThat(text(result), containsString("MCP Nitrite Arch"));
361+
assertThat(text(result), containsString("Nitrite integration test architecture"));
362+
}
363+
364+
@Test
365+
@Order(29)
366+
void mcp_update_architecture_returns_error_for_nonexistent_architecture() {
367+
ToolResponse result = architectureTools.updateArchitecture(
368+
"finos", 999999, "1.1.0", "{\"name\": \"ghost\"}", null, null);
369+
assertThat(result.isError(), is(true));
370+
assertThat(text(result), containsString("not found"));
371+
}
372+
373+
// --- Control Tools (create paths) ---
374+
375+
@Test
376+
@Order(30)
335377
void mcp_create_control_requirement() {
336378
ToolResponse result = controlTools.createControlRequirement(
337379
"security", "MCP Nitrite Control", "Nitrite integration test control requirement", CONTROL_REQUIREMENT_JSON);
@@ -345,31 +387,31 @@ void mcp_create_control_requirement() {
345387
}
346388

347389
@Test
348-
@Order(27)
390+
@Order(31)
349391
void mcp_list_controls_contains_created() {
350392
ToolResponse result = controlTools.listControls("security");
351393
assertThat(result.isError(), is(false));
352394
assertThat(text(result), containsString("MCP Nitrite Control"));
353395
}
354396

355397
@Test
356-
@Order(28)
398+
@Order(32)
357399
void mcp_list_control_versions_after_create() {
358400
ToolResponse result = controlTools.listControlVersions("security", createdControlId);
359401
assertThat(result.isError(), is(false));
360402
assertThat(text(result), containsString("1.0.0"));
361403
}
362404

363405
@Test
364-
@Order(29)
406+
@Order(33)
365407
void mcp_get_control_after_create() {
366408
ToolResponse result = controlTools.getControl("security", createdControlId, "1.0.0");
367409
assertThat(result.isError(), is(false));
368410
assertThat(text(result), containsString("mcp-nitrite-control"));
369411
}
370412

371413
@Test
372-
@Order(30)
414+
@Order(34)
373415
void mcp_create_control_configuration() {
374416
ToolResponse result = controlTools.createControlConfiguration(
375417
"security", createdControlId, CONTROL_CONFIGURATION_JSON);
@@ -378,7 +420,7 @@ void mcp_create_control_configuration() {
378420
}
379421

380422
@Test
381-
@Order(31)
423+
@Order(35)
382424
void mcp_create_control_configuration_for_missing_control_returns_error() {
383425
ToolResponse result = controlTools.createControlConfiguration(
384426
"security", 99999, CONTROL_CONFIGURATION_JSON);
@@ -387,7 +429,7 @@ void mcp_create_control_configuration_for_missing_control_returns_error() {
387429
}
388430

389431
@Test
390-
@Order(32)
432+
@Order(36)
391433
void mcp_create_control_requirement_rejects_invalid_json() {
392434
ToolResponse result = controlTools.createControlRequirement(
393435
"security", "Bad", "desc", "not-json");

‎calm-hub/src/main/java/org/finos/calm/mcp/tools/ArchitectureTools.java‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,16 @@ public class ArchitectureTools {
3131
@ConfigProperty(name = "calm.mcp.enabled", defaultValue = "true")
3232
boolean mcpEnabled;
3333

34+
/**
35+
* Mirrors the gate applied to {@code PUT /architectures/{id}/versions/{version}}
36+
* in {@link org.finos.calm.resources.ArchitectureResource}. When false, the
37+
* MCP {@code updateArchitecture} tool refuses to overwrite an existing version,
38+
* matching the deployment's REST mutation posture.
39+
*/
40+
@Inject
41+
@ConfigProperty(name = "allow.put.operations", defaultValue = "false")
42+
boolean allowPutOperations;
43+
3444
@Inject
3545
ArchitectureStore architectureStore;
3646

@@ -129,6 +139,95 @@ public ToolResponse getArchitecture(
129139
}
130140
}
131141

142+
@Tool(description = "Publish or overwrite an architecture version against an existing architecture ID. " +
143+
"This is an upsert: if the supplied version already exists for the architecture it will be replaced, " +
144+
"otherwise it is added as a new version. Provided primarily for legacy/backwards-compatibility flows " +
145+
"that retain a stable architecture ID across versions. Disabled by default - requires the deployment " +
146+
"to set 'allow.put.operations=true' (the same flag that gates the equivalent REST PUT). " +
147+
"If 'name' or 'description' are omitted, the architecture's existing values are preserved; " +
148+
"supplying them will overwrite the stored values for that architecture ID.")
149+
public ToolResponse updateArchitecture(
150+
@ToolArg(description = "The namespace containing the architecture") String namespace,
151+
@ToolArg(description = "The architecture ID to publish a new version for (positive integer)") int architectureId,
152+
@ToolArg(description = "The version string to publish or overwrite (e.g. '1.1.0')") String version,
153+
@ToolArg(description = "The full CALM architecture JSON content for this version") String architectureJson,
154+
@ToolArg(description = "Optional new architecture name. When omitted the existing name is preserved.", required = false) String name,
155+
@ToolArg(description = "Optional new architecture description. When omitted the existing description is preserved.", required = false) String description) {
156+
Optional<ToolResponse> err = McpValidationHelper.firstError(
157+
() -> McpValidationHelper.checkEnabled(mcpEnabled),
158+
() -> McpValidationHelper.validateNamespace(namespace),
159+
() -> McpValidationHelper.validatePositiveId(architectureId, "Architecture ID"),
160+
() -> McpValidationHelper.validateVersion(version),
161+
() -> McpValidationHelper.validateNotBlank(architectureJson, "Architecture JSON"),
162+
() -> McpValidationHelper.validateMaxLength(architectureJson, McpValidationHelper.MAX_JSON_PAYLOAD_LENGTH, "Architecture JSON"),
163+
() -> McpValidationHelper.validateJson(architectureJson, "Architecture JSON"),
164+
() -> McpValidationHelper.validateMaxLength(name, McpValidationHelper.MAX_NAME_LENGTH, "Architecture name"),
165+
() -> McpValidationHelper.validateDescriptionLength(description, "Architecture description"));
166+
if (err.isPresent()) return err.get();
167+
168+
if (!allowPutOperations) {
169+
return ToolResponse.error("Error: Updating architecture versions is disabled on this CalmHub. " +
170+
"Set 'allow.put.operations=true' to enable overwriting an existing architecture version.");
171+
}
172+
173+
try {
174+
// Preserve existing name/description when caller does not supply them, otherwise the
175+
// store's unconditional $set on architectures.$.name and architectures.$.description
176+
// would silently null them out (causing list output to fall back to "Architecture <id>").
177+
String resolvedName = name;
178+
String resolvedDescription = description;
179+
if (resolvedName == null || resolvedDescription == null) {
180+
NamespaceArchitectureSummary existing = findArchitectureSummary(namespace, architectureId);
181+
if (existing != null) {
182+
if (resolvedName == null) {
183+
resolvedName = existing.getName();
184+
}
185+
if (resolvedDescription == null) {
186+
resolvedDescription = existing.getDescription();
187+
}
188+
}
189+
}
190+
191+
Architecture architecture = new Architecture.ArchitectureBuilder()
192+
.setNamespace(namespace)
193+
.setId(architectureId)
194+
.setVersion(version)
195+
.setName(resolvedName)
196+
.setDescription(resolvedDescription)
197+
.setArchitecture(architectureJson)
198+
.build();
199+
architectureStore.updateArchitectureForVersion(architecture);
200+
logger.info("Architecture [{}] updated with version [{}] in namespace [{}]", architectureId, version, namespace);
201+
return ToolResponse.success("Architecture " + architectureId + " updated successfully with version '" + version + "' in namespace '" + namespace + "'.");
202+
} catch (NamespaceNotFoundException e) {
203+
logger.warn("Namespace not found [{}]", namespace, e);
204+
return ToolResponse.error("Error: Namespace '" + namespace + "' not found.");
205+
} catch (ArchitectureNotFoundException e) {
206+
logger.warn("Architecture [{}] not found in namespace [{}]", architectureId, namespace, e);
207+
return ToolResponse.error("Error: Architecture " + architectureId + " not found in namespace '" + namespace + "'.");
208+
}
209+
}
210+
211+
/**
212+
* Looks up the existing summary (name/description) for an architecture so that the
213+
* update tool can preserve them when the caller does not supply replacements.
214+
* Returns {@code null} if the namespace cannot be loaded or the architecture is absent;
215+
* downstream {@code updateArchitectureForVersion} will surface those errors with the
216+
* correct user-facing message.
217+
*/
218+
private NamespaceArchitectureSummary findArchitectureSummary(String namespace, int architectureId) {
219+
try {
220+
for (NamespaceArchitectureSummary summary : architectureStore.getArchitecturesForNamespace(namespace)) {
221+
if (summary.getId() != null && summary.getId() == architectureId) {
222+
return summary;
223+
}
224+
}
225+
} catch (NamespaceNotFoundException e) {
226+
logger.debug("Namespace [{}] not found while resolving existing architecture summary", namespace);
227+
}
228+
return null;
229+
}
230+
132231
@Tool(description = "Create a new architecture in a namespace. Returns the allocated architecture ID and version.")
133232
public ToolResponse createArchitecture(
134233
@ToolArg(description = "The namespace to create the architecture in") String namespace,
@@ -141,6 +240,7 @@ public ToolResponse createArchitecture(
141240
() -> McpValidationHelper.validateMaxLength(name, McpValidationHelper.MAX_NAME_LENGTH, "Architecture name"),
142241
() -> McpValidationHelper.validateDescriptionLength(description, "Architecture description"),
143242
() -> McpValidationHelper.validateNotBlank(architectureJson, "Architecture JSON"),
243+
() -> McpValidationHelper.validateMaxLength(architectureJson, McpValidationHelper.MAX_JSON_PAYLOAD_LENGTH, "Architecture JSON"),
144244
() -> McpValidationHelper.validateJson(architectureJson, "Architecture JSON"));
145245
if (err.isPresent()) return err.get();
146246

0 commit comments

Comments
 (0)