fix: release default web run port and align codex defaults#72
Conversation
📝 WalkthroughWalkthroughAdded OS-specific logic to detect and forcibly free the default web port before starting the Web UI; changed default model reasoning effort from "high" to "medium" across app state, startup normalization, UI labels, config template, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmdStart)
participant OS as OS (lsof / netstat / fuser / ps / taskkill)
participant Server as WebServer (createWebServer)
CLI->>CLI: resolveWebPort(port, host)
CLI->>OS: releaseRunPortIfNeeded(port, host)
alt default port path
OS->>OS: enumerate listeners (lsof/fuser/netstat)
OS->>OS: filter for managed "codexmate run"/"cli.js run" PIDs
OS->>OS: terminate matching PIDs (SIGKILL / taskkill)
OS-->>CLI: {attempted:true, released:true, pids:[...]}
else non-default port or nothing to release
OS-->>CLI: {attempted:false, released:false, pids:[], reason:'non-default-port'}
end
CLI->>Server: createWebServer(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
tests/unit/web-run-host.test.mjs (1)
300-302: Reduce fragility incmdStartordering assertion.This regex is tightly coupled to exact declaration forms (
const/let) and nearby statements, so harmless refactors can break the test. Consider narrowing the assertion to the ordering contract only.♻️ Suggested stabilization diff
test('cmdStart releases the resolved port before creating the web server', () => { - assert.match(cliContent, /const port = resolveWebPort\(\);\s*const host = resolveWebHost\(options\);\s*releaseRunPortIfNeeded\(port\);\s*let serverHandle = createWebServer\(/s); + assert.match( + cliContent, + /releaseRunPortIfNeeded\(port\);\s*(?:const|let)\s+serverHandle\s*=\s*createWebServer\(/s + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-run-host.test.mjs` around lines 300 - 302, The test's regex is too fragile because it depends on exact declaration keywords and surrounding lines; update the assertion in the 'cmdStart releases the resolved port before creating the web server' test so it only asserts ordering of resolveWebPort, releaseRunPortIfNeeded, and createWebServer. For example, replace the strict regex with either a relaxed regex that checks the three symbols appear in sequence (e.g. match resolveWebPort(...) then releaseRunPortIfNeeded(...) then createWebServer(...), allowing arbitrary text between), or compute string indices of "resolveWebPort(", "releaseRunPortIfNeeded(", and "createWebServer(" on cliContent and assert their index order (resolve < release < create); keep references to the functions cmdStart/resolveWebPort/releaseRunPortIfNeeded/createWebServer to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 280-333: The code currently calls runCommand('fuser', ['-k',
`${numericPort}/tcp`]) immediately; instead, change the non-Windows path to
first run runCommand('lsof', ['-ti', `tcp:${numericPort}`]) (use
lsofResult.stdout to populate seenPids), then verify those PIDs (skip currentPid
and check process identity where applicable via addManagedRunPidsFromPs or ps
-ef) and only then kill each PID explicitly (using process.kill or the existing
killProcess(pid, 'SIGKILL') call) and set released accordingly; update
references to fuserResult/lsofResult/seenPids/runCommand/addManagedRunPidsFromPs
so fuser -k is no longer destructive and is either removed or only used as a
last-resort after verification.
In `@web-ui/modules/app.methods.startup-claude.mjs`:
- Around line 40-43: The code sets this.modelReasoningEffort to the raw
normalized string or 'medium' only when empty, allowing invalid non-empty
values; update the assignment to validate the normalized effort against an
allowed set (e.g., ['low','medium','high']) before assigning. After computing
effort from statusRes.modelReasoningEffort (trim().toLowerCase()), check if
allowedIncludes(effort) and assign it, otherwise assign the default 'medium' to
this.modelReasoningEffort; reference the variables
statusRes.modelReasoningEffort and the property this.modelReasoningEffort in
your change.
---
Nitpick comments:
In `@tests/unit/web-run-host.test.mjs`:
- Around line 300-302: The test's regex is too fragile because it depends on
exact declaration keywords and surrounding lines; update the assertion in the
'cmdStart releases the resolved port before creating the web server' test so it
only asserts ordering of resolveWebPort, releaseRunPortIfNeeded, and
createWebServer. For example, replace the strict regex with either a relaxed
regex that checks the three symbols appear in sequence (e.g. match
resolveWebPort(...) then releaseRunPortIfNeeded(...) then createWebServer(...),
allowing arbitrary text between), or compute string indices of
"resolveWebPort(", "releaseRunPortIfNeeded(", and "createWebServer(" on
cliContent and assert their index order (resolve < release < create); keep
references to the functions
cmdStart/resolveWebPort/releaseRunPortIfNeeded/createWebServer to locate the
code under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2683965f-57c0-4011-a296-c913baf2ca42
📒 Files selected for processing (7)
cli.jstests/e2e/test-config.jstests/unit/web-run-host.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/partials/index/panel-config-codex.html
📜 Review details
🔇 Additional comments (9)
web-ui/partials/index/panel-config-codex.html (1)
81-82: Default label update is consistent with the new baseline.The selector text now correctly marks
mediumas default and no longer markshigh, matching the rest of this PR.web-ui/app.js (1)
36-36: App-state default change looks correct.Initializing
modelReasoningEfforttomediumis aligned with the updated UI and test expectations.tests/e2e/test-config.js (1)
411-414: Legacy template assertion is a good regression guard.This check correctly protects the “no explicit
model_reasoning_effortin restored legacy defaults” behavior.tests/unit/web-ui-behavior-parity.test.mjs (1)
108-108: Parity harness default update is correct.Using
mediumhere keeps baseline behavior aligned with current app startup defaults.tests/unit/web-run-host.test.mjs (5)
121-125: Good extraction hook for isolated function testing.This keeps the new
releaseRunPortIfNeededlogic testable without requiring full CLI execution context.
161-181: No-op coverage for non-default ports is solid.The assertion on both return shape and zero subprocess calls is precise and valuable.
183-207: Happy-pathfusercoverage is clear and complete.You’re validating command invocation, parsed PID result, and user-facing log output in one place.
209-247: Fallback tolsofis well validated.The test correctly proves fallback order and kill behavior when
fuseris unavailable.
249-298:psfallback test correctly protects current process.This is a strong guard against self-termination regressions while still validating managed-process cleanup.
|
@coderabbitai 如果我直接合并会带来回归性异常吗 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4240 🏁 Script executed: Length of output: 5160 🏁 Script executed: Length of output: 14795
🔍 回归风险分析✅ 低风险 / 可接受的变更
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli.js (1)
336-353:⚠️ Potential issue | 🔴 CriticalDon't promote a failed port lookup into “kill every managed run”.
When
seenPidsis still empty here, theps -effallback is no longer port-scoped. The later kill loop will then SIGKILL any othercodexmate run/cli.js runprocess it finds, including instances using a different port.psshould only validate PIDs that were already tied tonumericPort; if no port owner was identified, skip cleanup and let the normal bind failure surface.🛠️ Safer fallback
if (!(lsofResult && lsofResult.error) && candidatePids.size > 0) { const managedPsResult = readPsResult(); if (!(managedPsResult && managedPsResult.error)) { addManagedRunPidsFromPs(managedPsResult.stdout, candidatePids); } } - if (killProcess && seenPids.size === 0) { - const managedPsResult = readPsResult(); - if (!(managedPsResult && managedPsResult.error)) { - addManagedRunPidsFromPs(managedPsResult.stdout); - } - } + // `ps` output alone does not prove port ownership. If no port-scoped PID + // was found, leave cleanup to the normal EADDRINUSE path instead of + // killing unrelated `codexmate run` processes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 336 - 353, The current fallback calls readPsResult() and addManagedRunPidsFromPs(managedPsResult.stdout) when seenPids is empty, which can add PIDs not tied to numericPort and lead to killing unrelated processes; change the logic so the ps-based parsing only accepts PIDs explicitly associated with numericPort (either by enhancing addManagedRunPidsFromPs to filter by numericPort or by parsing managedPsResult.stdout here and only adding port-matching PIDs), and if no port-owner PIDs are identified leave seenPids empty and skip the kill loop entirely (so killProcess/currentPid/released logic never runs for non-port-scoped results).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 227-230: The releaseRunPortIfNeeded function currently only takes
port and may force-kill every PID listening on that port on Windows; change its
signature to accept host as well, and before invoking taskkill in the
Windows-specific branch verify the candidate PID actually corresponds to the
same listener (match the socket owner/address via existing process identity
check used elsewhere in the file) and skip killing PIDs that are bound to a
different interface or that aren’t the codexmate process; update any callers
(e.g., the places referenced around the other ranges) to pass the host through
so the function can scope cleanup correctly.
---
Duplicate comments:
In `@cli.js`:
- Around line 336-353: The current fallback calls readPsResult() and
addManagedRunPidsFromPs(managedPsResult.stdout) when seenPids is empty, which
can add PIDs not tied to numericPort and lead to killing unrelated processes;
change the logic so the ps-based parsing only accepts PIDs explicitly associated
with numericPort (either by enhancing addManagedRunPidsFromPs to filter by
numericPort or by parsing managedPsResult.stdout here and only adding
port-matching PIDs), and if no port-owner PIDs are identified leave seenPids
empty and skip the kill loop entirely (so killProcess/currentPid/released logic
never runs for non-port-scoped results).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64a6597c-4a51-44ec-88b1-8835f136359d
📒 Files selected for processing (4)
cli.jstests/unit/provider-share-command.test.mjstests/unit/web-run-host.test.mjsweb-ui/modules/app.methods.startup-claude.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/modules/app.methods.startup-claude.mjs
- tests/unit/web-run-host.test.mjs
📜 Review details
🔇 Additional comments (1)
tests/unit/provider-share-command.test.mjs (1)
760-839: Good coverage for reasoning-effort normalization paths.This validates both critical outcomes: unsupported values fallback to
'medium', while allowed'xhigh'is preserved.
|
@coderabbitai 如果我直接合并会带来回归性异常吗 |
|
Tip For best results, initiate chat on the files or code changes.
对绝大多数用户，可以安全合并，不会引发回归性异常。 唯一的边缘风险仍是
如果你的用户群不存在多实例并发场景，直接合并即可。如果存在，建议在 fallback kill 前加上端口二次验证，再合并。 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 412-440: The cleanup code uses lsof/fuser and
candidatePids/seenPids to kill by port only, ignoring the requested host, so
killProcess(currentPid) may terminate other instances bound to different
addresses; update the non-Windows path (the runCommand/lsof/fuser logic that
populates candidatePids, the call sites readPsResult and
addManagedRunPidsFromPs, and the loop that iterates seenPids before calling
killProcess) to verify each candidate's local listener address matches the
requested host (or skip if verification is impossible) before adding to seenPids
or issuing SIGKILL; i.e., after obtaining PIDs from lsof/fuser/readPsResult,
query the listener address for each PID (or use lsof output flags to include
address) and filter candidatePids so only processes bound to the same host:port
are left, otherwise do not perform cleanup.
- Around line 281-293: The isMatchingWindowsListenerAddress matcher currently
ignores wildcard listeners (listenerHost '0.0.0.0' or '::') when comparing
against a concrete normalizedHost, allowing a wildcard listener like
'0.0.0.0:3737' to be treated as non-conflicting with '--host localhost' or
'--host 127.0.0.1'; update isMatchingWindowsListenerAddress to treat wildcard
listener hosts as matching any concrete normalizedHost (and still handle the
special 'localhost'↔'127.0.0.1'/'::1' mapping), i.e. after extracting
listenerHost and ensuring normalizedHost exists, return true if listenerHost is
'0.0.0.0' or '::' (or if normalizedHost is those values keep existing behavior),
otherwise preserve the existing localhost and normalized comparisons using
extractListenerHost, normalizedHost and normalizeListenerHost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e5e5e9d-3194-44d3-850f-d973c864f4ec
📒 Files selected for processing (2)
cli.jstests/unit/web-run-host.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/web-run-host.test.mjs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (1)
cli.js (1)
11080-11080: Right place to trigger the cleanup.Doing the port-release step before
createWebServer()is the correct ordering for this flow.
| const isMatchingWindowsListenerAddress = (localAddress) => { | ||
| const listenerHost = extractListenerHost(localAddress); | ||
| if (!listenerHost || !normalizedHost) { | ||
| return false; | ||
| } | ||
| if (normalizedHost === 'localhost') { | ||
| return listenerHost === '127.0.0.1' || listenerHost === '::1'; | ||
| } | ||
| if (normalizedHost === '0.0.0.0' || normalizedHost === '::') { | ||
| return listenerHost === normalizedHost; | ||
| } | ||
| return listenerHost === normalizeListenerHost(normalizedHost); | ||
| }; |
There was a problem hiding this comment.
Match wildcard listeners against host-specific Windows starts.
This matcher never treats 0.0.0.0:3737 / [::]:3737 listeners as conflicts for host-specific starts. A stale wildcard codexmate run will therefore survive a later --host localhost, --host 127.0.0.1, or other concrete-address start even though it still owns the port, so the cleanup can still fall through to EADDRINUSE.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 281 - 293, The isMatchingWindowsListenerAddress matcher
currently ignores wildcard listeners (listenerHost '0.0.0.0' or '::') when
comparing against a concrete normalizedHost, allowing a wildcard listener like
'0.0.0.0:3737' to be treated as non-conflicting with '--host localhost' or
'--host 127.0.0.1'; update isMatchingWindowsListenerAddress to treat wildcard
listener hosts as matching any concrete normalizedHost (and still handle the
special 'localhost'↔'127.0.0.1'/'::1' mapping), i.e. after extracting
listenerHost and ensuring normalizedHost exists, return true if listenerHost is
'0.0.0.0' or '::' (or if normalizedHost is those values keep existing behavior),
otherwise preserve the existing localhost and normalized comparisons using
extractListenerHost, normalizedHost and normalizeListenerHost.
| const lsofResult = runCommand( | ||
| 'lsof', | ||
| ['-ti', `tcp:${numericPort}`], | ||
| { stdoutPidSet: candidatePids, stderrPidSet: null } | ||
| ); | ||
| const shouldTryFuser = !!(lsofResult && lsofResult.error && lsofResult.error.code === 'ENOENT'); | ||
| if (shouldTryFuser && candidatePids.size === 0) { | ||
| runCommand( | ||
| 'fuser', | ||
| [`${numericPort}/tcp`], | ||
| { stdoutPidSet: candidatePids, stderrPidSet: candidatePids } | ||
| ); | ||
| } | ||
| if (candidatePids.size > 0) { | ||
| const managedPsResult = readPsResult(); | ||
| if (!(managedPsResult && managedPsResult.error)) { | ||
| addManagedRunPidsFromPs(managedPsResult.stdout, candidatePids); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (processRef.platform !== 'win32' && killProcess && !released && seenPids.size > 0) { | ||
| for (const pid of seenPids) { | ||
| if (pid === currentPid) { | ||
| continue; | ||
| } | ||
| try { | ||
| killProcess(pid, 'SIGKILL'); | ||
| released = true; |
There was a problem hiding this comment.
Unix cleanup still kills by port instead of by requested listener.
The non-Windows path ignores host entirely: lsof/fuser collect every managed codexmate run on port 3737, then Line 433 SIGKILLs all verified matches. That can terminate another instance bound to a different specific address even when the requested --host would coexist. Filter Unix candidates by local listener address before they reach seenPids, or skip cleanup when the listener address cannot be verified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 412 - 440, The cleanup code uses lsof/fuser and
candidatePids/seenPids to kill by port only, ignoring the requested host, so
killProcess(currentPid) may terminate other instances bound to different
addresses; update the non-Windows path (the runCommand/lsof/fuser logic that
populates candidatePids, the call sites readPsResult and
addManagedRunPidsFromPs, and the loop that iterates seenPids before calling
killProcess) to verify each candidate's local listener address matches the
requested host (or skip if verification is impossible) before adding to seenPids
or issuing SIGKILL; i.e., after obtaining PIDs from lsof/fuser/readPsResult,
query the listener address for each PID (or use lsof output flags to include
address) and filter candidatePids so only processes bound to the same host:port
are left, otherwise do not perform cleanup.
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
Changes
Tests