[#1146] Route HTTP commands through BusCommandRouterProcessor in-process#1168
Conversation
Extract BusCommandRouterProxyClient and CommandRouterHttpAPIConfig so the HTTP API uses a dedicated issuer ServiceBus with configurable WebSocket chunk size, while bus_client keeps its original terminal behavior.
Extract HTTP-related sources and Bazel targets into agents/command_router/http_api and update main and test BUILD deps to the new package path.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds HTTP command dispatch and chunked streaming to the command router, extends HTTP API configuration, moves the HTTP API into its own Bazel package, and updates singleton startup wiring and tests. ChangesCommand router HTTP API
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpAPI as CommandRouterHttpAPI
participant Processor as BusCommandRouterProcessor
participant Poller as BusCommandRouterProxyStreamPoller
participant Proxy as BusCommandRouterProxy
Client->>HttpAPI: POST /command-router/executions
HttpAPI->>Processor: dispatch_http_command(caller_proxy, http_requestor_id)
Processor->>Proxy: setup proxy nodes and run_command()
HttpAPI->>Poller: poll_stream(caller_proxy, command_type, stream_items_per_chunk)
loop until completion, abort, or error
Poller-->>HttpAPI: on_chunk(chunk)
end
Poller-->>HttpAPI: on_error / on_aborted / success
HttpAPI-->>Client: update execution state and streamed events
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
src/agents/command_router/http_api/CommandRouterHttpAPI.cc (1)
16-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit file-level
stdnamespace import.This
.ccuses unqualified standard-library types but only getsstdthrough included headers. Add it beside the domain imports. As per coding guidelines, "src/**/*.cc: Use file-levelusing namespace stdand domainusing namespacelines in C++ source files."Proposed fix
using namespace command_router; +using namespace std; using namespace commons; using namespace processor; using namespace agents;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc` around lines 16 - 19, The source file uses unqualified standard-library names but is missing the required file-level std import. Add using namespace std alongside the existing domain imports in CommandRouterHttpAPI.cc so the file follows the C++ source-file namespace convention used here; keep the change local to this translation unit and place it near the other using namespace lines.Source: Coding guidelines
src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc (2)
127-138: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReserve chunk capacity before streaming pushes.
append_answer_chunk()repeatedlypush_backs intochunk_dataup toitems_per_chunk; reserve that capacity once to avoid reallocations while polling large query/evolution result streams. As per path instructions, "MEMORY & PERFORMANCE (high priority): Flag unnecessary copies of large objects ... missing reserve() before repeated push_back/emplace_back."Proposed fix
- vector<string> chunk_data; + vector<string> chunk_data; + chunk_data.reserve(items_per_chunk);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc` around lines 127 - 138, The polling loop in BusCommandRouterProxyStreamPoller::poll_stream repeatedly fills chunk_data through append_answer_chunk(), but it never reserves space before the repeated push_back operations. Add a one-time reserve on chunk_data using items_per_chunk before the while loop so the streaming path avoids repeated reallocations and copying during large result streams.Source: Path instructions
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit file-level
stdnamespace import.This
.ccuses unqualifiedfunction,vector,shared_ptr, andstring; right now it relies on the header’s side effect. Addusing namespace std;beside the domain namespace imports. As per coding guidelines, "src/**/*.cc: Use file-levelusing namespace stdand domainusing namespacelines in C++ source files."Proposed fix
using namespace command_router; +using namespace std; using namespace commons; using namespace agents;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc` around lines 6 - 8, The source file-level namespace imports are incomplete, so the unqualified standard library symbols used in BusCommandRouterProxyStreamPoller.cc depend on a header side effect. Add the explicit file-level std namespace import alongside the existing command_router, commons, and agents using directives, keeping the source file aligned with the C++ source-file namespace import guideline.Source: Coding guidelines
src/agents/command_router/http_api/CommandExecution.cc (1)
7-18: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPre-size the event buffer in the constructor.
eventsgrows up to a known cap, so this hot path pays avoidable reallocations and string moves while executions are active. Reservingmax_eventshere keeps the buffer growth predictable.Suggested change
CommandExecution::CommandExecution(const string& execution_id, const string& command_type, const string& command_text, size_t max_events) : execution_id(execution_id), command_type(command_type), command_text(command_text), max_events(max_events) { if (this->max_events == 0) { RAISE_ERROR("max_events must be greater than 0"); } + this->events.reserve(this->max_events); }As per path instructions, "Watch for ... missing reserve() before repeated push_back/emplace_back."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandExecution.cc` around lines 7 - 18, The CommandExecution constructor currently initializes the known-bounded event buffer without reserving capacity, causing avoidable reallocations during repeated event appends. Update CommandExecution::CommandExecution to reserve max_events on the events container after validating max_events, so the buffer capacity matches the expected upper bound and hot-path push_back/emplace_back calls stay predictable.Source: Path instructions
src/tests/cpp/command_router_http_api_test.cc (1)
63-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace fixed startup sleeps with readiness polling.
The
500ms/300msdelays make the suite timing-dependent on CI load. If the bus or listener comes up slower, the later HTTP assertions fail as setup flakes rather than real regressions. Poll/ping(or a processor-ready condition) until success instead of sleeping a fixed interval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/command_router_http_api_test.cc` around lines 63 - 73, The test setup in CommandRouterHttpAPI initialization is relying on fixed Utils::sleep delays, which makes startup timing flaky; replace the 500ms/300ms waits with readiness polling. Use the existing CommandRouterHttpAPI, DedicatedThread, and ServiceBus/BusCommandRouterProcessor setup to repeatedly check a processor-ready signal or the /ping endpoint until it succeeds before continuing with assertions, rather than assuming the bus and API are ready after a fixed interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agents/command_router/BusCommandRouterProcessor.cc`:
- Around line 64-85: The caller proxy is marked issued and started before the
processor proxy setup can still fail, which leaves it unusable on retry. In
BusCommandRouterProcessor::dispatch, delay committing caller_proxy state or add
rollback in the failure path around PortPool::get_port() and setup_proxy_node()
for the processor_proxy. If the second allocation or proxy setup throws, reset
caller_proxy->issued and release any reserved proxy state so retries can proceed
cleanly.
In `@src/agents/command_router/http_api/CommandExecution.cc`:
- Around line 58-64: In CommandExecution::publish_chunk, received_count is being
incremented before publish_event succeeds, which can leave progress out of sync
if the event store is full. Move the received_count update to after the
publish_event call in CommandExecution::publish_chunk so the counter only
advances when the chunk event is actually recorded.
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 374-375: The raw HTTP command text is being rewritten in
CommandRouterHttpAPI::dispatch by replacing every percent sign in router_arg,
which can corrupt valid payloads before they reach the processor. Remove the
unconditional Utils::replace_all transformation and pass exec->command_text
through unchanged; only apply any percent-to-dollar translation in a clearly
documented compatibility path if one exists.
- Around line 487-489: `try_admit_execution()` is rejecting requests immediately
in the saturation path, which bypasses the queue and makes
`max_queued_executions` unused. Update the admission logic in
`CommandRouterHttpAPI::try_admit_execution` so that when `running_executions` is
at `settings.max_concurrent_executions`, requests can still be admitted into the
queue until the queued limit is reached, and only return
`AdmitResult::ConcurrentLimit` once both running and queued capacity are
exhausted.
In `@src/agents/command_router/http_api/CommandRouterHttpAPIConfig.cc`:
- Around line 34-44: CommandRouterHttpAPIConfig::load currently accepts
http_api.max_events_per_execution even when it is 0, which allows an invalid
HTTP execution setting to pass through. Add a fail-fast validation in the same
config parsing path, рядом with the existing stream_items_per_chunk check, so
settings.max_events_per_execution must be at least 1 before returning settings.
Use the existing CommandRouterHttpAPIConfig and
settings.max_events_per_execution symbols to locate the fix.
- Around line 17-24: The from_config parsing in CommandRouterHttpAPIConfig
should reject ports outside the valid TCP range, not just non-integer values.
After parsing tokens[1] with stoi, validate the resulting port in the same block
and raise an error if it is less than 1 or greater than 65535, so bad configs
fail fast before any bind attempt. Keep the fix localized around from_config and
the existing RAISE_ERROR handling for invalid config_path values.
In `@src/tests/cpp/BUILD`:
- Around line 1041-1049: The test target’s linkopts in src/tests/cpp/BUILD are
hard-coding a machine-specific library path and direct -l flags, which makes the
target depend on local installations. Update the affected BUILD target to use
Bazel-managed dependencies instead, such as existing cc_library targets or a
local cc_import/wrapper for the PostgreSQL, Redis, and MongoDB libs, and remove
the explicit /usr/local/lib and direct library flags from the target definition.
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 193-197: The test fixture in SetUpTestSuite is not exercising true
concurrency because server.start is launched with only one worker, so the
max_concurrent_executions cap cannot be validated reliably. Update the setup to
use at least two workers in server.start, or strengthen the test around the
command router HTTP API to explicitly assert a queued-to-running transition;
keep the fix centered on the SetUpTestSuite/server.start setup and the
concurrency assertions in this fixture.
- Around line 176-179: The 429 fixture in SetUpTestSuite only limits concurrent
executions, but it still leaves max_queued_executions at the default so the
second request can be queued instead of rejected. Update HttpAPISettings in the
test setup to explicitly disable queueing (or set max_queued_executions to zero)
alongside max_concurrent_executions in command_router_http_api_test.cc so
rejects_concurrency_limit exercises rejection correctly.
In `@src/tests/scripts/command_router_http_client.py`:
- Around line 93-123: The cancel flow in command_router_http_client.py is racy
because before_cancel waits for both a running status and a chunk event,
allowing fast executions to complete before the cancel request is sent. Update
the cancel test to trigger the POST
/command-router/executions/{execution_id}/cancel as soon as the websocket
reports running, using before_cancel and read_ws_events to gate only on running.
Keep the chunk assertion in a separate scenario or after cancel-independent
verification so the cancel test remains deterministic and still uses
until_aborted to confirm the aborted event.
---
Nitpick comments:
In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc`:
- Around line 127-138: The polling loop in
BusCommandRouterProxyStreamPoller::poll_stream repeatedly fills chunk_data
through append_answer_chunk(), but it never reserves space before the repeated
push_back operations. Add a one-time reserve on chunk_data using items_per_chunk
before the while loop so the streaming path avoids repeated reallocations and
copying during large result streams.
- Around line 6-8: The source file-level namespace imports are incomplete, so
the unqualified standard library symbols used in
BusCommandRouterProxyStreamPoller.cc depend on a header side effect. Add the
explicit file-level std namespace import alongside the existing command_router,
commons, and agents using directives, keeping the source file aligned with the
C++ source-file namespace import guideline.
In `@src/agents/command_router/http_api/CommandExecution.cc`:
- Around line 7-18: The CommandExecution constructor currently initializes the
known-bounded event buffer without reserving capacity, causing avoidable
reallocations during repeated event appends. Update
CommandExecution::CommandExecution to reserve max_events on the events container
after validating max_events, so the buffer capacity matches the expected upper
bound and hot-path push_back/emplace_back calls stay predictable.
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 16-19: The source file uses unqualified standard-library names but
is missing the required file-level std import. Add using namespace std alongside
the existing domain imports in CommandRouterHttpAPI.cc so the file follows the
C++ source-file namespace convention used here; keep the change local to this
translation unit and place it near the other using namespace lines.
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 63-73: The test setup in CommandRouterHttpAPI initialization is
relying on fixed Utils::sleep delays, which makes startup timing flaky; replace
the 500ms/300ms waits with readiness polling. Use the existing
CommandRouterHttpAPI, DedicatedThread, and ServiceBus/BusCommandRouterProcessor
setup to repeatedly check a processor-ready signal or the /ping endpoint until
it succeeds before continuing with assertions, rather than assuming the bus and
API are ready after a fixed interval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e00e1a3d-9e46-482e-8243-fb77f22a6379
📒 Files selected for processing (24)
config/das.jsonsrc/agents/command_router/BUILDsrc/agents/command_router/BusCommandRouterProcessor.ccsrc/agents/command_router/BusCommandRouterProcessor.hsrc/agents/command_router/CommandRouterHttpAPI.ccsrc/agents/command_router/CommandRouterHttpAPI.hsrc/agents/command_router/http_api/BUILDsrc/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.ccsrc/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.hsrc/agents/command_router/http_api/CommandExecution.ccsrc/agents/command_router/http_api/CommandExecution.hsrc/agents/command_router/http_api/CommandRouterHttpAPI.ccsrc/agents/command_router/http_api/CommandRouterHttpAPI.hsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.ccsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.hsrc/agents/command_router/http_api/CommandRouterHttpAPISingleton.ccsrc/agents/command_router/http_api/CommandRouterHttpAPISingleton.hsrc/main/BUILDsrc/main/bus_node.ccsrc/service_bus/BusCommandProxy.hsrc/tests/cpp/BUILDsrc/tests/cpp/bus_command_router_test.ccsrc/tests/cpp/command_router_http_api_test.ccsrc/tests/scripts/command_router_http_client.py
💤 Files with no reviewable changes (4)
- src/tests/cpp/bus_command_router_test.cc
- src/agents/command_router/CommandRouterHttpAPI.cc
- src/agents/command_router/CommandRouterHttpAPI.h
- src/agents/command_router/BUILD
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🧹 Nitpick comments (5)
src/agents/command_router/http_api/CommandRouterHttpAPI.cc (1)
16-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit file-level
stdnamespace import.This
.ccuses unqualified standard-library types but only getsstdthrough included headers. Add it beside the domain imports. As per coding guidelines, "src/**/*.cc: Use file-levelusing namespace stdand domainusing namespacelines in C++ source files."Proposed fix
using namespace command_router; +using namespace std; using namespace commons; using namespace processor; using namespace agents;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc` around lines 16 - 19, The source file uses unqualified standard-library names but is missing the required file-level std import. Add using namespace std alongside the existing domain imports in CommandRouterHttpAPI.cc so the file follows the C++ source-file namespace convention used here; keep the change local to this translation unit and place it near the other using namespace lines.Source: Coding guidelines
src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc (2)
127-138: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReserve chunk capacity before streaming pushes.
append_answer_chunk()repeatedlypush_backs intochunk_dataup toitems_per_chunk; reserve that capacity once to avoid reallocations while polling large query/evolution result streams. As per path instructions, "MEMORY & PERFORMANCE (high priority): Flag unnecessary copies of large objects ... missing reserve() before repeated push_back/emplace_back."Proposed fix
- vector<string> chunk_data; + vector<string> chunk_data; + chunk_data.reserve(items_per_chunk);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc` around lines 127 - 138, The polling loop in BusCommandRouterProxyStreamPoller::poll_stream repeatedly fills chunk_data through append_answer_chunk(), but it never reserves space before the repeated push_back operations. Add a one-time reserve on chunk_data using items_per_chunk before the while loop so the streaming path avoids repeated reallocations and copying during large result streams.Source: Path instructions
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit file-level
stdnamespace import.This
.ccuses unqualifiedfunction,vector,shared_ptr, andstring; right now it relies on the header’s side effect. Addusing namespace std;beside the domain namespace imports. As per coding guidelines, "src/**/*.cc: Use file-levelusing namespace stdand domainusing namespacelines in C++ source files."Proposed fix
using namespace command_router; +using namespace std; using namespace commons; using namespace agents;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc` around lines 6 - 8, The source file-level namespace imports are incomplete, so the unqualified standard library symbols used in BusCommandRouterProxyStreamPoller.cc depend on a header side effect. Add the explicit file-level std namespace import alongside the existing command_router, commons, and agents using directives, keeping the source file aligned with the C++ source-file namespace import guideline.Source: Coding guidelines
src/agents/command_router/http_api/CommandExecution.cc (1)
7-18: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPre-size the event buffer in the constructor.
eventsgrows up to a known cap, so this hot path pays avoidable reallocations and string moves while executions are active. Reservingmax_eventshere keeps the buffer growth predictable.Suggested change
CommandExecution::CommandExecution(const string& execution_id, const string& command_type, const string& command_text, size_t max_events) : execution_id(execution_id), command_type(command_type), command_text(command_text), max_events(max_events) { if (this->max_events == 0) { RAISE_ERROR("max_events must be greater than 0"); } + this->events.reserve(this->max_events); }As per path instructions, "Watch for ... missing reserve() before repeated push_back/emplace_back."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandExecution.cc` around lines 7 - 18, The CommandExecution constructor currently initializes the known-bounded event buffer without reserving capacity, causing avoidable reallocations during repeated event appends. Update CommandExecution::CommandExecution to reserve max_events on the events container after validating max_events, so the buffer capacity matches the expected upper bound and hot-path push_back/emplace_back calls stay predictable.Source: Path instructions
src/tests/cpp/command_router_http_api_test.cc (1)
63-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace fixed startup sleeps with readiness polling.
The
500ms/300msdelays make the suite timing-dependent on CI load. If the bus or listener comes up slower, the later HTTP assertions fail as setup flakes rather than real regressions. Poll/ping(or a processor-ready condition) until success instead of sleeping a fixed interval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/command_router_http_api_test.cc` around lines 63 - 73, The test setup in CommandRouterHttpAPI initialization is relying on fixed Utils::sleep delays, which makes startup timing flaky; replace the 500ms/300ms waits with readiness polling. Use the existing CommandRouterHttpAPI, DedicatedThread, and ServiceBus/BusCommandRouterProcessor setup to repeatedly check a processor-ready signal or the /ping endpoint until it succeeds before continuing with assertions, rather than assuming the bus and API are ready after a fixed interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agents/command_router/BusCommandRouterProcessor.cc`:
- Around line 64-85: The caller proxy is marked issued and started before the
processor proxy setup can still fail, which leaves it unusable on retry. In
BusCommandRouterProcessor::dispatch, delay committing caller_proxy state or add
rollback in the failure path around PortPool::get_port() and setup_proxy_node()
for the processor_proxy. If the second allocation or proxy setup throws, reset
caller_proxy->issued and release any reserved proxy state so retries can proceed
cleanly.
In `@src/agents/command_router/http_api/CommandExecution.cc`:
- Around line 58-64: In CommandExecution::publish_chunk, received_count is being
incremented before publish_event succeeds, which can leave progress out of sync
if the event store is full. Move the received_count update to after the
publish_event call in CommandExecution::publish_chunk so the counter only
advances when the chunk event is actually recorded.
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 374-375: The raw HTTP command text is being rewritten in
CommandRouterHttpAPI::dispatch by replacing every percent sign in router_arg,
which can corrupt valid payloads before they reach the processor. Remove the
unconditional Utils::replace_all transformation and pass exec->command_text
through unchanged; only apply any percent-to-dollar translation in a clearly
documented compatibility path if one exists.
- Around line 487-489: `try_admit_execution()` is rejecting requests immediately
in the saturation path, which bypasses the queue and makes
`max_queued_executions` unused. Update the admission logic in
`CommandRouterHttpAPI::try_admit_execution` so that when `running_executions` is
at `settings.max_concurrent_executions`, requests can still be admitted into the
queue until the queued limit is reached, and only return
`AdmitResult::ConcurrentLimit` once both running and queued capacity are
exhausted.
In `@src/agents/command_router/http_api/CommandRouterHttpAPIConfig.cc`:
- Around line 34-44: CommandRouterHttpAPIConfig::load currently accepts
http_api.max_events_per_execution even when it is 0, which allows an invalid
HTTP execution setting to pass through. Add a fail-fast validation in the same
config parsing path, рядом with the existing stream_items_per_chunk check, so
settings.max_events_per_execution must be at least 1 before returning settings.
Use the existing CommandRouterHttpAPIConfig and
settings.max_events_per_execution symbols to locate the fix.
- Around line 17-24: The from_config parsing in CommandRouterHttpAPIConfig
should reject ports outside the valid TCP range, not just non-integer values.
After parsing tokens[1] with stoi, validate the resulting port in the same block
and raise an error if it is less than 1 or greater than 65535, so bad configs
fail fast before any bind attempt. Keep the fix localized around from_config and
the existing RAISE_ERROR handling for invalid config_path values.
In `@src/tests/cpp/BUILD`:
- Around line 1041-1049: The test target’s linkopts in src/tests/cpp/BUILD are
hard-coding a machine-specific library path and direct -l flags, which makes the
target depend on local installations. Update the affected BUILD target to use
Bazel-managed dependencies instead, such as existing cc_library targets or a
local cc_import/wrapper for the PostgreSQL, Redis, and MongoDB libs, and remove
the explicit /usr/local/lib and direct library flags from the target definition.
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 193-197: The test fixture in SetUpTestSuite is not exercising true
concurrency because server.start is launched with only one worker, so the
max_concurrent_executions cap cannot be validated reliably. Update the setup to
use at least two workers in server.start, or strengthen the test around the
command router HTTP API to explicitly assert a queued-to-running transition;
keep the fix centered on the SetUpTestSuite/server.start setup and the
concurrency assertions in this fixture.
- Around line 176-179: The 429 fixture in SetUpTestSuite only limits concurrent
executions, but it still leaves max_queued_executions at the default so the
second request can be queued instead of rejected. Update HttpAPISettings in the
test setup to explicitly disable queueing (or set max_queued_executions to zero)
alongside max_concurrent_executions in command_router_http_api_test.cc so
rejects_concurrency_limit exercises rejection correctly.
In `@src/tests/scripts/command_router_http_client.py`:
- Around line 93-123: The cancel flow in command_router_http_client.py is racy
because before_cancel waits for both a running status and a chunk event,
allowing fast executions to complete before the cancel request is sent. Update
the cancel test to trigger the POST
/command-router/executions/{execution_id}/cancel as soon as the websocket
reports running, using before_cancel and read_ws_events to gate only on running.
Keep the chunk assertion in a separate scenario or after cancel-independent
verification so the cancel test remains deterministic and still uses
until_aborted to confirm the aborted event.
---
Nitpick comments:
In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc`:
- Around line 127-138: The polling loop in
BusCommandRouterProxyStreamPoller::poll_stream repeatedly fills chunk_data
through append_answer_chunk(), but it never reserves space before the repeated
push_back operations. Add a one-time reserve on chunk_data using items_per_chunk
before the while loop so the streaming path avoids repeated reallocations and
copying during large result streams.
- Around line 6-8: The source file-level namespace imports are incomplete, so
the unqualified standard library symbols used in
BusCommandRouterProxyStreamPoller.cc depend on a header side effect. Add the
explicit file-level std namespace import alongside the existing command_router,
commons, and agents using directives, keeping the source file aligned with the
C++ source-file namespace import guideline.
In `@src/agents/command_router/http_api/CommandExecution.cc`:
- Around line 7-18: The CommandExecution constructor currently initializes the
known-bounded event buffer without reserving capacity, causing avoidable
reallocations during repeated event appends. Update
CommandExecution::CommandExecution to reserve max_events on the events container
after validating max_events, so the buffer capacity matches the expected upper
bound and hot-path push_back/emplace_back calls stay predictable.
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 16-19: The source file uses unqualified standard-library names but
is missing the required file-level std import. Add using namespace std alongside
the existing domain imports in CommandRouterHttpAPI.cc so the file follows the
C++ source-file namespace convention used here; keep the change local to this
translation unit and place it near the other using namespace lines.
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 63-73: The test setup in CommandRouterHttpAPI initialization is
relying on fixed Utils::sleep delays, which makes startup timing flaky; replace
the 500ms/300ms waits with readiness polling. Use the existing
CommandRouterHttpAPI, DedicatedThread, and ServiceBus/BusCommandRouterProcessor
setup to repeatedly check a processor-ready signal or the /ping endpoint until
it succeeds before continuing with assertions, rather than assuming the bus and
API are ready after a fixed interval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e00e1a3d-9e46-482e-8243-fb77f22a6379
📒 Files selected for processing (24)
config/das.jsonsrc/agents/command_router/BUILDsrc/agents/command_router/BusCommandRouterProcessor.ccsrc/agents/command_router/BusCommandRouterProcessor.hsrc/agents/command_router/CommandRouterHttpAPI.ccsrc/agents/command_router/CommandRouterHttpAPI.hsrc/agents/command_router/http_api/BUILDsrc/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.ccsrc/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.hsrc/agents/command_router/http_api/CommandExecution.ccsrc/agents/command_router/http_api/CommandExecution.hsrc/agents/command_router/http_api/CommandRouterHttpAPI.ccsrc/agents/command_router/http_api/CommandRouterHttpAPI.hsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.ccsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.hsrc/agents/command_router/http_api/CommandRouterHttpAPISingleton.ccsrc/agents/command_router/http_api/CommandRouterHttpAPISingleton.hsrc/main/BUILDsrc/main/bus_node.ccsrc/service_bus/BusCommandProxy.hsrc/tests/cpp/BUILDsrc/tests/cpp/bus_command_router_test.ccsrc/tests/cpp/command_router_http_api_test.ccsrc/tests/scripts/command_router_http_client.py
💤 Files with no reviewable changes (4)
- src/tests/cpp/bus_command_router_test.cc
- src/agents/command_router/CommandRouterHttpAPI.cc
- src/agents/command_router/CommandRouterHttpAPI.h
- src/agents/command_router/BUILD
🛑 Comments failed to post (10)
src/agents/command_router/BusCommandRouterProcessor.cc (1)
64-85: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Rollback
caller_proxyif processor setup fails.Line 64 marks the caller proxy as issued and Lines 67-71 start its proxy node before Lines 77-85 can still fail. If the second port allocation or
setup_proxy_node()throws, that same proxy is now permanently unusable because Line 55 will reject any retry, and any retained caller proxy also keeps its reserved port until destruction. Please either complete all fallible allocation first or add rollback for the caller-side proxy state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/BusCommandRouterProcessor.cc` around lines 64 - 85, The caller proxy is marked issued and started before the processor proxy setup can still fail, which leaves it unusable on retry. In BusCommandRouterProcessor::dispatch, delay committing caller_proxy state or add rollback in the failure path around PortPool::get_port() and setup_proxy_node() for the processor_proxy. If the second allocation or proxy setup throws, reset caller_proxy->issued and release any reserved proxy state so retries can proceed cleanly.src/agents/command_router/http_api/CommandExecution.cc (1)
58-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update
received_countonly after the chunk event is stored.If
publish_event()throws because the buffer is full,received_counthas already been incremented, so the execution reports progress for a chunk that was never recorded or streamed. Move the counter assignment after the successful publish.Suggested change
void CommandExecution::publish_chunk(int seq, const vector<string>& data) { - this->received_count += static_cast<int>(data.size()); - this->publish_event({{"execution_id", this->execution_id}, - {"type", "chunk"}, - {"seq", seq}, - {"data", data}, - {"received_count", this->received_count}}); + int next_received_count = this->received_count + static_cast<int>(data.size()); + this->publish_event({{"execution_id", this->execution_id}, + {"type", "chunk"}, + {"seq", seq}, + {"data", data}, + {"received_count", next_received_count}}); + this->received_count = next_received_count; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.void CommandExecution::publish_chunk(int seq, const vector<string>& data) { int next_received_count = this->received_count + static_cast<int>(data.size()); this->publish_event({{"execution_id", this->execution_id}, {"type", "chunk"}, {"seq", seq}, {"data", data}, {"received_count", next_received_count}}); this->received_count = next_received_count;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandExecution.cc` around lines 58 - 64, In CommandExecution::publish_chunk, received_count is being incremented before publish_event succeeds, which can leave progress out of sync if the event store is full. Move the received_count update to after the publish_event call in CommandExecution::publish_chunk so the counter only advances when the chunk event is actually recorded.src/agents/command_router/http_api/CommandRouterHttpAPI.cc (2)
374-375: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not rewrite raw HTTP command text.
HTTP JSON can carry
$directly, so replacing every%with$corrupts valid commands containing literal percent signs or percent-like tokens before dispatching to the processor. Keep the payload unchanged unless a documented compatibility mode explicitly requests this translation.Proposed fix
- string router_arg = exec->command_text; - Utils::replace_all(router_arg, "%", "$"); + string router_arg = exec->command_text;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.string router_arg = exec->command_text;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc` around lines 374 - 375, The raw HTTP command text is being rewritten in CommandRouterHttpAPI::dispatch by replacing every percent sign in router_arg, which can corrupt valid payloads before they reach the processor. Remove the unconditional Utils::replace_all transformation and pass exec->command_text through unchanged; only apply any percent-to-dollar translation in a clearly documented compatibility path if one exists.Source: Path instructions
487-489: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow admitted requests to queue when concurrency is saturated.
try_admit_execution()returnsConcurrentLimitas soon asrunning_executionsreaches the limit, somax_queued_executionsis bypassed during steady-state saturation. That makes the new queue setting ineffective for the main overload case.Proposed fix
- if (this->running_executions >= this->settings.max_concurrent_executions) { - return AdmitResult::ConcurrentLimit; - } if (this->settings.max_queued_executions > 0 && this->pending_executions >= this->settings.max_queued_executions) { return AdmitResult::QueueFull;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (this->settings.max_queued_executions > 0 && this->pending_executions >= this->settings.max_queued_executions) { return AdmitResult::QueueFull; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc` around lines 487 - 489, `try_admit_execution()` is rejecting requests immediately in the saturation path, which bypasses the queue and makes `max_queued_executions` unused. Update the admission logic in `CommandRouterHttpAPI::try_admit_execution` so that when `running_executions` is at `settings.max_concurrent_executions`, requests can still be admitted into the queue until the queued limit is reached, and only return `AdmitResult::ConcurrentLimit` once both running and queued capacity are exhausted.Source: Path instructions
src/agents/command_router/http_api/CommandRouterHttpAPIConfig.cc (2)
17-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject out-of-range ports during config parsing.
stoi()accepts values like-1and70000, so invalid endpoints currently survivefrom_config()and only fail later when the server tries to bind. Validating1..65535here will keep bad configs fail-fast.Suggested change
int port; try { port = stoi(tokens[1]); } catch (const exception&) { RAISE_ERROR("Invalid " + config_path + " configuration: port must be an integer"); } + if (port < 1 || port > 65535) { + RAISE_ERROR("Invalid " + config_path + " configuration: port must be between 1 and 65535"); + } return {tokens[0], port}; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.int port; try { port = stoi(tokens[1]); } catch (const exception&) { RAISE_ERROR("Invalid " + config_path + " configuration: port must be an integer"); } if (port < 1 || port > 65535) { RAISE_ERROR("Invalid " + config_path + " configuration: port must be between 1 and 65535"); } return {tokens[0], port};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPIConfig.cc` around lines 17 - 24, The from_config parsing in CommandRouterHttpAPIConfig should reject ports outside the valid TCP range, not just non-integer values. After parsing tokens[1] with stoi, validate the resulting port in the same block and raise an error if it is less than 1 or greater than 65535, so bad configs fail fast before any bind attempt. Keep the fix localized around from_config and the existing RAISE_ERROR handling for invalid config_path values.
34-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject
max_events_per_execution == 0at config load.
CommandExecutionalready treats zero as invalid, so this parser currently accepts a bad config that only blows up when the first execution is created. Please fail fast here, alongside the existingstream_items_per_chunkcheck.Suggested change
settings.max_events_per_execution = command_router_config.at_path("http_api.max_events_per_execution") .get_or<size_t>(settings.max_events_per_execution); + if (settings.max_events_per_execution == 0) { + RAISE_ERROR("command_router.http_api.max_events_per_execution must be at least 1"); + } settings.execution_retention_ms = command_router_config.at_path("http_api.execution_retention_ms") .get_or<long long>(settings.execution_retention_ms); settings.stream_items_per_chunk = command_router_config.at_path("http_api.stream_items_per_chunk") .get_or<size_t>(settings.stream_items_per_chunk);As per path instructions,
config/**changes should keep HTTP execution settings "present and valid."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.settings.max_events_per_execution = command_router_config.at_path("http_api.max_events_per_execution") .get_or<size_t>(settings.max_events_per_execution); if (settings.max_events_per_execution == 0) { RAISE_ERROR("command_router.http_api.max_events_per_execution must be at least 1"); } settings.execution_retention_ms = command_router_config.at_path("http_api.execution_retention_ms") .get_or<long long>(settings.execution_retention_ms); settings.stream_items_per_chunk = command_router_config.at_path("http_api.stream_items_per_chunk") .get_or<size_t>(settings.stream_items_per_chunk); if (settings.stream_items_per_chunk == 0) { RAISE_ERROR("command_router.http_api.stream_items_per_chunk must be at least 1"); } return settings;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPIConfig.cc` around lines 34 - 44, CommandRouterHttpAPIConfig::load currently accepts http_api.max_events_per_execution even when it is 0, which allows an invalid HTTP execution setting to pass through. Add a fail-fast validation in the same config parsing path, рядом with the existing stream_items_per_chunk check, so settings.max_events_per_execution must be at least 1 before returning settings. Use the existing CommandRouterHttpAPIConfig and settings.max_events_per_execution symbols to locate the fix.Source: Path instructions
src/tests/cpp/BUILD (1)
1041-1049: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail fd -HI 'BUILD|BUILD.bazel' . | xargs rg -n -C2 \ 'command_router_http_api_test|/usr/local/lib|-lpqxx|-lpq|-lhiredis_cluster|-lhiredis|-lmongocxx|-lbsoncxx'Repository: singnet/das
Length of output: 13267
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C2 --hidden --glob 'BUILD*' --glob '*.bzl' \ 'cc_import|new_local_repository|local_repository|http_archive|pqxx|hiredis_cluster|mongocxx|bsoncxx|hiredis' \ src .bazelrc WORKSPACE MODULE.bazelRepository: singnet/das
Length of output: 11263
Avoid hard-coding
/usr/local/libin this test target. The direct-L/-lflags make it depend on machine-local libraries, so CI and remote execution can fail if those packages aren’t installed in the same layout. Replace them with Bazel-managed deps or a localcc_import/wrapper instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/BUILD` around lines 1041 - 1049, The test target’s linkopts in src/tests/cpp/BUILD are hard-coding a machine-specific library path and direct -l flags, which makes the target depend on local installations. Update the affected BUILD target to use Bazel-managed dependencies instead, such as existing cc_library targets or a local cc_import/wrapper for the PostgreSQL, Redis, and MongoDB libs, and remove the explicit /usr/local/lib and direct library flags from the target definition.src/tests/cpp/command_router_http_api_test.cc (2)
176-179: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disable queueing in the 429 fixture.
max_queued_executionsstill uses its default (500), so this setup should admit the second execution aspendinginstead of rejecting it. The currentrejects_concurrency_limittest only passes if queue admission is broken.Suggested fix
static void SetUpTestSuite() { HttpAPISettings settings; settings.max_concurrent_executions = 1; + settings.max_queued_executions = 0; server.start(TEST_PORT_LIMITS, settings); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.static void SetUpTestSuite() { HttpAPISettings settings; settings.max_concurrent_executions = 1; settings.max_queued_executions = 0; server.start(TEST_PORT_LIMITS, settings);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/command_router_http_api_test.cc` around lines 176 - 179, The 429 fixture in SetUpTestSuite only limits concurrent executions, but it still leaves max_queued_executions at the default so the second request can be queued instead of rejected. Update HttpAPISettings in the test setup to explicitly disable queueing (or set max_queued_executions to zero) alongside max_concurrent_executions in command_router_http_api_test.cc so rejects_concurrency_limit exercises rejection correctly.
193-197: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This fixture can't validate a 2-running cap with one worker.
server.start(19007, settings, 1)serializes execution in the thread pool, somax_observed_running <= 2will pass even if admission logic never allows two concurrent runs. Use at least two workers, or assert a queued→running transition explicitly.Suggested fix
- server.start(19007, settings, 1); + server.start(19007, settings, 2);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.static void SetUpTestSuite() { HttpAPISettings settings; settings.max_concurrent_executions = 2; settings.max_queued_executions = 10; server.start(19007, settings, 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/command_router_http_api_test.cc` around lines 193 - 197, The test fixture in SetUpTestSuite is not exercising true concurrency because server.start is launched with only one worker, so the max_concurrent_executions cap cannot be validated reliably. Update the setup to use at least two workers in server.start, or strengthen the test around the command router HTTP API to explicitly assert a queued-to-running transition; keep the fix centered on the SetUpTestSuite/server.start setup and the concurrency assertions in this fixture.src/tests/scripts/command_router_http_client.py (1)
93-123: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The cancel scenario is racy after waiting for a chunk.
before_cancel()waits for both"running"and a"chunk"event. For fast queries, the execution can reach"completed"before the cancel POST, so this script intermittently fails on the200expectation or never sees"aborted". Cancel as soon as the execution reaches"running", and keep chunk verification in a separate non-cancel scenario if you still want both assertions.🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 105-108: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{base_url}/command-router/executions/{execution_id}/cancel",
timeout=10,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).(ssrf-requests)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/scripts/command_router_http_client.py` around lines 93 - 123, The cancel flow in command_router_http_client.py is racy because before_cancel waits for both a running status and a chunk event, allowing fast executions to complete before the cancel request is sent. Update the cancel test to trigger the POST /command-router/executions/{execution_id}/cancel as soon as the websocket reports running, using before_cancel and read_ws_events to gate only on running. Keep the chunk assertion in a separate scenario or after cancel-independent verification so the cancel test remains deterministic and still uses until_aborted to confirm the aborted event.
… admission/cancel flow, require router processor at construction, and tighten stream poller error handling.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/agents/command_router/http_api/CommandRouterHttpAPISingleton.h (1)
25-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment doesn't mention the new
processorrequirement.The Doxygen note above
create_and_startstill only describes the config-driven build/start flow; worth a quick mention that a non-nullBusCommandRouterProcessoris now required (it currentlyRAISE_ERRORs otherwise).📝 Suggested doc tweak
- /** `@brief` Build and start HTTP API from command_router.http_api config. Sets HTTP_API and - * INITIALIZED. */ + /** `@brief` Build and start HTTP API from command_router.http_api config and the given + * BusCommandProcessor (must be castable to BusCommandRouterProcessor). Sets HTTP_API and + * INITIALIZED. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPISingleton.h` around lines 25 - 28, The Doxygen comment for create_and_start in CommandRouterHttpAPISingleton should be updated to mention the new processor requirement. Clarify that a non-null BusCommandRouterProcessor/BusCommandProcessor must be provided when building and starting the HTTP API, and note that the method raises an error if it is missing so the API contract matches the implementation.src/agents/command_router/BusCommandRouterProcessor.cc (1)
50-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative-path tests for
dispatch_http_command.Only the happy-path (
dispatch_http_command_get_returns_params) is covered. This method has several new fatal-error branches (null caller proxy, re-dispatch of an already-issued proxy, emptyhttp_requestor_id, port-pool exhaustion) that aren't exercised.As per path instructions, "TEST COVERAGE: Behavior changes here should have matching *_test.cc updates; suggest concrete test cases (edge cases, error paths, concurrency) not trivial assertions."
Want me to draft the additional
*_test.cccases for these error branches?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/BusCommandRouterProcessor.cc` around lines 50 - 90, Add negative-path coverage for BusCommandRouterProcessor::dispatch_http_command in the matching *_test.cc file, since only the happy path is currently tested. Create concrete tests for each fatal branch: null caller_proxy, re-dispatch when caller_proxy->issued is already true, empty http_requestor_id, and PortPool::get_port returning 0 for both the caller and processor proxy setup. Keep the tests focused on the error paths and verify the method raises the expected failures before it reaches setup_proxy_node or run_command.Source: Path instructions
src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc (2)
20-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReserve
chunk_datacapacity to avoid reallocations.
chunk_datagrows via repeatedpush_backup toitems_per_chunkbefore eachemit_chunk/clear cycle, with noreserve(). Sinceitems_per_chunkis known up front, reserving avoids reallocations on the hot streaming path.⚡ Proposed fix
vector<string> chunk_data; + chunk_data.reserve(items_per_chunk); while (!finished_or_error()) {As per path instructions, "Watch for... missing reserve() before repeated push_back/emplace_back."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc` around lines 20 - 33, The append_answer_chunk hot path in BusCommandRouterProxyStreamPoller should preallocate chunk_data before the repeated push_back loop. Add a reserve based on items_per_chunk near the start of append_answer_chunk, before popping answers from router_proxy and appending via answer->to_string(...), so the vector avoids repeated reallocations across the emit_chunk cycle.Source: Path instructions
72-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared get/set polling pattern.
The
get(lines 72-95) andset(lines 97-120) branches duplicate the same busy-wait/error/empty-check structure, differing only in the field polled and the error strings. A small helper parameterized on the field accessor and command-name strings would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc` around lines 72 - 120, The get and set branches in BusCommandRouterProxyStreamPoller::poll duplicate the same wait/error/empty-check flow, so extract that shared logic into a small helper. Parameterize the helper with the polled response accessor (params_response vs set_param_ack) and the command name strings used in the error messages. Keep the existing behavior for handle_abort(), finished_or_error(), on_error, and on_chunk unchanged while simplifying both branches to call the helper.src/agents/command_router/http_api/CommandRouterHttpAPI.cc (1)
314-317: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUndocumented
%→$substitution in router argument.
Utils::replace_all(router_arg, "%", "$")silently rewrites every%incommand_textto$. This is likely an HTTP-transport encoding for MeTTa variables (which use a$prefix), but without a comment this is opaque, and it will also corrupt any command text that legitimately contains a literal%.Please confirm the client-side contract that encodes
$as%before sending over HTTP, and consider adding a short comment (or using a less ambiguous escape) documenting the encoding rule and its limitations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc` around lines 314 - 317, The router argument normalization in CommandRouterHttpAPI::command handling silently rewrites all percent signs in exec->command_text, so document the client-side encoding contract that maps $ to % over HTTP and clarify the limitation. Add a short comment near Utils::replace_all(router_arg, "%", "$") in the CommandRouterHttpAPI path, or replace it with a less ambiguous transport encoding, so future readers understand why BusCommandRouterProxy receives the transformed string and what literal % cases are unsupported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agents/command_router/BusCommandRouterProcessor.cc`:
- Around line 86-87: The in-process dispatch in BusCommandRouterProcessor
currently copies the large command payload from caller_proxy into
processor_proxy. Update the assignment path to transfer ownership with std::move
for caller_proxy->command and caller_proxy->args, and verify those fields are
not used again after dispatch (including through get_command()/get_args())
before changing the BusCommandRouterProcessor logic.
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 169-170: In CommandRouterHttpAPI::CommandRouterHttpAPI response
handling, status_for_response is declared twice in the same scope, which causes
a C++ redefinition compile error. Remove the duplicate declaration and keep only
one status_for_response assignment where exec->status_string() is used, making
sure the surrounding response-building logic still references that single
variable.
In `@src/agents/command_router/http_api/CommandRouterHttpAPIConfig.cc`:
- Around line 10-20: The HTTP API endpoint parsing in parse_host_port currently
only validates the host:port shape and allows non-numeric ports to slip through
until std::stoi in from_config throws uncaught exceptions. Update
parse_host_port (or the from_config path that consumes it) to explicitly
validate or catch std::invalid_argument/std::out_of_range and rethrow with
RAISE_ERROR using the existing command_router.<endpoint> context, and add tests
covering inputs like localhost:abc and localhost:8080abc.
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 71-74: Avoid reinitializing ServiceBus statics with a narrower
command set in the test fixture. Update the shared initialization path around
ServiceBus::initialize_statics and service_bus_statics_initialized so all tests
use one “initialize once” helper with the superset of commands (including
BUS_COMMAND_ROUTER and PATTERN_MATCHING_QUERY) and a consistent port range. Then
remove the narrower reset from the dispatch test and align the other affected
call sites so the suite is order-independent and consistent.
- Around line 99-106: The teardown order in stop() is wrong:
CommandRouterHttpApiTest clears api while DedicatedThread/api_thread may still
be using the raw ThreadMethod* passed from this->api.get(). Update stop() to
reset or stop the api_thread (and any thread pool backing it) before nulling
api, then continue clearing router_processor, router_bus, and query_bus after
the thread is fully torn down.
---
Nitpick comments:
In `@src/agents/command_router/BusCommandRouterProcessor.cc`:
- Around line 50-90: Add negative-path coverage for
BusCommandRouterProcessor::dispatch_http_command in the matching *_test.cc file,
since only the happy path is currently tested. Create concrete tests for each
fatal branch: null caller_proxy, re-dispatch when caller_proxy->issued is
already true, empty http_requestor_id, and PortPool::get_port returning 0 for
both the caller and processor proxy setup. Keep the tests focused on the error
paths and verify the method raises the expected failures before it reaches
setup_proxy_node or run_command.
In `@src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc`:
- Around line 20-33: The append_answer_chunk hot path in
BusCommandRouterProxyStreamPoller should preallocate chunk_data before the
repeated push_back loop. Add a reserve based on items_per_chunk near the start
of append_answer_chunk, before popping answers from router_proxy and appending
via answer->to_string(...), so the vector avoids repeated reallocations across
the emit_chunk cycle.
- Around line 72-120: The get and set branches in
BusCommandRouterProxyStreamPoller::poll duplicate the same
wait/error/empty-check flow, so extract that shared logic into a small helper.
Parameterize the helper with the polled response accessor (params_response vs
set_param_ack) and the command name strings used in the error messages. Keep the
existing behavior for handle_abort(), finished_or_error(), on_error, and
on_chunk unchanged while simplifying both branches to call the helper.
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 314-317: The router argument normalization in
CommandRouterHttpAPI::command handling silently rewrites all percent signs in
exec->command_text, so document the client-side encoding contract that maps $ to
% over HTTP and clarify the limitation. Add a short comment near
Utils::replace_all(router_arg, "%", "$") in the CommandRouterHttpAPI path, or
replace it with a less ambiguous transport encoding, so future readers
understand why BusCommandRouterProxy receives the transformed string and what
literal % cases are unsupported.
In `@src/agents/command_router/http_api/CommandRouterHttpAPISingleton.h`:
- Around line 25-28: The Doxygen comment for create_and_start in
CommandRouterHttpAPISingleton should be updated to mention the new processor
requirement. Clarify that a non-null
BusCommandRouterProcessor/BusCommandProcessor must be provided when building and
starting the HTTP API, and note that the method raises an error if it is missing
so the API contract matches the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2632cd2b-c9ac-454d-9e58-3776ef9012e1
📒 Files selected for processing (19)
config/das.jsonsrc/agents/command_router/BUILDsrc/agents/command_router/BusCommandRouterProcessor.ccsrc/agents/command_router/BusCommandRouterProcessor.hsrc/agents/command_router/http_api/BUILDsrc/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.ccsrc/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.hsrc/agents/command_router/http_api/CommandRouterHttpAPI.ccsrc/agents/command_router/http_api/CommandRouterHttpAPI.hsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.ccsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.hsrc/agents/command_router/http_api/CommandRouterHttpAPISingleton.ccsrc/agents/command_router/http_api/CommandRouterHttpAPISingleton.hsrc/main/BUILDsrc/main/bus_node.ccsrc/service_bus/BusCommandProxy.hsrc/tests/cpp/BUILDsrc/tests/cpp/bus_command_router_test.ccsrc/tests/cpp/command_router_http_api_test.cc
💤 Files with no reviewable changes (2)
- src/tests/cpp/bus_command_router_test.cc
- src/agents/command_router/BUILD
Reject non-numeric ports and zero stream_items_per_chunk during config load, and repair the HTTP API test fixture with coverage for invalid endpoint values.
3 participants
This PR routes Command Router HTTP executions through BusCommandRouterProcessor in-process.
dispatch_http_command()to set up caller/processor proxy peers locally and run commands through the existing router processorBusCommandRouterProxyStreamPollerto stream chunked results for get, set, query, and evolution