fix(mcp): move policy into the ADT layer, where both callers inherit it

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<HttpResponse, Error> Get(
         std::string_view path,
         const HttpHeaders& headers = {}) override;

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<HttpResponse, Error> Get(
         std::string_view path,
         const HttpHeaders& headers = {}) = 0;

src/adt/adt_session.cpp

@@ -167,6 +167,7 @@ httplib::Headers BuildRequestHeaders(
 struct AdtSession::Impl {
     std::unique_ptr<httplib::Client> client;
     std::string sap_client;
+    std::string logon_user;
     std::string accept_language;                 // resolved SAP logon language
     std::optional<std::string> csrf_token;       // ADT paths (/sap/bc/adt/)
     std::optional<std::string> 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<HttpResponse, Error> AdtSession::Get(std::string_view path,
                                             const HttpHeaders& headers) {
     auto result = impl_->DoGet(path, headers);

src/adt/bw_repo_utils.cpp

@@ -1,4 +1,5 @@
 #include <erpl_adt/adt/bw_repo_utils.hpp>
+#include <ctime>
 
 #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<std::vector<BwApplicationLogEntry>, 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<std::vector<BwApplicationLogEntry>, Error>::Err(

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()) {

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());

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<HttpResponse, Error>::Ok(
+        {200, {}, R"(<feed xmlns="http://www.w3.org/2005/Atom"/>)"}));
+
+    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<HttpResponse, Error>::Ok(
+        {200, {}, R"(<feed xmlns="http://www.w3.org/2005/Atom"/>)"}));
+
+    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);
+}

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")

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<HttpResponse, Error> 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<HttpResponse, Error> Dequeue(
         std::deque<Result<HttpResponse, Error>>& queue,
         std::string_view operation,