chore: release 0.8.1 - #23
Merged
Merged
Conversation
Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Junie <junie@jetbrains.com>
WalkthroughLarge refactor splitting CLI and server monolith into modular runtime and server surfaces, adding many CLI command runners (auth, daemon, proxy, monitor, test, stdio, etc.), reorganizing server capability routing/execution and lifecycle, introducing daemon/proxy reconnection coordination and OAuth SSE token handling, and bumping version to 0.8.1 with documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "CLI (runCliMain)"
participant Runtime as "Runtime Dispatcher"
participant DaemonRunner as "run-daemon"
participant Registry as "Instance Registry"
participant Daemon as "Daemon Process"
CLI->>Runtime: parse args & build dependencies
Runtime->>Runtime: dispatchCliRuntime(mode)
Runtime->>DaemonRunner: runDaemonCommand(options)
DaemonRunner->>Registry: ensure instance dir / write registration
DaemonRunner->>Daemon: start/createDaemon(startupOptions)
Daemon-->>DaemonRunner: started (socket, pid)
DaemonRunner->>Registry: register instance (post-start)
DaemonRunner->>CLI: return / await shutdown hooks
Note right of DaemonRunner: on shutdown/unregister
DaemonRunner->>Daemon: stop()
DaemonRunner->>Registry: unregister instance
Possibly related PRs
🚥 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)
📝 Coding Plan for PR comments
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5b9660385
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+147
to
+150
| if (allowSpawn && !options.noSpawn) { | ||
| const startupKey = options.configHash | ||
| ? `config:${options.configHash}` | ||
| : `endpoint:${configuredEndpoint ?? "default"}:${sharedSecret ?? ""}`; |
There was a problem hiding this comment.
When proxy is started with --daemon-socket, reconnects can still ignore that explicit endpoint because the recovery path checks registry/spawn first whenever configHash is present (and run-proxy always passes one). After a disconnect, this can silently reattach the proxy to a different daemon (or auto-spawn a local one) instead of the operator-specified socket, which can route requests to the wrong runtime.
Useful? React with 👍 / 👎.
Comment on lines
+172
to
+177
| if (configuredEndpoint) { | ||
| endpoint = configuredEndpoint; | ||
| return configuredEndpoint; | ||
| } | ||
|
|
||
| const transportOptions: { endpoint: string; timeoutMs?: number } = { | ||
| endpoint, | ||
| throw new Error("Daemon endpoint not available"); |
There was a problem hiding this comment.
runProxy() still advertises a default empty options object, but with the new target-resolution logic that code path now throws Daemon endpoint not available unless a configHash or explicit endpoint is provided. This breaks callers that invoke runProxy() without options, because no registry lookup/spawn fallback exists in the no-configHash branch anymore.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
src/server/capability-tool-surface.ts-89-99 (1)
89-99:⚠️ Potential issue | 🟡 MinorTypeScript type error with
exactOptionalPropertyTypes.Static analysis reports a type incompatibility: the return type includes
confirmationToken: string | undefined, butCapabilityToolRequestdefines it as an optional property (confirmationToken?: string). WithexactOptionalPropertyTypes: true, these are not assignable.The fix is to conditionally include the property only when it has a value.
🐛 Proposed fix for exactOptionalPropertyTypes compliance
return { action: typeof parsedArgs["action"] === "string" ? parsedArgs["action"] : "", arguments: isRecord(parsedArgs["arguments"]) ? (parsedArgs["arguments"] as Record<string, unknown>) : {}, - confirmationToken: - typeof parsedArgs["confirmation_token"] === "string" - ? parsedArgs["confirmation_token"] - : undefined, + ...(typeof parsedArgs["confirmation_token"] === "string" + ? { confirmationToken: parsedArgs["confirmation_token"] } + : {}), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/capability-tool-surface.ts` around lines 89 - 99, The returned object currently always includes confirmationToken with type string | undefined, which conflicts with CapabilityToolRequest's optional confirmationToken? when exactOptionalPropertyTypes is enabled; update the return value construction so the confirmationToken property is only added when parsedArgs["confirmation_token"] is a string (e.g., build the base object with action and arguments from parsedArgs and then conditionally spread in confirmationToken when typeof parsedArgs["confirmation_token"] === "string"), referencing the existing parsedArgs and isRecord checks to locate the code and ensure the final object matches CapabilityToolRequest's optional property shape.tests/run-daemon.test.ts-47-49 (1)
47-49:⚠️ Potential issue | 🟡 MinorRemove or restore this global env mutation.
runDaemonCommandresolves the daemon secret fromprocessRef.env, and this suite passes a stubbedprocessRefin the command test. That makes this setup unused here, while still leakingMCP_SQUARED_DAEMON_SECRETinto later tests that use the real process.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run-daemon.test.ts` around lines 47 - 49, The test suite mutates global state by setting process.env.MCP_SQUARED_DAEMON_SECRET in beforeEach even though runDaemonCommand uses a stubbed processRef; remove this global mutation or restore it after the test to avoid leaking into other tests: either delete the beforeEach that sets process.env.MCP_SQUARED_DAEMON_SECRET, or replace it with a beforeEach/afterEach pair that saves the original value, sets the env for the test, and restores the original in afterEach (referencing the beforeEach helper, the MCP_SQUARED_DAEMON_SECRET env var, and the runDaemonCommand/processRef usage to guide placement).tests/daemon-proxy.test.ts-403-410 (1)
403-410:⚠️ Potential issue | 🟡 MinorAvoid a fixed sleep before asserting reconnection.
Rebinding after daemon replacement is asynchronous; the hard-coded 250ms delay is likely to flake on slower CI. This file already has
waitFor, and it fits this assertion better.⏱️ Suggested change
await firstDaemon.stop(); await secondDaemon.start(); - await new Promise((resolve) => setTimeout(resolve, 250)); - - const recoveredTools = (await client.listTools()).tools.map( - (tool) => tool.name, - ); + const recoveredTools = await waitFor( + async () => (await client.listTools()).tools.map((tool) => tool.name), + (toolNames) => toolNames.includes("time_util"), + 3000, + ); expect(recoveredTools).toContain("time_util");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/daemon-proxy.test.ts` around lines 403 - 410, Replace the fixed 250ms sleep with a polling assertion using the existing waitFor helper: after calling firstDaemon.stop() and secondDaemon.start(), poll until client.listTools() returns a tools list that contains "time_util" (the recoveredTools check) instead of awaiting new Promise(resolve => setTimeout(...)). Locate the block around firstDaemon.stop(), secondDaemon.start(), and the recoveredTools = (await client.listTools()).tools.map(...) assertion and change it to use waitFor to repeatedly call client.listTools() (or check recoveredTools) until the expectation is met or a timeout occurs.src/cli/runtime-dispatch.ts-43-49 (1)
43-49:⚠️ Potential issue | 🟡 MinorReturn from the auth error path.
If
process.exitis mocked or intercepted, this falls through and callsrunAuth(undefined). Returning the exit call keeps the dispatcher safe under tests and other embedded runners.🛠️ Minimal fix
case "auth": if (!args.authTarget) { console.error("Error: auth command requires an upstream name."); console.error("Usage: mcp-squared auth <upstream>"); - process.exit(1); + return process.exit(1); } await dependencies.runAuth(args.authTarget); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/runtime-dispatch.ts` around lines 43 - 49, The auth case falls through after calling process.exit(1) which can invoke runAuth(undefined) if exit is mocked; update the "auth" branch so the exit path returns immediately (e.g., replace or prefix process.exit(1) with return process.exit(1) or add a return right after it) to ensure dependencies.runAuth(args.authTarget) is only called when args.authTarget is defined; locate the switch case handling "auth" that checks args.authTarget and modify that block accordingly (references: args.authTarget and dependencies.runAuth).tests/server-handlers.test.ts-330-355 (1)
330-355:⚠️ Potential issue | 🟡 MinorAssert confirmation in the describe output.
This test enables
confirm: ["general:delete_file"], so__describe_actionsshould reportrequiresConfirmation: truefordelete_file. Keeping the expectation atfalseeither makes the test fail against the current policy behavior or bakes in a mismatch between discovery and execution.🧪 Suggested fix
expect(actions[0]).toMatchObject({ action: "delete_file", - requiresConfirmation: false, + requiresConfirmation: true, summary: "Delete file", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server-handlers.test.ts` around lines 330 - 355, Update the test expectations so the __describe_actions discovery output reflects the configured confirmation requirement: when createSecurityConfig includes confirm: ["general:delete_file"], assert that the reported actions[0].requiresConfirmation is true rather than false; locate the block using McpSquaredServer, mockCatalogerForSingleTool, getCapabilityHandler and parseExecutePayload that calls execute with action "__describe_actions" and change the expectation for the "delete_file" action's requiresConfirmation to true.src/cli/main-runtime.ts-53-55 (1)
53-55:⚠️ Potential issue | 🟡 MinorRead stderr TTY state from
process.stderr, notstdout.Line 54 currently wires
isStderrTtytoprocess.stdout.isTTY, causing incorrect TTY detection when stdout and stderr have independent TTY states (e.g., stdout redirected to a file while stderr remains on a terminal). This affects the daemon vs. proxy mode decision at runtime-dispatch.ts:77.Suggested fix
- isStderrTty: process.stdout.isTTY, + isStderrTty: process.stderr.isTTY,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/main-runtime.ts` around lines 53 - 55, The isStderrTty value is incorrectly read from process.stdout; update the return object in main-runtime's initialization so isStderrTty uses process.stderr.isTTY (leave isStdinTty using process.stdin.isTTY) to ensure TTY detection used by the daemon/proxy decision (runtime-dispatch logic) reflects stderr's actual state; change the reference to the unique symbol isStderrTty in the returned object accordingly.src/server/index.ts-289-297 (1)
289-297:⚠️ Potential issue | 🟡 MinorFix type mismatch in
buildServerCapabilityRouterscall.Line 290 passes
this.cataloger.getStatus().entries(), which has typeIterableIterator<[string, {status: ConnectionStatus; error: string | undefined}]>, butbuildServerCapabilityRoutersexpectsstatusEntriesto beIterable<readonly [string, CatalogStatus]>whereCatalogStatus.erroris typed asError | undefined. The error field type mismatch (string | undefinedvsError | undefined) will cause a strict TypeScript error. Either updateCatalogStatusin capability-surface.ts to match the actual return type, or normalize the status map before passing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/index.ts` around lines 289 - 297, The call to buildServerCapabilityRouters passes this.cataloger.getStatus().entries() whose entries have error: string | undefined but the expected CatalogStatus.error is Error | undefined; fix by normalizing the status entries before passing them: iterate over this.cataloger.getStatus().entries(), map each [key, {status, error}] to [key, {status, error: error ? new Error(error) : undefined}] (or otherwise wrap/convert the string into an Error or undefined) and pass that Iterable<readonly [string, CatalogStatus]> to buildServerCapabilityRouters; alternatively, if you prefer type-level fix, update CatalogStatus.error in capability-surface.ts to string | undefined so the types align (choose one approach and apply consistently to buildServerCapabilityRouters, this.cataloger.getStatus().entries(), and CatalogStatus).
🧹 Nitpick comments (2)
src/server/runtime-lifecycle.ts (2)
99-106: Informational messages logged vialogErrorare semantically misleading.The
logErrorcallback is used for success/info messages like "Embeddings: initialized" and "Embeddings: enabled but runtime unavailable". While this works (since the default writes tostderr), it conflates error logging with informational output.Consider renaming to
logor adding a separatelogInfocallback to clarify intent. This is a minor naming concern that doesn't affect functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/runtime-lifecycle.ts` around lines 99 - 106, The info messages are using the error-specific callback logError (e.g., the calls inside the embeddings init branch that log "[mcp²] Embeddings: initialized..." and the runtime-unavailable fallback), which is semantically misleading; add a new callback (e.g., logInfo) to the runtime lifecycle API or rename the existing logError to a neutral name (e.g., log) and update all informational call sites to use it (references: logError calls in the embeddings initialization branch in runtime-lifecycle, and any other non-error uses), and ensure backward compatibility by falling back to logError when logInfo is not provided so behavior remains unchanged.
80-89: Connection failures are silently swallowed.The
catchblock on line 84-85 returns{ key, success: false }but doesn't log or surface the connection error. While the caller may handle this via status checks, consider logging a warning for debugging purposes.🔧 Proposed fix to log connection failures
const connectionPromises = enabledUpstreams.map(async ([key, upstream]) => { try { await cataloger.connect(key, upstream); return { key, success: true as const }; - } catch { + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + logError(`[mcp²] Failed to connect upstream "${key}" — ${message}`); return { key, success: false as const }; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/runtime-lifecycle.ts` around lines 80 - 89, The connectionPromise map swallows errors when calling cataloger.connect(key, upstream) — update the catch in the enabledUpstreams.map(async ([key, upstream]) => { ... }) so it logs the caught exception along with the upstream key before returning { key, success: false }. Use the existing logging facility in this module (e.g., runtime/server logger variable) or console.warn if none exists, and include both key and error.message/stack to aid debugging while preserving the current return shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/run-auth.ts`:
- Around line 238-242: The current check only rejects callbacks when
result.state exists but is invalid; update the logic so a missing OAuth state is
treated as a failure as well by rejecting when result.state is falsy or when
authProvider.verifyState(result.state) returns false. Modify the conditional
around result.state and authProvider.verifyState in the run-auth.ts callback
handling (the block that calls processRef.exit(1)) so that omitted state values
also trigger the error message "OAuth state mismatch - possible CSRF attack."
and exit the process instead of proceeding to exchange the code.
- Around line 19-24: Replace the ad-hoc OAuthProviderLike with the SDK's
official transport type (use the exported OAuthClientProvider type from the SDK)
so the DI surface matches StreamableHTTPClientTransport's expected type; update
the interface usage in run-auth.ts (replace references to OAuthProviderLike with
OAuthClientProvider and adjust any local method names to the SDK shape) and
remove the unsafe cast currently used where the narrower type caused a mismatch
(locations referencing StreamableHTTPClientTransport and the cast around the
provider). Ensure you import the SDK type and update any call sites to match the
SDK method names so types line up with OAuthClientProvider.
In `@src/cli/run-daemon.ts`:
- Around line 108-114: The daemon secret from resolveDaemonSharedSecret is being
left in processRef.argv and thus persisted by buildCliInstanceEntry; before
calling buildCliInstanceEntry, sanitize processRef (or a shallow copy) by
locating and redacting the CLI arg value (e.g., replace the token following the
--sharedSecret/--shared-secret flag or mask the flag's value in processRef.argv)
so that processRef.argv.join(" ") no longer contains the raw secret; apply the
same redaction in the alternative branch (the block around lines 119-128) and
ensure daemonOptions.sharedSecret still gets the resolved secret but only the
sanitized processRef is passed into buildCliInstanceEntry.
- Line 123: Compute the result of resolveLauncherHint(processRef.env) into a
local const (e.g., const launcherHint = resolveLauncherHint(processRef.env)) and
only include the launcher property in the BuildCliInstanceEntryOptions object
when that const is not undefined (for example using a conditional spread or an
if to add the property). This ensures the created options conform to
BuildCliInstanceEntryOptions by omitting launcher when undefined rather than
passing undefined directly.
In `@src/cli/run-proxy.ts`:
- Line 61: The launcher property is declared as optional (launcher?: string) in
BuildCliInstanceEntryOptions but you are passing a possibly undefined value from
resolveLauncherHint(processRef.env); change the call site in run-proxy.ts to
only include launcher when the resolver returns a defined string—either by
extracting const launcher = resolveLauncherHint(processRef.env) and
conditionally spreading { ...(launcher ? { launcher } : {}) } into the options
object, or by using a conditional object literal that adds launcher only when
not undefined—this ensures the BuildCliInstanceEntryOptions type receives either
a string or no property, satisfying exactOptionalPropertyTypes.
In `@src/cli/run-stdio-server.ts`:
- Line 133: The object currently sets launcher:
resolveLauncherHint(processRef.env) which yields string | undefined and violates
exactOptionalPropertyTypes; change to only include the launcher property when it
is defined by first calling const launcherHint =
resolveLauncherHint(processRef.env) and then conditionally adding the property
(e.g. use a conditional spread like ...(launcherHint ? { launcher: launcherHint
} : {}) or an if to set the property) so the property is omitted when undefined;
update references in this scope where launcher is assigned.
In `@src/cli/run-test.ts`:
- Around line 117-127: Validation currently only considers errors tied to
upstreams (errorUpstreams) and ignores top-level validation errors; update the
check after validateConfig to detect any validationIssues with severity ===
"error" and no upstream (e.g., issue.upstream is falsy), print
formatValidationIssues(validationIssues) and then exit non-zero immediately to
fail fast; apply the same change where validationIssues and errorUpstreams are
used later in the file (the blocks referencing validateConfig, validationIssues,
errorUpstreams, and formatValidationIssues) so that top-level errors always
abort the run.
- Line 8: The import of the non-existent type UpstreamConfig should be removed
from run-test.ts and any casts or references to UpstreamConfig replaced with the
actual exported type UpstreamServerConfig (from UpstreamServerSchema) or dropped
to let TypeScript infer McpSquaredConfig["upstreams"][string]; specifically
remove the import of UpstreamConfig and remove the cast to UpstreamConfig at the
upstream usage site, then either type that value as UpstreamServerConfig or
leave it untyped to rely on inference.
In `@src/cli/runtime-bootstrap.ts`:
- Around line 58-70: The returned InstanceRegistryEntry is emitting role:
undefined which breaks with exactOptionalPropertyTypes; change the object
construction to only include role when it is defined (use the same
conditional-spread pattern as launcher) so that role is omitted rather than
assigned undefined—update the return in the function that builds the
InstanceRegistryEntry to use ...(role ? { role } : {}) instead of directly
including role.
In `@src/daemon/proxy.ts`:
- Around line 234-299: When creating a new SocketClientTransport in
connectDaemon, the previous daemonTransport must be cleanly shut down first to
avoid leaked sockets/stale traffic; before assigning daemonTransport =
transport, check if daemonTransport exists and call its proper shutdown method
(e.g., close/stop/shutdown on the existing SocketClientTransport) and remove its
event handlers, then assign the new transport and wire up its handlers. Update
the reconnect path (the code around transport.onclose / reconnectDaemon and the
similar block at lines ~325-337) to ensure the old transport is closed and
nulled before spawning a replacement and that any pending timers are cleared
(use clearReconnectTimer and reuse closeStdio as needed) so only one active
transport exists at a time.
- Around line 136-177: resolveDaemonTarget currently throws when neither
options.configHash nor configuredEndpoint is set, causing runProxy() to fail for
the default daemon namespace; instead, when options.configHash is falsy and
configuredEndpoint is not provided, compute the default hashed socket via
getDaemonSocketPath(options.configHash) (or pass undefined/null), assign
endpoint = hashedEndpoint and return it (mirror the earlier branch that returns
a hashedEndpoint), so resolveDaemonTarget, and thus runProxy, will fall back to
the default daemon namespace without throwing; update the final else/throw
branch to perform this fallback and keep sharedSecret behavior unchanged.
In `@src/server/capability-tool-executor.ts`:
- Around line 41-48: The mapping currently treats any object with a "type" key
as a text block, which lets non-text blocks bypass stringification and yields
objects without a guaranteed text field; update the content.map logic in
capability-tool-executor (the mapping that returns { type: "text"; text: string
}) to explicitly detect objects where entry.type === "text" and ensure they have
a string text property (e.g., coerce missing text to String(entry.text || "")),
and for all other cases (non-text objects and primitives) return { type: "text",
text: JSON.stringify(entry) } so ResponseResourceManager always receives
well-formed text blocks.
In `@src/server/index.ts`:
- Around line 320-333: The callback types for requestId should be tightened to
number: update the registerCapabilityTools callback signature so
onCapabilityRequestStarted returns requestId: number (using the numeric value
from this.statsCollector.startRequest()) and onCapabilityRequestFinished accepts
requestId: number (not string|number) so it can be passed directly to
StatsCollector.endRequest; specifically change the requestId type in the
onCapabilityRequestStarted/onCapabilityRequestFinished definitions to number and
ensure any upstream type declarations for registerCapabilityTools reflect
requestId: number throughout.
In `@src/server/server-shell.ts`:
- Around line 31-39: Ensure the shared runtime started by args.startCore() is
always cleaned up on failure: wrap the sequence that calls
args.registerConfiguredSessionSurface(args.server) and await
args.server.connect(transport) (and the later block that calls
args.server.close()) in try/catch/finally so that on any exception you
deterministically call args.server.close() (safely) and await args.stopCore();
also ensure args.statsCollector.incrementActiveConnections() is only called
after successful connect and decrement or rollback is performed if connect or
subsequent operations fail; update the logic in the block surrounding
startCore()/registerConfiguredSessionSurface()/server.connect()/server.close()
(also apply same pattern to the later 49-51 block) to perform safe cleanup even
when close() itself throws.
In `@src/upstream/client.ts`:
- Around line 347-358: The connect retry is still racing the original
timeoutPromise, so recreate/reset the timeout before retrying after the OAuth
branch: after creating httpTransport (httpTransportFactory, transport and after
safelyCloseTransport) cancel/clear the previous timeout (if any) and create a
new timeoutPromise for the full timeout period, then call await
Promise.race([client.connect(transport!), newTimeoutPromise]); this ensures
client.connect (in client.connect) uses a fresh timeout budget for the post-auth
attempt.
In `@tests/server-capability-tool-executor.test.ts`:
- Around line 46-53: Add a test case to ensure normalizeToolResultContent
stringifies non-text entries even when they have a type field (e.g., { type:
"image", ... }): call normalizeToolResultContent with a mixed array that
includes a typed non-text object and assert the result contains a text block
whose text is JSON.stringify(the object). Reference the
normalizeToolResultContent helper to locate the behavior to verify and add the
new expectation alongside the existing test.
---
Minor comments:
In `@src/cli/main-runtime.ts`:
- Around line 53-55: The isStderrTty value is incorrectly read from
process.stdout; update the return object in main-runtime's initialization so
isStderrTty uses process.stderr.isTTY (leave isStdinTty using
process.stdin.isTTY) to ensure TTY detection used by the daemon/proxy decision
(runtime-dispatch logic) reflects stderr's actual state; change the reference to
the unique symbol isStderrTty in the returned object accordingly.
In `@src/cli/runtime-dispatch.ts`:
- Around line 43-49: The auth case falls through after calling process.exit(1)
which can invoke runAuth(undefined) if exit is mocked; update the "auth" branch
so the exit path returns immediately (e.g., replace or prefix process.exit(1)
with return process.exit(1) or add a return right after it) to ensure
dependencies.runAuth(args.authTarget) is only called when args.authTarget is
defined; locate the switch case handling "auth" that checks args.authTarget and
modify that block accordingly (references: args.authTarget and
dependencies.runAuth).
In `@src/server/capability-tool-surface.ts`:
- Around line 89-99: The returned object currently always includes
confirmationToken with type string | undefined, which conflicts with
CapabilityToolRequest's optional confirmationToken? when
exactOptionalPropertyTypes is enabled; update the return value construction so
the confirmationToken property is only added when
parsedArgs["confirmation_token"] is a string (e.g., build the base object with
action and arguments from parsedArgs and then conditionally spread in
confirmationToken when typeof parsedArgs["confirmation_token"] === "string"),
referencing the existing parsedArgs and isRecord checks to locate the code and
ensure the final object matches CapabilityToolRequest's optional property shape.
In `@src/server/index.ts`:
- Around line 289-297: The call to buildServerCapabilityRouters passes
this.cataloger.getStatus().entries() whose entries have error: string |
undefined but the expected CatalogStatus.error is Error | undefined; fix by
normalizing the status entries before passing them: iterate over
this.cataloger.getStatus().entries(), map each [key, {status, error}] to [key,
{status, error: error ? new Error(error) : undefined}] (or otherwise
wrap/convert the string into an Error or undefined) and pass that
Iterable<readonly [string, CatalogStatus]> to buildServerCapabilityRouters;
alternatively, if you prefer type-level fix, update CatalogStatus.error in
capability-surface.ts to string | undefined so the types align (choose one
approach and apply consistently to buildServerCapabilityRouters,
this.cataloger.getStatus().entries(), and CatalogStatus).
In `@tests/daemon-proxy.test.ts`:
- Around line 403-410: Replace the fixed 250ms sleep with a polling assertion
using the existing waitFor helper: after calling firstDaemon.stop() and
secondDaemon.start(), poll until client.listTools() returns a tools list that
contains "time_util" (the recoveredTools check) instead of awaiting new
Promise(resolve => setTimeout(...)). Locate the block around firstDaemon.stop(),
secondDaemon.start(), and the recoveredTools = (await
client.listTools()).tools.map(...) assertion and change it to use waitFor to
repeatedly call client.listTools() (or check recoveredTools) until the
expectation is met or a timeout occurs.
In `@tests/run-daemon.test.ts`:
- Around line 47-49: The test suite mutates global state by setting
process.env.MCP_SQUARED_DAEMON_SECRET in beforeEach even though runDaemonCommand
uses a stubbed processRef; remove this global mutation or restore it after the
test to avoid leaking into other tests: either delete the beforeEach that sets
process.env.MCP_SQUARED_DAEMON_SECRET, or replace it with a beforeEach/afterEach
pair that saves the original value, sets the env for the test, and restores the
original in afterEach (referencing the beforeEach helper, the
MCP_SQUARED_DAEMON_SECRET env var, and the runDaemonCommand/processRef usage to
guide placement).
In `@tests/server-handlers.test.ts`:
- Around line 330-355: Update the test expectations so the __describe_actions
discovery output reflects the configured confirmation requirement: when
createSecurityConfig includes confirm: ["general:delete_file"], assert that the
reported actions[0].requiresConfirmation is true rather than false; locate the
block using McpSquaredServer, mockCatalogerForSingleTool, getCapabilityHandler
and parseExecutePayload that calls execute with action "__describe_actions" and
change the expectation for the "delete_file" action's requiresConfirmation to
true.
---
Nitpick comments:
In `@src/server/runtime-lifecycle.ts`:
- Around line 99-106: The info messages are using the error-specific callback
logError (e.g., the calls inside the embeddings init branch that log "[mcp²]
Embeddings: initialized..." and the runtime-unavailable fallback), which is
semantically misleading; add a new callback (e.g., logInfo) to the runtime
lifecycle API or rename the existing logError to a neutral name (e.g., log) and
update all informational call sites to use it (references: logError calls in the
embeddings initialization branch in runtime-lifecycle, and any other non-error
uses), and ensure backward compatibility by falling back to logError when
logInfo is not provided so behavior remains unchanged.
- Around line 80-89: The connectionPromise map swallows errors when calling
cataloger.connect(key, upstream) — update the catch in the
enabledUpstreams.map(async ([key, upstream]) => { ... }) so it logs the caught
exception along with the upstream key before returning { key, success: false }.
Use the existing logging facility in this module (e.g., runtime/server logger
variable) or console.warn if none exists, and include both key and
error.message/stack to aid debugging while preserving the current return shape.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d0a8f4f-360c-4a8a-8ec6-b79260aa4042
📒 Files selected for processing (51)
.github/CONTRIBUTING.mdCHANGELOG.mdREADME.mdWARP.mddocs/PROJECT-INCEPTION-REQUIREMENTS.mdpackage.jsonsrc/cli/main-runtime.tssrc/cli/run-auth.tssrc/cli/run-daemon.tssrc/cli/run-import.tssrc/cli/run-install.tssrc/cli/run-migrate.tssrc/cli/run-monitor.tssrc/cli/run-proxy.tssrc/cli/run-stdio-server.tssrc/cli/run-test.tssrc/cli/runtime-bootstrap.tssrc/cli/runtime-dispatch.tssrc/cli/runtime-profiles.tssrc/cli/tui-runtime.tssrc/config/validate.tssrc/daemon/proxy.tssrc/daemon/server.tssrc/index.tssrc/server/capability-surface.tssrc/server/capability-tool-executor.tssrc/server/capability-tool-surface.tssrc/server/index.tssrc/server/runtime-lifecycle.tssrc/server/server-shell.tssrc/server/session-surface.tssrc/upstream/client.tstests/config.test.tstests/daemon-proxy.test.tstests/daemon-server.test.tstests/main-runtime.test.tstests/run-auth.test.tstests/run-daemon.test.tstests/run-management-commands.test.tstests/run-monitor.test.tstests/run-proxy.test.tstests/run-stdio-server.test.tstests/run-test.test.tstests/runtime-bootstrap.test.tstests/server-capability-surface.test.tstests/server-capability-tool-executor.test.tstests/server-handlers.test.tstests/server-runtime-lifecycle.test.tstests/server-session-surface.test.tstests/server-shell.test.tstests/upstream-client-oauth.test.ts
Comment on lines
+19
to
+24
| interface OAuthProviderLike { | ||
| tokens(): unknown; | ||
| isTokenExpired(): boolean; | ||
| verifyState(state: string): boolean; | ||
| clearCodeVerifier(): void; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files src/cli/run-auth.tsRepository: aditzel/mcp-squared
Length of output: 82
🏁 Script executed:
cat -n src/cli/run-auth.ts | head -30Repository: aditzel/mcp-squared
Length of output: 1200
🏁 Script executed:
cat -n src/cli/run-auth.ts | sed -n '80,95p'Repository: aditzel/mcp-squared
Length of output: 590
🏁 Script executed:
cat -n src/cli/run-auth.ts | sed -n '195,205p'Repository: aditzel/mcp-squared
Length of output: 518
🏁 Script executed:
rg "OAuthClientProvider" --type ts --type jsRepository: aditzel/mcp-squared
Length of output: 698
🏁 Script executed:
cat -n src/oauth/provider.ts | head -50Repository: aditzel/mcp-squared
Length of output: 2105
🏁 Script executed:
rg "class StreamableHTTPClientTransport|interface OAuthClientProvider" --type ts --type js -A 3Repository: aditzel/mcp-squared
Length of output: 45
🏁 Script executed:
cat -n src/cli/run-auth.ts | sed -n '50,70p'Repository: aditzel/mcp-squared
Length of output: 861
🏁 Script executed:
cat -n src/cli/run-auth.ts | sed -n '35,65p'Repository: aditzel/mcp-squared
Length of output: 1137
🏁 Script executed:
rg "StreamableHTTPClientTransport" -B 2 -A 5 --type ts --type jsRepository: aditzel/mcp-squared
Length of output: 10222
🏁 Script executed:
cat -n src/cli/run-auth.ts | sed -n '120,145p'Repository: aditzel/mcp-squared
Length of output: 848
🏁 Script executed:
cat -n src/cli/run-auth.ts | sed -n '155,215p'Repository: aditzel/mcp-squared
Length of output: 2196
🏁 Script executed:
rg "interface OAuthClientProvider|class StreamableHTTPClientTransport" --type ts --type js -A 10Repository: aditzel/mcp-squared
Length of output: 45
🏁 Script executed:
# Check if there are any build errors in the repository
find . -name "tsconfig.json" -o -name "package.json" | head -5Repository: aditzel/mcp-squared
Length of output: 93
🏁 Script executed:
# Look at the actual SDK types used elsewhere in the codebase
cat -n src/oauth/provider.ts | sed -n '1,150p'Repository: aditzel/mcp-squared
Length of output: 5764
🏁 Script executed:
# Check what methods McpOAuthProvider actually implements
ast-grep --pattern 'class McpOAuthProvider {
$$$
}'Repository: aditzel/mcp-squared
Length of output: 45
🏁 Script executed:
# Check the actual type expected by StreamableHTTPClientTransport in its usage
cat -n src/upstream/client.ts | sed -n '160,190p'Repository: aditzel/mcp-squared
Length of output: 974
🏁 Script executed:
cat -n src/oauth/provider.ts | sed -n '145,250p'Repository: aditzel/mcp-squared
Length of output: 4025
🏁 Script executed:
# Check what OAuthClientProvider requires - look for its usage pattern
rg "implements OAuthClientProvider" -A 20 --type ts --type jsRepository: aditzel/mcp-squared
Length of output: 1295
🏁 Script executed:
# Verify if the current code compiles - check for any TypeScript config
cat tsconfig.json | head -30Repository: aditzel/mcp-squared
Length of output: 840
🏁 Script executed:
# Look at how StreamableHTTPClientTransport is used in other parts of the codebase to understand what it needs
cat -n src/cli/run-auth.ts | sed -n '215,250p'Repository: aditzel/mcp-squared
Length of output: 1325
🏁 Script executed:
# Check if there's any documentation about what finishAuth is for
rg "finishAuth" --type ts --type js -B 3 -A 3Repository: aditzel/mcp-squared
Length of output: 5873
🏁 Script executed:
# Check if there are any build errors reported
cat -n src/cli/run-auth.ts | sed -n '66,95p'Repository: aditzel/mcp-squared
Length of output: 1083
🏁 Script executed:
# Verify the implementation of createAuthProvider to see what it actually returns
cat -n src/cli/run-auth.ts | sed -n '66,82p'Repository: aditzel/mcp-squared
Length of output: 655
🏁 Script executed:
# Check the actual implementation of createAuthProvider to see if it returns McpOAuthProvider
rg "createAuthProvider" -B 5 -A 15 --type ts --type js | head -60Repository: aditzel/mcp-squared
Length of output: 3116
🏁 Script executed:
# Check how the interfaces are actually used in the main runAuthCommand function
cat -n src/cli/run-auth.ts | sed -n '98,175p'Repository: aditzel/mcp-squared
Length of output: 2663
Use the SDK's auth/transport types in this DI surface.
StreamableHTTPClientTransport expects the SDK's OAuthClientProvider, but OAuthProviderLike is narrower. This causes a type error at line 87 and forces an unsafe cast at line 200.
🛠️ Suggested fix
-import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js";
+import {
+ type OAuthClientProvider,
+ UnauthorizedError,
+} from "@modelcontextprotocol/sdk/client/auth.js";
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js";
import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js";
@@
-interface OAuthProviderLike {
+interface OAuthProviderLike extends OAuthClientProvider {
tokens(): unknown;
isTokenExpired(): boolean;
verifyState(state: string): boolean;
clearCodeVerifier(): void;
}
@@
createTransport: (options: {
authProvider: OAuthProviderLike;
headers?: Record<string, string>;
url: string;
- }) => { finishAuth(code: string): Promise<void> };
+ }) => Transport & { finishAuth(code: string): Promise<void> };
@@
- await client.connect(transport as unknown as Transport);
+ await client.connect(transport);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/run-auth.ts` around lines 19 - 24, Replace the ad-hoc
OAuthProviderLike with the SDK's official transport type (use the exported
OAuthClientProvider type from the SDK) so the DI surface matches
StreamableHTTPClientTransport's expected type; update the interface usage in
run-auth.ts (replace references to OAuthProviderLike with OAuthClientProvider
and adjust any local method names to the SDK shape) and remove the unsafe cast
currently used where the narrower type caused a mismatch (locations referencing
StreamableHTTPClientTransport and the cast around the provider). Ensure you
import the SDK type and update any call sites to match the SDK method names so
types line up with OAuthClientProvider.
Comment on lines
+238
to
+242
| if (result.state && !authProvider.verifyState(result.state)) { | ||
| console.error("\nError: OAuth state mismatch - possible CSRF attack."); | ||
| processRef.exit(1); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Treat a missing OAuth state as a failed callback.
src/oauth/provider.ts:166-175 shows the provider generates a state value, but this branch only rejects mismatches when result.state is present. A callback that omits state will currently exchange the code anyway, which weakens the CSRF protection on the auth flow.
🔐 Suggested fix
- if (result.state && !authProvider.verifyState(result.state)) {
- console.error("\nError: OAuth state mismatch - possible CSRF attack.");
+ if (!result.state || !authProvider.verifyState(result.state)) {
+ console.error(
+ "\nError: Missing or mismatched OAuth state - possible CSRF attack.",
+ );
processRef.exit(1);
return;
}📝 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.
Suggested change
| if (result.state && !authProvider.verifyState(result.state)) { | |
| console.error("\nError: OAuth state mismatch - possible CSRF attack."); | |
| processRef.exit(1); | |
| return; | |
| } | |
| if (!result.state || !authProvider.verifyState(result.state)) { | |
| console.error( | |
| "\nError: Missing or mismatched OAuth state - possible CSRF attack.", | |
| ); | |
| processRef.exit(1); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/run-auth.ts` around lines 238 - 242, The current check only rejects
callbacks when result.state exists but is invalid; update the logic so a missing
OAuth state is treated as a failure as well by rejecting when result.state is
falsy or when authProvider.verifyState(result.state) returns false. Modify the
conditional around result.state and authProvider.verifyState in the run-auth.ts
callback handling (the block that calls processRef.exit(1)) so that omitted
state values also trigger the error message "OAuth state mismatch - possible
CSRF attack." and exit the process instead of proceeding to exchange the code.
Comment on lines
+108
to
+114
| const sharedSecret = resolveDaemonSharedSecret( | ||
| options.sharedSecret, | ||
| processRef.env, | ||
| ); | ||
| if (sharedSecret) { | ||
| daemonOptions.sharedSecret = sharedSecret; | ||
| } |
There was a problem hiding this comment.
Redact --sharedSecret before building the instance entry.
This path accepts a daemon secret from argv, then passes the raw processRef into buildCliInstanceEntry(). That helper stores processRef.argv.join(" ") in the registry entry (src/cli/runtime-bootstrap.ts:48-70), so a CLI-supplied secret is persisted in plaintext.
Also applies to: 119-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/run-daemon.ts` around lines 108 - 114, The daemon secret from
resolveDaemonSharedSecret is being left in processRef.argv and thus persisted by
buildCliInstanceEntry; before calling buildCliInstanceEntry, sanitize processRef
(or a shallow copy) by locating and redacting the CLI arg value (e.g., replace
the token following the --sharedSecret/--shared-secret flag or mask the flag's
value in processRef.argv) so that processRef.argv.join(" ") no longer contains
the raw secret; apply the same redaction in the alternative branch (the block
around lines 119-128) and ensure daemonOptions.sharedSecret still gets the
resolved secret but only the sanitized processRef is passed into
buildCliInstanceEntry.
| buildCliInstanceEntry({ | ||
| configPath, | ||
| id: `daemon-${configHash}`, | ||
| launcher: resolveLauncherHint(processRef.env), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the file mentioned in the review
git ls-files src/cli/run-daemon.tsRepository: aditzel/mcp-squared
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Check TypeScript configuration for exactOptionalPropertyTypes
find . -name "tsconfig.json" -o -name "tsconfig*.json" | head -5Repository: aditzel/mcp-squared
Length of output: 78
🏁 Script executed:
#!/bin/bash
# Search for resolveLauncherHint function definition
rg "resolveLauncherHint" --type ts --type tsx -B 2 -A 5Repository: aditzel/mcp-squared
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Search for BuildCliInstanceEntryOptions interface
rg "BuildCliInstanceEntryOptions|interface.*InstanceEntry" --type ts --type tsx -A 10Repository: aditzel/mcp-squared
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Search for resolveLauncherHint function definition (without tsx type)
rg "resolveLauncherHint" -B 2 -A 5Repository: aditzel/mcp-squared
Length of output: 6121
🏁 Script executed:
#!/bin/bash
# Search for BuildCliInstanceEntryOptions interface
rg "BuildCliInstanceEntryOptions|interface.*InstanceEntry" -A 10Repository: aditzel/mcp-squared
Length of output: 1213
🏁 Script executed:
#!/bin/bash
# Check tsconfig.json for exactOptionalPropertyTypes
cat tsconfig.json | grep -A 5 -B 5 "exactOptionalPropertyTypes"Repository: aditzel/mcp-squared
Length of output: 375
🏁 Script executed:
#!/bin/bash
# Read the specific file to understand context around line 123
head -150 src/cli/run-daemon.ts | tail -50Repository: aditzel/mcp-squared
Length of output: 1186
🏁 Script executed:
#!/bin/bash
# Check if other files (stdio, proxy) have the same pattern
rg "launcher: resolveLauncherHint" -B 2 -A 2Repository: aditzel/mcp-squared
Length of output: 848
🏁 Script executed:
#!/bin/bash
# Check how processRef is constructed and whether CLI args include secrets
rg "processRef.*argv|sharedSecret.*argv" -B 3 -A 3Repository: aditzel/mcp-squared
Length of output: 822
🏁 Script executed:
#!/bin/bash
# Look at the complete buildCliInstanceEntry function to understand what happens with processRef
rg "function buildCliInstanceEntry" -A 20Repository: aditzel/mcp-squared
Length of output: 1104
🏁 Script executed:
#!/bin/bash
# Verify that the typing issue is actually present by checking if there are any compilation errors mentioned
# or if the code compiles successfully
find . -name "*.test.ts" | xargs grep -l "run-daemon\|buildCliInstanceEntry" | head -3Repository: aditzel/mcp-squared
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check if there are any jest/test outputs or CI configs that might show compilation status
ls -la | grep -E "jest|test|tsconfig|package.json"Repository: aditzel/mcp-squared
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Examine what happens with argv and whether secrets could leak
rg "sharedSecret|argv" src/cli/run-daemon.ts -B 2 -A 2Repository: aditzel/mcp-squared
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Check the test file to see how buildCliInstanceEntry is called
cat tests/run-daemon.test.tsRepository: aditzel/mcp-squared
Length of output: 6191
🏁 Script executed:
#!/bin/bash
# Look for CLI argument parsing to see if --sharedSecret might end up in argv
rg "sharedSecret|argv" -B 3 -A 3 | head -80Repository: aditzel/mcp-squared
Length of output: 3973
🏁 Script executed:
#!/bin/bash
# Check if there's a CLI argument parser or yargs/commander setup
rg "yargs|commander|program\.|option\(" --type js -B 1 -A 1 | head -60Repository: aditzel/mcp-squared
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Check if there are any build scripts or if tests pass
cat package.json | grep -A 5 -B 5 "scripts\|test"Repository: aditzel/mcp-squared
Length of output: 1944
🏁 Script executed:
#!/bin/bash
# Try to understand if the code actually compiles or if there's a build issue
# Check for any compilation errors or build artifacts
find . -name "*.ts" -path "*/src/cli/run-daemon.ts" -exec head -150 {} \; | tail -50Repository: aditzel/mcp-squared
Length of output: 1186
🏁 Script executed:
#!/antml/bash
# Verify the exact line 123 in the file and surrounding context
sed -n '100,150p' src/cli/run-daemon.tsRepository: aditzel/mcp-squared
Length of output: 1202
Omit launcher when it resolves to undefined.
With exactOptionalPropertyTypes enabled, passing resolveLauncherHint(processRef.env) directly violates the BuildCliInstanceEntryOptions contract when the function returns undefined. The property must either be omitted or hold a defined value.
🛠️ Suggested fix
+ const launcher = resolveLauncherHint(processRef.env);
const registration = createInstanceRegistration(
buildCliInstanceEntry({
configPath,
id: `daemon-${configHash}`,
- launcher: resolveLauncherHint(processRef.env),
processRef,
role: "daemon",
socketPath: monitorSocketPath,
version: VERSION,
+ ...(launcher ? { launcher } : {}),
}),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/run-daemon.ts` at line 123, Compute the result of
resolveLauncherHint(processRef.env) into a local const (e.g., const launcherHint
= resolveLauncherHint(processRef.env)) and only include the launcher property in
the BuildCliInstanceEntryOptions object when that const is not undefined (for
example using a conditional spread or an if to add the property). This ensures
the created options conform to BuildCliInstanceEntryOptions by omitting launcher
when undefined rather than passing undefined directly.
| buildCliInstanceEntry({ | ||
| configPath, | ||
| id: `proxy-${processRef.pid}`, | ||
| launcher: resolveLauncherHint(processRef.env), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -70 src/cli/run-proxy.ts | tail -20Repository: aditzel/mcp-squared
Length of output: 640
🏁 Script executed:
sed -n '50,75p' src/cli/run-proxy.tsRepository: aditzel/mcp-squared
Length of output: 797
🏁 Script executed:
find . -name "tsconfig.json" -o -name "tsconfig.*.json" | head -5Repository: aditzel/mcp-squared
Length of output: 78
🏁 Script executed:
grep -r "exactOptionalPropertyTypes" .Repository: aditzel/mcp-squared
Length of output: 118
🏁 Script executed:
sed -n '50,80p' src/cli/run-proxy.ts | cat -nRepository: aditzel/mcp-squared
Length of output: 1111
🏁 Script executed:
rg "resolveLauncherHint" --type ts -A 3Repository: aditzel/mcp-squared
Length of output: 3167
🏁 Script executed:
rg "buildCliInstanceEntry" --type ts -A 5 -B 2Repository: aditzel/mcp-squared
Length of output: 6366
🏁 Script executed:
rg "createInstanceRegistration" --type ts -A 5 -B 2Repository: aditzel/mcp-squared
Length of output: 5480
🏁 Script executed:
fd ".*\.ts" | xargs rg "interface.*InstanceRegistration|type.*InstanceRegistration" -A 10Repository: aditzel/mcp-squared
Length of output: 1515
Fix the exactOptionalPropertyTypes violation for launcher property.
The launcher?: string property in BuildCliInstanceEntryOptions cannot accept string | undefined from resolveLauncherHint(). Conditionally spread the launcher or extract and conditionally include it.
Suggested fix
+ const launcher = resolveLauncherHint(processRef.env);
const registration = createInstanceRegistration(
buildCliInstanceEntry({
configPath,
id: `proxy-${processRef.pid}`,
- launcher: resolveLauncherHint(processRef.env),
processRef,
role: "proxy",
socketPath: monitorSocketPath,
version: VERSION,
+ ...(launcher ? { launcher } : {}),
}),
);📝 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.
Suggested change
| launcher: resolveLauncherHint(processRef.env), | |
| const launcher = resolveLauncherHint(processRef.env); | |
| const registration = createInstanceRegistration( | |
| buildCliInstanceEntry({ | |
| configPath, | |
| id: `proxy-${processRef.pid}`, | |
| processRef, | |
| role: "proxy", | |
| socketPath: monitorSocketPath, | |
| version: VERSION, | |
| ...(launcher ? { launcher } : {}), | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/run-proxy.ts` at line 61, The launcher property is declared as
optional (launcher?: string) in BuildCliInstanceEntryOptions but you are passing
a possibly undefined value from resolveLauncherHint(processRef.env); change the
call site in run-proxy.ts to only include launcher when the resolver returns a
defined string—either by extracting const launcher =
resolveLauncherHint(processRef.env) and conditionally spreading { ...(launcher ?
{ launcher } : {}) } into the options object, or by using a conditional object
literal that adds launcher only when not undefined—this ensures the
BuildCliInstanceEntryOptions type receives either a string or no property,
satisfying exactOptionalPropertyTypes.
Comment on lines
+41
to
+48
| return content.map((entry) => { | ||
| if (typeof entry === "object" && entry !== null && "type" in entry) { | ||
| return entry as { type: "text"; text: string }; | ||
| } | ||
| return { | ||
| type: "text" as const, | ||
| text: JSON.stringify(entry), | ||
| }; |
There was a problem hiding this comment.
Normalize only real text blocks.
Line 42 currently casts any { type: ... } object to { type: "text"; text: string }. That means non-text content skips stringification, while bare strings become quoted JSON, so the client sees the wrong payload and ResponseResourceManager gets blocks without a guaranteed text field.
🧩 Proposed fix
export function normalizeToolResultContent(content: unknown[]): Array<{
type: "text";
text: string;
}> {
return content.map((entry) => {
- if (typeof entry === "object" && entry !== null && "type" in entry) {
- return entry as { type: "text"; text: string };
+ if (
+ typeof entry === "object" &&
+ entry !== null &&
+ "type" in entry &&
+ (entry as { type?: unknown }).type === "text" &&
+ typeof (entry as { text?: unknown }).text === "string"
+ ) {
+ return {
+ type: "text" as const,
+ text: (entry as { text: string }).text,
+ };
+ }
+ if (typeof entry === "string") {
+ return {
+ type: "text" as const,
+ text: entry,
+ };
}
return {
type: "text" as const,
- text: JSON.stringify(entry),
+ text: JSON.stringify(entry) ?? String(entry),
};
});
}📝 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.
Suggested change
| return content.map((entry) => { | |
| if (typeof entry === "object" && entry !== null && "type" in entry) { | |
| return entry as { type: "text"; text: string }; | |
| } | |
| return { | |
| type: "text" as const, | |
| text: JSON.stringify(entry), | |
| }; | |
| export function normalizeToolResultContent(content: unknown[]): Array<{ | |
| type: "text"; | |
| text: string; | |
| }> { | |
| return content.map((entry) => { | |
| if ( | |
| typeof entry === "object" && | |
| entry !== null && | |
| "type" in entry && | |
| (entry as { type?: unknown }).type === "text" && | |
| typeof (entry as { text?: unknown }).text === "string" | |
| ) { | |
| return { | |
| type: "text" as const, | |
| text: (entry as { text: string }).text, | |
| }; | |
| } | |
| if (typeof entry === "string") { | |
| return { | |
| type: "text" as const, | |
| text: entry, | |
| }; | |
| } | |
| return { | |
| type: "text" as const, | |
| text: JSON.stringify(entry) ?? String(entry), | |
| }; | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/capability-tool-executor.ts` around lines 41 - 48, The mapping
currently treats any object with a "type" key as a text block, which lets
non-text blocks bypass stringification and yields objects without a guaranteed
text field; update the content.map logic in capability-tool-executor (the
mapping that returns { type: "text"; text: string }) to explicitly detect
objects where entry.type === "text" and ensure they have a string text property
(e.g., coerce missing text to String(entry.text || "")), and for all other cases
(non-text objects and primitives) return { type: "text", text:
JSON.stringify(entry) } so ResponseResourceManager always receives well-formed
text blocks.
Comment on lines
+320
to
+333
| onCapabilityRequestStarted: () => ({ | ||
| requestId: this.statsCollector.startRequest(), | ||
| startTime: Date.now(), | ||
| }), | ||
| onCapabilityRequestFinished: ({ | ||
| requestId, | ||
| capability, | ||
| { | ||
| title: this.capabilityTitle(capability), | ||
| description: this.capabilitySummary(capability), | ||
| annotations: { | ||
| readOnlyHint: false, | ||
| destructiveHint: false, | ||
| openWorldHint: true, | ||
| }, | ||
| inputSchema: { | ||
| action: z | ||
| .string() | ||
| .describe( | ||
| `Action ID for ${capability}. Use "${DESCRIBE_ACTION}" to inspect available actions and schemas.`, | ||
| ), | ||
| arguments: z | ||
| .record(z.string(), z.unknown()) | ||
| .default({}) | ||
| .describe("Arguments for the selected capability action"), | ||
| confirmation_token: z | ||
| .string() | ||
| .optional() | ||
| .describe( | ||
| "Optional confirmation token for actions that require explicit confirmation", | ||
| ), | ||
| }, | ||
| }, | ||
| async (rawArgs) => | ||
| this.runTaskSpan(capability, async () => { | ||
| const requestId = this.statsCollector.startRequest(); | ||
| const startTime = Date.now(); | ||
| let success = false; | ||
|
|
||
| try { | ||
| const liveRouter = this.getCapabilityRouter(capability); | ||
| const parsedArgs: Record<string, unknown> = isRecord(rawArgs) | ||
| ? { ...rawArgs } | ||
| : {}; | ||
| const action = | ||
| typeof parsedArgs["action"] === "string" | ||
| ? parsedArgs["action"] | ||
| : ""; | ||
| const confirmationToken = | ||
| typeof parsedArgs["confirmation_token"] === "string" | ||
| ? parsedArgs["confirmation_token"] | ||
| : undefined; | ||
| const actionArgs = isRecord(parsedArgs["arguments"]) | ||
| ? (parsedArgs["arguments"] as Record<string, unknown>) | ||
| : {}; | ||
|
|
||
| if (action.length === 0) { | ||
| return { | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: JSON.stringify({ | ||
| error: "Missing required action", | ||
| capability, | ||
| }), | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; | ||
| } | ||
|
|
||
| const visibleRoutes = liveRouter.actions | ||
| .map((route) => { | ||
| const visibility = getToolVisibilityCompiled( | ||
| capability, | ||
| route.action, | ||
| this.compiledPolicy, | ||
| ); | ||
| if (!visibility.visible) { | ||
| return null; | ||
| } | ||
| return { | ||
| route, | ||
| requiresConfirmation: visibility.requiresConfirmation, | ||
| }; | ||
| }) | ||
| .filter( | ||
| ( | ||
| entry, | ||
| ): entry is { | ||
| route: CapabilityRouter["actions"][number]; | ||
| requiresConfirmation: boolean; | ||
| } => entry !== null, | ||
| ); | ||
|
|
||
| const visibleActions = visibleRoutes | ||
| .map(({ route, requiresConfirmation }) => { | ||
| const actionInfo: { | ||
| action: string; | ||
| summary: string; | ||
| inputSchema: ToolInputSchema; | ||
| requiresConfirmation: boolean; | ||
| baseAction?: string; | ||
| instance?: string; | ||
| instanceTitle?: string; | ||
| } = { | ||
| action: route.action, | ||
| summary: route.summary, | ||
| inputSchema: route.inputSchema, | ||
| requiresConfirmation, | ||
| }; | ||
|
|
||
| if ((route.collisionGroupSize ?? 1) > 1) { | ||
| actionInfo.baseAction = route.baseAction; | ||
| actionInfo.instance = route.instanceKey ?? route.serverKey; | ||
| actionInfo.instanceTitle = | ||
| route.instanceTitle ?? | ||
| route.instanceKey ?? | ||
| route.serverKey; | ||
| } | ||
|
|
||
| return actionInfo; | ||
| }) | ||
| .sort((a, b) => a.action.localeCompare(b.action)); | ||
|
|
||
| if (action === DESCRIBE_ACTION) { | ||
| success = true; | ||
| return { | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: JSON.stringify({ | ||
| capability, | ||
| actions: visibleActions, | ||
| totalActions: visibleActions.length, | ||
| }), | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
|
|
||
| const exactRoute = liveRouter.actions.find( | ||
| (entry) => | ||
| entry.action === action || | ||
| (entry.legacyActions ?? []).includes(action), | ||
| ); | ||
|
|
||
| const ambiguousCandidates = visibleRoutes | ||
| .filter(({ route }) => route.baseAction === action) | ||
| .map(({ route }) => route.action) | ||
| .sort((a, b) => a.localeCompare(b)); | ||
|
|
||
| if (ambiguousCandidates.length > 1) { | ||
| return { | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: JSON.stringify({ | ||
| requires_disambiguation: true, | ||
| capability, | ||
| action, | ||
| candidates: ambiguousCandidates, | ||
| }), | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; | ||
| } | ||
|
|
||
| const selectedRoute = | ||
| exactRoute ?? | ||
| (ambiguousCandidates.length === 1 | ||
| ? visibleRoutes.find( | ||
| ({ route }) => route.baseAction === action, | ||
| )?.route | ||
| : undefined); | ||
|
|
||
| if (selectedRoute == null) { | ||
| return { | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: JSON.stringify({ | ||
| error: "Unknown action", | ||
| capability, | ||
| action, | ||
| availableActions: visibleActions.map((a) => a.action), | ||
| }), | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; | ||
| } | ||
|
|
||
| const callResult = await this.executeRoutedTool({ | ||
| capability, | ||
| action: selectedRoute.action, | ||
| policyAction: | ||
| exactRoute != null ? action : selectedRoute.action, | ||
| routeId: | ||
| selectedRoute.canonicalRouteId ?? | ||
| `${capability}:${selectedRoute.action}`, | ||
| qualifiedToolName: selectedRoute.qualifiedName, | ||
| toolNameForCall: selectedRoute.qualifiedName, | ||
| args: actionArgs, | ||
| ...(confirmationToken != null ? { confirmationToken } : {}), | ||
| }); | ||
| success = !callResult.isError; | ||
| return callResult; | ||
| } catch { | ||
| return { | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: JSON.stringify({ | ||
| error: "Action execution failed", | ||
| }), | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; | ||
| } finally { | ||
| const responseTime = Date.now() - startTime; | ||
| this.statsCollector.endRequest( | ||
| requestId, | ||
| success, | ||
| responseTime, | ||
| capability, | ||
| "capability", | ||
| ); | ||
| } | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private normalizeToolResultContent(content: unknown[]): Array<{ | ||
| type: "text"; | ||
| text: string; | ||
| }> { | ||
| return content.map((entry) => { | ||
| if (typeof entry === "object" && entry !== null && "type" in entry) { | ||
| return entry as { type: "text"; text: string }; | ||
| } | ||
| return { | ||
| type: "text" as const, | ||
| text: JSON.stringify(entry), | ||
| }; | ||
| success, | ||
| startTime, | ||
| }) => { | ||
| this.statsCollector.endRequest( | ||
| requestId, | ||
| success, | ||
| Date.now() - startTime, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the specific lines mentioned in the review
head -n 340 src/server/index.ts | tail -n 25Repository: aditzel/mcp-squared
Length of output: 859
🏁 Script executed:
# Find the StatsCollector class and endRequest method signature
rg -A 5 "class StatsCollector|endRequest" src/server/index.tsRepository: aditzel/mcp-squared
Length of output: 222
🏁 Script executed:
# Look for registerCapabilityTools and its type definitions
rg -B 5 -A 10 "registerCapabilityTools" src/server/index.tsRepository: aditzel/mcp-squared
Length of output: 1666
🏁 Script executed:
# Check if there are any type definitions or interfaces for the callback
fd "*.ts" src/ | head -20Repository: aditzel/mcp-squared
Length of output: 346
🏁 Script executed:
# Find StatsCollector class definition
rg -B 2 -A 20 "class StatsCollector" src/Repository: aditzel/mcp-squared
Length of output: 1308
🏁 Script executed:
# Find capability-tool-surface.js and examine registerCapabilityTools
cat src/server/capability-tool-surface.ts | head -100Repository: aditzel/mcp-squared
Length of output: 2926
🏁 Script executed:
# Search for the type definition of the callbacks in registerCapabilityTools
rg -B 5 -A 20 "onCapabilityRequestStarted|onCapabilityRequestFinished" src/server/capability-tool-surface.tsRepository: aditzel/mcp-squared
Length of output: 2584
🏁 Script executed:
# Find StatsCollector.startRequest and endRequest methods
rg -A 8 "startRequest|endRequest" src/server/stats.ts | head -50Repository: aditzel/mcp-squared
Length of output: 1142
🏁 Script executed:
# Get more context on these methods
cat src/server/stats.ts | grep -A 10 "startRequest\|endRequest"Repository: aditzel/mcp-squared
Length of output: 1269
Fix the requestId type in the onCapabilityRequestStarted and onCapabilityRequestFinished callbacks to remain number.
The callback contract in registerCapabilityTools currently allows requestId: string | number, but StatsCollector.startRequest() returns number and StatsCollector.endRequest() expects number. This type widening creates a type safety hole. Tighten the callback types to requestId: number to preserve the concrete handle type end-to-end.
🧰 Tools
🪛 GitHub Check: Cross-platform smoke (macos-latest)
[failure] 331-331:
Argument of type 'string | number' is not assignable to parameter of type 'number'.
🪛 GitHub Check: Cross-platform smoke (windows-latest)
[failure] 331-331:
Argument of type 'string | number' is not assignable to parameter of type 'number'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/index.ts` around lines 320 - 333, The callback types for requestId
should be tightened to number: update the registerCapabilityTools callback
signature so onCapabilityRequestStarted returns requestId: number (using the
numeric value from this.statsCollector.startRequest()) and
onCapabilityRequestFinished accepts requestId: number (not string|number) so it
can be passed directly to StatsCollector.endRequest; specifically change the
requestId type in the onCapabilityRequestStarted/onCapabilityRequestFinished
definitions to number and ensure any upstream type declarations for
registerCapabilityTools reflect requestId: number throughout.
Comment on lines
+31
to
+39
| await args.startCore(); | ||
|
|
||
| if (!args.baseToolsRegistered) { | ||
| args.registerConfiguredSessionSurface(args.server); | ||
| } | ||
|
|
||
| const transport = new StdioServerTransport(); | ||
| await args.server.connect(transport); | ||
| args.statsCollector.incrementActiveConnections(); |
There was a problem hiding this comment.
Rollback the shared runtime on lifecycle failures.
If registerConfiguredSessionSurface() or server.connect() throws after startCore(), the core is left running. If server.close() throws, stopCore() never executes. Those are exactly the failure paths where the shared runtime should be cleaned up deterministically.
Also applies to: 49-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/server-shell.ts` around lines 31 - 39, Ensure the shared runtime
started by args.startCore() is always cleaned up on failure: wrap the sequence
that calls args.registerConfiguredSessionSurface(args.server) and await
args.server.connect(transport) (and the later block that calls
args.server.close()) in try/catch/finally so that on any exception you
deterministically call args.server.close() (safely) and await args.stopCore();
also ensure args.statsCollector.incrementActiveConnections() is only called
after successful connect and decrement or rollback is performed if connect or
subsequent operations fail; update the logic in the block surrounding
startCore()/registerConfiguredSessionSurface()/server.connect()/server.close()
(also apply same pattern to the later 49-51 block) to perform safe cleanup even
when close() itself throws.
Comment on lines
+347
to
358
| if (sseConfig && httpTransportFactory) { | ||
| await safelyCloseTransport(transport); | ||
| httpTransport = httpTransportFactory( | ||
| sseConfig, | ||
| log, | ||
| verbose, | ||
| authProvider, | ||
| ); | ||
| transport = httpTransport as Transport; | ||
| } | ||
| // biome-ignore lint/style/noNonNullAssertion: transport is assigned above for all code paths | ||
| await Promise.race([client.connect(transport!), timeoutPromise]); |
There was a problem hiding this comment.
Reset the connect timeout before the post-OAuth retry.
The retry still races against the original timeout created for the first connect(), so the browser auth round-trip eats into the initial 30s budget. If authorization takes longer than the remaining time, the second connect fails with Connection timeout even after a successful login.
Proposed fix
- log(`Connecting (timeout: ${timeoutMs}ms)...`);
- const connectStart = Date.now();
- // biome-ignore lint/style/noNonNullAssertion: transport is assigned above for all code paths
- const connectPromise = client.connect(transport!);
- let timeoutId: ReturnType<typeof setTimeout> | undefined;
-
- const timeoutPromise = new Promise<never>((_, reject) => {
- timeoutId = setTimeout(
- () => reject(new Error("Connection timeout")),
- timeoutMs,
- );
- });
+ log(`Connecting (timeout: ${timeoutMs}ms)...`);
+ const connectStart = Date.now();
+ const connectWithTimeout = async (): Promise<void> => {
+ let attemptTimeoutId: ReturnType<typeof setTimeout> | undefined;
+ try {
+ const attemptTimeoutPromise = new Promise<never>((_, reject) => {
+ attemptTimeoutId = setTimeout(
+ () => reject(new Error("Connection timeout")),
+ timeoutMs,
+ );
+ });
+ // biome-ignore lint/style/noNonNullAssertion: transport is assigned above for all code paths
+ await Promise.race([client.connect(transport!), attemptTimeoutPromise]);
+ } finally {
+ if (attemptTimeoutId !== undefined) {
+ clearTimeout(attemptTimeoutId);
+ }
+ }
+ };
try {
- await Promise.race([connectPromise, timeoutPromise]);
+ await connectWithTimeout();
} catch (err) {
// Handle OAuth authorization required
if (err instanceof UnauthorizedError && authProvider && httpTransport) {
@@
if (sseConfig && httpTransportFactory) {
await safelyCloseTransport(transport);
httpTransport = httpTransportFactory(
sseConfig,
log,
@@
);
transport = httpTransport as Transport;
}
- // biome-ignore lint/style/noNonNullAssertion: transport is assigned above for all code paths
- await Promise.race([client.connect(transport!), timeoutPromise]);
+ await connectWithTimeout();
} else {
// client_credentials should have worked automatically
throw err;
}
} else {
throw err;
}
- } finally {
- if (timeoutId !== undefined) {
- clearTimeout(timeoutId);
- }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/upstream/client.ts` around lines 347 - 358, The connect retry is still
racing the original timeoutPromise, so recreate/reset the timeout before
retrying after the OAuth branch: after creating httpTransport
(httpTransportFactory, transport and after safelyCloseTransport) cancel/clear
the previous timeout (if any) and create a new timeoutPromise for the full
timeout period, then call await Promise.race([client.connect(transport!),
newTimeoutPromise]); this ensures client.connect (in client.connect) uses a
fresh timeout budget for the post-auth attempt.
Comment on lines
+46
to
+53
| describe("normalizeToolResultContent", () => { | ||
| test("stringifies non-text entries", () => { | ||
| expect(normalizeToolResultContent([{ ok: true }, 42])).toEqual([ | ||
| { type: "text", text: JSON.stringify({ ok: true }) }, | ||
| { type: "text", text: JSON.stringify(42) }, | ||
| ]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add a typed non-text case here.
src/server/capability-tool-executor.ts currently treats any object with a type property as already normalized, so { type: "image", ... } bypasses stringification even though this helper promises text blocks. This test only covers values without type, so that regression would still pass.
🧪 Suggested coverage
test("stringifies non-text entries", () => {
- expect(normalizeToolResultContent([{ ok: true }, 42])).toEqual([
+ expect(
+ normalizeToolResultContent([{ ok: true }, 42, { type: "image", url: "x" }]),
+ ).toEqual([
{ type: "text", text: JSON.stringify({ ok: true }) },
{ type: "text", text: JSON.stringify(42) },
+ { type: "text", text: JSON.stringify({ type: "image", url: "x" }) },
]);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/server-capability-tool-executor.test.ts` around lines 46 - 53, Add a
test case to ensure normalizeToolResultContent stringifies non-text entries even
when they have a type field (e.g., { type: "image", ... }): call
normalizeToolResultContent with a mixed array that includes a typed non-text
object and assert the result contains a text block whose text is
JSON.stringify(the object). Reference the normalizeToolResultContent helper to
locate the behavior to verify and add the new expectation alongside the existing
test.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (7)
src/cli/run-auth.ts (2)
251-255:⚠️ Potential issue | 🔴 CriticalReject callbacks that omit
state.The provider generates a state token for this flow, so accepting a callback with no
statebypasses the CSRF check entirely. Missing and mismatched values should fail the same way.Suggested fix
- if (result.state && !authProvider.verifyState(result.state)) { + if (!result.state || !authProvider.verifyState(result.state)) { console.error("\nError: OAuth state mismatch - possible CSRF attack."); processRef.exit(1); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run-auth.ts` around lines 251 - 255, The callback currently only rejects when result.state exists but fails verification, letting missing state bypass CSRF checks; update the logic so that missing or invalid state both trigger rejection: check result.state and authProvider.verifyState(result.state) together and if result.state is falsy OR verifyState returns false, log the same error and call processRef.exit(1). Reference the existing symbols result.state, authProvider.verifyState, and processRef.exit to locate and modify the conditional.
19-24:⚠️ Potential issue | 🟠 MajorMake the auth DI surface match the SDK contract.
createTransportis typed as returning onlyfinishAuth(), andOAuthProviderLikeis narrower than the SDK provider type, soclient.connect(transport as unknown as Transport)can hide broken test doubles and API drift. This should be typed asTransport & { finishAuth(...) }plus the SDK auth-provider interface, then the cast can go away.In `@modelcontextprotocol/sdk` v1.27.1, what interface does `StreamableHTTPClientTransport` accept as its `authProvider`, and does `StreamableHTTPClientTransport` itself satisfy the shared `Transport` contract accepted by `Client.connect`?Also applies to: 60-64, 89-101, 212-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run-auth.ts` around lines 19 - 24, The DI surface types are too narrow and rely on casts; update the signatures so the transport returned by createTransport is typed as Transport & { finishAuth(...): Promise<void> } and make OAuthProviderLike extend the SDK auth-provider interface used by StreamableHTTPClientTransport (rather than a custom minimal shape) so you can call client.connect(transport) without casting; specifically adjust the createTransport return type, the OAuthProviderLike interface, and any places calling client.connect(transport as unknown as Transport) (see createTransport, finishAuth, OAuthProviderLike, and client.connect) to use the combined Transport+SDK-auth-provider types and remove the cast.src/cli/run-stdio-server.ts (1)
129-140:⚠️ Potential issue | 🔴 CriticalOmit
launcherhere as well when it resolves toundefined.
launcher?: stringcannot acceptstring | undefinedunderexactOptionalPropertyTypes; the property needs to be omitted instead of passed through.#!/bin/bash set -euo pipefail # Verify the optional-property contract and the current call site. rg -n '"exactOptionalPropertyTypes"\s*:\s*true' tsconfig.json rg -n 'launcher\?: string' src/cli/runtime-bootstrap.ts sed -n '129,140p' src/cli/run-stdio-server.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run-stdio-server.ts` around lines 129 - 140, The call passes launcher even when resolveLauncherHint(processRef.env) returns undefined which violates exactOptionalPropertyTypes; change the call site so that buildCliInstanceEntry is given launcher only when defined (e.g., compute launcher = resolveLauncherHint(...) then conditionally include it into the object instead of always passing the launcher variable) so that createInstanceRegistration/buildCliInstanceEntry receive no launcher property when undefined.src/cli/run-proxy.ts (1)
57-68:⚠️ Potential issue | 🔴 CriticalOmit
launcherwhen it resolves toundefined.Same
exactOptionalPropertyTypesblocker as the other CLI runners:launcher?: stringmust be absent, notundefined.#!/bin/bash set -euo pipefail # Verify the optional-property contract and the current call site. rg -n '"exactOptionalPropertyTypes"\s*:\s*true' tsconfig.json rg -n 'launcher\?: string' src/cli/runtime-bootstrap.ts sed -n '57,68p' src/cli/run-proxy.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run-proxy.ts` around lines 57 - 68, The current call passes launcher even when resolveLauncherHint(processRef.env) returns undefined which yields launcher: undefined and violates exactOptionalPropertyTypes; change the creation of the CLI instance entry so that launcher is only included when defined — e.g. compute launcher = resolveLauncherHint(processRef.env) then build the object passed to buildCliInstanceEntry using a conditional spread so that createInstanceRegistration(buildCliInstanceEntry({... , role: "proxy", ..., version: VERSION, ...(launcher !== undefined ? { launcher } : {})})) only adds the launcher property when launcher is not undefined.src/cli/run-daemon.ts (2)
119-130:⚠️ Potential issue | 🔴 CriticalOmit
launcherwhen it resolves toundefined.With
exactOptionalPropertyTypes,launcher?: stringmust be omitted, not set toundefined. This call site still passesstring | undefined, so it keeps the same build blocker.#!/bin/bash set -euo pipefail # Verify the optional-property contract and the current call site. rg -n '"exactOptionalPropertyTypes"\s*:\s*true' tsconfig.json rg -n 'launcher\?: string' src/cli/runtime-bootstrap.ts sed -n '119,130p' src/cli/run-daemon.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run-daemon.ts` around lines 119 - 130, The call to buildCliInstanceEntry passes launcher which may be undefined, violating exactOptionalPropertyTypes; update the call site so launcher is omitted when resolveLauncherHint(processRef.env) returns undefined — e.g., keep const launcher = resolveLauncherHint(processRef.env) and change the buildCliInstanceEntry invocation to only include the launcher property when launcher is defined (use a conditional spread or conditional property inclusion) so buildCliInstanceEntry/createInstanceRegistration receive an object without a launcher key instead of launcher: undefined.
108-114:⚠️ Potential issue | 🔴 CriticalRedact
--sharedSecretbefore persisting the daemon instance entry.
buildCliInstanceEntry()storesprocessRef.argv.join(" ")in the registry entry, so a CLI-supplied daemon secret is currently written in plaintext.Also applies to: 121-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run-daemon.ts` around lines 108 - 114, The CLI can persist the plaintext secret because processRef.argv is stored by buildCliInstanceEntry; before calling buildCliInstanceEntry (and anywhere registry entries are written) sanitize processRef.argv by producing a redactedArgv that strips or replaces the daemon secret: detect flags like "--sharedSecret" or "--shared-secret" in both "--flag=value" and "--flag value" forms and replace the value with "<REDACTED>" (or replace the entire "--flag=value" token), and use that redactedArgv in place of processRef.argv when building the entry; update code paths around resolveDaemonSharedSecret, daemonOptions.sharedSecret and the buildCliInstanceEntry calls (including the similar block around lines 121-130) to pass the redacted argv.src/cli/run-test.ts (1)
117-127:⚠️ Potential issue | 🟠 MajorFail fast on top-level config errors.
errorUpstreamsonly tracks issues that carry anupstreamname. Root-level validation errors are printed, but both the targeted and all-upstreams paths can still exit 0 afterwards.Also applies to: 140-154, 167-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run-test.ts` around lines 117 - 127, The top-level validation errors (issues without an upstream) are being printed but the process still continues; update the post-validation logic in run-test.ts (around validateConfig, errorUpstreams, formatValidationIssues) to fail fast: after computing validationIssues, if any issue has severity === "error" and no upstream (i.e., issue.upstream is falsy), call process.exit(1) (after printing the formatted issues) so the CLI exits non-zero for root-level config errors; keep the existing handling for upstream-specific errors using errorUpstreams unchanged.
🧹 Nitpick comments (5)
src/server/capability-surface.ts (1)
36-42: Share the describe-action token instead of hardcoding it here.
buildServerInstructions()embeds"__describe_actions"whilesrc/server/capability-tool-surface.tsalready exportsDESCRIBE_ACTION. For a release focused on consistency, this is an easy place for help text and runtime behavior to drift.Use the shared constant
+import { DESCRIBE_ACTION } from "./capability-tool-surface.js"; + export function buildServerInstructions(): string { return [ "Tool surface is generated at connect time from inferred upstream capabilities.", "Each capability tool accepts `action`, `arguments`, and optional `confirmation_token`.", - 'Call a capability tool with `action = "__describe_actions"` to inspect available actions and schemas.', + `Call a capability tool with \`action = "${DESCRIBE_ACTION}"\` to inspect available actions and schemas.`, "Use returned action IDs for execution calls; if disambiguation is required, choose one candidate action and retry.", ].join(" "); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/capability-surface.ts` around lines 36 - 42, The help text in buildServerInstructions embeds the literal "__describe_actions" which can drift from the shared constant; update buildServerInstructions() to import and use the exported DESCRIBE_ACTION from capability-tool-surface.ts instead of the hardcoded string, and ensure you add the import for DESCRIBE_ACTION at the top of the file so the returned instructions reference the shared DESCRIBE_ACTION identifier.src/server/capability-tool-surface.ts (1)
12-16: DeduplicateCapabilityToolResultbefore it drifts.This shape is already exported from
src/server/capability-tool-executor.ts(lines 4-8). Keeping two public copies of the same contract will eventually desync and makes imports ambiguous. Prefer one shared definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/capability-tool-surface.ts` around lines 12 - 16, Remove the duplicate type definition for CapabilityToolResult in this file and import the shared type instead of redefining it: replace the local export of CapabilityToolResult with an import of the exported type from the existing declaration (the CapabilityToolResult exported from capability-tool-executor) so there is a single canonical contract referenced by both modules.tests/run-daemon.test.ts (3)
196-206: Consider extending coverage to verifycreateDaemonandcreateRuntimefactories.The test validates the presence of utility functions but doesn't verify
createDaemonandcreateRuntime— the core factory functions that create the daemon and runtime instances.♻️ Suggested addition
expect(typeof deps.computeConfigHash).toBe("function"); expect(typeof deps.getSocketFilePath).toBe("function"); expect(typeof deps.loadConfig).toBe("function"); + expect(typeof deps.createDaemon).toBe("function"); + expect(typeof deps.createRuntime).toBe("function"); expect(deps.processRef).toBe(process);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run-daemon.test.ts` around lines 196 - 206, The test currently checks utility functions but should also assert the default factory functions and their basic behavior: verify that deps.createDaemon and deps.createRuntime (returned from createRunDaemonDependencies) are functions, call each factory with minimal required args to ensure they return objects of the expected shape (e.g., a daemon with a start/stop or run method and a runtime with expected methods/properties), and add assertions that these returned objects are non-null and expose the expected methods; use the existing createRunDaemonDependencies invocation and reference deps.createDaemon and deps.createRuntime to locate the code to update.
125-127: Consider using a more robust wait after triggering the signal handler.The
await Promise.resolve()may not be sufficient to wait for the full async shutdown sequence. Ifshutdowninvolves multiple async operations (e.g.,await daemon.stop()followed by cleanup), a single microtask tick might not be enough.Consider waiting for the exit mock to be called or using a small timeout/flush:
♻️ Suggested improvement
handlers.get("SIGINT")?.(); - await Promise.resolve(); + // Wait for async shutdown to complete + await new Promise((resolve) => setTimeout(resolve, 0));Or alternatively, wait for the exit call:
handlers.get("SIGINT")?.(); await vi.waitFor(() => expect(exit).toHaveBeenCalled());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run-daemon.test.ts` around lines 125 - 127, Replace the fragile microtask wait after invoking the signal handler with a robust wait that ensures the full async shutdown completes: after calling handlers.get("SIGINT")?.() wait for the mocked exit to be called (use vi.waitFor or equivalent) or await a small timeout/flush so the shutdown sequence (e.g., daemon.stop and cleanup) finishes; reference the test symbols handlers, "SIGINT", and the exit mock to locate and update the assertion/wait logic.
43-49: Clean up the environment variable inafterEachto prevent state leakage.The
beforeEachsetsprocess.env["MCP_SQUARED_DAEMON_SECRET"]butafterEachdoesn't restore it. This could leak state to subsequent tests.♻️ Suggested fix
+ let originalSecret: string | undefined; + + beforeEach(() => { + originalSecret = process.env["MCP_SQUARED_DAEMON_SECRET"]; process.env["MCP_SQUARED_DAEMON_SECRET"] = "env-secret"; }); afterEach(() => { mock.restore(); + if (originalSecret === undefined) { + delete process.env["MCP_SQUARED_DAEMON_SECRET"]; + } else { + process.env["MCP_SQUARED_DAEMON_SECRET"] = originalSecret; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run-daemon.test.ts` around lines 43 - 49, The test sets process.env["MCP_SQUARED_DAEMON_SECRET"] in beforeEach but never restores it; modify the test to save the original value (e.g., const originalSecret = process.env["MCP_SQUARED_DAEMON_SECRET"] inside beforeEach or a surrounding scope) and then in afterEach (alongside mock.restore()) restore it: if originalSecret is undefined delete process.env["MCP_SQUARED_DAEMON_SECRET"] else set it back to originalSecret; reference the existing beforeEach and afterEach blocks and the environment variable name to locate where to add the save/restore logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-11: Move the dependency-exception/release-runbook note out of
the "## [Unreleased]" section and add it under the "## [0.8.1]" release notes
within the "### Changed" category; specifically, remove the existing bullet from
Unreleased and paste it into the 0.8.1 section (keeping the "### Changed"
heading and Keep a Changelog categories formatting intact) so the 0.8.1 release
contains the documented change before merging.
In `@docs/DEPENDENCY_EXCEPTIONS.md`:
- Line 7: The table row describing "`file-type` infinite loop in ASF parser
(`>=13.0.0 <21.3.1`)" currently uses the placeholder "Pending maintainer
tracking issue" in the Linked Issue column; replace that placeholder with a real
tracking issue reference (issue URL or repository/issue ID) and update the
status if appropriate so audits can follow up, locating the row by the unique
text "`file-type` infinite loop in ASF parser" or the dependency chain
"`@opentui/core -> jimp -> `@jimp/core` -> file-type`" and ensuring the Linked
Issue cell contains a clickable URL/issue identifier and optional brief status
(e.g., "Open: https://.../issues/1234").
In `@docs/RELEASING.md`:
- Line 41: Update the hard-coded example version strings in RELEASING.md:
replace occurrences of "0.7.0", "v0.7.0", and any commands like `bun pm version
0.7.0`/`push v0.7.0` with either the current release "0.8.1"/"v0.8.1" or,
preferably, a generic placeholder format `<version>`/`v<version>` so the runbook
won't drift; ensure you update all matching examples and command snippets in the
file (search for "0.7.0" and "v0.7.0") and keep the command examples consistent
like `bun pm version <version>` and `push v<version>`.
In `@src/cli/run-proxy.ts`:
- Around line 59-68: The CLI daemon secret in processRef.argv is being persisted
because buildCliInstanceEntry() stores processRef.argv.join(" "), so before
calling buildCliInstanceEntry (used inside createInstanceRegistration) sanitize
processRef.argv by removing or redacting any '--sharedSecret' flag and its
value; e.g., make a shallow copy of processRef (or argv) and replace the secret
value with '<redacted>' or remove both the flag and following value, then pass
the sanitized processRef (or sanitized argv) into buildCliInstanceEntry; apply
the same redaction logic for the other occurrence around the code that builds
the proxy instance entry (lines referenced in the comment).
In `@src/server/capability-tool-surface.ts`:
- Around line 257-318: exactRoute currently uses liveRouter.actions so routes
filtered out by getVisibleCapabilityRoutes() remain callable; change exactRoute
to search only visibleRoutes (e.g., inspect each visibleRoutes item for
route.action and route.legacyActions) instead of liveRouter.actions, update any
uses of exactRoute selection logic to derive from visibleRoutes.find(...) (or
equivalent) so both exact and legacy alias matches honor visibility before
proceeding to ambiguousCandidates/selectedRoute and executeRoute.
In `@tests/daemon-proxy.test.ts`:
- Around line 560-575: The tests mutate the global
process.env["MCP_CLIENT_NAME"] (seen around the createProxyBridge calls that
produce firstBridge and secondBridge) and never restore it; save the original
value (e.g., const prev = process.env["MCP_CLIENT_NAME"]) before overriding and
restore it after the test (either in an afterEach/try...finally or immediately
after creating the bridges) so the global env is returned to its prior state;
apply the same pattern for other occurrences (the blocks that set
MCP_CLIENT_NAME and call createProxyBridge/spawnReplacementDaemon) to prevent
leaked client names across tests.
- Around line 464-477: The test currently patches
SocketClientTransport.prototype.start (originalStart, startCalls, startSpy)
which mutates behavior process-wide; instead create or obtain the specific
SocketClientTransport instance used in this test and spy on that instance's
start method (e.g., spyOn(instance, "start")) so the injected transient failure
only affects this transport; restore the original implementation in the test
teardown/finally and keep the same retry logic (reject once then delegate to the
original instance method) but reference the instance method rather than the
prototype to avoid leaking failures to other tests.
In `@tests/daemon-server.test.ts`:
- Around line 330-340: The test's createSessionServer uses sessionIndex =
closeCount so multiple sessions created before any close all capture 0; replace
this with a creation counter (e.g., createCount) that you increment each time
createSessionServer is called and capture into sessionIndex so each created
session gets a unique index; update uses of sessionIndex in the close() logic
(and related arrays like closedSessionIds and the closingGate handling) so only
the intended session waits on closingGate. Also apply the same creation-counter
fix to the other occurrences noted (around the blocks corresponding to the other
two ranges).
---
Duplicate comments:
In `@src/cli/run-auth.ts`:
- Around line 251-255: The callback currently only rejects when result.state
exists but fails verification, letting missing state bypass CSRF checks; update
the logic so that missing or invalid state both trigger rejection: check
result.state and authProvider.verifyState(result.state) together and if
result.state is falsy OR verifyState returns false, log the same error and call
processRef.exit(1). Reference the existing symbols result.state,
authProvider.verifyState, and processRef.exit to locate and modify the
conditional.
- Around line 19-24: The DI surface types are too narrow and rely on casts;
update the signatures so the transport returned by createTransport is typed as
Transport & { finishAuth(...): Promise<void> } and make OAuthProviderLike extend
the SDK auth-provider interface used by StreamableHTTPClientTransport (rather
than a custom minimal shape) so you can call client.connect(transport) without
casting; specifically adjust the createTransport return type, the
OAuthProviderLike interface, and any places calling client.connect(transport as
unknown as Transport) (see createTransport, finishAuth, OAuthProviderLike, and
client.connect) to use the combined Transport+SDK-auth-provider types and remove
the cast.
In `@src/cli/run-daemon.ts`:
- Around line 119-130: The call to buildCliInstanceEntry passes launcher which
may be undefined, violating exactOptionalPropertyTypes; update the call site so
launcher is omitted when resolveLauncherHint(processRef.env) returns undefined —
e.g., keep const launcher = resolveLauncherHint(processRef.env) and change the
buildCliInstanceEntry invocation to only include the launcher property when
launcher is defined (use a conditional spread or conditional property inclusion)
so buildCliInstanceEntry/createInstanceRegistration receive an object without a
launcher key instead of launcher: undefined.
- Around line 108-114: The CLI can persist the plaintext secret because
processRef.argv is stored by buildCliInstanceEntry; before calling
buildCliInstanceEntry (and anywhere registry entries are written) sanitize
processRef.argv by producing a redactedArgv that strips or replaces the daemon
secret: detect flags like "--sharedSecret" or "--shared-secret" in both
"--flag=value" and "--flag value" forms and replace the value with "<REDACTED>"
(or replace the entire "--flag=value" token), and use that redactedArgv in place
of processRef.argv when building the entry; update code paths around
resolveDaemonSharedSecret, daemonOptions.sharedSecret and the
buildCliInstanceEntry calls (including the similar block around lines 121-130)
to pass the redacted argv.
In `@src/cli/run-proxy.ts`:
- Around line 57-68: The current call passes launcher even when
resolveLauncherHint(processRef.env) returns undefined which yields launcher:
undefined and violates exactOptionalPropertyTypes; change the creation of the
CLI instance entry so that launcher is only included when defined — e.g. compute
launcher = resolveLauncherHint(processRef.env) then build the object passed to
buildCliInstanceEntry using a conditional spread so that
createInstanceRegistration(buildCliInstanceEntry({... , role: "proxy", ...,
version: VERSION, ...(launcher !== undefined ? { launcher } : {})})) only adds
the launcher property when launcher is not undefined.
In `@src/cli/run-stdio-server.ts`:
- Around line 129-140: The call passes launcher even when
resolveLauncherHint(processRef.env) returns undefined which violates
exactOptionalPropertyTypes; change the call site so that buildCliInstanceEntry
is given launcher only when defined (e.g., compute launcher =
resolveLauncherHint(...) then conditionally include it into the object instead
of always passing the launcher variable) so that
createInstanceRegistration/buildCliInstanceEntry receive no launcher property
when undefined.
In `@src/cli/run-test.ts`:
- Around line 117-127: The top-level validation errors (issues without an
upstream) are being printed but the process still continues; update the
post-validation logic in run-test.ts (around validateConfig, errorUpstreams,
formatValidationIssues) to fail fast: after computing validationIssues, if any
issue has severity === "error" and no upstream (i.e., issue.upstream is falsy),
call process.exit(1) (after printing the formatted issues) so the CLI exits
non-zero for root-level config errors; keep the existing handling for
upstream-specific errors using errorUpstreams unchanged.
---
Nitpick comments:
In `@src/server/capability-surface.ts`:
- Around line 36-42: The help text in buildServerInstructions embeds the literal
"__describe_actions" which can drift from the shared constant; update
buildServerInstructions() to import and use the exported DESCRIBE_ACTION from
capability-tool-surface.ts instead of the hardcoded string, and ensure you add
the import for DESCRIBE_ACTION at the top of the file so the returned
instructions reference the shared DESCRIBE_ACTION identifier.
In `@src/server/capability-tool-surface.ts`:
- Around line 12-16: Remove the duplicate type definition for
CapabilityToolResult in this file and import the shared type instead of
redefining it: replace the local export of CapabilityToolResult with an import
of the exported type from the existing declaration (the CapabilityToolResult
exported from capability-tool-executor) so there is a single canonical contract
referenced by both modules.
In `@tests/run-daemon.test.ts`:
- Around line 196-206: The test currently checks utility functions but should
also assert the default factory functions and their basic behavior: verify that
deps.createDaemon and deps.createRuntime (returned from
createRunDaemonDependencies) are functions, call each factory with minimal
required args to ensure they return objects of the expected shape (e.g., a
daemon with a start/stop or run method and a runtime with expected
methods/properties), and add assertions that these returned objects are non-null
and expose the expected methods; use the existing createRunDaemonDependencies
invocation and reference deps.createDaemon and deps.createRuntime to locate the
code to update.
- Around line 125-127: Replace the fragile microtask wait after invoking the
signal handler with a robust wait that ensures the full async shutdown
completes: after calling handlers.get("SIGINT")?.() wait for the mocked exit to
be called (use vi.waitFor or equivalent) or await a small timeout/flush so the
shutdown sequence (e.g., daemon.stop and cleanup) finishes; reference the test
symbols handlers, "SIGINT", and the exit mock to locate and update the
assertion/wait logic.
- Around line 43-49: The test sets process.env["MCP_SQUARED_DAEMON_SECRET"] in
beforeEach but never restores it; modify the test to save the original value
(e.g., const originalSecret = process.env["MCP_SQUARED_DAEMON_SECRET"] inside
beforeEach or a surrounding scope) and then in afterEach (alongside
mock.restore()) restore it: if originalSecret is undefined delete
process.env["MCP_SQUARED_DAEMON_SECRET"] else set it back to originalSecret;
reference the existing beforeEach and afterEach blocks and the environment
variable name to locate where to add the save/restore logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74de2663-db90-4365-a6ab-ea94bd175a34
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.junie/memory/errors.md.junie/memory/feedback.md.junie/memory/language.json.junie/memory/memory.version.junie/memory/tasks.mdCHANGELOG.mddocs/DEPENDENCY_EXCEPTIONS.mddocs/DEPENDENCY_MAINTENANCE.mddocs/RELEASING.mdpackage.jsonsrc/cli/run-auth.tssrc/cli/run-daemon.tssrc/cli/run-proxy.tssrc/cli/run-stdio-server.tssrc/cli/run-test.tssrc/cli/runtime-bootstrap.tssrc/server/capability-surface.tssrc/server/capability-tool-surface.tssrc/server/index.tstests/daemon-proxy.test.tstests/daemon-server.test.tstests/main-runtime.test.tstests/run-auth.test.tstests/run-daemon.test.tstests/run-management-commands.test.tstests/run-monitor.test.tstests/run-proxy.test.tstests/run-stdio-server.test.tstests/run-test.test.tstests/server-runtime-lifecycle.test.tstests/server-session-surface.test.ts
✅ Files skipped from review due to trivial changes (2)
- .junie/memory/language.json
- .junie/memory/memory.version
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/run-proxy.test.ts
- tests/server-runtime-lifecycle.test.ts
- tests/run-auth.test.ts
- tests/run-stdio-server.test.ts
- tests/main-runtime.test.ts
- tests/run-monitor.test.ts
- tests/run-test.test.ts
- tests/server-session-surface.test.ts
- tests/run-management-commands.test.ts
- src/cli/runtime-bootstrap.ts
Comment on lines
+10
to
+11
| ### Changed | ||
| - Documented the short-term accepted OpenTUI transitive `file-type` advisory in `docs/DEPENDENCY_EXCEPTIONS.md` and tightened the dependency-maintenance/release process so temporary audit exceptions are only allowed when reviewed reachability remains low and local-only. |
There was a problem hiding this comment.
Move this docs/process note into the 0.8.1 section before merge.
These dependency-exception and release-runbook changes are part of the 0.8.1 release patch, but they're left only under ## [Unreleased]. After 0.8.1 is cut, the released notes will be incomplete and the changelog will still describe already-shipped behavior as unreleased. As per coding guidelines, “Always update CHANGELOG.md in the same patch for user-visible changes (features, fixes, behavior changes, CLI/TUI UX changes, and docs that affect usage). Add changelog entries under ## [Unreleased] using Keep a Changelog categories.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 10 - 11, Move the
dependency-exception/release-runbook note out of the "## [Unreleased]" section
and add it under the "## [0.8.1]" release notes within the "### Changed"
category; specifically, remove the existing bullet from Unreleased and paste it
into the 0.8.1 section (keeping the "### Changed" heading and Keep a Changelog
categories formatting intact) so the 0.8.1 release contains the documented
change before merging.
| | Advisory | Package(s) | Severity | Rationale | Controls | Owner | Review By | Linked Issue | | ||
| |---|---|---|---|---|---|---|---| | ||
| | _None_ | - | - | - | - | - | - | - | | ||
| | `file-type` infinite loop in ASF parser (`>=13.0.0 <21.3.1`) | `@opentui/core -> jimp -> @jimp/core -> file-type` | Moderate | Short-term acceptance is limited to MCP²'s local interactive TUI path. `@opentui/core` is only loaded through `src/tui/config-loader.ts` and `src/tui/monitor-loader.ts`, while the main server/proxy/auth/status/test paths do not import it. The reviewed TUI surfaces exchange config text and monitor JSON state, not arbitrary image or binary payloads, so current project-specific reachability is low. A plain Bun override to `file-type@21.x` cleared `bun audit` but proved runtime-incompatible with `@jimp/core@1.6.0`. | Reassess before the review date and on every `@opentui/core`/`jimp` bump; keep TUI imports lazy; do not route untrusted binary/image content through TUI commands without revisiting this exception; prefer upstream remediation over major-version overrides. | Maintainers | 2026-04-12 | Pending maintainer tracking issue | |
There was a problem hiding this comment.
Replace the placeholder with a real tracking issue.
Leaving Linked Issue as Pending maintainer tracking issue makes this exception hard to audit and easy to miss at the next review date.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/DEPENDENCY_EXCEPTIONS.md` at line 7, The table row describing
"`file-type` infinite loop in ASF parser (`>=13.0.0 <21.3.1`)" currently uses
the placeholder "Pending maintainer tracking issue" in the Linked Issue column;
replace that placeholder with a real tracking issue reference (issue URL or
repository/issue ID) and update the status if appropriate so audits can follow
up, locating the row by the unique text "`file-type` infinite loop in ASF
parser" or the dependency chain "`@opentui/core -> jimp -> `@jimp/core` ->
file-type`" and ensuring the Linked Issue cell contains a clickable URL/issue
identifier and optional brief status (e.g., "Open: https://.../issues/1234").
| ``` | ||
| This includes `bun run audit` as a hard gate and verifies `dist/index.js` has no unresolved `@/...` runtime imports. | ||
| If an advisory is temporarily accepted, record it in `docs/DEPENDENCY_EXCEPTIONS.md` and link the tracking issue before proceeding. | ||
| If an advisory is temporarily accepted, record it in `docs/DEPENDENCY_EXCEPTIONS.md`, verify the reviewed reachability is still low for the current release, and link the tracking issue before proceeding. |
There was a problem hiding this comment.
Replace the hard-coded 0.7.0 examples before cutting 0.8.1.
This checklist still tells maintainers to run bun pm version 0.7.0, push v0.7.0, and verify v0.7.0. Since this PR is the 0.8.1 release patch, the runbook is easy to follow incorrectly. Please update those examples in this patch or switch them to <version> / v<version> placeholders to avoid drift next time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/RELEASING.md` at line 41, Update the hard-coded example version strings
in RELEASING.md: replace occurrences of "0.7.0", "v0.7.0", and any commands like
`bun pm version 0.7.0`/`push v0.7.0` with either the current release
"0.8.1"/"v0.8.1" or, preferably, a generic placeholder format
`<version>`/`v<version>` so the runbook won't drift; ensure you update all
matching examples and command snippets in the file (search for "0.7.0" and
"v0.7.0") and keep the command examples consistent like `bun pm version
<version>` and `push v<version>`.
Comment on lines
+59
to
+68
| const registration = createInstanceRegistration( | ||
| buildCliInstanceEntry({ | ||
| configPath, | ||
| id: `proxy-${processRef.pid}`, | ||
| launcher, | ||
| processRef, | ||
| role: "proxy", | ||
| socketPath: monitorSocketPath, | ||
| version: VERSION, | ||
| }), |
There was a problem hiding this comment.
Redact --sharedSecret before building the proxy instance entry.
buildCliInstanceEntry() persists processRef.argv.join(" "), so a CLI-provided daemon secret will end up in the registry in plaintext.
Also applies to: 110-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/run-proxy.ts` around lines 59 - 68, The CLI daemon secret in
processRef.argv is being persisted because buildCliInstanceEntry() stores
processRef.argv.join(" "), so before calling buildCliInstanceEntry (used inside
createInstanceRegistration) sanitize processRef.argv by removing or redacting
any '--sharedSecret' flag and its value; e.g., make a shallow copy of processRef
(or argv) and replace the secret value with '<redacted>' or remove both the flag
and following value, then pass the sanitized processRef (or sanitized argv) into
buildCliInstanceEntry; apply the same redaction logic for the other occurrence
around the code that builds the proxy instance entry (lines referenced in the
comment).
Comment on lines
+257
to
+318
| const exactRoute = liveRouter.actions.find( | ||
| (entry) => | ||
| entry.action === parsedRequest.action || | ||
| (entry.legacyActions ?? []).includes(parsedRequest.action), | ||
| ); | ||
|
|
||
| const ambiguousCandidates = visibleRoutes | ||
| .filter(({ route }) => route.baseAction === parsedRequest.action) | ||
| .map(({ route }) => route.action) | ||
| .sort((a, b) => a.localeCompare(b)); | ||
|
|
||
| if (ambiguousCandidates.length > 1) { | ||
| return buildCapabilityToolResponse( | ||
| { | ||
| requires_disambiguation: true, | ||
| capability, | ||
| action: parsedRequest.action, | ||
| candidates: ambiguousCandidates, | ||
| }, | ||
| true, | ||
| ); | ||
| } | ||
|
|
||
| const selectedRoute = | ||
| exactRoute ?? | ||
| (ambiguousCandidates.length === 1 | ||
| ? visibleRoutes.find( | ||
| ({ route }) => route.baseAction === parsedRequest.action, | ||
| )?.route | ||
| : undefined); | ||
|
|
||
| if (selectedRoute == null) { | ||
| return buildCapabilityToolResponse( | ||
| { | ||
| error: "Unknown action", | ||
| capability, | ||
| action: parsedRequest.action, | ||
| availableActions: visibleActions.map( | ||
| (action) => action.action, | ||
| ), | ||
| }, | ||
| true, | ||
| ); | ||
| } | ||
|
|
||
| const callResult = await executeRoute({ | ||
| capability, | ||
| action: selectedRoute.action, | ||
| policyAction: | ||
| exactRoute != null | ||
| ? parsedRequest.action | ||
| : selectedRoute.action, | ||
| routeId: | ||
| selectedRoute.canonicalRouteId ?? | ||
| `${capability}:${selectedRoute.action}`, | ||
| qualifiedToolName: selectedRoute.qualifiedName, | ||
| toolNameForCall: selectedRoute.qualifiedName, | ||
| args: parsedRequest.arguments, | ||
| ...(parsedRequest.confirmationToken != null | ||
| ? { confirmationToken: parsedRequest.confirmationToken } | ||
| : {}), | ||
| }); |
There was a problem hiding this comment.
Resolve actions from visibleRoutes, not liveRouter.actions.
exactRoute currently searches every live route, so an action filtered out by getVisibleCapabilityRoutes() is still directly callable by name or legacy alias. That makes the visibility check informational instead of authoritative.
[suggested fix]
Limit exact matches to visible routes
- const exactRoute = liveRouter.actions.find(
- (entry) =>
- entry.action === parsedRequest.action ||
- (entry.legacyActions ?? []).includes(parsedRequest.action),
- );
+ const exactRoute = visibleRoutes.find(
+ ({ route }) =>
+ route.action === parsedRequest.action ||
+ (route.legacyActions ?? []).includes(parsedRequest.action),
+ )?.route;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/capability-tool-surface.ts` around lines 257 - 318, exactRoute
currently uses liveRouter.actions so routes filtered out by
getVisibleCapabilityRoutes() remain callable; change exactRoute to search only
visibleRoutes (e.g., inspect each visibleRoutes item for route.action and
route.legacyActions) instead of liveRouter.actions, update any uses of
exactRoute selection logic to derive from visibleRoutes.find(...) (or
equivalent) so both exact and legacy alias matches honor visibility before
proceeding to ambiguousCandidates/selectedRoute and executeRoute.
Comment on lines
+464
to
+477
| const originalStart = SocketClientTransport.prototype.start; | ||
| let startCalls = 0; | ||
| const startSpy = spyOn( | ||
| SocketClientTransport.prototype, | ||
| "start", | ||
| ).mockImplementation(function ( | ||
| this: SocketClientTransport, | ||
| ): Promise<void> { | ||
| startCalls += 1; | ||
| if (startCalls === 1) { | ||
| return Promise.reject(new Error("Transient reconnect failure")); | ||
| } | ||
| return originalStart.call(this); | ||
| }); |
There was a problem hiding this comment.
Avoid patching SocketClientTransport.prototype.start process-wide.
This spy affects every daemon transport in the process until the finally runs. If another test file is exercising reconnect logic at the same time, it can inherit the injected failure and flake.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/daemon-proxy.test.ts` around lines 464 - 477, The test currently
patches SocketClientTransport.prototype.start (originalStart, startCalls,
startSpy) which mutates behavior process-wide; instead create or obtain the
specific SocketClientTransport instance used in this test and spy on that
instance's start method (e.g., spyOn(instance, "start")) so the injected
transient failure only affects this transport; restore the original
implementation in the test teardown/finally and keep the same retry logic
(reject once then delegate to the original instance method) but reference the
instance method rather than the prototype to avoid leaking failures to other
tests.
Comment on lines
+560
to
+575
| process.env["MCP_CLIENT_NAME"] = "proxy-concurrent-recovery-1"; | ||
| const firstBridge = await createProxyBridge({ | ||
| stdioTransport: firstProxyTransport, | ||
| endpoint: firstDaemon.getSocketPath(), | ||
| configHash, | ||
| heartbeatIntervalMs: 50, | ||
| spawnDaemon: spawnReplacementDaemon, | ||
| }); | ||
| process.env["MCP_CLIENT_NAME"] = "proxy-concurrent-recovery-2"; | ||
| const secondBridge = await createProxyBridge({ | ||
| stdioTransport: secondProxyTransport, | ||
| endpoint: firstDaemon.getSocketPath(), | ||
| configHash, | ||
| heartbeatIntervalMs: 50, | ||
| spawnDaemon: spawnReplacementDaemon, | ||
| }); |
There was a problem hiding this comment.
Restore MCP_CLIENT_NAME after each test.
These tests mutate process.env and never roll it back. Because the env object is process-global, leaked client names can bleed into unrelated bridge/launcher assertions outside this describe block.
Also applies to: 654-677, 762-785, 919-942, 1090-1113, 1280-1303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/daemon-proxy.test.ts` around lines 560 - 575, The tests mutate the
global process.env["MCP_CLIENT_NAME"] (seen around the createProxyBridge calls
that produce firstBridge and secondBridge) and never restore it; save the
original value (e.g., const prev = process.env["MCP_CLIENT_NAME"]) before
overriding and restore it after the test (either in an afterEach/try...finally
or immediately after creating the bridges) so the global env is returned to its
prior state; apply the same pattern for other occurrences (the blocks that set
MCP_CLIENT_NAME and call createProxyBridge/spawnReplacementDaemon) to prevent
leaked client names across tests.
Comment on lines
+330
to
+340
| createSessionServer() { | ||
| const sessionIndex = closeCount; | ||
| return { | ||
| async connect() {}, | ||
| async close() { | ||
| closeCount += 1; | ||
| closedSessionIds.push(`session-${sessionIndex}`); | ||
| if (sessionIndex === 0) { | ||
| await closingGate.promise; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Use a creation counter for the delayed-close gate.
sessionIndex is derived from closeCount, so every session created before the first close captures 0. That means these tests can stall more than the stale session they intend to model, which weakens the scenario they are asserting.
Also applies to: 435-445, 536-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/daemon-server.test.ts` around lines 330 - 340, The test's
createSessionServer uses sessionIndex = closeCount so multiple sessions created
before any close all capture 0; replace this with a creation counter (e.g.,
createCount) that you increment each time createSessionServer is called and
capture into sessionIndex so each created session gets a unique index; update
uses of sessionIndex in the close() logic (and related arrays like
closedSessionIds and the closingGate handling) so only the intended session
waits on closingGate. Also apply the same creation-counter fix to the other
occurrences noted (around the blocks corresponding to the other two ranges).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
0.8.1Verification
bun testbun run buildbun run lintbun run coverage:checkCo-authored-by: Junie junie@jetbrains.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation