Add optional CLOJURE_MCP_TRAINING_DIR training-log emit - #164
Open
theselbalancingscooter wants to merge 3 commits into
Open
Add optional CLOJURE_MCP_TRAINING_DIR training-log emit#164theselbalancingscooter wants to merge 3 commits into
theselbalancingscooter wants to merge 3 commits into
Conversation
Off by default. When the env var is set to a writable directory, every tool call gets appended to session-<uuid>-turns.jsonl and a session summary is snapshotted on JVM shutdown. The emitted shape matches the schema used by an external training-corpus pipeline for LLM fine-tuning, so downstream ingests can read the files directly. Why: LLM users of clojure-mcp produce high-signal training data (REPL-verified structural editing) that's hard to capture from other IDEs. Making the emit an opt-in env var means anyone building a training pipeline can wire this up without asking users to run a fork. Design: - New ns clojure_mcp.training_log — pure emit, no network I/O. - One-line change to `create-async-tool`'s continuation to call training-log/record-tool-call! after the tool's clj-result-k fires. - Zero effect on tool behaviour: emit runs post-response, and any exception in the emit path is WARN-logged + swallowed so a training-log bug cannot break tool responses. - No new dependencies — uses clojure.data.json + timbre, both already transitive. Sensitive files: traces contain tool arguments (file contents, shell commands, eval expressions) and results. README section calls this out and suggests treating the training dir as private. Tests: hermetic (tmp-dir per test). Covers happy path, disabled no-op, error flag, monotonic turn-index, and the invariant that emit never throws on I/O failure. Reference PRD: this shape matches agent/docs/PRD-clojure-mcp-corpus.md in a sibling repo, which describes the external ingest expectations. No hard coupling: any consumer that reads the documented JSONL shape works.
📝 WalkthroughWalkthroughThe server adds opt-in per-session training logs for completed MCP tool calls. It writes JSONL records, creates an EDN shutdown summary, isolates logging failures, documents sensitive data capture, and adds hermetic tests. ChangesTraining logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPToolInvocation
participant training_log
participant TrainingLogFiles
MCPToolInvocation->>training_log: record-tool-call!(tool-name, arg-map, result-strs, error?)
training_log->>TrainingLogFiles: append JSONL record
training_log->>training_log: update session metadata
training_log->>TrainingLogFiles: write EDN summary on JVM shutdown
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/clojure_mcp/core.clj`:
- Around line 119-122: Update the callback flow around
training-log/record-tool-call! and mono-fill-k so mono-fill-k signals the
adapted tool result before training-log I/O runs. Dispatch the training record
through a bounded asynchronous writer, preserving the existing opt-in and
non-throwing behavior while preventing filesystem delays or exceptions from
blocking the MCP response.
In `@src/clojure_mcp/training_log.clj`:
- Around line 98-100: In src/clojure_mcp/training_log.clj lines 98-100, replace
the broad Throwable catch around the training emit with specific expected
exception handling, and increment the atom-based failure counter when an emit
fails. Apply the same specific catch and failure-state update to the
summary-write handling at lines 123-125, reusing the shared atom and preserving
the existing warning context.
- Around line 75-97: The turn-index reservation and JSONL append in the
training-log emission flow must be atomic and serialized. Update the enclosing
function around session-state, turn, and line construction so concurrent
callbacks reserve unique sequential :turn-index values and append records in
that same order through one lock or serialized writer; add a concurrent emission
test covering uniqueness and ordering.
In `@test/clojure_mcp/training_log_test.clj`:
- Around line 87-92: Update emit-never-throws-on-io-failure-test to use a
temporary failure fixture: create a regular file within fresh-dir, configure
CLOJURE_MCP_TRAINING_DIR to that file path, invoke tl/record-tool-call!, and
assert that no turn file is created. Remove the non-portable /proc dependency
while preserving the no-exception assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ec5a810-5380-4653-94fc-8aaa392a1cfe
📒 Files selected for processing (4)
README.mdsrc/clojure_mcp/core.cljsrc/clojure_mcp/training_log.cljtest/clojure_mcp/training_log_test.clj
Four review findings, all addressed:
1. Don't block MCP response on emit I/O (core.clj + training_log.clj)
- core.clj: call mono-fill-k BEFORE record-tool-call! so the
client response goes out first even if the ordering ever mattered.
- training_log.clj: record-tool-call! now submits to a
single-thread ExecutorService (writer-executor) and returns
immediately. Zero response latency from disk I/O.
2. Atomic turn-index reservation + concurrent test (training_log.clj)
- record-tool-call-sync! (runs on the writer thread) uses
swap-vals! for a single-CAS reserve+increment. Reading the old
value and swapping are one atomic operation.
- Single-thread executor guarantees append order matches submission
order — no reordering across the append.
- New test: concurrent-emit-preserves-unique-indexes-test hammers
50 submissions across 8 threads, asserts no duplicate indexes
and full 0..N-1 range.
3. Catch Exception not Throwable + failure counter (training_log.clj)
- Both catches (per-turn record and shutdown summary write) now
catch Exception, letting fatal Error subclasses (OOM, ThreadDeath)
propagate as they should.
- New public `failure-count` atom is bumped on every swallowed
exception. Ops can graph training-log emit health without
scraping logs. Matches the CLAUDE.md guideline "atom for
tracking errors".
4. Portable I/O-failure test fixture (training_log_test.clj)
- Replaced /proc/1/... with a portable fixture: create a regular
file, point CLOJURE_MCP_TRAINING_DIR at that file path, invoke
record-tool-call!, assert no exception + failure-count bumped +
no JSONL file created (proves the failure path actually ran).
Also: shutdown hook now drains the writer-executor before writing the
summary, so the turn-count in summary.edn matches what actually landed
in the JSONL.
Tests: 6 tests / 16 assertions (was 5/11). Core suite unchanged
(6/17). All hermetic.
Author
|
Addressed all 4 CodeRabbit findings in
Bonus: shutdown hook now drains the writer-executor before writing summary, so Tests: 6/16 (was 5/11), core suite unchanged (6/17). All hermetic. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/clojure_mcp/training_log_test.clj (1)
1-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo dedicated test for shutdown draining / summary accuracy.
The PR description lists "shutdown draining" among the tested behaviors, but none of the six
deftestblocks here exerciseflush-and-write-summary!or assert that the EDN summary's counts match the JSONL content after a drain. Consider adding a test that calls#'tl/flush-and-write-summary!directly and asserts the summary file's:turn-countmatches the number of persisted JSONL lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/clojure_mcp/training_log_test.clj` around lines 1 - 145, Add a dedicated deftest for shutdown draining around tl/flush-and-write-summary! that records multiple tool calls, waits for queued writes, invokes the summary flush, and reads the generated EDN summary. Assert the summary’s :turn-count equals the number of persisted JSONL lines, covering both drain completion and summary accuracy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/clojure_mcp/training_log.clj`:
- Around line 14-17: Make the background writer executor used by
record-tool-call! bounded, replacing the default unbounded queue created by
Executors/newSingleThreadExecutor with an explicitly capacity-limited executor
configuration. Preserve immediate non-blocking submission and the existing
RejectedExecutionException handling, ensuring the docstring’s “bounded” behavior
matches the implementation.
- Around line 175-182: Update flush-and-write-summary! to capture the boolean
result of .awaitTermination and log a warning when it returns false, indicating
the 5-second drain timed out before all writer tasks completed. Preserve the
existing InterruptedException handling and write-summary! flow.
- Around line 90-126: Separate persisted-record accounting from the reservation
state in record-tool-call-sync!: do not advance the summary-facing :turn-count
or :tools-used before io/make-parents and spit succeed. Keep turn-index
reservation available for record ordering, then update the successful-write
counters only after the JSONL append completes, and ensure write-summary!
reports those persisted counters so failed writes are excluded.
---
Nitpick comments:
In `@test/clojure_mcp/training_log_test.clj`:
- Around line 1-145: Add a dedicated deftest for shutdown draining around
tl/flush-and-write-summary! that records multiple tool calls, waits for queued
writes, invokes the summary flush, and reads the generated EDN summary. Assert
the summary’s :turn-count equals the number of persisted JSONL lines, covering
both drain completion and summary accuracy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 806984a7-b34a-48fc-b942-4dc3446ac961
📒 Files selected for processing (3)
src/clojure_mcp/core.cljsrc/clojure_mcp/training_log.cljtest/clojure_mcp/training_log_test.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- src/clojure_mcp/core.clj
Comment on lines
+14
to
+17
| Zero effect on tool behaviour: | ||
| - The MCP response is NOT blocked on emit I/O. The interceptor | ||
| hands the (name, args, result) triple to a bounded background | ||
| writer and returns immediately. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Docstring claims a "bounded" writer, but the executor queue is unbounded.
Line 16 says the interceptor "hands the (name, args, result) triple to a bounded background writer." Executors/newSingleThreadExecutor (Line 71) wraps an unbounded LinkedBlockingQueue. Under sustained disk I/O backpressure, the queue can grow without limit and risk memory exhaustion. The existing RejectedExecutionException catch in record-tool-call! (Lines 142-146) only fires after .shutdown, so it never protects against queue growth today.
Bound the queue so the documented behavior matches the implementation, or update the docstring to say "unbounded" if that is the intended tradeoff.
🔧 Proposed fix to bound the writer queue
(defonce ^:private ^ExecutorService writer-executor
(let [thread-factory
(reify java.util.concurrent.ThreadFactory
(newThread [_ r]
(doto (Thread. r "clojure-mcp-training-log")
(.setDaemon true))))]
- (Executors/newSingleThreadExecutor thread-factory)))
+ (java.util.concurrent.ThreadPoolExecutor.
+ 1 1 0 TimeUnit/MILLISECONDS
+ (java.util.concurrent.LinkedBlockingQueue. 1000)
+ thread-factory)))Also applies to: 62-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/clojure_mcp/training_log.clj` around lines 14 - 17, Make the background
writer executor used by record-tool-call! bounded, replacing the default
unbounded queue created by Executors/newSingleThreadExecutor with an explicitly
capacity-limited executor configuration. Preserve immediate non-blocking
submission and the existing RejectedExecutionException handling, ensuring the
docstring’s “bounded” behavior matches the implementation.
1. Docstring "bounded" -> "serialised" + unbounded-queue rationale (training_log.clj:14-24). newSingleThreadExecutor's queue IS unbounded — that's intentional for this opt-in dev tool: silently dropping training records to protect memory is worse than the records themselves, and unbounded queue growth only happens when disk I/O is stalled indefinitely (a system-wide problem the operator is already seeing). 2. Two-stage turn accounting fixes summary-vs-JSONL drift (training_log.clj record-tool-call-sync!). Previous impl bumped :turn-count + :tools-used before the spit — if the write threw, the summary reported N while the JSONL had M<N. Now: reservation atomically bumps :turn-index only. :turn-count and :tools-used advance ONLY after a successful spit. Summary counts match what actually landed. Failed writes leave an honest gap in the index sequence (a curator UI can detect it). Regression test: write-failure-does-not-drift-turn-count-test — with an I/O failure fixture, asserts turn-count stays 0 and failure-count bumps twice. 3. Checked .awaitTermination result (training_log.clj flush-and-write-summary!). Discarded false let write-summary! run against unsettled state. Now: WARN-log + bump failure-count on drain timeout so operators can detect incomplete flushes. 4. New test flush-and-summary-drain-writes-accurate-counts-test. Drives 3 record submissions, calls flush-and-write-summary!, asserts JSONL has 3 lines AND summary EDN's :turn-count = 3. Regression fixture for the "summary reports N while JSONL has M<N" class of bug the review flagged. Test-fixture helper (test file): ensure-writer-executor! rebuilds the executor if a prior test shut it down. Without this the drain test would break every test that runs after it. Tests: 8 tests, 23 assertions, 0 failures (was 6/16). Core suite unchanged.
Author
|
Addressed CodeRabbit round-2 findings in
Tests: 8 / 23 (was 6/16). Core suite unchanged. |
There was a problem hiding this comment.
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/training_log_test.clj (1)
42-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore all modified training-log state.
with-training-dir*resetstl/session-stateandtl/failure-count, but thefinallyblock does not restore their original values. Later tests can observe state from this test namespace.Proposed fix
(defn- with-training-dir* [dir f] - (let [original @#'tl/training-dir] + (let [original @#'tl/training-dir + original-enabled? @#'tl/enabled? + original-session-state @@#'tl/session-state + original-failure-count `@tl/failure-count`] (try ... (finally (alter-var-root #'tl/training-dir (constantly original)) - (alter-var-root #'tl/enabled? (constantly (some? original))))))) + (alter-var-root #'tl/enabled? (constantly original-enabled?)) + (reset! @#'tl/session-state original-session-state) + (reset! tl/failure-count original-failure-count)))))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/clojure_mcp/training_log_test.clj` around lines 42 - 55, Update with-training-dir* to capture the original values of tl/session-state and tl/failure-count before resetting them, then restore both atoms in the finally block alongside training-dir and enabled?.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/clojure_mcp/training_log_test.clj`:
- Around line 42-55: Update with-training-dir* to capture the original values of
tl/session-state and tl/failure-count before resetting them, then restore both
atoms in the finally block alongside training-dir and enabled?.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f178e31c-4297-4b01-bb62-264c165013b3
📒 Files selected for processing (2)
src/clojure_mcp/training_log.cljtest/clojure_mcp/training_log_test.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- src/clojure_mcp/training_log.clj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an opt-in JSONL emit hook to clojure-mcp so users building
LLM training-corpus pipelines can capture their tool-call
trajectories without asking downstream users to run a fork.
Off by default. Enabled by setting the
CLOJURE_MCP_TRAINING_DIRenvironment variable to a writable directory before starting the
server. Zero effect on existing users — the interceptor no-ops when
the env var is unset.
How it works
Every tool call already flows through
clojure-mcp.core/create-async-tool'sclj-result-kcontinuation. This PR adds ONE line inside thatcontinuation to call
training-log/record-tool-call!after thetool has produced its result but before the response goes back to the
client. Each call appends one line to
session-<uuid>-turns.jsonl;a
session-<uuid>-summary.ednsnapshots on JVM shutdown.Emitted line shape (JSON, one per tool call):
{"session-id":"…uuid…", "turn-index":0, "model":"unknown", "tool-calls":[{"tool":"clojure_eval", "input":{"code":"(+ 1 2)"}, "output":"3", "status":"ok", "ms":null}], "outcome":"verified", "timestamp":"2026-08-02T…Z"}Design constraints
is WARN-logged and swallowed. A bug in the training-log ns CANNOT
break a tool response. Test asserts this invariant.
clojure.data.json+taoensso.timbre(both already transitive on
org.clojure/data.jsonandcom.taoensso/timbre).commands, and eval expressions. README section calls this out and
suggests treating the training dir as private.
Non-goals
server is out of scope.
before publishing.
dispatch path.
Changes
src/clojure_mcp/training_log.clj— new (90 LOC)src/clojure_mcp/core.clj— +2 lines (require + one-line interceptor)test/clojure_mcp/training_log_test.clj— new (~100 LOC, hermetic)README.md— new '## 📊 Optional: Training-log emit' sectionTest plan
clojure -M:test -n clojure-mcp.training-log-test— 5 tests,11 assertions, 0 failures (covers happy path, disabled no-op,
error flag, monotonic turn-index, emit-never-throws invariant)
clojure -M:test -n clojure-mcp.core-test— 6 tests, 17assertions, 0 failures (existing tests unchanged)
Reference
The emitted shape is designed to be consumed by any pipeline that
reads the documented JSONL. The pipeline that motivated this PR is
described in a sibling repo's
docs/PRD-clojure-mcp-corpus.md— nohard coupling; any consumer reading this shape works.
Summary by CodeRabbit
New Features
CLOJURE_MCP_TRAINING_DIRis configured, the server records JSONL traces and an EDN session summary containing inputs, outputs, statuses, and timestamps.Documentation