From 3ea377e56f2e8d2c1f38d838c53b1e007c33bbb6 Mon Sep 17 00:00:00 2001 From: Joachim Rosskopf Date: Mon, 31 Aug 2026 17:33:00 +0200 Subject: [PATCH 1/2] fix(mcp): move policy into the ADT layer, where both callers inherit it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probing the 77 MCP tools the way an agent calls them — over the protocol rather than through the CLI — found two commands that work from the command line and fail as tools. Both were my own fixes from earlier today, applied in the CLI handler: - `bw_application_log` answered HTTP 400 "Parameter username could not be found". The CLI defaults username and the timestamp window; the MCP handler passed nothing, so the tool called a route that requires all three. - `bw_validate` answered HTTP 500 "Action 'validate' is not valid". The CLI had been corrected to send `exists`, but the tool's own default still said `validate`, and its schema documented that as the default. The lesson is the layering, not the two bugs: policy that a route requires belongs where every caller passes through. BwGetApplicationLog now applies the defaults itself, and the CLI passes only what the user typed. The MCP validation tool no longer overrides the option default at all. Defaulting the username needed the session's own user, which nothing below the CLI could see, so IAdtSession gained LogonUser(). That is what made the CLI-only workaround necessary in the first place. Verified over the MCP protocol: bw_application_log with no arguments returns a feed, bw_validate returns an empty message list for a real object and a not-found error for a missing one. Also confirmed by the same sweep, and not defects: the six tools my probe called with wrong argument names declare those names correctly in their schemas, and bw_transport_check's /sap/bw/modeling/cto route is absent on this release, like the four dead BW routes already documented. The accessor is LogonUserName(), not LogonUser(): windows.h defines LogonUser as a macro expanding to LogonUserA, and httplib pulls windows.h in on the MSVC build, so the first name broke that platform only. --- include/erpl_adt/adt/adt_session.hpp | 2 ++ include/erpl_adt/adt/i_adt_session.hpp | 9 +++++ src/adt/adt_session.cpp | 4 +++ src/adt/bw_repo_utils.cpp | 33 +++++++++++++++++- src/cli/command_executor.cpp | 30 +++-------------- src/mcp/mcp_tool_handlers.cpp | 6 ++-- test/adt/test_bw_repo_utils.cpp | 46 ++++++++++++++++++++++++++ test/mocks/mock_adt_session.hpp | 5 +++ 8 files changed, 107 insertions(+), 28 deletions(-) diff --git a/include/erpl_adt/adt/adt_session.hpp b/include/erpl_adt/adt/adt_session.hpp index 86ae211..1332190 100644 --- a/include/erpl_adt/adt/adt_session.hpp +++ b/include/erpl_adt/adt/adt_session.hpp @@ -58,6 +58,8 @@ class AdtSession : public IAdtSession { // -- IAdtSession implementation ------------------------------------------ + [[nodiscard]] std::string LogonUserName() const override; + [[nodiscard]] Result Get( std::string_view path, const HttpHeaders& headers = {}) override; diff --git a/include/erpl_adt/adt/i_adt_session.hpp b/include/erpl_adt/adt/i_adt_session.hpp index 8e24e63..8ccb2a2 100644 --- a/include/erpl_adt/adt/i_adt_session.hpp +++ b/include/erpl_adt/adt/i_adt_session.hpp @@ -65,6 +65,15 @@ class IAdtSession { // -- HTTP verbs ---------------------------------------------------------- + // The user this session authenticates as. Some routes need it as a + // parameter (the BW application log filters by user), and without it here + // only the CLI could supply a default — which is how the MCP tool ended up + // broken while the CLI worked. + // + // Not LogonUser(): windows.h defines that as a macro expanding to + // LogonUserA, and httplib pulls windows.h in on the MSVC build. + [[nodiscard]] virtual std::string LogonUserName() const = 0; + [[nodiscard]] virtual Result Get( std::string_view path, const HttpHeaders& headers = {}) = 0; diff --git a/src/adt/adt_session.cpp b/src/adt/adt_session.cpp index f406d5a..d534b4f 100644 --- a/src/adt/adt_session.cpp +++ b/src/adt/adt_session.cpp @@ -167,6 +167,7 @@ httplib::Headers BuildRequestHeaders( struct AdtSession::Impl { std::unique_ptr client; std::string sap_client; + std::string logon_user; std::string accept_language; // resolved SAP logon language std::optional csrf_token; // ADT paths (/sap/bc/adt/) std::optional bw_csrf_token_; // BW paths (/sap/bw/modeling/) @@ -186,6 +187,7 @@ struct AdtSession::Impl { const std::string& sap_client_value, const AdtSessionOptions& opts) : sap_client(sap_client_value), + logon_user(user), accept_language(ResolveAcceptLanguage(opts.language)), options(opts) { base_url_ = (use_https ? "https://" : "http://") + host + ":" + @@ -498,6 +500,8 @@ AdtSession::~AdtSession() = default; // --------------------------------------------------------------------------- // Get — with 403 CSRF retry // --------------------------------------------------------------------------- +std::string AdtSession::LogonUserName() const { return impl_->logon_user; } + Result AdtSession::Get(std::string_view path, const HttpHeaders& headers) { auto result = impl_->DoGet(path, headers); diff --git a/src/adt/bw_repo_utils.cpp b/src/adt/bw_repo_utils.cpp index 3299bd7..4da9c13 100644 --- a/src/adt/bw_repo_utils.cpp +++ b/src/adt/bw_repo_utils.cpp @@ -1,4 +1,5 @@ #include +#include #include "adt_utils.hpp" #include "atom_parser.hpp" @@ -275,6 +276,19 @@ std::string BuildApplicationLogUrl(const BwApplicationLogOptions& options) { return path; } +// SAP's yyyyMMddHHmmss stamp, as the application-log route expects it. +std::string FormatSapTimestamp(std::time_t when) { + std::tm tm_value{}; +#ifdef _WIN32 + localtime_s(&tm_value, &when); +#else + localtime_r(&when, &tm_value); +#endif + char buffer[16]; + std::strftime(buffer, sizeof(buffer), "%Y%m%d%H%M%S", &tm_value); + return buffer; +} + std::string BuildMessageUrl(const BwMessageTextOptions& options) { std::string path = std::string(kMessagePath) + "/" + UrlEncode(options.identifier) + "/" + UrlEncode(options.text_type); @@ -402,7 +416,24 @@ BwGetNodePath(IAdtSession& session, const std::string& object_uri) { Result, Error> BwGetApplicationLog(IAdtSession& session, const BwApplicationLogOptions& options) { - auto path = BuildApplicationLogUrl(options); + // username, starttimestamp and endtimestamp are all mandatory: without + // them the route answers HTTP 400 "Parameter username could not be found". + // Defaulting here rather than in the CLI is deliberate — the same defaults + // were once applied only in the CLI handler, which left the MCP tool + // broken while the command worked. + auto effective = options; + if (!effective.username.has_value() || effective.username->empty()) { + effective.username = session.LogonUserName(); + } + const auto now = std::time(nullptr); + if (!effective.end_timestamp.has_value() || effective.end_timestamp->empty()) { + effective.end_timestamp = FormatSapTimestamp(now); + } + if (!effective.start_timestamp.has_value() || effective.start_timestamp->empty()) { + effective.start_timestamp = FormatSapTimestamp(now - 7 * 24 * 60 * 60); + } + + auto path = BuildApplicationLogUrl(effective); auto xml_result = FetchAtom(session, path, "BwGetApplicationLog"); if (xml_result.IsErr()) { return Result, Error>::Err( diff --git a/src/cli/command_executor.cpp b/src/cli/command_executor.cpp index 8b75490..d1d81fd 100644 --- a/src/cli/command_executor.cpp +++ b/src/cli/command_executor.cpp @@ -364,19 +364,6 @@ std::string ResolveConnectionSetting(const CommandArgs& args, return saved.empty() ? fallback : saved; } -// SAP's yyyyMMddHHmmss timestamp, as the BW application-log route expects it. -std::string FormatSapTimestamp(std::time_t when) { - std::tm tm_value{}; -#ifdef _WIN32 - localtime_s(&tm_value, &when); -#else - localtime_r(&when, &tm_value); -#endif - char buffer[16]; - std::strftime(buffer, sizeof(buffer), "%Y%m%d%H%M%S", &tm_value); - return buffer; -} - // Resolve the effective logon user the same way CreateSession does: // explicit flag > environment > saved credentials > default. std::string ResolveUserName(const CommandArgs& args) { @@ -4095,19 +4082,12 @@ int HandleBwApplicationLog(const CommandArgs& args) { auto session = RequireSession(args, fmt); if (!session) return 99; - // All three parameters are mandatory on the backend — without them it - // answers HTTP 400 "Parameter username could not be found", which is why - // a bare `bw applog` never worked. Default to this user's log over the - // last week rather than making the caller spell out a timestamp format. + // The mandatory parameters are defaulted in BwGetApplicationLog, so the + // CLI and the MCP tool behave the same; only explicit flags are passed. BwApplicationLogOptions opts; - opts.username = HasFlag(args, "username") ? GetFlag(args, "username") - : ResolveUserName(args); - opts.end_timestamp = HasFlag(args, "end") ? GetFlag(args, "end") - : FormatSapTimestamp(std::time(nullptr)); - opts.start_timestamp = - HasFlag(args, "start") - ? GetFlag(args, "start") - : FormatSapTimestamp(std::time(nullptr) - 7 * 24 * 60 * 60); + if (HasFlag(args, "username")) opts.username = GetFlag(args, "username"); + if (HasFlag(args, "start")) opts.start_timestamp = GetFlag(args, "start"); + if (HasFlag(args, "end")) opts.end_timestamp = GetFlag(args, "end"); auto result = BwGetApplicationLog(*session, opts); if (result.IsErr()) { diff --git a/src/mcp/mcp_tool_handlers.cpp b/src/mcp/mcp_tool_handlers.cpp index 69d80a2..76e9512 100644 --- a/src/mcp/mcp_tool_handlers.cpp +++ b/src/mcp/mcp_tool_handlers.cpp @@ -2074,7 +2074,7 @@ void RegisterAdtTools(ToolRegistry& registry, IAdtSession& session) { MakeSchema( {{"object_type", StringProp("Object type")}, {"object_name", StringProp("Object name")}, - {"action", StringProp("Validation action (default: validate)")}}, + {"action", StringProp("exists (default), new, standard_transport or is_plannable")}}, {"object_type", "object_name"}), [&session](const nlohmann::json& params) -> ToolResult { ToolResult err; @@ -2086,7 +2086,9 @@ void RegisterAdtTools(ToolRegistry& registry, IAdtSession& session) { BwValidationOptions opts; opts.object_type = *object_type; opts.object_name = *object_name; - opts.action = OptString(params, "action", "validate"); + // "validate" is not an action the backend accepts; leaving this + // empty takes BwValidationOptions' default of "exists". + opts.action = OptString(params, "action", ""); auto result = BwValidateObject(session, opts); if (result.IsErr()) return MakeErrorResult(result.Error()); diff --git a/test/adt/test_bw_repo_utils.cpp b/test/adt/test_bw_repo_utils.cpp index f6b3540..031ed99 100644 --- a/test/adt/test_bw_repo_utils.cpp +++ b/test/adt/test_bw_repo_utils.cpp @@ -150,3 +150,49 @@ TEST_CASE("BwGetMessageText: validates required parameters", "[adt][bw][repo-uti auto result = BwGetMessageText(mock, opts); REQUIRE(result.IsErr()); } + + +// =========================================================================== +// The application-log route needs username, starttimestamp and endtimestamp; +// without them it answers HTTP 400. Defaulting them in the ADT layer rather +// than in the CLI handler is deliberate: when the defaults lived in the CLI, +// the MCP tool called the route with nothing and stayed broken. +// =========================================================================== + +TEST_CASE("BwGetApplicationLog: defaults the mandatory parameters", + "[adt][bw][repo_utils]") { + MockAdtSession mock; + mock.SetLogonUserName("TESTUSER"); + mock.EnqueueGet(Result::Ok( + {200, {}, R"()"})); + + auto result = BwGetApplicationLog(mock, BwApplicationLogOptions{}); + REQUIRE(result.IsOk()); + + REQUIRE(mock.GetCallCount() == 1); + const auto& path = mock.GetCalls()[0].path; + CHECK(path.find("username=TESTUSER") != std::string::npos); + CHECK(path.find("starttimestamp=") != std::string::npos); + CHECK(path.find("endtimestamp=") != std::string::npos); +} + +TEST_CASE("BwGetApplicationLog: explicit values win over the defaults", + "[adt][bw][repo_utils]") { + MockAdtSession mock; + mock.SetLogonUserName("TESTUSER"); + mock.EnqueueGet(Result::Ok( + {200, {}, R"()"})); + + BwApplicationLogOptions options; + options.username = "SOMEONE"; + options.start_timestamp = "20260101000000"; + options.end_timestamp = "20261231235959"; + + auto result = BwGetApplicationLog(mock, options); + REQUIRE(result.IsOk()); + + const auto& path = mock.GetCalls()[0].path; + CHECK(path.find("username=SOMEONE") != std::string::npos); + CHECK(path.find("starttimestamp=20260101000000") != std::string::npos); + CHECK(path.find("endtimestamp=20261231235959") != std::string::npos); +} diff --git a/test/mocks/mock_adt_session.hpp b/test/mocks/mock_adt_session.hpp index 1602f22..35b17a1 100644 --- a/test/mocks/mock_adt_session.hpp +++ b/test/mocks/mock_adt_session.hpp @@ -149,6 +149,10 @@ class MockAdtSession : public IAdtSession { // -- IAdtSession implementation ------------------------------------------ + [[nodiscard]] std::string LogonUserName() const override { return logon_user_; } + + void SetLogonUserName(std::string user) { logon_user_ = std::move(user); } + Result Get( std::string_view path, const HttpHeaders& headers) override { @@ -226,6 +230,7 @@ class MockAdtSession : public IAdtSession { } private: + std::string logon_user_ = "DEVELOPER"; static Result Dequeue( std::deque>& queue, std::string_view operation, From 17f14106460a673837cf15d98b325ef38a9c4637 Mon Sep 17 00:00:00 2001 From: Joachim Rosskopf Date: Mon, 31 Aug 2026 17:39:59 +0200 Subject: [PATCH 2/2] test(integration): call the MCP tools over the protocol, not only the CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other test in the suite drives the CLI, which left the blind spot this session kept falling into: a fix applied in the CLI handler can leave the same operation broken as a tool, and nothing noticed. It happened three times — bw applog and bw validate both worked from the command line while their tools answered HTTP 400 and 500. These speak JSON-RPC to `erpl-adt mcp` on stdin and assert the read-only tools answer without isError: the core ADT tools, the two BW tools that were broken as tools, that results carry structuredContent as data rather than a JSON string, and that tools/list gives every tool annotations and a title. One more asserts the silent-success contract holds for tools too: adt_run_atc, adt_run_tests and adt_check_syntax must report an error for an object that does not exist, not an empty finding list. Deliberately shallow — the point is coverage of the *path*. Each tool's semantics stay with the CLI tests, which already carry them. --- test/integration_py/test_25_mcp_smoke.py | 133 +++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 test/integration_py/test_25_mcp_smoke.py diff --git a/test/integration_py/test_25_mcp_smoke.py b/test/integration_py/test_25_mcp_smoke.py new file mode 100644 index 0000000..1f90e23 --- /dev/null +++ b/test/integration_py/test_25_mcp_smoke.py @@ -0,0 +1,133 @@ +"""Call the MCP tools over the protocol, the way an agent does. + +Every other test in this suite drives the CLI. That left a blind spot: a fix +applied in the CLI handler can leave the same operation broken as a tool, and +nothing noticed. It happened three times — `bw applog` and `bw validate` both +worked from the command line while their tools returned HTTP 400 and 500, and +the defaults that made the CLI work lived only in the CLI. + +So these tests speak JSON-RPC to `erpl-adt mcp` on stdin, and assert that the +read-only tools answer without `isError`. They are deliberately shallow: the +point is coverage of the *path*, not of each tool's semantics, which the +CLI tests already carry. +""" + +import json +import subprocess + + +def mcp_call(cli, calls, timeout=600): + """Send tools/call messages to the stdio MCP server, return results by id.""" + messages = [ + {"jsonrpc": "2.0", "id": i, "method": "tools/call", + "params": {"name": name, "arguments": args}} + for i, (name, args) in enumerate(calls) + ] + cmd = [cli.binary, "--host", cli.host, "--port", str(cli.port), + "--user", cli.user, "--password", cli.password, + "--client", cli.client, "mcp"] + proc = subprocess.run(cmd, input="\n".join(json.dumps(m) for m in messages), + capture_output=True, text=True, timeout=timeout) + out = {} + for line in proc.stdout.splitlines(): + line = line.strip() + if not line.startswith("{"): + continue + msg = json.loads(line) + if "id" in msg: + out[msg["id"]] = msg + return [out.get(i) for i in range(len(calls))] + + +def tool_error(response): + """Return the error text when a tool call failed, else None.""" + if response is None: + return "no response" + if "error" in response: + return f"JSON-RPC {response['error'].get('code')}: {response['error'].get('message')}" + result = response.get("result", {}) + if result.get("isError"): + return result["content"][0]["text"][:200] + return None + + +class TestMcpToolsAnswer: + """Read-only tools must work over the protocol, not only via the CLI.""" + + def test_tools_list_is_served(self, cli): + cmd = [cli.binary, "--host", cli.host, "--port", str(cli.port), + "--user", cli.user, "--password", cli.password, + "--client", cli.client, "mcp"] + proc = subprocess.run( + cmd, input=json.dumps({"jsonrpc": "2.0", "id": 1, "method": "tools/list"}), + capture_output=True, text=True, timeout=120) + tools = json.loads(proc.stdout.strip().splitlines()[0])["result"]["tools"] + assert len(tools) > 50 + # Every tool carries the metadata a host needs to present it. + for tool in tools: + assert tool["name"] + assert tool["description"] + assert tool["inputSchema"]["type"] == "object" + assert "annotations" in tool, f"{tool['name']} has no annotations" + assert tool.get("title"), f"{tool['name']} has no title" + + def test_core_adt_tools(self, cli): + calls = [ + ("adt_discover", {}), + ("adt_search", {"query": "CL_ABAP_RANDOM", "max_results": 2}), + ("adt_read_object", {"uri": "/sap/bc/adt/oo/classes/cl_abap_random"}), + ("adt_read_source", {"uri": "/sap/bc/adt/oo/classes/cl_abap_random/source/main"}), + ("adt_read_table", {"table_name": "SFLIGHT"}), + ("adt_list_package", {"package_name": "$TMP"}), + ("adt_package_exists", {"package_name": "$TMP"}), + ("adt_list_transports", {}), + ] + failures = [f"{name}: {err}" + for (name, _), resp in zip(calls, mcp_call(cli, calls)) + if (err := tool_error(resp))] + assert not failures, "MCP tools failed:\n " + "\n ".join(failures) + + def test_tools_return_structured_content(self, cli): + """Results carry the payload as data, not only as a JSON string.""" + [resp] = mcp_call(cli, [("adt_search", {"query": "CL_ABAP_RANDOM", + "max_results": 1})]) + assert tool_error(resp) is None + assert resp is not None + result = resp["result"] + assert "content" in result + assert "structuredContent" in result, "structuredContent missing" + assert not isinstance(result["structuredContent"], str) + + def test_bw_tools_that_only_ever_worked_from_the_cli(self, cli, bw_available): + """The two that were broken as tools while the command line was fine. + + bw_application_log answered HTTP 400 "Parameter username could not be + found" and bw_validate answered HTTP 500 "Action 'validate' is not + valid", because the defaults that fixed the CLI lived in its handler. + """ + objects = mcp_call(cli, [("bw_search", {"query": "*", "object_type": "ADSO", + "max_results": 1})])[0] + assert tool_error(objects) is None + assert objects is not None + found = json.loads(objects["result"]["content"][0]["text"]) + adso = found[0]["name"] if found else None + + calls = [("bw_application_log", {}), ("bw_discover", {}), ("bw_sysinfo", {})] + if adso: + calls.append(("bw_validate", {"object_type": "ADSO", "object_name": adso})) + + failures = [f"{name}: {err}" + for (name, _), resp in zip(calls, mcp_call(cli, calls)) + if (err := tool_error(resp))] + assert not failures, "MCP tools failed:\n " + "\n ".join(failures) + + def test_a_missing_object_is_an_error_not_an_empty_result(self, cli): + """The silent-success contract holds for tools too, not just the CLI.""" + calls = [ + ("adt_run_atc", {"uri": "/sap/bc/adt/oo/classes/zzz_erpl_missing_99"}), + ("adt_run_tests", {"uri": "/sap/bc/adt/oo/classes/zzz_erpl_missing_99"}), + ("adt_check_syntax", {"uri": "/sap/bc/adt/oo/classes/zzz_erpl_missing_99"}), + ] + for (name, _), resp in zip(calls, mcp_call(cli, calls)): + assert tool_error(resp) is not None, ( + f"{name} reported success for an object that does not exist")