[#1195] Add parameters support to POST /command-router/executions - #1200
Open
marcocapozzoli wants to merge 5 commits into
Open
[#1195] Add parameters support to POST /command-router/executions#1200marcocapozzoli wants to merge 5 commits into
marcocapozzoli wants to merge 5 commits into
Conversation
Contributor
WalkthroughThe command-router execution endpoint accepts an optional JSON ChangesCommand-router parameter execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant CommandRouterHttpAPI
participant SystemParametersSingleton
participant RouterCommandExecutor
HTTPClient->>CommandRouterHttpAPI: POST execution with parameters
CommandRouterHttpAPI->>SystemParametersSingleton: validate parameter schema
SystemParametersSingleton-->>CommandRouterHttpAPI: expected parameter type
CommandRouterHttpAPI->>RouterCommandExecutor: execute generated param command
RouterCommandExecutor-->>CommandRouterHttpAPI: execution result
CommandRouterHttpAPI-->>HTTPClient: HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/agents/command_router/http_api/CommandRouterHttpAPI.cc (1)
600-640: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant
value.get<string>()calls in thelong/doublestring paths.
value.get<string>()is called twice in thelongbranch (lines 606-607) and three times in thedoublebranch (lines 623-627), each producing a fresh string copy. Bind once to a local reference, as already done in theunsigned intstring branch (line 589).♻️ Proposed refactor
} else if (holds_alternative<long>(param_it->second)) { if (value.is_number_integer()) { formatted_value = std::to_string(value.get<long long>()); } else if (value.is_string()) { + const string& text = value.get<string>(); try { size_t consumed = 0; - const long parsed = stol(value.get<string>(), &consumed); - if (consumed != value.get<string>().size()) { + const long parsed = stol(text, &consumed); + if (consumed != text.size()) { return fail("Parameter '" + key + "' expects integer"); }} else if (value.is_string()) { + const string& text = value.get<string>(); try { size_t consumed = 0; - stod(value.get<string>(), &consumed); - if (consumed != value.get<string>().size()) { + stod(text, &consumed); + if (consumed != text.size()) { return fail("Parameter '" + key + "' expects number"); } - formatted_value = value.get<string>(); + formatted_value = 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 600 - 640, In the long and double string-conversion branches of the parameter formatting logic, bind value.get<string>() once to a local reference and reuse it for parsing, length comparison, and formatted output. Match the existing approach in the unsigned int string branch while preserving all current validation and error behavior.
🤖 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/http_api/CommandRouterHttpAPI.cc`:
- Around line 541-542: Update get_command_router_params() to return a stable
const reference or immutable memoized view instead of a Properties value, and
ensure its stored result outlives each caller. Preserve build_set_param_arg()’s
const Properties& usage while avoiding a temporary allocation for every
processed parameter.
- Around line 557-578: Update the bool conversion branch in the
parameter-formatting logic to accept string values "1" and "0" in addition to
"true" and "false", mapping them to the corresponding boolean text. Keep
rejecting other strings and preserve the existing error message and numeric
handling.
- Around line 163-188: Make parameter application in the command handler
validate/build every parameter argument before executing any set command. Store
the results from build_set_param_arg, return the existing 400 response
immediately on validation failure, then perform execute_router_command("set",
...) in a separate pass using the prevalidated arguments; preserve the current
router-error handling for execution failures.
- Around line 579-599: Update the unsigned-int handling branch in the parameter
formatting logic to reject values greater than numeric_limits<unsigned
int>::max(). Apply this validation consistently to unsigned JSON numbers,
non-negative signed integers, and digit-only strings, returning the existing
clean parameter error instead of forwarding oversized values.
---
Nitpick comments:
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 600-640: In the long and double string-conversion branches of the
parameter formatting logic, bind value.get<string>() once to a local reference
and reuse it for parsing, length comparison, and formatted output. Match the
existing approach in the unsigned int string branch while preserving all current
validation and error behavior.
🪄 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: 91fabef2-cd79-43e6-aa09-ac2b778c78e0
📒 Files selected for processing (3)
src/agents/command_router/http_api/CommandRouterHttpAPI.ccsrc/agents/command_router/http_api/CommandRouterHttpAPI.hsrc/tests/cpp/command_router_http_api_test.cc
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agents/command_router/http_api/CommandRouterHttpAPI.cc (1)
163-164: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-copying
Propertiesfor router parameter validation.
get_command_router_params()already returns by value, andPropertiesinheritsunordered_map, so itsfind()lookup is const-compatible. Bind the result viaconst Properties&, updatebuild_set_param_arg()to takeconst Properties&, and this request can avoid the extraPropertiescopy.🤖 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 163 - 164, Update the command-router parameter validation to bind the result of get_command_router_params() as const Properties&, and change build_set_param_arg() to accept const Properties& so find() remains const-compatible without copying the Properties object.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/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 163-164: Update the command-router parameter validation to bind
the result of get_command_router_params() as const Properties&, and change
build_set_param_arg() to accept const Properties& so find() remains
const-compatible without copying the Properties object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 67bf5c1b-7574-4901-bf7d-03627bb2b691
📒 Files selected for processing (3)
src/agents/command_router/http_api/CommandRouterHttpAPI.ccsrc/agents/command_router/http_api/CommandRouterHttpAPI.hsrc/tests/cpp/command_router_http_api_test.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- src/agents/command_router/http_api/CommandRouterHttpAPI.h
- src/tests/cpp/command_router_http_api_test.cc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/agents/command_router/http_api/CommandRouterHttpAPI.cc (2)
655-659: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject whitespace-only strings before dispatch.
A value like
" "passestext.empty()but is trimmed to empty by the router’ssethandler, producing a 500 instead of the required client-validation 400. Validate the trimmed value is non-empty and add a regression case.🤖 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 655 - 659, Update the parameter validation around the string handling in CommandRouterHttpAPI to trim the input before checking emptiness, rejecting whitespace-only values with the existing fail path and client-validation response. Preserve the original non-empty value behavior for dispatch, and add a regression test covering a whitespace-only string.
176-184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve percent characters in parameter values.
At Lines 176-184, generated
setarguments go throughexecute_router_command(), which rewrites every%to$at Lines 426-428. A valid string parameter such as"50%"is therefore persisted as"50$". Dispatch generatedsetarguments without command-text translation and add a regression case for%.🤖 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 176 - 184, Update the generated set-argument path in the command router loop to dispatch parameter values without the command-text translation performed by execute_router_command, preserving literal percent characters such as “50%”. Add a regression case covering a generated set argument containing % and verify the persisted value remains unchanged.
🧹 Nitpick comments (1)
src/agents/command_router/http_api/CommandRouterHttpAPI.cc (1)
165-175: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReserve and move prevalidated arguments.
set_argsgrows repeatedly, andconst optional<string>forces an extra string copy into it. Reserve the object size and move the generated argument.Suggested change
vector<pair<string, string>> set_args; + set_args.reserve(body["parameters"].size()); for (const auto& [key, value] : body["parameters"].items()) { string validation_error; - const optional<string> args = + optional<string> args = this->build_set_param_arg(known_params, key, value, validation_error); @@ - set_args.emplace_back(key, args.value()); + set_args.emplace_back(key, std::move(*args));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/CommandRouterHttpAPI.cc` around lines 165 - 175, In the parameter-processing block, reserve set_args capacity using body["parameters"].size() before the loop to avoid repeated reallocations, and make the optional result from build_set_param_arg movable so emplace_back transfers the generated string instead of copying it. Preserve the existing validation error response and early return behavior.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/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 602-610: The unsigned string handling in the value conversion
branch must reject any input that is not composed entirely of ASCII digits
before calling stoull. Update the logic around parsed, consumed, and fits_uint
to validate the complete text first, preserving the existing uint_error failure
path and accepting only digit-only values.
---
Outside diff comments:
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 655-659: Update the parameter validation around the string
handling in CommandRouterHttpAPI to trim the input before checking emptiness,
rejecting whitespace-only values with the existing fail path and
client-validation response. Preserve the original non-empty value behavior for
dispatch, and add a regression test covering a whitespace-only string.
- Around line 176-184: Update the generated set-argument path in the command
router loop to dispatch parameter values without the command-text translation
performed by execute_router_command, preserving literal percent characters such
as “50%”. Add a regression case covering a generated set argument containing %
and verify the persisted value remains unchanged.
---
Nitpick comments:
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 165-175: In the parameter-processing block, reserve set_args
capacity using body["parameters"].size() before the loop to avoid repeated
reallocations, and make the optional result from build_set_param_arg movable so
emplace_back transfers the generated string instead of copying it. Preserve the
existing validation error response and early return behavior.
🪄 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: f0fee96f-67fe-4a99-b4cf-ea9432994aaf
📒 Files selected for processing (1)
src/agents/command_router/http_api/CommandRouterHttpAPI.cc
Comment on lines
+602
to
+610
| } else if (value.is_string()) { | ||
| const string& text = value.get<string>(); | ||
| try { | ||
| size_t consumed = 0; | ||
| const unsigned long long parsed = stoull(text, &consumed); | ||
| if (consumed != text.size() || !fits_uint(parsed)) { | ||
| return fail(uint_error); | ||
| } | ||
| formatted_value = text; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C5 'stoull\(|formatted_value = text|all_digits|set_router_param' \
src/agents/command_router/http_api/CommandRouterHttpAPI.cc \
src/agents/command_router/BusCommandRouterProcessor.ccRepository: singnet/das
Length of output: 7525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- CommandRouterHttpAPI.cc relevant section ---\n'
sed -n '530,625p' src/agents/command_router/http_api/CommandRouterHttpAPI.cc | cat -n
printf '\n--- BusCommandRouterProcessor.cc relevant section ---\n'
sed -n '120,180p' src/agents/command_router/BusCommandRouterProcessor.cc | cat -n
printf '\n--- Behavioral probes for stoull parsing and processor validation ---\n'
python3 - <<'PY'
import subprocess, json, textwrap, tmpdir
import tempfile, os, pathlib, sys, re
# No-repo dependency here; just demonstrate relevant stoull semantics outside repo.
samples = ["0", "1", "+1", "-0", "01", "1 ", " 1", "1a", "-1"]
for s in samples:
try:
import ctypes, ctypes.util
# Python does not expose stoull directly; use a tiny C helper if gcc is available.
if subprocess.run(["gcc", "--version"], stdout=subprocess.PIPE, stderr=subprocess.PIPE).returncode == 0:
break
except Exception:
pass
PY
cat >/tmp/stoull_probe.cc <<'CPP'
`#include` <cstdlib>
`#include` <errno.h>
`#include` <iostream>
`#include` <string>
int main() {
const char* samples[] = {"0","1","+1","-0","01","1 "," 1","1a","-1",nullptr};
for (auto p = samples; *p; ++p) {
errno = 0;
char* end = nullptr;
unsigned long long v = std::strtoull(*p, &end, 10);
std::cout << *p << "\tends_with=" << (*end == '\0') << "\tconsumed=" << (end-*p) << "\tblob_len=" << std::strlen(*p) << "\tval=" << v << "\terr=" << errno << "\n";
}
return 0;
}
CPP
g++ -std=c++17 /tmp/stoull_probe.cc -o /tmp/stoull_probe && /tmp/stoull_probeRepository: singnet/das
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CommandRouterHttpAPI.cc relevant section ---'
sed -n '530,625p' src/agents/command_router/http_api/CommandRouterHttpAPI.cc | cat -n
printf '%s\n' '--- BusCommandRouterProcessor.cc relevant section ---'
sed -n '120,180p' src/agents/command_router/BusCommandRouterProcessor.cc | cat -n
printf '%s\n' '--- Behavioral probes for C strtoull parsing used by stoull ---'
cat >/tmp/stoull_probe.cc <<'CPP'
`#include` <cstdlib>
`#include` <cerrno>
`#include` <iostream>
`#include` <cstring>
int main() {
const char* samples[] = {"0","1","+1","-0","01","1 "," 1","1a","-1",nullptr};
for (auto p = samples; *p; ++p) {
errno = 0;
char* end = nullptr;
unsigned long long v = std::strtoull(*p, &end, 10);
std::cout << *p << "\tends_with=" << (*end == '\0') << "\tconsumed=" << (end-*p) << "\tblob_len=" << std::strlen(*p) << "\tval=" << v << "\terr=" << errno << "\n";
}
return 0;
}
CPP
g++ -std=c++17 /tmp/stoull_probe.cc -o /tmp/stoull_probe && /tmp/stoull_probeRepository: singnet/das
Length of output: 7887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import subprocess, tempfile, os
samples = ["0", "1", "+1", "-0", "01", "1 ", " 1", "1a", "-1"]
for s in samples:
try:
v = int(s)
except ValueError as e:
v = f"ValueError({e!s})"
print(f"{s!r}\tconsumed={len(s)}\tvalue={v}")
c = """
`#include` <cstdlib>
`#include` <cerrno>
`#include` <iostream>
`#include` <cstring>
int main() {
const char* samples[] = {"0","1","+1","-0","01",nullptr};
for (auto p = samples; *p; ++p) {
errno = 0;
char* end = nullptr;
unsigned long long v = std::strtoull(*p, &end, 10);
std::cout << *p << "\\tends_with=" << (*end == '\\0') << "\\tconsumed=" << (int)(end-*p) << "\\tblob_len=" << (int)std::strlen(*p) << "\\tval=" << v << "\\terr=" << errno << "\\n";
}
}
"""
with tempfile.NamedTemporaryFile(suffix=".cc", delete=False) as f:
f.write(c.encode())
tf = f.name
try:
out = subprocess.run(["clang++", "-std=c++17", tf], capture_output=True, text=True)
if out.returncode != 0:
print("clang++ failed:", out.stderr)
else:
subprocess.run([tf])
finally:
os.unlink(tf)
PYRepository: singnet/das
Length of output: 594
Reject non-digit unsigned strings before parsing.
stoull accepts signed forms such as "+1" and "-0", and the parser currently accepts a trailing " " because it only checks consumed != text.size(). Since the router parameter handling rejects unsigned values unless they are all digits, forward parsing as 400/500 should not depend on compiler/toolchain quirks. Require text to be all ASCII digits before parsing, or canonicalize using std::to_string(parsed) after rejection.
🤖 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 602
- 610, The unsigned string handling in the value conversion branch must reject
any input that is not composed entirely of ASCII digits before calling stoull.
Update the logic around parsed, consumed, and fits_uint to validate the complete
text first, preserving the existing uint_error failure path and accepting only
digit-only values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR lets clients send router/query parameters in the HTTP execution request body, so you no longer need a separate set call before running a query.
Changes
Example
{ "command_type": "query", "command_text": "(Similarity \"human\" %C)", "parameters": { "populate_metta_mapping": true, "use_metta_as_query_tokens": "true", } }Resolves #1195