Conversation
It can be used for easy integration with AI agents. It is also useful for local execution or testing purposes.
📝 WalkthroughWalkthroughIntroduces a new shell wrapper script that orchestrates running linux-mcp-server inside a Podman container. The script configures defaults for SSH keys, container image, and logs directory, creates necessary directories, constructs a Podman command with volume mounts and environment variable passthrough, then executes the container. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 (1)
scripts/linux-mcp-container.sh (1)
46-48: Consider wildcard passthrough for allLINUX_MCP_*environment variables.Currently, only three specific
LINUX_MCP_*variables are passed through (USER, KEY_PASSPHRASE, LOG_LEVEL). If future configuration options are added to the server, they will be silently ignored unless the script is updated. Consider a more flexible approach.Instead of selectively passing through env vars, iterate over all
LINUX_MCP_*prefixed variables:-# Pass through all linux-mcp-server environment variables -[[ -n "$LINUX_MCP_USER" ]] && PODMAN_ARGS+=(-e "LINUX_MCP_USER=$LINUX_MCP_USER") -[[ -n "$LINUX_MCP_KEY_PASSPHRASE" ]] && PODMAN_ARGS+=(-e "LINUX_MCP_KEY_PASSPHRASE=$LINUX_MCP_KEY_PASSPHRASE") -[[ -n "$LINUX_MCP_LOG_LEVEL" ]] && PODMAN_ARGS+=(-e "LINUX_MCP_LOG_LEVEL=$LINUX_MCP_LOG_LEVEL") +# Pass through all LINUX_MCP_* environment variables +while IFS='=' read -r name value; do + if [[ "$name" =~ ^LINUX_MCP_ ]]; then + PODMAN_ARGS+=(-e "$name=$value") + fi +done < <(compgen -e)
| set -e | ||
|
|
||
| # Defaults | ||
| SSH_KEY="${LINUX_MCP_SSH_KEY_PATH:-$HOME/.ssh/id_rsa}" |
There was a problem hiding this comment.
Validate SSH key existence before mounting.
The script defaults to $HOME/.ssh/id_rsa but never checks if it exists. Podman's mount error will be cryptic and may not make the root cause obvious to users. Adding a simple existence check improves debuggability.
Apply this diff to validate the SSH key before attempting to mount:
# Ensure logs directory exists
mkdir -p "$LOGS_DIR"
+
+# Validate SSH key exists
+if [[ ! -f "$SSH_KEY" ]]; then
+ echo "Error: SSH key not found at $SSH_KEY" >&2
+ exit 1
+fiAlso applies to: 41-41
There was a problem hiding this comment.
I agree with this comment. Adding a check for the SSH key is a good idea.
There was a problem hiding this comment.
@subpop, thank you for confirming! The SSH key validation will help users quickly identify configuration issues before the container attempts to start, making the error messages much clearer. Let me know if you need any clarification on the implementation.
🧠 Learnings used
Learnt from: CR
Repo: rhel-lightspeed/linux-mcp-server PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:00:25.960Z
Learning: Applies to src/linux_mcp_server/tools/*.py : Use `mcp.tool()`, `log_tool_call`, and `disallow_local_execution_in_containers` decorators when creating tools
| LOGS_DIR="$HOME/.local/share/linux-mcp-server/logs" | ||
|
|
||
| # Ensure logs directory exists | ||
| mkdir -p "$LOGS_DIR" |
There was a problem hiding this comment.
Logs directory permissions may restrict container write access.
The mkdir -p "$LOGS_DIR" command creates the directory with the current user's umask. If the container runs as UID 1001 (as configured), it may lack write permissions to a directory created by a different user, causing log failures.
Apply this diff to ensure the container user can write to the logs directory:
-# Ensure logs directory exists
-mkdir -p "$LOGS_DIR"
+# Ensure logs directory exists and is writable by container user
+mkdir -p "$LOGS_DIR"
+chmod 755 "$LOGS_DIR"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/linux-mcp-container.sh around line 35, the mkdir -p "$LOGS_DIR" call
can create the directory with umask-restricted permissions that prevent the
container running as UID 1001 from writing logs; after creating the directory,
change ownership to the container UID/GID and set permissive write permissions
(for example chown -R 1001:1001 "$LOGS_DIR" and chmod -R 0755 "$LOGS_DIR") so
the container user can write to the logs directory reliably.
There was a problem hiding this comment.
This is a reasonable precaution to take.
There was a problem hiding this comment.
@subpop Thank you for acknowledging the concern! Let me know if you need any help implementing the permission changes or have questions about the approach.
🧠 Learnings used
Learnt from: CR
Repo: rhel-lightspeed/linux-mcp-server PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:00:25.960Z
Learning: Applies to src/linux_mcp_server/tools/*.py : Use `mcp.tool()`, `log_tool_call`, and `disallow_local_execution_in_containers` decorators when creating tools
| # Build podman command | ||
| PODMAN_ARGS=( | ||
| run --rm -i | ||
| --userns "keep-id:uid=1001,gid=0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the script file
find . -name "linux-mcp-container.sh" -type fRepository: rhel-lightspeed/linux-mcp-server
Length of output: 108
🏁 Script executed:
# List the repo structure to understand the project layout
git ls-files | head -30Repository: rhel-lightspeed/linux-mcp-server
Length of output: 768
🏁 Script executed:
# Search for Dockerfile or podman-related files
fd -e Dockerfile -e containerfile -o -name "*container*" -o -name "*podman*" 2>/dev/null | head -20Repository: rhel-lightspeed/linux-mcp-server
Length of output: 58
🏁 Script executed:
# Examine the main script
cat -n ./scripts/linux-mcp-container.shRepository: rhel-lightspeed/linux-mcp-server
Length of output: 2078
🏁 Script executed:
# Check the main Containerfile
cat -n Containerfile | head -80Repository: rhel-lightspeed/linux-mcp-server
Length of output: 3188
🏁 Script executed:
# Check the devcontainer Containerfile
cat -n .devcontainer/ContainerfileRepository: rhel-lightspeed/linux-mcp-server
Length of output: 697
🏁 Script executed:
# Get more of the Containerfile to see user setup
cat -n Containerfile | tail -40Repository: rhel-lightspeed/linux-mcp-server
Length of output: 1557
🏁 Script executed:
# Search for useradd or RUN commands that set up the mcp user
rg "useradd|groupadd|chown" Containerfile -A 2 -B 2Repository: rhel-lightspeed/linux-mcp-server
Length of output: 225
🏁 Script executed:
# Look for any documentation about container setup or user requirements
cat -n README.md | head -100Repository: rhel-lightspeed/linux-mcp-server
Length of output: 4156
Consider making UID/GID configurable to match Containerfile's flexibility.
The hard-coded uid=1001,gid=0 matches the default ARG UID=1001 in the Containerfile and is documented in the README. However, the Containerfile allows UID to be overridden at build time. If someone rebuilds the image with a different UID, the script will silently fail with permission errors. Make these configurable via environment variables (e.g., MCP_UID and MCP_GID) with sensible defaults and document in script comments.
🤖 Prompt for AI Agents
In scripts/linux-mcp-container.sh around line 40, the --userns option hardcodes
uid=1001,gid=0 which can mismatch a rebuilt image; make UID/GID configurable by
reading MCP_UID and MCP_GID environment variables (defaults MCP_UID=1001 and
MCP_GID=0), validate or fallback to the defaults if empty/non-numeric, and
substitute those variables into the --userns argument; add a short comment above
explaining the env vars and defaults so the behavior is documented.
There was a problem hiding this comment.
I am not sure this is necessary. I doubt many people will rebuild the container image using a different UID.
There was a problem hiding this comment.
@subpop, that's a fair point. If rebuilding with a custom UID is an edge case for your users, the current hardcoded approach keeps things simpler. The defaults match the Containerfile and should work for the typical deployment scenario. I'm good with keeping it as is.
🧠 Learnings used
Learnt from: CR
Repo: rhel-lightspeed/linux-mcp-server PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:00:25.960Z
Learning: Applies to src/linux_mcp_server/tools/*.py : Use `mcp.tool()`, `log_tool_call`, and `disallow_local_execution_in_containers` decorators when creating tools
samdoran
changed the title
|
Rather than an external wrapper script that only works with podman, if this functionality is desired it should be integrated into the program directly. We could add another entry point or subcommand to do this. That would make the interface much better and we could then test the code that is running the container instead of having an external shell script. |
Thanks @samdoran ! That could be much better yes. The only thing is that the container is not part of this upstream project. It is downstream if I am not mistaken. In that case, does it make sense to modify the upstream software to run the server as a container? |
|
I think so. The container image is most definitely part of the upstream project. |
| set -e | ||
|
|
||
| # Defaults | ||
| SSH_KEY="${LINUX_MCP_SSH_KEY_PATH:-$HOME/.ssh/id_rsa}" |
There was a problem hiding this comment.
I agree with this comment. Adding a check for the SSH key is a good idea.
| # Build podman command | ||
| PODMAN_ARGS=( | ||
| run --rm -i | ||
| --userns "keep-id:uid=1001,gid=0" |
There was a problem hiding this comment.
I am not sure this is necessary. I doubt many people will rebuild the container image using a different UID.
| LOGS_DIR="$HOME/.local/share/linux-mcp-server/logs" | ||
|
|
||
| # Ensure logs directory exists | ||
| mkdir -p "$LOGS_DIR" |
There was a problem hiding this comment.
This is a reasonable precaution to take.
samdoran
left a comment
samdoran
left a comment
There was a problem hiding this comment.
I still think this should not be a wrapper script but should be another entry point or subcommand in the Python program itself.
3 participants
It can be used for easy integration with AI agents.
It is also useful for local execution or testing purposes.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.