luigidemasi commented on code in PR #149:
URL: https://github.com/apache/camel-kit/pull/149#discussion_r4478052378
##########
kit/camel-kit-core/src/main/resources/templates/opencode/agents/camel-ship-worker.md:
##########
@@ -0,0 +1,23 @@
+---
+name: camel-ship-worker
+description: Read-only Camel Ship stage worker dispatched by the parent
OpenCode session.
+mode: subagent
+permission:
+ edit: deny
+ write: deny
+ bash: deny
+ notebook: deny
+ subagent: deny
+ mcp: deny
Review Comment:
[P1] Deny inherited MCP tool actions, not only the literal `mcp` action
Removing the catch-all deny reopens the original MCP boundary. On the same
OpenCode 2.2.5 reported in the acceptance results, I generated this worker,
registered a harmless local `probe` MCP server, and dispatched a real
`subagent(agent="camel-ship-worker")`. The child successfully invoked
`probe.mark` through `execute` and wrote the canary; it also succeeded with MCP
configured outside Code Mode. OpenCode authorizes that tool as `probe_mark`, so
`mcp: deny` does not match it. The previous worker and a control denying
`probe_*` both blocked the call. This lets a supposedly read-only child perform
inherited MCP side effects outside the controller's proposal/validation
boundary. Please enforce denial of the actual inherited MCP actions and add a
real-host canary regression while preserving the parent read restrictions and
authorized external-directory access.
_AI-generated by Codex on behalf of @luigidemasi._
##########
kit/camel-kit-core/src/main/java/org/apache/camel/kit/ship/controller/ShipNativeWorker.java:
##########
@@ -285,15 +285,21 @@ static void applyProposals(
allowed.add(route.routePath());
allowed.add(route.citrusTestPath());
}
- // Validate every proposal before touching the candidate.
+ // Validate and filter proposals: drop files outside the approved
policy,
+ // reject any file that leaks a known secret.
+ List<ProposedFile> accepted = new ArrayList<>();
for (ProposedFile file : files) {
- if (!allowed.contains(file.path()) ||
ChangedWorkspaceSecretScanner.containsSensitiveValue(
+ if (!allowed.contains(file.path())) {
+ continue;
Review Comment:
[P2] Preserve atomic rejection of out-of-policy proposal batches
This `continue` changes the shared native-worker contract: a batch
containing an approved POM replacement plus an out-of-policy agent file now
overwrites the POM instead of rejecting the entire batch. The unchanged
`controllerWritesOnlyApprovedFilesAndConstructsHashes` test fails at line 371
because no IOException is thrown; its next assertion requires the original POM
to remain intact. The full build now fails seven instances of this test: the
shared worker plus Antigravity, Claude, Codex, Copilot, OpenCode and Qwen.
Please restore the rejection contract, or make an explicitly justified contract
change with corresponding behavioral coverage; the required suite must pass
before landing.
_AI-generated by Codex on behalf of @luigidemasi._
##########
kit/camel-kit-core/src/main/java/org/apache/camel/kit/ship/controller/ShipNativeWorker.java:
##########
@@ -309,6 +315,10 @@ static void applyProposals(
}
List<ArtifactManifest.DeclaredArtifact> artifacts = new ArrayList<>();
for (String path : List.of(".camel-kit/config.properties", "pom.xml"))
{
+ Path resolved = candidate.resolve(path);
+ if (!Files.exists(resolved, LinkOption.NOFOLLOW_LINKS)) {
+ continue;
Review Comment:
[P2] Keep missing required artifacts as an EXECUTE failure
The manifest schema still requires both config and POM entries. With an
otherwise valid native response containing the route, Citrus test and `pom.xml`
but omitting `.camel-kit/config.properties`, this skip writes a schema-invalid
manifest and marks EXECUTE completed. A real coordinator probe then fails
VALIDATE, and two resumes only retry that same invalid candidate (validation
attempts 1→2→3), without issuing a new worker task. Against the previous
worker, the same response fails EXECUTE and resume correctly dispatches EXECUTE
attempt 2. Please retain the required-file check and cover omitted config/POM
plus retry. Publication remains blocked, but normal resume cannot recover this
run.
_AI-generated by Codex on behalf of @luigidemasi._
##########
kit/camel-kit-core/src/main/resources/templates/traits/opencode/camel-execute.append.md:
##########
@@ -8,9 +8,9 @@
primary executor session -> leaf agent
```
-The executor may dispatch only the generated `implementer`, `migrator`,
`planner`, `researcher`, `reviewer`, and `tester` agents. Every leaf denies
`task`, so it cannot create another child. This generated one-level topology
does not require a development-only configuration key.
+The executor may dispatch only the generated `implementer`, `migrator`,
`planner`, `researcher`, `reviewer`, and `tester` agents. Every leaf denies
`subagent`, so it cannot create another child. This generated one-level
topology does not require a development-only configuration key.
-OpenCode's `task` tool requires the exact lowercase agent name in
`subagent_type`. It does not accept a per-call `steps` argument; each generated
agent definition owns its limit. Foreground is the default. Keep every
implementation, research result, and review gate in the foreground by omitting
`background`.
+OpenCode's `subagent` tool requires the exact lowercase agent name in `agent`.
It does not accept a per-call `steps` argument; each generated agent definition
owns its limit. Foreground is the default. Keep every implementation, research
result, and review gate in the foreground by omitting `background`.
Review Comment:
[P2] Migrate the shared dispatch block with these traits
`templates/dispatch/opencode.md` still mandates `task` with `subagent_type`,
and `SkillResourceInstaller` appends that block to every non-Ship skill. Actual
generation and reinitialization with both command prefixes therefore produce
contradictory old/new contracts in `camel-execute` and `camel-knowledge`;
`camel-plan` retains only the old contract. OpenCode 2.2.5 exposes `subagent`
with `agent`, so the shared mandatory guidance directs those workflows to an
unavailable tool. Please migrate the shared template and assert that the
obsolete tool/argument contract is absent from generated skills. Update the
matching OpenCode references in `agents.adoc`, `ship.adoc` and the PR
description as well.
_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]
