feat(cpp): gaia-bash — native C++ bash coding agent with TUI, API server, MCP server

[kovtcharov-amd]

Why this matters

Before: the GAIA C++ framework had an agent loop, LLM client, and tool registry — but no production CLI agent, no interactive TUI, no file I/O tools, no session persistence, and no way for external tools (Claude Code, OpenCode) to use GAIA agents.

After: gaia-bash is a fully functional native binary bash coding agent with five interfaces — interactive TUI, single-query CLI, pipe mode, REST API server, and MCP stdio server — plus a reusable C++ framework that any future agent can build on.

Verified: builds on Windows MSVC 2022, 431/435 tests pass (4 pre-existing WiFi test failures), MCP protocol tested end-to-end (tools/list, tools/call, prompts/list).

Threads

• C++ framework upgrades (M1): ProcessRunner, FileIOTools, GitTools, ReplRunner (2-thread with Ctrl-C cancel), TuiConsole (FTXUI + markdown renderer), SessionStore, tool argument validation — all reusable by future C++ agents
• gaia-bash agent (M2): BashAgent with bash_execute + env_inspect tools, bash-expert system prompt, CLI with argument parsing, slash commands (/run, /env)
• Integration layer: REST API server (OpenAI-compatible /v1/chat/completions, /v1/tools) and MCP stdio server (JSON-RPC tools/list, tools/call, prompts/list) for Claude Code / OpenCode integration
• Eval framework: 25 scenarios across 5 categories (script writing, review, tool usage, error handling, POSIX compliance) with ground truth and Python adapter

Test plan

• cmake -B build && cmake --build build on Windows MSVC 2022 — compiles clean
• tests_mock.exe — 431/435 pass (4 pre-existing WiFi failures)
• gaia-bash --help — prints usage
• echo '{"method":"initialize"}' | gaia-bash --mcp — MCP handshake works
• echo '{"method":"tools/list"}' | gaia-bash --mcp — returns 10 tools with JSON Schema
• echo '{"method":"tools/call","params":{"name":"bash_execute","arguments":{"command":"echo hello"}}}' | gaia-bash --mcp — executes command, returns stdout
• Linux/macOS build (needs CI)
• Interactive TUI mode (needs Lemonade Server + model)
• API server /v1/chat/completions (needs Lemonade Server)
• Eval scenario execution (needs Lemonade Server)

[github-actions]

gaia-bash is a well-architected addition — the layered separation between gaia_core framework upgrades and bash-specific code is clean, test coverage is unusually thorough for a feature this size, and the security story for git tools is done right. Two issues need resolution before merge: missing path validation in FileIOTools (the plan doc explicitly promised it) and a non-unique completion ID that will confuse OpenAI-compatible clients under load.

---

Issues Found

🟡 FileIOTools missing validatePath() — plan promised it, implementation skipped it

cpp/src/file_tools.cpp opens every path the LLM supplies — file_read, file_write, file_edit, file_search — without calling validatePath() from security.h. Compare with git_tools.cpp:153,162,234,269 which consistently calls isSafeShellArg() before building shell commands.

The plan doc (docs/plans/bash-agent.mdx, M1-P1 deliverables table) explicitly specifies: "All use validatePath() from security.cpp."

Impact: in --mcp mode and --serve mode the confirm callback is wired to ALLOW_ONCE (main.cpp:2309-2312 and main.cpp:2332-2334), so file_read (ALLOW policy) and file_write/file_edit (CONFIRM policy, but auto-allowed) accept any path the LLM or API caller supplies. The API server binds localhost-only (api_server.cpp:690), which limits blast radius, but the MCP stdio server has no network constraint — any LLM prompt injection that convinces the agent to read /etc/shadow or write a cron job will succeed.

The fix is to call validatePath(fs::current_path().string(), path) (or a user-configurable base path) before opening any file in doFileRead, doFileWrite, doFileEdit, and doFileSearch.

json FileIOTools::doFileRead(const json& args) {
    static constexpr size_t kMaxReadBytes = 32 * 1024;

    try {
        std::string path = args.value("path", "");
        if (path.empty()) {
            return json{{"error", "path is required"}};
        }

        // Prevent path traversal outside the current working directory.
        if (\!validatePath(fs::current_path().string(), path)) {
            return json{{"error", "Access denied: path escapes allowed directory: " + path}};
        }
        // ... rest of implementation

Apply the same pattern to doFileWrite, doFileEdit, doFileSearch.

---

🟡 Non-unique completionId under concurrent requests

api_server.cpp:190-194:

static std::string generateCompletionId() {
    auto now = std::chrono::system_clock::now().time_since_epoch();
    auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(now).count();
    return "chatcmpl-" + std::to_string(ms);
}

Two requests arriving in the same millisecond receive identical IDs. httplib runs a thread pool (api_server.cpp:252), so concurrent /v1/chat/completions requests will collide. Most OpenAI-compatible clients treat completion IDs as stable identifiers for caching, deduplication, or billing; duplicate IDs produce confusing client-side behaviour.

#include <atomic>

static std::string generateCompletionId() {
    static std::atomic<uint64_t> counter{0};
    auto now = std::chrono::system_clock::now().time_since_epoch();
    auto ms  = std::chrono::duration_cast<std::chrono::milliseconds>(now).count();
    return "chatcmpl-" + std::to_string(ms) + "-" + std::to_string(counter.fetch_add(1));
}

---

🟡 Streaming mode blocks until the full response is ready

api_server.cpp:414-500: handleStreamingCompletion calls agent.processQuery() synchronously, waits for the complete result, then sends the entire response as one SSE chunk followed by [DONE]. Clients that set "stream": true see no tokens until the model finishes — the same UX as non-streaming.

This isn't a correctness bug (SSE is valid), but it contradicts the documented streaming behaviour and will confuse clients like the Python openai SDK's streaming iterator. The user-facing doc (docs/cpp/bash-agent.mdx) shows streaming curl examples that imply token-by-token output. At minimum, the ApiServer header docstring at api_server.h:37-38 should note that streaming is currently simulated, and the user guide should match.

---

🟡 isToolAvailable passes unsanitised toolName to the shell

bash_tools.cpp:247-261:

bool BashTools::isToolAvailable(const std::string& toolName) {
    // ...
    std::string cmd = "which " + toolName + " >/dev/null 2>&1";
    ProcessResult result = ProcessRunner::run(cmd, 3000);

All current callers pass hardcoded string literals ("bash", "shellcheck", "docker", etc.), so there is no live injection path today. But the method is static on a public class (bash_tools.h:57) — any future caller passing an LLM-supplied string would be injectable. isSafeShellArg() is already available in security.h:

bool BashTools::isToolAvailable(const std::string& toolName) {
    if (toolName.empty() || \!isSafeShellArg(toolName)) {
        return false;
    }
    // ...

---

🟢 Default model in main.cpp doesn't match the user guide

main.cpp:2357:

config.modelId = "gemma-4-e4b";

docs/cpp/bash-agent.mdx (Configuration table) documents the default as Qwen3-Coder-Next-GGUF. lemonade_client.py also registers "gemma-4-e4b" in the "bash" AgentProfile. The docs and the code should agree, whichever is correct. If gemma-4-e4b is the intended default (for hardware accessibility), update the doc; if Qwen3-Coder-Next-GGUF is the intended default, update both main.cpp and lemonade_client.py.

---

Strengths

• Security in git tools is done right. git_tools.cpp calls isSafeShellArg() on every user-supplied ref and path argument before passing them into shell commands (lines 153, 162, 234, 269). The test suite at tests/test_git_tools.cpp:7339-7382 explicitly covers semicolon, pipe, backtick, and $(...) injection, and verifies the expected error string. This is the model the file tools should follow.
• Test coverage for a C++ feature this size is unusually strong. Six new test files with ~1500 lines covering round-trip serialization (test_session.cpp), output capping and timeout kill (test_process.cpp), JSONL event shape validation (test_json_event_handler.cpp), full REPL command dispatch without a real agent (test_repl.cpp), and TUI console rendering logic (test_tui_console.cpp). The fixtures are clean and the negative-path coverage is thorough.
• Python changes are minimal and correctly scoped. The faiss log filter, OSError guard in export_import.py, and the AgentProfile registration are all single-purpose, backed by tests, and don't touch unrelated paths.
• GAIA_BUILD_TUI=OFF in CI is the right call. The build_cpp.yml addition ensures the FTXUI FetchContent dependency doesn't gate unrelated CI runs, and TuiConsole is correctly compiled only when GAIA_HAS_TUI is defined.
• API server binds to 127.0.0.1 only (api_server.cpp:690), reducing the attack surface of the unauthenticated /v1/tools/:name endpoint to local processes.

---

Verdict

Request changes. The missing validatePath in FileIOTools contradicts the plan spec and creates a path-traversal gap in MCP/serve mode; the non-unique completion ID is a correctness bug for API clients. Both are small, targeted fixes. The streaming simulation issue and isToolAvailable latent injection are important but don't block the feature — they should be addressed in this PR or tracked as immediate follow-ups before gaia-bash ships to users.

[github-actions]

This is a strong PR that delivers a production-quality native binary — 9,600+ net lines, five operating modes, 10 tools, a real TUI, REST API, and MCP server. C++ code quality is high throughout: PIMPL in ApiServer, RAII InFlightGuard, proper ProcessRunner with fork/exec + non-blocking I/O, and cancellation support via cancelled_ atomic. The Python-side changes are small and well-tested. A few things need attention before merge.

---

Issues

🔴 Critical

Unrestricted file write and shell execution in MCP mode (cpp/agents/bash/main.cpp:2308–2316)

In --mcp mode (and --json-events mode), the confirm callback is replaced with unconditional ALLOW_ONCE:

mcpAgent.setToolConfirmCallback(
    [](const std::string&, const gaia::json&) {
        return gaia::ToolConfirmResult::ALLOW_ONCE;
    });

file_write and bash_execute both carry ToolPolicy::CONFIRM, which means they require user approval in interactive mode. Auto-allowing them in MCP mode is architecturally intentional — the comment says "external agent handles safety" — but there is no warning anywhere (docs, README, MCP server header, or the Claude Code config snippet in mcp_server.h) that anyone dropping gaia-bash --mcp into a Claude Code mcpServers entry is handing the host LLM unrestricted write access to any path the process can reach.

The eval scenario bash-error-readonly confirms the threat is understood ("Try to write the text 'test' to /etc/shadow") but the test only checks that the tool returns an error from the OS — it doesn't test the case where the path is writable.

Please add a prominent security note to docs/cpp/bash-agent.mdx in the MCP section and to the McpServer class doc, along the lines of:

Security: MCP mode auto-approves all tool calls (no per-operation confirmation). The host LLM can read/write any file reachable by the process and execute arbitrary shell commands. Run gaia-bash --mcp only from a process with a restricted working directory and filesystem permissions appropriate for your threat model.

@kovtcharov-amd please review before merge.

---

🟡 Important

Model ID applied only in interactive path (cpp/agents/bash/main.cpp:2357)

config.modelId = "gemma-4-e4b";

This line is only reached by the interactive/single-query branch. The --serve, --mcp, and --json-events branches construct their own AgentConfig without setting modelId, so they default to AgentConfig's built-in default ("Qwen3-4B-GGUF", types.h:309). Users who rely on the documented default of gemma-4-e4b for the API server or MCP server will be surprised. The bash profile in lemonade_client.py correctly lists gemma-4-e4b; the C++ side should match.

            gaia::AgentConfig apiConfig;
            apiConfig.debug = debug;
            apiConfig.contextSize = 32768;
            apiConfig.modelId = "gemma-4-e4b";
            if (!modelOverride.empty()) apiConfig.modelId = modelOverride;

Apply the same fix to mcpConfig and jeConfig.

---

Fake streaming in REST API — docs and surface claim otherwise (cpp/agents/bash/api_server.cpp:414–500)

handleStreamingCompletion processes the entire query first (calling agent.processQuery() to completion), then wraps the full result as a single SSE chunk. The implementation comment acknowledges this, but docs/cpp/bash-agent.mdx describes the API as supporting "streaming + non-streaming" without qualification. API consumers relying on incremental token delivery (e.g. for progressive display) will receive the entire response at once.

Either add a note to the docs:

Note: The streaming response is simulated — the full answer is computed before any SSE data is sent. True token-by-token streaming requires Lemonade Server support for SSE passthrough (planned).

Or rename the endpoint behavior in the ApiServer class doc. The current comment in api_server.cpp is correct; it just needs to surface in user-facing docs.

---

docs/plans/bash-agent.mdx missing from docs/docs.json

Every other plan doc in docs/plans/ is navigable through the docs site (docs/docs.json:393–408). docs/plans/bash-agent.mdx is not listed, so it won't appear in the sidebar. Add it:

              "plans/bash-agent",
              "plans/agent-ui",

---

🟢 Minor

Tool count discrepancy in docs (docs/cpp/bash-agent.mdx, overview section)

The overview claims "16 built-in tools" but registration registers 10: file_read, file_write, file_edit, file_search (4) + git_status, git_diff, git_log, git_show (4) + bash_execute, env_inspect (2). Correct to "10 built-in tools" or list the actual tools.

---

Quick start recommends wrong model (docs/cpp/bash-agent.mdx:~2938)

The quick start says gaia download Qwen3-Coder-Next-GGUF but the code default and the lemonade_client.py bash profile both specify gemma-4-e4b. Update to match.

---

std::cerr for startup messages (cpp/agents/bash/api_server.cpp:682–688)

std::cerr << "[ApiServer] Listening on port " << impl_->port << std::endl;

Other GAIA C++ agents route status through the console/output handler. These raw std::cerr prints bypass the output system and will be mixed with MCP transport if the server is ever started in a context where stderr is the MCP channel. Minor but worth noting for future-proofing.

---

Strengths

• ProcessRunner abstraction is the right approach — the two-path design (simple popen for no-timeout, fork+fcntl(O_NONBLOCK) polling loop for POSIX timeout, CreateProcess+threads for Windows) handles the edge cases correctly without busy-looping. The thread-safety note in the header doc is honest about the cwd/env limitation.
• isSafeShellArg used consistently in git_tools.cpp — all user-supplied path and ref parameters in git tool callbacks are validated before string-concatenation into shell commands. This is the right pattern.
• Test coverage is comprehensive — 9 new C++ test files covering process execution, file tools, git tools, session persistence, REPL commands, TUI console, and JSON event handler. The Python test_faiss_log_filter.py and test_export_import.py additions follow GAIA test conventions.

---

Verdict

Request changes — the MCP security boundary needs a documented warning before merge (🔴), and the model ID inconsistency across modes should be fixed (🟡). The fake-streaming doc gap and missing docs.json entry are quick fixes. The core implementation is solid and the PR is close to mergeable.

[github-actions]

Massive, well-architected PR — the C++ framework additions (ProcessRunner, SessionStore, ReplRunner, TuiConsole) are reusable and solid, and the Python-side changes are small and fully tested. One security issue in the MCP auto-allow path needs to be addressed before merge; the rest are important but not blocking on their own.

---

Summary

gaia-bash lands as a production-quality native binary with five interfaces, extensive tests (6 new C++ test files), and proper docs. The architecture is clean: ToolPolicy::CONFIRM guards destructive tools, validateId() blocks path traversal in session IDs, and the POSIX fork/exec timeout loop is correct. The Python changes — faiss log filter, export_import robustness, bash agent profile — are each small, purposeful, and tested.

The single blocking issue is that MCP mode and --json-events mode unconditionally bypass the ToolPolicy::CONFIRM guard on bash_execute, letting any MCP client or subprocess driver execute arbitrary shell commands without confirmation.

---

Issues Found

🔴 Critical — MCP and JSON-events modes bypass bash_execute confirmation

In cpp/agents/bash/main.cpp, both MCP mode and --json-events mode install a catch-all confirm callback that returns ALLOW_ONCE for every tool:

// main.cpp:2311
mcpAgent.setToolConfirmCallback(
    [](const std::string&, const gaia::json&) {
        return gaia::ToolConfirmResult::ALLOW_ONCE;
    });

bash_execute is declared ToolPolicy::CONFIRM precisely because it runs arbitrary shell commands. Silently downgrading it to ALLOW_ONCE in MCP mode means any MCP client (Claude Code, OpenCode, a script) can call tools/call with {"name":"bash_execute","arguments":{"command":"rm -rf /"}} and it executes without a prompt. The comment "the external agent handles safety" is not a sufficient guarantee — external callers are not trusted to enforce local system safety.

Required fix: Either keep CONFIRM policy active in MCP/JSON-events modes (surface the confirmation callback to the transport layer — e.g. log it and auto-allow only safe-policy tools), or add an explicit allowlist of auto-allowed tools and require bash_execute to stay gated:

// Only auto-allow ALLOW-policy tools; CONFIRM-policy tools still need a gate
mcpAgent.setToolConfirmCallback(
    [&mcpAgent](const std::string& name, const gaia::json&) {
        auto& tools = mcpAgent.tools().allTools();
        auto it = tools.find(name);
        if (it != tools.end() && it->second.policy == ToolPolicy::ALLOW) {
            return gaia::ToolConfirmResult::ALLOW_ONCE;
        }
        return gaia::ToolConfirmResult::DENY;  // or prompt via stderr
    });

This applies to both the --mcp block (main.cpp:2311) and the --json-events block (main.cpp:2333).

🔒 SECURITY CONCERN: arbitrary shell execution via MCP auto-allow bypass — @kovtcharov-amd please review before merge.

---

🟡 Important — prompts/get executes the agent query (MCP spec deviation)

cpp/agents/bash/mcp_server.cpp:2713 — handlePromptsGet calls agent_.processQuery(promptText), which runs the full agent loop against Lemonade. The MCP spec defines prompts/get as returning a message template for the host to inject into a conversation — not as a server-side execution endpoint. Callers expect to receive the prompt content, not a completed response.

This also means a prompts/get call blocks until the agent finishes (potentially minutes), violating the expectation that prompt listing/retrieval is fast. It silently shares the agent's inFlight_ guard with tools/call so concurrent prompts/get + tools/call returns a 409.

Fix: return the prompt template text only; let the host decide when and how to send it to the LLM.

---

🟡 Important — setHistory() data race with history()

cpp/include/gaia/agent.h:2829-2835:

void setHistory(std::vector<Message> history) {
    if (inFlight_.load()) {
        throw std::runtime_error("Cannot set history while processQuery() is running");
    }
    conversationHistory_ = std::move(history);  // no lock held
}

history() (line 2823) takes configMutex_ before reading conversationHistory_. setHistory() writes without acquiring the mutex. If another thread calls history() while a session resume calls setHistory(), there is a data race on conversationHistory_. Add the mutex to setHistory().

---

🟡 Important — CORS wildcard on local API server

cpp/agents/bash/api_server.cpp:259:

res.set_header("Access-Control-Allow-Origin", "*");

The server binds to 127.0.0.1 only, but a wildcard CORS header still allows any browser page to make credentialed cross-origin requests to the local API. A malicious website open in the same browser can reach POST /v1/tools/bash_execute and execute shell commands. Consider restricting to Origin: null (file:// origins) or a specific local-origin whitelist, or add token-based auth for the API endpoint.

---

🟢 Minor — docs say "16 built-in tools"; only 10 are registered

docs/cpp/bash-agent.mdx:8917 claims "16 built-in tools" but BashAgent::registerTools() registers: file_read, file_write, file_edit, file_search (4) + git_status, git_diff, git_log, git_show (4) + bash_execute, env_inspect (2) = 10 tools. The MCP tools/list endpoint will confirm 10. Update the copy or document the discrepancy.

---

🟢 Minor — std::cerr for startup diagnostics

cpp/agents/bash/api_server.cpp:681-688 — the startup banner and endpoint list go to std::cerr. In MCP mode stdout is the transport and stderr is the correct channel, but in --serve mode it's cleaner to use agent.console() or a logger abstraction consistent with the rest of GAIA. Minor: does not affect correctness.

---

Strengths

• ProcessRunner POSIX implementation (cpp/src/process.cpp) — the fork/exec loop with waitpid(WNOHANG), non-blocking pipes, SIGKILL on timeout, and final drain reads is correct and handles all the classic subprocess gotchas. The Windows CreateProcess + thread-per-pipe pattern is also sound.
• Session ID validation — SessionStore::validateId() rejecting non-alphanumeric/hyphen/underscore characters prevents path traversal attacks through session IDs. Good defensive design.
• Python changes are clean and fully tested — the faiss log filter has a complete 8-scenario test matrix covering every suppressed and kept message; test_exportable_agent_dirs_skip_unreadable_entries directly reproduces the fix's invariant using monkeypatching.
• GAIA_BUILD_TUI=OFF in CI (build_cpp.yml:9) — correctly gates the optional FTXUI dependency so the Windows MSVC build stays hermetic.

---

Verdict

Request changes — the MCP auto-allow bypass is a security issue that needs fixing before merge. The prompts/get spec deviation and setHistory race are important correctness concerns that should be addressed in the same pass. The rest can land as-is or as follow-ups.

[github-actions]

Massive PR — well-architected C++ agent framework with a real security gap in the API server that needs addressing before merge. The MCP server, ReplRunner, and eval framework are production-quality; the API server has two issues (one security, one functional) that are straightforward to fix.

---

Summary

gaia-bash lands a clean PIMPL ApiServer, a solid JSON-RPC McpServer, a reusable ReplRunner, and the first full-featured GAIA C++ CLI agent. The FTXUI TUI, session persistence, and eval framework are notably well executed. The system prompt (POSIX-first, set -euo pipefail, destructive-op warnings) is thoughtful. The only changes needed are (1) the CORS wildcard + ALLOW-policy tools creating a cross-site file-read vector, and (2) --serve mode silently disabling shell execution due to a missing confirm callback.

---

Issues Found

🔴 Critical — CORS wildcard exposes ALLOW-policy tools to any webpage (api_server.cpp:257-263)

🔒 SECURITY CONCERN @kovtcharov-amd

Every response adds Access-Control-Allow-Origin: *. The server binds to 127.0.0.1:8200. file_read, env_inspect, git_status, git_diff, git_log, and git_show all carry ToolPolicy::ALLOW (no confirmation). The combination lets any malicious webpage the user is browsing call POST http://localhost:8200/v1/tools/file_read and read arbitrary files, or env_inspect to probe the environment — no user interaction required.

bash_execute and file_write are safe here (they fail-closed in serve mode; see next issue), but the read surface is wide open.

Fix — drop CORS for the localhost API, or restrict it to a known origin:

    void addCorsHeaders(httplib::Response& res) {
        res.set_header("Access-Control-Allow-Origin", "http://localhost:5174");
        res.set_header("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS");
        res.set_header("Access-Control-Allow-Headers", "Content-Type, Authorization");
    }

If cross-origin access needs to remain configurable, make the origin a constructor parameter. Removing CORS headers entirely (so browsers enforce the same-origin policy) is the safest default for a localhost API.

---

🟡 Important — --serve mode silently disables shell execution (main.cpp serve block, tool_registry.cpp:186-189)

ToolRegistry::executeTool fails-closed when a CONFIRM-policy tool has no callback set:

if (!confirmCallback_) {
    return json{{"status", "error"},
                {"error", "Tool '" + resolvedName + "' requires confirmation but no callback is set"}};
}

The --serve block in main.cpp never calls setToolConfirmCallback, so bash_execute (and file_write, file_edit) will always return an error through the REST API — the agent's core capability is silently broken. The test plan marks [ ] API server /v1/chat/completions (needs Lemonade Server) as unchecked, which is likely why this wasn't caught.

MCP mode sets an auto-allow callback; API server mode should do the same (or document the limitation with a --no-confirm flag):

// In the --serve block, after creating apiAgent:
apiAgent.setToolConfirmCallback(
    [](const std::string&, const gaia::json&) {
        return gaia::ToolConfirmResult::ALLOW_ONCE;
    });

---

🟢 Minor — doGitDiff builds statCmd without reusing validated variables (git_tools.cpp:176-184)

ref and path are validated via isSafeShellArg then stored in local variables, but the second command (statCmd) re-fetches the raw strings from args rather than reusing the validated locals. Technically safe (the values can't change between the two blocks), but reads as if the second use is unvalidated and would become a bug after any refactor that reorders the checks:

    // Reuse already-validated ref/path locals for the --stat command
    if (!ref.empty()) {
        statCmd += " " + ref;
    }
    if (!path.empty()) {
        statCmd += " -- " + path;
    }

---

🟢 Minor — isToolAvailable is public but constructs a shell command from its argument (bash_tools.cpp:247-257)

std::string cmd = "which " + toolName + " >/dev/null 2>&1";

Called internally with hardcoded names only, but declared public. Any caller passing a name with shell metacharacters (;, |, $(…)) gets injection. Either make it private (nothing external needs it) or add a character-allowlist check mirroring isSafeShellArg.

---

🟢 Minor — Streaming mode blocks before sending headers (api_server.cpp:413-500)

handleStreamingCompletion calls agent.processQuery(userInput) (which can block for 30+ seconds) before setting Content-Type: text/event-stream. Clients with standard HTTP timeouts (5–30 s) will close the connection before receiving any data. The inline comment documents this honestly, but worth upgrading to a // TODO: true streaming requires async processQuery to track as a known gap.

---

Strengths

• Fail-closed CONFIRM default (tool_registry.cpp:186-189) is the right default; the security issue is specifically in the API server not opting in to auto-allow intentionally.
• isSafeShellArg discipline in git_tools.cpp — all user-supplied ref and path values are validated before concatenation into shell commands.
• FTXUI gated with GAIA_BUILD_TUI=OFF default-off for CI — clean optional-dependency pattern; the CI workflow toggle is correct.
• MCP protocol completeness — initialize, tools/list, tools/call, prompts/list, prompts/get all implemented with correct JSON-RPC 2.0 error codes. The prompts/get live-executing through the agent is a nice pattern.
• lemonade_client.py addition — minimal, correct; adds the "bash" AgentProfile in the right place with sensible defaults.

---

Verdict

Request changes — the CORS wildcard + ALLOW-policy read tools is a real data exfiltration vector via any malicious webpage; it needs a fix before merge. The missing confirm callback in --serve mode is a functional bug that would surface immediately on first integration test. Both are one-liner fixes. Everything else is minor.

[github-actions]

Substantial, well-structured PR that delivers a working native C++ bash coding agent with five usable interfaces. The framework code (ProcessRunner, SessionStore, FTXUI TUI) is clean and shows good engineering discipline — proper PIMPL, RAII guards, fail-closed confirmation policy. The security story for bash_execute is correctly handled: ToolPolicy::CONFIRM with no callback set in --serve mode means the agent loop can't fire shell commands without a confirm handler. One critical gap remains in the API server's CORS + unauthenticated tool access surface.

---

Issues

🔴 Critical — CORS wildcard + unauthenticated file_read in API server

@kovtcharov-amd — please review before merge.

api_server.cpp:259 sets Access-Control-Allow-Origin: * unconditionally. The server binds to 127.0.0.1, which looks safe — but any webpage open in the user's browser can cross-origin POST to localhost:8200 while the server is running. file_read has ToolPolicy::ALLOW (no confirmation required), so POST /v1/tools/file_read {"path": "/home/user/.ssh/id_rsa"} succeeds silently from a malicious tab. git_diff, git_log, git_status, git_show, and env_inspect are also ALLOW-policy and can be called the same way.

bash_execute and file_write are CONFIRM-policy and are correctly blocked (no confirmCallback_ set in --serve mode → fail-closed 500). But the read-only tools are fully open.

Two complementary fixes:

    void addCorsHeaders(httplib::Response& res) {
        res.set_header("Access-Control-Allow-Origin", "http://localhost:5174");
        res.set_header("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS");
        res.set_header("Access-Control-Allow-Headers", "Content-Type, Authorization");
    }

And consider gating POST /v1/tools/:name behind a pre-shared secret passed via --api-key (stored in the same ~/.gaia/security directory already used by SecurityStore). The GET read-only tool endpoints are lower priority.

---

🟡 Important — file_read ToolPolicy::ALLOW with no path bounds

file_tools.cpp (doFileRead): the tool accepts any absolute path — /etc/passwd, ~/.ssh/id_rsa, ~/.aws/credentials — without restriction. Since the policy is ALLOW, the agent can invoke it silently during any conversation. Even without the API-server CORS issue, prompt-injection attacks against the LLM could cause file exfiltration.

Consider adding a rootDir field to AgentConfig (default: cwd) and rejecting paths that resolve outside it:

json FileIOTools::doFileRead(const json& args) {
    static constexpr size_t kMaxReadBytes = 32 * 1024;

    try {
        std::string path = args.value("path", "");
        if (path.empty()) {
            return json{{"error", "path is required"}};
        }

        // Resolve and bounds-check against cwd to prevent exfiltration
        std::error_code ec;
        fs::path resolved = fs::weakly_canonical(fs::path(path), ec);
        if (\!ec) {
            fs::path cwd = fs::current_path(ec);
            if (\!ec) {
                auto rel = fs::relative(resolved, cwd, ec);
                if (\!ec && \!rel.empty() && rel.native().find("..") \!= std::string::npos) {
                    return json{{"error", "path escapes working directory: " + path}};
                }
            }
        }

This at least blocks naive ../../ traversals. A proper allowlist (project root) is stronger.

---

🟡 Important — generateCompletionId collision under concurrent requests

api_server.cpp:190-194:

static std::string generateCompletionId() {
    auto now = std::chrono::system_clock::now().time_since_epoch();
    auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(now).count();
    return "chatcmpl-" + std::to_string(ms);
}

Two requests within the same millisecond produce the same ID. Use an atomic counter:

static std::string generateCompletionId() {
    static std::atomic<uint64_t> counter{0};
    return "chatcmpl-" + std::to_string(counter.fetch_add(1, std::memory_order_relaxed));
}

---

🟡 Important — ProcessRunner::run with cwd is not thread-safe

process.cpp changes the process-level working directory with changeCwd(cwd) before spawning the child, then restores it. If two threads call ProcessRunner::run concurrently with non-empty cwd, they race on chdir(). Current callers pass cwd="" so this isn't triggered today, but the API invites future misuse. A single-line warning comment at the function signature prevents the next contributor from hitting it:

/// @warning The `cwd` parameter changes the process-level working directory and is NOT
/// thread-safe. Pass cwd="" (the default) if called from a multi-threaded context.
ProcessResult ProcessRunner::run(
        const std::string& command,
        int timeoutMs,
        const std::string& cwd,
        const std::map<std::string, std::string>& env,
        size_t maxOutputBytes) {

---

🟢 Minor — hardcoded /tmp in runSimple (POSIX path)

process.cpp:4986:

char tmpTemplate[] = "/tmp/gaia_stderr_XXXXXX";

/tmp isn't the standard temp dir on all systems (some distros, Android, etc.). The runWithTimeout path (which handles all positive-timeout calls, including doBashExecute) already avoids this, but runSimple is still callable:

        // mkstemp for safe temp file creation
        std::string tmpDir = []() -> std::string {
            std::error_code ec;
            auto p = std::filesystem::temp_directory_path(ec);
            return ec ? "/tmp" : p.string();
        }();
        std::string tmpl = tmpDir + "/gaia_stderr_XXXXXX";
        std::vector<char> tmpTemplate(tmpl.begin(), tmpl.end());
        tmpTemplate.push_back('\0');
        int fd = mkstemp(tmpTemplate.data());
        if (fd >= 0) {
            close(fd);
            stderrFile = std::string(tmpTemplate.data());
        }

---

🟢 Minor — MCP server auto-allows all tools with inline comment that says "external agent handles safety"

main.cpp:2311-2314:

mcpAgent.setToolConfirmCallback(
    [](const std::string&, const gaia::json&) {
        return gaia::ToolConfirmResult::ALLOW_ONCE;
    });

The comment "external agent handles safety" is accurate for Claude Code (which has its own tool confirmation UI), but documents an implicit security contract that future callers must honour. Consider making this explicit in the McpServer API so that callers must opt-in to auto-allow rather than silence being safe. For now, a one-line doc comment on the McpServer constructor or on run() would capture this invariant.

---

🟢 Minor — Linux/macOS CI gating is unchecked

The PR description explicitly notes the Linux/macOS build is unverified. The build_cpp.yml workflow only runs on Windows MSVC. POSIX-specific code paths (fork/exec, pipe, mkstemp) won't be exercised. Before merge, adding a Linux leg (even ubuntu-latest with g++) would catch the issues that the Windows build silently skips.

---

Strengths

• Fail-closed confirmation model is correct: bash_execute with ToolPolicy::CONFIRM and no confirmCallback_ set in --serve mode means the agent loop cannot fire shell commands without explicit handler registration. This is the right default.
• Session ID validation prevents path traversal: session.cpp:5891-5901 enforces alphanumeric + hyphen + underscore only before constructing the file path. Clean.
• isSafeShellArg applied consistently to all user-supplied git arguments: all four git tools validate ref and path before appending to the git command string. No injection gaps found.
• FTXUI gated and CI-disabled correctly: GAIA_BUILD_TUI=OFF in the CI cmake invocation avoids a hard dependency on FTXUI in environments that don't need it.
• Comprehensive eval framework: 25 scenarios across 5 categories with explicit ground-truth JSON and a Python adapter. This is the right level of investment for a new agent.

---

Verdict

Request changes — the CORS wildcard + unauthenticated file_read via the API server is a blocking issue. The fix is small (restrict the Allow-Origin header and/or add an API key gate). Everything else is important-but-non-blocking and can land in a fast-follow. The framework and agent code are high quality; this is close to mergeable.

[github-actions]

Solid engineering throughout — the framework primitives (ProcessRunner, ReplRunner, SessionStore, JsonEventOutputHandler) are genuinely reusable, the test coverage is good, and the five-interface design is clean. One CSRF-class security issue needs addressing before merge.

---

Issues

🔴 Critical — CORS wildcard on a localhost command-execution API (api_server.cpp:97)

@kovtcharov-amd — security concern.

addCorsHeaders() sets Access-Control-Allow-Origin: * on every response, including POST /v1/tools/bash_execute. In --serve mode all tool confirmations are auto-approved (ALLOW_ONCE). A malicious webpage in the user's browser can therefore cross-origin POST to http://localhost:8200/v1/tools/bash_execute and run arbitrary commands as the current user — a straightforward CSRF exploit.

Binding to 127.0.0.1 prevents external access, but CORS * explicitly grants browser JavaScript permission to cross-origin requests from any origin. The fix is to drop the wildcard or require an Authorization header the browser can't send cross-origin without a preflight you control.

    res.set_header("Access-Control-Allow-Origin", "http://localhost");
    res.set_header("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS");
    res.set_header("Access-Control-Allow-Headers", "Content-Type, Authorization");

Alternatively, gate on Origin: null (file://) or a pre-shared bearer token — whichever matches the intended clients (Python bridge vs browser WebUI).

---

🟡 Important — SessionStore::generateId() has an unprotected static (data race if called from multiple threads) (session.cpp:6118–6136)

static int counter and static std::string lastTimestamp are modified without a mutex. generateId() is called in ReplRunner::run() today, but SessionStore is shared with ApiServer's thread pool via setSessionStore(). If a future path (e.g. auto-saving on completion) calls generateId() from a request thread, this is UB.

std::string SessionStore::generateId() {
    static std::mutex mu;
    static int counter = 0;
    static std::string lastTimestamp;

    std::lock_guard<std::mutex> lock(mu);
    std::string ts = nowIdTimestamp();
    if (ts == lastTimestamp) {
        ++counter;
    } else {
        lastTimestamp = ts;
        counter = 0;
    }
    std::string base = "session-" + ts;
    return counter > 0 ? base + "-" + std::to_string(counter) : base;
}

---

🟢 Minor — Streaming is simulated (one chunk, not incremental) — needs a note in the API doc (api_server.cpp:418–500)

handleStreamingCompletion runs agent.processQuery() to completion, then emits the full result as a single SSE data chunk followed by [DONE]. Clients using streaming (e.g. the Python eval adapter with "stream": true) get no incremental tokens. The code comment acknowledges this, but the endpoint doc in bash-agent.mdx doesn't. Worth one sentence in the docs so integrators aren't surprised.

---

🟢 Minor — generateCompletionId() collisions under rapid requests (api_server.cpp:28–32)

IDs are millisecond timestamps (chatcmpl-<ms>). Two requests fired within 1 ms get the same ID. The agent's inFlight_ guard makes concurrent chat completions impossible (409), but /health or /v1/tools requests could theoretically race. A random nonce or a simple atomic counter appended to the timestamp would avoid this:

static std::string generateCompletionId() {
    static std::atomic<uint64_t> counter{0};
    auto now = std::chrono::system_clock::now().time_since_epoch();
    auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(now).count();
    return "chatcmpl-" + std::to_string(ms) + "-" + std::to_string(counter.fetch_add(1));
}

---

🟢 Minor — TUI code is never exercised in CI (build_cpp.yml:109)

-DGAIA_BUILD_TUI=OFF means tui_console.cpp, tui_markdown.cpp, and test_tui_console.cpp are always skipped. This is a reasonable trade-off (FTXUI is optional), but worth a short comment in the workflow explaining why it's off (build times / optional dep), so future contributors don't wonder.

                -DGAIA_BUILD_TUI=OFF \          # FTXUI optional dep; TUI tested locally

---

Strengths

• CSRF-aside, the security layering on bash execution is well done. ToolPolicy::CONFIRM protects all mutating tools; isSafeShellArg guards git arguments; single-quote escaping on POSIX is correct; isToolAvailable uses a hardcoded safe list, never user input.
• SessionStore ID validation is solid. validateId() allows only [a-zA-Z0-9_-] before constructing the file path, preventing path traversal even if a session ID is delivered via the API.
• ProcessRunner's thread-safety caveat is documented inline (process.h:213-217). The doc says "concurrent use only safe when cwd/env are empty (the default)" — this is accurate and easy for callers to verify.
• MCP server handles protocol corner cases correctly. notifications/initialized returns an empty result (not a method-not-found error), tools/list emits valid JSON Schema inputSchema, and isError detection covers both {"status":"error"} and {"error":"..."} patterns.
• Test coverage is genuine. Seven new test files cover process execution (including timeout and kill), file tools edge cases, git tools with safety validation, session save/load/list round-trips, and REPL command dispatch. All exercised without a live Lemonade server.

---

Verdict

Request changes. The CORS wildcard is a one-line fix but it enables CSRF → arbitrary code execution for any user with gaia-bash --serve running, which is too risky to ship as-is. Addressing that (plus the minor generateId race) unblocks merge.

[github-actions]

This is a landmark PR — shipping a fully functional native bash coding agent with five interfaces in one pass is a genuinely hard thing to land cleanly, and the overall execution is impressive. Two issues need to be addressed before merge: the streaming API silently blocks (users will see hangs), and gaia-bash doesn't install via cmake --install.

---

Issues

🟡 Simulated streaming blocks until completion (cpp/agents/bash/api_server.cpp:426-437)

handleStreamingCompletion calls agent.processQuery() to completion before writing a single SSE byte. Any client using "stream": true (Claude Code, OpenCode, curl) will hang silently for the full inference duration before receiving anything. The SSE contract is that the first chunk arrives quickly. This makes the streaming endpoint a footgun.

The cleanest fix before merge: reject "stream": true with a clear error instead of pretending to stream.

        if (stream) {
            res.status = 400;
            res.set_content(
                errorJson("Streaming is not yet supported. Use stream: false.",
                          "invalid_request_error").dump(),
                "application/json");
            return;
        }
        handleNonStreamingCompletion(res, userInput, model, completionId, created);

Or, document it prominently in docs/cpp/bash-agent.mdx and in the startup banner — whichever ships faster. The current silent-hang behavior will confuse every caller.

---

🟡 gaia-bash not in the install target (cpp/CMakeLists.txt)

The add_executable(gaia-bash ...) block has no install(TARGETS gaia-bash ...) call. cmake --install (which the CI install-test job exercises) will not deploy the binary. A developer following the CMake install workflow gets gaia_core but not the agent.

    install(TARGETS gaia-bash
        RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
    )

Add this at the end of the if(GAIA_BUILD_BASH_AGENT) block (after the target_link_libraries calls).

---

🟡 cmd.exe /C fallback has no escaping (cpp/agents/bash/bash_tools.cpp:85-86)

The POSIX path escapes single quotes; the Windows+bash path escapes \ and ". But when no POSIX shell is found on Windows, the command goes to cmd.exe unescaped:

fullCommand = "cmd.exe /C " + command;

cmd.exe metacharacters — %VAR% expansion, &, |, >, ^ — will be interpreted in ways the user didn't express. At minimum, a comment flagging the behavior difference would help; ideally, wrap in cmd.exe /C ""<escaped>"" following cmd.exe quoting rules, or document that without a POSIX shell, cmd.exe semantics apply.

---

🟡 Linux/macOS CI not run

The PR test plan explicitly marks "Linux/macOS build (needs CI)" as unchecked. The CI only exercises the Windows MSVC build path. The POSIX fork/exec path in process.cpp:390-413, the bash -c '...' wrapping in bash_tools.cpp:91-103, and the file/git tools all need a Linux run. Since the existing build-and-test job already covers Linux, adding -DGAIA_BUILD_BASH_AGENT=ON (it's ON by default) to that job's cmake flags — and running ./gaia-bash --help as a smoke test — would close this gap without new infrastructure.

---

🟢 generateCompletionId() is not unique under load (cpp/agents/bash/api_server.cpp:190-193)

Millisecond-based IDs collide if two requests arrive in the same millisecond. The agent rejects concurrent processing with 409, but IDs are generated before the rejection. A simple fix:

static std::string generateCompletionId() {
    static std::atomic<uint64_t> counter{0};
    auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(
        std::chrono::system_clock::now().time_since_epoch()).count();
    return "chatcmpl-" + std::to_string(ms) + "-" + std::to_string(counter++);
}

---

🟢 docs/plans/bash-agent.mdx not wired into docs navigation

docs/cpp/bash-agent.mdx correctly appears in docs.json (line 45). docs/plans/bash-agent.mdx does not — other plans (plans/agent-ui, plans/setup-wizard) are all wired in. Add "plans/bash-agent" to the plans section of docs.json, or move the file to docs/spec/ if it's not meant to be public-facing.

---

🟢 AgentProfile model name diverges from C++ default (src/gaia/llm/lemonade_client.py:282-286)

Python uses models=["gemma-4-e4b"]; the C++ binary defaults to "Gemma-4-E4B-it-GGUF". These likely resolve to the same model through the GAIA registry, but a comment linking them would prevent a future contributor from "fixing" one side without updating the other.

---

Strengths

• Test depth is excellent. Six new test files, 430+ tests covering ProcessRunner, FileIOTools, GitTools, SessionStore, ReplRunner, JsonEventHandler — and explicit injection-rejection tests in test_git_tools.cpp that verify security module validation actually fires.
• Session ID path traversal is properly blocked. SessionStore::validateId restricts IDs to [a-zA-Z0-9_-] before constructing filesystem paths; the API server inherits this protection automatically.
• MCP protocol correctness. All debug/status output goes to stderr; stdout is exclusively JSON-RPC, matching the spec. The notifications/initialized no-response case is handled correctly.
• API server localhost-only binding. server.listen("127.0.0.1", port) rather than 0.0.0.0 is the right default for a local dev tool.
• FTXUI optional build is clean. The GAIA_BUILD_TUI guard correctly saves and restores BUILD_SHARED_LIBS around FTXUI (which can't be built as a DLL), and the CI correctly sets -DGAIA_BUILD_TUI=OFF for the headless install test.

---

Verdict

Request changes — streaming and install target are blocking. The streaming issue will confuse every integration test that uses stream: true; the missing install rule means cmake --install doesn't deploy the binary. Both are small targeted fixes. The rest of the PR is shippable once those are addressed.

[itomek]

@kovtcharov-amd — impressive, genuinely well-architected subsystem, and the bot passes have enumerated the line-level items thoroughly, so I'll focus on what most affects how this lands. The strengths are real: the fork/exec + non-blocking-pipe ProcessRunner is correct, the isSafeShellArg discipline in the git tools is the model the file tools should follow, session-ID validation blocks path traversal, and the MCP JSON-RPC handling (incl. the notifications/initialized no-response case) is spec-correct.

One thing worth your judgment, not a blocker: at ~9.5K additions across 52 files this bundles four separable deliverables — the reusable gaia_core framework primitives, the bash agent, the three integration servers, and the eval framework. The framework layer is the lower-risk, independently-testable part; landing it as one commit means any review friction on the servers gates the framework too. If splitting is still feasible (framework first, then agent + servers on top), each piece gets a faster focused review and the framework starts paying off for future C++ agents sooner. If not practical this late, that's your call — flagging because the blast radius of one 52-file merge is large.

Two smaller items inline (ms-only completion IDs can collide; wildcard CORS is low-risk given the 127.0.0.1 bind and local-only model but tidier pinned). The early "binds to 0.0.0.0" bot findings are stale — head correctly binds 127.0.0.1. Also worth a docs reconciliation: the streaming is simulated (single SSE chunk) and the tool count differs from what's registered, so first-run users aren't surprised. Approving.

---

Generated by Claude Code

[itomek]

generateCompletionId is still millisecond-only in head, so two requests in the same ms get identical IDs. Append an atomic counter (+ "-" + counter.fetch_add(1)). Also: addCorsHeaders sets Access-Control-Allow-Origin: * — since everything binds 127.0.0.1 and runs locally this is low-risk for GAIA, but pinning to the specific local UI origin (or dropping the header to let the browser enforce same-origin) would be tidier if --serve ever gains an auto-allow confirm callback. Optional.

---

Generated by Claude Code

[github-actions]

Solid native C++ bash agent with a well-thought-out five-interface design; no blocking issues found. The security-critical paths (localhost-only API binding, session ID validation, ALLOW_ONCE callback in serve mode) are all correctly handled. Everything below is minor polish.

---

Summary

gaia-bash lands with a clean architecture: five mutually exclusive runtime modes (TUI, CLI, pipe, REST API, MCP stdio) each with appropriate tool-confirmation behavior set at startup. The ProcessRunner, FileIOTools, GitTools, SessionStore, and TuiConsole are properly separated reusable framework components. Test coverage spans all new subsystems (431/435 passing). The one Python change is a minimal AGENT_PROFILE addition.

The most important thing for the author to know: the Windows runWithTimeout wraps an already-shell-wrapped command a second time (cmd /c bash -c "..."). It works in practice but is redundant and worth tidying before more agents share this codepath.

---

Issues Found

🟢 Minor — Windows double-shell invocation in runWithTimeout (process.cpp:5128)

bash_tools.cpp builds fullCommand = "bash -c \"<escaped>\"" or "cmd.exe /C <cmd>", then passes it to ProcessRunner::run(). The Windows timeout path in runWithTimeout prepends another cmd /c, producing cmd /c bash -c "...". This works because cmd /c will find bash on PATH, but it's unnecessary wrapping and the exit-code path goes through two shells instead of one. The no-timeout path (runSimple → _popen) doesn't add the extra layer, so the two paths behave slightly differently. For the case where no POSIX shell is available and the fallback is cmd.exe /C <cmd>, the result is cmd /c cmd.exe /C <cmd>, which is also redundant.

Consider having runWithTimeout on Windows accept and execute the command string directly via CreateProcessA without the cmd /c prefix — the command is already fully formed by the caller.

    // Build command line for CreateProcess — command is already fully formed by caller.
    std::string cmdLine = command;

---

🟢 Minor — SIGKILL on timeout skips SIGTERM (process.cpp:5281)

kill(pid, SIGKILL);

This is fast but leaves no cleanup window for children that hold temp files, advisory locks, or interprocess connections. The 30-second default timeout in bash_execute is long enough that a short SIGTERM grace period (e.g. 500 ms) wouldn't be noticed by users but would allow well-behaved scripts to clean up:

        // Give the process a short window to clean up before force-killing.
        kill(pid, SIGTERM);
        std::this_thread::sleep_for(std::chrono::milliseconds(500));
        if (waitpid(pid, nullptr, WNOHANG) == 0) {
            kill(pid, SIGKILL);
            waitpid(pid, nullptr, 0);
        }

---

🟢 Minor — Health endpoint example shows "tools":16 (docs/cpp/bash-agent.mdx)

The bash-agent doc shows:

# {"status":"ok","model":"Qwen3-Coder-Next","tools":16}

Only 9 tools are registered at startup (4 FileIO + 4 Git + 2 Bash). 16 was the count in a different context. Update the example to match, or use a placeholder like "tools": 9.

---

🟢 Minor — api_server.cpp:572 comment is misleading

// Execute through the mutable toolRegistry() to allow policy checks.
json result = agent.toolRegistry().executeTool(toolName, args);

The comment implies the CONFIRM policy is enforced here, but in --serve mode the agent already has ALLOW_ONCE set globally (main.cpp:2285). The real reason to call via toolRegistry() rather than agent.processQuery() is to bypass the LLM and run the tool directly. Update to:

        // Direct tool execution — bypasses LLM and executes the callback directly.
        // Policy is already set to ALLOW_ONCE for API server mode (see main.cpp).
        json result = agent.toolRegistry().executeTool(toolName, args);

---

🟢 Minor — env_inspect broad catch (...) swallows failures silently (bash_tools.cpp:1090)

Each tool-check block catches ... and returns "bash not available" / "unknown" without even a debug-mode log. A surprising bash environment (POSIX tool returning unexpected output, execution blocked by seccomp, etc.) will silently degrade to unhelpful strings. At minimum, write to stderr when debug mode is active:

    } catch (const std::exception& e) {
        shellVersion = std::string("bash not available: ") + e.what();
    } catch (...) {
        shellVersion = "bash not available (unknown error)";
    }

---

🟢 Nit — Plan doc status says "Implemented" with unchecked test-plan items (docs/plans/bash-agent.mdx:line 9499)

The <Info>Status: Implemented</Info> block reads confidently, but the PR test plan has three unchecked items: Linux/macOS build CI, interactive TUI mode, and eval scenario execution (all require Lemonade Server). "M1 Complete / M2 In Progress" or "Partially Implemented" would be more accurate.

---

Strengths

• Session ID validation in SessionStore::validateId (alphanumeric + - + _ only) is tight and the corresponding tests cover path traversal, hidden files, dots, and empty string — exactly the right threat model for a filesystem-backed store (cpp/tests/test_session.cpp:8645).
• POSIX shell quoting in bash_execute uses the correct '\'' pattern for single-quote escaping; the Windows path escapes only \ and " inside a double-quoted wrapper, which is correct for bash -c "..." on Git Bash / MSYS2.
• CMake FTXUI isolation — saving and restoring BUILD_SHARED_LIBS before FetchContent_MakeAvailable(ftxui) is the right call; FTXUI exports no symbols for DLL builds and would cause LNK1181 on MSVC without this guard (cpp/CMakeLists.txt:29-37).
• Five-mode tool-confirmation handling is consistent: --mcp, --json-events, and --serve all explicitly call setToolConfirmCallback(ALLOW_ONCE) with clear comments; only the interactive REPL path uses the default confirmation dialog.

---

Verdict

Approve with suggestions. All five runtime modes are production-quality, security boundaries are correctly placed (localhost-only REST, session ID validation, no CONFIRM-policy blocking in server mode), and the test suite is comprehensive. The Windows double-wrapping and SIGKILL nits are worth cleaning up before this codebase gets more users, but neither is a blocker.

[github-actions]

This is a substantial, well-engineered milestone for the C++ framework — ProcessRunner, FileIOTools, GitTools, SessionStore, ReplRunner, TuiConsole, ApiServer, and McpServer all land together with test coverage. Two thread-safety issues and a localhost SSRF surface in --serve mode need attention before merge.

---

Issues Found

🟡 Important — localhost SSRF: CORS * + auto-allow bash_execute in --serve mode (cpp/agents/bash/api_server.cpp:259–261, cpp/agents/bash/main.cpp:2285–2287)

The API server sends Access-Control-Allow-Origin: * and handles the preflight OPTIONS request, so browsers will honour the CORS grant. At the same time, the --serve mode auto-allows every tool call — including bash_execute (CONFIRM-policy tool). The combination means that any webpage the user has open while gaia-bash --serve is running can POST to http://127.0.0.1:8200/v1/chat/completions and trigger arbitrary shell command execution with no user prompt. The 127.0.0.1 bind address is the only firewall.

CORS wildcard on a local server that exposes shell execution is the standard localhost SSRF setup that has been exploited in many local-tool products (Jupyter, VS Code Server, etc.).

Two hardening options:

Option A — restrict CORS (preferred):

        res.set_header("Access-Control-Allow-Origin", "http://localhost");
        res.set_header("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS");
        res.set_header("Access-Control-Allow-Headers", "Content-Type");

Option B — require a random token (X-Gaia-Token: <token-printed-at-startup>) and reject requests that omit it. A browser-origin cross-site request cannot set custom headers without CORS cooperation, so this also closes the attack. The Authorization header approach already works since browsers won't send it cross-origin without the server's permission.

@kovtcharov-amd — flagging because this is a security-relevant design decision for any user who runs --serve.

---

🟡 Important — setHistory() missing configMutex_ while history() (getter) holds it (cpp/include/gaia/agent.h:2873–2878)

history() takes configMutex_ before reading conversationHistory_; setHistory() does not hold that same mutex when writing. The inFlight_ check guards against processQuery() concurrent calls, but another thread that holds the result of history() or calls it concurrently with setHistory() can still race.

    void setHistory(std::vector<Message> history) {
        if (inFlight_.load()) {
            throw std::runtime_error("Cannot set history while processQuery() is running");
        }
        std::lock_guard<std::mutex> lock(configMutex_);
        conversationHistory_ = std::move(history);
    }

---

🟡 Important — SessionStore::generateId() has non-thread-safe function-local statics (cpp/src/session.cpp:6152–6153)

static int counter and static std::string lastTimestamp inside the function body are not mutex-protected. Concurrent calls from, e.g., the ApiServer's per-request handlers, can race and produce duplicate IDs.

std::string SessionStore::generateId() {
    static std::mutex idMutex;
    static int counter = 0;
    static std::string lastTimestamp;

    std::lock_guard<std::mutex> lock(idMutex);
    std::string ts = nowIdTimestamp();
    if (ts == lastTimestamp) {
        ++counter;
    } else {
        lastTimestamp = ts;
        counter = 0;
    }
    std::string base = "session-" + ts;
    return counter == 0 ? base : base + "-" + std::to_string(counter);
}

---

🟢 Minor — cmd.exe /C fallback passes the LLM command with no escaping (cpp/agents/bash/bash_tools.cpp:1015)

When no POSIX shell is found on Windows, bash_execute falls back to fullCommand = "cmd.exe /C " + command; — &, |, >, <, &&, || are cmd.exe metacharacters and will be interpreted. The CONFIRM policy gates this interactively but not in --serve/--print/MCP modes (which all auto-allow). One-line fix:

        fullCommand = "cmd.exe /C \"" + command + "\"";

Wrapping in outer quotes suppresses most injections via metacharacters. For belt-and-suspenders, consider escaping ^, &, |, >, < as ^<char> inside the outer quotes.

---

🟢 Minor — doGitDiff stat-command path re-concatenates ref without calling isSafeShellArg() a second time (cpp/src/git_tools.cpp:4527–4530)

Not an exploitable bug today (the ref was already validated above and an early return would have happened), but it's fragile: if the stat block were ever hoisted above the guard, it becomes injection. Extract the validated ref into a local at the top and reuse it:

    std::string safeRef;
    if (args.contains("ref") && args["ref"].is_string()) {
        safeRef = args["ref"].get<std::string>();
        if (!isSafeShellArg(safeRef)) {
            return json{{"error", "Invalid ref argument: contains unsafe characters."}};
        }
        cmd += " " + safeRef;
    }
    // ... later, for statCmd:
    if (!safeRef.empty()) {
        statCmd += " " + safeRef;
    }

---

🟢 Minor — generateCompletionId() not unique under concurrent burst (cpp/agents/bash/api_server.cpp:192–194)

Millisecond timestamp only; two requests landing within the same millisecond get the same chatcmpl- ID. Add a counter or a random suffix:

static std::string generateCompletionId() {
    auto now = std::chrono::system_clock::now().time_since_epoch();
    auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(now).count();
    static std::atomic<int> seq{0};
    return "chatcmpl-" + std::to_string(ms) + "-" + std::to_string(seq.fetch_add(1));
}

---

Strengths

• Security-conscious git tools: isSafeShellArg() is correctly applied to every LLM-supplied path and ref arg in git_diff, git_log, and git_show. The hardcoded-subcommand pattern ("git diff --staged " + safeRef) keeps the injection surface small.
• SessionStore ID validation: validateId() uses a strict allowlist (alphanumeric/hyphens/underscores) — clean path-traversal prevention. pathForId() never constructs the path without going through validation first.
• Tool arg schema validation: The new validateArgsAgainstSchema() in ToolRegistry gives all registered tools free required-field and type-checking without per-tool boilerplate. The fallback from custom validateArgs to schema validation is a good layered design.
• Cancel support: requestCancel() / cancelled_ atomic with reset at the start of each processQuery() is a clean two-thread pattern. The SIGINT → cancel → REPL-continues flow in ReplRunner avoids the kill-the-process pitfall.
• TUI as optional build target: GAIA_BUILD_TUI=OFF in build_cpp.yml CI correctly guards the FTXUI dependency from blocking builds on systems without it.

---

Verdict

Request changes. Three 🟡 issues (CORS/SSRF, setHistory lock, generateId race) need to be resolved before merge. All have concrete one-line-to-five-line fixes shown above. The CORS concern in particular should get a design decision sign-off from @kovtcharov-amd since it touches the security model for the --serve mode. The 🟢 items are safe to fix in a follow-up or address in the same pass.

[github-actions]

🟡 cpp/agents/bash/bash_tools.cpp — silent fallback to cmd.exe violates the fail-loudly rule

When no POSIX shell is found on Windows, doBashExecute silently falls back to cmd.exe:

} else {
    // No bash/sh available — run via cmd.exe directly
    fullCommand = "cmd.exe /C " + command;
}

The tool description promises bash/sh execution ("runs in the detected shell (bash preferred, sh fallback)") and the LLM will generate POSIX syntax. Silently routing that through cmd.exe produces confusing failures (array syntax, [[, $() substitutions, etc.) with no indication to the caller that the environment changed — exactly the "default-to-something-that-works-ish" pattern CLAUDE.md prohibits.

} else {
    return {{"error", "No POSIX shell (bash or sh) found. "
                      "Install Git Bash or enable WSL to use bash_execute on Windows."}};
}

The same path also concatenates command directly into the cmd.exe /C string with no escaping, so a command containing & or | would silently fork additional cmd.exe processes.

[github-actions]

🔴 CSRF/RCE via wildcard CORS + unauthenticated local API server — cpp/agents/bash/api_server.cpp:309, cpp/agents/bash/main.cpp (~line 2334)

The API server sets Access-Control-Allow-Origin: * and has no authentication. In --serve mode, all tool confirmations are auto-allowed:

// API server has no stdin — auto-allow all tool confirmations
apiAgent.setToolConfirmCallback(
    [](const std::string&, const gaia::json&) {
        return gaia::ToolConfirmResult::ALLOW_ONCE;
    });

Any webpage the user visits can fire a cross-origin POST /v1/tools/bash_execute (or a crafted chat prompt to /v1/chat/completions) that runs arbitrary shell commands on the user's machine with no confirmation. The CONFIRM policy on bash_execute is silently voided in this mode.

Minimum fix: restrict CORS to localhost origins only (not *), and/or require a pre-shared token header that the browser can't forge cross-origin.

// Instead of "*":
res.set_header("Access-Control-Allow-Origin", "http://localhost:" + std::to_string(port));

@kovtcharov-amd — this needs a look before merge.