Merge pull request #31 from thinkgrid-labs/dev

fix: skip agent/ignore/doc files, report dropped rule sets, add rules…

Author: akosidencio

CHANGELOG.md

@@ -4,8 +4,40 @@
 
 Config is not code, and reviewing it as though it were produced confident
 nonsense. Reported from the field: every file under `.claude/` came back HIGH.
+Rule sets had the mirror-image problem — they could stop applying without ever
+saying so.
 
-### Fixed
+### Fixed — rule sets
+
+- **A rule set dropped for prompt budget is now reported**, with the budget and
+  the reason. Previously it vanished silently — the failure this feature was
+  otherwise built to avoid.
+- **The lowest severity is dropped first.** Rule sets load in filename order, so
+  overflow used to shed whichever sorted last: a `high` security set lost to a
+  style guide on the letter `s`. Severity is the only ranking the author
+  declared, so it decides. Surviving sets still *render* in filename order, so
+  the cache key does not move.
+
+### Added
+
+- **`!` exclusion globs in `scope`** — `["src/**", "!src/legacy/**"]`, the way
+  CodeRabbit's `path_filters` reads. An exclusion beats every include.
+- **`always: true`** to apply a rule set to every file regardless of `scope`.
+  An empty `scope` already meant this; now it can be said out loud, and
+  `rules list` marks it.
+- **`description:`** — one line for humans, shown by `rules list`, never sent to
+  the model.
+- **`globs:` and `alwaysApply:` accepted as aliases**, so a rule set ported from
+  `.cursor/rules/` loads unedited.
+- **`diffmind rules check`** — validates every rule set and pattern rule and
+  exits non-zero, for CI. Catches parse errors, duplicate ids, identical bodies,
+  scopes that match nothing, and invalid regex in `rules.toml`.
+- **A `scope` of only `!` exclusions is now a parse error.** It matched no file
+  at all, which is a rule set that silently does no work.
+- **`examples/nextjs-app-router/`** — a complete, working rule set for a
+  React/Next.js project: three scoped `.md` rule sets and 13 regex rules.
+
+### Fixed — what gets reviewed
 
 - **Coding-agent and editor config is no longer reviewed.** `.claude/`,
   `.cursor/`, `.windsurf/`, `.aider/`, `.vscode/`, `.idea/`, `.zed/` and

README.md

@@ -396,6 +396,42 @@ Your repository's own `.gitignore` is never touched.
 
 ## Your team's standards
 
+Everything lives in `.diffmind/`, committed to your repo:
+
+```
+.diffmind/
+├── config.toml        # settings — model, base branch, gate thresholds
+├── rules.toml         # regex rules: free, deterministic, never vary
+└── rules/*.md         # written rules: judgement, read by the model
+```
+
+### Set it up
+
+```bash
+diffmind rules init     # writes a starter .diffmind/rules/default.md
+# edit it, then:
+diffmind rules list     # shows what loads, its severity ceiling and its globs
+diffmind                # review with them applied
+```
+
+**A complete worked example lives in
+[`examples/nextjs-app-router/`](examples/nextjs-app-router/)** — three scoped
+rule sets plus 13 regex rules for a real React/Next.js project. Copy the folder
+and edit.
+
+### Which file does a rule go in?
+
+**If a regex can decide it, it goes in `rules.toml`.** It costs nothing, never
+changes between runs, and is safe to block a build on. Only judgement goes in a
+`.md`.
+
+| | `rules.toml` | `rules/*.md` |
+| --- | --- | --- |
+| Read by | regex, before the model runs | the model, in the prompt |
+| Cost | free | tokens, every review |
+| Same answer every run | always | depends on the model |
+| Good for | `next/router` in `app/`, `@ts-ignore`, hardcoded secrets | "is this abstraction earning its keep" |
+
 ### Written rules — `.diffmind/rules/*.md`
 
 For rules that need judgement instead of a pattern. They live in the repo and
@@ -405,7 +441,8 @@ people carry.
 
 ```markdown
 ---
-scope: ["src/api/**/*.ts"]
+description: Conventions for the public API layer
+scope: ["src/api/**/*.ts", "!src/api/legacy/**"]
 severity: high
 ---
 
@@ -416,11 +453,25 @@ severity: high
 - Reject changes that widen a response struct without a version bump.
 ```
 
-`scope` is a glob for the files the rule set applies to (leave it out for the
-whole repo). `severity` is a **maximum** for findings from that rule set — it can
-lower a finding's severity but never raise it. `id` defaults to the filename.
-Create a starter file with `diffmind rules init`, and see what loads with
-`diffmind rules list`.
+| Field | Meaning |
+| --- | --- |
+| `id` | Suppression handle, `rulebook.<id>`. Defaults to the filename. |
+| `description` | One line, for humans. Shown by `rules list`; never sent to the model. |
+| `scope` | Globs this governs. Omit for the whole repo. A `!` prefix excludes, and beats every include. |
+| `always` | Apply to every file regardless of `scope`. Paid for on every review — make it deliberate. |
+| `severity` | A **maximum** for findings from this set. Can lower a finding's severity, never raise it. Also the drop order when the budget is tight. |
+
+`globs:` and `alwaysApply:` are accepted as aliases, so a rule ported from
+`.cursor/rules/` loads without an edit.
+
+Create a starter file with `diffmind rules init`, see what loads with
+`diffmind rules list`, and validate the lot with `diffmind rules check` —
+which exits non-zero, so it belongs in CI:
+
+```bash
+diffmind rules check    # parse errors, duplicate ids, identical bodies,
+                        # dead globs, invalid regex in rules.toml
+```
 
 A finding from a rule set gets the ID `rulebook.<id>` and can be silenced like
 any other. If the model credits a rule set that does not apply to that file, the
@@ -432,6 +483,32 @@ which rule sets apply, so every review in a group starts with an identical
 prefix. That is what makes prompt-prefix caching possible later. A rule set that
 fails to parse is reported and skipped, never ignored quietly.
 
+#### Scope them, or they get dropped
+
+Every rule set matching a file is pasted into that file's prompt, and the prompt
+shares a byte budget with the diff and the symbol context — the diff always keeps
+at least half. **When the rule sets do not all fit, whole ones are dropped**
+(never truncated: half a rule reads as a complete rule that says something else).
+
+Two things make that safe rather than mysterious:
+
+- **Lowest `severity` goes first**, so a `high` security set outlives a style
+  guide. Unranked sets go before ranked ones.
+- **You are told.** The run prints which sets did not fit and why.
+
+Still, the real lever is `scope`, not brevity. Scoped well, only two or three
+sets ever apply at once:
+
+```
+react.md                    scope: ["**/*.tsx"]
+nextjs-app-router.md        scope: ["app/**", "src/app/**"]
+server-actions-security.md  scope: ["**/actions.ts", "**/route.ts"]
+```
+
+Two rules of thumb: keep each set to a page or two — attention dilutes, and
+twenty pages of rules makes the model worse at each one — and run
+`diffmind rules list` after editing to confirm the globs are what you meant.
+
 ### Pattern rules — `.diffmind/rules.toml`
 
 Regex, checked against added lines before the model runs: instant, always the

apps/tui-cli/src/cli.rs

@@ -297,6 +297,8 @@ pub enum RulesAction {
     Init,
     /// List the rule sets that would be loaded, and what they govern
     List,
+    /// Validate every rule set and pattern rule. Exit 1 on a problem — for CI.
+    Check,
 }
 
 #[derive(Subcommand, Debug)]

apps/tui-cli/src/daemon.rs

@@ -667,6 +667,8 @@ mod tests {
                 rules: vec![],
                 rulebooks: vec![core_engine::Rulebook {
                     id: "api".into(),
+                    description: None,
+                    always: false,
                     scope: vec!["src/api/**".into()],
                     severity: Some(core_engine::Severity::High),
                     body: "- Handlers return ApiError.".into(),

apps/tui-cli/src/main.rs

@@ -297,16 +297,22 @@ fn run_command(
                     return Ok(0);
                 }
                 for b in &books {
-                    let scope = if b.scope.is_empty() {
+                    let scope = if b.always {
+                        "always (every file)".to_string()
+                    } else if b.scope.is_empty() {
                         "whole repository".to_string()
                     } else {
                         b.scope.join(", ")
                     };
                     let severity = b.severity.map(|s| s.as_str()).unwrap_or("unset");
                     println!("  {:<24} {severity:<7} {scope}", b.id);
+                    if let Some(d) = &b.description {
+                        println!("  {:<24} {:<7} {d}", "", "");
+                    }
                 }
                 Ok(0)
             }
+            cli::RulesAction::Check => rules::check(project_root),
         },
 
         cli::Commands::Cache { action } => {
@@ -573,6 +579,39 @@ impl ProjectRules {
     }
 }
 
+/// Say so when the prompt budget cannot fit every rule set.
+///
+/// The alternative is what shipped before: a rule set stops applying and
+/// nothing anywhere says it did. The check is deliberately at the *widest*
+/// budget — depth 0, before any retry shrinks it — so this warns about the
+/// rule sets that can never apply, not about a transient squeeze on one
+/// oversized unit.
+fn warn_on_dropped_rulebooks(backend: &dyn ReviewBackend, settings: &Settings, books: &[Rulebook]) {
+    if books.is_empty() {
+        return;
+    }
+    let budget = core_engine::section_budget_bytes(
+        backend.context_tokens(),
+        settings.max_tokens as usize,
+        0,
+    );
+    let dropped = core_engine::rulebooks_dropped(books, budget);
+    if dropped.is_empty() {
+        return;
+    }
+
+    eprintln!(
+        "  !  {} rule set(s) do not fit the prompt budget ({budget} bytes) and were \
+         dropped: {}",
+        dropped.len(),
+        dropped.join(", ")
+    );
+    eprintln!(
+        "     Lowest severity is dropped first. Narrow their `scope`, shorten them, \
+         or use a backend with a larger context window."
+    );
+}
+
 pub fn build_analyzer(
     backend: Box<dyn ReviewBackend>,
     settings: &Settings,
@@ -581,6 +620,8 @@ pub fn build_analyzer(
     ticket: Option<String>,
     project: ProjectRules,
 ) -> ReviewAnalyzer {
+    warn_on_dropped_rulebooks(&*backend, settings, &project.books);
+
     let mut analyzer = ReviewAnalyzer::new(backend)
         .with_unit_grouper(unit_grouper(project_root))
         .with_languages(detect_languages(diff))

apps/tui-cli/src/rules.rs

@@ -128,6 +128,116 @@ fn warn_on_duplicate_ids(rules: &[CustomRule]) {
     }
 }
 
+/// Validate `.diffmind/rules/` and `.diffmind/rules.toml`. Returns the exit code.
+///
+/// Exists because every other check in this module warns on stderr during a
+/// review, where it scrolls past behind the findings. A rule set that stopped
+/// working deserves to fail CI, not to be mentioned in passing.
+pub fn check(project_root: &Path) -> anyhow::Result<i32> {
+    let mut problems = 0usize;
+
+    let dir = project_root.join(".diffmind").join("rules");
+    // Name files, not ids: when two rule sets collide the ids are identical, so
+    // an id-based message tells the reader nothing about which file to edit.
+    let mut parsed: Vec<(std::path::PathBuf, Rulebook)> = Vec::new();
+
+    if dir.is_dir() {
+        let mut entries: Vec<std::path::PathBuf> = walkdir::WalkDir::new(&dir)
+            .into_iter()
+            .filter_map(|e| e.ok())
+            .filter(|e| e.file_type().is_file())
+            .map(|e| e.path().to_path_buf())
+            .filter(|p| p.extension().is_some_and(|x| x == "md"))
+            .collect();
+        entries.sort();
+
+        for path in entries {
+            let path = path.as_path();
+            let stem = path
+                .file_stem()
+                .map(|s| s.to_string_lossy().to_string())
+                .unwrap_or_default();
+            let text = match std::fs::read_to_string(path) {
+                Ok(t) => t,
+                Err(e) => {
+                    println!("  ✗  {}: {e}", path.display());
+                    problems += 1;
+                    continue;
+                }
+            };
+            match core_engine::rulebook::parse(&stem, &text) {
+                Ok(book) => {
+                    // A glob nobody can satisfy is the silent-no-op this whole
+                    // command exists to surface.
+                    for glob in &book.scope {
+                        if glob.trim().is_empty() {
+                            println!("  ✗  {}: empty glob in `scope`", path.display());
+                            problems += 1;
+                        }
+                    }
+                    println!("  ✓  {:<28} {}", book.id, path.display());
+                    parsed.push((path.to_path_buf(), book));
+                }
+                Err(e) => {
+                    println!("  ✗  {}: {e}", path.display());
+                    problems += 1;
+                }
+            }
+        }
+    }
+
+    // Two rule sets saying the same thing cost tokens twice and give the model
+    // two chances to report the same finding.
+    for i in 0..parsed.len() {
+        for j in (i + 1)..parsed.len() {
+            if parsed[i].1.body == parsed[j].1.body {
+                println!(
+                    "  ✗  {} and {} have identical bodies — delete one.",
+                    parsed[i].0.display(),
+                    parsed[j].0.display()
+                );
+                problems += 1;
+            }
+        }
+    }
+
+    // Counted from what was parsed above rather than by calling
+    // `load_rulebooks`, which would re-parse every file and print its own copy
+    // of the warnings this command has already reported.
+    let mut ids: std::collections::HashMap<&str, Vec<&std::path::Path>> = Default::default();
+    for (path, book) in &parsed {
+        ids.entry(book.id.as_str()).or_default().push(path);
+    }
+    let mut collisions: Vec<_> = ids.iter().filter(|(_, v)| v.len() > 1).collect();
+    collisions.sort_by_key(|(id, _)| *id);
+    for (id, paths) in collisions {
+        let names: Vec<String> = paths.iter().map(|p| p.display().to_string()).collect();
+        println!(
+            "  ✗  {} rule sets share the id '{id}' ({}) — set an explicit `id`.",
+            paths.len(),
+            names.join(", ")
+        );
+        problems += 1;
+    }
+
+    // Pattern rules: an invalid regex is skipped at review time with a warning
+    // nobody reads. Here it is a failure.
+    for rule in load_custom_rules(project_root) {
+        if let Err(e) = regex::Regex::new(&rule.pattern) {
+            println!("  ✗  rule '{}': invalid regex — {e}", rule.effective_id());
+            problems += 1;
+        }
+    }
+
+    if problems == 0 {
+        println!("\n  {} rule set(s), no problems.", parsed.len());
+        Ok(0)
+    } else {
+        println!("\n  {problems} problem(s).");
+        Ok(1)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

examples/nextjs-app-router/.diffmind/.gitignore

@@ -0,0 +1,10 @@
+# Written by diffmind. Generated state — not worth committing.
+# Deliberately absent: rules/, rules.toml, config.toml, baseline.json.
+cache/
+runs/
+models/
+graph.db
+graph.db-wal
+graph.db-shm
+symbols.json
+daemon.json