luigidemasi commented on code in PR #149:
URL: https://github.com/apache/camel-kit/pull/149#discussion_r4491521199
##########
kit/camel-kit-core/src/main/java/org/apache/camel/kit/generator/OpenCodeGenerator.java:
##########
@@ -30,22 +45,147 @@ public void generate(InitContext ctx) throws Exception {
Map<String, Object> data = new HashMap<>(
Map.of(
"COMMAND_PREFIX", ctx.commandPrefix()));
- // AGENTS.md already generated by the shared generator flow.
+
+ List<String> mcpServers = readMcpServerNames(ctx);
+ Set<String> seen = new LinkedHashSet<>();
+ boolean collisionDetected = false;
+ StringBuilder denials = new StringBuilder();
+ for (String server : mcpServers) {
+ String normalized = normalizeServerName(server);
+ if (BUILTIN_PERMISSION_PREFIXES.contains(normalized)) {
+ collisionDetected = true;
+ } else if (seen.add(normalized)) {
+ denials.append(" ").append(normalized).append("_*: deny\n");
+ }
+ }
+ data.put("MCP_TOOL_DENIALS", denials.toString());
// OpenCode-specific: generate agent definitions
- generateAgents(ctx, data);
+ generateAgents(ctx, data, !collisionDetected);
+ }
+
+ static String normalizeServerName(String name) {
+ StringBuilder sb = new StringBuilder(name.length());
+ for (int i = 0; i < name.length(); i++) {
+ char c = name.charAt(i);
+ sb.append(isAsciiAlphanumericOrHyphen(c) ? c : '_');
+ }
+ return sb.toString();
+ }
+
+ private static boolean isAsciiAlphanumericOrHyphen(char c) {
+ return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0'
&& c <= '9') || c == '-';
+ }
+
+ private List<String> readMcpServerNames(InitContext ctx) throws
IOException {
+ Set<String> names = new LinkedHashSet<>();
+ for (Path configFile :
OpenCodeProjectConfig.existingFiles(ctx.projectDir())) {
+ collectMcpServersFromFile(configFile, names);
+ }
+ for (Path ancestorFile : ancestorConfigFiles(ctx.projectDir())) {
+ collectMcpServersFromFile(ancestorFile, names);
+ }
+ for (Path globalFile : globalConfigFiles()) {
+ collectMcpServersFromFile(globalFile, names);
+ }
+ Path explicitConfig = explicitConfigFile();
+ if (explicitConfig != null) {
+ collectMcpServersFromFile(explicitConfig, names);
+ }
+ return List.copyOf(names);
Review Comment:
[P1] Include inline host configuration in the MCP deny inventory
`OPENCODE_CONFIG_CONTENT` is another supported OpenCode config source, but
this method returns without inspecting it. I set an inline config containing a
harmless local `probe` MCP server before **both generation and host startup**,
then dispatched the generated `camel-ship-worker` through the real OpenCode
2.2.5 `subagent` tool. The child called `probe.mark` through `execute` and
wrote its canary: the generated worker lacks `probe_*: deny`, and `mcp: deny`
does not block that action. Adding only `probe_*: deny` blocked the same call.
This is a pre-existing configuration source, not a server added after init.
Please include inline content in the effective server inventory (and audit
it against the supported host config sources), or decline native worker
generation when the effective MCP set cannot be bounded. Add a regression that
sets this source before generation and verifies the resulting worker denial.
OpenCode's supported source is documented at [the config precedence
documentation](https://opencode.ai/docs/config/#precedence-order) and loaded in
[the 2.2.5 config
implementation](https://github.com/anomalyco/opencode/blob/v2.2.5/packages/core/src/config.ts).
_AI-generated by Codex on behalf of @luigidemasi._
##########
kit/camel-kit-core/src/test/java/org/apache/camel/kit/generator/OpenCodeGeneratorTest.java:
##########
@@ -952,4 +1015,235 @@ void
regenerationRemovesCitrusServerWhenOnlyPermissionAlreadyGone() throws Excep
assertTrue(result.contains("\"camel\""), "camel mcp must remain");
assertTrue(result.contains("\"camel_*\""), "camel_* permission must
remain");
}
+
+ @Test
+ void shipWorkerDeniesServersFromAllProjectConfigLayers() throws Exception {
+ InitContext ctx = createContext();
+
+ Files.writeString(tempDir.resolve("opencode.json"), """
+ {
+ "mcp": {
+ "lower-layer-probe": {"type": "local", "command":
["echo"], "enabled": true}
+ }
+ }
+ """);
+ Files.writeString(tempDir.resolve("opencode.jsonc"), """
+ {
+ "mcp": {}
+ }
+ """);
+
+ new OpenCodeGenerator().generate(ctx);
+
+ JsonNode lower = readOpenCodeConfig(tempDir.resolve("opencode.json"));
+ assertTrue(lower.path("mcp").has("lower-layer-probe"),
+ "Lower config layer must retain its custom MCP server");
+
+ String worker = Files.readString(
+ tempDir.resolve(".opencode/agents/camel-ship-worker.md"));
+ assertTrue(worker.contains("lower-layer-probe_*: deny"),
+ "Ship worker must deny MCP servers from lower-precedence
config layers");
+ assertTrue(worker.contains("camel_*: deny"),
+ "Ship worker must still deny bundled camel MCP server tools");
+ }
+
+ @Test
+ void shipWorkerNormalizesServerNamesForDenialGlobs() throws Exception {
+ InitContext ctx = createContext();
+ new OpenCodeGenerator().generate(ctx);
+
+ Path configFile = tempDir.resolve("opencode.json");
+ com.fasterxml.jackson.databind.node.ObjectNode config
+ = (com.fasterxml.jackson.databind.node.ObjectNode) new
ObjectMapper().readTree(configFile.toFile());
+ com.fasterxml.jackson.databind.node.ObjectNode probe = new
ObjectMapper().createObjectNode();
+ probe.put("type", "local");
+ probe.set("command", new ObjectMapper().createArrayNode().add("echo"));
+ probe.put("enabled", true);
+ ((com.fasterxml.jackson.databind.node.ObjectNode)
config.path("mcp")).set("probe.local", probe);
+ Files.writeString(configFile, new
ObjectMapper().writerWithDefaultPrettyPrinter()
+ .writeValueAsString(config));
+
+ new OpenCodeGenerator().generate(ctx);
+
+ String worker = Files.readString(
+ tempDir.resolve(".opencode/agents/camel-ship-worker.md"));
+ assertTrue(worker.contains("probe_local_*: deny"),
+ "Ship worker must normalize dots to underscores in MCP server
denial globs");
+ assertFalse(worker.contains("probe.local_*: deny"),
+ "Ship worker must not use raw server names with dots in denial
globs");
+ }
+
+ @Test
+ void shipWorkerNotGeneratedWhenExternalServerCollidesWithBuiltin() throws
Exception {
+ InitContext ctx = createContext();
+ new OpenCodeGenerator().generate(ctx);
+
+ Path configFile = tempDir.resolve("opencode.json");
+ com.fasterxml.jackson.databind.node.ObjectNode config
+ = (com.fasterxml.jackson.databind.node.ObjectNode) new
ObjectMapper().readTree(configFile.toFile());
+ com.fasterxml.jackson.databind.node.ObjectNode external = new
ObjectMapper().createObjectNode();
+ external.put("type", "remote");
+ external.put("url", "https://example.test/external";);
+ ((com.fasterxml.jackson.databind.node.ObjectNode)
config.path("mcp")).set("external", external);
+ Files.writeString(configFile, new
ObjectMapper().writerWithDefaultPrettyPrinter()
+ .writeValueAsString(config));
+
+ new OpenCodeGenerator().generate(ctx);
+
+ Path workerFile =
tempDir.resolve(".opencode/agents/camel-ship-worker.md");
+ assertFalse(Files.exists(workerFile),
+ "Ship worker must not be generated when an MCP server collides
with a built-in permission prefix");
+
assertTrue(Files.exists(tempDir.resolve(".opencode/agents/brainstormer.md")),
+ "Other agent definitions must still be generated");
+ }
+
+ @Test
+ void shipWorkerDeletedOnReInitWhenCollisionAppears() throws Exception {
+ InitContext ctx = createContext();
+ new OpenCodeGenerator().generate(ctx);
+
+ Path workerFile =
tempDir.resolve(".opencode/agents/camel-ship-worker.md");
+ assertTrue(Files.exists(workerFile), "Worker must exist after initial
generation");
+
+ Path configFile = tempDir.resolve("opencode.json");
+ com.fasterxml.jackson.databind.node.ObjectNode config
+ = (com.fasterxml.jackson.databind.node.ObjectNode) new
ObjectMapper().readTree(configFile.toFile());
+ com.fasterxml.jackson.databind.node.ObjectNode external = new
ObjectMapper().createObjectNode();
+ external.put("type", "remote");
+ external.put("url", "https://example.test/external";);
+ ((com.fasterxml.jackson.databind.node.ObjectNode)
config.path("mcp")).set("external", external);
+ Files.writeString(configFile, new
ObjectMapper().writerWithDefaultPrettyPrinter()
+ .writeValueAsString(config));
+
+ new OpenCodeGenerator().generate(ctx);
+
+ assertFalse(Files.exists(workerFile),
+ "Ship worker must be deleted on re-init when collision is
detected");
+ }
+
+ @Test
+ void shipWorkerDeniesNativeFormatServers() throws Exception {
+ InitContext ctx = createContext();
+
+ Files.writeString(tempDir.resolve("opencode.json"), """
+ {
+ "mcp": {
+ "servers": {
+ "probe": {"type": "local", "command": ["echo"],
"enabled": true},
+ "scanner": {"type": "local", "command": ["echo"],
"enabled": true}
+ }
+ }
+ }
+ """);
+
+ new OpenCodeGenerator().generate(ctx);
+
+ String worker = Files.readString(
+ tempDir.resolve(".opencode/agents/camel-ship-worker.md"));
+ assertTrue(worker.contains("probe_*: deny"),
+ "Ship worker must deny servers declared in native mcp.servers
format");
+ assertTrue(worker.contains("scanner_*: deny"),
+ "Ship worker must deny all servers from native mcp.servers
container");
+ }
+
+ @Test
+ void shipWorkerUsesAsciiOnlyNormalization() throws Exception {
+ assertEquals("caf_", OpenCodeGenerator.normalizeServerName("café"),
+ "Non-ASCII letters must be replaced with underscore");
+ assertEquals("probe_local",
OpenCodeGenerator.normalizeServerName("probe.local"),
+ "Dots must be replaced with underscore");
+ assertEquals("my-server",
OpenCodeGenerator.normalizeServerName("my-server"),
+ "Hyphens must be preserved");
+ assertEquals("__", OpenCodeGenerator.normalizeServerName("üñ"),
+ "All non-ASCII characters must be replaced");
+ }
+
+ @Test
+ void shipWorkerDeniesServersFromAncestorConfig() throws Exception {
+ InitContext ctx = createContext();
+
+ Path ancestorDir = tempDir.getParent();
+ Path ancestorConfig = ancestorDir.resolve("opencode.json");
+ Files.writeString(ancestorConfig, """
Review Comment:
[P2] Keep the ancestor fixture inside the owned temporary directory
`tempDir` is a JUnit `@TempDir`, so `tempDir.getParent()` is the shared JVM
temporary root (normally `/tmp`), not a directory owned by this test.
`Files.writeString` overwrites an existing `opencode.json`, and the `finally`
block deletes it. I invoked this exact test with a disposable pre-existing
config in that parent: the test passed and the pre-existing file was gone. It
can also race other projects/tests using that root.
Create the project as a child of `tempDir` and put the ancestor config
inside `tempDir`, so setup and cleanup touch only files owned by the fixture.
_AI-generated by Codex on behalf of @luigidemasi._
##########
kit/camel-kit-core/src/test/java/org/apache/camel/kit/generator/OpenCodeGeneratorTest.java:
##########
@@ -952,4 +1015,235 @@ void
regenerationRemovesCitrusServerWhenOnlyPermissionAlreadyGone() throws Excep
assertTrue(result.contains("\"camel\""), "camel mcp must remain");
assertTrue(result.contains("\"camel_*\""), "camel_* permission must
remain");
}
+
+ @Test
+ void shipWorkerDeniesServersFromAllProjectConfigLayers() throws Exception {
+ InitContext ctx = createContext();
+
+ Files.writeString(tempDir.resolve("opencode.json"), """
+ {
+ "mcp": {
+ "lower-layer-probe": {"type": "local", "command":
["echo"], "enabled": true}
+ }
+ }
+ """);
+ Files.writeString(tempDir.resolve("opencode.jsonc"), """
+ {
+ "mcp": {}
+ }
+ """);
+
+ new OpenCodeGenerator().generate(ctx);
+
+ JsonNode lower = readOpenCodeConfig(tempDir.resolve("opencode.json"));
+ assertTrue(lower.path("mcp").has("lower-layer-probe"),
+ "Lower config layer must retain its custom MCP server");
+
+ String worker = Files.readString(
+ tempDir.resolve(".opencode/agents/camel-ship-worker.md"));
+ assertTrue(worker.contains("lower-layer-probe_*: deny"),
+ "Ship worker must deny MCP servers from lower-precedence
config layers");
+ assertTrue(worker.contains("camel_*: deny"),
+ "Ship worker must still deny bundled camel MCP server tools");
+ }
+
+ @Test
+ void shipWorkerNormalizesServerNamesForDenialGlobs() throws Exception {
+ InitContext ctx = createContext();
+ new OpenCodeGenerator().generate(ctx);
+
+ Path configFile = tempDir.resolve("opencode.json");
+ com.fasterxml.jackson.databind.node.ObjectNode config
+ = (com.fasterxml.jackson.databind.node.ObjectNode) new
ObjectMapper().readTree(configFile.toFile());
+ com.fasterxml.jackson.databind.node.ObjectNode probe = new
ObjectMapper().createObjectNode();
+ probe.put("type", "local");
+ probe.set("command", new ObjectMapper().createArrayNode().add("echo"));
+ probe.put("enabled", true);
+ ((com.fasterxml.jackson.databind.node.ObjectNode)
config.path("mcp")).set("probe.local", probe);
+ Files.writeString(configFile, new
ObjectMapper().writerWithDefaultPrettyPrinter()
+ .writeValueAsString(config));
+
+ new OpenCodeGenerator().generate(ctx);
+
+ String worker = Files.readString(
+ tempDir.resolve(".opencode/agents/camel-ship-worker.md"));
+ assertTrue(worker.contains("probe_local_*: deny"),
+ "Ship worker must normalize dots to underscores in MCP server
denial globs");
+ assertFalse(worker.contains("probe.local_*: deny"),
+ "Ship worker must not use raw server names with dots in denial
globs");
+ }
+
+ @Test
+ void shipWorkerNotGeneratedWhenExternalServerCollidesWithBuiltin() throws
Exception {
+ InitContext ctx = createContext();
+ new OpenCodeGenerator().generate(ctx);
+
+ Path configFile = tempDir.resolve("opencode.json");
+ com.fasterxml.jackson.databind.node.ObjectNode config
+ = (com.fasterxml.jackson.databind.node.ObjectNode) new
ObjectMapper().readTree(configFile.toFile());
+ com.fasterxml.jackson.databind.node.ObjectNode external = new
ObjectMapper().createObjectNode();
+ external.put("type", "remote");
+ external.put("url", "https://example.test/external";);
+ ((com.fasterxml.jackson.databind.node.ObjectNode)
config.path("mcp")).set("external", external);
+ Files.writeString(configFile, new
ObjectMapper().writerWithDefaultPrettyPrinter()
+ .writeValueAsString(config));
+
+ new OpenCodeGenerator().generate(ctx);
+
+ Path workerFile =
tempDir.resolve(".opencode/agents/camel-ship-worker.md");
+ assertFalse(Files.exists(workerFile),
+ "Ship worker must not be generated when an MCP server collides
with a built-in permission prefix");
+
assertTrue(Files.exists(tempDir.resolve(".opencode/agents/brainstormer.md")),
+ "Other agent definitions must still be generated");
+ }
+
+ @Test
+ void shipWorkerDeletedOnReInitWhenCollisionAppears() throws Exception {
+ InitContext ctx = createContext();
+ new OpenCodeGenerator().generate(ctx);
+
+ Path workerFile =
tempDir.resolve(".opencode/agents/camel-ship-worker.md");
+ assertTrue(Files.exists(workerFile), "Worker must exist after initial
generation");
+
+ Path configFile = tempDir.resolve("opencode.json");
+ com.fasterxml.jackson.databind.node.ObjectNode config
+ = (com.fasterxml.jackson.databind.node.ObjectNode) new
ObjectMapper().readTree(configFile.toFile());
+ com.fasterxml.jackson.databind.node.ObjectNode external = new
ObjectMapper().createObjectNode();
+ external.put("type", "remote");
+ external.put("url", "https://example.test/external";);
+ ((com.fasterxml.jackson.databind.node.ObjectNode)
config.path("mcp")).set("external", external);
+ Files.writeString(configFile, new
ObjectMapper().writerWithDefaultPrettyPrinter()
+ .writeValueAsString(config));
+
+ new OpenCodeGenerator().generate(ctx);
+
+ assertFalse(Files.exists(workerFile),
+ "Ship worker must be deleted on re-init when collision is
detected");
+ }
+
+ @Test
+ void shipWorkerDeniesNativeFormatServers() throws Exception {
+ InitContext ctx = createContext();
+
+ Files.writeString(tempDir.resolve("opencode.json"), """
+ {
+ "mcp": {
+ "servers": {
+ "probe": {"type": "local", "command": ["echo"],
"enabled": true},
+ "scanner": {"type": "local", "command": ["echo"],
"enabled": true}
+ }
+ }
+ }
+ """);
+
+ new OpenCodeGenerator().generate(ctx);
+
+ String worker = Files.readString(
+ tempDir.resolve(".opencode/agents/camel-ship-worker.md"));
+ assertTrue(worker.contains("probe_*: deny"),
+ "Ship worker must deny servers declared in native mcp.servers
format");
+ assertTrue(worker.contains("scanner_*: deny"),
+ "Ship worker must deny all servers from native mcp.servers
container");
+ }
+
+ @Test
+ void shipWorkerUsesAsciiOnlyNormalization() throws Exception {
+ assertEquals("caf_", OpenCodeGenerator.normalizeServerName("café"),
+ "Non-ASCII letters must be replaced with underscore");
+ assertEquals("probe_local",
OpenCodeGenerator.normalizeServerName("probe.local"),
+ "Dots must be replaced with underscore");
+ assertEquals("my-server",
OpenCodeGenerator.normalizeServerName("my-server"),
+ "Hyphens must be preserved");
+ assertEquals("__", OpenCodeGenerator.normalizeServerName("üñ"),
+ "All non-ASCII characters must be replaced");
+ }
+
+ @Test
+ void shipWorkerDeniesServersFromAncestorConfig() throws Exception {
+ InitContext ctx = createContext();
+
+ Path ancestorDir = tempDir.getParent();
+ Path ancestorConfig = ancestorDir.resolve("opencode.json");
+ Files.writeString(ancestorConfig, """
+ {
+ "mcp": {
+ "ancestor-probe": {"type": "local", "command": ["echo"],
"enabled": true}
+ }
+ }
+ """);
+
+ try {
+ new OpenCodeGenerator().generate(ctx);
+
+ String worker = Files.readString(
+ tempDir.resolve(".opencode/agents/camel-ship-worker.md"));
+ assertTrue(worker.contains("ancestor-probe_*: deny"),
+ "Ship worker must deny MCP servers inherited from ancestor
config");
+ } finally {
+ Files.deleteIfExists(ancestorConfig);
+ }
+ }
+
+ @Test
+ void collectMcpServersFromFileHandlesGlobalConfig() throws Exception {
+ Path globalConfig = tempDir.resolve("global-opencode.json");
+ Files.writeString(globalConfig, """
+ {
+ "mcp": {
+ "global-probe": {"type": "local", "command": ["echo"],
"enabled": true}
+ }
+ }
+ """);
+
+ Set<String> names = new LinkedHashSet<>();
+ OpenCodeGenerator.collectMcpServersFromFile(globalConfig, names);
Review Comment:
[P2] Exercise global discovery through worker generation
Both new global-config tests pass an arbitrary file directly to
`collectMcpServersFromFile`; they never exercise the XDG/default-global
location or its connection to `generate()`. Consequently they cannot catch the
original global-only MCP permission regression. I replaced the global-discovery
loop with an empty list: all 58 `OpenCodeGeneratorTest` /
`ShippedAssetStructureTest` tests still passed. Removing the explicit-config
lookup also passed all 58; reverting the ancestor and native-`type` fixes each
failed its regression test, and the restored source passed all 58.
Please generate a worker in an isolated environment with the MCP server
present only in the actual global config location, and assert the emitted
denial. The regression should fail if the `globalConfigFiles()` loop is
removed. The new explicit `OPENCODE_CONFIG` discovery also needs a test through
generation rather than only parser-level assertions.
_AI-generated by Codex on behalf of @luigidemasi._
--
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]
