feat(coaching): contextual coaching triggers (BM25 auto-match) - #213
Merged
Conversation
One file was doing data model, storage/CRUD, and CLI presentation. Split by concern: rule.rs (CoachingRule + coaching-<id>.md format), store.rs (rules_dir CRUD, apply/list/remove), cli.rs (print_list, cli_apply, cli_remove). mod.rs re-exports only what's consumed outside the module tree. Behavior unchanged -- build clean, all 473 tests pass.
Implement v2 spec: coaching rules can declare # Trigger: tool:<csv>; auto for contextual injection via PreToolUse/UserPromptSubmit instead of always firing at SessionStart. BM25 auto-match reuses existing crate::compact::score_lines (no new deps). Tasks: - RuleTrigger type + trigger-line format (rule.rs) - BM25 auto-match via score_lines, rule_bodies_for_tool/prompt (store.rs) - CLI --trigger-tool/--trigger-auto flags (cli.rs + cli/coaching.rs) - Hook wiring: SessionStart rename, PreToolUse, UserPromptSubmit (hook.rs)
📝 WalkthroughWalkthroughCoaching rules are reorganized into model, storage, and CLI modules. Rules can trigger on specific tools or prompt matches, with matching bodies injected into session-start, tool-use, and prompt-submit hooks. ChangesCoaching rule triggers
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RuleStore
participant RuleFile
participant Hook
CLI->>RuleStore: apply_rule with trigger configuration
RuleStore->>RuleFile: write coaching rule metadata and body
Hook->>RuleStore: query rules for session, tool, or prompt
RuleStore-->>Hook: matching rule bodies
Hook-->>Hook: append coaching bodies to hook output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
src/hook.rs (1)
547-565: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the hook outputs, not only the selectors.
These tests do not prove the new rules reach
SessionStart,PreToolUse, orUserPromptSubmitoutput. Refactor the input/output handling into testable helpers (or capture hook output) and assert the emitted JSON/message includes—and excludes—the expected rule bodies.
src/hook.rs#L547-L565: assertsession_start_messagecontains the untriggered rule and excludes triggered rules.src/hook.rs#L588-L607: invoke the pre-tool hook path with a matching tool payload and assert itssystemMessage.src/hook.rs#L609-L628: invoke the prompt-submit hook path with a matching prompt payload and assertadditionalContext.🤖 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/hook.rs` around lines 547 - 565, Refactor the hook input/output handling into testable helpers or capture the hook output, then update all three tests in src/hook.rs: lines 547-565 should assert session_start_message includes untriggered rule bodies and excludes triggered ones; lines 588-607 should invoke the PreToolUse path with a matching tool payload and assert systemMessage; lines 609-628 should invoke the UserPromptSubmit path with a matching prompt payload and assert additionalContext.
🤖 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/coaching/rule.rs`:
- Around line 68-85: Validate rule header fields before serialization in the
rule-writing path and format_trigger_line: reject titles and tool names
containing newlines or the grammar delimiters (commas/semicolons), and reject
empty RuleTrigger values that would parse back as None. Propagate the validation
error instead of writing invalid or semantically altered headers, including the
additional serialization path noted in the review.
- Around line 95-97: Validate the ID produced in the filename parsing flow after
`strip_prefix("coaching-")` and before listing or counting the rule, using the
same format validation enforced by `remove_rule`. Skip or reject filenames with
invalid IDs so they cannot contribute to `MAX_RULES` or appear as unremovable
CLI entries, while preserving valid rule handling.
In `@src/coaching/store.rs`:
- Around line 52-66: The rule count validation and file write in the coaching
store are not serialized across processes, allowing concurrent writes to exceed
MAX_RULES. Update the surrounding store operation to acquire a cross-process
filesystem lock before list_rules, overwrite validation, write_rule_file, and
the final reread, releasing it afterward; also publish the rule file via atomic
rename if supported by the existing rule-writing path.
---
Nitpick comments:
In `@src/hook.rs`:
- Around line 547-565: Refactor the hook input/output handling into testable
helpers or capture the hook output, then update all three tests in src/hook.rs:
lines 547-565 should assert session_start_message includes untriggered rule
bodies and excludes triggered ones; lines 588-607 should invoke the PreToolUse
path with a matching tool payload and assert systemMessage; lines 609-628 should
invoke the UserPromptSubmit path with a matching prompt payload and assert
additionalContext.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f82d83f-e0c0-4587-b81a-61415b33fdb9
📒 Files selected for processing (7)
src/cli/coaching.rssrc/coaching.rssrc/coaching/cli.rssrc/coaching/mod.rssrc/coaching/rule.rssrc/coaching/store.rssrc/hook.rs
💤 Files with no reviewable changes (1)
- src/coaching.rs
Comment on lines
+68
to
+85
| if tools.is_empty() && !auto_match { | ||
| None | ||
| } else { | ||
| Some(RuleTrigger { tools, auto_match }) | ||
| } | ||
| } | ||
|
|
||
| /// Inverse of `parse_trigger_line` — renders a `RuleTrigger` back into | ||
| /// the `tool:a,b; auto` text that goes after `# Trigger:`. | ||
| fn format_trigger_line(trigger: &RuleTrigger) -> String { | ||
| let mut parts = Vec::new(); | ||
| if !trigger.tools.is_empty() { | ||
| parts.push(format!("tool:{}", trigger.tools.join(","))); | ||
| } | ||
| if trigger.auto_match { | ||
| parts.push("auto".to_string()); | ||
| } | ||
| parts.join("; ") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate header fields before serialization.
Raw title and tool names can contain newlines or ,/;, injecting header fields or changing parsed tools. An empty trigger also round-trips to None, silently making the rule unconditional. Reject values outside the file grammar before writing.
Proposed validation
pub(super) fn write_rule_file(
dir: &std::path::Path,
id: &str,
title: &str,
body: &str,
trigger: Option<&RuleTrigger>,
) -> std::io::Result<()> {
+ let invalid_title = title.chars().any(|c| matches!(c, '\r' | '\n'));
+ let invalid_trigger = trigger.is_some_and(|trigger| {
+ (!trigger.auto_match && trigger.tools.is_empty())
+ || trigger.tools.iter().any(|tool| {
+ tool.is_empty()
+ || tool != tool.trim()
+ || tool.chars().any(|c| matches!(c, ',' | ';' | '\r' | '\n'))
+ })
+ });
+ if invalid_title || invalid_trigger {
+ return Err(std::io::Error::new(
+ std::io::ErrorKind::InvalidInput,
+ "rule fields cannot be represented safely",
+ ));
+ }Also applies to: 157-163
🤖 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/coaching/rule.rs` around lines 68 - 85, Validate rule header fields
before serialization in the rule-writing path and format_trigger_line: reject
titles and tool names containing newlines or the grammar delimiters
(commas/semicolons), and reject empty RuleTrigger values that would parse back
as None. Propagate the validation error instead of writing invalid or
semantically altered headers, including the additional serialization path noted
in the review.
- Fix overindented doc-list continuation lines in coaching/mod.rs (clippy::doc_overindented_list_items was failing CI). - Validate rule fields before serialization: reject titles containing newlines, tool names containing newlines/commas/semicolons, and Some(RuleTrigger) values with no tools and auto_match=false that would silently round-trip back to None on reparse. - Skip rule files whose filename-derived id fails is_valid_rule_id, so a malformed filename can never count toward MAX_RULES or become an unremovable entry. - Serialize apply_rule/remove_rule across processes with a cross-process advisory lock (create_new sentinel file), closing a TOCTOU race where concurrent callers could both pass the MAX_RULES check and both write. - Publish rule files via write-then-rename so a concurrent reader never observes a partially written file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/coaching/store.rs`:
- Around line 42-47: Replace the fixed-timeout sentinel deletion in the
lock-acquisition logic around OpenOptions with an OS-backed advisory lock, or
implement a renewable ownership-token lease whose token is verified before any
cleanup. Update the relevant lock type’s Drop behavior so a process cannot
remove another process’s active lock, and preserve exclusive serialization of
count-check/write/remove operations even when a writer runs longer than two
seconds.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e0e16a1-5559-4619-93c8-c1ecb9469ebb
📒 Files selected for processing (4)
src/coaching/mod.rssrc/coaching/rule.rssrc/coaching/store.rssrc/hook.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/coaching/mod.rs
- src/coaching/rule.rs
- src/hook.rs
Comment on lines
+42
to
+47
| let _ = std::fs::remove_file(&path); | ||
| std::fs::OpenOptions::new() | ||
| .write(true) | ||
| .create_new(true) | ||
| .open(&path)?; | ||
| Ok(Self { path }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not break a live lock after a fixed timeout.
A slow but healthy writer can exceed two seconds; a second process then deletes its sentinel and enters the critical section. When the first process exits, Drop can also unlink the second process’s lock. This reintroduces concurrent count-check/write/remove operations and can exceed MAX_RULES or lose updates.
Use an OS-backed advisory lock that the kernel releases on process exit, or a renewable lease with an ownership token that is verified before deletion.
🤖 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/coaching/store.rs` around lines 42 - 47, Replace the fixed-timeout
sentinel deletion in the lock-acquisition logic around OpenOptions with an
OS-backed advisory lock, or implement a renewable ownership-token lease whose
token is verified before any cleanup. Update the relevant lock type’s Drop
behavior so a process cannot remove another process’s active lock, and preserve
exclusive serialization of count-check/write/remove operations even when a
writer runs longer than two seconds.
4 tasks
This was referenced Jul 25, 2026
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
1 participant
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
# Trigger:line:tool:<names>(fires via PreToolUse) and/orauto(fires via UserPromptSubmit, scored with BM25 against the rule's own title+body using the existingcrate::compact::score_lines— same scorer built for PreCompact, zero new deps, no threshold to tune)untriggered_rule_bodies())src/coaching.rsintosrc/coaching/{rule,store,cli}.rs(data model / storage / CLI)agentflare coaching applygains--trigger-tool <name>(repeatable) and--trigger-autoflagsTest plan
cargo build --workspace --all-featuresclean, zero warningscargo test --workspace --all-featuresgreen (495 passed, 0 failed)cargo fmt --checkcleanSummary by CodeRabbit
--trigger-tool(repeatable) and--trigger-auto.