Config validation#104
Conversation
- Add metosin/malli dependency for data-driven schema validation - Create comprehensive config schemas in clojure-mcp.config.schema - Support all configuration options including models, agents, resources, prompts - Provide automatic spell-checking for typos in configuration keys - Include human-readable error messages with documentation references - Add comprehensive test suite for validation scenarios - Support environment variable references [:env "VAR_NAME"] - Validate nested structures like thinking config, agent config, etc. - Use closed map schema to detect unknown/misspelled keys
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Malli-based configuration schema and validators, wires pre-merge validation into config loading with canonical paths and improved error reporting, updates docs and example agent tool IDs from hyphens to underscores, introduces :track-file-changes, removes an agent option, adds tests, adds malli dependency, and replaces clojure.spec model validation with the new schema. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Startup as Startup
participant Core as core/fetch-config
participant Loader as config/load-config
participant Schema as config.schema
Note over Startup,Core: Startup requests configuration load
Startup->>Core: fetch-config
Core->>Loader: load-config-handling-validation-errors(home, project)
Loader->>Loader: build list [{:config <map> :file-path <canonical-path>} ...]
Loader->>Schema: validate-configs(list)
alt any invalid
Schema-->>Loader: explain-config errors
Loader-->>Core: throw ex-info ::schema-error with {:errors :model :config :file-path}
Core->>Startup: print validation report and rethrow/exit
else all valid
Schema-->>Loader: valid
Loader->>Loader: merge validated configs
Loader-->>Core: merged-config
Core->>Startup: continue startup with merged config
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 4
🧹 Nitpick comments (7)
test/clojure_mcp/config/schema_test.clj (2)
44-47: Tool ID style in tests differs from docs (hyphens vs underscores).Docs migrated to underscore IDs; this test still uses hyphens. Consider updating for consistency or explicitly covering both forms.
- :enable-tools [:clojure-eval :read-file] + :enable-tools [:clojure_eval :read_file]Do example configs under resources/ also use underscores consistently?
137-142: Add tests for PromptEntry XOR rule once schema enforces it.Cover both “both provided” and “neither provided” cases.
+ (deftest prompt-content-xor-file-path-test + (testing "Both :content and :file-path should be invalid" + (let [config {:prompts {"p" {:description "d" :content "x" :file-path "p.mustache"}}}] + (is (some? (schema/explain-config config))))) + (testing "Neither :content nor :file-path should be invalid" + (let [config {:prompts {"p" {:description "d"}}}] + (is (some? (schema/explain-config config))))))Also applies to: 198-203
doc/configuring-agents.md (3)
76-85: Fix MD058: add blank lines around tables.Surround each table with a blank line to satisfy markdownlint.
-### Read-Only Tools -| Tool ID | Description | +### Read-Only Tools + +| Tool ID | Description | |---------|-------------| | `:LS` | Directory tree view | | `:read_file` | Read file contents with pattern matching | | `:grep` | Search file contents | | `:glob_files` | Find files by pattern | | `:think` | Reasoning tool | | `:clojure_inspect_project` | Project structure analysis | + -### Evaluation Tools -| Tool ID | Description | +### Evaluation Tools + +| Tool ID | Description | |---------|-------------| | `:clojure_eval` | Execute Clojure code in REPL | | `:bash` | Execute shell commands | -### File Editing Tools -| Tool ID | Description | + +### File Editing Tools + +| Tool ID | Description | |---------|-------------| | `:file_write` | Create or overwrite files | | `:file_edit` | Edit files by text replacement | | `:clojure_edit` | Structure-aware Clojure editing | | `:clojure_edit_replace_sexp` | S-expression replacement | -### Agent Tools -| Tool ID | Description | + +### Agent Tools + +| Tool ID | Description | |---------|-------------| | `:dispatch_agent` | Launch sub-agents | | `:architect` | Technical planning assistant | | `:scratch_pad` | Persistent data storage | | `:code_critique` | Code review feedback |Also applies to: 86-94, 94-101, 100-107
301-306: Tool ID typo: use underscore form.Use
clojure_inspect_projectto match the rest of the doc and examples.-- Current project structure from `clojure-inspect-project` +- Current project structure from `clojure_inspect_project`
510-514: Tighten guidance: explicitly call out underscore tool IDs.Small doc tweak to minimize confusion migrating from hyphens.
- - Check tool IDs match exactly (use underscores: `:read_file` not `:read-file`) + - Check tool IDs match exactly (underscore IDs): `:read_file` (not `:read-file`)src/clojure_mcp/core.clj (1)
262-267: Add info log after loading config; fix README doc linkAdd an info log after loading the config to record config-file and user-dir. Repo contains CONFIG.md at repo root, but README.md links to doc/CONFIG.md — update README or move CONFIG.md.
- (let [config (load-config-handling-validation-errors config-file user-dir) + (let [config (load-config-handling-validation-errors config-file user-dir) + _ (log/info "Loaded configuration" {:file config-file :user-dir user-dir}) final-env-type (or cli-env-type (if (contains? config :nrepl-env-type) (:nrepl-env-type config) env-type))] (assoc nrepl-client-map ::config/config (assoc config :nrepl-env-type final-env-type))))Found: CONFIG.md (repo root). References: README.md:1005 -> doc/CONFIG.md; src/clojure_mcp/core.clj prints "See CONFIG.md" (~line 250).
src/clojure_mcp/config.clj (1)
130-144: Pre-merge validation flow is correct; consider not swallowing EDN parse errors.Current load-config-file logs and returns {} on read/parse failure, which then skips validation here and silently ignores a broken file. Consider promoting read errors to an exception (with :type ::read-error and :file-path) so users aren’t surprised by ignored configs.
If you want, I can draft a minimal change to load-config-file that preserves existing happy-paths but throws on parse errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deps.edn(1 hunks)doc/configuring-agents.md(15 hunks)resources/clojure-mcp/agents/clojure_edit_agent.edn(0 hunks)resources/configs/example-agents.edn(3 hunks)src/clojure_mcp/config.clj(4 hunks)src/clojure_mcp/config/schema.clj(1 hunks)src/clojure_mcp/core.clj(1 hunks)test/clojure_mcp/config/schema_test.clj(1 hunks)
💤 Files with no reviewable changes (1)
- resources/clojure-mcp/agents/clojure_edit_agent.edn
🧰 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/core.cljsrc/clojure_mcp/config/schema.cljsrc/clojure_mcp/config.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/core.cljsrc/clojure_mcp/config/schema.cljtest/clojure_mcp/config/schema_test.cljsrc/clojure_mcp/config.clj
test/**/*.{clj,cljc}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
deftestwith descriptive names;testingfor subsections;isfor assertions in tests
Files:
test/clojure_mcp/config/schema_test.clj
🧠 Learnings (3)
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
PR: bhauman/clojure-mcp#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/core.cljsrc/clojure_mcp/config/schema.cljtest/clojure_mcp/config/schema_test.cljsrc/clojure_mcp/config.clj
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
PR: bhauman/clojure-mcp#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/config.clj
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
PR: bhauman/clojure-mcp#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-02T20:23:28.499Z
Learning: Applies to src/**/*.{clj,cljc} : Include clear tool `:description` for LLM guidance
Applied to files:
doc/configuring-agents.md
🪛 markdownlint-cli2 (0.17.2)
doc/configuring-agents.md
87-87: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
93-93: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
101-101: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🔇 Additional comments (10)
deps.edn (1)
11-13: Approve — metosin/malli 0.19.1 is current.
0.19.1 (released June 9, 2025) is the latest stable release — no changes required to deps.edn.src/clojure_mcp/config/schema.clj (1)
151-158: Align agent-level tool ID types with top-level config (keywords vs. strings)Top-level :enable-tools/:disable-tools accept keywords or strings; agent-level schema in src/clojure_mcp/config/schema.clj (lines ~151–158) only allows keywords — either loosen the agent schema to allow strings for parity or document that agent-level IDs must be keywords.
Suggested parity diff (apply if you want agent-level === top-level):
- [:enable-tools {:optional true - :description "Tools the agent can access: :all, specific list, or nil (no tools)"} - [:or [:= :all] [:sequential :keyword]]] + [:enable-tools {:optional true + :description "Tools the agent can access: :all, specific list, or nil (no tools)"} + [:or [:= :all] [:sequential [:or :keyword :string]]]] - [:disable-tools {:optional true - :description "Tools to exclude even if enabled (applied after enable-tools)"} - [:maybe [:sequential :keyword]]] + [:disable-tools {:optional true + :description "Tools to exclude even if enabled (applied after enable-tools)"} + [:maybe [:sequential [:or :keyword :string]]]]Would config/tool-id-enabled? and related filters accept string IDs at the agent level today?
src/clojure_mcp/config.clj (2)
6-6: Good: schema ns alias added and aligned with file layout.Namespace aliasing follows project conventions and matches src/clojure_mcp/config/schema.clj.
118-118: Docstring matches behavior.Now accurately states “Validates both configs before merging.”
resources/configs/example-agents.edn (6)
17-17: Underscored tool IDs: research-agent entry looks consistent.:enable-tools updated to [:grep :glob_files :read_file :clojure_inspect_project]. Matches new naming.
25-26: Underscored tool IDs: refactor_assistant entry is consistent.
33-35: Underscored tool IDs: test_explorer enable/disable lists are consistent.
42-42: Underscored tool IDs: doc_reader entry is consistent.
51-52: Underscored tool IDs: code_writer entry is consistent (long token preserved).
62-62: full-access-agent updated correctly — manual repo verification requiredrg returned "No files were searched" so I couldn't confirm absence of deprecated hyphenated tool IDs. Re-run locally:
# Search with ripgrep (wide search, ignore node_modules/target) rg -n --hidden --no-ignore -S -E 'read-file|glob-files|clojure-inspect-project|dispatch-agent' --glob '!*node_modules/**' --glob '!*target/**' || true # If rg still skips files, run with --debug: rg --debug -n --hidden --no-ignore -S -E 'read-file|glob-files|clojure-inspect-project|dispatch-agent' || true # Fallback using find+grep: find . \( -path './node_modules' -o -path './target' -o -path './.git' \) -prune -o -type f -print0 | xargs -0 -r grep -nE 'read-file|glob-files|clojure-inspect-project|dispatch-agent' || true
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/clojure_mcp/config/schema.clj (1)
206-227: Resolved: enforce exactly one of :content or :file-path.The XOR check via [:and ... [:fn ...]] is correct and addresses prior feedback.
🧹 Nitpick comments (6)
src/clojure_mcp/config/schema.clj (6)
13-16: Disallow empty paths.Paths shouldn’t be empty strings. Recommend length check.
Apply:
-(def Path - "Schema for file system paths" - :string) +(def Path + "Schema for file system paths" + [:string {:min 1}])
17-21: Tighten EnvRef var name to be non-empty.Prevents [:env ""] footguns.
Apply:
(def EnvRef "Schema for environment variable references like [:env \"VAR_NAME\"]" [:tuple {:description "Environment variable reference"} [:= :env] - :string]) + [:string {:min 1}]])
40-41: Provider enum is missing :gemini.You expose a :gemini submap below; allow it in :provider too or clarify docs.
Apply:
- [:provider {:optional true} [:enum :openai :anthropic :google]] + [:provider {:optional true} [:enum :openai :anthropic :google :gemini]]
96-97: Constrain OpenAI logit-bias to common range.OpenAI typically enforces [-100, 100]. Bound the ints to reduce invalid configs.
Apply:
- [:logit-bias {:optional true} [:map-of :string :int]] + [:logit-bias {:optional true} [:map-of :string [:int {:min -100 :max 100}]]]If you’ve seen different limits recently, keep as-is and update the bounds accordingly.
143-146: Description mentions default but schema doesn’t set one.Either declare a default or drop “(default)” from the description.
Apply one:
Option A (add default):
- [:context {:optional true - :description "Context to provide: true (default), false (none), or file paths list"} + [:context {:optional true + :description "Context to provide: true (default), false (none), or file paths list" + :default true} [:or :boolean [:sequential :string]]]Option B (doc fix):
- [:context {:optional true - :description "Context to provide: true (default), false (none), or file paths list"} + [:context {:optional true + :description "Context to provide: true/false, or file paths list"} [:or :boolean [:sequential :string]]]
162-164: Set default true for :track-file-changes or adjust wording.Doc says “(default: true)” but schema has no default.
Apply:
- [:track-file-changes {:optional true - :description "Whether to track and display file diffs (default: true)"} + [:track-file-changes {:optional true + :description "Whether to track and display file diffs (default: true)" + :default true} :boolean]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/clojure_mcp/config.clj(4 hunks)src/clojure_mcp/config/schema.clj(1 hunks)src/clojure_mcp/core.clj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/clojure_mcp/config.clj
- src/clojure_mcp/core.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/config/schema.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/schema.clj
🧠 Learnings (1)
📚 Learning: 2025-08-02T20:23:28.499Z
Learnt from: CR
PR: bhauman/clojure-mcp#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/schema.clj
🔇 Additional comments (4)
src/clojure_mcp/config/schema.clj (4)
1-7: Good namespace and aliases.Uses :require with aliases per guidelines; concise docstring.
281-289: Nice humanized error reporting with spell-checking.Good UX for config authors.
299-311: Schema introspection helpers look good.Handy for docs; no issues spotted.
233-276: Approve: top-level Config keys look good; no stale coerce-config referencesClosed map and types are sensible; provider/tool filters consistent. ripgrep found no matches for
coerce-configand no occurrences of:provider :gemini— “gemini” only appears in model names, tests, and provider-specific handlers (e.g. src/clojure_mcp/config/schema.clj, src/clojure_mcp/agent/langchain.clj, src/clojure_mcp/agent/langchain/model.clj).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/clojure_mcp/config/schema.clj (10)
11-15: Defaulting validate-env-vars to true is surprising; default to falsePrevents configs with env refs from failing in dev/CI unless explicitly enabled.
Apply:
-(def ^:dynamic *validate-env-vars* - "When true, validates that environment variables actually exist and aren't blank. - Can be bound to false for testing." - true) +(def ^:dynamic *validate-env-vars* + "When true, validates that environment variables actually exist and aren't blank. + Default false to avoid surprising failures; bind to true in strict environments." + false)
30-41: Tighten EnvRef: validate ENV name formatAvoid obviously bad names; keep runtime existence check as-is.
Apply:
(def EnvRef "Schema for environment variable references like [:env \"VAR_NAME\"]" [:and [:tuple {:description "Environment variable reference"} [:= :env] NonBlankString] + [:fn {:error/message "ENV var name must match [A-Z_][A-Z0-9_]*"} + (fn [[_ env-var]] + (boolean (re-matches #"[A-Z_][A-Z0-9_]*" env-var)))] [:fn {:error/message "Environment variable can't be empty"} (fn [[_ env-var]] (or (not *validate-env-vars*) (let [val (System/getenv env-var)] (and val (not (string/blank? val))))))]])
86-91: Require :schema when :response-format :type is :jsonPrevents runtime failures when asking for JSON without a schema.
Apply:
- [:response-format {:optional true} - [:map {:closed true} - [:type [:enum :json :text]] - [:schema {:optional true} :map]]] + [:response-format {:optional true} + [:and + [:map {:closed true} + [:type [:enum :json :text]] + [:schema {:optional true} :map]] + [:fn {:error/message "When :response-format :type is :json, provide :schema"} + (fn [{:keys [type schema]}] + (if (= type :json) (some? schema) true))]]]
110-117: Constrain OpenAI :logit-bias values to API rangeOpenAI expects -100..100; catch out-of-range earlier.
Apply:
- [:logit-bias {:optional true} [:map-of NonBlankString :int]] + [:logit-bias {:optional true} [:map-of NonBlankString [:int {:min -100 :max 100]]]]
154-157: Use Path for file path list in :contextDescription says “file paths list”; enforce Path.
Apply:
- [:context {:optional true - :description "Context to provide: true (default), false (none), or file paths list"} - [:or :boolean [:sequential NonBlankString]]] + [:context {:optional true + :description "Context to provide: true (default), false (none), or file paths list"} + [:or :boolean [:sequential Path]]]
159-166: Allow explicit nil for :enable-tools to match docDoc says “:all, specific list, or nil”; permit nil when key is present.
Apply:
- [:enable-tools {:optional true - :description "Tools the agent can access: :all, specific list, or nil (no tools)"} - [:or [:= :all] [:sequential :keyword]]] + [:enable-tools {:optional true + :description "Tools the agent can access: :all, specific list, or nil (no tools)"} + [:maybe [:or [:= :all] [:sequential :keyword]]]]
168-171: Allow explicit nil for :memory-size per descriptionKeeps schema aligned with “false/nil/<10 stateless”.
Apply:
- [:memory-size {:optional true - :description "Memory behavior: false/nil/<10 = stateless, >=10 = persistent window"} - [:or [:= false] [:int {:min 0}]]] + [:memory-size {:optional true + :description "Memory behavior: false/nil/<10 = stateless, >=10 = persistent window"} + [:maybe [:or [:= false] [:int {:min 0}]]]]
191-197: Drop [:maybe ...] on optional keys (:url, :mime-type)Key is already optional; allowing explicit nil adds little and can confuse.
Apply:
- [:url {:optional true - :description "Custom URL for resource (defaults to custom://kebab-case-name)"} - [:maybe NonBlankString]] + [:url {:optional true + :description "Custom URL for resource (defaults to custom://kebab-case-name)"} + NonBlankString] @@ - [:mime-type {:optional true - :description "MIME type (auto-detected from file extension if not specified)"} - [:maybe NonBlankString]]]) + [:mime-type {:optional true + :description "MIME type (auto-detected from file extension if not specified)"} + NonBlankString]])
257-259: Use Path for :scratch-pad-fileThis is a file path; enforce with Path like elsewhere.
Apply:
- [:scratch-pad-file {:optional true} NonBlankString] + [:scratch-pad-file {:optional true} Path]
261-286: Add referential integrity and uniqueness checks (agents/models)Catch config mistakes early: duplicate agent ids and unknown model refs.
Apply:
- [:agents {:optional true} [:sequential AgentConfig]] + [:agents {:optional true} [:sequential AgentConfig]] + ;; Cross-field validations + [:fn {:error/message "Agent :id values must be unique"} + (fn [{:keys [agents]}] + (let [ids (map :id agents)] + (= (count ids) (count (distinct ids)))))] + [:fn {:error/message "Each agent's :model must reference a key under :models"} + (fn [{:keys [agents models]}] + (let [model-keys (set (keys models))] + (every? (fn [{:keys [model]}] + (or (nil? model) (contains? model-keys model))) + agents)))]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/clojure_mcp/config/schema.clj(1 hunks)test/clojure_mcp/config/schema_test.clj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/clojure_mcp/config/schema_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/config/schema.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/schema.clj
🔇 Additional comments (6)
src/clojure_mcp/config/schema.clj (6)
1-9: Namespace and requires look goodAlias usage and ns-path alignment are correct.
20-25: NonBlankString helper is solidCovers empty and whitespace-only strings.
203-216: PromptArg schema reads wellClear, minimal, and fits usage.
217-239: Nice: XOR between :content and :file-pathPrevents the common footgun. Good addition.
292-305: Validation helpers LGTMexplain-config + valid? are straightforward.
310-322: Schema introspection helpers LGTMHandy for docs; no issues.
bhauman
changed the title
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 (1)
src/clojure_mcp/agent/langchain/model.clj (1)
314-323: Bug: Uses :gemini key but schema defines provider-specific config under :google.This makes Gemini options unreachable from validated configs.
- (get-in config [:gemini :allow-code-execution]) - (.allowCodeExecution (get-in config [:gemini :allow-code-execution])) - (get-in config [:gemini :include-code-execution-output]) - (.includeCodeExecutionOutput (get-in config [:gemini :include-code-execution-output])) - (get-in config [:gemini :response-logprobs]) - (.responseLogprobs (get-in config [:gemini :response-logprobs])) - (get-in config [:gemini :enable-enhanced-civic-answers]) - (.enableEnhancedCivicAnswers (get-in config [:gemini :enable-enhanced-civic-answers])) - (get-in config [:gemini :logprobs]) - (.logprobs (int (get-in config [:gemini :logprobs]))))))) + (get-in config [:google :allow-code-execution]) + (.allowCodeExecution (get-in config [:google :allow-code-execution])) + (get-in config [:google :include-code-execution-output]) + (.includeCodeExecutionOutput (get-in config [:google :include-code-execution-output])) + (get-in config [:google :response-logprobs]) + (.responseLogprobs (get-in config [:google :response-logprobs])) + (get-in config [:google :enable-enhanced-civic-answers]) + (.enableEnhancedCivicAnswers (get-in config [:google :enable-enhanced-civic-answers])) + (get-in config [:google :logprobs]) + (.logprobs (int (get-in config [:google :logprobs])))))))
♻️ Duplicate comments (1)
src/clojure_mcp/config/schema.clj (1)
245-248: Nice: XOR constraint on :content vs :file-path is in place.This closes the footgun where neither or both could be provided.
🧹 Nitpick comments (2)
src/clojure_mcp/config/schema.clj (2)
164-167: Use Path for file path lists in :context.Schema says “file paths list” but validates generic strings. Prefer Path for consistency.
- [:context {:optional true - :description "Context to provide: true (default), false (none), or file paths list"} - [:or :boolean [:sequential NonBlankString]]] + [:context {:optional true + :description "Context to provide: true (default), false (none), or file paths list"} + [:or :boolean [:sequential Path]]]
30-41: Make env lookup overridable for tests and align with model namespace.EnvRef validation always calls System/getenv, ignoring test overrides used elsewhere. Add an overridable getter.
+(def ^:dynamic *get-env* + "Overrideable env lookup used by schema-time checks (defaults to System/getenv)." + (fn [k] (System/getenv k))) (def EnvRef "Schema for environment variable references like [:env \"VAR_NAME\"]" [:and [:tuple {:description "Environment variable reference"} [:= :env] NonBlankString] [:fn {:error/message "Environment variable can't be empty"} (fn [[_ env-var]] (or (not *validate-env-vars*) - (let [val (System/getenv env-var)] + (let [val (*get-env* env-var)] (and val (not (string/blank? val))))))]])Usage in tests:
(binding [clojure-mcp.config.schema/get-env (fn [k] (get {"OPENAI_API_KEY" "x"} k))] ...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/clojure_mcp/agent/langchain/model.clj(4 hunks)src/clojure_mcp/agent/langchain/model_spec.clj(0 hunks)src/clojure_mcp/config/schema.clj(1 hunks)test/clojure_mcp/agent/langchain/model_spec_test.clj(0 hunks)
💤 Files with no reviewable changes (2)
- test/clojure_mcp/agent/langchain/model_spec_test.clj
- src/clojure_mcp/agent/langchain/model_spec.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/agent/langchain/model.cljsrc/clojure_mcp/config/schema.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/agent/langchain/model.cljsrc/clojure_mcp/config/schema.clj
🔇 Additional comments (6)
src/clojure_mcp/config/schema.clj (2)
254-297: LGTM on top-level Config shape and key metadata.Closed maps + spell-checking and added descriptions improve UX.
337-360: Validation helpers look good.Clear ex-info payloads with humanized Malli errors.
src/clojure_mcp/agent/langchain/model.clj (4)
4-4: Good switch to schema validators.Alias and dependency update match the migration away from spec.
406-408: Validation hook on builder creation: good.Model key + config validated before use.
430-431: Validation on create-builder-from-config: good.
514-516: Validation on nREPL-driven path: good.
1 participant
Summary
This PR enhances the configuration schema validation by adding comprehensive field descriptions and cleaning up unused code.
Changes
1. Added Field Descriptions to ResourceEntry Schema
:description,:file-path,:url,:mime-type2. Added Field Descriptions to PromptEntry and PromptArg Schemas
:name,:description,:required?:description,:content,:file-path,:args3. Removed Unused coerce-config Function
coerce-configfunction fromschema.cljwhich was just a pass-throughcoercion-testfromschema_test.cljTesting
Impact
These changes make the configuration schemas more consistent and self-documenting, improving the developer experience when working with ClojureMCP configurations.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests