Skip to content

CAMEL-24663: camel-jbang-plugin-tui - drive the TUI from coding agents over the Agent Client Protocol (ACP) - #26244

Open
luigidemasi wants to merge 10 commits into
apache:mainfrom
luigidemasi:tui-acp-prototype
Open

CAMEL-24663: camel-jbang-plugin-tui - drive the TUI from coding agents over the Agent Client Protocol (ACP)#26244
luigidemasi wants to merge 10 commits into
apache:mainfrom
luigidemasi:tui-acp-prototype

Conversation

@luigidemasi

Copy link
Copy Markdown
Contributor

Description

camel tui gets 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 in session/new, and drives the TUI through the same tui_* 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), and acp:custom with camel.tui.ai.acp.command. A preset whose executable is not on PATH shows an install hint; the npx ones need Node.js 22, which the Claude adapter declares.
  • Gemini CLI and Google Antigravity are deliberately not offered: Gemini CLI stopped serving personal accounts and Antigravity has no ACP support yet.
  • F2 Settings rows "ACP Command" and "ACP Logos" (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/new carrying the MCP server, session/prompt with streamed session/update (text chunks, tool calls and their status, usage, available commands), session/cancel, and session/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 --mcp was 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-style tui_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

  • Streamed answer with elapsed time and the tokens the agent reports; one status line per tool call, updated in place.
  • Esc cancels the turn. The next prompt waits for the cancelled turn's response, as ACP allows the agent to keep streaming until then, so late chunks cannot land in the next answer.
  • /clear opens a new session, a provider switch closes the agent.
  • The agent's slash commands and skills appear as /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 /x input is forwarded as is.
  • The panel's own /retry, /context, /compact, /tools and /prompt answer 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 (AiPanelAcpTest 45, AcpAgentClientTest 20, AcpHeaderStripTest 9, AcpPermissionPopupTest 8, TuiMcpServerPortTest 1) around an in-process fake ACP agent over pipes; existing tests updated for the new settings rows and provider list.

Notes for reviewers

  • ACP v1 is targeted; v2 is still a draft.
  • The six PNGs under 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.
  • Windows: the PATH check resolves .cmd shims and the resolved path is what gets launched; unit-tested on Linux only, not exercised on Windows.
  • AiPanelPromptBudgetTest fails on current main (6c5903d grew the tool schemas past its budgets); the three failures are identical without this branch.
  • Exercised locally with Claude Code, Codex, OpenCode and IBM Bob; Qwen Code verified up to the handshake and session; DeepSeek Harness defined from its documentation and not exercised.

Target

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it): CAMEL-24663

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes (run as mvnd clean install -DskipTests -Dlicense.skip=true from 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 -Psourcecheck including the license check).

AI-assisted contributions

  • Every commit carries a Co-Authored-By: Claude Fable 5.1 trailer. 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

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in AiPanel
  • Lazy agent spawning (no freeze on slow npx downloads)
  • FakeAcpAgent test 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 volatile for cross-thread fields (acpClient, acpSessionId, acpAgentInfo, etc.)
  • ConcurrentHashMap for pending requests, synchronized for writer access
  • closeAcpClient() runs agent.close() on a daemon thread to avoid blocking the TUI event thread — nice touch
  • The AcpTurnListener methods are synchronized, which correctly prevents interleaving between the reader thread and the agent thread's finish() call

Potential concerns (minor):

  1. AcpAgentClient.readLoop() line parsingJsoner.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.

  2. AcpPermissionPopup thread safety — The open() method sets several fields before setting visible = true (the publishing write). This relies on volatile providing a happens-before relationship. The comment documents this intentionally, which is good, but synchronized on the whole popup would be more conventional.

  3. 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.

  4. Settings popup scrolling — The new scroll logic in SettingsPopup is a welcome fix for short terminals. The lineOf() static method correctly accounts for divider positions.

  5. TuiMcpServer.getPort() ephemeral port support — Clean addition. The volatile on mcpPort in CamelMonitor is appropriate since ensureMcpServer() 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

luigidemasi and others added 10 commits September 9, 2026 20:56
…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
@davsclaus

Copy link
Copy Markdown
Contributor

rebased so the tui test with the budget is fixed

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 constantsROW_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 davsclaus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 about AiPanelPromptBudgetTest failing is now stale — it passes here (3/3). Worth removing from the description.
  • Checked git log / git blame on TuiMcpServer, TuiToolRegistry, McpFacade and AiProviderSelector for 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:custom never appears in Ctrl+P until camel.tui.ai.acp.command is 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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants