Fix: support configurable Electron ports and free port fallback#1460
Fix: support configurable Electron ports and free port fallback#1460ramarivera wants to merge 6 commits intoMCPJam:mainfrom
Conversation
dosubot
bot
added
the
size:M
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughAdds port utilities ( 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/src/main.ts (1)
131-154:⚠️ Potential issue | 🟠 MajorHost mismatch between server binding and window URL.
In dev mode, the server binds to
"localhost"(Line 133), which on modern dual-stack systems may resolve to::1(IPv6). YetserverUrlat Line 293 is always constructed with127.0.0.1(IPv4). On systems wherelocalhostresolves exclusively to IPv6, the BrowserWindow will fail to connect.Either bind consistently to
127.0.0.1in all modes, or propagate the resolved hostname into the URL:🔧 Option A: Use the same hostname everywhere (simplest)
- const hostname = app.isPackaged ? "127.0.0.1" : "localhost"; + const hostname = "127.0.0.1";🔧 Option B: Propagate hostname into serverUrl
Return
hostnamefromstartHonoServeralongside the port, then:- const serverUrl = `http://127.0.0.1:${serverPort}`; + const serverUrl = `http://${serverHostname}:${serverPort}`;Apply similarly at Lines 329 and 335.
Also applies to: 289-293
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/bin/start.js`:
- Around line 148-163: findAvailablePort currently ignores process.env.HOST by
computing its own defaultHost; change its signature to accept a host parameter
(e.g., host, startPort, maxPortOffset = 100, verbose = false) and use that host
for isPortAvailable checks instead of deriving defaultHost, then update the call
site in main() to pass baseHost (the value that honours process.env.HOST) into
findAvailablePort so the port scan probes the same interface the server will
bind to.
In `@mcpjam-inspector/src/main.ts`:
- Around line 115-122: The port-scan loop can probe ports >65535; update the
scanning logic in the block that uses requestedPort, PORT_SCAN_LIMIT and
isPortAvailable to cap the maximum probed port to 65535 (e.g., compute a maxPort
= Math.min(requestedPort + PORT_SCAN_LIMIT, 65535)) and iterate from
requestedPort+1 up to maxPort; also handle the case where requestedPort >= 65535
by skipping the scan or returning an immediate error, and keep the log.warn
message unchanged but ensure it reports a valid fallback port from the capped
range.
🧹 Nitpick comments (3)
mcpjam-inspector/src/main.ts (1)
87-100: Consider adding a timeout to prevent indefinite hangs.If
listenneither succeeds nor errors (rare but possible with unusual network configurations), this promise will never resolve, stalling startup silently.🛡️ Proposed timeout guard
function isPortAvailable(port: number, host: string): Promise<boolean> { return new Promise((resolve) => { const server = createServer(); + const timeout = setTimeout(() => { + server.close(); + resolve(false); + }, 1000); server.listen(port, host, () => { server.close(() => { + clearTimeout(timeout); resolve(true); }); }); server.on("error", () => { + clearTimeout(timeout); resolve(false); }); }); }mcpjam-inspector/bin/start.js (2)
382-391: Dead assignment —envVars.BASE_URLis overwritten at line 690.The
PORTandBASE_URLvalues set here are unconditionally replaced after port resolution (lines 688–691). Consider removing the premature assignment to avoid confusion about where the canonical values originate.Simplify
if (parsingFlags && arg === "--port" && i + 1 < args.length) { const port = args[++i]; envVars.PORT = port; - // Default: localhost in development, 127.0.0.1 in production - const defaultHost = - process.env.ENVIRONMENT === "dev" ? "localhost" : "127.0.0.1"; - const baseHost = process.env.HOST || defaultHost; - envVars.BASE_URL = `http://${baseHost}:${port}`; continue; }
775-778: Redundant shadow ofbaseHost. These lines recompute the samedefaultHost/hostalready captured asbaseHoston line 661. Reuse it:Simplify
- const defaultHost = - process.env.ENVIRONMENT === "dev" ? "localhost" : "127.0.0.1"; - const host = process.env.HOST || defaultHost; - let url = process.env.BASE_URL || `http://${host}:${PORT}`; + let url = process.env.BASE_URL || `http://${baseHost}:${PORT}`;
There was a problem hiding this comment.
Pull request overview
This PR implements configurable Electron ports with automatic fallback to available ports when the default port is occupied. The change addresses the limitation of hardcoded port 6274, adding support for ELECTRON_PORT, SERVER_PORT, and PORT environment variables with intelligent port scanning when no explicit port is specified.
Changes:
- Added dynamic port resolution with configurable environment variables (ELECTRON_PORT, SERVER_PORT, PORT)
- Implemented port availability checking and automatic fallback scanning (up to +100 ports)
- Modified server initialization to set SERVER_PORT before importing server module, ensuring correct configuration propagation
- Updated both Electron main process (src/main.ts) and CLI startup script (bin/start.js) with consistent port handling logic
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| mcpjam-inspector/src/main.ts | Adds port resolution, validation, and scanning logic; moves server import to after port configuration |
| mcpjam-inspector/bin/start.js | Updates CLI port handling with availability checks and fallback scanning; improves port configuration flow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- respect HOST in CLI port scanning fallback - harden port probe lifecycle with timeout and listener cleanup - cap port scan at 65535 and improve fallback error handling
dosubot
bot
added
size:L
|
Implemented all non-conflicting review items from CodeRabbit/Copilot on PR #1460:
If you want, I can also resolve those threads in GitHub UI, but the implementation side is now fully in 🤖 This content was generated with AI assistance using GPT-5. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/bin/start.js`:
- Line 714: The call to findAvailablePort is hardcoded with verbose = true
causing per-port warnings to always display; change the third argument from true
to the existing verboseLogs flag so the call becomes await
findAvailablePort(requestedPort, host, verboseLogs) (affecting the resolvedPort
assignment), and ensure any related log output uses the verbose-only helpers
(verboseInfo/verboseSuccess/verboseStep) where appropriate.
- Around line 123-167: In isPortAvailable, the timeout path calls server.close()
after cleanup() which removed error listeners and can cause an unhandled
'error'; before calling server.close() in the timeout branch, attach a temporary
no-op error handler (e.g. server.once('error', noop)) to swallow any late socket
errors, then call server.close() and ensure the no-op is removed if/when onError
or onListening run (cleanup already removes listeners), mirroring the safeguard
used in main.ts to prevent unhandled 'error' events.
In `@mcpjam-inspector/src/main.ts`:
- Around line 83-127: In isPortAvailable, the timeout path calls server.close()
after cleanup which can drop the only error listener and cause an unhandled
'error' crash; modify the timeout handler so it first attaches a temporary no-op
error listener (e.g., server.once("error", noop)) before calling server.close
(and prefer closing with a callback to resolve(false)), and ensure cleanup only
removes the intended onError/onListening listeners so the no-op stays long
enough to swallow any stray error.
🧹 Nitpick comments (3)
mcpjam-inspector/src/main.ts (1)
165-167: Hostname derivation repeated four times — extract once.
app.isPackaged ? "127.0.0.1" : "localhost"appears at lines 171, 330, 367, and 401. A single helper (or a lazily-evaluated module-level value) eliminates the drift risk.Example consolidation
+function getHostname(): string { + return app.isPackaged ? "127.0.0.1" : "localhost"; +}Then replace every inline ternary with
getHostname().Also applies to: 171-171, 330-330, 367-367, 401-401
mcpjam-inspector/bin/start.js (2)
694-697: Redundant alias:hostis justbaseHost.
const host = baseHostadds a name without adding meaning. Consider usingbaseHostdirectly — or, ifhostreads better, dropbaseHost.Simplification
const defaultHost = process.env.ENVIRONMENT === "dev" ? "localhost" : "127.0.0.1"; - const baseHost = process.env.HOST || defaultHost; - const host = baseHost; + const host = process.env.HOST || defaultHost;Then replace
baseHost→hoston line 725 as well.
686-692:parsePortthrow lands outside the port-specifictry/catch.If
requestedPortCandidateis an invalid string (e.g.,--port abc),parsePortthrows at line 691, which is before thetryblock at line 699. The user gets a generic "Failed to start MCP Inspector" message instead of the richer port-specific guidance on lines 707–709. Consider moving the parse inside thetryblock or wrapping it with a friendlier catch:Suggested adjustment
+ let requestedPort; + try { + requestedPort = requestedPortCandidate + ? parsePort(requestedPortCandidate) + : 6274; + } catch (e) { + logError(`Invalid port value: ${requestedPortCandidate}`); + logInfo("Provide a valid port number (1-65535) with --port <number>"); + process.exit(1); + } - const requestedPort = requestedPortCandidate - ? parsePort(requestedPortCandidate) - : 6274;
- add no-op close-time error guards with timeout handling in port probes - route fallback port scan verbosity through verboseLogs - align parsePort integer validation checks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/bin/start.js`:
- Around line 186-212: The guard in findAvailablePort incorrectly throws when
maxPort === startPort (e.g., startPort 65535) and prevents probing that single
port; change the check to only throw when maxPort < startPort (or otherwise
ensure startPort is probed before throwing) so the loop can attempt port ==
startPort (refer to function findAvailablePort and variables maxPort and
startPort) and preserve the existing error message if no ports in the range are
available.
🧹 Nitpick comments (4)
mcpjam-inspector/bin/start.js (2)
703-706: Redundant alias:hostis justbaseHost.
const host = baseHoston line 706 adds an indirection with no semantic distinction. Consider usingbaseHostdirectly in the subsequent code.
695-701:SERVER_PORTread fromprocess.envbutPORTread fromenvVars— verify intent.Line 693 already merges
envVarsintoprocess.env, soprocess.env.PORTwould also be set at this point. The asymmetry between checkingprocess.env.SERVER_PORTandenvVars.PORTis subtle. IfPORTwere set in the environment before invocation (not via--port), it would be ignored here.If intentional, a brief comment would spare future readers the detective work.
mcpjam-inspector/src/main.ts (2)
339-341: Hostname derived identically in four places — consider a module-level helper.
app.isPackaged ? "127.0.0.1" : "localhost"appears at lines 180, 339, 376, and 410. A singlegetHostname()(or a top-levelconst) would eliminate the repetition and make a future change (e.g., honouring an env override) a one-line edit.Sketch
+const getHostname = () => (app.isPackaged ? "127.0.0.1" : "localhost"); + async function startHonoServer(): Promise<number> { try { - const hostname = app.isPackaged ? "127.0.0.1" : "localhost"; + const hostname = getHostname();Then replace all other occurrences with
getHostname().Also applies to: 376-378, 410-410
83-136: Near-identical port utilities duplicated acrossbin/start.jsandsrc/main.ts.
isPortAvailableand the scan logic are effectively copy-pasted between the two files. If these diverge over time, one will silently lag behind in bug fixes (as already happened with the off-by-one infindAvailablePort).Consider extracting a shared
port-utilsmodule importable by both entry points when convenient.Also applies to: 138-172
- change no-range check from <= to < so single-port scan at boundary is still attempted
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/bin/start.js`:
- Around line 695-710: The code treats falsy strings like "" or "0"
inconsistently: requestedPort is computed via parsePort(requestedPortCandidate)
but hasExplicitPort is based on requestedPortCandidate !== undefined; change
hasExplicitPort to rely on the parse result so parsePort is the single arbiter.
Concretely, after computing requestedPort (via
parsePort(requestedPortCandidate)), set hasExplicitPort = requestedPort !==
undefined (or !== null depending on parsePort) instead of checking
requestedPortCandidate, so invalid values cause parsePort errors and empty
strings fall through to the default behavior.
🧹 Nitpick comments (1)
mcpjam-inspector/bin/start.js (1)
703-706:hostis a redundant alias forbaseHost.Line 706 assigns
const host = baseHostwith no subsequent divergence — every usage could simply referencebaseHost. The extra binding adds a small cognitive tax for no benefit.Proposed cleanup
const defaultHost = process.env.ENVIRONMENT === "dev" ? "localhost" : "127.0.0.1"; const baseHost = process.env.HOST || defaultHost; - const host = baseHost;Then replace
hostwithbaseHostat lines 713, 723, and 819.
| const requestedPortCandidate = | ||
| process.env.SERVER_PORT !== undefined | ||
| ? process.env.SERVER_PORT | ||
| : envVars.PORT; | ||
| const requestedPort = requestedPortCandidate | ||
| ? parsePort(requestedPortCandidate) | ||
| : 6274; | ||
| let PORT; | ||
| const defaultHost = | ||
| process.env.ENVIRONMENT === "dev" ? "localhost" : "127.0.0.1"; | ||
| const baseHost = process.env.HOST || defaultHost; | ||
| const host = baseHost; | ||
|
|
||
| try { | ||
| // Check if user explicitly set a port via --port flag | ||
| const hasExplicitPort = envVars.PORT !== undefined; | ||
| const hasExplicitPort = requestedPortCandidate !== undefined; |
There was a problem hiding this comment.
Truthiness vs. identity: "0" and "" silently become port 6274 yet are treated as explicit.
Line 699 uses a truthiness test — falsy strings like "0" or "" fall through to the default 6274. But line 710 uses !== undefined, so those same values are considered explicitly requested. The result: --port 0 or SERVER_PORT="" silently checks 6274 as if the user asked for it, yielding a confusing error ("Explicitly requested port 6274 is not available") or quiet success on a port the user never intended.
Align the two checks so parsePort is the single arbiter of validity:
Proposed fix
const requestedPortCandidate =
process.env.SERVER_PORT !== undefined
? process.env.SERVER_PORT
: envVars.PORT;
- const requestedPort = requestedPortCandidate
- ? parsePort(requestedPortCandidate)
- : 6274;
+ const requestedPort =
+ requestedPortCandidate !== undefined && requestedPortCandidate !== ""
+ ? parsePort(requestedPortCandidate)
+ : 6274;
let PORT;
const defaultHost =
process.env.ENVIRONMENT === "dev" ? "localhost" : "127.0.0.1";
const baseHost = process.env.HOST || defaultHost;
const host = baseHost;
try {
// Check if user explicitly set a port via --port flag
- const hasExplicitPort = requestedPortCandidate !== undefined;
+ const hasExplicitPort =
+ requestedPortCandidate !== undefined && requestedPortCandidate !== "";This way --port 0 produces the clear "Invalid port value: 0" error from parsePort, and an empty SERVER_PORT="" falls through to the default scan — both matching user intent.
🤖 Prompt for AI Agents
In `@mcpjam-inspector/bin/start.js` around lines 695 - 710, The code treats falsy
strings like "" or "0" inconsistently: requestedPort is computed via
parsePort(requestedPortCandidate) but hasExplicitPort is based on
requestedPortCandidate !== undefined; change hasExplicitPort to rely on the
parse result so parsePort is the single arbiter. Concretely, after computing
requestedPort (via parsePort(requestedPortCandidate)), set hasExplicitPort =
requestedPort !== undefined (or !== null depending on parsePort) instead of
checking requestedPortCandidate, so invalid values cause parsePort errors and
empty strings fall through to the default behavior.
- add README guidance for testing via local checkout and npx - mirror fork testing flow in AGENTS.md testing section
This reverts commit 73a4d71.
|
Your AI(s) always have something new to say, so I will keep going with this once a human reviews it, otherwise I'm wasting my time and tokens here |
What changed
src/main.ts, remove hardcoded Electron port usage and resolve the port dynamically.ELECTRON_PORT, thenSERVER_PORT, thenPORT.SERVER_PORTbefore importingserver/appso server config consumes the selected port.bin/start.js, update startup port behavior.SERVER_PORTor CLI--port.localhostin dev,127.0.0.1otherwise).Self verification
node --check bin/start.jsandnode --check src/main.ts.@mcpjam/sdklocal alias entrypoint (sdk/dist/index.mjs) is missing in this environment.npm testandnpm run build:serverto fail independent of these port changes.🤖 This content was generated with AI assistance using GPT-5.