[#1196] Stream query answers as JSON over the Command Router HTTP API#1197
[#1196] Stream query answers as JSON over the Command Router HTTP API#1197marcocapozzoli wants to merge 7 commits into
Conversation
|
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:
WalkthroughThe command-router streaming contract now uses JSON arrays instead of ChangesJSON command-router streaming
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant BusCommandRouterProxyStreamPoller
participant QueryAnswer
participant CommandRouterHttpAPI
participant CommandExecution
BusCommandRouterProxyStreamPoller->>QueryAnswer: to_json(metta_flag)
QueryAnswer-->>BusCommandRouterProxyStreamPoller: structured JSON answer
BusCommandRouterProxyStreamPoller->>CommandRouterHttpAPI: JSON array chunk
CommandRouterHttpAPI->>CommandExecution: publish_chunk(seq, data)
CommandExecution-->>CommandRouterHttpAPI: JSON event published
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.
🧹 Nitpick comments (1)
src/tests/cpp/command_router_http_api_test.cc (1)
385-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover populated MeTTa mappings and scalar fields.
This only verifies raw-value fallback. Add a JSON serialization or stream test with
metta_expressionpopulated, asserting the"metta"key, mapped assignment,importance, andstrength.🤖 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 385 - 397, Extend the existing poll_stream test around BusCommandRouterProxyStreamPoller::poll_stream with a populated metta_expression case, using JSON serialization or streamed output. Assert the resulting JSON includes the "metta" key, the mapped assignment, and the expected scalar importance and strength values, while retaining the current raw-value fallback assertions.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 385-397: Extend the existing poll_stream test around
BusCommandRouterProxyStreamPoller::poll_stream with a populated metta_expression
case, using JSON serialization or streamed output. Assert the resulting JSON
includes the "metta" key, the mapped assignment, and the expected scalar
importance and strength values, while retaining the current raw-value fallback
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 457f3207-23d5-4116-9c49-790b2344988d
📒 Files selected for processing (9)
src/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/query_engine/QueryAnswer.ccsrc/agents/query_engine/QueryAnswer.hsrc/tests/cpp/command_router_http_api_test.cc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tests/cpp/query_answer_test.cc (1)
507-538: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining
from_jsonvalidation branches.This test exercises several
RAISE_ERRORpaths but misses two introduced by this PR: thehandles/assignmentsize-limit checks (QueryAnswer.ccLines 220-223, 251-254), and ahandles[i]element that is not itself an array (e.g.{"handles": [1]}), which is distinct from the already-tested "handle element has wrong type" case.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."
json handles_not_array = {{"strength", 0.0}, {"importance", 0.0}, {"handles", json::array({1})}, {"assignment", json::object()}, {"metta_expression", json::object()}}; EXPECT_THROW(QueryAnswer().from_json(handles_not_array), runtime_error);🤖 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/query_answer_test.cc` around lines 507 - 538, Extend QueryAnswer.from_json edge-case coverage in from_json_edge_cases with tests for handles and assignment collections exceeding their validation size limits, asserting runtime_error. Also add a case where handles contains a scalar element such as 1, distinct from an array element with an invalid handle type, and assert the same exception.Source: Path instructions
src/agents/query_engine/QueryAnswer.cc (1)
143-192: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnconditional metta JSON allocations even when
metta_flagis false.
metta_group_jsonis constructed for every handle group (andmetta_handles_json/assignment_metta_jsononce) even whenmetta_flagis false. Since nlohmann::json stores arrays/objects as heap pointers, eachjson::array()/json::object()call allocates. Theelsebranch at Lines 183-186 then discards these and builds fresh empty containers instead of reusing them — so this work is 100% wasted on the (likely default/common) non-metta path of what appears to be a per-answer streaming hot path.As per path instructions, "Avoid heap allocations in hot paths ... MEMORY & PERFORMANCE (high priority)".
⚡ Proposed fix: only allocate metta containers when needed
- json handles_json = json::array(); - json metta_handles_json = json::array(); + json handles_json = json::array(); + json metta_handles_json = metta_flag ? json::array() : json(); for (const vector<string>& group : this->handles) { json group_json = json::array(); - json metta_group_json = json::array(); + json metta_group_json = metta_flag ? json::array() : json(); for (const string& handle : group) { group_json.push_back(handle); if (metta_flag) { auto it = this->metta_expression.find(handle); metta_group_json.push_back( (it != this->metta_expression.end() && !it->second.empty()) ? it->second : handle); } } handles_json.push_back(group_json); if (metta_flag) { metta_handles_json.push_back(metta_group_json); } } json assignment_json = json::object(); - json assignment_metta_json = json::object(); + json assignment_metta_json = metta_flag ? json::object() : json(); for (const auto& pair : this->assignment.table) {🤖 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/query_engine/QueryAnswer.cc` around lines 143 - 192, Update QueryAnswer::to_json so metta_handles_json, assignment_metta_json, and each per-group metta_group_json are allocated only when metta_flag is true. Preserve the existing metta output when enabled, while assigning the required empty array/object directly in the false branch without creating discarded containers on the non-metta path.Source: Path instructions
🤖 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/query_engine/QueryAnswer.cc`:
- Around line 194-277: Update QueryAnswer::from_json so all strength,
importance, handles, assignment, and metta_expression validation completes
before mutating this, or build the parsed result in temporary state and commit
it only after success. Preserve the existing successful parsing behavior while
ensuring any RAISE_ERROR leaves the current QueryAnswer unchanged or in a
consistently reset state.
---
Nitpick comments:
In `@src/agents/query_engine/QueryAnswer.cc`:
- Around line 143-192: Update QueryAnswer::to_json so metta_handles_json,
assignment_metta_json, and each per-group metta_group_json are allocated only
when metta_flag is true. Preserve the existing metta output when enabled, while
assigning the required empty array/object directly in the false branch without
creating discarded containers on the non-metta path.
In `@src/tests/cpp/query_answer_test.cc`:
- Around line 507-538: Extend QueryAnswer.from_json edge-case coverage in
from_json_edge_cases with tests for handles and assignment collections exceeding
their validation size limits, asserting runtime_error. Also add a case where
handles contains a scalar element such as 1, distinct from an array element with
an invalid handle type, and assert the same exception.
🪄 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: e32f548e-8d8c-4375-82b2-36303f4494db
📒 Files selected for processing (4)
src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.ccsrc/agents/query_engine/QueryAnswer.ccsrc/agents/query_engine/QueryAnswer.hsrc/tests/cpp/query_answer_test.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/agents/command_router/http_api/BusCommandRouterProxyStreamPoller.cc
andre-senna
left a comment
andre-senna
left a comment
There was a problem hiding this comment.
@marcocapozzoli
Please address my last comment before merging.
| } | ||
| } | ||
|
|
||
| this->strength = strength; |
There was a problem hiding this comment.
I understand the point of having this "summarization" of everything that's required to build the QueryAnswer object but because we have this->handles and this->assignment (which are not scalar types) we are ending up doing unnecessary copies. Therefore, I suggest using "this->*" directly in each place above in order to avoid this unnecessary copy here.
2 participants
Summary
QueryAnswer::to_json()to serialize QueryAnswers as structured JSON objects.QueryAnswer::from_json()to rebuild a QueryAnswer from a JSON object.to_json()withmetta_flag=false.{ "handles":[["289476dbf62faffc149e3658bc368b6b"]], "assignment":{"X":"a408f6dd446cdd4fa56f82e77fe6c870"}, "metta_expressions":[], "assignment_metta":{}, "importance":0.0, "strength":0.0 }to_json()withmetta_flag=true.{ "handles":[["289476dbf62faffc149e3658bc368b6b"]], "assignment": {"X":"a408f6dd446cdd4fa56f82e77fe6c870"}, "metta_expressions": [["(Similarity \"human\" \"ent\")"]], "assignment_metta":{"X":"\"ent\""}, "strength":0.0, "importance":0.0 }Resolves #1196