feat: complete 0.9 runtime supervisor with leases, health tracking, adapters, and middleware - #25
Merged
Merged
Conversation
Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Phase 2 of the 0.9 runtime-supervisor plan: - Thread daemon/proxy client identity into RuntimeCallContext via sessionId/agentId. - Pass identity from shared daemon/proxy path down to Cataloger.callTool(). - Preserve current behavior for direct stdio mode where no identity exists. - Add focused tests for session-affine serialization, agent-based locking, and no-context parallelism. - Update docs/RUNTIME-SUPERVISOR.md and CHANGELOG.md. Co-authored-by: Codex <codex@openai.com>
…dapters, and middleware Implements the remaining 0.9 runtime supervisor work items: - Agent lease system (AgentLeaseManager) for temporary exclusive access to singleton stdio servers with configurable TTL and automatic expiry - Runtime health tracking (HealthTracker) for upstream health state, restart counts, and audit events with JSON persistence - Runtime adapter system (UpstreamAdapter interface) with built-in stdio/HTTP adapters and adapter registry - mcporter SDK adapter factory for wrapping generated typed SDKs - Proxy middleware system (ProxyMiddleware) with session affinity, request logging, retry with exponential backoff, and auth injection Co-authored-by: MiMoCode Agent <mimo@xiaomi.com>
|
Warning Review limit reached
More reviews will be available in 24 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThe PR updates project status and docs, adds runtime policy and supervision infrastructure, splits CLI and server orchestration into modules, introduces upstream adapters and daemon recovery, and expands tests across the new execution and lifecycle flows. ChangesRuntime supervisor foundation
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: b6325a4898
ℹ️ 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
+86
to
+90
| lifecycle: RuntimeLifecycleModeSchema.optional(), | ||
| concurrency: RuntimeConcurrencyModeSchema.optional(), | ||
| maxPoolSize: z.number().int().min(1).optional(), | ||
| restart: RuntimeRestartPolicySchema.optional(), | ||
| lease: RuntimeLeaseSchema.optional(), |
There was a problem hiding this comment.
The schema now accepts lifecycle, maxPoolSize, restart, and lease, and status --verbose reports those values, but production execution only uses runtime.concurrency: McpSquaredServer still constructs a plain new Cataloger(), Cataloger.connect() creates exactly one SDK client/transport per upstream, and Cataloger.callTool() only passes calls through UpstreamCallSupervisor. In any config that sets lifecycle = "ephemeral"/"pooled", maxPoolSize, restart, or lease.enabled, the CLI will accept and display the policy while continuing to use the existing singleton connection with no restart or lease behavior.
Useful? React with 👍 / 👎.
Comment on lines
+484
to
+487
| healthTracking: HealthTrackingSchema.default({ | ||
| enabled: false, | ||
| maxAuditEvents: 1000, | ||
| }), |
There was a problem hiding this comment.
Setting [operations.healthTracking] enabled = true is now valid config, but no production path constructs a HealthTracker from this section or passes it to UpstreamCallSupervisor (the default server path creates new Cataloger() with no tracker). As a result, users can enable the advertised audit/health persistence and still get no persisted health state or tool-call audit events.
Useful? React with 👍 / 👎.
Comment on lines
+139
to
+147
| if (options.configHash) { | ||
| const registry = await loadLiveDaemonRegistry(options.configHash); | ||
| if (registry) { | ||
| sharedSecret ??= registry.sharedSecret; | ||
| endpoint = registry.endpoint; | ||
| return registry.endpoint; | ||
| } | ||
|
|
||
| if (allowSpawn && !options.noSpawn) { |
There was a problem hiding this comment.
When callers use the exported default runProxy()/createProxyBridge() path without passing either endpoint or configHash, the initial connectDaemon(true) reaches resolveDaemonTarget(true) but the spawn branch is nested under if (options.configHash), so it falls through to Daemon endpoint not available. The previous implementation spawned a daemon whenever no endpoint was found and noSpawn was false, so embedders using the default proxy API now fail before starting a daemon.
Useful? React with 👍 / 👎.
- Update smol-toml, hono, @hono/node-server, yaml to patched versions - Add dependency exceptions for documented transitive vulnerabilities (protobufjs, hono, fast-uri, qs, ip-address, path-to-regexp) - Configure bun audit to ignore documented GHSA IDs Co-authored-by: MiMoCode Agent <mimo@xiaomi.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Tests were setting process.env['MCP_CLIENT_NAME'] which is global state, causing race conditions when tests run in parallel. Added explicit clientName option to ProxyBridgeOptions so tests pass client identity directly without mutating environment variables. Co-authored-by: MiMoCode Agent <mimo@xiaomi.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (2)
src/cli/run-stdio-server.ts-150-164 (1)
150-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the forced-exit timer after shutdown completes.
forceExitTimeris never cleared. IfprocessRef.exitis mocked or non-terminating, the timer can still fire and issue a secondexit(1).Suggested fix
- try { - const forceExitTimer = setTimeout(() => { + const forceExitTimer = setTimeout(() => { console.error("Forcing shutdown after timeout"); processRef.exit(1); - }, 2000); - forceExitTimer.unref(); - + }, 2000); + forceExitTimer.unref(); + try { await server.stop(); registration.unregisterInstance(); + clearTimeout(forceExitTimer); processRef.exit(exitCode); } catch (err) { const message = err instanceof Error ? err.message : String(err); console.error(`Error during shutdown: ${message}`); registration.unregisterInstance(); + clearTimeout(forceExitTimer); processRef.exit(1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/run-stdio-server.ts` around lines 150 - 164, The forced-exit timer (forceExitTimer) is never cleared and can fire after a successful shutdown; before calling processRef.exit(exitCode) in the try block clear it (clearTimeout(forceExitTimer)), and also clear it in the catch block before calling processRef.exit(1); update the shutdown flow around await server.stop(), registration.unregisterInstance(), and both exit paths to ensure forceExitTimer is cleared to prevent a late forced exit.src/runtime/health-tracker.ts-196-203 (1)
196-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn snapshots instead of live
UpstreamHealthobjects.
getHealth()andgetAllHealth()currently expose the same mutable objects stored inthis.upstreams, so any caller can accidentally rewrite counters or status and corrupt later persistence. Returning cloned snapshots avoids external mutation of tracker state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/health-tracker.ts` around lines 196 - 203, getHealth and getAllHealth currently return live UpstreamHealth objects from this.upstreams; modify them to return cloned snapshots to prevent external mutation. In getHealth(upstreamKey) return a copy of the stored object (e.g., clone the value retrieved from this.upstreams.get and return null if missing) and in getAllHealth() return Array.from(this.upstreams.values()).map(v => /* clone v */) so callers receive new objects (use a shallow/deep clone appropriate for UpstreamHealth fields, e.g., object spread + cloning nested counters or JSON.parse(JSON.stringify(...)) if safe). Ensure you reference getHealth, getAllHealth and this.upstreams when making the changes.
🧹 Nitpick comments (6)
tests/server-handlers.test.ts (1)
353-353: ⚡ Quick winUse explicit non-error state for success assertions.
Line 353 asserts
isErrorisundefined; this weakens the success contract. Assertfalseto keep success/error state unambiguous.Proposed assertion update
- expect(describeResult.isError).toBeUndefined(); + expect(describeResult.isError).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server-handlers.test.ts` at line 353, The test currently asserts describeResult.isError is undefined which is ambiguous; update the assertion to explicitly assert success by checking describeResult.isError === false so the success/error contract is unambiguous—locate the test where describeResult is used (the failing expectation line with expect(describeResult.isError).toBeUndefined()) and change it to assert false instead.tests/adapters.test.ts (1)
50-53: ⚡ Quick winStrengthen the process.env fallback assertion.
Line 50-Line 53 only checks for
"/test"suffix, which can pass even if fallback resolution is broken. Use a temporary unique env var and assert exact replacement.Proposed test tightening
test("falls back to process.env", () => { - const result = resolveEnvVars("$PATH/test", {}); - expect(result).toContain("/test"); + const prev = process.env.MCP2_TEST_FALLBACK; + process.env.MCP2_TEST_FALLBACK = "/tmp/mcp2-bin"; + try { + const result = resolveEnvVars("$MCP2_TEST_FALLBACK/test", {}); + expect(result).toBe("/tmp/mcp2-bin/test"); + } finally { + if (prev === undefined) { + delete process.env.MCP2_TEST_FALLBACK; + } else { + process.env.MCP2_TEST_FALLBACK = prev; + } + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/adapters.test.ts` around lines 50 - 53, The test for resolveEnvVars should use a unique temporary environment variable instead of relying on PATH; set a unique key in process.env (e.g., UNIQUE_TEST_ENV) to a known value, call resolveEnvVars("$UNIQUE_TEST_ENV/test", {}), and assert the returned string equals the exact concatenation of that env value and "/test"; ensure the test cleans up the temporary process.env entry afterwards. This targets the resolveEnvVars function and the test named "falls back to process.env".tests/run-proxy.test.ts (1)
98-98: ⚡ Quick winRename this test to match the asserted behavior.
The title says startup failure unregisters the proxy instance, but Line 130 asserts the opposite (
deleteInstanceEntryis not called). Please align the name with the assertion to avoid future confusion.Proposed rename
- test("unregisters the proxy instance if startup fails", async () => { + test("does not unregister the proxy instance when startup fails before registration", async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/run-proxy.test.ts` at line 98, The test title is misleading: it currently reads "unregisters the proxy instance if startup fails" but the assertion checks that deleteInstanceEntry is NOT called; update the test name string in the test("...") for the test in tests/run-proxy.test.ts (the test that references deleteInstanceEntry and asserts it was not called) to accurately reflect the behavior, e.g. "does not unregister the proxy instance if startup fails" or "does not call deleteInstanceEntry when startup fails".src/config/schema.ts (1)
178-228: 💤 Low valueConsider simplifying nested lease config resolution.
The lease configuration resolution on lines 222-225 could be flattened for readability, though the current implementation is correct.
♻️ Optional simplification
lease: { - enabled: config.runtime?.lease?.enabled ?? defaults.lease.enabled, - maxDurationMs: - config.runtime?.lease?.maxDurationMs ?? defaults.lease.maxDurationMs, + enabled: config.runtime?.lease?.enabled ?? defaults.lease.enabled, + maxDurationMs: config.runtime?.lease?.maxDurationMs ?? defaults.lease.maxDurationMs, },Or consider using a helper:
lease: { ...defaults.lease, ...(config.runtime?.lease ?? {}), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/schema.ts` around lines 178 - 228, The lease resolution in resolveUpstreamRuntimeDefaults is correct but can be simplified for readability: replace the explicit field-by-field merge of lease (currently using config.runtime?.lease?.enabled and maxDurationMs with defaults.lease.*) with a shallow merge that spreads defaults.lease first and then overrides with config.runtime?.lease (e.g., lease: { ...defaults.lease, ...(config.runtime?.lease ?? {}) }); this keeps behavior identical while flattening logic referencing DEFAULT_STDIO_RUNTIME_CONFIG/DEFAULT_SSE_RUNTIME_CONFIG and config.runtime?.lease.src/upstream/adapters/stdio.ts (1)
58-77: 💤 Low valueHardcoded
Bun.sleep(100)limits portability and lacks justification.Line 63 uses
Bun.sleep(100), which:
- Ties the code to the Bun runtime (not portable to Node.js)
- Uses an arbitrary 100ms delay without explanation
If cross-runtime compatibility is needed, consider a platform-agnostic sleep helper. The delay duration should also be documented or made configurable.
♻️ Platform-agnostic sleep helper
// Add platform-agnostic sleep utility const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); async function postStdioConnect( _context: AdapterContext, client: Client, ): Promise<PostConnectResult> { // Give the process a moment to fail if it's going to (100ms chosen empirically) await sleep(100); // ... rest of function }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/upstream/adapters/stdio.ts` around lines 58 - 77, The postStdioConnect function currently uses Bun.sleep(100), which couples the code to Bun and hides why 100ms was chosen; replace Bun.sleep(100) with a platform-agnostic sleep helper (e.g., const sleep = (ms:number)=>new Promise(r=>setTimeout(r,ms))) and call await sleep(delayMs) inside postStdioConnect instead of Bun.sleep; make delayMs configurable (via parameter, constant, or config value) and document the chosen default (e.g., 100ms) where the helper or postStdioConnect is defined so the runtime is portable and the timeout rationale is clear.src/status/runner.ts (1)
53-53: 💤 Low valueRedundant
| undefinedwith optional operator.The
?operator already makes the fieldundefinedwhen absent, so| undefinedis redundant (though not incorrect).♻️ Simplification
- runtime?: EffectiveUpstreamRuntimeConfig | undefined; + runtime?: EffectiveUpstreamRuntimeConfig;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/status/runner.ts` at line 53, The field declaration runtime?: EffectiveUpstreamRuntimeConfig | undefined redundantly unions with undefined; remove the "| undefined" so the property reads runtime?: EffectiveUpstreamRuntimeConfig to rely on the optional operator. Update the declaration in the same interface/type where runtime is defined (reference symbol: runtime) and run a quick TypeScript typecheck to ensure no other code relied on the explicit union.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/DEPENDENCY_EXCEPTIONS.md`:
- Line 7: The exception entry for the `file-type` infinite loop is overdue;
update the `Review By` date in DEPENDENCY_EXCEPTIONS.md (the table row
referencing `@opentui/core -> jimp -> `@jimp/core` -> file-type`) to a new future
review deadline, replace "Pending maintainer tracking issue" with a concrete
issue/PR reference or tracker ID, and add a short note in the same table cell
stating who is responsible for follow-up (e.g., maintainer GitHub handle or
team) and the action expected (reassess on `@opentui/core` or `jimp` bumps and
before routing untrusted binaries through TUI).
In `@src/cli/main-runtime.ts`:
- Around line 54-55: The isStderrTty property is incorrectly wired to
process.stdout.isTTY; update the assignment so isStderrTty reads
process.stderr.isTTY instead to reflect the actual stderr TTY state (modify the
object/initializer where isStderrTty and isStdinTty are set in main-runtime.ts).
In `@src/cli/run-auth.ts`:
- Around line 251-255: The callback currently only checks state when
result.state is present, allowing missing state to bypass CSRF protection;
update the validation in the callback handler (where result.state and
authProvider.verifyState are used) to treat a missing state as an error: if
result.state is falsy OR authProvider.verifyState(result.state) returns false,
log the OAuth state mismatch and exit (processRef.exit(1)) just as when
verification fails.
In `@src/cli/run-daemon.ts`:
- Around line 105-107: The registry entry currently always uses
monitorSocketPath instead of the effective daemon socket path; update the
instance registration to use the final socket path value from daemonOptions
(taking into account options.socketPath override) rather than the original
monitorSocketPath. Locate the code that builds the instance/registry entry (the
block that references monitorSocketPath) and replace that field with the
resolved socket path from daemonOptions.socketPath (or the variable that holds
the effective daemon socket) so consumers get the correct endpoint.
In `@src/cli/runtime-bootstrap.ts`:
- Around line 93-113: The shutdown hook can be invoked multiple times
(SIGINT/SIGTERM and stdin 'end'/'close'), so wrap calls to shutdown in a
one-time guard: inside registerShutdownHooks create a local boolean (e.g.,
"shutdownCalled") and a small wrapper function (e.g., "invokeShutdown(code)")
that returns immediately if shutdownCalled is true, sets it true on first call,
then calls void shutdown(code); replace all direct shutdown(...) calls (the
signal handlers and stdin handlers) with this wrapper so cleanup runs only once;
alternatively use processRef.once for signals but still protect stdin handlers
with the same one-time guard to avoid duplicate teardown.
In `@src/daemon/proxy.ts`:
- Around line 325-337: The stdioTransport.onmessage handler currently drops
messages when daemonTransport is null or send() rejects; change it to buffer
outbound messages (e.g., in a queue) while daemon is disconnected and only
drain/flush the queue after connectDaemon()/reconnectDaemon() completes
successfully, or alternatively fail the stdio side immediately on permanent
errors so callers can retry. Specifically, introduce a message queue and enqueue
messages inside stdioTransport.onmessage when daemonTransport is falsy or when
activeTransport.send() rejects; trigger reconnectDaemon() as before but do not
discard messages, and on successful connectDaemon()/reconnectDaemon() drain the
queue by calling daemonTransport.send() for each item (with appropriate
backpressure/retry/failure handling that will respond to stdio if sends
permanently fail). Ensure logic still respects stopping, stdioClosed, and
canRecoverDaemon flags and use existing functions/variables:
stdioTransport.onmessage, daemonTransport, reconnectDaemon, connectDaemon,
activeTransport.send, stopping, stdioClosed, canRecoverDaemon.
In `@src/daemon/server.ts`:
- Around line 345-349: The code is trusting caller-supplied session.clientId and
promotes it to runtime identity and eviction logic; instead stop using
session.clientId as agentId in getRuntimeCallContext and eviction; bind agent
identity to a server-issued/authenticated value stored on the server-side
session (e.g., session.agentId or session.authenticatedAgentId populated during
authentication when the session is created/validated) and use that for
sessionServer/createSessionServer getRuntimeCallContext and any eviction logic;
remove any use of the incoming message payload (session.clientId) for identity
or eviction (also update the logic in the related block around the 402-421
range) so only server-authenticated identifiers determine agent identity and
session-affinity.
In `@src/runtime/health-tracker.ts`:
- Around line 237-246: Parse the file content into unknown instead of casting to
HealthStore, validate that the top-level object has version === 1, that
store.upstreams is an object whose entries have the expected Health shape before
calling this.upstreams.set, and that store.auditEvents is an array of valid
audit event objects (each containing a numeric id); only adopt validated entries
into this.upstreams and this.auditEvents. Replace the current eventIdCounter =
this.auditEvents.length with logic that computes the next id as (max(existing
audit event id) + 1) or 1 if none. Update places referenced in this file (e.g.,
the variables store, this.upstreams, this.auditEvents, this.eventIdCounter and
methods getAuditEvents()/save()) to rely on the validated data. Ensure failures
in validation bail out without mutating internal state and log or throw a clear
error so corrupted health.json is not adopted.
In `@src/runtime/supervisor.ts`:
- Around line 72-74: The current run() path skips lease enforcement when
context.agentId is missing and getLockKey() can return null for session_affine
without identity, allowing concurrent access; modify run() and
runLeaseAware()/getLockKey() so a missing identity is treated as a non-holder
(i.e., still subject to lease waits) and when session_affine yields neither
sessionId nor agentId, return an upstream-wide lockKey instead of null; update
logic around this.leaseManager, Supervisor.run(), Supervisor.runLeaseAware(),
and getLockKey()/session_affine handling to enforce leases and fall back to
upstreamKey when no identity exists.
In `@src/server/capability-tool-executor.ts`:
- Around line 42-44: The current check returns any object with a "type" field as
text by force-casting "entry" to { type: "text"; text: string }, which lets
non-text blocks and invalid text values leak through; update the condition to
explicitly verify entry.type === "text" and that typeof entry.text === "string"
(and entry !== null), then return entry cast to the text shape only in that
case, otherwise return the non-text path (e.g., undefined/null or fallthrough)
so the function's text-only contract is preserved.
In `@src/server/capability-tool-surface.ts`:
- Around line 276-280: The code uses parsedRequest.action or legacy aliases as
the forwarded policyAction which lets alias names bypass canonical policy
checks; find where exactRoute is computed (the liveRouter.actions find using
parsedRequest.action) and change the policyAction to use the canonical route
action (exactRoute.action) instead of parsedRequest.action or its aliases, with
a safe fallback to parsedRequest.action only if exactRoute is undefined; update
the forwarding site that sets policyAction (the code around the policyAction
variable/argument at the block forwarding lines ~325-328) to reference
exactRoute.action.
In `@src/server/server-shell.ts`:
- Around line 31-39: The session lifecycle can leak core runtime state if
args.server.connect throws or if args.server.close fails: wrap the connect
sequence (calls to startCore, registerConfiguredSessionSurface, new
StdioServerTransport, args.server.connect, and
args.statsCollector.incrementActiveConnections) in try/catch/finally so that a
failed connect will call args.stopCore() and undo any partial registration;
likewise, wrap shutdown (args.server.close and
args.statsCollector.decrementActiveConnections) so that a failed close still
calls args.stopCore(), and ensure stats increment/decrement are only applied on
successful connect/close (use try { await args.server.connect(...) ;
args.statsCollector.incrementActiveConnections(); } catch (e) { await
args.stopCore(); throw e } and a similar pattern around args.server.close to
always await args.stopCore() in a finally block).
In `@src/upstream/adapters/stdio.ts`:
- Around line 24-52: The merge of process.env with resolvedEnv in
createStdioTransport uses an unsafe cast to Record<string,string> which can
allow undefined values into params.env; replace that cast by building a safe env
object (for example merge process.env and resolvedEnv into a plain object then
filter out entries whose value is undefined) and pass that filtered
Record<string,string> to StdioClientTransport via params.env so
StdioClientTransport never receives undefined env values.
In `@src/upstream/cataloger.ts`:
- Around line 634-653: The thunk passed to callSupervisor.run currently closes
over the outer `client` and `connection`, which can become stale; update the
lambda passed to `this.callSupervisor.run(...)` to re-resolve the live
connection from `this.connections.get(result.tool.serverKey)` and fetch
`connection.client` inside the callback, check `connection` exists and
`connection.status === "connected"` (and that `client` is present), and if not,
throw/return an appropriate error to abort the call; keep usage of
`parseQualifiedName`/`bareToolName`, `args`, and `context` the same but ensure
the actual remote call uses the re-read `connection.client` rather than the
outer `client`.
In `@src/upstream/client.ts`:
- Around line 345-358: The retry is still racing client.connect(transport)
against the original timeoutPromise created before the OAuth flow, so recreate a
fresh timeout promise for the post-OAuth connect attempt; before awaiting
Promise.race([client.connect(transport!), timeoutPromise]) in the retry block,
build a new timeoutPromise (using the same timeoutMs/timeout creation logic used
originally) so that each connect() call races against a newly-started timeout;
locate this in the retry branch around safelyCloseTransport/
httpTransportFactory/transport and replace the reused timeoutPromise with the
newly created one.
In `@src/upstream/proxy-middleware.ts`:
- Around line 204-214: The retry detection currently in the middleware fn
(checking ctx.response?.error, retryCount, maxRetries and setting
ctx.state["shouldRetry"]/["retryDelayMs"]) only runs in the "after_call" phase
per options.phases; register this middleware for the error phase as well or move
the retry-detection logic into the on_error handler so transport failures set
ctx.state["shouldRetry"] and ctx.state["retryDelayMs"]. Update the
options.phases to include "on_error" (or relocate the fn's logic to the on_error
hook) so functions using on_error participate in backoff, keeping the same
checks for retryCount, maxRetries, baseDelayMs and state keys.
- Around line 130-145: The sessionMap used by the "session-affinity" middleware
(symbol: sessionMap, middleware name "session-affinity", handler fn) never
evicts entries causing unbounded memory growth; implement eviction by either
replacing sessionMap with an LRU/TTL cache (use an LRU cache library or a Map
keyed to sessionId with stored timestamps and a max size/ttl) and run periodic
cleanup, or add a teardown hook tied to session lifecycle (e.g., remove
sessionMap entry when ctx.callContext signals session end) so entries are
removed when sessions terminate; ensure lookups still use
sessionMap.get(sessionId) and updates use sessionMap.set(sessionId,
ctx.upstreamKey).
In `@tests/daemon-proxy.test.ts`:
- Around line 403-411: Replace the fixed 250ms sleep with a condition-based wait
that polls until the reconnection is observed: after calling await
secondDaemon.start(), loop or use a waitFor helper to repeatedly call await
client.listTools() (or reuse recoveredTools logic) and check that the returned
tool names include "time_util", with a sensible timeout (e.g., few seconds) and
short interval between attempts; update the block around firstDaemon,
secondDaemon, and client.listTools() so the test only proceeds when the
recovered tool condition is met instead of awaiting a hardcoded setTimeout.
- Around line 560-570: The test mutates process.env["MCP_CLIENT_NAME"] multiple
times (e.g., around the createProxyBridge calls) without restoring it, risking
cross-test leakage; update each mutation site (the assignments before
createProxyBridge invocations) to capture the previous value, set the new value
for the duration of that setup, and then restore the original value after the
bridge is created (or use try/finally) so MCP_CLIENT_NAME is returned to its
prior value; references to update include the MCP_CLIENT_NAME env key and the
createProxyBridge usages in the test file so you can locate and wrap each
mutation (also apply the same restore pattern at the other listed locations).
---
Minor comments:
In `@src/cli/run-stdio-server.ts`:
- Around line 150-164: The forced-exit timer (forceExitTimer) is never cleared
and can fire after a successful shutdown; before calling
processRef.exit(exitCode) in the try block clear it
(clearTimeout(forceExitTimer)), and also clear it in the catch block before
calling processRef.exit(1); update the shutdown flow around await server.stop(),
registration.unregisterInstance(), and both exit paths to ensure forceExitTimer
is cleared to prevent a late forced exit.
In `@src/runtime/health-tracker.ts`:
- Around line 196-203: getHealth and getAllHealth currently return live
UpstreamHealth objects from this.upstreams; modify them to return cloned
snapshots to prevent external mutation. In getHealth(upstreamKey) return a copy
of the stored object (e.g., clone the value retrieved from this.upstreams.get
and return null if missing) and in getAllHealth() return
Array.from(this.upstreams.values()).map(v => /* clone v */) so callers receive
new objects (use a shallow/deep clone appropriate for UpstreamHealth fields,
e.g., object spread + cloning nested counters or JSON.parse(JSON.stringify(...))
if safe). Ensure you reference getHealth, getAllHealth and this.upstreams when
making the changes.
---
Nitpick comments:
In `@src/config/schema.ts`:
- Around line 178-228: The lease resolution in resolveUpstreamRuntimeDefaults is
correct but can be simplified for readability: replace the explicit
field-by-field merge of lease (currently using config.runtime?.lease?.enabled
and maxDurationMs with defaults.lease.*) with a shallow merge that spreads
defaults.lease first and then overrides with config.runtime?.lease (e.g., lease:
{ ...defaults.lease, ...(config.runtime?.lease ?? {}) }); this keeps behavior
identical while flattening logic referencing
DEFAULT_STDIO_RUNTIME_CONFIG/DEFAULT_SSE_RUNTIME_CONFIG and
config.runtime?.lease.
In `@src/status/runner.ts`:
- Line 53: The field declaration runtime?: EffectiveUpstreamRuntimeConfig |
undefined redundantly unions with undefined; remove the "| undefined" so the
property reads runtime?: EffectiveUpstreamRuntimeConfig to rely on the optional
operator. Update the declaration in the same interface/type where runtime is
defined (reference symbol: runtime) and run a quick TypeScript typecheck to
ensure no other code relied on the explicit union.
In `@src/upstream/adapters/stdio.ts`:
- Around line 58-77: The postStdioConnect function currently uses
Bun.sleep(100), which couples the code to Bun and hides why 100ms was chosen;
replace Bun.sleep(100) with a platform-agnostic sleep helper (e.g., const sleep
= (ms:number)=>new Promise(r=>setTimeout(r,ms))) and call await sleep(delayMs)
inside postStdioConnect instead of Bun.sleep; make delayMs configurable (via
parameter, constant, or config value) and document the chosen default (e.g.,
100ms) where the helper or postStdioConnect is defined so the runtime is
portable and the timeout rationale is clear.
In `@tests/adapters.test.ts`:
- Around line 50-53: The test for resolveEnvVars should use a unique temporary
environment variable instead of relying on PATH; set a unique key in process.env
(e.g., UNIQUE_TEST_ENV) to a known value, call
resolveEnvVars("$UNIQUE_TEST_ENV/test", {}), and assert the returned string
equals the exact concatenation of that env value and "/test"; ensure the test
cleans up the temporary process.env entry afterwards. This targets the
resolveEnvVars function and the test named "falls back to process.env".
In `@tests/run-proxy.test.ts`:
- Line 98: The test title is misleading: it currently reads "unregisters the
proxy instance if startup fails" but the assertion checks that
deleteInstanceEntry is NOT called; update the test name string in the
test("...") for the test in tests/run-proxy.test.ts (the test that references
deleteInstanceEntry and asserts it was not called) to accurately reflect the
behavior, e.g. "does not unregister the proxy instance if startup fails" or
"does not call deleteInstanceEntry when startup fails".
In `@tests/server-handlers.test.ts`:
- Line 353: The test currently asserts describeResult.isError is undefined which
is ambiguous; update the assertion to explicitly assert success by checking
describeResult.isError === false so the success/error contract is
unambiguous—locate the test where describeResult is used (the failing
expectation line with expect(describeResult.isError).toBeUndefined()) and change
it to assert false instead.
🪄 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: b951b31c-ee7f-42c3-9aa7-d614a8b75a06
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (86)
.github/CONTRIBUTING.md.gitignore.junie/memory/errors.md.junie/memory/feedback.md.junie/memory/language.json.junie/memory/memory.version.junie/memory/tasks.mdCHANGELOG.mdREADME.mdWARP.mdbiome.jsondocs/ARCHITECTURE.mddocs/DEPENDENCY_EXCEPTIONS.mddocs/DEPENDENCY_MAINTENANCE.mddocs/PROJECT-INCEPTION-REQUIREMENTS.mddocs/RELEASING.mddocs/RUNTIME-SUPERVISOR.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/index.tssrc/config/paths.tssrc/config/schema.tssrc/config/validate.tssrc/daemon/proxy.tssrc/daemon/server.tssrc/index.tssrc/init/runner.tssrc/runtime/agent-lease.tssrc/runtime/health-tracker.tssrc/runtime/supervisor.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/status/runner.tssrc/upstream/adapter-registry.tssrc/upstream/adapter.tssrc/upstream/adapters/mcporter.tssrc/upstream/adapters/sse.tssrc/upstream/adapters/stdio.tssrc/upstream/cataloger.tssrc/upstream/client.tssrc/upstream/proxy-middleware.tstests/adapters.test.tstests/agent-lease.test.tstests/cataloger.test.tstests/config.test.tstests/daemon-proxy.test.tstests/daemon-server.test.tstests/health-tracker.test.tstests/main-runtime.test.tstests/proxy-middleware.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/runtime-supervisor.test.tstests/server-capability-surface.test.tstests/server-capability-tool-executor.test.tstests/server-handlers.test.tstests/server-runtime-lifecycle.test.tstests/server-runtime.test.tstests/server-session-surface.test.tstests/server-shell.test.tstests/status-runner.test.tstests/upstream-client-oauth.test.ts
| | 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.
Exception review deadline is already expired.
The Review By date is 2026-04-12, which is already in the past as of 2026-06-11. This leaves the accepted vulnerability exception overdue and outside its stated control window; the linked issue should also be a concrete tracking ticket, not “Pending”.
Suggested patch shape
-| ... | Maintainers | 2026-04-12 | Pending maintainer tracking issue |
+| ... | Maintainers | <new-future-review-date> | <tracking-issue-link-or-id> |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/DEPENDENCY_EXCEPTIONS.md` at line 7, The exception entry for the
`file-type` infinite loop is overdue; update the `Review By` date in
DEPENDENCY_EXCEPTIONS.md (the table row referencing `@opentui/core -> jimp ->
`@jimp/core` -> file-type`) to a new future review deadline, replace "Pending
maintainer tracking issue" with a concrete issue/PR reference or tracker ID, and
add a short note in the same table cell stating who is responsible for follow-up
(e.g., maintainer GitHub handle or team) and the action expected (reassess on
`@opentui/core` or `jimp` bumps and before routing untrusted binaries through
TUI).
Comment on lines
+54
to
+55
| isStderrTty: process.stdout.isTTY, | ||
| isStdinTty: process.stdin.isTTY, |
There was a problem hiding this comment.
Fix isStderrTty wiring to use stderr.
Line 54 assigns isStderrTty from process.stdout.isTTY; that can route default mode selection incorrectly when stdout/stderr TTY state differs.
Suggested patch
return {
- isStderrTty: process.stdout.isTTY,
+ isStderrTty: process.stderr.isTTY,
isStdinTty: process.stdin.isTTY,📝 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
| isStderrTty: process.stdout.isTTY, | |
| isStdinTty: process.stdin.isTTY, | |
| isStderrTty: process.stderr.isTTY, | |
| isStdinTty: process.stdin.isTTY, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/main-runtime.ts` around lines 54 - 55, The isStderrTty property is
incorrectly wired to process.stdout.isTTY; update the assignment so isStderrTty
reads process.stderr.isTTY instead to reflect the actual stderr TTY state
(modify the object/initializer where isStderrTty and isStdinTty are set in
main-runtime.ts).
Comment on lines
+251
to
+255
| 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.
Require OAuth state in callback validation.
Line 251 only verifies state when present, so a callback with no state is accepted. That weakens CSRF protection for the auth flow.
Suggested patch
- 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;
}📝 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: OAuth state mismatch - possible CSRF attack."); | |
| processRef.exit(1); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/run-auth.ts` around lines 251 - 255, The callback currently only
checks state when result.state is present, allowing missing state to bypass CSRF
protection; update the validation in the callback handler (where result.state
and authProvider.verifyState are used) to treat a missing state as an error: if
result.state is falsy OR authProvider.verifyState(result.state) returns false,
log the OAuth state mismatch and exit (processRef.exit(1)) just as when
verification fails.
Comment on lines
+105
to
+107
| if (options.socketPath) { | ||
| daemonOptions.socketPath = options.socketPath; | ||
| } |
There was a problem hiding this comment.
Use the effective daemon socket path in the instance registry entry.
Line 128 always records monitorSocketPath, even when Lines 105-107 override daemon socketPath. This can publish the wrong endpoint for registry consumers when --socket-path is used.
Suggested fix
- const monitorSocketPath = getSocketFilePath(configHash);
+ const monitorSocketPath = getSocketFilePath(configHash);
+ const effectiveDaemonSocketPath = options.socketPath ?? monitorSocketPath;
const runtime = createRuntime({
config,
monitorSocketPath,
});
@@
- if (options.socketPath) {
- daemonOptions.socketPath = options.socketPath;
+ if (options.socketPath) {
+ daemonOptions.socketPath = options.socketPath;
}
@@
processRef,
role: "daemon",
- socketPath: monitorSocketPath,
+ socketPath: effectiveDaemonSocketPath,
version: VERSION,
}),
);Also applies to: 121-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/run-daemon.ts` around lines 105 - 107, The registry entry currently
always uses monitorSocketPath instead of the effective daemon socket path;
update the instance registration to use the final socket path value from
daemonOptions (taking into account options.socketPath override) rather than the
original monitorSocketPath. Locate the code that builds the instance/registry
entry (the block that references monitorSocketPath) and replace that field with
the resolved socket path from daemonOptions.socketPath (or the variable that
holds the effective daemon socket) so consumers get the correct endpoint.
Comment on lines
+93
to
+113
| export function registerShutdownHooks({ | ||
| includeStdin, | ||
| onExit, | ||
| processRef = process, | ||
| shutdown, | ||
| }: RegisterShutdownHooksOptions): void { | ||
| processRef.on("SIGINT", () => { | ||
| void shutdown(0); | ||
| }); | ||
| processRef.on("SIGTERM", () => { | ||
| void shutdown(0); | ||
| }); | ||
|
|
||
| if (includeStdin) { | ||
| processRef.stdin.on("close", () => { | ||
| void shutdown(0); | ||
| }); | ||
| processRef.stdin.on("end", () => { | ||
| void shutdown(0); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard shutdown handling so cleanup executes only once.
Lines 99-112 can trigger shutdown() multiple times (notably stdin end + close), which risks duplicate stop/unregister paths and racey teardown behavior.
Suggested fix
export function registerShutdownHooks({
includeStdin,
onExit,
processRef = process,
shutdown,
}: RegisterShutdownHooksOptions): void {
- processRef.on("SIGINT", () => {
- void shutdown(0);
- });
- processRef.on("SIGTERM", () => {
- void shutdown(0);
- });
+ let shuttingDown = false;
+ const triggerShutdown = (exitCode: number): void => {
+ if (shuttingDown) return;
+ shuttingDown = true;
+ void shutdown(exitCode);
+ };
+
+ processRef.on("SIGINT", () => {
+ triggerShutdown(0);
+ });
+ processRef.on("SIGTERM", () => {
+ triggerShutdown(0);
+ });
if (includeStdin) {
processRef.stdin.on("close", () => {
- void shutdown(0);
+ triggerShutdown(0);
});
processRef.stdin.on("end", () => {
- void shutdown(0);
+ triggerShutdown(0);
});
}📝 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
| export function registerShutdownHooks({ | |
| includeStdin, | |
| onExit, | |
| processRef = process, | |
| shutdown, | |
| }: RegisterShutdownHooksOptions): void { | |
| processRef.on("SIGINT", () => { | |
| void shutdown(0); | |
| }); | |
| processRef.on("SIGTERM", () => { | |
| void shutdown(0); | |
| }); | |
| if (includeStdin) { | |
| processRef.stdin.on("close", () => { | |
| void shutdown(0); | |
| }); | |
| processRef.stdin.on("end", () => { | |
| void shutdown(0); | |
| }); | |
| } | |
| export function registerShutdownHooks({ | |
| includeStdin, | |
| onExit, | |
| processRef = process, | |
| shutdown, | |
| }: RegisterShutdownHooksOptions): void { | |
| let shuttingDown = false; | |
| const triggerShutdown = (exitCode: number): void => { | |
| if (shuttingDown) return; | |
| shuttingDown = true; | |
| void shutdown(exitCode); | |
| }; | |
| processRef.on("SIGINT", () => { | |
| triggerShutdown(0); | |
| }); | |
| processRef.on("SIGTERM", () => { | |
| triggerShutdown(0); | |
| }); | |
| if (includeStdin) { | |
| processRef.stdin.on("close", () => { | |
| triggerShutdown(0); | |
| }); | |
| processRef.stdin.on("end", () => { | |
| triggerShutdown(0); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/runtime-bootstrap.ts` around lines 93 - 113, The shutdown hook can be
invoked multiple times (SIGINT/SIGTERM and stdin 'end'/'close'), so wrap calls
to shutdown in a one-time guard: inside registerShutdownHooks create a local
boolean (e.g., "shutdownCalled") and a small wrapper function (e.g.,
"invokeShutdown(code)") that returns immediately if shutdownCalled is true, sets
it true on first call, then calls void shutdown(code); replace all direct
shutdown(...) calls (the signal handlers and stdin handlers) with this wrapper
so cleanup runs only once; alternatively use processRef.once for signals but
still protect stdin handlers with the same one-time guard to avoid duplicate
teardown.
Comment on lines
345
to
358
| // Retry connection after auth | ||
| log("Retrying connection after OAuth..."); | ||
| 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.
Start a fresh connect timeout for the post-OAuth retry.
The retry still races client.connect() against the original timeoutPromise, which started before the browser flow. If authorization takes longer than timeoutMs, the recreated transport will fail immediately on retry even though auth succeeded. Build a new timeout window for each connect attempt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/upstream/client.ts` around lines 345 - 358, The retry is still racing
client.connect(transport) against the original timeoutPromise created before the
OAuth flow, so recreate a fresh timeout promise for the post-OAuth connect
attempt; before awaiting Promise.race([client.connect(transport!),
timeoutPromise]) in the retry block, build a new timeoutPromise (using the same
timeoutMs/timeout creation logic used originally) so that each connect() call
races against a newly-started timeout; locate this in the retry branch around
safelyCloseTransport/ httpTransportFactory/transport and replace the reused
timeoutPromise with the newly created one.
Comment on lines
+130
to
+145
| const sessionMap = new Map<string, string>(); | ||
|
|
||
| return { | ||
| name: "session-affinity", | ||
| fn: (ctx) => { | ||
| const sessionId = ctx.callContext?.sessionId; | ||
| if (!sessionId) return; | ||
|
|
||
| // Track session -> upstream mapping | ||
| const existing = sessionMap.get(sessionId); | ||
| if (existing && existing !== ctx.upstreamKey) { | ||
| // Session was previously on a different upstream | ||
| ctx.state["sessionAffinityConflict"] = true; | ||
| ctx.state["previousUpstream"] = existing; | ||
| } | ||
| sessionMap.set(sessionId, ctx.upstreamKey); |
There was a problem hiding this comment.
sessionMap grows without any eviction path.
Every new sessionId is retained forever, and this middleware has no cleanup hook to remove dead sessions. In a long-lived proxy process, that becomes an unbounded in-memory cache. Add TTL/LRU eviction or a teardown path tied to session end.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/upstream/proxy-middleware.ts` around lines 130 - 145, The sessionMap used
by the "session-affinity" middleware (symbol: sessionMap, middleware name
"session-affinity", handler fn) never evicts entries causing unbounded memory
growth; implement eviction by either replacing sessionMap with an LRU/TTL cache
(use an LRU cache library or a Map keyed to sessionId with stored timestamps and
a max size/ttl) and run periodic cleanup, or add a teardown hook tied to session
lifecycle (e.g., remove sessionMap entry when ctx.callContext signals session
end) so entries are removed when sessions terminate; ensure lookups still use
sessionMap.get(sessionId) and updates use sessionMap.set(sessionId,
ctx.upstreamKey).
Comment on lines
+204
to
+214
| fn: (ctx) => { | ||
| const retryCount = (ctx.state["retryCount"] as number) ?? 0; | ||
| if (ctx.response?.error && retryCount < maxRetries) { | ||
| ctx.state["retryCount"] = retryCount + 1; | ||
| ctx.state["retryDelayMs"] = baseDelayMs * 2 ** retryCount; | ||
| ctx.state["shouldRetry"] = true; | ||
| } | ||
| }, | ||
| options: { | ||
| phases: ["after_call"], | ||
| priority: 150, |
There was a problem hiding this comment.
Retry state is skipped for the dedicated error phase.
The retry middleware only subscribes to after_call, so callers that surface transport failures through on_error never set shouldRetry/retryDelayMs. Register the middleware for the error phase as well, or move retry detection there so thrown failures participate in backoff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/upstream/proxy-middleware.ts` around lines 204 - 214, The retry detection
currently in the middleware fn (checking ctx.response?.error, retryCount,
maxRetries and setting ctx.state["shouldRetry"]/["retryDelayMs"]) only runs in
the "after_call" phase per options.phases; register this middleware for the
error phase as well or move the retry-detection logic into the on_error handler
so transport failures set ctx.state["shouldRetry"] and
ctx.state["retryDelayMs"]. Update the options.phases to include "on_error" (or
relocate the fn's logic to the on_error hook) so functions using on_error
participate in backoff, keeping the same checks for retryCount, maxRetries,
baseDelayMs and state keys.
Comment on lines
+403
to
+411
| await firstDaemon.stop(); | ||
| await secondDaemon.start(); | ||
| await new Promise((resolve) => setTimeout(resolve, 250)); | ||
|
|
||
| const recoveredTools = (await client.listTools()).tools.map( | ||
| (tool) => tool.name, | ||
| ); | ||
| expect(recoveredTools).toContain("time_util"); | ||
| } finally { |
There was a problem hiding this comment.
Replace fixed sleep with condition-based recovery wait.
Line 405 uses a hardcoded 250ms delay before asserting reconnection. This is timing-sensitive and can flake under slower CI runs; use waitFor on the recovered tool condition instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/daemon-proxy.test.ts` around lines 403 - 411, Replace the fixed 250ms
sleep with a condition-based wait that polls until the reconnection is observed:
after calling await secondDaemon.start(), loop or use a waitFor helper to
repeatedly call await client.listTools() (or reuse recoveredTools logic) and
check that the returned tool names include "time_util", with a sensible timeout
(e.g., few seconds) and short interval between attempts; update the block around
firstDaemon, secondDaemon, and client.listTools() so the test only proceeds when
the recovered tool condition is met instead of awaiting a hardcoded setTimeout.
…r election The test was asserting that bridge-1 is the owner after daemon replacement, but owner election during reconnection is non-deterministic since the replacement daemon doesn't retain the previous owner identity. Updated assertion to verify the owner is one of the 3 expected bridges. Co-authored-by: MiMoCode Agent <mimo@xiaomi.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25 +/- ##
==========================================
+ Coverage 69.84% 76.63% +6.79%
==========================================
Files 112 140 +28
Lines 12520 14018 +1498
==========================================
+ Hits 8744 10743 +1999
+ Misses 3776 3275 -501 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
Completes all remaining 0.9 runtime supervisor work items for dark-factory multi-agent operations:
AgentLeaseManagerprovides temporary exclusive access to singleton stdio servers with configurable TTL and automatic expiry. Non-holder calls queue until lease release.HealthTrackerrecords upstream health state, restart counts, tool call metrics, and audit events with optional JSON persistence to~/.config/mcp-squared/data/health.json.UpstreamAdapterinterface extracts transport-specific logic (stdio process spawning, HTTP transport creation) from the Cataloger into pluggable adapters with a registry.ProxyMiddlewaresystem with built-in session affinity, request logging, retry with exponential backoff, and auth injection middlewares.Changes
New Files
src/runtime/agent-lease.ts— AgentLeaseManagersrc/runtime/health-tracker.ts— HealthTrackersrc/upstream/adapter.ts— UpstreamAdapter interfacesrc/upstream/adapters/stdio.ts— Stdio adaptersrc/upstream/adapters/sse.ts— SSE/HTTP adaptersrc/upstream/adapters/mcporter.ts— mcporter SDK adaptersrc/upstream/adapter-registry.ts— Adapter registrysrc/upstream/proxy-middleware.ts— ProxyMiddleware systemNew Tests
tests/agent-lease.test.ts— 25 teststests/health-tracker.test.ts— 25 teststests/adapters.test.ts— 17 teststests/proxy-middleware.test.ts— 16 testsModified Files
src/config/schema.ts— AddedHealthTrackingSchemaandRuntimeLeaseSchemasrc/config/paths.ts— AddedgetDataDir()andensureDataDir()src/runtime/supervisor.ts— Integrated lease and health trackingdocs/RUNTIME-SUPERVISOR.md— Marked all 0.9 items as doneCHANGELOG.md— Documented all new featuresVerification
Co-authored-by: MiMoCode Agent mimo@xiaomi.com
Summary by CodeRabbit
New Features
Documentation
Chores