iloveeyjafjalla opened a new pull request, #6756:
URL: https://github.com/apache/rocketmq-dashboard/pull/6756
### Which Issue(s) This PR Fixes
- Fixes #6754
### Brief Description
`rmqctl mcp config` copied `--command` into the emitted snippet verbatim, so
a relative value such as
`./dist/rmqctl` - the natural invocation when the binary is not on PATH -
produced a snippet whose
`command` only exists relative to the shell that generated it. Every MCP
client launches the server
from its own working directory, so the spawn fails and the tools silently
never appear in the client,
which is the failure mode #5916 names for a missing `--instance-id`.
- `rmqctl/cmd/mcp.go`: a `--command` value that contains a path separator is
resolved with
`filepath.Abs` before it goes into the snippet. A bare name stays
untouched, because the client
resolves it through PATH, which is what the flag's documented default
(`rmqctl`) relies on.
- The same working-directory argument was already applied to `--config`
earlier today (#6460, "The
MCP client may launch from a different working directory"); this closes
the remaining argument that
a client has to resolve itself.
- The resolution lives in a small helper next to the command, so the snippet
builder changes by one
line.
### How Did You Test This Change?
Red first, on the current `rocketmq-studio` tip, with the added tests in
place and only `mcp.go`
reverted:
```
$ cd rmqctl && go test ./cmd/ -run
TestMCPConfigResolvesCommandPathBeforeClientLaunch
--- FAIL: TestMCPConfigResolvesCommandPathBeforeClientLaunch (0.17s)
--- FAIL: ..././rmqctl (0.05s)
mcp_config_command_test.go:55: command = "./rmqctl"; want the
selected absolute path "C:\...\001\rmqctl"
--- FAIL: .../dist\rmqctl (0.06s)
mcp_config_command_test.go:55: command = "dist\rmqctl"; want the
selected absolute path "C:\...\001\dist\rmqctl"
FAIL
```
Green with the change:
```
$ cd rmqctl && go test ./cmd/ -run
"TestMCPConfigResolvesCommandPathBeforeClientLaunch|TestMCPConfigKeepsABareCommandName"
ok github.com/apache/rocketmq-dashboard/rmqctl/cmd 2.013s
$ go test ./... all packages ok
$ go vet ./... clean
$ gofmt -l cmd internal scripts
(clean)
```
The first test mirrors the config-path test next to it (`t.Chdir` to a temp
dir, run the command,
parse the snippet, resolve the emitted value from a *different* working
directory and require it to
reach the same file); the second pins that the bare default stays bare.
Live, with the binary built from this branch and the one built from the tip:
```
$ rmqctl.exe mcp config --command ./rmqctl.exe --instance-id local-broker
# before
"command": "./rmqctl.exe"
$ rmqctl.exe mcp config --command ./rmqctl.exe --instance-id local-broker
# after
"command": "C:\Users\...\verify-wt\rmqctl\rmqctl.exe"
$ rmqctl.exe mcp config --instance-id local-broker
# after, bare default
"command": "rmqctl"
```
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`fix(rmqctl): ...`)
- [x] Tests added or updated for non-trivial changes —
`mcp_config_command_test.go` covers a relative
path with `/`, a relative path with the platform separator, an
already-absolute path, and the
untouched bare default
- [x] New UI text has both Chinese and English entries under `web/src/i18n/`
— no web text changed
- [x] Architecture constraints stay green — no Java/backend change, so the
ArchUnit checks are untouched
- [x] New source files carry the ASF license header — the new test file does
- [x] Documentation touched where behaviour changed — the helper's comment
states the rule; the flag's
help text already says "executable path used by the MCP client"
Disclosed rather than left to be discovered on merge, because these touch
the same area:
- #5916 (open) also edits `mcp.go`, adding the fail-closed guard for a
missing `--instance-id` inside
the same `RunE`; this change touches the `command` field of the snippet
map and adds a helper after
the command, so the two are compatible but a rebase of either one is
likely.
- #6460 (merged today) made `--config` absolute for the same reason; this is
the sibling argument.
- #6206 / #6200 are tests over the same command surface and do not touch it.
AI assistance: the analysis, the change and the tests were produced with AI
assistance (an agent
working in this repository) and reviewed by me before submission; the
command output above is from the
actual runs.
--
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]
