Copilot commented on code in PR #13549:
URL: https://github.com/apache/apisix/pull/13549#discussion_r3411239018
##########
t/plugin/mcp-bridge.t:
##########
@@ -57,3 +57,40 @@ property "command" is required
property "command" validation failed: wrong type: expected string, got number
done
property "args" validation failed: wrong type: expected array, got string
+
+
+
+=== TEST 2: stderr content with JSON special characters keeps the notification
well-formed
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local pipe = require("ngx.pipe")
+
+ -- a subprocess line that contains double quotes, a backslash and
the
+ -- "}}" sequence, all of which break naive string concatenation
+ local payload = [[Error: invalid input "x\y" }}]]
+
+ local proc = assert(pipe.spawn({"sh", "-c", [[printf '%s\n' "$1"
1>&2]], "sh", payload}))
+ proc:set_timeouts(nil, 1000, 1000)
+ local line = assert(proc:stderr_read_line())
+
+ -- build the notification the same way the plugin does
+ local msg = core.json.encode({
+ jsonrpc = "2.0",
+ method = "notifications/stderr",
+ params = {
+ content = line,
+ },
+ })
Review Comment:
This test validates that `core.json.encode` round-trips special characters,
but it doesn't actually exercise the `mcp-bridge` plugin code path that
builds/sends the `notifications/stderr` message. A future regression back to
string concatenation in the plugin would still pass this test. Consider
refactoring the plugin to expose a small helper (e.g.,
`build_stderr_notification(content)`) and asserting against that, or writing an
integration-style test that captures `server.transport:send` output.
##########
t/plugin/mcp-bridge.t:
##########
@@ -57,3 +57,40 @@ property "command" is required
property "command" validation failed: wrong type: expected string, got number
done
property "args" validation failed: wrong type: expected array, got string
+
+
+
+=== TEST 2: stderr content with JSON special characters keeps the notification
well-formed
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local pipe = require("ngx.pipe")
+
+ -- a subprocess line that contains double quotes, a backslash and
the
+ -- "}}" sequence, all of which break naive string concatenation
+ local payload = [[Error: invalid input "x\y" }}]]
+
+ local proc = assert(pipe.spawn({"sh", "-c", [[printf '%s\n' "$1"
1>&2]], "sh", payload}))
+ proc:set_timeouts(nil, 1000, 1000)
+ local line = assert(proc:stderr_read_line())
+
Review Comment:
The spawned `ngx.pipe` subprocess is never reaped. Even though it exits
quickly, not calling `proc:wait()` can leave zombies until the worker exits and
may make the test suite flaky over time.
##########
apisix/plugins/mcp-bridge.lua:
##########
@@ -109,9 +109,25 @@ local function on_connect(conf, ctx)
local line, _
line, _, stderr_partial = proc:stderr_read_line()
if line then
- local ok, err = server.transport:send(
-
'{"jsonrpc":"2.0","method":"notifications/stderr","params":{"content":"'
- .. (stderr_partial and stderr_partial .. line or
line) .. '"}}')
+ local content = stderr_partial and stderr_partial ..
line or line
+ local msg, enc_err = core.json.encode({
+ jsonrpc = "2.0",
+ method = "notifications/stderr",
+ params = {
+ content = content,
+ },
+ })
+ if not msg then
+ msg = core.json.encode({
+ jsonrpc = "2.0",
+ method = "notifications/stderr",
+ params = {
+ content = "failed to encode stderr
content: " .. (enc_err or "unknown"),
+ },
+ })
+ end
Review Comment:
If `core.json.encode` returns nil (e.g., internal encoder error), the
fallback `core.json.encode` can also return nil; in that case
`server.transport:send(msg)` will pass nil and `transport/sse.lua` will error
when concatenating. Add a last-resort non-encoding fallback to guarantee `msg`
is always a string, and consider logging the original encode error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
