Add dry_run parameter to file editing tools #121
Conversation
- Add optional dry-run parameter that accepts 'diff' or 'new-source' - When dry-run is set, skip file save, timestamp update, and Emacs highlight - With 'diff' mode: returns diff of proposed changes - With 'new-source' mode: returns complete new source code - Parameter is not exposed in tool schema (internal use for clients) - Addresses issue #115
- Extract and pass dry-run parameter through validate-inputs - Enable dry-run modes: "diff" (returns diff) and "new-source" (returns complete source) - Both modes skip file write operations while preserving all other pipeline steps - Clean up debug logging 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Change parameter name from dry-run to dry_run (with underscore) - Update all references in file_edit and form_edit tools - Fix key mismatch in validate-inputs return map 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add dry_run parameter to sexp-edit-pipeline - Extract and pass dry_run through validate-inputs and execute-tool - Update format-results to handle new-source output - Skip file write operations when dry_run is set 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughDry-run mode is introduced across file-edit and form-edit tool pipelines. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Tool
participant Pipeline
participant FileOps
Client->>Tool: execute-tool (with dry_run)
Tool->>Tool: validate inputs (extract dry_run)
Tool->>Pipeline: call pipeline (dry_run parameter)
Pipeline->>Pipeline: transform content
alt dry_run is set
Pipeline->>Pipeline: skip I/O (save-file, highlight-form)
Pipeline->>Pipeline: format-result (new-source or diff)
else dry_run is nil
Pipeline->>FileOps: save-file
Pipeline->>FileOps: update-file-timestamp
Pipeline->>FileOps: highlight-form
Pipeline->>Pipeline: format-result (standard result)
end
Pipeline-->>Tool: result with diff/new-source
Tool->>Tool: format-results (handle new-source)
Tool-->>Client: formatted response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Changes span six files with consistent patterns (dry-run parameter threading, context spec extensions, conditional I/O skipping, result formatting updates), but require understanding multiple interconnected pipeline systems and validation logic. The heterogeneity across file-edit and form-edit tools and test updates demands separate reasoning per component despite pattern regularity. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/clojure_mcp/tools/form_edit/combined_edit_tool.clj (1)
111-124: Add dry_run to the tool schema properties.The tool schema is missing the
dry_runparameter definition, preventing LLMs from discovering and using this feature. The parameter is destructured and used in validation (line 129) and execution (line 156), but isn't documented in the schema.Apply this diff to add the parameter to the schema:
(defmethod tool-system/tool-schema :clojure-edit-form [_] {:type "object" :required ["file_path" "form_identifier" "form_type" "operation" "content"] :properties {"file_path" {:type "string" :description "Path to the file containing the form to edit"} "form_identifier" {:type "string" :description "Complete identifier of the form (e.g., function name, \"shape/area\", or \"convert-length [:feet :inches]\")"} "form_type" {:type "string" :description "Type of the form (e.g., \"defn\", \"def\", \"ns\")"} "operation" {:type "string" :enum ["replace" "insert_before" "insert_after"] :description "The editing operation to perform"} "content" {:type "string" - :description "New content to use for the operation"}}}) + :description "New content to use for the operation"} + "dry_run" {:type "string" + :enum ["diff" "new-source"] + :description "Optional: return preview without writing. 'diff' returns unified diff, 'new-source' returns complete modified file content"}}})As per coding guidelines.
src/clojure_mcp/tools/file_edit/tool.clj (2)
41-49: Add dry_run to the tool schema properties.The tool schema is missing the
dry_runparameter definition. Although the parameter is used in execute-tool (line 84), it's not documented in the schema, preventing LLMs from discovering this feature.Apply this diff:
(defmethod tool-system/tool-schema :file-edit [_] {:type :object :properties {:file_path {:type :string :description "The absolute path to the file to modify (must be absolute, not relative)"} :old_string {:type :string :description "The text to replace (must match the file contents exactly, including all whitespace and indentation)."} :new_string {:type :string - :description "The edited text to replace the old_string"}} + :description "The edited text to replace the old_string"} + :dry_run {:type :string + :enum ["diff" "new-source"] + :description "Optional: return preview without writing. 'diff' returns unified diff, 'new-source' returns complete modified file content"}} :required [:file_path :old_string :new_string]})As per coding guidelines.
51-81: Validate dry_run parameter in validate-inputs.The
dry_runparameter is used in execute-tool (line 84) but isn't validated. If an invalid value is provided, it will silently pass through and potentially cause unexpected behavior.Apply this diff to add validation:
(defmethod tool-system/validate-inputs :file-edit [{:keys [nrepl-client-atom]} inputs] - (let [{:keys [file_path old_string new_string]} inputs + (let [{:keys [file_path old_string new_string dry_run]} inputs nrepl-client @nrepl-client-atom] ;; Check required parameters (when-not file_path (throw (ex-info "Missing required parameter: file_path" {:inputs inputs}))) (when-not (contains? inputs :old_string) (throw (ex-info "Missing required parameter: old_string" {:inputs inputs}))) (when-not (contains? inputs :new_string) (throw (ex-info "Missing required parameter: new_string" {:inputs inputs}))) + ;; Validate dry_run if provided + (when (and dry_run (not (#{"diff" "new-source"} dry_run))) + (throw (ex-info (str "Invalid dry_run value: " dry_run ". Must be 'diff' or 'new-source'") + {:inputs inputs}))) + ;; Reject empty old_string - direct users to file_write instead (when (empty? old_string) (throw (ex-info "Empty old_string is not supported. To create a new file, use file_write instead." {:inputs inputs}))) ;; Check for identical strings (early rejection) (when (= old_string new_string) (throw (ex-info "No changes to make: old_string and new_string are exactly the same." {:inputs inputs}))) ;; Validate path using the utility function (let [validated-path (valid-paths/validate-path-with-client file_path nrepl-client)] ;; Return validated inputs with normalized path (assoc inputs :file_path validated-path :old_string old_string - :new_string new_string)))) + :new_string new_string + :dry_run dry_run))))As per coding guidelines: "Validate inputs and provide helpful error messages in MCP tools".
src/clojure_mcp/tools/form_edit/tool.clj (4)
165-169: Fix arity mismatch - pass nil for dry_run parameter.The call to
pipeline/edit-form-pipelineis missing thedry_runparameter. The pipeline signature was updated to accept 7 parameters (line 577 in pipeline.clj), but this call only passes 6. This is causing the ArityException reported in the pipeline failures.Apply this diff:
(defmethod tool-system/execute-tool :clojure-edit-replace-form [{:keys [nrepl-client-atom] :as tool} inputs] (let [{:keys [file_path form_name form_type content]} inputs - result (pipeline/edit-form-pipeline file_path form_name form_type content :replace tool) + result (pipeline/edit-form-pipeline file_path form_name form_type content :replace nil tool) formatted-result (pipeline/format-result result)] formatted-result))
257-261: Fix arity mismatch - pass nil for dry_run parameter.The call to
pipeline/edit-form-pipelineis missing thedry_runparameter, causing an arity mismatch with the updated pipeline signature.Apply this diff:
(defmethod tool-system/execute-tool :clojure-edit-insert-before-form [{:keys [nrepl-client-atom] :as tool} inputs] (let [{:keys [file_path form_name form_type content]} inputs - result (pipeline/edit-form-pipeline file_path form_name form_type content :before tool) + result (pipeline/edit-form-pipeline file_path form_name form_type content :before nil tool) formatted-result (pipeline/format-result result)] formatted-result))
349-353: Fix arity mismatch - pass nil for dry_run parameter.The call to
pipeline/edit-form-pipelineis missing thedry_runparameter, causing an arity mismatch with the updated pipeline signature.Apply this diff:
(defmethod tool-system/execute-tool :clojure-edit-insert-after-form [{:keys [nrepl-client-atom] :as tool} inputs] (let [{:keys [file_path form_name form_type content]} inputs - result (pipeline/edit-form-pipeline file_path form_name form_type content :after tool) + result (pipeline/edit-form-pipeline file_path form_name form_type content :after nil tool) formatted-result (pipeline/format-result result)] formatted-result))
525-543: Add dry_run to the tool schema properties.The
clojure_update_sexptool schema is missing thedry_runparameter definition. The parameter is used in validation (line 548) and execution (line 599) but isn't documented in the schema.Apply this diff:
(defmethod tool-system/tool-schema :clojure-update-sexp [{:keys [multi-op]}] {:type :object :properties (cond-> {:file_path {:type :string :description "Path to the file to edit"} :match_form {:type :string :description "The s-expression to find (include # for anonymous functions)"} :new_form {:type :string :description "The s-expression to use for the operation"} :replace_all {:type :boolean :description - (format "Whether to %s all occurrences (default: false)" - (if multi-op "apply operation to" "replace"))}} + (format "Whether to %s all occurrences (default: false)" + (if multi-op "apply operation to" "replace"))} + :dry_run {:type :string + :enum ["diff" "new-source"] + :description "Optional: return preview without writing. 'diff' returns unified diff, 'new-source' returns complete modified file content"}} multi-op (assoc :operation {:type :string :enum ["replace" "insert_before" "insert_after"] :description "The editing operation to perform"})) :required (cond-> [:file_path :match_form :new_form] multi-op (conj :operation))})As per coding guidelines.
🧹 Nitpick comments (1)
src/clojure_mcp/tools/form_edit/pipeline.clj (1)
611-650: Consider adding dry_run support to remaining pipelines.The
docstring-edit-pipeline(line 624) andsexp-replace-pipeline(line 752) don't include thedry_runparameter, whileedit-form-pipelineandsexp-edit-pipelinedo. This creates an inconsistent API surface.For feature completeness and consistency, consider adding dry_run support to these pipelines as well.
Would you like me to generate the changes needed to add dry_run support to these two pipelines?
Also applies to: 738-777
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/clojure_mcp/tools/file_edit/pipeline.clj(4 hunks)src/clojure_mcp/tools/file_edit/tool.clj(1 hunks)src/clojure_mcp/tools/form_edit/combined_edit_tool.clj(2 hunks)src/clojure_mcp/tools/form_edit/pipeline.clj(6 hunks)src/clojure_mcp/tools/form_edit/tool.clj(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/tools/form_edit/tool.cljsrc/clojure_mcp/tools/file_edit/tool.cljsrc/clojure_mcp/tools/form_edit/pipeline.cljsrc/clojure_mcp/tools/form_edit/combined_edit_tool.cljsrc/clojure_mcp/tools/file_edit/pipeline.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/tools/form_edit/tool.cljsrc/clojure_mcp/tools/file_edit/tool.cljsrc/clojure_mcp/tools/form_edit/pipeline.cljsrc/clojure_mcp/tools/form_edit/combined_edit_tool.cljsrc/clojure_mcp/tools/file_edit/pipeline.clj
🪛 GitHub Actions: CI
src/clojure_mcp/tools/form_edit/tool.clj
[error] 167-167: ArityException: Wrong number of args (6) passed to: clojure-mcp.tools.form-edit.pipeline/edit-form-pipeline
src/clojure_mcp/tools/file_edit/tool.clj
[error] 167-167: ArityException: Wrong number of args (6) passed to: clojure-mcp.tools.form-edit.pipeline/edit-form-pipeline
src/clojure_mcp/tools/form_edit/combined_edit_tool.clj
[error] 167-167: ArityException: Wrong number of args (6) passed to: clojure-mcp.tools.form-edit.pipeline/edit-form-pipeline
🔇 Additional comments (5)
src/clojure_mcp/tools/file_edit/pipeline.clj (1)
27-27: LGTM!The dry-run implementation is well-structured:
- Spec correctly defines valid values (line 27)
- Parameter is properly threaded into context (line 131)
- File I/O operations are conditionally skipped (lines 154-160)
- Result formatting correctly handles both diff and new-source modes (lines 175-188)
Also applies to: 131-131, 154-160, 175-188
src/clojure_mcp/tools/form_edit/pipeline.clj (3)
46-46: LGTM!The dry-run implementation for
edit-form-pipelineis well-structured:
- Spec correctly defines valid values (line 46)
- Included in context spec (line 54)
- Parameter properly threaded into context (line 583)
- File I/O operations conditionally skipped (lines 603-609)
Also applies to: 54-54, 577-609
794-825: LGTM!The dry-run implementation for
sexp-edit-pipelinecorrectly mirrors the pattern used inedit-form-pipeline:
- Parameter accepted in signature (line 794)
- Stored in context (line 801)
- File operations conditionally bypassed (lines 819-825)
545-560: LGTM!The
format-resultfunction correctly handles both dry-run modes:
- Returns
:new-sourcewhendry_runis "new-source" (lines 551-552)- Returns
:difffor default and "diff" mode (lines 555-556)- Maintains backward compatibility with legacy
:resultkey (lines 559-560)src/clojure_mcp/tools/form_edit/tool.clj (1)
420-425: Thedocstring-edit-pipelinefunction does not require adry_runparameter. Its signature accepts 5 parameters:file-path,form-name,form-type,new-docstring, andconfig. The call at line 423 correctly passes 5 arguments with matching arity. Unlikeedit-form-pipeline, which has a distinct purpose and requires additional parameters includingdry_run,docstring-edit-pipelineis designed differently and functions correctly as called.Likely an incorrect or invalid review comment.
Updated three execute-tool methods in tool.clj and two test methods in pipeline_test.clj to pass the new dry_run parameter (as nil) to edit-form-pipeline, which now requires 7 arguments instead of 6. All 365 tests now pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
bhauman
force-pushed
the
issue-115-dry-run-parameter
branch
from
aa83a94 to
905170b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/clojure_mcp/tools/form_edit/tool.clj (1)
167-167: Thread dry_run support through individual form-edit tools for feature parity with combined toolThe
nilvalues for thedry_runparameter (lines 167, 259, 351) are functionally correct—they disable dry-run mode. However, for consistency withcombined_edit_tool.clj, which threadsdry_runfrom inputs, these three tools should also accept and pass through an optionaldry_runparameter. This would require:
- Adding
dry_runas an optional input in each tool's schema- Extracting and threading
dry_runthrough theedit-form-pipelinecall in eachexecute-toolmethod
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/clojure_mcp/tools/form_edit/tool.clj(5 hunks)test/clojure_mcp/tools/form_edit/pipeline_test.clj(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/clojure_mcp/tools/form_edit/pipeline_test.clj
🧰 Additional context used
📓 Path-based instructions (2)
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/tools/form_edit/tool.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/tools/form_edit/tool.clj
| (let [file-path (validate-file-path inputs nrepl-client-atom) | ||
| {:keys [match_form new_form operation replace_all whitespace_sensitive]} inputs] | ||
| {:keys [match_form new_form operation replace_all whitespace_sensitive dry_run]} inputs] | ||
| (when-not match_form |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate dry_run values and document in schema; also expose whitespace_sensitive
- dry_run isn’t validated; accept only "diff" or "new-source" and give a helpful error otherwise.
- Tool schema lacks dry_run and whitespace_sensitive, so clients may reject these inputs. Add both as optional properties with descriptions.
Suggested changes:
Validation (enforce enum):
(defmethod tool-system/validate-inputs :clojure-update-sexp
[{:keys [nrepl-client-atom multi-op]} inputs]
(let [file-path (validate-file-path inputs nrepl-client-atom)
- {:keys [match_form new_form operation replace_all whitespace_sensitive dry_run]} inputs]
+ {:keys [match_form new_form operation replace_all whitespace_sensitive dry_run]} inputs]
...
+ (when (some? dry_run)
+ (when-not (#{"diff" "new-source"} dry_run)
+ (throw (ex-info "Invalid dry_run: must be \"diff\" or \"new-source\""
+ {:inputs inputs :dry_run dry_run}))))
...
{:file_path file-path
...
- :whitespace_sensitive (boolean (or whitespace_sensitive false))
- :dry_run dry_run}))
+ :whitespace_sensitive (boolean (or whitespace_sensitive false))
+ :dry_run dry_run}))Schema (add properties):
(defmethod tool-system/tool-schema :clojure-update-sexp [{:keys [multi-op]}]
{:type :object
:properties
- (cond-> {:file_path {:type :string
+ (cond-> {:file_path {:type :string
:description "Path to the file to edit"}
:match_form {:type :string
:description "The s-expression to find (include # for anonymous functions)"}
:new_form {:type :string
:description "The s-expression to use for the operation"}
:replace_all {:type :boolean
:description
(format "Whether to %s all occurrences (default: false)"
(if multi-op "apply operation to" "replace"))}
+ :whitespace_sensitive {:type :boolean
+ :description "When true, treat insignificant whitespace as significant during matching (default: false)"}
+ :dry_run {:type :string
+ :enum ["diff" "new-source"]
+ :description "Dry run mode: \"diff\" returns unified diff; \"new-source\" returns full modified source; no file writes performed"}}
multi-op
(assoc :operation {:type :string
:enum ["replace" "insert_before" "insert_after"]
:description "The editing operation to perform"}))Also applies to: 596-597, 599-606
🤖 Prompt for AI Agents
In src/clojure_mcp/tools/form_edit/tool.clj around line 549 (and also apply same
changes at lines ~596-597 and 599-606): add validation that dry_run accepts only
the strings "diff" or "new-source" and return a clear error if any other value
is provided (e.g., check the param and when-not in the same place as match_form
to throw a helpful error). Then update the tool schema definition to include
optional properties "dry_run" (string, enum ["diff","new-source"], description
"If set, controls output mode; allowed values: 'diff' or 'new-source'") and
"whitespace_sensitive" (boolean, optional, description "If true, treat
whitespace as significant during edits"). Ensure the schema keys are optional
and documented, and wire the validated dry_run value into existing code paths
where dry_run is used.
Summary
Adds optional
dry_runparameter to file editing tools, allowing users to preview changes without writing to disk.Changes
Tools Modified
file_edit- File string replacement toolclojure_edit- Clojure form editing toolclojure_edit_replace_sexp- S-expression replacement toolDry Run Modes
dry_run="diff"- Returns unified diff of changes without writing filedry_run="new-source"- Returns complete modified file content without writingImplementation Details
::dry-runspec to pipeline contextTesting
All three tools tested with:
dry_run="diff"mode - verified no file writes, diff returneddry_run="new-source"mode - verified no file writes, full source returnedCommits
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features