Detect babashka scripts as Clojure files #87
Conversation
WalkthroughExpanded Clojure file detection centralizes in utils/valid_paths.clj, adding Babashka shebang support and .lpy recognition. File write and unified read tools now delegate to this helper. Unified read additionally excludes .edn. Tests added for shebang detection across write, read, and utility layers. Minor docstring/require adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FileWrite as file_write/core
participant ValidPaths as utils/valid_paths
participant FS as Filesystem
Caller->>FileWrite: write-file(path, content)
FileWrite->>ValidPaths: clojure-file?(path)
ValidPaths->>FS: path-exists?(path)
alt exists
ValidPaths->>FS: read first line
ValidPaths-->>ValidPaths: check ext or Babashka shebang
end
ValidPaths-->>FileWrite: true/false
FileWrite-->>Caller: proceed per detection (unchanged flow)
sequenceDiagram
participant Caller
participant UnifiedRead as unified_read_file/tool
participant ValidPaths as utils/valid_paths
participant FS as Filesystem
Caller->>UnifiedRead: collapsible-clojure-file?(path)
UnifiedRead->>ValidPaths: clojure-file?(path)
ValidPaths->>FS: optional read first line
ValidPaths-->>UnifiedRead: true/false
UnifiedRead-->>Caller: true if clojure-file? and not .edn
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (7)
src/clojure_mcp/utils/valid_paths.clj (2)
85-96: Broaden Babashka shebang detection to cover env -S and /usr/local installs.Many bb scripts use
#!/usr/bin/env -S bb(to allow args), and bb may live under/usr/local/bin/bb. A small regex will reduce false negatives and allow trailing args.Apply this diff to the detection block:
- (let [line (-> r line-seq first)] - (and line - (or (str/starts-with? line "#!/usr/bin/env bb") - (str/starts-with? line "#!/usr/bin/bb") - (str/starts-with? line "#!/bin/bb"))))) + (let [line (-> r line-seq first)] + (boolean + (and line + (re-matches + #"^#!\s*(/usr/bin/env(\s+-S)?\s+bb|.*/bb)(\s|$).*" + line)))))This matches:
#!/usr/bin/env bb#!/usr/bin/env -S bb- Any shebang pointing to a
.../bbexecutable (e.g.,/usr/local/bin/bb,/usr/bin/bb,/bin/bb)- Optional trailing arguments after
bb
97-117: LGTM: Centralized, extension-first detection with bb shebang fallback.
- Extension checks are case-insensitive and short-circuit before file IO.
- Predicate naming and docstring are clear.
- Inclusion of
.lpyis a useful addition.Minor docstring polish to reflect the broader shebang support would help future readers.
Apply this small docstring update:
- Also detects files starting with a Babashka shebang (`bb`)." + Also detects files starting with a Babashka shebang (e.g., `#!/usr/bin/env bb`, + `#!/usr/bin/env -S bb`, or any shebang pointing to a `.../bb` executable)."test/clojure_mcp/utils/valid_paths_test.clj (1)
105-116: Solid coverage; make cleanup resilient with try/finally.If an assertion or write fails mid-block, the temp file delete might be skipped. Wrap in try/finally to guarantee cleanup.
Apply this diff:
(testing "Detect Babashka shebang" (let [tmp (io/file (System/getProperty "java.io.tmpdir") "bb-script.sh")] - (spit tmp "#!/usr/bin/env bb\n(println :hi)") - (is (valid-paths/clojure-file? (.getPath tmp))) - (.delete tmp))) + (try + (spit tmp "#!/usr/bin/env bb\n(println :hi)") + (is (valid-paths/clojure-file? (.getPath tmp))) + (finally + (.delete tmp))))) (testing "Regular bash script not detected" (let [tmp (io/file (System/getProperty "java.io.tmpdir") "bash-script.sh")] - (spit tmp "#!/bin/bash\necho hi") - (is (not (valid-paths/clojure-file? (.getPath tmp)))) - (.delete tmp)))) + (try + (spit tmp "#!/bin/bash\necho hi") + (is (not (valid-paths/clojure-file? (.getPath tmp)))) + (finally + (.delete tmp))))))Optional: add cases for
#!/usr/bin/env -S bband/usr/local/bin/bbonce the regex change lands.test/clojure_mcp/tools/file_write/core_test.clj (1)
57-63: Good behavior test; ensure temp file cleanup with try/finally.This makes the test robust against mid-block failures.
Apply this diff:
(testing "Detecting Babashka shebang" (let [tmp (io/file *test-dir* "script.sh")] - (spit tmp "#!/usr/bin/env bb\n(println :hi)") - (is (file-write-core/is-clojure-file? (.getPath tmp))) - (.delete tmp)))) + (try + (spit tmp "#!/usr/bin/env bb\n(println :hi)") + (is (file-write-core/is-clojure-file? (.getPath tmp))) + (finally + (.delete tmp))))))Optional: consider adding a
.lpyextension case in this test block to exercise the new extension support as well.test/clojure_mcp/tools/unified_read_file/tool_test.clj (1)
64-67: LGTM; add try/finally for temp file deletion.Keeps the test directory tidy even on failures.
Apply this diff:
- (let [tmp (io/file *test-dir* "script.sh")] - (spit tmp "#!/usr/bin/env bb\n(println :hi)") - (is (unified-read-file-tool/collapsible-clojure-file? (.getPath tmp))) - (.delete tmp)) + (let [tmp (io/file *test-dir* "script.sh")] + (try + (spit tmp "#!/usr/bin/env bb\n(println :hi)") + (is (unified-read-file-tool/collapsible-clojure-file? (.getPath tmp))) + (finally + (.delete tmp))))src/clojure_mcp/tools/file_write/core.clj (1)
12-21: Predicate delegation is correct; sync write-file docstring for .bb/shebang.
booleancoercion avoids leaking nils; delegating to utils keeps behavior consistent across tools.The write-file docstring still lists only
.clj/.cljs/.cljc/.edn. Since.bband bb-shebangs are now handled as Clojure files here, update the docstring accordingly.Proposed docstring tweak (outside this hunk):
;; In write-file docstring, replace: ;; "For Clojure files (.clj, .cljs, .cljc, .edn), applies linting and formatting." ;; with: ;; "For Clojure files (.clj, .cljs, .cljc, .edn, .bb) or files with a Babashka shebang, applies linting and formatting."src/clojure_mcp/tools/unified_read_file/tool.clj (1)
33-37: Optional: Centralize the “collapsible” predicate in utils.valid-pathsTests in
tool_test.cljalready cover the EDN exclusion (see line 68), so no extra tests are needed.
- Move the inline logic into
clojure_mcp.utils.valid-paths/collapsible-clojure-file?- Delegate from
unified_read_file/tool.cljto keep a single source of truth.Proposed diff in
src/clojure_mcp/tools/unified_read_file/tool.clj:(defn collapsible-clojure-file? "Determines if a file is a collapsible Clojure source file." [file-path] (when file-path - (and (valid-paths/clojure-file? file-path) - (not (str/ends-with? (str/lower-case file-path) ".edn"))))) + (valid-paths/collapsible-clojure-file? file-path)))Add helper in
src/clojure_mcp/utils/valid_paths.clj:(ns clojure_mcp.utils.valid-paths (:require [clojure.string :as str])) (defn collapsible-clojure-file? [path] (and (clojure-file? path) (not (str/ends-with? (str/lower-case (str path)) ".edn"))))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/clojure_mcp/tools/file_write/core.clj(1 hunks)src/clojure_mcp/tools/unified_read_file/tool.clj(1 hunks)src/clojure_mcp/utils/valid_paths.clj(1 hunks)test/clojure_mcp/tools/file_write/core_test.clj(1 hunks)test/clojure_mcp/tools/unified_read_file/tool_test.clj(1 hunks)test/clojure_mcp/utils/valid_paths_test.clj(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/unified_read_file/tool_test.cljtest/clojure_mcp/utils/valid_paths_test.cljtest/clojure_mcp/tools/file_write/core_test.cljsrc/clojure_mcp/tools/file_write/core.cljsrc/clojure_mcp/tools/unified_read_file/tool.cljsrc/clojure_mcp/utils/valid_paths.clj
test/**/*.{clj,cljc}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use
deftestwith descriptive names;testingfor subsections;isfor assertions in tests
Files:
test/clojure_mcp/tools/unified_read_file/tool_test.cljtest/clojure_mcp/utils/valid_paths_test.cljtest/clojure_mcp/tools/file_write/core_test.clj
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/file_write/core.cljsrc/clojure_mcp/tools/unified_read_file/tool.cljsrc/clojure_mcp/utils/valid_paths.clj
🔇 Additional comments (2)
src/clojure_mcp/utils/valid_paths.clj (1)
85-96: Safe and focused shebang read — nice guardrails.Good use of path-exists? and with-open with a try/catch to safely read just the first line. This keeps the check lightweight and failure-safe.
src/clojure_mcp/tools/file_write/core.clj (1)
5-11: Consolidating detection via valid-paths is the right move.The alias-based require is clean, and reusing a single source of truth for file-type detection reduces drift between tools.
Summary
bbshebangs inclojure-file?Testing
clojure -X:test(fails: command not found)https://chatgpt.com/codex/tasks/task_b_68a0c1b3724c83308265bb45eee7fe34
Summary by CodeRabbit