CAMEL-24663: camel-jbang-plugin-tui - drive the TUI from coding agents over the Agent Client Protocol (ACP) - #26244
CAMEL-24663: camel-jbang-plugin-tui - drive the TUI from coding agents over the Agent Client Protocol (ACP)#26244luigidemasi wants to merge 10 commits into
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
AI Review — PR #26244 (CAMEL-24663: ACP integration for camel-jbang-plugin-tui)
This is a substantial, well-architected feature PR that adds Agent Client Protocol (ACP) support to the TUI's AI panel. The implementation is clean, the test coverage is excellent (83 new tests), and the documentation is thorough.
Overall Assessment
VERDICT: COMMENT — The code is solid and ready for committer review. A few observations below, mostly informational.
Observations
Architecture (positive):
- Clean separation:
AcpAgentClient(protocol),AcpHeaderStrip(UI),AcpPermissionPopup(permissions), panel integration inAiPanel - Lazy agent spawning (no freeze on slow
npxdownloads) FakeAcpAgenttest harness with piped streams is well-designed for deterministic async testing- Proper cancelled-turn handling with
awaitCancelledTurn()grace period - Permission auto-approval for TUI tools is security-conscious: checks registered tool names, rejects file/shell kinds
Thread safety:
- Good use of
volatilefor cross-thread fields (acpClient,acpSessionId,acpAgentInfo, etc.) ConcurrentHashMapfor pending requests,synchronizedfor writer accesscloseAcpClient()runsagent.close()on a daemon thread to avoid blocking the TUI event thread — nice touch- The
AcpTurnListenermethods are synchronized, which correctly prevents interleaving between the reader thread and the agent thread'sfinish()call
Potential concerns (minor):
-
AcpAgentClient.readLoop()line parsing —Jsoner.deserialize(line, (JsonObject) null)silently returns null for valid JSON that isn't an object (e.g., a JSON array). The diagnostic message says "malformed" which is slightly misleading — it could be a valid JSON array response from a buggy agent. Cosmetic, not functional. -
AcpPermissionPopupthread safety — Theopen()method sets several fields before settingvisible = true(the publishing write). This relies onvolatileproviding a happens-before relationship. The comment documents this intentionally, which is good, butsynchronizedon the whole popup would be more conventional. -
Logo PNG licensing — The PR description explicitly flags the six vendor logo PNGs for committer review regarding ASF policy. This is the right call; the fallback to coloured glyphs makes removal straightforward.
-
Settings popup scrolling — The new scroll logic in
SettingsPopupis a welcome fix for short terminals. ThelineOf()static method correctly accounts for divider positions. -
TuiMcpServer.getPort()ephemeral port support — Clean addition. ThevolatileonmcpPortinCamelMonitoris appropriate sinceensureMcpServer()writes it from the agent thread.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Category | — | feature |
| Labels | docs, dsl |
correct |
| Milestone | (none) | 4.23.0 (new feature → next minor) |
🔀 Backport Status
Not needed — this is a new feature, not a bug fix.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
| error.put("message", "Method not found: " + method); | ||
| response.put("error", error); | ||
| } | ||
| try { |
There was a problem hiding this comment.
Nit: Jsoner.deserialize(line, (JsonObject) null) returns null both when the line is genuinely malformed and when it's valid JSON that isn't an object (e.g., a JSON array). The diagnostic below says "malformed" for both cases. In practice this doesn't matter — an ACP agent sending a JSON array is broken anyway — but the message could be slightly more precise (e.g., "Ignoring non-object line from agent"). Very minor.
| } | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
The comment at line 116–117 documents the volatile publication pattern well. Just a note for reviewers: this is a valid Java memory model pattern — the volatile write to visible (the last statement in open()) establishes a happens-before with the volatile read in isVisible(), guaranteeing that the other fields (title, kind, inputLines, options) set above it are visible to the reading thread. No issue here.
…rototype) Adds an Agent Client Protocol backend to the F8 AI panel: a hand-rolled ACP v1 client over stdio, agent presets (Claude Code, Codex, IBM Bob, Qwen Code, OpenCode, DeepSeek Harness) plus a custom command, on-demand start of the embedded MCP server handed to the agent in session/new, streamed answers and tool lines, a permission popup (camel-tui tools auto-approved), agent slash commands as /agent:<name>, a header strip with the agent logo (kitty) or a glyph, settings rows, tests and user-manual docs. Squashed from 30 commits kept on branch tui-acp-prototype-backup. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
… /prompt for the ACP agent instead of the LLM path With an ACP agent selected, the panel's /retry, /context, /compact, /tools and /prompt still described the LLM path: no client to retry with, a model history that does not exist, a tool set the agent never sees. /retry now resends the last question or /agent: command through the agent; /context shows the agent, its session, working directory, MCP server, command count, preamble size and the tokens reported so far; /prompt shows the preamble under a heading naming the agent; /compact and /tools say the agent manages its own history and tools, pointing to /agent:compact when the agent offers it, and /tools <mode> is refused without touching the settings. The LLM path is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
…in the ACP permission handler, never file or shell calls The handler auto-approved any tool call whose title contained "camel-tui", so a file edit titled "Edit /tmp/camel-tui/route.yaml" was approved without asking. A title now only identifies a tool when it has the Gemini-derived shape "tui_get_state (camel-tui MCP Server)" and the tool is one the TUI registers, and kinds that touch files or run commands never qualify. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
…ore sending the next prompt ACP lets the agent answer a cancelled session/prompt and keep streaming until that response. The client dropped the pending future on interrupt, so the next prompt installed its listener straight away and collected the old turn's late updates. The interrupted request now keeps its future and the next prompt waits up to 10s for it before installing its own listener. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
…e ACP session is being created The Claude adapter advertises available_commands_update before it answers session/new, so after /clear the second session's commands arrived while currentSession still pointed at the first one and were dropped as foreign. newSession now clears the list before it asks, and the client remembers which session the stored list came from. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
…ort screens render() gave the header as many rows as it wanted and the option list the rest, which was zero on a small terminal: the popup asked for a decision with no choices on screen. The option rows are now reserved first and the header is clipped instead, title first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
… stays visible on short terminals The dialog drew its 24 content lines from the top of the popup whatever the popup's height, so on an 80x25 terminal AI History, ACP Command and ACP Logos were drawn over the border or off it, and selecting them changed nothing on screen. The content now scrolls to keep the selected row inside the popup, and the rows outside it are not drawn. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
…olved on PATH so Windows .cmd shims work The precheck accepted npx.cmd/npx.exe but the preset's command was passed as is, and ProcessBuilder only tries .exe on Windows: the check passed and the launch failed. isOnPath is now a thin wrapper over resolveExecutable, and spawnAcpAgent launches the file that was actually found. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
@agentclientprotocol/claude-agent-acp 0.75.1 declares engines.node >= 22, so the install hint and the documentation asking for Node.js 18 sent users to a runtime the adapter refuses. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
- the permission handler's file-or-shell guard is now pinned by a test, and an unwired tool registry fails closed instead of falling back to a name pattern (the panel's tests wire the real registry through setToolRegistryForTesting) - the cancelled-turn test gets a @timeout so a missed interrupt fails the build instead of wedging it, and no longer resets the interrupt flag from inside an assertion - newSession() forgets an owed cancelled turn, so a new session's first question is not delayed by the old one - SettingsPopup's divider count is a constant shared by render() and lineOf() - resolveExecutable() probes .cmd/.exe for an absolute path too - the ACP paragraph of the TUI docs is re-wrapped to the file's width Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov
75f51e6 to
aafef2a
Compare
|
rebased so the tui test with the budget is fixed |
gnodet
left a comment
There was a problem hiding this comment.
AI Re-Review — PR #26244 (post-push aafef2a)
Following up on the previous review (commit 75f51e6). The new commit "Address the Task 18 review findings" makes four solid improvements:
1. Security tightening (isRegisteredTuiTool) — The TUI_TOOL_NAME regex fallback is removed. When no TuiToolRegistry is wired, nothing is auto-approved, so the user is always asked. This closes the hole where a file-system tool with a tui_* name could have been auto-approved without the registry check. Good change, and the two new tests (withoutAToolRegistryNothingIsAutoApproved, executeKindIsNotAutoApprovedEvenForARegisteredTuiTool) cover both paths.
2. Session lifecycle (newSession clears cancelledTurn) — Setting cancelledTurn = null in newSession() prevents a stale cancelled-turn future from blocking the first prompt of a fresh session (e.g., after /clear). This is the correct fix: the old session's owed response is irrelevant to the new session, and the session-id filter in handleNotification already discards late updates from abandoned sessions. The new newSessionForgetsTheCancelledTurn test verifies the path end-to-end.
3. Executable resolution (resolveExecutable + firstExecutable) — Extracted firstExecutable() so absolute paths also check .cmd/.exe siblings. Previously an absolute path was checked only with Files.isExecutable(direct), missing Windows shims. The refactoring is clean and the new assertion covers it.
4. SettingsPopup constants — ROW_COUNT and DIVIDERS are now package-visible constants instead of a local variable and a magic 4, which allows the new lineOfAccountsForEveryRowAndDivider test to assert that the scroll arithmetic stays in sync when rows are added. Minor but good for maintenance.
The @Timeout(30) annotations on the async cancelled-turn tests are a good safety net — those tests can hang if the concurrency logic regresses.
No new code concerns from this push. The observations from the previous review (diagnostic message wording on line 557, and the volatile publication pattern note on AcpPermissionPopup) are informational and don't need changes.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Category | — | feature |
| Labels | docs, dsl |
correct |
| Milestone | (none) | 4.23.0 (new feature → next minor) |
🔀 Backport Status
Not needed — this is a new feature, not a bug fix.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
davsclaus
left a comment
There was a problem hiding this comment.
Review: ACP coding-agent provider for the TUI AI panel
Thanks for this — it is a substantial and genuinely careful piece of work. The hand-rolled ACP client with no new dependency, the in-process fake agent over pipes, and the fact that the three earlier review findings (title-based auto-approve, the cancelled-turn leak, the Windows .cmd shim) are all fixed and pinned by tests, made this a pleasure to review.
What I verified, rather than only reading the diff:
- Built and ran the full module from this branch in a worktree:
mvn install-> BUILD SUCCESS, 1314 tests, 0 failures, 0 errors. mvn -Psourcecheck -DskipTests install-> BUILD SUCCESS (formatting, imports, license headers clean).- The branch's merge-base is current
main(4462fd61418f), so the PR body's note aboutAiPanelPromptBudgetTestfailing is now stale — it passes here (3/3). Worth removing from the description. - Checked
git log/git blameonTuiMcpServer,TuiToolRegistry,McpFacadeandAiProviderSelectorfor prior intent. Nothing here reverts an earlier deliberate decision.
Conventions are in order: JIRA linked, CAMEL-24663: commit prefixes, Co-Authored-By trailers, no new dependencies, docs added to camel-jbang-tui.adoc. This is a new feature rather than a migration, so no upgrade-guide entry is needed.
Items that need a committer/PMC decision
1. The bundled vendor logos. src/main/resources/tui/logos/*.png are six third-party marks shipped inside a released Apache JAR, and they are the only third-party binaries under src/main/resources anywhere in the repo (the one other PNG is Camel's own logo). Nothing was added to LICENSE.txt / NOTICE.txt. You already raise this yourself, and I agree it needs a legal/PMC answer rather than a reviewer's opinion. The coloured-glyph fallback already exists and is covered by AcpHeaderStripTest, so dropping the PNGs is a low-cost path if the answer is no.
2. The silent auto-approve reaches state-changing tools — see the inline comment on AiPanel.isRegisteredTuiTool.
3. The on-demand MCP server is unauthenticated and now starts without --mcp — see the inline comment on CamelMonitor.ensureMcpServer.
Items 2 and 3 are the two places where this PR moves a trust boundary, so they deserve a deliberate answer even if that answer is "acceptable for a developer tool".
Open questions
- Is the "MCP tool calls arrive with
kind: other" assumption correct for the Claude and Codex adapters? If they do tag MCP calls with a meaningful kind, finding 2 shrinks a lot — but the guard probably should not depend on adapter behaviour the TUI cannot verify. acp:customnever appears in Ctrl+P untilcamel.tui.ai.acp.commandis set, so it is only discoverable via F2 -> Settings. Intentional?
Scope
This is a rules, conventions and behaviour review against the project's contribution rules. It does not replace CodeRabbit, Sourcery, or SonarCloud static analysis.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| private boolean isRegisteredTuiTool(String tool) { | ||
| TuiToolRegistry registry = toolRegistry; | ||
| // no registry wired yet: nothing is a known TUI tool, so the user is asked rather than the call approved | ||
| return registry != null && registry.getToolDefinitions().stream().anyMatch(td -> td.name().equals(tool)); |
There was a problem hiding this comment.
The auto-approve covers state-changing tools, not just read-only ones.
isTuiTool() above rejects the kinds edit/delete/move/execute/fetch, but an MCP tool call from an ACP adapter is normally reported as kind other, so that guard does not fire for mcp__camel-tui__* calls at all. This line then matches against getToolDefinitions() — all tools, not CORE_TOOLS — which includes tui_control (start/stop/restart an integration), tui_execute_sql, tui_update_row, tui_send_keys, tui_action, tui_run_example and tui_open_project.
tui_write_file is fine: I checked McpFacade.writeFile and it still forces the TUI write dialog, explicitly ignoring confirm=false outside WriteMode.AUTO. The others have no confirmation of their own.
So in practice an agent can restart the running integration, or execute SQL against the user's data source, and the user sees only a status line — never a prompt. That is a bigger grant than the design note ("calls to the camel-tui MCP server are approved silently") reads as.
Would an explicit read-only allowlist be better here than "any registered tool"? Something like CORE_TOOLS minus the mutating ones, or a dedicated set, so adding a new mutating tool later cannot silently widen the grant. The docs line "calls to the TUI's own tools are approved without asking" would want the same tightening — as written it reads as if only inspection tools are meant.
| * --mcp nor an earlier call did. Used by the AI panel to hand the TUI tools to an ACP agent. No .mcp.json is | ||
| * written for an on-demand server. Called from the AI panel's agent thread. | ||
| */ | ||
| synchronized String ensureMcpServer() throws IOException { |
There was a problem hiding this comment.
The on-demand MCP server is unauthenticated, and this flips a previously opt-in surface on by default.
Selecting an ACP provider now opens an HTTP endpoint that can drive the TUI without the user ever passing --mcp. The mitigations are real and I verified them in TuiMcpServer: loopback-only bind, Origin rejection, application/json required, ephemeral port, no .mcp.json written. The docs state all of this honestly, which I appreciate.
What is missing is any token, so any local process can port-scan 127.0.0.1 and reach the same tools discussed in the AiPanel comment. Since the URL is handed to the agent in session/new anyway, a random secret path segment or a bearer token would cost very little and close local-process access entirely.
Camel's security model treats insecure defaults as in scope, so this seems worth a deliberate PMC answer rather than being settled implicitly by the merge.
Minor, in the same method: if --mcp was given but the initial start() failed with BindException, mcpServer stays null and this method retries the same busy port and throws. Falling back to port 0 in that case would be more forgiving.
| } | ||
| if (tokens > 0) { | ||
| AiProviderSelector.AcpPreset preset = acpPreset; | ||
| sessionTotalTokens = tokens; |
There was a problem hiding this comment.
sessionTotalTokens means something different on the two paths.
The LLM path accumulates (sessionTotalTokens += ... at lines 1396 and 1412); this assigns. ACP's usage_update.used is the current context size, not a session total, so the title bar's total: N tokens and /context's "Tokens reported by the agent so far" both mislabel the number — it will go down when the agent compacts.
Display-only, so not blocking. Either accumulate per-turn deltas, or relabel it as context size on the ACP path.
| } | ||
|
|
||
| /** True when {@code executable} is an absolute path to an executable file or is found on the PATH. */ | ||
| static boolean isOnPath(String executable) { |
There was a problem hiding this comment.
isOnPath is only referenced from AiProviderSelectorTest — nothing in production calls it. The install-hint behaviour the PR description mentions is actually delivered by resolveExecutable() in AiPanel.spawnAcpAgent.
Either wire it up (showing the hint in the Ctrl+P provider list before the user picks an unavailable preset would be a nice touch) or drop it, so it does not read as an unused public helper.
| if (logo == null) { | ||
| return null; | ||
| } | ||
| return logos.computeIfAbsent(logo, name -> { |
There was a problem hiding this comment.
HashMap.computeIfAbsent does not store a null result, so when a logo resource is missing or unreadable the mapping function runs again on every render rather than being cached — despite the javadoc saying "cached". Same in logoBytes() just below.
Cheap in practice (a missing getResourceAsStream returns fast), but a sentinel value or a containsKey check would make the cache actually hold and match the doc.
Description
camel tuigets a new kind of AI provider for its F8 panel: an external coding agent spoken to over the Agent Client Protocol (ACP). The agent runs as a subprocess of the TUI, receives the TUI's own MCP server insession/new, and drives the TUI through the sametui_*tools that external MCP clients already use, while its answer streams into the panel.Why: users who already run Claude Code, Codex, Qwen Code, OpenCode, IBM Bob or DeepSeek Harness keep their login, model, memory, skills and slash commands, and the TUI needs no API key of its own. The LLM providers already in the panel are untouched.
What is in the change
Presets and settings
acp:claude(npx -y @agentclientprotocol/claude-agent-acp),acp:codex(npx -y @agentclientprotocol/codex-acp),acp:bob(bob acp),acp:qwen(qwen --acp),acp:opencode(opencode acp),acp:dsh(npx -y @deepseek-ai/dsh --profile acp), andacp:customwithcamel.tui.ai.acp.command. A preset whose executable is not on PATH shows an install hint; thenpxones need Node.js 22, which the Claude adapter declares.camel.tui.ai.acp.logos=auto|on|off). The Settings popup now scrolls the selected row into view on short terminals, which also fixes the "AI History" row being hidden at 25 lines.Protocol client (
AcpAgentClient, no new dependency)ACP v1 as newline-delimited JSON-RPC over stdio:
initialize,authenticate(one retry when the agent asks for it, otherwise the preset's login hint is shown),session/newcarrying the MCP server,session/promptwith streamedsession/update(text chunks, tool calls and their status, usage, available commands),session/cancel, andsession/request_permission. Errors, timeouts and an agent exit are reported in the conversation together with the agent's stderr tail. A JSON-RPC error from a healthy agent keeps the session alive.MCP on demand
When
--mcpwas not given, the embedded MCP server starts at the first prompt on 127.0.0.1 with a free port and no.mcp.json, and its URL is handed to the agent.Permissions
Calls to the TUI's own tools are approved automatically: a tool the TUI registry knows, identified by the adapter's
mcp__camel-tui__name or the Gemini-styletui_get_state (camel-tui MCP Server)title, and never a call of kind edit, delete, move, execute or fetch. Everything else opens a popup with the options the agent offers: Enter selects, Esc rejects, Ctrl+C cancels the turn.Panel integration
/clearopens a new session, a provider switch closes the agent./agent:<name>in the hints and TAB completion;/agent:alone lists them;/agent:<name>always reaches the agent, even when a panel command has the same name; any other unknown/xinput is forwarded as is./retry,/context,/compact,/toolsand/promptanswer for the agent instead of the LLM path.Header
Two rows under the panel title: the preset's logo where the terminal supports kitty graphics (uploaded once, placed by id, quiet mode, so it neither flashes nor answers on stdin) or a coloured glyph elsewhere, plus the agent's name and version, session id, working directory and command count.
Docs
New section "Using a coding agent (ACP)" in the TUI user manual.
Tests
83 new tests (
AiPanelAcpTest45,AcpAgentClientTest20,AcpHeaderStripTest9,AcpPermissionPopupTest8,TuiMcpServerPortTest1) around an in-process fake ACP agent over pipes; existing tests updated for the new settings rows and provider list.Notes for reviewers
src/main/resources/tui/logos/are the vendors' marks (Anthropic, OpenAI, IBM, Alibaba, OpenCode, DeepSeek). They are kept in the PR for the committers to decide; if ASF policy prefers not to ship them, the header falls back to the coloured glyph and the files can be dropped, or loaded from a user directory instead..cmdshims and the resolved path is what gets launched; unit-tested on Linux only, not exercised on Windows.AiPanelPromptBudgetTestfails on currentmain(6c5903d grew the tool schemas past its budgets); the three failures are identical without this branch.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes (run asmvnd clean install -DskipTests -Dlicense.skip=truefrom the root, 692 modules green, no generated changes; the license formatter was skipped only because it trips on untracked local files outside this change, and the module itself passed-Psourcecheckincluding the license check).AI-assisted contributions
Co-Authored-By: Claude Fable 5.1trailer. The code was written with Claude Code (model Claude Fable 5.1), reviewed and tested by the author.Claude Code on behalf of luigidemasi
🤖 Generated with Claude Code
https://claude.ai/code/session_018ppswyHx8nZfCvTTzZtkov