[somtri]

What does this PR do?

Type of change: Bug fix

Fixes #2551.

When modelopt-mcp runs as a stdio MCP server, its stdin is the MCP transport. The three modelopt-launcher calls in submit_job leave stdin unset, so each child inherits that pipe. On Windows, a real MCP client calling submit_job(dry_run=True) then waits for the 60 s timeout and gets dry_run_timeout (details under Testing).

This PR passes stdin=subprocess.DEVNULL at those three calls: the Docker Popen, the Slurm run and the dry-run run. The other subprocess calls in bridge.py (git, docker, ssh, nemo, gh) are unchanged. They were not observed to hang and were not tested. ssh forwards inherited stdin unless -n is set. Whether that matters here was not checked.

Usage

No change to the arguments or result fields of any tool.

Testing

One new parametrized test, test_launcher_children_do_not_inherit_server_stdin (cases dry_run, slurm, docker), runs the real submit_job_impl and the real subprocess.run and Popen. The test process's stdin is a pipe that holds 5 bytes. A stand-in launcher, a short Python script, writes how many bytes it could read from stdin. The test expects 0.

• Without the fix: 3 failed. The child read the 5 bytes.
• With the fix: 3 passed. Removing any one of the three stdin= lines fails only that case.
• tools/mcp/tests/test_bridge.py on Windows 11: 66 passed, 3 failed. The 3 failures are test_provision_ssh_*. On the same machine they fail the same way on main without this change (63 passed, 3 failed). CI was not run.
• ruff 0.15.20 (check and format): no findings. bandit 1.7.9 (the repo pin, Python 3.12): no findings. mypy was not run.

Manual check on Windows 11, Python 3.12, mcp 1.30.0, with a stdio MCP client calling submit_job with dry_run=True on examples/smoke/nvidia_smi.yaml:

• Without the fix: dry_run_timeout after 60.0 s, in 2 of 2 runs.
• With the fix: ok: true, exit_code: 0, in under 7 s on repeat runs (4.3 s in the last three). The first run after setup was slower.

What this does not show: the cause of the hang (a pipe that nothing reads did not reproduce it, only the server's own live stdin did), Linux or macOS behavior, the live Docker and Slurm paths, or the uv run --with-editable managed-checkout path. The new test checks that the child does not read the server's stdin. It does not reproduce the hang.

Before your PR is "Ready for review"

Make sure you read and follow Contributor guidelines and your commits are signed (git commit -s -S).

Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded trust_remote_code=True, torch.load(..., weights_only=False), pickle, etc.).

• Is this change backward compatible?: ✅
• If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
• Did you write any new necessary tests?: ✅
• Did you update Changelog?: N/A (tools/mcp is not listed in CHANGELOG.rst)
• Did you get Claude approval on this PR?: N/A (/claude review has not been run on this PR)

Additional Information

An earlier attempt, #2557, added the same three stdin= arguments with tests that mock subprocess. Its author closed it without a human review. This PR adds a test that runs the real subprocess path.

Summary by CodeRabbit

• Bug Fixes
  • Docker and Slurm submissions, along with dry runs, no longer inherit input from the server’s transport. This prevents child processes from reading unintended input and helps ensure launcher behavior is consistent across these submission modes.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: NVIDIA/Model-Optimizer
• Branch: main
• File: .coderabbit.yaml
• Commit: 059588b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 63acd2b4-7af0-441c-9794-2d67f6154522

Commits

Reviewing files that changed from the base of the PR and between eeb1f94 and 2b89d33.

Files selected for processing: 2

• tools/mcp/modelopt_mcp/bridge.py
• tools/mcp/tests/test_bridge.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Walkthrough

The Docker, Slurm, and dry-run launcher subprocesses now receive DEVNULL for stdin. A parameterized regression test checks that child processes read no data from the server process’s stdin.

Changes: Launcher stdin isolation

Layer / File(s)	Summary
Set launcher subprocess stdin tools/mcp/modelopt_mcp/bridge.py	Docker, Slurm, and dry-run launcher subprocesses now receive stdin=subprocess.DEVNULL.
Test launcher stdin isolation tools/mcp/tests/test_bridge.py	A parameterized regression test checks that child processes read zero bytes from stdin for dry-run, Slurm, and Docker submissions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: chenhanyu

Merge Risk: Minimal · up to 2b89d

Launcher subprocesses no longer inherit server stdin. No merge-blocking issue was identified.

Pre-merge checks: 6

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: preventing launcher subprocesses from inheriting the MCP server's stdin.
Linked Issues check		Issue #2551 requires the three modelopt-launcher subprocesses to use stdin=subprocess.DEVNULL: Docker Popen, Slurm run, and dry-run run. The whole-PR diff adds this argument at all three cal…
Out of Scope Changes check		The PR changes only launcher stdin handling and adds a regression test for issue #2551. The test directly exercises the affected submission modes. No unrelated production behavior, public API, or resu…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Security Anti-Patterns		PASS. The PR changes only tools/mcp/modelopt_mcp/bridge.py and its test. Added code only sets stdin=subprocess.DEVNULL and adds stdin-isolation test logic. The diff adds no `torch.load(..., weight…

Full details: Linked Issues check

Explanation

Issue #2551 requires the three modelopt-launcher subprocesses to use stdin=subprocess.DEVNULL: Docker Popen, Slurm run, and dry-run run. The whole-PR diff adds this argument at all three call sites in tools/mcp/modelopt_mcp/bridge.py. The new parametrized test covers dry_run, slurm, and docker modes and asserts that the child reads zero bytes from the server stdin.

Full details: Out of Scope Changes check

Explanation

The PR changes only launcher stdin handling and adds a regression test for issue #2551. The test directly exercises the affected submission modes. No unrelated production behavior, public API, or result field changes are present in the reviewed diff.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files.

Full details: Security Anti-Patterns

Explanation

PASS. The PR changes only tools/mcp/modelopt_mcp/bridge.py and its test. Added code only sets stdin=subprocess.DEVNULL and adds stdin-isolation test logic. The diff adds no torch.load(..., weights_only=False), allow_pickle=True, hardcoded trust_remote_code=True, external-input eval/exec, # nosec comments, or dependency manifest changes. Existing # nosec comments in bridge.py are unchanged from the base revision.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[kevalmorabia97]

Thanks for the contribution! This PR now has merge conflicts with main. Could you update your branch with the latest main and fix the conflicts so we can continue the review?

We've added the waiting for feedback label while we wait for your update.

[somtri]

Merged the latest main into the branch in 2b89d33. The only conflict was the import block at the top of tools/mcp/tests/test_bridge.py, where both sides had added imports; the resolution keeps both. The diff against main is still the three stdin=subprocess.DEVNULL lines in bridge.py plus the new test. Locally the new test passes on the merged branch, and the rest of test_bridge.py gives the same results as on main.