[#1146] Add synchronous get/set execution on POST /command-router/executions

[marcocapozzoli]

No description provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f5272966-ef31-4bef-995b-ebf7318a3aee

📥 Commits

Reviewing files that changed from the base of the PR and between 886249f and ccb7d24.

📒 Files selected for processing (1)

• src/agents/command_router/http_api/CommandRouterHttpAPI.cc

🚧 Files skipped from review as they are similar to previous changes (1)

• src/agents/command_router/http_api/CommandRouterHttpAPI.cc

---

• Adds synchronous get/set execution to POST /command-router/executions, returning 200 results while preserving asynchronous 202 scheduling for other commands.
• Centralizes router dispatch, stream polling, argument normalization, abort handling, and exception reporting; failed executions skip completion recording.
• Aggregating streamed chunks into a vector increases allocation on the synchronous path; shared http_requestor_id state should be reviewed for concurrent-request thread safety.
• Adds four C++ HTTP API tests covering synchronous get/set behavior, persistence, and unknown-parameter errors. No client-suite tests are indicated.

Walkthrough

Adds synchronous execution for get and set commands, introduces shared router dispatch state and a reusable execution helper, preserves asynchronous handling for other command types, and adds HTTP API tests for results, persistence, and invalid parameters.

Changes

Sync command execution

Layer / File(s)	Summary
Dispatch contract and shared requestor state src/agents/command_router/http_api/CommandRouterHttpAPI.h	Adds callback-based helper declarations, synchronous command classification, shared http_requestor_id state, and updated route documentation.
Shared router dispatch and async integration src/agents/command_router/http_api/CommandRouterHttpAPI.cc	Initializes http_requestor_id, centralizes dispatch and stream polling, and prevents asynchronous completion recording after dispatch failure.
Synchronous executions route src/agents/command_router/http_api/CommandRouterHttpAPI.cc	Executes get and set requests immediately, aggregates chunks, returns 200 or 500, and leaves other command types on the asynchronous path.
Synchronous execution validation src/tests/cpp/command_router_http_api_test.cc	Tests synchronous get results, set persistence, boolean parameter retrieval, and unknown parameter errors.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant CommandRouterHttpAPI
  participant execute_router_command
  participant RouterProcessor

  Client->>CommandRouterHttpAPI: POST /command-router/executions
  alt get or set command
    CommandRouterHttpAPI->>execute_router_command: dispatch synchronously
    execute_router_command->>RouterProcessor: normalize arguments and dispatch
    RouterProcessor-->>execute_router_command: stream chunks or error
    execute_router_command-->>CommandRouterHttpAPI: chunks or failure
    CommandRouterHttpAPI-->>Client: 200 result or 500 error
  else other command type
    CommandRouterHttpAPI-->>Client: 202 accepted
    CommandRouterHttpAPI->>execute_router_command: dispatch from async execution
    execute_router_command->>RouterProcessor: poll router stream
  end

Loading

Possibly related PRs

• singnet/das#1156: Both PRs modify CommandRouterHttpAPI HTTP server behavior.
• singnet/das#1165: Introduces the execution-based asynchronous workflow reused by this dispatch refactor.
• singnet/das#1168: Provides the in-process HTTP dispatch and stream-polling mechanisms used by this PR.

Suggested reviewers: andre-senna, ccgsnet

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided, so there is nothing substantive to validate against the changeset.	Add a brief description of the endpoint change and added tests, or provide a short summary of the intended behavior.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: synchronous get/set handling on the executions endpoint.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Tests For Behavior Changes	✅ Passed	PASS: PR adds command-router HTTP tests for the new sync get/set path and a new StoppableThread test suite for detach/reap behavior.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch masc/1146-3

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[marcocapozzoli]

@CodeRabbit full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

🧹 Nitpick comments (1)

src/tests/cpp/command_router_http_api_test.cc (1)

451-509: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Solid sync-path coverage; consider adding a type-mismatch case.

These tests correctly exercise execute_router_command's sync branch against real get/set behavior, and each is self-contained (set-then-get within the same test), avoiding order-dependent fragility. One gap: set_router_param has a distinct error branch for type mismatches (e.g., assigning a non-true/false string to a bool parameter) that produces a different error message than "Unknown parameter" — currently untested.

🧪 Example additional test

TEST_F(CommandRouterHttpAPITest, set_param_rejects_type_mismatch) {
    auto res = client().Post("/command-router/executions",
                             make_execution_body("set", "param use_cache not_a_bool").dump(),
                             "application/json");
    ASSERT_TRUE(res);
    EXPECT_EQ(res->status, 500);

    auto payload = json::parse(res->body);
    EXPECT_TRUE(payload.contains("error"));
    EXPECT_NE(payload["error"].get<string>().find("cannot assign value"), string::npos);
}

🤖 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 451 - 509, Add a
test for the missing type-mismatch error path in CommandRouterHttpAPITest by
exercising execute_router_command through the sync POST endpoint with
set_router_param using a bool parameter like use_cache and a non-bool value.
Assert the response is a 500, parse the JSON error payload, and verify it
contains the type-mismatch message (not the unknown-parameter branch) so
set_router_param’s distinct validation path is covered.

🤖 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 451-509: Add a test for the missing type-mismatch error path in
CommandRouterHttpAPITest by exercising execute_router_command through the sync
POST endpoint with set_router_param using a bool parameter like use_cache and a
non-bool value. Assert the response is a 500, parse the JSON error payload, and
verify it contains the type-mismatch message (not the unknown-parameter branch)
so set_router_param’s distinct validation path is covered.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: faf9bea8-4434-4071-87db-1ec4fbc25431

📥 Commits

Reviewing files that changed from the base of the PR and between 9803625 and 886249f.

📒 Files selected for processing (3)

• src/agents/command_router/http_api/CommandRouterHttpAPI.cc
• src/agents/command_router/http_api/CommandRouterHttpAPI.h
• src/tests/cpp/command_router_http_api_test.cc

[andre-senna]

Please wait for @ccgsnet approval before merging