From 6389f8a8e8a2a6ab70aee0233c50cadfb15f4640 Mon Sep 17 00:00:00 2001 From: Dennis Paler Date: Wed, 12 Aug 2026 22:59:39 +0800 Subject: [PATCH] fix: skip agent/ignore/doc files, report dropped rule sets, add rules check --- CHANGELOG.md | 34 +++- README.md | 89 ++++++++- apps/tui-cli/src/cli.rs | 2 + apps/tui-cli/src/daemon.rs | 2 + apps/tui-cli/src/main.rs | 43 ++++- apps/tui-cli/src/rules.rs | 110 +++++++++++ .../nextjs-app-router/.diffmind/.gitignore | 10 + .../nextjs-app-router/.diffmind/rules.toml | 136 +++++++++++++ .../.diffmind/rules/nextjs-app-router.md | 42 ++++ .../.diffmind/rules/react.md | 30 +++ .../rules/server-actions-security.md | 50 +++++ examples/nextjs-app-router/README.md | 94 +++++++++ packages/core-engine/src/analyzer.rs | 29 ++- packages/core-engine/src/lib.rs | 5 +- packages/core-engine/src/prompt.rs | 146 ++++++++++++-- packages/core-engine/src/rulebook.rs | 181 +++++++++++++++++- 16 files changed, 965 insertions(+), 38 deletions(-) create mode 100644 examples/nextjs-app-router/.diffmind/.gitignore create mode 100644 examples/nextjs-app-router/.diffmind/rules.toml create mode 100644 examples/nextjs-app-router/.diffmind/rules/nextjs-app-router.md create mode 100644 examples/nextjs-app-router/.diffmind/rules/react.md create mode 100644 examples/nextjs-app-router/.diffmind/rules/server-actions-security.md create mode 100644 examples/nextjs-app-router/README.md diff --git a/CHANGELOG.md b/CHANGELOG.md index ae74ba8..87f2717 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 0aefd37..9cda8ae 100644 --- a/README.md +++ b/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.`. 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.` 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 diff --git a/apps/tui-cli/src/cli.rs b/apps/tui-cli/src/cli.rs index b61c563..525326b 100644 --- a/apps/tui-cli/src/cli.rs +++ b/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)] diff --git a/apps/tui-cli/src/daemon.rs b/apps/tui-cli/src/daemon.rs index 67a7bee..44d900e 100644 --- a/apps/tui-cli/src/daemon.rs +++ b/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(), diff --git a/apps/tui-cli/src/main.rs b/apps/tui-cli/src/main.rs index 3907b4c..f5a12c0 100644 --- a/apps/tui-cli/src/main.rs +++ b/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, settings: &Settings, @@ -581,6 +620,8 @@ pub fn build_analyzer( ticket: Option, 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)) diff --git a/apps/tui-cli/src/rules.rs b/apps/tui-cli/src/rules.rs index 3aec4bc..be8ade7 100644 --- a/apps/tui-cli/src/rules.rs +++ b/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 { + 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 = 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 = 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::*; diff --git a/examples/nextjs-app-router/.diffmind/.gitignore b/examples/nextjs-app-router/.diffmind/.gitignore new file mode 100644 index 0000000..e52400a --- /dev/null +++ b/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 diff --git a/examples/nextjs-app-router/.diffmind/rules.toml b/examples/nextjs-app-router/.diffmind/rules.toml new file mode 100644 index 0000000..04c3e77 --- /dev/null +++ b/examples/nextjs-app-router/.diffmind/rules.toml @@ -0,0 +1,136 @@ +# Deterministic rules for a Next.js App Router project. +# +# These are matched by regex against added (`+`) lines. No model is involved: +# they cost nothing, never vary between runs, and are safe to block a build on. +# Judgement calls belong in `.diffmind/rules/*.md` instead. +# +# Suppress one inline with: +# // diffmind-ignore-next-line nextjs.raw-img + +# ─── Security ──────────────────────────────────────────────────────────────── + +[[rule]] +id = "nextjs.public-env-secret" +pattern = 'NEXT_PUBLIC_[A-Z0-9_]*(SECRET|PASSWORD|PRIVATE|CREDENTIAL)' +message = "A secret is exposed to the browser: NEXT_PUBLIC_* values are inlined into the client bundle." +fix = "Drop the NEXT_PUBLIC_ prefix and read it only on the server, or confirm this value is genuinely public." +severity = "high" +category = "security" + +# Separated from the rule above because TOKEN and KEY have common false +# positives (NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY, NEXT_PUBLIC_MAPBOX_TOKEN are +# both meant to be public), so this one warns rather than blocks. +[[rule]] +id = "nextjs.public-env-key" +pattern = 'NEXT_PUBLIC_[A-Z0-9_]*(_KEY|_TOKEN)\b' +message = "NEXT_PUBLIC_* is inlined into the client bundle — confirm this key is meant to be public." +fix = "If it is a publishable/anon key, keep it. If it authorises writes, move it server-side." +severity = "medium" +category = "security" + +[[rule]] +id = "nextjs.dangerously-set-inner-html" +pattern = 'dangerouslySetInnerHTML' +message = "dangerouslySetInnerHTML renders unescaped HTML — an XSS sink if any part of the value is user-controlled." +fix = "Render as text, or sanitise with a vetted library before injecting." +severity = "high" +category = "security" +files = ["*.tsx", "*.jsx"] + +# Rust's regex crate has no lookahead, so this cannot check for the *absence* of +# rel= on the same tag. It flags every _blank and asks the reviewer to confirm. +[[rule]] +id = "nextjs.target-blank" +pattern = '''target=\{?["']_blank''' +message = 'target="_blank" gives the opened page access to window.opener unless rel is set.' +fix = 'Add rel="noopener noreferrer".' +severity = "low" +category = "security" +files = ["*.tsx", "*.jsx"] + +# ─── App Router correctness ────────────────────────────────────────────────── + +[[rule]] +id = "nextjs.pages-router-data-fn" +pattern = '\b(getServerSideProps|getStaticProps|getStaticPaths)\b' +message = "This is a Pages Router API and does nothing in the App Router — the data is never fetched." +fix = "Fetch directly in the server component, or use generateStaticParams for dynamic segments." +severity = "high" +category = "quality" +files = ["app/**", "src/app/**"] + +[[rule]] +id = "nextjs.legacy-router-import" +pattern = '''from\s+["']next/router["']''' +message = "next/router is Pages Router and throws in the App Router." +fix = "Import useRouter, usePathname and useSearchParams from next/navigation." +severity = "high" +category = "quality" +files = ["app/**", "src/app/**"] + +[[rule]] +id = "nextjs.head-in-app-router" +pattern = '''from\s+["']next/head["']''' +message = "next/head has no effect in the App Router." +fix = "Export a `metadata` object or a `generateMetadata` function instead." +severity = "medium" +category = "quality" +files = ["app/**", "src/app/**"] + +# ─── Performance ───────────────────────────────────────────────────────────── + +[[rule]] +id = "nextjs.raw-img" +pattern = '` over a hand-rolled `isLoading` flag. +- `error.tsx` must be a client component and should offer `reset()`. +- Time, randomness or `window`-derived values on first paint will + hydrate-mismatch — move them into an effect or behind a mounted check. +- Treat `params` and `searchParams` as untrusted user input. +- A new public page without `metadata`/`generateMetadata` ships with no title. +- Prefer `next/image` and `next/link` over bare `` and `` for internal + routes; prefer `next/font` over a stylesheet font URL. diff --git a/examples/nextjs-app-router/.diffmind/rules/react.md b/examples/nextjs-app-router/.diffmind/rules/react.md new file mode 100644 index 0000000..a22a6b0 --- /dev/null +++ b/examples/nextjs-app-router/.diffmind/rules/react.md @@ -0,0 +1,30 @@ +--- +description: React correctness, hooks discipline and accessibility +id: react +scope: ["**/*.tsx", "**/*.jsx"] +severity: medium +--- + +# React review standards + +Judgement calls only — anything a regex can decide is in `.diffmind/rules.toml`. + +- **Derived state must be computed, not stored.** A `useState` whose only writer + is a `useEffect` mirroring a prop will desynchronise. Compute it during render. +- **An effect that subscribes, opens or times something needs a cleanup.** + Missing teardown is a leak. +- **Dependency arrays must be honest.** A value read inside the effect and left + out of the deps is a stale-closure bug. If the array was trimmed to stop a + loop, the loop is the real problem — usually an object or function recreated + each render. +- **Work that belongs to a user action belongs in the handler**, not in an + effect that fires on mount. +- **A list key must identify the item, not its position.** Index keys corrupt + state when the list reorders or filters. +- **Memoisation needs a reason.** Flag both directions: a missing memo on + something expensive in a hot list, and a pointless one around a string concat. +- **A context value built inline** (`value={{ user, setUser }}`) re-renders every + consumer on every parent render. +- **Accessibility**: interactive elements reachable by keyboard (a `
` + needs role, `tabIndex` and a key handler — or just use `