Repository navigation
Codex/separate agent core - #36
Conversation
Fixes 7 bugs surfaced by the cloud review of this branch: - agent-session: join the worker before returning its inference slot to the pool in delete_session (prevents slot reuse / KV-cache corruption when a new session reallocates the slot while the old worker is still streaming). - agent.cpp: reap the auto-spawned llama-server on double-Ctrl-C; std::exit(130) skips automatic destructors, so a file-scope pointer lets the signal handler call stop() before exiting (no more orphaned server holding the model in RAM). - http-inference-backend: detect exceed_context_size_error in both the SSE error branch and the non-2xx fallback so compaction-on-overflow recovery fires on the HTTP backend. - agent-loop-compaction: pin the summary request to the session's id_slot so it stays a cache hit and cannot evict another session's prompt cache. - tool-plan / CLI: add tool_result::no_truncate_display so the rich plan output is not cut mid-ANSI/UTF-8 by the 500-byte display cap. - permission-policy: match dangerous patterns by substring (not prefix) so compound commands like "cd build && rm -rf /" are still flagged dangerous. - http-inference-backend: only set image_support_known when /props actually reports modalities, so images aren't silently dropped against older servers. Adds unit tests for the compound-dangerous, context-overflow, and props-without-modalities cases.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 332d2a88a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::vector<std::string> args; | ||
| args.push_back(server_path.string()); | ||
| args.push_back("--host"); | ||
| args.push_back("127.0.0.1"); | ||
| args.push_back("--port"); | ||
| args.push_back(std::to_string(port)); | ||
| args.push_back("--parallel"); | ||
| args.push_back("1"); | ||
| args.push_back("--cache-prompt"); | ||
| args.push_back("--slots"); | ||
| args.push_back("--no-ui"); |
There was a problem hiding this comment.
When --backend is left at its new default auto and a sibling llama-server exists, the CLI now takes this path and starts a separate server, but the spawned argv only includes a small subset of the already-parsed options. Server-supported flags documented in tools/server/README.md and parsed in common/arg.cpp such as --lora, --flash-attn, --tensor-split, --no-mmap, and thread settings are silently dropped, so a command that previously loaded the requested adapters/runtime settings locally will run a different model/configuration instead. Please either forward the original supported arguments to llama-server or avoid auto-spawn when non-forwarded options are present.
Useful? React with 👍 / 👎.
Addresses the Codex P1 on PR #36. With --backend defaulting to auto, the CLI spawns a sibling llama-server but previously only forwarded a small subset of options (-m/-hf/-mu, --mmproj, -c, -b, -ngl). Flags that the local backend honors were silently dropped, so a command could run a different configuration than requested, including correctness-affecting ones like --lora and rope/yarn. try_auto_spawn_llama_server now forwards the user-supplied model/runtime options from the parsed common_params: LoRA adapters (--lora/--lora-scaled), threads (-t/-tb), --no-mmap, --mlock, -fa, -ctk/-ctv, -mg, -sm, -ts, --numa, -dev, and the rope/yarn settings. Only flags llama-server is known to accept are emitted (a rejected flag would make the child exit and stall the readiness wait), and enum/array values reuse the parser's canonical spellings so they round-trip.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ece22bcf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (available_slots_.empty()) { | ||
| return -1; |
There was a problem hiding this comment.
When this is backed by llama-server and all total_slots are already allocated, this returns -1 but create_session() still constructs the session; that -1 is copied into agent_cfg.inference_id_slot, and http_inference_backend::build_request_body() only sends id_slot when it is non-negative. In the scenario with more agent sessions than server slots, the extra session is therefore unpinned and llama-server can route it onto a slot already reserved for another session, evicting that session's prompt cache and breaking the slot isolation this pool is trying to provide. Please fail/queue session creation instead of silently creating an unpinned session once the pool is exhausted.
Useful? React with 👍 / 👎.
Replaces the hand-maintained allow-list added in the previous commit. Instead of enumerating flags by hand (which rots whenever llama-server gains an option, and silently drops new flags until someone updates the list), try_auto_spawn now: - re-parses the user's (agent-flag-stripped) argv with common_params_to_map to learn which options were explicitly provided, and - forwards each one that llama-server accepts, using the parser's own example filter (in_example(SERVER) || in_example(COMMON), minus excludes). New server flags are forwarded automatically with no changes here. Model selection and the fixed server-management flags (host/port/parallel/slots/no-ui/ cache-prompt) stay explicit and are skipped from the forwarded set to avoid duplicates. Two-value args (which common_params_to_map can't yet map) fall back to the local backend so nothing is silently dropped. Also hardens the readiness wait: it now detects the child exiting (waitpid WNOHANG) and bails immediately instead of blocking the full 120s, and clears the pid so stop() can't signal a recycled pid. This makes a rejected forwarded flag degrade to the local backend in milliseconds.
…sted Addresses a Codex P2 on PR #36. With a multi-slot llama-server backend (agent-server defaults to n_parallel=4), creating more sessions than slots made allocate_slot_locked() return -1, but create_session() still built the session. That -1 propagates to agent_cfg.inference_id_slot and build_request_body() omits id_slot when negative, so the extra session is unpinned and llama-server can route it onto another session's slot, evicting that session's prompt cache and breaking the isolation the pool provides. create_session() now refuses to create an unpinned session once the pool is exhausted (only when slots are actually pooled: is_llama_server && total_slots > 0, so the local backend's legitimate slot -1 is unaffected), returning an empty id. The create-session route surfaces this as HTTP 503.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fffb22212
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inference_request request; | ||
| request.messages = std::move(messages); | ||
| request.tools = chat_tools; | ||
| request.stream = true; | ||
| request.timings_per_token = true; | ||
| request.cache_prompt = true; | ||
| request.return_progress = true; | ||
| request.id_slot = config_.inference_id_slot; | ||
| request.parse_tool_calls = parse_tool_calls; |
There was a problem hiding this comment.
When llama-agent is run against an explicit HTTP backend (for example --backend http --server-url ... -n 32), common_params_parse still accepts the user's -n/--predict value, but this newly built inference_request leaves n_predict at its default -1. The HTTP backend only sends n_predict/max_tokens when that field is non-negative, so the remote server ignores the user's requested generation cap and uses its own default; local inference still inherits params.n_predict via params_from_json_cmpl, so this regression is specific to HTTP-backed runs.
Useful? React with 👍 / 👎.
The agent's tools self-register via REGISTER_TOOL static initializers compiled into the libllama-agent-core.a static library. llama-agent and llama-agent-server linked that archive without --whole-archive, so the linker dropped the unreferenced tool translation units and both binaries started with an EMPTY tool registry: the agent could describe actions in prose but never actually call read/write/edit/bash/ glob/update_plan. Confirmed on Linux (GNU ld) and macOS (ld64) — no _GLOBAL__sub_I_tool_*.cpp initializers in the built binary. Add an agent_force_link_core() helper that whole-archives the agent-core library into each consumer (--whole-archive on GNU ld, -force_load on Apple, /WHOLEARCHIVE on MSVC), using $<TARGET_FILE> so the archive path stays adjacent to the flags (CMake reorders bare target names out from between raw -Wl flags). Applied to both llama-agent and llama-agent-server.
The HTTP backend diverged from local_inference_backend on tool calling. The local
backend sends the tools natively (common_chat_tools_to_json_oaicompat + the model's
jinja chat-template parser) and never injects a text protocol. The HTTP backend
instead sent no tools field, set parse_tool_calls=false, and injected a hand-rolled
<tool_call>{...} XML protocol as plain text. Models trained for native tool use
(e.g. Qwen3) don't reliably follow that injected protocol — in testing Qwen3.6 echoed
the literal "{...}" placeholder and emitted empty/malformed arguments, so no tool ever
executed against a llama-server backend (now the default via --backend auto).
build_request_body now sends tools as a real field and lets llama-server parse them
(parse_tool_calls=true) when talking to a detected llama-server, mirroring the local
backend. The existing native_tool_calls accumulator already handles the streamed
response. The injected XML protocol remains the fallback for generic OpenAI endpoints
that don't support the tools field. Test updated to assert the native tools field.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f1babd92a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "-m", "--model", "-hf", "-hfr", "--hf-repo", "-hff", "--hf-file", "-mu", "--model-url", | ||
| }; | ||
|
|
||
| std::map<common_arg, std::string> user_args; |
There was a problem hiding this comment.
When auto-spawning llama-server, this new forwarding path first collapses the CLI into std::map<common_arg, std::string>, so repeatable options are reduced to one value before the loop forwards them. Fresh evidence is that this code now relies on common_params_to_map, whose map assignment keeps only one value per option, while the normal parser invokes handlers for each occurrence; for inputs like --lora a.gguf --lora b.gguf the local backend loads both adapters but the auto-spawned server receives only the last one, changing the requested model configuration.
Useful? React with 👍 / 👎.
llama_backend_init()/llama_numa_init() ran at startup, before the --backend auto decision. They dlopen every GGML backend and initialize the GPU runtime (CUDA/ Metal/...), so try_auto_spawn_llama_server's fork() then ran in a process with the GPU runtime already initialized (unsupported by NVIDIA/AMD/Apple), and the child ran non-async-signal-safe C++ between fork and execvp; a failed spawn would also fall back to the local backend on a forked CUDA context. Move both init calls into the local-backend branch so they only run when the local backend is actually used. Verified on an H100: the --backend auto parent now has zero libggml-cuda mappings and zero /dev/nvidia fds, and auto-spawn still works.
meta() returned a const& but refresh_meta() rewrote every field (incl. std::string) under a mutex on each call, while callers read the fields lock-free. In agent-server one local_inference_backend is shared across sessions (n_parallel=4), so multiple worker threads call meta() concurrently -> data race / UB. Populate meta_ once via std::call_once on the first call (values from server_ctx_ are stable after model load; first call happens during inference, i.e. post-load) and return the frozen, immutable snapshot thereafter. Drop refresh_meta()/meta_mutex_, matching http_inference_backend. (call_once rather than constructor-init because agent-server constructs the backend before load_model.)
When a prompt overflows context and compaction is disabled (--no-compaction) or try_compact() cannot free space (conversation too short), run() fell through to the empty-response branch and returned stop_reason=COMPLETED with empty output and no diagnostic -- a fake '[Completed]' with nothing printed. Emit an error and return AGENT_ERROR instead, mirroring run_streaming's handling of the same condition.
build_request_body set body[parse_tool_calls]=use_native_tools without consulting request.parse_tool_calls. A caller passing parse_tool_calls=false with a non-empty tools list would get native tool calls emitted by the server and then dropped by complete()'s else branch. Include request.parse_tool_calls in use_native_tools so we only ask the server to emit tool calls when the caller wants them parsed (latent today; mirrors local_inference_backend's tool_choice=none path).
cleanup() released the inference slot before erasing the session -- the inverse of delete_session's stop-then-release ordering. If an external shared_ptr outlives the erase, the slot could be reused while the old session/worker still issues inference pinned to it, corrupting the new session's prompt cache. Move expired sessions out, stop() them outside the lock, then release their slots. (cleanup() has no callers yet; this keeps the invariant correct for when an idle-sweep timer is added.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac7521eedd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void agent_session_manager::release_slot_locked(int32_t slot) { | ||
| if (slot >= 0 && slots_initialized_) { | ||
| available_slots_.insert(slot); | ||
| } |
There was a problem hiding this comment.
When a DELETE /session/:id or cleanup races with a POST /session/:id/chat that already obtained a shared_ptr from get_session(), the session is erased and this releases its id_slot even though that handler can still call send_message() on the old session object after stop() returned. A newly created session can then be allocated the same llama-server slot, so both workers issue pinned requests to one slot and can evict each other's prompt cache; please mark erased sessions closed or otherwise defer releasing the slot until no handler can start work on the old object.
Useful? React with 👍 / 👎.
github-actions
Bot
added
documentation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90aeaa12b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (arg == "--server-url") { | ||
| if (i + 1 < argc) { | ||
| server_url = argv[i + 1]; |
There was a problem hiding this comment.
When using the new explicit HTTP mode, e.g. llama-agent --backend http --server-url http://localhost:8080, this option is stripped here but the remaining argv is still parsed as LLAMA_EXAMPLE_CLI below, which enforces --model for non-server examples. As a result, remote-backed runs fail before reaching the HTTP backend unless the user supplies an unrelated local -m value; the HTTP path should either relax the CLI model requirement or parse/validate after deciding that inference is remote.
Useful? React with 👍 / 👎.
Clarifies where the model/server actually runs for each mode: local is in-process (no server/port), auto spawns a private llama-server child on a random loopback port, and http connects to a server you run yourself.
💡 Codex Reviewllama-agent/tools/agent/permission-policy.cpp Line 114 in b2d55fa Canonicalize paths before project-prefix checks
When a tool asks for a path containing llama-agent/tools/agent/tools/tool-bash.cpp Line 236 in b2d55fa Avoid re-waiting for already reaped bash children
When ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The agent constructs tool_result via the aggregate {success, output, error}
triple in ~56 places, leaving the remaining members (which have default member
initializers) unlisted. Apple clang accepts this, but the CI cuda / ubuntu-x64
jobs build with LLAMA_FATAL_WARNINGS=ON, where GCC's -Werror=missing-field-
initializers (part of -Wextra) turns each into a hard error. Add an explicit
tool_result(bool, string, string) constructor (plus a defaulted default ctor) so
those call sites are constructor calls rather than aggregate initialization,
which silences the warning everywhere at once. No call-site changes needed.
After the tool_result constructor fix, the LLAMA_FATAL_WARNINGS CI jobs (cuda, ubuntu-x64) surfaced three more -Werror classes in the agent, reproduced locally by building the agent targets with the same -Wall -Wextra -Wpedantic -Wcast-qual -Werror flags: - terminal-image.h: render_image_to_terminal's 'mime' parameter is unused (-Werror=unused-parameter) -> mark it [[maybe_unused]]. - stb-image-impl.cpp compiles the vendored stb_image.h, which trips -Werror=cast-qual -> add -Wno-cast-qual to the llama-agent and test-agent-inference-backend targets (the two that compile it), matching how tools/mtmd already handles the same header. - test-compaction.cpp: four variables are used only inside assert(), so they are unused under NDEBUG (Release) -> mark them [[maybe_unused]]. All agent binaries and unit tests now build clean under the CI strict flags.
💡 Codex Review Require Windows artifact before publishing release
I checked GitHub Actions job-if semantics: because this condition includes the status function llama-agent/tools/agent/mcp/mcp-client.cpp Line 143 in 43af6ba Fetch all MCP tool pages
I checked the MCP 2024-11-05 tools spec, and ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Overview
Additional information
Requirements