fix: resolve MCP session count leak causing ChatGPT TimeoutError#89
fix: resolve MCP session count leak causing ChatGPT TimeoutError#89gufyhvvyfycyddy-code wants to merge 4 commits into
Conversation
Root cause: duplicate sessionRegistry.markActive(sessionId) calls for existing sessions but only one markIdle, causing inFlight counter to permanently remain >=1. After 64 sessions, new initialize sessions (the only inFlight=0 sessions) were immediately evicted, causing ChatGPT TimeoutError. Fix: Remove duplicate markActive. Single markActive per request. markIdle moved to finally block, only called when markActive was executed (activeSessionId pattern). initialize requests without markActive do not call markIdle. Add atCapacity getter and 503 response when all 64 sessions are busy. Register returns false when at capacity with no evictable idle sessions. Tests: 8 regression tests covering normal request, exception handling, 100 consecutive sessions, 64 idle eviction, 64 active rejection, full MCP flow, 70 consecutive requests, old bug documentation. Custom Windows stable build also includes: PR Waishnav#71 session cleanup/graceful shutdown, Waishnav#43 Windows drive root fix, Waishnav#41 native PowerShell, Waishnav#85 12000 char output limit, Waishnav#76 context scan ignore, Waishnav#65/Waishnav#66 AGENTS/CLAUDE realpath, Waishnav#69 ChatGPT MCP compatibility.
📝 WalkthroughWalkthroughThis change adds configurable shell execution, safe path and context handling, MCP session reservations and cleanup, advanced guarded tools, runtime diagnostics, cost tracking, inline output truncation, OAuth resource handling, and expanded lifecycle and acceptance tests. ChangesPlatform execution and configuration
Filesystem and MCP runtime
Server capabilities and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 15
🧹 Nitpick comments (5)
src/config.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated inline-output constant and parser.
DEFAULT_INLINE_OUTPUT_CHARACTERS(12000) andparseInlineOutputCharactershere mirror the definitions insrc/output-truncation.ts(per relevant snippet, same 12000 default and<100/>200000clamp). Two independent copies will drift. Prefer importing the constant/parser fromoutput-truncation.ts(or a shared module) rather than re-declaring.Also applies to: 167-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` at line 14, Remove the duplicated DEFAULT_INLINE_OUTPUT_CHARACTERS and parseInlineOutputCharacters definitions from the config flow, and import and reuse the existing constant and parser from output-truncation.ts. Update the references in the affected config logic while preserving the current default and validation behavior.src/server.ts (2)
1609-1611: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment contains corrupted (mojibake) characters.
鈥?on Line 1610 is a mangled em-dash from an encoding round-trip. Clean up the comment text for readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 1609 - 1611, Clean up the comment preceding advanced tool registration by replacing the corrupted `鈥?` characters with a readable em dash, while preserving the existing wording and dependency details.
1642-1651: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBypass path serves without Helmet security headers.
When
allowedHostsincludes"*", the app is a plainexpress()with only manual CORS headers — no Helmet defaults (X-Content-Type-Options,X-Frame-Options, etc.). Consider addinghelmet()on this branch to keep header hardening consistent with the standard app.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 1642 - 1651, Update the bypassHostValidation branch in the app initialization to apply the same Helmet security middleware as createMcpExpressApp, while preserving the existing express.json setup and manual CORS behavior. Ensure bypass requests receive the standard security headers such as X-Content-Type-Options and X-Frame-Options.Source: Linters/SAST tools
src/mcp-session-registry.ts (2)
244-252: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
closePromiseis shared betweencloseAllandshutdown.Both
closeAll()(Lines 224-228) andshutdown()reusethis.closePromise. IfcloseAll()runs first, a latershutdown()returns the already-resolvedcloseAllpromise and silently skips HTTP drain andappCleanup. The current server wiring only callsshutdown, so this is latent, but consider separate guards to avoid a future ordering bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp-session-registry.ts` around lines 244 - 252, Separate the in-progress guards used by closeAll() and shutdown() instead of sharing this.closePromise. Update shutdown() and its doShutdown flow to use a shutdown-specific promise, while preserving closeAll()’s existing promise and behavior so either method still performs its own required cleanup when invoked after the other.
188-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead
evictOldestIdle()helpersrc/mcp-session-registry.ts:188-200
register()already inlines the same oldest-idle eviction logic, and this private method is never called. Delete it or haveregister()delegate to it to avoid two copies drifting apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp-session-registry.ts` around lines 188 - 200, Remove the unused private evictOldestIdle() helper from McpSessionRegistry, since register() already contains the eviction logic; do not add another duplicate implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Around line 89-97: Update the host normalization callback in the hosts map so
bracketed IPv6 literals such as “[::1]” are preserved as the hostname before
applying port/path stripping. Keep existing handling for ordinary hostnames and
unbracketed values, ensuring normalized hosts match URL.hostname values used for
Host header comparison.
In `@src/context-ignore.ts`:
- Around line 81-84: Update the parent-traversal validation in the normalization
flow around `normalized` so it rejects any path containing a `..` segment,
including middle and trailing segments such as `a/../b`. Split the normalized
path into segments and detect an exact `..` segment, while preserving the
existing rejection record and continue behavior.
In `@src/cost-stats.ts`:
- Around line 63-91: Add a per-session removal method to CostTracker that
deletes the sessionId entry from sessionCallCount, allowing callers to clean up
when an MCP session closes. Keep record and getSessionCallCount behavior
unchanged, and ensure the new hook safely handles unknown or already-removed
session IDs.
In `@src/mcp-session-lifecycle.test.ts`:
- Around line 84-88: Update Test 1 around simulateExistingSessionRequest so the
test callback is async and awaits or returns the simulated request promise.
Ensure both inFlight assertions execute within the test’s awaited promise chain,
allowing failures to propagate to the test’s existing error handling.
In `@src/oauth-provider.ts`:
- Around line 15-29: Update isFlexibleResourceAllowed to restrict the /mcp
pathname exception to configured.origin, or an explicit trusted tunnel-origin
allowlist, instead of accepting arbitrary origins; preserve same-origin and
exact-URL checks. Apply the identical trusted-origin rule wherever src/server.ts
validates authResult.resource so token issuance and acceptance enforce the same
audience binding.
In `@src/output-truncation.ts`:
- Around line 86-97: Update the truncation sizing around markerOverhead and
available so the reserved marker and head/tail content never exceed
maxCharacters, including when maxCharacters is below markerOverhead or below the
original output length. Clamp available to the remaining requested budget
without a minimum of 100, then ensure head and tail together do not exceed
originalChars so omitted remains non-negative and headText and tailText cannot
overlap.
In `@src/pi-tools.ts`:
- Around line 123-133: Re-save src/pi-tools.ts as UTF-8 without a BOM, remove
the leading encoding marker, and replace the garbled “鈥?” sequences in the
documentation comments with proper em dashes. Preserve all code and comment
wording otherwise.
- Around line 182-195: Replace the dynamic require inside the Windows branch
with the existing spawn import used by the surrounding module. Keep the taskkill
invocation and its stdio/windowsHide options unchanged, ensuring taskkill /F /T
executes instead of falling back to child.kill("SIGKILL").
In `@src/process-platform.ts`:
- Around line 128-148: Update findBashOnWindows to perform an actual filesystem
existence check for each candidate instead of relying on the ignored spawnSync
call. Return a candidate only when the check confirms it exists, continue
through all candidates when it does not, and preserve the null fallback when
none are available.
In `@src/realpath-utils.ts`:
- Line 89: Remove the lowercase conversion from both key-generation sites in
src/realpath-utils.ts at lines 89-89 and 106-106: update the realpath-derived
keys in the relevant functions to normalize backslashes without calling
toLowerCase(), preserving distinct case-sensitive paths while retaining existing
separator normalization.
- Around line 49-64: Refactor isRealpathInsideRoots to use Node’s path.relative
for containment checks instead of lowercasing, slash replacement, and string
prefix matching. Treat a path as inside a root when the relative result is empty
or does not begin with .. and is not an absolute path, preserving correct
behavior for filesystem roots and case-sensitive filesystems; remove the
redundant drive-root special case.
In `@src/roots.ts`:
- Around line 54-68: Update assertAllowedPathRealpath to resolve the deepest
existing ancestor with existsSync, canonicalize that ancestor via
safeRealpathSync, and append the relative non-existent suffix to form the
effective real path before checking allowedRoots. Add the required existsSync
and dirname imports, use the existing relative/resolve path utilities, remove
the unsafe lexical isPathInsideRoot fallback, and return or reject based only on
containment of the effective real path.
In `@src/runtime-diagnostics.ts`:
- Around line 56-124: Replace the synchronous safeExec and checkPathCommand
calls in runDiagnose with async execFile-based helpers, preserving their
existing fallback values and timeout behavior. Make runDiagnose asynchronous and
execute npm, git, PowerShell lookup, and path availability checks concurrently
with Promise.all, while retaining the current platform-specific command set and
DiagnoseOutput fields.
In `@src/server.ts`:
- Around line 1787-1833: Protect the /devspace-runtime/diagnose,
/devspace-runtime/costs, and /devspace-runtime/smoke handlers using the same
bearer-auth middleware or guard applied to /mcp. Ensure unauthenticated requests
are rejected before runDiagnose, costTracker.getSummary, or runSmoke executes,
while preserving the existing authenticated responses.
- Around line 1868-1873: Update the resourceAllowed logic near requestedResource
so the /mcp pathname fallback only succeeds when requestedResource.origin
matches an allowed server origin, including config.publicBaseUrl and the current
dynamicPublicUrl, while preserving the existing resourceServerUrl checks. Apply
the same origin allowlist behavior in the corresponding resource validation
logic in oauth-provider.ts.
---
Nitpick comments:
In `@src/config.ts`:
- Line 14: Remove the duplicated DEFAULT_INLINE_OUTPUT_CHARACTERS and
parseInlineOutputCharacters definitions from the config flow, and import and
reuse the existing constant and parser from output-truncation.ts. Update the
references in the affected config logic while preserving the current default and
validation behavior.
In `@src/mcp-session-registry.ts`:
- Around line 244-252: Separate the in-progress guards used by closeAll() and
shutdown() instead of sharing this.closePromise. Update shutdown() and its
doShutdown flow to use a shutdown-specific promise, while preserving
closeAll()’s existing promise and behavior so either method still performs its
own required cleanup when invoked after the other.
- Around line 188-200: Remove the unused private evictOldestIdle() helper from
McpSessionRegistry, since register() already contains the eviction logic; do not
add another duplicate implementation.
In `@src/server.ts`:
- Around line 1609-1611: Clean up the comment preceding advanced tool
registration by replacing the corrupted `鈥?` characters with a readable em dash,
while preserving the existing wording and dependency details.
- Around line 1642-1651: Update the bypassHostValidation branch in the app
initialization to apply the same Helmet security middleware as
createMcpExpressApp, while preserving the existing express.json setup and manual
CORS behavior. Ensure bypass requests receive the standard security headers such
as X-Content-Type-Options and X-Frame-Options.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84b50f96-aef2-434c-8535-faee1bcdd991
📒 Files selected for processing (20)
package.jsonsrc/advanced-tools.d.tssrc/config.tssrc/context-ignore.tssrc/cost-stats.tssrc/mcp-session-lifecycle.test.tssrc/mcp-session-registry.tssrc/oauth-provider.tssrc/output-truncation.tssrc/pi-tools.tssrc/process-platform.test.tssrc/process-platform.tssrc/process-sessions.test.tssrc/process-sessions.tssrc/realpath-utils.tssrc/roots.tssrc/runtime-diagnostics.tssrc/server.tssrc/user-config.tssrc/workspaces.ts
| const cleaned = hosts.map((host) => { | ||
| const trimmed = host.trim(); | ||
| if (!trimmed) return ""; | ||
| // Strip http:// or https:// scheme | ||
| const withoutScheme = trimmed.replace(/^https?:\/\//i, ""); | ||
| // Strip port and path — keep just hostname for Host header comparison | ||
| const hostname = withoutScheme.split(/[:/]/)[0]; | ||
| return hostname || trimmed; | ||
| }).filter(Boolean); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bracketed IPv6 hosts are corrupted by the split.
withoutScheme.split(/[:/]/)[0] breaks bracketed IPv6 literals: [::1] → "[", so a Host header derived from an IPv6 publicBaseUrl (new URL(...).hostname returns [::1]) would not match. Plain ::1 survives only via the || trimmed fallback. Consider special-casing bracketed literals before stripping port/path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.ts` around lines 89 - 97, Update the host normalization callback
in the hosts map so bracketed IPv6 literals such as “[::1]” are preserved as the
hostname before applying port/path stripping. Keep existing handling for
ordinary hostnames and unbracketed values, ensuring normalized hosts match
URL.hostname values used for Host header comparison.
| if (normalized === ".." || normalized.startsWith("../")) { | ||
| rejected.push({ input, reason: "parent traversal (..) after normalization" }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix parent traversal validation bypass after normalization.
The current check only verifies if the normalized path exactly matches .. or starts with ../. It fails to reject parent traversals in the middle or end of the path (e.g., an input of a\../b normalizes to a/../b and completely bypasses this check).
Check all path segments to reliably reject any traversal.
🔒️ Proposed fix
- if (normalized === ".." || normalized.startsWith("../")) {
+ if (normalized.split("/").includes("..")) {
rejected.push({ input, reason: "parent traversal (..) after normalization" });
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (normalized === ".." || normalized.startsWith("../")) { | |
| rejected.push({ input, reason: "parent traversal (..) after normalization" }); | |
| continue; | |
| } | |
| if (normalized.split("/").includes("..")) { | |
| rejected.push({ input, reason: "parent traversal (..) after normalization" }); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context-ignore.ts` around lines 81 - 84, Update the parent-traversal
validation in the normalization flow around `normalized` so it rejects any path
containing a `..` segment, including middle and trailing segments such as
`a/../b`. Split the normalized path into segments and detect an exact `..`
segment, while preserving the existing rejection record and continue behavior.
| export class CostTracker { | ||
| private records: ToolCallRecord[] = []; | ||
| private readonly maxRecords: number; | ||
| private sessionCallCount = new Map<string, number>(); | ||
|
|
||
| constructor(maxRecords: number = DEFAULT_MAX_RECORDS) { | ||
| this.maxRecords = maxRecords; | ||
| } | ||
|
|
||
| /** Record a completed tool call. */ | ||
| record(call: Omit<ToolCallRecord, "approxTokens"> & { approxTokens?: number }): void { | ||
| const approxTokens = call.approxTokens ?? Math.round(call.returnedChars / CHARS_PER_TOKEN); | ||
| const entry: ToolCallRecord = { ...call, approxTokens }; | ||
| this.records.push(entry); | ||
| if (this.records.length > this.maxRecords) { | ||
| this.records.shift(); | ||
| } | ||
| if (call.sessionId) { | ||
| this.sessionCallCount.set( | ||
| call.sessionId, | ||
| (this.sessionCallCount.get(call.sessionId) ?? 0) + 1, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** Get the number of tool calls in a session. */ | ||
| getSessionCallCount(sessionId: string): number { | ||
| return this.sessionCallCount.get(sessionId) ?? 0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
sessionCallCount grows unbounded for the process lifetime.
Unlike records, which is capped at maxRecords, sessionCallCount (Line 66) never removes entries — clear() wipes everything, but there's no per-session removal API. Every distinct sessionId ever passed to record() (Line 80-85) stays in the map forever. Given this PR's stated purpose is fixing an MCP session leak, this is a parallel un-bounded growth path that will accumulate one entry per session for the server's uptime.
♻️ Add a removal hook so callers can clean up on session close
/** Get the number of tool calls in a session. */
getSessionCallCount(sessionId: string): number {
return this.sessionCallCount.get(sessionId) ?? 0;
}
+
+ /** Remove a session's call count (call this from onSessionClose). */
+ clearSession(sessionId: string): void {
+ this.sessionCallCount.delete(sessionId);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class CostTracker { | |
| private records: ToolCallRecord[] = []; | |
| private readonly maxRecords: number; | |
| private sessionCallCount = new Map<string, number>(); | |
| constructor(maxRecords: number = DEFAULT_MAX_RECORDS) { | |
| this.maxRecords = maxRecords; | |
| } | |
| /** Record a completed tool call. */ | |
| record(call: Omit<ToolCallRecord, "approxTokens"> & { approxTokens?: number }): void { | |
| const approxTokens = call.approxTokens ?? Math.round(call.returnedChars / CHARS_PER_TOKEN); | |
| const entry: ToolCallRecord = { ...call, approxTokens }; | |
| this.records.push(entry); | |
| if (this.records.length > this.maxRecords) { | |
| this.records.shift(); | |
| } | |
| if (call.sessionId) { | |
| this.sessionCallCount.set( | |
| call.sessionId, | |
| (this.sessionCallCount.get(call.sessionId) ?? 0) + 1, | |
| ); | |
| } | |
| } | |
| /** Get the number of tool calls in a session. */ | |
| getSessionCallCount(sessionId: string): number { | |
| return this.sessionCallCount.get(sessionId) ?? 0; | |
| } | |
| export class CostTracker { | |
| private records: ToolCallRecord[] = []; | |
| private readonly maxRecords: number; | |
| private sessionCallCount = new Map<string, number>(); | |
| constructor(maxRecords: number = DEFAULT_MAX_RECORDS) { | |
| this.maxRecords = maxRecords; | |
| } | |
| /** Record a completed tool call. */ | |
| record(call: Omit<ToolCallRecord, "approxTokens"> & { approxTokens?: number }): void { | |
| const approxTokens = call.approxTokens ?? Math.round(call.returnedChars / CHARS_PER_TOKEN); | |
| const entry: ToolCallRecord = { ...call, approxTokens }; | |
| this.records.push(entry); | |
| if (this.records.length > this.maxRecords) { | |
| this.records.shift(); | |
| } | |
| if (call.sessionId) { | |
| this.sessionCallCount.set( | |
| call.sessionId, | |
| (this.sessionCallCount.get(call.sessionId) ?? 0) + 1, | |
| ); | |
| } | |
| } | |
| /** Get the number of tool calls in a session. */ | |
| getSessionCallCount(sessionId: string): number { | |
| return this.sessionCallCount.get(sessionId) ?? 0; | |
| } | |
| /** Remove a session's call count (call this from onSessionClose). */ | |
| clearSession(sessionId: string): void { | |
| this.sessionCallCount.delete(sessionId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cost-stats.ts` around lines 63 - 91, Add a per-session removal method to
CostTracker that deletes the sessionId entry from sessionCallCount, allowing
callers to clean up when an MCP session closes. Keep record and
getSessionCallCount behavior unchanged, and ensure the new hook safely handles
unknown or already-removed session IDs.
|
|
||
| /** | ||
| * Flexible resource check 鈥?local customization. | ||
| * Accepts any URL whose pathname ends with /mcp, instead of exact checkResourceAllowed. | ||
| * This allows dynamic tunnels (Cloudflare, Tailscale) to work without reconfiguration. | ||
| */ | ||
| function isFlexibleResourceAllowed(requested: URL, configured: URL): boolean { | ||
| // Same origin: always allowed | ||
| if (requested.origin === configured.origin) return true; | ||
| // Any URL whose pathname ends with /mcp is allowed (dynamic tunnels) | ||
| if (requested.pathname.endsWith("/mcp")) return true; | ||
| // Fall back to exact check for safety | ||
| return requested.href === configured.href; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files src/oauth-provider.ts
printf '\n== outline ==\n'
ast-grep outline src/oauth-provider.ts --view expanded || true
printf '\n== relevant search ==\n'
rg -n "verifyAccessToken|mcpAuthRouter|resourceServerUrl|resource\b|issueTokens|isFlexibleResourceAllowed" src -SRepository: Waishnav/devspace
Length of output: 7565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant sections with line numbers, plus any nearby token-minting logic.
sed -n '1,320p' src/oauth-provider.ts | cat -nRepository: Waishnav/devspace
Length of output: 14317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "FILE:"; sed -n '1,320p' src/oauth-provider.ts | nl -ba | sed -n '1,320p'
echo
echo "SEARCH:"
rg -n "verifyAccessToken|mcpAuthRouter|resourceServerUrl|resource\b|issueTokens|isFlexibleResourceAllowed" src -SRepository: Waishnav/devspace
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1840,1885p' src/server.ts | cat -nRepository: Waishnav/devspace
Length of output: 2384
Restrict the /mcp exception to trusted origins
requested.pathname.endsWith("/mcp") accepts arbitrary origins, and src/server.ts applies the same rule when accepting authResult.resource. That lets tokens be minted and later accepted for any https://…/mcp resource, weakening OAuth audience binding. Require configured.origin to match or gate this on an explicit tunnel allowlist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/oauth-provider.ts` around lines 15 - 29, Update isFlexibleResourceAllowed
to restrict the /mcp pathname exception to configured.origin, or an explicit
trusted tunnel-origin allowlist, instead of accepting arbitrary origins;
preserve same-origin and exact-URL checks. Apply the identical trusted-origin
rule wherever src/server.ts validates authResult.resource so token issuance and
acceptance enforce the same audience binding.
| const result: string[] = []; | ||
| for (const p of paths) { | ||
| const real = await safeRealpath(p); | ||
| const key = real.toLowerCase().replace(/\\/g, "/"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove lowercase conversion to preserve case-sensitive paths.
On case-sensitive filesystems (like Linux), converting paths to lowercase will incorrectly deduplicate and drop distinct files (e.g., Makefile and makefile). Node's realpath natively canonicalizes the case of existing paths on case-insensitive systems (Windows/macOS), making manual .toLowerCase() conversion completely unnecessary.
src/realpath-utils.ts#L89-L89: changeconst key = real.toLowerCase().replace(/\\/g, "/");toconst key = real.replace(/\\/g, "/");src/realpath-utils.ts#L106-L106: changeconst key = real.toLowerCase().replace(/\\/g, "/");toconst key = real.replace(/\\/g, "/");
📍 Affects 1 file
src/realpath-utils.ts#L89-L89(this comment)src/realpath-utils.ts#L106-L106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/realpath-utils.ts` at line 89, Remove the lowercase conversion from both
key-generation sites in src/realpath-utils.ts at lines 89-89 and 106-106: update
the realpath-derived keys in the relevant functions to normalize backslashes
without calling toLowerCase(), preserving distinct case-sensitive paths while
retaining existing separator normalization.
| export function assertAllowedPathRealpath(path: string, allowedRoots: string[]): string { | ||
| const resolvedPath = resolve(expandHomePath(path)); | ||
| const realPath = safeRealpathSync(resolvedPath); | ||
| if (allowedRoots.some((root) => { | ||
| const realRoot = safeRealpathSync(resolve(expandHomePath(root))); | ||
| return isRealpathInsideRoots(realPath, [realRoot]); | ||
| })) { | ||
| return realPath; | ||
| } | ||
| // Fallback to non-realpath check (in case realpath fails but path is valid) | ||
| if (allowedRoots.some((root) => isPathInsideRoot(resolvedPath, root))) { | ||
| return resolvedPath; | ||
| } | ||
| throw new AccessDeniedError(`Path is outside allowed roots (realpath check): ${path}`); | ||
| } No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Critical Path Traversal / Sandbox Escape via symlinks.
The fallback logic bypasses the realpath containment check entirely. If a symlink points outside allowedRoots, the first check isRealpathInsideRoots correctly fails, but execution falls through to the non-realpath isPathInsideRoot fallback. Since the symlink's lexical path is inside allowedRoots, it returns true and permits access to the external file. This bypass also applies to non-existent files written through an external-pointing symlink.
To fix this securely, find the deepest existing ancestor of the path, resolve its realpath, and append any non-existent segments. Enforce that this effective realpath is inside the allowed roots, removing the unsafe fallback completely.
🔒️ Proposed fix
Update the imports at the top of the file to include existsSync from node:fs and dirname from node:path, then replace the function:
export function assertAllowedPathRealpath(path: string, allowedRoots: string[]): string {
const resolvedPath = resolve(expandHomePath(path));
// Find the deepest existing ancestor to prevent symlink escapes
// even for non-existent target files.
let current = resolvedPath;
while (!existsSync(current)) {
const parent = dirname(current);
if (parent === current) break;
current = parent;
}
const realPath = safeRealpathSync(current);
const nonExistentSuffix = relative(current, resolvedPath);
const finalRealPath = nonExistentSuffix ? resolve(realPath, nonExistentSuffix) : realPath;
if (allowedRoots.some((root) => {
const realRoot = safeRealpathSync(resolve(expandHomePath(root)));
return isRealpathInsideRoots(finalRealPath, [realRoot]);
})) {
return finalRealPath;
}
throw new AccessDeniedError(`Path is outside allowed roots (realpath check): ${path}`);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/roots.ts` around lines 54 - 68, Update assertAllowedPathRealpath to
resolve the deepest existing ancestor with existsSync, canonicalize that
ancestor via safeRealpathSync, and append the relative non-existent suffix to
form the effective real path before checking allowedRoots. Add the required
existsSync and dirname imports, use the existing relative/resolve path
utilities, remove the unsafe lexical isPathInsideRoot fallback, and return or
reject based only on containment of the effective real path.
| function safeExec(file: string, args: string[]): string { | ||
| try { | ||
| return execFileSync(file, args, { | ||
| encoding: "utf8", | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| }).trim(); | ||
| } catch { | ||
| return "unavailable"; | ||
| } | ||
| } | ||
|
|
||
| function checkPathCommand(cmd: string): boolean { | ||
| try { | ||
| execFileSync( | ||
| process.platform === "win32" ? "where" : "which", | ||
| [cmd], | ||
| { encoding: "utf8", timeout: 5000, windowsHide: true }, | ||
| ); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| export function runDiagnose(input: DiagnoseInput = {}): DiagnoseOutput { | ||
| const mem = process.memoryUsage(); | ||
|
|
||
| const pathCommands = ["node", "npm", "git", "rg"]; | ||
| if (process.platform === "win32") { | ||
| pathCommands.push("powershell", "taskkill"); | ||
| } | ||
|
|
||
| // Find PowerShell path | ||
| let powershellPath: string | null = null; | ||
| if (process.platform === "win32") { | ||
| try { | ||
| powershellPath = safeExec("where", ["powershell"]).split(/\r?\n/)[0] || null; | ||
| } catch { | ||
| powershellPath = null; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| node: process.version, | ||
| npm: safeExec("npm", ["--version"]), | ||
| git: safeExec("git", ["--version"]), | ||
| platform: process.platform, | ||
| arch: process.arch, | ||
| shell: input.configInfo?.shell ?? "auto", | ||
| powershellPath, | ||
| devspaceVersion: process.env.npm_package_version ?? "unknown", | ||
| processUptimeSec: Math.round(process.uptime()), | ||
| memoryUsageMB: { | ||
| rss: Math.round(mem.rss / 1024 / 1024 * 10) / 10, | ||
| heapUsed: Math.round(mem.heapUsed / 1024 / 1024 * 10) / 10, | ||
| heapTotal: Math.round(mem.heapTotal / 1024 / 1024 * 10) / 10, | ||
| external: Math.round(mem.external / 1024 / 1024 * 10) / 10, | ||
| }, | ||
| sessions: input.registrySnapshot ?? null, | ||
| costs: input.costSnapshot ?? null, | ||
| config: input.configInfo ?? null, | ||
| recentErrors: (input.recentErrors ?? []).slice(-10), | ||
| pathCheck: pathCommands.map((cmd) => ({ | ||
| command: cmd, | ||
| available: checkPathCommand(cmd), | ||
| })), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Chained synchronous execFileSync calls can block the event loop for tens of seconds.
runDiagnose performs up to ~9 sequential blocking spawns (safeExec for npm/git/powershell plus checkPathCommand for up to 6 commands), each with its own 5000ms timeout. If several commands are missing or slow (e.g. rg, taskkill, powershell on a given platform), a single diagnostics request can block Node's single event loop thread for many seconds to tens of seconds, stalling every concurrent MCP session on the server — the same class of availability risk this PR is meant to fix elsewhere.
Consider switching to async execFile/promisify(execFile) and running the checks concurrently (Promise.all) rather than serially with execFileSync.
(The static-analysis hint about child_process import enabling OS command injection is a false positive here — file/cmd args passed to execFileSync are always hardcoded literals, never derived from request input.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime-diagnostics.ts` around lines 56 - 124, Replace the synchronous
safeExec and checkPathCommand calls in runDiagnose with async execFile-based
helpers, preserving their existing fallback values and timeout behavior. Make
runDiagnose asynchronous and execute npm, git, PowerShell lookup, and path
availability checks concurrently with Promise.all, while retaining the current
platform-specific command set and DiagnoseOutput fields.
Source: Linters/SAST tools
| // Healthz endpoint with session info | ||
| app.get("/healthz", (_req, res) => { | ||
| res.json({ ok: true, name: "devspace" }); | ||
| res.json({ | ||
| ok: true, | ||
| name: "devspace", | ||
| sessions: sessionRegistry.size, | ||
| }); | ||
| }); | ||
|
|
||
| // PR #69 D: Runtime diagnostics endpoint | ||
| app.get("/devspace-runtime/diagnose", (_req, res) => { | ||
| const diag = runDiagnose({ | ||
| registrySnapshot: sessionRegistry.snapshot(), | ||
| costSnapshot: costTracker.getSummary() as unknown as Record<string, unknown>, | ||
| configInfo: { | ||
| port: config.port, | ||
| inlineOutputCharacters: config.inlineOutputCharacters, | ||
| contextIgnorePaths: config.contextIgnorePaths, | ||
| shell: config.shell, | ||
| widgets: config.widgets, | ||
| publicBaseUrl: config.publicBaseUrl, | ||
| }, | ||
| recentErrors: [], | ||
| }); | ||
| res.json(diag); | ||
| }); | ||
|
|
||
| app.get("/devspace-runtime/costs", (_req, res) => { | ||
| res.json(costTracker.getSummary()); | ||
| }); | ||
|
|
||
| app.get("/devspace-runtime/smoke", async (_req, res) => { | ||
| const result = await runSmoke([ | ||
| { | ||
| name: "healthz", | ||
| fn: async () => { | ||
| try { | ||
| const r = await fetch(`http://127.0.0.1:${config.port}/healthz`); | ||
| return { ok: r.ok, detail: `status ${r.status}` }; | ||
| } catch (e) { | ||
| return { ok: false, detail: String(e) }; | ||
| } | ||
| }, | ||
| }, | ||
| ]); | ||
| res.json(result); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Diagnostic endpoints are unauthenticated and leak internal details.
/devspace-runtime/diagnose, /devspace-runtime/costs, and /devspace-runtime/smoke are registered before the bearer-auth flow that guards /mcp, so they are reachable without credentials. diagnose exposes memory usage, resolved config (publicBaseUrl, contextIgnorePaths, PowerShell path), and tool versions; costs exposes usage records; smoke triggers a server-side fetch. Gate these behind the same auth (or bind to loopback only).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` around lines 1787 - 1833, Protect the
/devspace-runtime/diagnose, /devspace-runtime/costs, and /devspace-runtime/smoke
handlers using the same bearer-auth middleware or guard applied to /mcp. Ensure
unauthenticated requests are rejected before runDiagnose,
costTracker.getSummary, or runSmoke executes, while preserving the existing
authenticated responses.
| // Flexible resource check (local customization): accept any /mcp URL path | ||
| const requestedResource = authResult.resource; | ||
| const resourceAllowed = | ||
| requestedResource.origin === resourceServerUrl.origin || | ||
| requestedResource.pathname.endsWith("/mcp") || | ||
| requestedResource.href === resourceServerUrl.href; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
RFC 8707 OAuth resource indicator audience validation MCP
💡 Result:
The Model Context Protocol (MCP) mandates the use of RFC 8707 (Resource Indicators for OAuth 2.0) to secure authentication by binding access tokens to specific resource servers, effectively mitigating risks like token misuse and confused deputy attacks [1][2][3]. In the context of the MCP, RFC 8707 functions as follows: 1. Request-Time Signaling: MCP clients are required to include the resource parameter in both authorization and token requests [1][2][4]. This parameter carries the canonical URI of the target MCP server, signaling to the authorization server (AS) exactly which resource is being requested [2][3]. 2. Audience Binding: Upon receiving this parameter, an RFC 8707-compliant authorization server should issue access tokens that are audience-restricted [5][6]. This is typically achieved by setting the aud (audience) claim in the issued JSON Web Token (JWT) to the URI of the requested resource (the MCP server) [7][6]. 3. Audience Validation: MCP servers, acting as OAuth 2.1 resource servers, are required to validate that any token presented to them was specifically issued for their use [1][2]. This involves checking that the token's aud claim matches the server's own identifier [7][6]. If a token is intended for a different service or lacks the appropriate audience restriction, the MCP server must reject it [1][6]. This implementation creates a critical security boundary: even if an attacker attempts to influence a client to request a token for a malicious endpoint, the token will be bound to that malicious endpoint's URI and rejected by any legitimate MCP server [6][3]. Because audience validation at the receiving end is essential to the effectiveness of RFC 8707, the MCP specification explicitly requires servers to perform this check as part of their standard operations [1][2][4].
Citations:
- 1: https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization
- 2: https://modelcontextprotocol.io/specification/draft/basic/authorization
- 3: https://den.dev/blog/mcp-authorization-resource/
- 4: https://mcp.mintlify.app/specification/2025-11-25/basic/authorization
- 5: https://www.rfc-editor.org/rfc/rfc8707.html
- 6: https://workos.com/blog/mcp-resource-indicators
- 7: https://workos.com/blog/oauth-resource-indicators-rfc-8707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant symbols and inspect the surrounding code.
rg -n "resourceServerUrl|dynamicPublicUrl|requestedResource|resourceAllowed|/mcp" src/server.ts
echo
echo "---- context around the check ----"
sed -n '1838,1890p' src/server.ts
echo
echo "---- broader auth/resource setup ----"
sed -n '1760,1838p' src/server.tsRepository: Waishnav/devspace
Length of output: 6897
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where resourceServerUrl and dynamicPublicUrl are derived.
rg -n "const resourceServerUrl|dynamicPublicUrl|new URL\\(|PUBLIC_URL|resource" src/server.ts src -g '!**/*.map'
echo
echo "---- file outline ----"
ast-grep outline src/server.ts --view expanded | sed -n '1,220p'Repository: Waishnav/devspace
Length of output: 14984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the resource-allowance helper and the token issuance/verification path.
sed -n '1,360p' src/oauth-provider.ts
echo
echo "---- auth utils definition (if vendored through package) ----"
node - <<'JS'
const fs = require('fs');
const path = 'src/oauth-provider.ts';
const text = fs.readFileSync(path, 'utf8');
const hits = [...text.matchAll(/isFlexibleResourceAllowed\([^)]*\)/g)].map(m => m.index);
console.log('matches:', hits.length);
JSRepository: Waishnav/devspace
Length of output: 12972
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant OAuth provider implementation.
cat -n src/oauth-provider.ts | sed -n '1,360p'Repository: Waishnav/devspace
Length of output: 15362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the OAuth provider's resource validation and issuance flow.
cat -n src/oauth-provider.ts | sed -n '1,360p'Repository: Waishnav/devspace
Length of output: 15362
Restrict the /mcp fallback to known origins
pathname.endsWith("/mcp") accepts any host, so a token for https://attacker.example/mcp can pass this check even though it is not bound to this server. Constrain this fallback to the server’s allowed origins (for example config.publicBaseUrl and the current dynamicPublicUrl), and keep src/oauth-provider.ts aligned with the same allowlist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` around lines 1868 - 1873, Update the resourceAllowed logic
near requestedResource so the /mcp pathname fallback only succeeds when
requestedResource.origin matches an allowed server origin, including
config.publicBaseUrl and the current dynamicPublicUrl, while preserving the
existing resourceServerUrl checks. Apply the same origin allowlist behavior in
the corresponding resource validation logic in oauth-provider.ts.
…window The previous fix (markActive/markIdle pairing) left a concurrency window: multiple initialize requests could pass the atCapacity check simultaneously before any of them registered a session. Under high concurrency this could exceed maxSessions. This patch adds tryReserveSlot/commitReservation/releaseReservation to McpSessionRegistry. Capacity now counts sessions + pending reservations. tryReserveSlot is synchronous and called BEFORE transport creation, atomically evicting an idle session if needed. commitReservation converts the reservation into a real session in onsessioninitialized. releaseReservation is called in finally if the reservation was never committed (e.g. server.connect threw). Double-commit and double-release are guarded. Also adds: healthz now exposes pendingReservations/occupiedCapacity/maxSessions; DEVSPACE_TEST_BYPASS_AUTH env var for integration tests only; 8 new reservation unit tests; 6 real HTTP/MCP integration tests; 21-point acceptance test suite restored to scripts/acceptance-21.ts. Verification: 16 unit tests pass, 6 integration tests pass, 21/21 acceptance pass, 100 concurrent initialize with 0 timeout, capacity never exceeds 64.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/mcp-session-registry.ts (1)
294-318: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffIdle reservations still need an expiry path
closeIdle()only reaps sessions, whileSessionReservation.createdAtis never read for cleanup. A leaked reservation can keepoccupiedCapacitypinned indefinitely, so consider expiring stale reservations during the sweep as defense in depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp-session-registry.ts` around lines 294 - 318, Update closeIdle() to also identify and expire stale SessionReservation entries based on createdAt during the sweep, releasing their occupiedCapacity and removing them from reservation tracking. Preserve the existing idle-session cleanup and return accurate closed/evicted counts, including any reservation evictions according to the registry’s established accounting and callback behavior.scripts/acceptance-21.ts (2)
195-197: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnguarded
openWorkspace()can abort the whole suite.Unlike every other test, this call runs outside
runTest. Ifopen_workspacethrows (server down, path invalid, tool error),main()rejects and the process exits via the top-levelcatchat Line 352 — skipping all remaining tests and producing no per-test report. Wrap it so a workspace failure is recorded and downstream tests still execute (or fail) individually.♻️ Proposed change
- // Open workspace FIRST (needed for bash tool) - workspaceId = await openWorkspace(); - log(`workspaceId: ${workspaceId}`); + // Open workspace FIRST (needed for bash tool) + await runTest(0, "open workspace", async () => { + workspaceId = await openWorkspace(); + log(`workspaceId: ${workspaceId}`); + return `workspaceId=${workspaceId}`; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/acceptance-21.ts` around lines 195 - 197, Guard the workspace setup in main by handling failures from openWorkspace() outside runTest, recording the workspace error without rejecting main(), and allowing the remaining tests to execute and report individually. Preserve the existing workspaceId logging on success and ensure downstream tests receive the resulting workspace state.
167-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeveral tests can never fail, inflating the pass count.
Test 21 (Lines 167-176) swallows any fetch error and returns a success string; Test 13 (Lines 229-237) and Test 15 (Lines 273-277) contain no assertions at all. These always report
[PASS]regardless of server behavior, so "21/21 passed" overstates coverage. Add real assertions (or explicitly mark skips) so the suite reflects actual outcomes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/acceptance-21.ts` around lines 167 - 176, Update the acceptance tests so they cannot report success without validating behavior: in Test 21’s “7676 healthy throughout” callback, assert the health request succeeds and only treat an explicitly intended unavailable-server condition as a skip; add meaningful assertions to the callbacks for Test 13 and Test 15, or explicitly mark them skipped when prerequisites are absent. Ensure runTest records failures or skips rather than returning success strings for unverified outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp-integration.test.ts`:
- Line 24: Replace the hardcoded machine-specific value in SERVER_DIR with a
repository-root path derived from process.cwd() or the module location, ensuring
the spawn configuration using SERVER_DIR as cwd works consistently across
developers and CI.
---
Nitpick comments:
In `@scripts/acceptance-21.ts`:
- Around line 195-197: Guard the workspace setup in main by handling failures
from openWorkspace() outside runTest, recording the workspace error without
rejecting main(), and allowing the remaining tests to execute and report
individually. Preserve the existing workspaceId logging on success and ensure
downstream tests receive the resulting workspace state.
- Around line 167-176: Update the acceptance tests so they cannot report success
without validating behavior: in Test 21’s “7676 healthy throughout” callback,
assert the health request succeeds and only treat an explicitly intended
unavailable-server condition as a skip; add meaningful assertions to the
callbacks for Test 13 and Test 15, or explicitly mark them skipped when
prerequisites are absent. Ensure runTest records failures or skips rather than
returning success strings for unverified outcomes.
In `@src/mcp-session-registry.ts`:
- Around line 294-318: Update closeIdle() to also identify and expire stale
SessionReservation entries based on createdAt during the sweep, releasing their
occupiedCapacity and removing them from reservation tracking. Preserve the
existing idle-session cleanup and return accurate closed/evicted counts,
including any reservation evictions according to the registry’s established
accounting and callback behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 100eaa44-7505-4c9f-a4f2-d1b18a8061ce
📒 Files selected for processing (6)
package.jsonscripts/acceptance-21.tssrc/mcp-integration.test.tssrc/mcp-session-lifecycle.test.tssrc/mcp-session-registry.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server.ts
| import { join } from "node:path"; | ||
|
|
||
| const NODE = process.execPath; // Use the same node that runs the test | ||
| const SERVER_DIR = "C:\\Users\\Administrator\\.devspace\\upgrade-work\\devspace"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Hardcoded absolute machine path breaks the test everywhere except one workstation.
SERVER_DIR points at a specific Windows user directory and is used as the spawn cwd (Line 61), so this integration test cannot run in CI or on any other developer machine. Derive the repo root from process.cwd() or the module location instead.
🔧 Proposed fix
-const SERVER_DIR = "C:\\Users\\Administrator\\.devspace\\upgrade-work\\devspace";
+const SERVER_DIR = process.cwd();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SERVER_DIR = "C:\\Users\\Administrator\\.devspace\\upgrade-work\\devspace"; | |
| const SERVER_DIR = process.cwd(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp-integration.test.ts` at line 24, Replace the hardcoded
machine-specific value in SERVER_DIR with a repository-root path derived from
process.cwd() or the module location, ensuring the spawn configuration using
SERVER_DIR as cwd works consistently across developers and CI.
- Add initializing/handshakeDeadline to TrackedSession - commitReservation sets inFlight=1, initializing=true - Eviction requires inFlight===0 AND initializing===false - completeHandshake after notifications/initialized - closeStaleHandshakes for 30s timeout cleanup (configurable) - Test barrier for deterministic capacity rejection - Close failure injection for acceptance Test 8 - Strict 21-point acceptance: PASS/SKIP/FAIL, no skip-as-pass - 18 unit tests + 6 integration tests + 21 acceptance all pass
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/mcp-integration.test.ts (1)
21-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHardcoded absolute machine path still breaks the test outside one workstation.
SERVER_DIRpoints at a specific Windows user directory and is passed as the spawncwd(Line 52), so this integration test cannot run in CI or on any other developer machine. Derive the repo root fromprocess.cwd()or the module location.🔧 Proposed fix
-const SERVER_DIR = "C:\\Users\\Administrator\\.devspace\\upgrade-work\\devspace"; +const SERVER_DIR = process.cwd();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp-integration.test.ts` at line 21, Replace the hardcoded machine-specific SERVER_DIR value with a repository-root path derived from process.cwd() or the module location, and keep the existing spawn configuration in the test using SERVER_DIR as its cwd.src/mcp-session-lifecycle.test.ts (1)
72-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest 1 still does not await the simulated request; its assertions cannot fail the run.
The
simulateExistingSessionRequest(...).then(...)chain runs detached from the test body, so an assertion failure inside these callbacks becomes an unhandled rejection thattest()'s.catchnever observes, leavingprocess.exitCode = 0. Every other lifecycle test (e.g., Test 2 at Line 87) awaits the helper. Despite the earlier "addressed" marker, the code here is unchanged.💚 Proposed fix
- simulateExistingSessionRequest(registry, sessionId, async () => { - assert.strictEqual(registry.get(sessionId)?.inFlight, 1); - }).then(() => { - assert.strictEqual(registry.get(sessionId)?.inFlight, 0); - }); + await simulateExistingSessionRequest(registry, sessionId, async () => { + assert.strictEqual(registry.get(sessionId)?.inFlight, 1); + }); + assert.strictEqual(registry.get(sessionId)?.inFlight, 0);Note: ensure the Test 1 callback passed to
test(...)isasyncso theawaitis valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp-session-lifecycle.test.ts` around lines 72 - 76, Update Test 1’s callback passed to test(...) to be async, and await simulateExistingSessionRequest(...) instead of starting a detached .then(...) chain. Keep both in-flight assertions in the awaited flow so assertion failures propagate through the test runner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/mcp-integration.test.ts`:
- Line 21: Replace the hardcoded machine-specific SERVER_DIR value with a
repository-root path derived from process.cwd() or the module location, and keep
the existing spawn configuration in the test using SERVER_DIR as its cwd.
In `@src/mcp-session-lifecycle.test.ts`:
- Around line 72-76: Update Test 1’s callback passed to test(...) to be async,
and await simulateExistingSessionRequest(...) instead of starting a detached
.then(...) chain. Keep both in-flight assertions in the awaited flow so
assertion failures propagate through the test runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6927b11b-2e6d-4b1c-98a4-c6f434df594c
📒 Files selected for processing (5)
scripts/acceptance-21.tssrc/mcp-integration.test.tssrc/mcp-session-lifecycle.test.tssrc/mcp-session-registry.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/acceptance-21.ts
- src/mcp-session-registry.ts
- src/server.ts
- Convert advanced-tools.js to TypeScript for reproducible build - Add independent handshake cleanup timer (interval = min(5000, max(1000, handshakeTimeoutMs/4))) - Call closeStaleHandshakes synchronously in tryReserveSlot before capacity check - Add closeSession(id) method for deterministic close failure testing - Add /test/close-registry-session endpoint - Add request barrier (DEVSPACE_TEST_REQUEST_BARRIER=1) for deterministic in-flight test - All test endpoints require NODE_ENV=test AND DEVSPACE_TEST_BYPASS_AUTH=1 - Update outdated comments in server.ts - 21 unit tests, 7 integration tests, 21 acceptance tests all pass
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/advanced-tools.ts`:
- Around line 140-146: Harden the blocked-command matching flow around
compilePattern and assertCommandAllowed against ReDoS: bound start_job command
length in its existing schema and reject unsafe or overly complex
blockedCommandPatterns before constructing RegExp, using the project’s
established validation/error behavior. Preserve case-insensitive matching and
the existing invalid-pattern error context.
- Around line 630-655: Replace the hardcoded "master" fallback in
registerGitStateTool with detection of the workspace’s current/default branch
before calling collectGitState, while preserving explicit branch handling and
safeGitToken validation. Apply the same branch-detection behavior to the branch
fallback in git_push so both tools work with repositories using main or other
default branches.
- Around line 387-420: Update the read_many loader around readFile and
sliceLines to read only the requested line range from each file, using bounded
streaming or equivalent incremental reads instead of loading the entire file
into memory. Preserve offset/limit semantics, existing per-file error handling,
and downstream truncation via truncateText and the shared character budget.
- Around line 150-202: Add an explicit eviction operation to the store returned
by createAdvancedGuardStore, removing the workspaceId entry from entries during
workspace or session teardown. Update the AdvancedGuardStore contract and its
callers to invoke this cleanup path when a workspace closes, while preserving
existing apply, assertion, and summary behavior.
- Around line 120-138: Replace the string-based containment checks in
pathIsInside and assertInsideRoot with the existing realpath-aware path
validation helper. Ensure both functions validate the resolved target against
the real workspace root so symlinked paths cannot bypass the workspace boundary,
while preserving their current boolean and error-return behavior.
In `@src/mcp-integration.test.ts`:
- Around line 78-93: Update stopServer to determine whether the server process
is still running using serverProcess.exitCode === null &&
serverProcess.signalCode === null after the SIGTERM wait, instead of checking
serverProcess.killed, so the SIGKILL fallback executes when necessary.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1042f508-bdfb-4376-8d90-a69485ea83a6
📒 Files selected for processing (6)
scripts/acceptance-21.tssrc/advanced-tools.tssrc/mcp-integration.test.tssrc/mcp-session-lifecycle.test.tssrc/mcp-session-registry.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mcp-session-registry.ts
- scripts/acceptance-21.ts
- src/server.ts
| function normalizedPath(path: string): string { | ||
| const normalized = resolve(path).replaceAll("/", sep); | ||
| return process.platform === "win32" ? normalized.toLowerCase() : normalized; | ||
| } | ||
|
|
||
| function pathIsInside(path: string, root: string): boolean { | ||
| const relationship = relative(root, path); | ||
| return relationship === "" || (!relationship.startsWith("..") && relationship !== ".." && !resolve(relationship).startsWith(`..${sep}`)); | ||
| } | ||
|
|
||
| function assertInsideRoot(path: string, root: string, label: string): string { | ||
| const absolutePath = resolve(path); | ||
| const absoluteRoot = resolve(root); | ||
| const relationship = relative(absoluteRoot, absolutePath); | ||
| if (relationship === ".." || relationship.startsWith(`..${sep}`) || resolve(relationship).startsWith(`..${sep}`)) { | ||
| throw new Error(`${label} is outside the workspace root.`); | ||
| } | ||
| return absolutePath; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether roots.ts/realpath-utils.ts perform realpath-based containment,
# and whether advanced-tools.ts should reuse them instead of local resolve()-based checks.
fd 'realpath-utils.ts|roots.ts' src
rg -n 'realpath|lstat|fs\.realpath' src/realpath-utils.ts src/roots.ts src/workspaces.ts 2>/dev/nullRepository: Waishnav/devspace
Length of output: 1668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline src/advanced-tools.ts --view expanded || true
ast-grep outline src/roots.ts --view expanded || true
ast-grep outline src/realpath-utils.ts --view expanded || true
# Read the relevant sections with line numbers.
echo "---- src/advanced-tools.ts ----"
sed -n '1,260p' src/advanced-tools.ts | cat -n
echo "---- src/roots.ts ----"
sed -n '1,220p' src/roots.ts | cat -n
echo "---- src/realpath-utils.ts ----"
sed -n '1,220p' src/realpath-utils.ts | cat -n
# Find references to the helpers and task guardrail path enforcement.
echo "---- references ----"
rg -n 'assertInsideRoot|pathIsInside|normalizedPath|apply_task_guardrails|task guardrails|guardrails|realpath' srcRepository: Waishnav/devspace
Length of output: 25887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect workspace root handling and any canonicalization before advanced tools get the root/path values.
echo "---- src/workspaces.ts ----"
sed -n '1,260p' src/workspaces.ts | cat -n
echo "---- root-related references ----"
rg -n 'safeRealpathSync|realpath|resolveAllowedPath|assertAllowedPathRealpath|isPathInsideRoot|allowedRoots|root:' src/workspaces.ts src/server.ts src/advanced-tools.ts src/roots.tsRepository: Waishnav/devspace
Length of output: 14756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tool handlers that rely on the guard store.
sed -n '430,760p' src/advanced-tools.ts | cat -n
echo "---- assertPathAllowed call sites ----"
rg -n 'assertPathAllowed\(|assertReadAllowed\(|apply_task_guardrails|protectedPaths|workspace.root' src/advanced-tools.ts src/server.ts src/workspaces.tsRepository: Waishnav/devspace
Length of output: 21814
Use realpath-based containment here resolve()/relative() only compare strings, so a symlinked workspace root or protected path can satisfy this guardrail while the actual target lies outside the intended root. Reuse the existing realpath-aware path validation instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/advanced-tools.ts` around lines 120 - 138, Replace the string-based
containment checks in pathIsInside and assertInsideRoot with the existing
realpath-aware path validation helper. Ensure both functions validate the
resolved target against the real workspace root so symlinked paths cannot bypass
the workspace boundary, while preserving their current boolean and error-return
behavior.
| function compilePattern(pattern: string): RegExp { | ||
| try { | ||
| return new RegExp(pattern, "i"); | ||
| } catch (error) { | ||
| throw new Error(`Invalid blocked command pattern: ${pattern}. ${error instanceof Error ? error.message : String(error)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
User-supplied regex patterns create a ReDoS vector.
compilePattern compiles blockedCommandPatterns strings directly into RegExp, and assertCommandAllowed runs .test() against every start_job command (itself unbounded — z.string().min(1) has no max()). A crafted pattern with catastrophic backtracking tested against an adversarial command can hang the single-threaded event loop, freezing the entire server (all sessions) for the duration of the match — not just the calling workspace. This matches the static-analysis ReDoS warning.
🛡️ Suggested mitigation
-function compilePattern(pattern: string): RegExp {
+const MAX_PATTERN_LENGTH = 200;
+
+function compilePattern(pattern: string): RegExp {
+ if (pattern.length > MAX_PATTERN_LENGTH) {
+ throw new Error(`Blocked command pattern exceeds ${MAX_PATTERN_LENGTH} characters: ${pattern}`);
+ }
try {
return new RegExp(pattern, "i");
} catch (error) {
throw new Error(`Invalid blocked command pattern: ${pattern}. ${error instanceof Error ? error.message : String(error)}`);
}
}Consider also bounding command length (z.string().min(1).max(...)) and/or validating pattern complexity with a package such as safe-regex before compiling.
Also applies to: 186-193
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 141-141: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 141-141: Do not use variable for regular expressions
Context: new RegExp(pattern, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/advanced-tools.ts` around lines 140 - 146, Harden the blocked-command
matching flow around compilePattern and assertCommandAllowed against ReDoS:
bound start_job command length in its existing schema and reject unsafe or
overly complex blockedCommandPatterns before constructing RegExp, using the
project’s established validation/error behavior. Preserve case-insensitive
matching and the existing invalid-pattern error context.
Source: Linters/SAST tools
| export function createAdvancedGuardStore(): AdvancedGuardStore { | ||
| const entries = new Map<string, GuardEntry>(); | ||
| return { | ||
| apply(workspaceId: string, root: string, input: unknown = {}) { | ||
| const inputObj = input as { protectedPaths?: string[]; blockedCommandPatterns?: string[] }; | ||
| const absoluteRoot = resolve(root); | ||
| const protectedPaths = [...new Set(inputObj.protectedPaths ?? [])].map((path) => { | ||
| const absolutePath = assertInsideRoot(resolve(absoluteRoot, path), absoluteRoot, `Protected path ${path}`); | ||
| return { | ||
| input: path, | ||
| absolutePath, | ||
| normalized: normalizedPath(absolutePath), | ||
| }; | ||
| }); | ||
| const blockedCommandPatterns = [...new Set(inputObj.blockedCommandPatterns ?? [])].map((pattern) => ({ | ||
| input: pattern, | ||
| regex: compilePattern(pattern), | ||
| })); | ||
| const entry: GuardEntry = { root: absoluteRoot, protectedPaths, blockedCommandPatterns }; | ||
| entries.set(workspaceId, entry); | ||
| return { | ||
| protectedPaths: protectedPaths.map((item) => item.input), | ||
| blockedCommandPatterns: blockedCommandPatterns.map((item) => item.input), | ||
| }; | ||
| }, | ||
| assertPathAllowed(workspaceId: string, path: string) { | ||
| const entry = entries.get(workspaceId); | ||
| if (!entry) return; | ||
| const candidate = normalizedPath(assertInsideRoot(path, entry.root, "Path")); | ||
| const blocked = entry.protectedPaths.find((item) => { | ||
| return candidate === item.normalized || candidate.startsWith(`${item.normalized}${sep}`); | ||
| }); | ||
| if (blocked) { | ||
| throw new Error(`Path is protected by active task guardrails: ${blocked.input}`); | ||
| } | ||
| }, | ||
| assertCommandAllowed(workspaceId: string, command: string) { | ||
| const entry = entries.get(workspaceId); | ||
| if (!entry) return; | ||
| const blocked = entry.blockedCommandPatterns.find((item) => item.regex.test(command)); | ||
| if (blocked) { | ||
| throw new Error(`Command is blocked by active task guardrails: ${blocked.input}`); | ||
| } | ||
| }, | ||
| summary(workspaceId: string) { | ||
| const entry = entries.get(workspaceId); | ||
| return { | ||
| protectedPaths: entry?.protectedPaths.map((item) => item.input) ?? [], | ||
| blockedCommandPatterns: entry?.blockedCommandPatterns.map((item) => item.input) ?? [], | ||
| }; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'createAdvancedGuardStore|advancedGuards\.' src/server.tsRepository: Waishnav/devspace
Length of output: 328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and find all references to advancedGuards and guard-store cleanup paths.
ast-grep outline src/server.ts --view expanded | sed -n '1,220p'
printf '\n--- references ---\n'
rg -n 'advancedGuards|createAdvancedGuardStore|assertPathAllowed|assertCommandAllowed|summary\(' src/server.ts src/advanced-tools.ts
printf '\n--- nearby server code around the store creation and teardown paths ---\n'
sed -n '1600,1765p' src/server.ts
printf '\n--- advanced-tools file around the store implementation ---\n'
sed -n '1,260p' src/advanced-tools.tsRepository: Waishnav/devspace
Length of output: 19974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- advanced tools references to workspaceId and guard application ---'
rg -n 'apply_task_guardrails|workspaceId|guards\.(apply|summary|assertPathAllowed|assertCommandAllowed)' src/advanced-tools.ts src/server.ts
printf '\n%s\n' '--- relevant sections in advanced-tools.ts ---'
sed -n '480,760p' src/advanced-tools.ts
printf '\n%s\n' '--- session/workspace lifecycle references in src/server.ts ---'
rg -n 'onSessionClose|onClose|workspace.*close|delete\\(|remove\\(|clear\\(' src/server.ts src/workspaces.ts src/process-sessions.tsRepository: Waishnav/devspace
Length of output: 26703
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workspace registry lifecycle ---'
ast-grep outline src/workspaces.ts --view expanded | sed -n '1,220p'
printf '\n%s\n' '--- searches in workspaces.ts ---'
rg -n 'delete\\(|remove\\(|clear\\(|close\\(|evict\\(|dispose\\(|on.*Close|workspaceId|Map<' src/workspaces.ts
printf '\n%s\n' '--- process session lifecycle ---'
ast-grep outline src/process-sessions.ts --view expanded | sed -n '1,240p'
printf '\n%s\n' '--- searches in process-sessions.ts ---'
rg -n 'delete\\(|remove\\(|clear\\(|close\\(|evict\\(|dispose\\(|on.*Close|sessionId|Map<' src/process-sessions.ts
printf '\n%s\n' '--- advanced guard references in server.ts ---'
rg -n 'advancedGuards|createAdvancedGuardStore|guards:' src/server.tsRepository: Waishnav/devspace
Length of output: 2979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/workspaces.ts: methods around workspace registry ---'
sed -n '72,220p' src/workspaces.ts
printf '\n%s\n' '--- src/workspaces.ts: any cleanup-related identifiers ---'
rg -n 'delete|remove|clear|close|evict|dispose|on[A-Z].*Close|on[A-Z].*Remove|on[A-Z].*Delete' src/workspaces.ts
printf '\n%s\n' '--- src/process-sessions.ts: methods around session manager ---'
sed -n '1,260p' src/process-sessions.ts
printf '\n%s\n' '--- src/process-sessions.ts: any cleanup-related identifiers ---'
rg -n 'delete|remove|clear|close|evict|dispose|on[A-Z].*Close|on[A-Z].*Remove|on[A-Z].*Delete' src/process-sessions.ts
printf '\n%s\n' '--- src/server.ts: advanced guard references and shutdown hooks ---'
rg -n 'advancedGuards|createAdvancedGuardStore|onSessionClose|onShutdown|close\\(|dispose\\(' src/server.tsRepository: Waishnav/devspace
Length of output: 4957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- remainder of src/workspaces.ts around createWorkspaceContext and any session removal APIs ---'
sed -n '220,420p' src/workspaces.ts
printf '\n%s\n' '--- src/process-sessions.ts full API surface around lifecycle ---'
sed -n '1,320p' src/process-sessions.ts
printf '\n%s\n' '--- focused lifecycle search in server.ts, workspaces.ts, process-sessions.ts ---'
rg -n 'createSession|getSession|touchSession|closeSession|deleteSession|removeSession|evictSession|clearSessions|onSessionClose|onSweep|terminate\\(' src/server.ts src/workspaces.ts src/process-sessions.tsRepository: Waishnav/devspace
Length of output: 14450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- advanced guard references across the repo ---'
rg -n 'advancedGuards|createAdvancedGuardStore|assertPathAllowed|assertCommandAllowed|summary\(' src
printf '\n%s\n' '--- server close/shutdown wiring ---'
sed -n '1627,1725p' src/server.ts
sed -n '1725,1815p' src/server.ts
sed -n '1815,1915p' src/server.ts
sed -n '1915,2015p' src/server.ts
sed -n '2015,2105p' src/server.ts
printf '\n%s\n' '--- workspace store shutdown/cleanup references ---'
rg -n 'shutdown\(|close\(|dispose\(|clear\(|removeSession|deleteSession|evictSession|onSessionClose|onSweep' src/workspaces.ts src/process-sessions.ts src/server.tsRepository: Waishnav/devspace
Length of output: 21598
Add an eviction path for guard entries
entries.set(workspaceId, entry) in src/advanced-tools.ts only grows the map; there’s no removal path on workspace/session teardown, so stale guardrail state can accumulate for the life of the server across many workspaces. Clear the entry when a workspace closes or add a TTL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/advanced-tools.ts` around lines 150 - 202, Add an explicit eviction
operation to the store returned by createAdvancedGuardStore, removing the
workspaceId entry from entries during workspace or session teardown. Update the
AdvancedGuardStore contract and its callers to invoke this cleanup path when a
workspace closes, while preserving existing apply, assertion, and summary
behavior.
| }, async ({ workspaceId, files, maxTotalCharacters }: { | ||
| workspaceId: string; | ||
| files: { path: string; offset?: number; limit?: number }[]; | ||
| maxTotalCharacters?: number; | ||
| }) => { | ||
| const workspace = workspaces.getWorkspace(workspaceId) as Workspace; | ||
| const budget = maxTotalCharacters ?? DEFAULT_READ_BUDGET; | ||
| let remaining = budget; | ||
| let anyTruncated = false; | ||
| const loaded = await Promise.all(files.map(async (file) => { | ||
| try { | ||
| const readPath = workspaces.resolveReadPath(workspace, file.path) as WorkspaceReadPath; | ||
| const raw = await readFile(readPath.absolutePath, "utf8"); | ||
| workspaces.markReadPathLoaded(workspace, readPath); | ||
| return { | ||
| path: file.path, | ||
| selected: sliceLines(raw, file.offset ?? 1, file.limit), | ||
| }; | ||
| } catch (error) { | ||
| return { path: file.path, error: error instanceof Error ? error.message : String(error) }; | ||
| } | ||
| })); | ||
| const outputs = loaded.map((file) => { | ||
| if ("error" in file && file.error) return { path: file.path, error: file.error, truncated: false }; | ||
| const selected = (file as { path: string; selected: string }).selected; | ||
| const clipped = truncateText(selected, Math.max(0, remaining)); | ||
| remaining = Math.max(0, remaining - clipped.text.length); | ||
| anyTruncated ||= clipped.truncated; | ||
| return { path: file.path, content: clipped.text, truncated: clipped.truncated }; | ||
| }); | ||
| const totalCharacters = outputs.reduce((sum, file) => sum + (file.content?.length ?? 0), 0); | ||
| const result = `Read ${outputs.filter((file) => !file.error).length}/${outputs.length} files (${totalCharacters} characters).`; | ||
| return textResult(result, { result, files: outputs, totalCharacters, truncated: anyTruncated }); | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
read_many loads entire files into memory before applying offset/limit.
readFile(readPath.absolutePath, "utf8") reads the full file content regardless of the requested offset/limit, then sliceLines trims it afterward. With up to MAX_READ_FILES (50) files per call and no per-file size bound, requesting a small line slice of a very large file still forces the whole file into memory, risking process-wide OOM (single Node process serving all sessions) even though the tool's purpose is a bounded, efficient multi-file read.
♻️ Suggested guard
+import { stat } from "node:fs/promises";
+const MAX_READ_FILE_BYTES = 5 * 1024 * 1024;
...
try {
const readPath = workspaces.resolveReadPath(workspace, file.path) as WorkspaceReadPath;
+ const fileStat = await stat(readPath.absolutePath);
+ if (fileStat.size > MAX_READ_FILE_BYTES) {
+ throw new Error(`File exceeds ${MAX_READ_FILE_BYTES} bytes; use offset/limit with a smaller range or start_job to grep it.`);
+ }
const raw = await readFile(readPath.absolutePath, "utf8");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/advanced-tools.ts` around lines 387 - 420, Update the read_many loader
around readFile and sliceLines to read only the requested line range from each
file, using bounded streaming or equivalent incremental reads instead of loading
the entire file into memory. Preserve offset/limit semantics, existing per-file
error handling, and downstream truncation via truncateText and the shared
character budget.
| const registerGitStateTool = (name: string, title: string, description: string) => { | ||
| registerTool(server, name, { | ||
| title, | ||
| description, | ||
| inputSchema: { | ||
| workspaceId: z.string(), | ||
| remote: z.string().optional(), | ||
| branch: z.string().optional(), | ||
| }, | ||
| outputSchema: gitStateSchema, | ||
| annotations: readOnly, | ||
| }, async ({ workspaceId, remote, branch }: { | ||
| workspaceId: string; | ||
| remote?: string; | ||
| branch?: string; | ||
| }) => { | ||
| const workspace = workspaces.getWorkspace(workspaceId) as Workspace; | ||
| const remoteName = safeGitToken(remote ?? "origin", "remote"); | ||
| const branchName = safeGitToken(branch ?? "master", "branch"); | ||
| const state = await collectGitState(workspace.root, remoteName, branchName); | ||
| const result = state.aligned | ||
| ? `HEAD, ${remoteName}/${branchName}, and remote ${branchName} are aligned at ${state.head}.` | ||
| : `Git refs are not aligned. HEAD=${state.head}, ${remoteName}/${branchName}=${state.origin}, remote=${state.remote || "missing"}.`; | ||
| return textResult([result, state.status ? `Status:\n${state.status}` : "Working tree is clean."].join("\n"), { result, ...state }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Default branch of "master" will break git_preflight/git_postflight on repos using main.
When branch isn't supplied, safeGitToken(branch ?? "master", "branch") defaults to master. collectGitState then runs git rev-parse origin/master and git ls-remote origin refs/heads/master, both of which fail with a non-zero exit for repos whose default branch is main (the common default since 2020) — runGit throws via assertExecutableSuccess, so the tool errors out entirely instead of reporting state. The same hardcoded "master" fallback is used in git_push (line 731).
🐛 Suggested fix — detect current branch instead of hardcoding
- }, async ({ workspaceId, remote, branch }: {
+ }, async ({ workspaceId, remote, branch }: {
workspaceId: string;
remote?: string;
branch?: string;
}) => {
const workspace = workspaces.getWorkspace(workspaceId) as Workspace;
const remoteName = safeGitToken(remote ?? "origin", "remote");
- const branchName = safeGitToken(branch ?? "master", "branch");
+ const branchName = safeGitToken(
+ branch ?? (await runGit(workspace.root, ["branch", "--show-current"], "git branch --show-current")).stdout.trim() || "main",
+ "branch",
+ );Apply the equivalent change to the branch ?? "master" fallback in git_push (line 731).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/advanced-tools.ts` around lines 630 - 655, Replace the hardcoded "master"
fallback in registerGitStateTool with detection of the workspace’s
current/default branch before calling collectGitState, while preserving explicit
branch handling and safeGitToken validation. Apply the same branch-detection
behavior to the branch fallback in git_push so both tools work with repositories
using main or other default branches.
| async function stopServer(): Promise<void> { | ||
| if (serverProcess) { | ||
| serverProcess.kill("SIGTERM"); | ||
| await new Promise((r) => setTimeout(r, 2000)); | ||
| if (!serverProcess.killed) serverProcess.kill("SIGKILL"); | ||
| serverProcess = null; | ||
| // Wait for port to be fully freed | ||
| for (let i = 0; i < 10; i++) { | ||
| try { | ||
| await fetch(`http://localhost:${TEST_PORT}/healthz`, { signal: AbortSignal.timeout(500) }); | ||
| // Port still in use — wait more | ||
| await new Promise((r) => setTimeout(r, 500)); | ||
| } catch { | ||
| break; // Port is free | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js child_process ChildProcess.killed vs exitCode meaning
💡 Result:
In Node.js, subprocess.killed and subprocess.exitCode serve distinct purposes regarding the lifecycle and termination status of a child process [1][2]. subprocess.killed The subprocess.killed property is a boolean that indicates whether subprocess.kill was successfully called on the ChildProcess object [1][2]. Crucially, this property does not signify that the process has actually terminated or ceased to exist; the process may still be running [1][3]. Its primary purpose is to track that a signal was sent via the Node.js API, which can be useful to prevent redundant calls to kill [3]. subprocess.exitCode The subprocess.exitCode property indicates the numeric exit code of the child process [1][2]. - If the child process is still running, the value is null [1][2]. - If the child process has exited normally, this property holds the exit code [1][2]. - If the child process was terminated by a signal, subprocess.exitCode will be null, and the signal information will be available in the subprocess.signalCode property [1][2]. In summary, subprocess.killed tracks the intent or action of sending a signal via the Node.js API [2][3], whereas subprocess.exitCode tracks the actual final status of the process after it has terminated [1][2].
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://github.com/nodejs/node/blob/master/doc/api/child_process.md
- 3:
childProcess.killedshould betrueafterprocess.kill(childProcess.pid)is called nodejs/node#27490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/mcp-integration.test.ts around stopServer ==="
sed -n '70,105p' src/mcp-integration.test.ts
echo
echo "=== Node child_process property semantics probe ==="
node <<'NODE'
const { spawn } = require('node:child_process');
const child = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 10000)'], {
stdio: 'ignore',
});
console.log('initial:', {
killed: child.killed,
exitCode: child.exitCode,
signalCode: child.signalCode,
});
const sent = child.kill('SIGTERM');
console.log('after kill(SIGTERM):', {
sent,
killed: child.killed,
exitCode: child.exitCode,
signalCode: child.signalCode,
});
setTimeout(() => {
console.log('after 250ms:', {
killed: child.killed,
exitCode: child.exitCode,
signalCode: child.signalCode,
});
try { child.kill('SIGKILL'); } catch {}
}, 250);
child.on('exit', () => process.exit(0));
NODERepository: Waishnav/devspace
Length of output: 1802
SIGKILL fallback never runs src/mcp-integration.test.ts:82 — serverProcess.killed becomes true as soon as SIGTERM is sent, so the SIGKILL branch is skipped even if the process is still alive. Check exitCode === null && signalCode === null instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp-integration.test.ts` around lines 78 - 93, Update stopServer to
determine whether the server process is still running using
serverProcess.exitCode === null && serverProcess.signalCode === null after the
SIGTERM wait, instead of checking serverProcess.killed, so the SIGKILL fallback
executes when necessary.
1 participant
Root Cause
Duplicate
sessionRegistry.markActive(sessionId)calls for existing MCP sessions, but only onesessionRegistry.markIdle(sessionId)call per request. This caused theinFlightcounter to permanently remain >= 1 for every session.After 64 sessions accumulated, new
initializerequests (the only sessions withinFlight=0) were immediately evicted by the registry, causing ChatGPT to receiveTimeoutError.Fix
1. Strict markActive/markIdle pairing (server.ts)
markActivecall (was called twice for existing sessions)markActiveper request, tracked viaactiveSessionIdvariablemarkIdlemoved tofinallyblock, only called whenactiveSessionIdwas setinitializerequests (nomarkActive) do not callmarkIdle2. Improved full-load behavior (mcp-session-registry.ts)
register()returnsboolean—falsewhen at capacity with no evictable idle sessionsatCapacitygetter —truewhen all 64 sessions haveinFlight > 0atCapacity,initializerequests get immediate HTTP 503 instead of being registered-then-evicted3. Regression tests (mcp-session-lifecycle.test.ts)
8 tests covering:
Verification
Custom Windows Stable Build
Also includes previous PRs: #71 session cleanup/graceful shutdown, #43 Windows drive root fix, #41 native PowerShell, #85 12000 char output limit, #76 context scan ignore, #65/#66 AGENTS/CLAUDE realpath, #69 ChatGPT MCP compatibility.
Summary by CodeRabbit