luigidemasi commented on code in PR #128:
URL: https://github.com/apache/camel-kit/pull/128#discussion_r4366398085
##########
camel-kit-core/src/test/java/io/github/luigidemasi/camelkit/generator/OpenCodeGeneratorTest.java:
##########
@@ -814,4 +820,99 @@ void preservesBaseSkillBehavior() throws Exception {
// MCP config exists
assertTrue(Files.exists(tempDir.resolve("opencode.json")));
}
+
+ @Test
+ void regenerationWithoutCitrusRemovesPreviousCitrusEntries() throws
Exception {
+ new OpenCodeGenerator().generate(createContext("camel-kit", true));
+ Path configFile = tempDir.resolve("opencode.json");
+ String withCitrus = Files.readString(configFile);
+ assertTrue(withCitrus.contains("\"citrus\""), "citrus mcp must be
present after opt-in init");
+ assertTrue(withCitrus.contains("\"citrus_*\""), "citrus_* permission
must be present after opt-in init");
+
+ new OpenCodeGenerator().generate(createContext("camel-kit", false));
+ String withoutCitrus = Files.readString(configFile);
+ assertFalse(withoutCitrus.contains("\"citrus\""),
+ "citrus mcp must be removed after opt-out regeneration");
+ assertFalse(withoutCitrus.contains("\"citrus_*\""),
+ "citrus_* permission must be removed after opt-out
regeneration");
+ assertTrue(withoutCitrus.contains("\"camel\""), "camel mcp must
remain");
+ assertTrue(withoutCitrus.contains("\"camel_*\""), "camel_* permission
must remain");
+ }
+
+ @Test
+ void regenerationRemovesCitrusWhenPermissionsReorderedBeforeGenerated()
throws Exception {
+ new OpenCodeGenerator().generate(createContext("camel-kit", true));
+ Path configFile = tempDir.resolve("opencode.json");
+ String current = Files.readString(configFile);
+ assertTrue(current.contains("\"citrus_*\""), "citrus_* must be present
after opt-in init");
+ int camelPos = current.indexOf("\"camel_*\"");
+ int citrusPos = current.indexOf("\"citrus_*\"");
+ assertTrue(camelPos < citrusPos, "camel_* must appear before citrus_*
in generated output");
+
+ String reordered = current
+ .replaceFirst("\"camel_\\*\" : \"ask\",", "")
+ .replaceFirst("\"camel-knowledge_\\*\" : \"ask\",", "")
+ .replace("\"citrus_*\" : \"ask\"",
+ "\"citrus_*\" : \"ask\",\n \"camel_*\" : \"ask\",\n
\"camel-knowledge_*\" : \"ask\"");
+ assertTrue(reordered.indexOf("\"citrus_*\"") <
reordered.indexOf("\"camel_*\""),
+ "citrus_* must now appear before camel_*");
+ Files.writeString(configFile, reordered);
+
+ new OpenCodeGenerator().generate(createContext("camel-kit", false));
+ String result = Files.readString(configFile);
+ assertFalse(result.contains("\"citrus\""), "citrus mcp must be
removed");
+ assertFalse(result.contains("\"citrus_*\""), "citrus_* permission must
be removed");
+ assertTrue(result.contains("\"camel\""), "camel mcp must remain");
+ assertTrue(result.contains("\"camel_*\""), "camel_* permission must
remain");
+ }
+
+ @Test
+ void regenerationRemovesCitrusFromLowerLayerWithHigherLayer() throws
Exception {
+ Files.writeString(tempDir.resolve("opencode.json"), """
+ {
+ "permission": {"citrus_*": "ask", "camel_*": "ask"},
+ "mcp": {
+ "citrus": {"type": "local", "command": "jbang"},
+ "camel": {"type": "local", "command": "jbang"}
+ }
+ }
+ """);
+ Files.writeString(tempDir.resolve("opencode.jsonc"), """
+ {
+ "permission": {"custom_rule": "deny"},
+ "mcp": {"custom": {"type": "remote", "url":
"https://example.test/mcp"}}
+ }
+ """);
+
+ new OpenCodeGenerator().generate(createContext("camel-kit", false));
+
+ JsonNode lower = readOpenCodeConfig(tempDir.resolve("opencode.json"));
+ assertFalse(lower.path("mcp").has("citrus"), "citrus must be removed
from lower layer");
+ assertFalse(lower.path("permission").has("citrus_*"),
+ "citrus_* permission must be removed from lower layer");
+
+ JsonNode higher =
readOpenCodeConfig(tempDir.resolve("opencode.jsonc"));
+ assertEquals("deny",
higher.path("permission").path("custom_rule").asText(),
+ "unrelated user config must be preserved");
+ assertTrue(higher.path("mcp").has("custom"), "unrelated user mcp must
be preserved");
+ assertTrue(higher.path("mcp").has("camel"), "camel mcp must remain in
higher layer");
+ }
+
+ @Test
+ void regenerationRemovesCitrusOnlyFromPermissionWhenServerAlreadyGone()
throws Exception {
+ new OpenCodeGenerator().generate(createContext("camel-kit", true));
+ Path configFile = tempDir.resolve("opencode.json");
+ String current = Files.readString(configFile);
+ assertTrue(current.contains("\"citrus_*\""), "citrus_* permission must
be present");
+ String noServer =
current.replaceAll("(?s)\"citrus\"\\s*:\\s*\\{[^}]*\\},?\\s*", "");
+ assertFalse(noServer.contains("\"citrus\""), "citrus server must be
removed manually");
+ assertTrue(noServer.contains("\"citrus_*\""), "citrus_* permission
must still be present");
+ Files.writeString(configFile, noServer);
Review Comment:
[P2] Cover the server-only early-return regression
The implementation now removes both one-entry remnants correctly, but this
new test does not protect the early-return guard: it removes the server and
leaves `citrus_*` last, so `isCurrentAndOrdered` is already false and removal
runs even if `hasCitrusEntries` is broken. Experimentally, changing the new
`||` in `hasCitrusEntries` to `&&` still passes all 36 `OpenCodeGeneratorTest`
tests. With that mutation, the production initialization probe saves
`citrus.mcp.enabled=false` but retains `mcp.citrus` when only the Citrus
permission was removed before regeneration—the server-only case from the
previous review.
Please add a regression test that starts with a generated opt-in config,
removes only `permission.citrus_*`, regenerates with opt-in disabled, and
asserts that `mcp.citrus` is removed while unrelated settings remain. For the
permission-only test here, move the remaining `citrus_*` before the Camel
permission rules so it also exercises the early-return decision. These tests
should fail when `||` is replaced with `&&` and pass on the current
implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
