add demo mcp example (remove jar before merging!!!)#2882
add demo mcp example (remove jar before merging!!!)#2882christiangoerdes wants to merge 3 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughA new example demonstrating MCP Tool Blocking with Membrane is added, comprising documentation, API gateway configuration, and startup scripts for both Windows and Unix platforms to run the MCP backend server and gateway. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
distribution/examples/mcp/membrane.sh (1)
12-15: Harden root detection by validatingstarter.jartoo.At Line 12, root detection only checks
LICENSE.txtandscripts/run-membrane.sh; addstarter.jarto avoid selecting stale/incomplete installs.Proposed improvement
- if [ -f "$dir/LICENSE.txt" ] && [ -f "$dir/scripts/run-membrane.sh" ]; then + if [ -f "$dir/LICENSE.txt" ] && [ -f "$dir/scripts/run-membrane.sh" ] && [ -f "$dir/starter.jar" ]; thenBased on learnings: When using environment variable shortcuts like
$MEMBRANE_HOMEin shell scripts that look for installation directories, validate that required files (likestarter.jarfor Membrane) exist in the candidate directory before returning it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/examples/mcp/membrane.sh` around lines 12 - 15, Root detection in membrane.sh currently only checks for LICENSE.txt and scripts/run-membrane.sh, which can yield stale or incomplete installs; update the guard that sets MEMBRANE_HOME and MEMBRANE_CALLER_DIR (and then exec run-membrane.sh) to also verify the presence of starter.jar in the candidate directory before exporting and exec’ing, so the conditional includes a test for "$dir/starter.jar" alongside the existing file checks.distribution/examples/mcp/start-backend.sh (1)
4-4: Add an explicit JAR existence check beforeexec.Line 4 currently fails with a generic Java error if the artifact is missing; a targeted check improves UX for the example.
Proposed improvement
cd "$SCRIPT_DIR" || exit 1 +if [ ! -f "./mcp-server-demo.jar" ]; then + echo "mcp-server-demo.jar not found in $SCRIPT_DIR" >&2 + exit 1 +fi exec java -jar ./mcp-server-demo.jar🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/examples/mcp/start-backend.sh` at line 4, Add a pre-flight existence check for the JAR before running the exec line: verify the file referenced by the existing exec command ("./mcp-server-demo.jar") exists and is a regular readable file, and if not print a clear error message and exit with a non-zero code; if the check passes, proceed to exec java -jar ./mcp-server-demo.jar.distribution/examples/mcp/membrane.cmd (1)
10-10: Strengthen root detection by checkingstarter.jarpresence.Line 10 currently accepts a directory with
LICENSE.txtandrun-membrane.cmdonly; includestarter.jarto avoid selecting incomplete roots.Proposed improvement
-if exist "%dir%\LICENSE.txt" if exist "%dir%\scripts\run-membrane.cmd" goto found +if exist "%dir%\LICENSE.txt" if exist "%dir%\scripts\run-membrane.cmd" if exist "%dir%\starter.jar" goto foundBased on learnings: When using environment variable shortcuts like
$MEMBRANE_HOMEin shell scripts that look for installation directories, validate that required files (likestarter.jarfor Membrane) exist in the candidate directory before returning it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/examples/mcp/membrane.cmd` at line 10, The root-detection condition in distribution/examples/mcp/membrane.cmd currently only checks for LICENSE.txt and scripts\run-membrane.cmd; update that conditional to also require starter.jar so incomplete directories aren't accepted — modify the compound IF that references "%dir%\LICENSE.txt" and "%dir%\scripts\run-membrane.cmd" to include a third existence check for "%dir%\starter.jar" (so the IF requires all three before jumping to the found label).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@distribution/examples/mcp/membrane.cmd`:
- Line 1: This batch script starts with the Windows-specific line "@echo off"
and must be committed with CRLF line endings to avoid cmd.exe parsing issues;
convert the file distribution/examples/mcp/membrane.cmd to use CRLF line endings
(or add a .gitattributes entry like "*.cmd text eol=crlf" if you want to enforce
it), then recommit so the repository stores/normalizes the file with CRLF.
In `@distribution/examples/mcp/README.md`:
- Around line 21-31: The README's startup instructions assume a local
mcp-server-demo.jar exists but don't state how to get or build it; update the
README to document producing/obtaining that JAR by adding one-line instructions
before the "Start the backend MCP server" section that reference the build or
download step (e.g., how to run the Maven/Gradle build or where to download the
artifact) and note that start-backend.sh and start-backend.cmd expect
mcp-server-demo.jar in the same directory; mention the exact artifact name
mcp-server-demo.jar and the scripts start-backend.sh / start-backend.cmd so
readers can locate the build command or download URL.
In `@distribution/examples/mcp/start-backend.cmd`:
- Line 1: The .cmd file containing the batch entry "@echo off" must use CRLF
line endings instead of LF; update start-backend.cmd to have CRLF line endings
(use your editor's line-ending conversion or unix2dos/EditorConfig) and commit
the change, and optionally add a .gitattributes rule to force CRLF for *.cmd
files to prevent future LF-only commits.
---
Nitpick comments:
In `@distribution/examples/mcp/membrane.cmd`:
- Line 10: The root-detection condition in
distribution/examples/mcp/membrane.cmd currently only checks for LICENSE.txt and
scripts\run-membrane.cmd; update that conditional to also require starter.jar so
incomplete directories aren't accepted — modify the compound IF that references
"%dir%\LICENSE.txt" and "%dir%\scripts\run-membrane.cmd" to include a third
existence check for "%dir%\starter.jar" (so the IF requires all three before
jumping to the found label).
In `@distribution/examples/mcp/membrane.sh`:
- Around line 12-15: Root detection in membrane.sh currently only checks for
LICENSE.txt and scripts/run-membrane.sh, which can yield stale or incomplete
installs; update the guard that sets MEMBRANE_HOME and MEMBRANE_CALLER_DIR (and
then exec run-membrane.sh) to also verify the presence of starter.jar in the
candidate directory before exporting and exec’ing, so the conditional includes a
test for "$dir/starter.jar" alongside the existing file checks.
In `@distribution/examples/mcp/start-backend.sh`:
- Line 4: Add a pre-flight existence check for the JAR before running the exec
line: verify the file referenced by the existing exec command
("./mcp-server-demo.jar") exists and is a regular readable file, and if not
print a clear error message and exit with a non-zero code; if the check passes,
proceed to exec java -jar ./mcp-server-demo.jar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 395556e0-044a-46d1-9857-e1265c1cae12
⛔ Files ignored due to path filters (1)
distribution/examples/mcp/mcp-server-demo.jaris excluded by!**/*.jar
📒 Files selected for processing (6)
distribution/examples/mcp/README.mddistribution/examples/mcp/apis.yamldistribution/examples/mcp/membrane.cmddistribution/examples/mcp/membrane.shdistribution/examples/mcp/start-backend.cmddistribution/examples/mcp/start-backend.sh
| @@ -0,0 +1,24 @@ | |||
| @echo off | |||
There was a problem hiding this comment.
Commit this batch file with CRLF endings.
Line 1 indicates a Windows batch script; LF-only endings can cause runtime parsing issues under cmd.exe.
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 24 LF sequences found
(E018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@distribution/examples/mcp/membrane.cmd` at line 1, This batch script starts
with the Windows-specific line "@echo off" and must be committed with CRLF line
endings to avoid cmd.exe parsing issues; convert the file
distribution/examples/mcp/membrane.cmd to use CRLF line endings (or add a
.gitattributes entry like "*.cmd text eol=crlf" if you want to enforce it), then
recommit so the repository stores/normalizes the file with CRLF.
| 2. Start the backend MCP server. | ||
|
|
||
| Linux / macOS: | ||
| ```sh | ||
| ./start-backend.sh | ||
| ``` | ||
|
|
||
| Windows: | ||
| ```bat | ||
| start-backend.cmd | ||
| ``` |
There was a problem hiding this comment.
Document how mcp-server-demo.jar is produced/obtained before startup.
Lines 21–31 instruct starting the backend directly, but scripts require a local mcp-server-demo.jar. If that jar is removed before merge, this example becomes non-runnable without an explicit build/download step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@distribution/examples/mcp/README.md` around lines 21 - 31, The README's
startup instructions assume a local mcp-server-demo.jar exists but don't state
how to get or build it; update the README to document producing/obtaining that
JAR by adding one-line instructions before the "Start the backend MCP server"
section that reference the build or download step (e.g., how to run the
Maven/Gradle build or where to download the artifact) and note that
start-backend.sh and start-backend.cmd expect mcp-server-demo.jar in the same
directory; mention the exact artifact name mcp-server-demo.jar and the scripts
start-backend.sh / start-backend.cmd so readers can locate the build command or
download URL.
| @@ -0,0 +1,4 @@ | |||
| @echo off | |||
There was a problem hiding this comment.
Use CRLF line endings for this .cmd file.
Line 1 starts a batch script that should be committed with CRLF; LF-only line endings can break label/call parsing in cmd.exe.
Suggested repo-level guard
+# .gitattributes
+*.cmd text eol=crlf🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 4 LF sequences found
(E018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@distribution/examples/mcp/start-backend.cmd` at line 1, The .cmd file
containing the batch entry "@echo off" must use CRLF line endings instead of LF;
update start-backend.cmd to have CRLF line endings (use your editor's
line-ending conversion or unix2dos/EditorConfig) and commit the change, and
optionally add a .gitattributes rule to force CRLF for *.cmd files to prevent
future LF-only commits.
1 participant
Summary by CodeRabbit
Documentation
Chores