-
-
Notifications
You must be signed in to change notification settings - Fork 76
fix: auto-correct dash-to-underscore filename mismatches in path validation (#48) #161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,8 @@ | |
| [clojure-mcp.tools.unified-read-file.tool :as unified-read-file-tool] | ||
| [clojure-mcp.tool-system :as tool-system] | ||
| [clojure-mcp.config :as config] | ||
| [clojure.java.io :as io])) | ||
| [clojure.java.io :as io] | ||
| [clojure.string :as str])) | ||
|
|
||
| ;; Setup test fixtures | ||
| (test-utils/apply-fixtures *ns*) | ||
|
|
@@ -27,9 +28,10 @@ | |
| (try | ||
| (f) | ||
| (finally | ||
| ;; Clean up | ||
| ;; Clean up all files recursively | ||
| (when (.exists test-dir) | ||
| (.delete test-dir))))))) | ||
| (doseq [file (reverse (file-seq test-dir))] | ||
| (.delete file)))))))) | ||
|
|
||
| (use-fixtures :each setup-test-files-fixture) | ||
|
|
||
|
|
@@ -93,3 +95,88 @@ | |
| formatted-str (first formatted)] | ||
| (is (not (re-find #"truncated" formatted-str)) | ||
| "Should not show truncation message when not truncated")))) | ||
|
|
||
| ;; --- Dash-to-underscore filename correction integration tests --- | ||
| ;; The core correction logic is tested in valid_paths_test.clj. | ||
| ;; These tests verify the read_file tool pipeline works with the correction. | ||
|
|
||
| (deftest dash-to-underscore-correction-test | ||
| (testing "File with dashes requested, underscore version exists - reads successfully" | ||
| (let [underscore-file (io/file *test-dir* "core_stuff.clj") | ||
| _ (spit underscore-file "(ns core-stuff)\n(defn hello [] :world)") | ||
| tool-instance (unified-read-file-tool/create-unified-read-file-tool *nrepl-client-atom*) | ||
| dash-path (.getAbsolutePath (io/file *test-dir* "core-stuff.clj")) | ||
| validated (tool-system/validate-inputs tool-instance {:path dash-path}) | ||
| result (tool-system/execute-tool tool-instance validated) | ||
| formatted (tool-system/format-results tool-instance result)] | ||
| (is (not (:error formatted)) "Should successfully read the corrected file") | ||
| (is (some #(str/includes? % "core-stuff") (:result formatted)) | ||
| "Should contain the file content") | ||
| (.delete underscore-file))) | ||
|
|
||
| (testing "File with underscores requested directly - reads successfully" | ||
| (let [underscore-file (io/file *test-dir* "core_stuff.clj") | ||
| _ (spit underscore-file "(ns core-stuff)\n(defn hello [] :world)") | ||
| tool-instance (unified-read-file-tool/create-unified-read-file-tool *nrepl-client-atom*) | ||
| underscore-path (.getAbsolutePath underscore-file) | ||
| validated (tool-system/validate-inputs tool-instance {:path underscore-path}) | ||
| result (tool-system/execute-tool tool-instance validated) | ||
| formatted (tool-system/format-results tool-instance result)] | ||
| (is (not (:error formatted)) "Should successfully read the file") | ||
| (.delete underscore-file))) | ||
|
|
||
| (testing "Neither dash nor underscore version exists - returns normal error" | ||
| (let [tool-instance (unified-read-file-tool/create-unified-read-file-tool *nrepl-client-atom*) | ||
| non-existent-path (.getAbsolutePath (io/file *test-dir* "no-such-file.clj"))] | ||
| (is (thrown-with-msg? clojure.lang.ExceptionInfo | ||
| #"does not exist" | ||
| (tool-system/validate-inputs tool-instance {:path non-existent-path})) | ||
| "Should throw an error when neither file version exists"))) | ||
|
|
||
| (testing "Non-Clojure files (.java) - do NOT auto-correct" | ||
| (let [underscore-file (io/file *test-dir* "core_stuff.java") | ||
| _ (spit underscore-file "public class core_stuff {}") | ||
| tool-instance (unified-read-file-tool/create-unified-read-file-tool *nrepl-client-atom*) | ||
| dash-path (.getAbsolutePath (io/file *test-dir* "core-stuff.java"))] | ||
| (is (thrown-with-msg? clojure.lang.ExceptionInfo | ||
| #"does not exist" | ||
| (tool-system/validate-inputs tool-instance {:path dash-path})) | ||
| "Should NOT auto-correct non-Clojure file extensions") | ||
| (.delete underscore-file))) | ||
|
|
||
| (testing "Directory components with dashes - only filename corrected" | ||
| (let [dashed-dir (io/file *test-dir* "my-cool-dir") | ||
| _ (.mkdirs dashed-dir) | ||
| underscore-file (io/file dashed-dir "my_file.clj") | ||
| _ (spit underscore-file "(ns my-cool-dir.my-file)") | ||
| tool-instance (unified-read-file-tool/create-unified-read-file-tool *nrepl-client-atom*) | ||
| dash-path (.getAbsolutePath (io/file dashed-dir "my-file.clj")) | ||
| validated (tool-system/validate-inputs tool-instance {:path dash-path}) | ||
| result (tool-system/execute-tool tool-instance validated) | ||
| formatted (tool-system/format-results tool-instance result)] | ||
| (is (not (:error formatted)) "Should read file with dashed directory and corrected filename") | ||
| (is (str/includes? (:path validated) "my-cool-dir") | ||
| "Directory dashes should be preserved") | ||
| (.delete underscore-file) | ||
| (.delete dashed-dir))) | ||
|
|
||
| (testing "Full pipeline through make-tool-tester with dash correction" | ||
| (let [underscore-file (io/file *test-dir* "pipeline_test.clj") | ||
| _ (spit underscore-file "(ns pipeline-test)\n(defn greet [name] (str \"Hello \" name))") | ||
| tool-instance (unified-read-file-tool/create-unified-read-file-tool *nrepl-client-atom*) | ||
| tool-fn (test-utils/make-tool-tester tool-instance) | ||
| dash-path (.getAbsolutePath (io/file *test-dir* "pipeline-test.clj")) | ||
| result (tool-fn {:path dash-path})] | ||
| (is (not (:error? result)) "Full pipeline should succeed with dash correction") | ||
| (.delete underscore-file))) | ||
|
|
||
| (testing "Babashka (.bb) files - do NOT auto-correct" | ||
| (let [underscore-file (io/file *test-dir* "my_script.bb") | ||
| _ (spit underscore-file "(println :hello)") | ||
| tool-instance (unified-read-file-tool/create-unified-read-file-tool *nrepl-client-atom*) | ||
| dash-path (.getAbsolutePath (io/file *test-dir* "my-script.bb"))] | ||
| (is (thrown-with-msg? clojure.lang.ExceptionInfo | ||
| #"does not exist" | ||
| (tool-system/validate-inputs tool-instance {:path dash-path})) | ||
| "Should NOT auto-correct .bb files") | ||
| (.delete underscore-file)))) | ||
|
Comment on lines
+103
to
+182
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify response-shape fields for unified-read-file and whether correction metadata exists.
# Inspect response map construction in the tool implementation.
rg -n -C5 'defn|format-results|execute|validate|:result|:error|:path|corrected|correction|notice|original' src/clojure_mcp/tools/unified_read_file
# Inspect existing tests for assertions on correction metadata/notice.
rg -n -C3 'dash-to-underscore|corrected|correction|notice|original|:path validated|format-results' test/clojure_mcp/tools/unified_read_file/tool_test.cljRepository: bhauman/clojure-mcp Length of output: 50377 🏁 Script executed: #!/bin/bash
# Search for correction-related metadata in response building
rg -n "correct|original|dash|underscore" src/clojure_mcp/tools/unified_read_file/tool.clj | head -30
# Check if there's any mention of what should be in the response for corrections
rg -n "corrected\|correction\|auto.*correct" src/clojure_mcp/tools/unified_read_file/
# Look at the valid_paths module to see what validate-path-with-client returns
find src -name "*valid*" -o -name "*path*" | head -20Repository: bhauman/clojure-mcp Length of output: 149 🏁 Script executed: #!/bin/bash
# Check the valid_paths implementation to understand path correction
find src -type f -name "*.clj" | xargs rg -l "validate-path-with-client" | head -5Repository: bhauman/clojure-mcp Length of output: 271 🏁 Script executed: #!/bin/bash
# Look for the actual implementation of dash-to-underscore correction
rg -n "dash\|underscore\|kebab" src/clojure_mcp/tools/unified_read_file/ -A5 -B5Repository: bhauman/clojure-mcp Length of output: 45 🏁 Script executed: #!/bin/bash
# Inspect valid_paths module
cat src/clojure_mcp/utils/valid_paths.cljRepository: bhauman/clojure-mcp Length of output: 12403 🏁 Script executed: #!/bin/bash
# Check if the tool response includes any correction metadata
rg -n "original\|corrected\|correction" src/clojure_mcp/tools/unified_read_file/tool.cljRepository: bhauman/clojure-mcp Length of output: 45 🏁 Script executed: #!/bin/bash
# Verify test expectations - what fields are being checked in format-results
sed -n '103,182p' test/clojure_mcp/tools/unified_read_file/tool_test.clj | grep -E "is |let \[|formatted|result"Repository: bhauman/clojure-mcp Length of output: 1634 🏁 Script executed: #!/bin/bash
# Check what fields validate-path-with-client actually returns
sed -n '78,120p' src/clojure_mcp/utils/valid_paths.cljRepository: bhauman/clojure-mcp Length of output: 1922 🏁 Script executed: #!/bin/bash
# Look at the full validate-inputs implementation to see all returned fields
sed -n '111,141p' src/clojure_mcp/tools/unified_read_file/tool.cljRepository: bhauman/clojure-mcp Length of output: 1408 🏁 Script executed: #!/bin/bash
# Check execute-tool return structure - does it include correction metadata?
sed -n '143,217p' src/clojure_mcp/tools/unified_read_file/tool.clj | grep -E ":path|:error|:original|:corrected|:mode" | head -20Repository: bhauman/clojure-mcp Length of output: 602 🏁 Script executed: #!/bin/bash
# Check format-results to see what fields are in final response
sed -n '275,299p' src/clojure_mcp/tools/unified_read_file/tool.cljRepository: bhauman/clojure-mcp Length of output: 909 Add assertions verifying correction metadata is included in tool responses. Tests confirm the dash-to-underscore correction works functionally, but they don't verify that the correction is communicated to the MCP client. The current implementation silently corrects file paths without surfacing this correction in the response. Responses need to include correction metadata (corrected filename and indication that auto-correction occurred) so users and client tools understand what path was actually accessed. This aligns with the guideline to "Validate inputs and provide helpful error messages in MCP tool implementations." Add assertions to verify:
Example assertions to include:
🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: bhauman/clojure-mcp
Length of output: 47980
🏁 Script executed:
Repository: bhauman/clojure-mcp
Length of output: 1295
🏁 Script executed:
Repository: bhauman/clojure-mcp
Length of output: 6567
🏁 Script executed:
Repository: bhauman/clojure-mcp
Length of output: 1400
🏁 Script executed:
Repository: bhauman/clojure-mcp
Length of output: 45
🏁 Script executed:
Repository: bhauman/clojure-mcp
Length of output: 1438
Correction metadata should be surfaced in read_file responses.
The path validation transparently corrects dash-to-underscore mismatches but returns only the corrected path string without metadata. The downstream response from
format-resultsincludes the corrected path only as embedded text in markdown, not as an explicit flag or notice in the response structure. Per issue#48,read_fileshould include both the corrected filename and an explicit correction indicator (e.g.,{"corrected_path": "...", "original_path": "...", "notice": "..."}) in its structured response, not just in the formatted markdown text.🤖 Prompt for AI Agents