Refactor nREPL evaluation for robustness and simplicity #132
Conversation
- Replaced long-lived nREPL connections and async polling with ephemeral, blocking connections per evaluation.
- `clojure-mcp.nrepl`:
- Removed `start-polling`, `stop-polling`, and callback logic.
- Added blocking `eval-code` taking a `:session-type` (e.g., `:default`, `:tools`, `:shadow`, `:figwheel`).
- State is now a simple map tracking session IDs by port and type.
- Added `current-ns` helper to track namespace by session type.
- Tools updates:
- `eval`: Updated to use blocking `eval-code` and accept `:session-type`.
- `bash`: Removed custom session creation; uses `:tools` session type.
- `figwheel` & `shadow`: Updated to use `:session-type` for dedicated sessions.
- Core & CLI:
- `clojure-mcp.core`: Removed polling initialization.
- `prompt-cli`: Removed polling initialization.
- Tests:
- Updated `eval` tool tests to reflect API changes.
WalkthroughRefactors nREPL interaction from polling/callbacks to synchronous, per-connection evals with symbolic session-types; enhances config.load-config to explicitly load → validate (home/project) → merge → process with canonical-path reporting and debug logs; updates tools/tests to propagate :session-type and removes polling lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP Service
participant NRepl as nREPL
rect rgb(245,240,255)
Note over Client,MCP: Previous (Polling + Callbacks)
Client->>MCP: eval-code(service, code, callback)
activate MCP
MCP->>NRepl: send request (long-lived/polled)
activate NRepl
NRepl-->>MCP: streaming responses (async)
MCP->>MCP: accumulate via polling callbacks
MCP-->>Client: invoke callback with results
deactivate NRepl
deactivate MCP
end
rect rgb(235,255,240)
Note over Client,MCP: New (Synchronous, session-type)
Client->>MCP: eval-code(service, code, {:session-type k})
activate MCP
MCP->>NRepl: open fresh connection + eval
activate NRepl
NRepl-->>MCP: collect responses (blocking)
deactivate NRepl
MCP->>MCP: process-responses & update per-port state
MCP-->>Client: return response sequence / processed result
deactivate MCP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🧰 Additional context used📓 Path-based instructions (3)src/**/*.{clj,cljc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{clj,cljc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.{clj,cljc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (9)📓 Common learnings📚 Learning: 2025-08-02T20:23:28.499ZApplied to files:
📚 Learning: 2025-08-02T20:23:28.499ZApplied to files:
📚 Learning: 2025-08-18T00:39:24.837ZApplied to files:
📚 Learning: 2025-08-02T20:23:28.499ZApplied to files:
📚 Learning: 2025-08-02T20:23:28.499ZApplied to files:
📚 Learning: 2025-08-02T20:23:28.499ZApplied to files:
📚 Learning: 2025-08-02T20:23:28.499ZApplied to files:
📚 Learning: 2025-08-02T20:23:28.499ZApplied to files:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Updated and to reflect the removal of direct session creation and the use of . - Updated and to remove obsolete polling calls and use the new signature. - Formatted modified test files with Usage: clj-paren-repair FILE [FILE ...] Fix delimiter errors and format Clojure files. Features enabled by default: - Delimiter error detection and repair - cljfmt formatting Options: -h, --help Show this help message.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/clojure_mcp/dialects.clj (1)
95-106: Missingclojure.stringrequire and alias causes a runtime error
fetch-project-directorycallsclojure.string/replaceat Line 105, butclojure.stringis not required in this namespace. This will fail at compile or runtime.Also, project guidelines prefer
:requirewith an alias (e.g.[clojure.string :as string]) rather than fully qualified calls.Suggested fix:
-(ns clojure-mcp.dialects - "Handles environment-specific behavior for different nREPL dialects. - ... - (:require [clojure.edn :as edn] - [clojure.java.io :as io] - [taoensso.timbre :as log] - [nrepl.core :as nrepl-core] - [clojure-mcp.nrepl :as nrepl] - [clojure-mcp.utils.file :as file-utils])) +(ns clojure-mcp.dialects + "Handles environment-specific behavior for different nREPL dialects. + ... + (:require [clojure.edn :as edn] + [clojure.java.io :as io] + [clojure.string :as string] + [taoensso.timbre :as log] + [nrepl.core :as nrepl-core] + [clojure-mcp.nrepl :as nrepl] + [clojure-mcp.utils.file :as file-utils])) ... - (if (and (vector? raw-result) (= 1 (count raw-result)) (string? (first raw-result))) - (clojure.string/replace (first raw-result) #"^\"|\"$" "") + (if (and (vector? raw-result) (= 1 (count raw-result)) (string? (first raw-result))) + (string/replace (first raw-result) #"^\"|\"$" "") raw-result))))
🧹 Nitpick comments (8)
src/clojure_mcp/prompt_cli.clj (1)
226-229: Global logging disable inrun-promptis appropriate for CLI, but note side effectsDisabling logging via
logging/configure-logging!before connecting keeps stdout clean for CLI usage; ifrun-promptis ever reused from a long-lived REPL or host app, consider an option or temporary override so logging isn’t permanently suppressed.src/clojure_mcp/config.clj (1)
112-155: Config load/validate/merge pipeline is clearer; consider behavior for missing CLI config filesThe refactored
load-config(load → validate per-file → merge → process → debug log) is easier to reason about and plays well with the schema error handling incore.clj. One behavioral question: whencli-config-fileis provided but the file doesn’t exist, it’s currently treated as{}and only logged asexists: false; if a user explicitly passes--configyou may want to treat a missing file as an error instead of silently ignoring it.src/clojure_mcp/tools/bash/tool.clj (1)
24-40: Session-type wiring for bash-over-nREPL is consistent with the new modelDeriving
session-typeas:toolswhenbash-over-nreplis enabled and threading it via:nrepl-session-typekeeps the tool config simple while preserving atom-based nREPL state. If you later add more bash session variants, consider making the session-type configurable instead of hard-coded.src/clojure_mcp/tools/bash/core.clj (1)
168-187: nREPL bash executor correctly propagates:session-typeDestructuring
session-typeand conditionally adding:session-typeinto theevaluate-codeoptions hooks bash-over-nREPL into the new session-type model without altering the existing EDN/timeout logic. As a tiny nit, the log key:nrelp-eval-output-mapis misspelled if you ever touch that logging again.src/clojure_mcp/tools/figwheel/tool.clj (2)
10-32: Figwheel startup now uses:session-type :figwheel; consider surfacing hard failuresCalling
nrepl/eval-codewith:session-type :figwheeland returning:figwheelfromstart-figwheelcleanly hooks figwheel into the new session-type model. Right now, even if startup throws, you just log and still return:figwheel; you might want to rethrow (or signal failure) so that a completely broken figwheel start is visible to the caller instead of only in logs.
70-79:execute-toolcorrectly usessession-typebut can simplify input handlingAsserting non-nil
session-typeand passing it to bothcurrent-nsandevaluate-with-repairensures all figwheel evals stay on the dedicated session. Given thatinputsshould already be normalized byvalidate-inputs, theassert (:code inputs)plus fallback to(get inputs "code")is slightly redundant; either rely solely on:codeor relax the assertion if you truly expect string-keyed inputs here.src/clojure_mcp/main_examples/shadow_main.clj (1)
33-48: Guard against missing:shadow-buildto avoid opaque NPE
(name shadow-build)will throw if:shadow-buildis absent or nil inconfig, giving an unhelpfulNullPointerException. Given this is user‑facing config, it would be better to validate and fail with a clear message before buildingstart-code.Consider something like:
-(defn start-shadow-repl [nrepl-client-atom {:keys [shadow-build shadow-watch]}] - (let [start-code (format +(defn start-shadow-repl [nrepl-client-atom {:keys [shadow-build shadow-watch] :as config}] + (when-not shadow-build + (throw (ex-info "Missing :shadow-build in Shadow CLJS config" + {:config config}))) + (let [start-code (format ...This keeps failures explicit and aligned with the “validate inputs and provide helpful error messages in MCP tools” guideline.
src/clojure_mcp/nrepl.clj (1)
6-15: State initialization matches atom-based model but assumes callers always usecreate
createattaches an atom under::statewith a per-port:portsmap, which fits the “atom-based state for consistent service access” guideline. Just be aware all public entry points (eval-code,interrupt,current-ns,describe) assume::stateis present—swap!on(get-state service)will blow up if someone bypassescreate.If you expect external callers to sometimes construct the service map themselves, consider a small helper that asserts
::stateis present and throws anex-infowith a helpful message instead of an NPE.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/clojure_mcp/config.clj(1 hunks)src/clojure_mcp/core.clj(1 hunks)src/clojure_mcp/dialects.clj(4 hunks)src/clojure_mcp/main_examples/shadow_main.clj(2 hunks)src/clojure_mcp/nrepl.clj(1 hunks)src/clojure_mcp/prompt_cli.clj(2 hunks)src/clojure_mcp/tools/bash/core.clj(2 hunks)src/clojure_mcp/tools/bash/tool.clj(3 hunks)src/clojure_mcp/tools/eval/core.clj(3 hunks)src/clojure_mcp/tools/eval/tool.clj(2 hunks)src/clojure_mcp/tools/figwheel/tool.clj(2 hunks)test/clojure_mcp/tools/eval/core_test.clj(1 hunks)test/clojure_mcp/tools/eval/tool_test.clj(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{clj,cljc}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{clj,cljc}: Use:requirewith ns aliases for imports (e.g.,[clojure.string :as string])
Include clear tool:descriptionfor LLM guidance
Validate inputs and provide helpful error messages in MCP tools
Return structured data with both result and error status in MCP tools
Maintain atom-based state for consistent service access in MCP tools
Files:
src/clojure_mcp/config.cljsrc/clojure_mcp/prompt_cli.cljsrc/clojure_mcp/dialects.cljsrc/clojure_mcp/tools/bash/core.cljsrc/clojure_mcp/tools/eval/tool.cljsrc/clojure_mcp/core.cljsrc/clojure_mcp/tools/eval/core.cljsrc/clojure_mcp/tools/bash/tool.cljsrc/clojure_mcp/tools/figwheel/tool.cljsrc/clojure_mcp/main_examples/shadow_main.cljsrc/clojure_mcp/nrepl.clj
**/*.{clj,cljc}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{clj,cljc}: Use kebab-case for vars/functions; end predicates with?(e.g.,is-top-level-form?)
Usetry/catchwith specific exception handling; use atom for tracking errors
Use 2-space indentation and maintain whitespace in edited forms
Align namespaces with directory structure (e.g.,clojure-mcp.repl-tools)
Files:
src/clojure_mcp/config.cljsrc/clojure_mcp/prompt_cli.cljsrc/clojure_mcp/dialects.cljsrc/clojure_mcp/tools/bash/core.cljsrc/clojure_mcp/tools/eval/tool.cljsrc/clojure_mcp/core.cljsrc/clojure_mcp/tools/eval/core.cljtest/clojure_mcp/tools/eval/tool_test.cljsrc/clojure_mcp/tools/bash/tool.cljsrc/clojure_mcp/tools/figwheel/tool.cljsrc/clojure_mcp/main_examples/shadow_main.cljtest/clojure_mcp/tools/eval/core_test.cljsrc/clojure_mcp/nrepl.clj
test/**/*.{clj,cljc}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
deftestwith descriptive names;testingfor subsections;isfor assertions in tests
Files:
test/clojure_mcp/tools/eval/tool_test.cljtest/clojure_mcp/tools/eval/core_test.clj
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to src/**/*.{clj,cljc} : Maintain atom-based state for consistent service access in MCP tools
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to src/**/*.{clj,cljc} : Validate inputs and provide helpful error messages in MCP tools
Applied to files:
src/clojure_mcp/config.cljsrc/clojure_mcp/prompt_cli.cljsrc/clojure_mcp/dialects.cljsrc/clojure_mcp/tools/eval/core.clj
📚 Learning: 2025-08-18T00:39:24.837Z
Learnt from: hugoduncan
Repo: bhauman/clojure-mcp PR: 86
File: src/clojure_mcp/subprocess.clj:0-0
Timestamp: 2025-08-18T00:39:24.837Z
Learning: In the clojure-mcp project, the user prefers to only parse stdout for nREPL port discovery in the subprocess.clj module, and explicitly does not want to parse stderr for port information.
Applied to files:
src/clojure_mcp/prompt_cli.cljsrc/clojure_mcp/dialects.cljsrc/clojure_mcp/core.clj
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to src/**/*.{clj,cljc} : Maintain atom-based state for consistent service access in MCP tools
Applied to files:
src/clojure_mcp/prompt_cli.cljsrc/clojure_mcp/dialects.cljsrc/clojure_mcp/core.cljtest/clojure_mcp/tools/eval/tool_test.cljsrc/clojure_mcp/main_examples/shadow_main.cljsrc/clojure_mcp/nrepl.clj
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to **/*.{clj,cljc} : Align namespaces with directory structure (e.g., `clojure-mcp.repl-tools`)
Applied to files:
src/clojure_mcp/dialects.clj
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to src/**/*.{clj,cljc} : Use `:require` with ns aliases for imports (e.g., `[clojure.string :as string]`)
Applied to files:
src/clojure_mcp/dialects.clj
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to src/**/*.{clj,cljc} : Return structured data with both result and error status in MCP tools
Applied to files:
src/clojure_mcp/dialects.cljsrc/clojure_mcp/tools/eval/core.clj
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Prefer REPL-driven development for rapid iteration and feedback
Applied to files:
src/clojure_mcp/core.cljtest/clojure_mcp/tools/eval/core_test.clj
🔇 Additional comments (17)
src/clojure_mcp/prompt_cli.clj (1)
18-19: Require aliases forfile-utilsandlogginglook correctAliases match usage (
file-utils/slurp-utf8,logging/configure-logging!), keeping dependencies explicit and consistent.src/clojure_mcp/config.clj (1)
59-85: Centralizedvalidate-configsimproves error reportingValidating each non-empty config with its canonical path and tagging
::schema-erroris a clean way to surface per-file schema issues and integrate withload-config-handling-validation-errors.test/clojure_mcp/tools/eval/tool_test.clj (1)
14-25: Fixture update to blockingeval-codematches new nREPL APIUsing a single blocking
(eval-code client "(require 'clojure.repl)")setup without polling keeps the tests simple and consistent with the refactored client.src/clojure_mcp/tools/bash/tool.clj (1)
119-125:execute-toolcorrectly chooses between local and nREPL-backed executionDispatching to
execute-bash-command-nreplwith:session-typewhennrepl-session-typeis set, and otherwise using the local executor, matches the intended bash-over-nREPL semantics without changing the result shape.src/clojure_mcp/core.clj (1)
283-297: Additional info log on nREPL client creation is useful and low-riskLogging
"nREPL client map created"right afternrepl/creategives a clear lifecycle marker during startup without changing behavior.test/clojure_mcp/tools/eval/core_test.clj (1)
13-26: Test fixture correctly updated for blockingeval-codeand removed pollingInitializing the test REPL with a single blocking
(eval-code …)and droppingstart-polling/stop-pollingkeeps the tests in sync with the refactored nREPL client while preserving behavior.src/clojure_mcp/main_examples/shadow_main.clj (2)
51-59: Session-type wiring for secondary Shadow connection looks good
shadow-eval-tool-secondary-connection-toolcorrectly starts the Shadow REPL on the secondary connection and then builds the eval tool with{:session-type :shadow}, ensuring subsequent evals reuse the same Shadow session type. No issues from the refactor here.
62-66: Shared-connection Shadow eval path is consistent with new modelThe shared-connection variant now delegates to
start-shadow-repland then creates the eval tool with{:session-type :shadow}against the primarynrepl-client-atom. This matches the new session-type model and avoids the older explicit session plumbing.src/clojure_mcp/dialects.clj (3)
79-89: Use ofnrepl/eval-code+nrepl-core/combine-responsesis consistent with new eval model
fetch-project-directory-helpernow evaluates the dialect-specific expression viaclojure-mcp.nrepl/eval-codeand then usesnrepl.core/combine-responsesto extract:value. That’s aligned with the new synchronous, per-session-type evaluation flow and keeps this helper independent of the higher-level eval tooling.
110-118: Environment initialization now correctly uses eval-codeSwitching
initialize-environmentto callnrepl/eval-codefor eachinit-expkeeps dialect init aligned with the new nREPL client abstraction. Since these are short, one-off bootstrapping forms, the lack of explicit:session-type(falling back to:default) is fine.
120-126: Using:session-type :toolsfor REPL helpers is appropriateLoading helper expressions via
nrepl/eval-codewith:session-type :toolscleanly separates “tools” state from default eval sessions, and matches how other tooling in this PR uses the tools session type.src/clojure_mcp/tools/eval/tool.clj (2)
12-16: Factory now cleanly carries:session-typeinto tool configThe updated arity for
create-eval-toolcorrectly captures an optional:session-typeand stores it on the tool config. This is the right place to anchor session-type selection for consumers.
68-73: execute-tool correctly propagates:session-typeand timeout into core eval
execute-toolnow threadssession-typeinto the inputs map and preserves the existing timeout-ms override behavior. That keeps the tool behavior aligned with the new session-type–based nREPL API without changing the external schema.src/clojure_mcp/tools/eval/core.clj (2)
43-56: Response aggregation correctly tracks outputs and error state
process-responsescaptures:out,:err, and:valuein order and marks:errorwhen either an exception (:ex) appears or:statuscontains"error"/"eval-error". That’s a solid fit for nREPL’s response model and works well with the existing partitioning/formatting pipeline.
104-122: Repaired-code handling remains correct with new eval pipeline
evaluate-with-repairstill repairs delimiter errors, then delegates toevaluate-codeand adds:repairedbased on whether the code changed. This composes cleanly with the new synchronous eval andprocess-responsesoutput structure.src/clojure_mcp/nrepl.clj (2)
59-82: Synchronouseval-codewith per-session-type tracking looks solidUsing a fresh connection per
eval-codecall, withensure-session!to reuse or create a per-session-typenREPL session and updating:current-nsfrom responses, is a clean design and aligns well with the new stateless-connection goal. The use oftruncation-lengthfor print quota also matches the higher-level formatting logic.
94-99: describe implementation is straightforward and matches new connection model
describenow opens a short-lived connection, creates a client, and returnsnrepl/combine-responsesfor a simple{:op "describe"}message. That’s minimal and consistent with the rest of the synchronous API.
src/clojure_mcp/nrepl.clj
Outdated
| (defn interrupt [service] | ||
| (let [state (get-state service) | ||
| id (:current-eval-id @state)] | ||
| (when id | ||
| (with-open [conn (connect service)] | ||
| (let [client (nrepl/client conn 1000) | ||
| ;; Assuming default session for interrupt for now as per typical usage | ||
| session-id (get-stored-session service :default)] | ||
| (nrepl/message client {:op "interrupt" :session session-id :interrupt-id id})))))) |
There was a problem hiding this comment.
interrupt only targets the default session, which can break timeouts for other session types
interrupt currently looks up the session ID via:
(let [state (get-state service)
id (:current-eval-id @state)]
...
(let [client (nrepl/client conn 1000)
;; Assuming default session for interrupt for now as per typical usage
session-id (get-stored-session service :default)]
(nrepl/message client {:op "interrupt" :session session-id :interrupt-id id})))But eval-code accepts a :session-type (defaulting to :default) and stores only :current-eval-id in state, not the associated session-type. When evaluate-code in tools.eval.core runs with :session-type :tools or :shadow, a timeout will call nrepl/interrupt, which then tries to interrupt the default session. That likely won’t stop the long-running eval in the non-default session.
To make interrupts work reliably across session-types, consider also tracking the current eval’s session-type and using it here, e.g.:
- (swap! (get-state service) assoc :current-eval-id id)
+ (swap! (get-state service) assoc
+ :current-eval-id id
+ :current-eval-session-type session-type)
...
-(defn interrupt [service]
- (let [state (get-state service)
- id (:current-eval-id @state)]
+(defn interrupt [service]
+ (let [state (get-state service)
+ {:keys [current-eval-id current-eval-session-type]} @state
+ session-type (or current-eval-session-type :default)]
- (when id
+ (when current-eval-id
(with-open [conn (connect service)]
(let [client (nrepl/client conn 1000)
- session-id (get-stored-session service :default)]
- (nrepl/message client {:op "interrupt" :session session-id :interrupt-id id}))))))
+ session-id (get-stored-session service session-type)]
+ (nrepl/message client {:op "interrupt"
+ :session session-id
+ :interrupt-id current-eval-id}))))))And clear :current-eval-session-type in the eval-code finally block alongside :current-eval-id.
This would make interrupt behave correctly for :tools, :shadow, and other session-types used throughout the PR.
🤖 Prompt for AI Agents
In src/clojure_mcp/nrepl.clj around lines 84 to 92, interrupt only looks up the
default session and thus fails to interrupt evals running in non-default
sessions; update the state to record the session-type for the current eval (e.g.
:current-eval-session-type) when eval-code starts, clear that key in the
eval-code finally block alongside :current-eval-id, and change interrupt to read
that session-type from state and pass it to get-stored-session (falling back to
:default if absent) before sending the nREPL interrupt message so the correct
session is targeted.
| (let [{:keys [code timeout_ms session-type]} opts | ||
| timeout-ms (or timeout_ms 20000) | ||
| outputs (atom []) | ||
| error-occurred (atom false) | ||
| form-str code | ||
| add-output! (fn [prefix value] (swap! outputs conj [prefix value])) | ||
| result-promise (promise)] | ||
| form-str code] | ||
|
|
||
| ;; Evaluate the code | ||
| ;; Push to eval history if available | ||
| (when-let [state (::nrepl/state nrepl-client)] | ||
| (swap! state update :clojure-mcp.repl-tools/eval-history conj form-str) | ||
|
|
||
| ;; Evaluate the code, using the namespace parameter if provided | ||
| (try | ||
| (nrepl/eval-code-msg | ||
| nrepl-client form-str | ||
| (if session {:session session} {}) | ||
| (->> identity | ||
| (nrepl/out-err | ||
| #(add-output! :out %) | ||
| #(add-output! :err %)) | ||
| (nrepl/value #(add-output! :value %)) | ||
| (nrepl/done (fn [_] | ||
| (deliver result-promise | ||
| {:outputs @outputs | ||
| :error @error-occurred}))) | ||
| (nrepl/error (fn [{:keys [exception]}] | ||
| (reset! error-occurred true) | ||
| (add-output! :err exception) | ||
| (deliver result-promise | ||
| {:outputs @outputs | ||
| :error true}))))) | ||
| (catch Exception e | ||
| ;; prevent connection errors from confusing the LLM | ||
| (log/error e "Error when trying to eval on the nrepl connection") | ||
| (throw | ||
| (ex-info | ||
| (str "Internal Error: Unable to reach the nREPL " | ||
| "thus we are unable to execute the bash command.") | ||
| {:error-type :connection-error} | ||
| e)))) | ||
| (swap! state update :clojure-mcp.repl-tools/eval-history conj form-str)) | ||
|
|
||
| ;; Wait for the result and return it | ||
| (let [tmb (Object.) | ||
| res (deref result-promise timeout-ms tmb)] | ||
| (if-not (= tmb res) | ||
| res | ||
| (try | ||
| (let [fut (future | ||
| (try | ||
| (let [responses (nrepl/eval-code nrepl-client form-str :session-type (or session-type :default))] | ||
| (process-responses responses)) | ||
| (catch Exception e | ||
| (log/error e "Error during nREPL eval") | ||
| {:outputs [[:err (str "Internal Error: " (.getMessage e))]] :error true}))) | ||
| res (deref fut timeout-ms :timeout)] | ||
| (if (= res :timeout) | ||
| (do | ||
| (future-cancel fut) | ||
| (nrepl/interrupt nrepl-client) | ||
| {:outputs [[:err (str "Eval timed out after " timeout-ms "ms.")] | ||
| [:err "Perhaps, you had an infinite loop or an eval that ran too long."]] | ||
| :error true})))))) | ||
| :error true}) | ||
| res)) | ||
| (catch Exception e | ||
| ;; prevent connection errors from confusing the LLM | ||
| (log/error e "Error when trying to eval on the nrepl connection") | ||
| (throw | ||
| (ex-info | ||
| (str "Internal Error: Unable to reach the nREPL " | ||
| "thus we are unable to execute the bash command.") | ||
| {:error-type :connection-error} | ||
| e)))))) |
There was a problem hiding this comment.
Outer connection-error message still refers to “bash command”
The outer catch in evaluate-code throws an ex-info with:
"thus we are unable to execute the bash command."
But this namespace is the generic eval core, not the bash tool, so that wording is confusing.
Consider updating the message to something eval-specific, e.g.:
- (str "Internal Error: Unable to reach the nREPL "
- "thus we are unable to execute the bash command.")
+ (str "Internal Error: Unable to reach the nREPL, "
+ "so we are unable to evaluate your Clojure code.")This keeps the semantics the same while making the error clearer to callers.
🤖 Prompt for AI Agents
In src/clojure_mcp/tools/eval/core.clj around lines 69 to 102, the outer catch
builds an ex-info whose message incorrectly mentions "bash command"; change the
message to refer to nREPL/eval (for example: "Internal Error: Unable to reach
the nREPL; unable to execute the requested eval.") while keeping the same
ex-info shape and metadata ({:error-type :connection-error}) and preserving the
original exception as the cause; update only the message text to be
eval-specific so callers see accurate context.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/clojure_mcp/tools/bash/config_test.clj (1)
29-79: Test inputs missing required:timeout-msparameter that will cause assertion failures in real execution.The test provides inputs
{:command "echo test" :working-directory ...}but bothexecute-bash-command(core.clj:50) andexecute-bash-command-nrepl(core.clj:172) assert thattimeout-msis required. The tool'svalidate-inputsmethod properly handles timeout defaults (tool.clj:78–79), butexecute-tool :bash(tool.clj:119–125) passes inputs directly to execute functions without calling validation or merging the tool's default:timeout-ms.The mocked execute functions in the test bypass these assertions, hiding the bug. In actual execution, calls would fail with "timeout-ms is required".
Fix: Either add
:timeout-msto test inputs (e.g.,:timeout-ms 180000), or ensureexecute-tool :bashmerges the tool's default timeout into inputs before calling execute functions.
🧹 Nitpick comments (2)
test/clojure_mcp/tools/bash/session_test.clj (1)
10-11: Consider renaming to reflect session-type testing.The function name
test-bash-execution-uses-sessionreferences "session" but the test now verifies:session-typebehavior. Consider renaming totest-bash-execution-uses-session-typefor clarity.-(deftest test-bash-execution-uses-session +(deftest test-bash-execution-uses-session-type (testing "Bash command execution passes session-type to evaluate-code"test/clojure_mcp/tools/project/tool_test.clj (1)
20-31: Fixture setup aligns with blocking eval; consider explicit session-type or basic assertionThe direct, blocking
nrepl/eval-codecalls for requiringclojure.replandclojure.ednmake sense with the new stateless client. Ifeval-codenow keys behavior off:session-type, it may be worth explicitly passing the same:session-typethat the project inspection tool uses (e.g.:toolsor:default) and/or asserting that the require completes successfully, just to catch unexpected REPL startup issues early in the fixture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/clojure_mcp/tools/bash/config_test.clj(2 hunks)test/clojure_mcp/tools/bash/session_test.clj(1 hunks)test/clojure_mcp/tools/project/tool_test.clj(1 hunks)test/clojure_mcp/tools/test_utils.clj(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{clj,cljc}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{clj,cljc}: Use kebab-case for vars/functions; end predicates with?(e.g.,is-top-level-form?)
Usetry/catchwith specific exception handling; use atom for tracking errors
Use 2-space indentation and maintain whitespace in edited forms
Align namespaces with directory structure (e.g.,clojure-mcp.repl-tools)
Files:
test/clojure_mcp/tools/bash/config_test.cljtest/clojure_mcp/tools/test_utils.cljtest/clojure_mcp/tools/bash/session_test.cljtest/clojure_mcp/tools/project/tool_test.clj
test/**/*.{clj,cljc}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
deftestwith descriptive names;testingfor subsections;isfor assertions in tests
Files:
test/clojure_mcp/tools/bash/config_test.cljtest/clojure_mcp/tools/test_utils.cljtest/clojure_mcp/tools/bash/session_test.cljtest/clojure_mcp/tools/project/tool_test.clj
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Prefer REPL-driven development for rapid iteration and feedback
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
Repo: bhauman/clojure-mcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to src/**/*.{clj,cljc} : Maintain atom-based state for consistent service access in MCP tools
Applied to files:
test/clojure_mcp/tools/project/tool_test.clj
🔇 Additional comments (5)
test/clojure_mcp/tools/bash/session_test.clj (1)
20-44: LGTM! Test correctly validates session-type propagation.The test implementation properly verifies the refactored behavior:
- Mocks
evaluate-codeto capture arguments- Asserts
:session-typekey is present with value:tools- Validates result formatting
This aligns well with the PR's objective to replace session objects with session-type identifiers.
test/clojure_mcp/tools/bash/config_test.clj (3)
9-14: LGTM!The test correctly verifies that
bash-over-nrepldefaults tofalsewhen not explicitly configured.
16-27: LGTM!The test correctly verifies that
bash-over-nreplcan be explicitly set totrueorfalsevia configuration.
81-100: LGTM!The test correctly verifies that
process-confighandles thebash-over-nreplparameter fortrue,false, and missing values, ensuring that missing values don't add the key to the processed config.test/clojure_mcp/tools/project/tool_test.clj (1)
93-97: All call sites are consistent; code change is correct.Verification confirms all
core/inspect-projectinvocations pass the atom directly:
src/clojure_mcp/tools/project/tool.clj:52passesnrepl-client-atomsrc/clojure_mcp/resources.clj:130passesnrepl-client-atomsrc/clojure_mcp/agent/general_agent.clj:48passesnrepl-client-atomtest/clojure_mcp/tools/project/tool_test.clj:96passes*client-atom*The change from
@*client-atom*to*client-atom*aligns this test with the established pattern throughout the codebase.
bhauman
changed the title
Summary
This PR is a major refactor of the nREPL evaluation code to make it more robust.
Changes
Removed needless multiplexing: Previously, evaluations were unnecessarily multiplexed. This has been simplified so that individual tool evaluations now open a socket and send the eval directly.
Cleaner architecture: The refactored code is more straightforward and easier to reason about.
Motivation
This work lays the groundwork for the next step: making ClojureMCP optionally start without a port and allowing more dynamic REPL interactions by making the port an optional parameter in the eval tool.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.