diff --git a/.diffmind/.gitignore b/.diffmind/.gitignore new file mode 100644 index 0000000..e52400a --- /dev/null +++ b/.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/CHANGELOG.md b/CHANGELOG.md index 7e74fb3..f65d762 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,23 @@ else's branch. Both surfaces share one engine; neither replaces the other. ### Added +- **Code graph.** A tree-sitter symbol graph for 13 languages — Rust, + TypeScript, TSX, JavaScript, Python, Go, Java, C#, Ruby, PHP, C, C++ and + Scala — stored in `.diffmind/graph.db` and updated incrementally + by mtime. Replaces the regex symbol index, which could only see `pub`/`export` + declarations and had no concept of a reference at all. + Review context now includes **the callers of every changed symbol** — a + changed signature is judged against the code that depends on it — plus the + enclosing definition, referenced definitions, and the file's tests. Everything + stays inside a byte budget, so context does not grow with the repository. + Bodies are read from the working tree rather than stored, so a snippet can + never disagree with the file being reviewed. Adding a language is one entry in + a table; contributions welcome. + +- **Cross-file review units.** When a symbol and code that calls it both change + in one diff, they are reviewed together as a single unit instead of separately. + Reviewed apart, the model judges an interaction while seeing only one side of + it as background. One call replaces two. - **Reviewer's cockpit** (`diffmind --tui`). Analyses on launch. Each finding shows the actual hunk the model reviewed and the context it was given. `a` accepts (and copies a review comment via OSC 52, which works over SSH), diff --git a/README.md b/README.md index dcb9a3d..17c4b29 100644 --- a/README.md +++ b/README.md @@ -1,191 +1,159 @@ -# Diffmind — Local AI Code Review for the Terminal +# Diffmind — a code review gate you can actually keep [![CI](https://github.com/thinkgrid-labs/diffmind/actions/workflows/ci.yml/badge.svg)](https://github.com/thinkgrid-labs/diffmind/actions/workflows/ci.yml) [![Latest Release](https://img.shields.io/github/v/release/thinkgrid-labs/diffmind)](https://github.com/thinkgrid-labs/diffmind/releases/latest) [![License: MIT](https://img.shields.io/badge/license-MIT-blue.svg)](LICENSE) [![Rust](https://img.shields.io/badge/built%20with-Rust-orange.svg)](https://www.rust-lang.org) -**Diffmind** is a free, open-source AI code review tool that runs entirely on your machine — no cloud, no API keys, no subscription. It analyzes your `git diff` using a local [Qwen2.5-Coder](https://huggingface.co/Qwen/Qwen2.5-Coder-1.5B-Instruct-GGUF) model and reports security issues, bugs, and code quality problems directly in your terminal. +**Diffmind reviews a `git diff` and reports security issues, bugs and quality +problems** — from a single binary, on your machine, with no API key and no +network. It runs as a CI gate, a git hook, or an interactive terminal cockpit +for whoever has to review the branch. -Your source code never leaves your environment. Works offline. Ships as a **single self-contained binary** for Linux, macOS, and Windows. +The hard part of an automated reviewer is not producing findings. It is +producing them *the same way twice*, keeping false positives from accumulating +until someone deletes the job, and doing it cheaply enough to run on every push. +That is what this is built around. --- -## Why Diffmind? +## When diffmind is the right tool -> The only AI code reviewer that keeps your code 100% private. +Reach for it when you need review that is: -| | **Diffmind** | Cloud AI review (Copilot, CodeRabbit, etc.) | -| -------------- | ---------------------------------- | ------------------------------------------- | -| **Privacy** | Code stays on your machine | Code sent to third-party servers | -| **Cost** | Free — one-time model download | Per-token billing or subscription | -| **Offline** | Works with no internet after setup | Requires connectivity | -| **CI/CD** | Single binary, no runtime deps | Needs API key management and secrets | -| **Compliance** | No data residency concerns | Data may cross jurisdictions | +- **Reproducible** — greedy decoding at a fixed seed with a constrained JSON + decoder. The same diff reviews the same way every time. A gate that flags a + line on Tuesday and not on Wednesday gets deleted within a month. +- **Free per run** — no per-token bill, so it can run on every push, on every + fork's PR, on a repo with no budget. +- **Offline and secretless** — nothing leaves the machine. No API key in CI, no + bot account, no third-party data processor to get approved. +- **Stateful** — a baseline, inline suppressions, stable rule IDs and a + recorded accept/wrong ratio. Review debt that a team carries for two years + needs somewhere to live. ---- +### When to use something else + +Diffmind runs local models by default. **It is not smarter than a frontier +model, and does not try to be.** If you want the deepest possible read of one +tricky diff and you have an interactive agent and a budget, use those — they +will find things diffmind will not. -## Features - -- **Deterministic pre-filter** — lockfiles, generated files, minified bundles, assets and formatting-only hunks are dropped before the model sees them, and the run tells you exactly what it skipped: `312 hunks → 74 reviewable (238 filtered: lockfiles, generated, formatting)`. -- **Deterministic detectors** — commented-out code, removed-but-still-used declarations, and your own regex rules. These run before the model, cost nothing, and are the findings you can trust unconditionally. -- **Security, bug, performance and maintainability review** by a local model -- **Review standards as markdown** — commit your team's own rules to `.diffmind/rules/`, scoped by path. Prose the model reads, versioned next to the code it governs. -- **Ticket-aware review** — check the diff actually implements the acceptance criteria (`--ticket`) -- **Suppressions** — inline `// diffmind-ignore` comments and a project baseline, so one false positive doesn't get the whole gate deleted -- **SARIF output** — inline PR annotations via GitHub Code Scanning, no bot account or token -- **Pluggable backends** — the bundled GGUF, or your own Ollama / vLLM / LM Studio endpoint -- **Daemon mode** — keep the model resident so reviews are near-instant -- **Local RAG** — feeds the model the *enclosing function* of each hunk, not just the diff -- **Reproducible** — greedy decoding with a fixed seed: the same diff always reviews the same way -- **Reviewer's cockpit** (`--tui`) — analyses on launch, shows the hunk and context behind each finding, and records accept / dismiss / wrong so the signal-to-noise ratio is measured rather than guessed -- JSON / Markdown output, and a proper CI gate +The intended shape is both: an agent when you are thinking hard about a single +change, and diffmind on the other several hundred, unattended, for free. --- -## Installation +## Two tiers of trust -### Linux & macOS — one command +Findings are not all worth the same, and the output says which is which. -```bash -curl -fsSL https://github.com/thinkgrid-labs/diffmind/releases/latest/download/install.sh | bash -``` +**Deterministic findings** — no model involved. Commented-out code (`DM001`), +a declaration removed while still referenced (`DM002`), your own regex rules +(`custom.`). These are pattern-true: they cost nothing, never vary, and +you can gate on them without thinking about it. -Auto-detects your OS and CPU architecture, verifies the SHA-256 checksum, and installs to `/usr/local/bin`. +**Model findings** (`DM900.`) — a model's judgement on a hunk, given +the enclosing function, the callers of what changed, referenced definitions and +the file's tests. Depth scales with the model you run: the default 1.5B is a +lint-grade reviewer that catches obvious mistakes; a 14B behind Ollama reads +much more like a colleague. Both are worth reading. Neither is worth trusting +blindly, which is why the cockpit records when they are wrong. -Pin a specific version — note that the variable goes on the **`bash`** side of the pipe, not the `curl` side: +The split is deliberate. **Set `--fail-on` for the tier you trust** — many teams +gate on high-severity deterministic findings and let model findings report +without blocking. -```bash -curl -fsSL https://github.com/thinkgrid-labs/diffmind/releases/latest/download/install.sh | VERSION=v0.9.0 bash -``` - -### Windows - -Download `diffmind-x86_64-pc-windows-msvc.zip` from [Releases](https://github.com/thinkgrid-labs/diffmind/releases), extract it, and put `diffmind.exe` on your `PATH`. +--- -### npm +## Install ```bash -npx @diffmind/cli --help -npm install -g @diffmind/cli -``` - -`@diffmind/cli` is a launcher — the binary ships in a per-platform package declared as an optional dependency, so npm downloads only the one matching your machine. Linux binaries are glibc-linked; on musl (Alpine) use a glibc base image or build from source. +# Linux & macOS — auto-detects arch, verifies the SHA-256, installs to /usr/local/bin +curl -fsSL https://github.com/thinkgrid-labs/diffmind/releases/latest/download/install.sh | bash -### Build from source +# npm — the binary ships in a per-platform optional dependency +npm install -g @diffmind/cli -```bash -git clone https://github.com/thinkgrid-labs/diffmind -cd diffmind +# from source cargo install --path apps/tui-cli ``` ---- - -## Quick Start - -```bash -diffmind download # one-time model download (~1.1 GB) -diffmind index # optional: index symbols for context-aware reviews -diffmind # review this branch against the repo's default branch -``` - -`--branch` is no longer assumed to be `main`: diffmind reads the repository's default branch from `origin/HEAD` and falls back to whichever of `main`/`master`/`develop`/`trunk` actually exists. +To pin a version, the variable goes on the **`bash`** side of the pipe: +`curl -fsSL … | VERSION=v0.9.0 bash`. -A bare `a..b` argument is recognised as a revision range without needing `--range`. The detection is strict — both endpoints must resolve as revisions and the string must not name an existing path — so `diffmind ../lib` and a file genuinely called `a..b` are still treated as paths. Paths after a range narrow it: `diffmind v1.2.0..HEAD src/api/`. - -```bash -diffmind --last # just the last commit -diffmind --staged # just what's staged -diffmind v1.2.0..HEAD # an explicit revision range -diffmind src/auth/ # just these paths -diffmind --tui # the reviewer's cockpit -``` +Windows: download `diffmind-x86_64-pc-windows-msvc.zip` from +[Releases](https://github.com/thinkgrid-labs/diffmind/releases) and put +`diffmind.exe` on your `PATH`. Linux binaries are glibc-linked; on musl +(Alpine) use a glibc base image or build from source. --- -## AI Model Setup - -Models download to `~/.diffmind/models/`. All are **Qwen2.5-Coder**, Q4_K_M quantised. Inference runs on the Apple Silicon GPU via Metal where available, and on CPU (with Accelerate/BLAS) everywhere else. +## Quick start ```bash -diffmind download # interactive picker with a hardware check -diffmind download --model 3b -diffmind download --model 1.5b --force # re-download -diffmind download --model 1.5b --verify # check an existing download's checksum +diffmind download # one-time model download (~1.1 GB) +diffmind index # build the code graph — recommended, big context win +diffmind # review this branch against the repo's default branch ``` -| Model | Size | Min RAM | Notes | -| -------------------- | ------- | ------- | ----------------------------------------- | -| Qwen2.5-Coder-0.5B | 0.4 GB | 2 GB | Fastest — lint-style, CI, low-end hardware | -| Qwen2.5-Coder-1.5B ★ | 1.1 GB | 4 GB | Recommended — balanced | -| Qwen2.5-Coder-3B | 2.1 GB | 6 GB | Deeper reasoning | -| Qwen2.5-Coder-7B | 4.7 GB | 10 GB | Strong security analysis | -| Qwen2.5-Coder-14B | 9.0 GB | 18 GB | Expert | -| Qwen2.5-Coder-32B | 20.0 GB | 40 GB | Maximum | - -Downloads are atomic and checksummed: an interrupted download can no longer leave a truncated file that looks valid. - ---- - -## Using your own model server - -The local GGUF is the default and the point of the tool, but "local" doesn't have to mean "in this process". If you already run Ollama, vLLM, or LM Studio, point diffmind at it — your code still never leaves your machine or your network, and you get a model class diffmind will never ship as a 20 GB download. - ```bash -# Ollama (default URL http://localhost:11434) -diffmind --backend ollama --backend-model qwen2.5-coder:14b - -# Anything speaking the OpenAI chat API — vLLM, LM Studio, llama.cpp server, LiteLLM -diffmind --backend openai-compatible \ - --backend-url http://localhost:8000/v1 \ - --backend-model my-model +diffmind --last # just the last commit +diffmind --staged # just what's staged +diffmind v1.2.0..HEAD # an explicit revision range +diffmind src/auth/ # just these paths +diffmind v1.2.0..HEAD src/api/ # a range, narrowed to paths +diffmind --tui # the reviewer's cockpit ``` -API keys are read from the environment only (`DIFFMIND_API_KEY` by default, override with `--backend-api-key-env`) — never from the config file, which gets committed. +The base branch is read from `origin/HEAD`, falling back to whichever of +`main` / `master` / `develop` / `trunk` exists — it is not assumed to be `main`. +A bare `a..b` argument is recognised as a revision range without `--range`; the +detection is strict, so `diffmind ../lib` and a file genuinely named `a..b` are +still treated as paths. --- -## Suppressions - -Findings carry a stable rule ID, shown in the output, so you can silence exactly one thing. - -| Rule ID | Meaning | -| ---------------- | -------------------------------------------------- | -| `DM001` | A code block was commented out instead of deleted | -| `DM002` | A declaration was removed but is still referenced | -| `DM900.` | A model-authored finding of that category | -| `custom.` | One of your `.diffmind/rules.toml` rules | -| `rulebook.` | A violation of one of your `.diffmind/rules/*.md` | - -### Inline - -```js -// diffmind-ignore-next-line DM001 -// const legacy = oldPath(); - -const x = 1; // diffmind-ignore - -/* diffmind-ignore-file DM900.maintainability */ -``` - -Listing no rule IDs suppresses everything at that location. `DM900` suppresses every model category without listing each one. - -### Baseline - -Adopting diffmind on an existing codebase shouldn't mean fixing everything first. - -```bash -diffmind baseline create # record today's findings as accepted -diffmind baseline show -diffmind baseline clear -``` - -Commit `.diffmind/baseline.json`. Future runs report only new issues. The baseline keys on a content fingerprint rather than a line number, so it survives unrelated edits above the finding. +## Language support + +Two things here are language-specific, and only one of them limits you. + +**Review itself works on any text diff** — any language, plus config files, SQL +migrations, shell scripts. The model reads the hunk; the deterministic detectors +and your regex rules run on added lines regardless of extension. + +**The code graph is the part that needs a grammar.** It supplies the enclosing +definition, the callers of a changed symbol, and referenced definitions — +tree-sitter parsers, 13 languages: + +| Language | Extensions | +| ---------- | -------------------------------- | +| Rust | `.rs` | +| TypeScript | `.ts` `.mts` `.cts` | +| TSX | `.tsx` | +| JavaScript | `.js` `.jsx` `.mjs` `.cjs` | +| Python | `.py` `.pyi` | +| Go | `.go` | +| Java | `.java` | +| C# | `.cs` | +| Ruby | `.rb` `.rake` | +| PHP | `.php` | +| C | `.c` `.h` | +| C++ | `.cpp` `.cc` `.cxx` `.hpp` `.hh` | +| Scala | `.scala` `.sc` | + +Each gets definitions, references and callers. Files outside the table are still +reviewed — with their hunk, their test file when it is conventionally named, and +your rule sets — just without caller context, so a changed signature is judged on +its own rather than against the code that depends on it. + +Adding a language is one entry in the `LANGS` table in +[`apps/tui-cli/src/graph/extract.rs`](apps/tui-cli/src/graph/extract.rs) — +contributions welcome. --- -## CI/CD +## The gate ### GitHub Action @@ -196,33 +164,26 @@ Commit `.diffmind/baseline.json`. Future runs report only new issues. The baseli fail-on: high ``` -That caches the model, installs the binary, reviews the PR diff, and uploads SARIF so findings appear inline on the diff. For a PR comment instead: - -```yaml -- uses: thinkgrid-labs/diffmind@v0.9.0 - with: - format: markdown - comment: true - fail-on: none -``` +Caches the model, installs the binary, reviews the PR diff, and uploads SARIF so +findings appear inline on the diff — no bot account, no token, no secret. For a +PR comment instead, set `format: markdown`, `comment: true`, `fail-on: none`. -`permissions: { security-events: write }` is required for the SARIF upload, `pull-requests: write` for comments. +Requires `permissions: { security-events: write }` for SARIF, or +`pull-requests: write` for comments. -### Manual +### Any other CI ```bash git diff origin/main...HEAD | diffmind --stdin --format sarif --output diffmind.sarif --fail-on high ``` -### Exit codes - -| Code | Meaning | +| Exit | Meaning | | ---- | ------------------------------------------------ | | `0` | Clean, or nothing at or above `--fail-on` | | `1` | Findings at or above `--fail-on` | | `2` | diffmind itself failed (bad flag, missing model) | -`1` and `2` used to be indistinguishable, so a crashed binary looked exactly like a failed review. +`1` and `2` are distinct, so a crashed binary never looks like a failed review. ### Git hooks @@ -231,63 +192,84 @@ diffmind install-hooks --hook pre-push --min-severity high diffmind install-hooks --hook pre-commit ``` -The generated hook exits 0 when diffmind isn't installed, so it never blocks a teammate who hasn't set it up. It refuses to overwrite a hook it didn't write unless you pass `--force`. - -Or via [pre-commit](https://pre-commit.com): - -```yaml -repos: - - repo: https://github.com/thinkgrid-labs/diffmind - rev: v0.9.0 - hooks: - - id: diffmind -``` +The generated hook exits 0 when diffmind isn't installed, so it never blocks a +teammate who hasn't set it up, and refuses to overwrite a hook it didn't write +unless you pass `--force`. Also available through +[pre-commit](https://pre-commit.com) — `repo: https://github.com/thinkgrid-labs/diffmind`, `id: diffmind`. --- ## The cockpit — `diffmind --tui` -```bash -diffmind --tui -``` +The gate is public and must be conservative. The cockpit is private, so it can +afford to show you more. Analysis starts on launch. Each finding shows the **actual hunk the model reviewed** and the **context it was given** — a finding you cannot check is one you will eventually stop reading. | Key | Action | -| --------- | ------------------------------------------------------------ | -| `j` / `k` | Move through findings | -| `a` | Accept — records the verdict and copies a review comment | -| `d` | Dismiss — read, not worth raising | -| `w` | Wrong — the finding was incorrect | +| --------------- | -------------------------------------------------------- | +| `j` / `k` | Move through findings | +| `a` | Accept — records the verdict and copies a review comment | +| `d` | Dismiss — read, not worth raising | +| `w` | Wrong — the finding was incorrect | | `PgUp` / `PgDn` | Scroll the detail pane | -| `r` | Re-run | -| `q` | Quit | +| `r` | Re-run | +| `q` | Quit | Verdicts are written through immediately, so closing the terminal mid-triage -loses nothing. They feed `diffmind stats`. +loses nothing. Accept copies via **OSC 52**, the terminal's own clipboard +escape — no dependency, and it works over SSH. tmux and screen need clipboard +passthrough; if the copy fails you are told, so you never believe you have +copied something you have not. -Accept copies via **OSC 52**, the terminal's own clipboard escape — no -dependency, and it works over SSH. tmux and screen need clipboard passthrough -enabled; if the copy fails you are told, so you never believe you have copied -something you have not. +--- -Because the output is private to you, the tool can afford to be wrong -occasionally: a bad finding costs one keystroke, not an author's afternoon. +## Keeping the gate alive ---- +Noise is this category's failure mode. One false positive that cannot be +silenced is how a review job gets deleted. Every finding therefore carries a +stable rule ID, shown in the output, so you can silence exactly one thing. -## Run history +### Inline + +```js +// diffmind-ignore-next-line DM001 +// const legacy = oldPath(); -Every review is filed to `.diffmind/runs//` — the findings as `run.json` -and `review.md`, plus what the run cost. `diffmind stats` reads them back: +const x = 1; // diffmind-ignore + +/* diffmind-ignore-file DM900.maintainability */ +``` + +Listing no rule IDs suppresses everything at that location. `DM900` suppresses +every model category without listing each one. + +### Baseline + +Adopting a reviewer on an existing codebase shouldn't mean fixing everything +first. + +```bash +diffmind baseline create # record today's findings as accepted +diffmind baseline show +diffmind baseline clear +``` + +Commit `.diffmind/baseline.json`; future runs report only new issues. The +baseline keys on a content fingerprint rather than a line number, so it survives +unrelated edits above the finding. + +### Measuring whether it earns its keep + +Every review is filed to `.diffmind/runs//`. `diffmind stats` reads them +back: ``` Runs 34 Median findings 3 Median time 6.2s - Median tokens 18420 Cache hits 61% Verdicts 71 accepted · 44 dismissed · 12 wrong @@ -298,74 +280,29 @@ and `review.md`, plus what the run cost. `diffmind stats` reads them back: 3 rulebook.house-style ``` -The **accept-to-wrong ratio** is the number that decides whether the tool is -earning its keep — noise is this category's known failure mode, and a reviewer -who cannot measure it will just quietly stop running the reviewer. Verdicts come -from the TUI. Dismissals are excluded from the ratio: choosing not to raise a -correct observation is not the tool being wrong. +The **accept-to-wrong ratio** is the number that decides whether to keep running +it — a reviewer who cannot measure noise will just quietly stop running the +reviewer. Verdicts come from the cockpit. Dismissals are excluded: choosing not +to raise a correct observation is not the tool being wrong. Run snapshots are overwritten when a sha is reviewed again; verdicts are append-only and survive `diffmind stats --clear`, because the ratio is only meaningful over months. -diffmind writes `.diffmind/.gitignore` covering `runs/`, `cache/`, `models/`, -`symbols.json` and `daemon.json` — your review notes stay private, while -`rules/`, `rules.toml`, `config.toml` and `baseline.json` remain committable. -Your repository's own `.gitignore` is never touched. - ---- - -## Daemon mode - -Every invocation otherwise pays the model-load cost — seconds, every time. - -```bash -diffmind serve # loads the model, unloads after 10 idle minutes -diffmind serve --status -diffmind serve --stop -``` - -Reviews automatically use a running daemon whose model and device match; `--no-daemon` opts out. It listens on `127.0.0.1` only and requires a per-instance token stored in a `0600` file, so no other user on the machine can submit work to it. +Diffmind writes `.diffmind/.gitignore` covering `runs/`, `cache/`, `models/`, +`graph.db` and `daemon.json` — your review notes stay private, while `rules/`, +`rules.toml`, `config.toml` and `baseline.json` remain committable. Your +repository's own `.gitignore` is never touched. --- -## Configuration +## Your team's standards -`.diffmind/config.toml` — precedence is CLI flag > config file > default. +### Prose rules — `.diffmind/rules/*.md` -```toml -[review] -branch = "develop" -model = "3b" -min_severity = "low" # what gets reported -fail_on = "high" # what fails the build -min_confidence = 0.0 # detectors score 0.9+; unscored model findings 0.5 -triage = "auto" # two-pass triage on large diffs -cache = true -temperature = 0.0 # 0 = greedy and reproducible -max_tokens = 1024 -# Extra paths to drop, on top of the built-in noise rules. -ignore = ["**/legacy/**", "*.generated.ts"] - -[backend] -kind = "local" # or "ollama" / "openai-compatible" -url = "http://localhost:11434" -model = "qwen2.5-coder:14b" -api_key_env = "DIFFMIND_API_KEY" -``` - -Unknown keys are reported rather than silently ignored. - -### Review standards — `.diffmind/rules/*.md` - -Rules that need judgement rather than a pattern. Written as prose, committed to -the repo, and read by the model on every review — so a team's review culture -becomes a reviewed artifact instead of tacit knowledge. - -```bash -diffmind rules init # scaffold .diffmind/rules/default.md -diffmind rules list # what would load, and what each governs -``` +Rules that need judgement rather than a pattern. Committed to the repo and read +by the model on every review, so review culture becomes a versioned artifact +instead of tacit knowledge. ```markdown --- @@ -380,180 +317,215 @@ severity: high - Reject changes that widen a response struct without a version bump. ``` -| Key | Description | -| ---------- | ----------------------------------------------------------------- | -| `scope` | Globs this rule set governs. Omit to cover the whole repository. | -| `severity` | **Ceiling** for findings attributed to it — never a promotion. | -| `id` | Name used to attribute and suppress. Defaults to the file stem. | +`scope` globs the files a rule set governs (omit for the whole repo); `severity` +is a **ceiling** for findings attributed to it, never a promotion; `id` defaults +to the file stem. Scaffold with `diffmind rules init`, check what loads with +`diffmind rules list`. -A finding the model attributes to a rule set gets the rule ID -`rulebook.`, so it suppresses like any other: -`// diffmind-ignore-next-line rulebook.api-conventions`. An attribution naming a -rule set that does not govern that file is discarded — a 1.5B will invent a -plausible name, and an invented one could never be suppressed. +A finding attributed to a rule set gets the ID `rulebook.` and suppresses +like any other. An attribution naming a rule set that does not govern that file +is discarded — a small model will invent a plausible name, and an invented one +could never be suppressed. Rule bodies go in the *stable* half of the prompt and units are grouped by which rule sets govern them, so every unit in a group sends a byte-identical prefix. -That is what keeps prompt-prefix caching possible; scoping rules per file would -otherwise make every prompt unique. - -A rule set that fails to parse is reported and skipped, never silently ignored. +That is what keeps prompt-prefix caching possible. A rule set that fails to +parse is reported and skipped, never silently ignored. ### Pattern rules — `.diffmind/rules.toml` -Regex rules run before the model: instant, deterministic, zero inference cost. +Regex, matched against added lines before the model runs: instant, +deterministic, zero inference cost. ```toml -[[rule]] -id = "no-console" -pattern = "console\\.log" -message = "Remove debug logging before merging" -fix = "Use the structured logger" -severity = "medium" -category = "quality" -files = ["*.ts", "*.tsx"] - [[rule]] id = "no-hardcoded-password" pattern = "password\\s*=\\s*[\"'][^\"']+[\"']" message = "Hardcoded password — use a secrets manager" -severity = "high" -category = "security" +fix = "Use a secrets manager" +severity = "high" # high | medium | low (default medium) +category = "security" # security | quality | performance | maintainability +files = ["*.ts", "**/generated/**"] ``` -| Field | Required | Description | -| ---------- | -------- | ----------------------------------------------------------------------- | -| `pattern` | ✓ | Regex matched against added lines | -| `message` | ✓ | Finding description | -| `id` | | Stable ID for suppression and SARIF. Defaults to a slug of the message. | -| `fix` | | Remediation hint | -| `severity` | | `high`, `medium`, `low` (default `medium`) | -| `category` | | `security`, `quality`, `performance`, `maintainability` | -| `files` | | Globs: `*.ts`, `**/generated/**`, or an exact path | +Only `pattern` and `message` are required; `id` defaults to a slug of the +message. --- -## Other commands +## Models and backends + +**The local model is the default and stays the default.** Everything else here +is opt-in. + +Models download to `~/.diffmind/models/` — all Qwen2.5-Coder, Q4_K_M quantised. +Inference runs on the Apple Silicon GPU via Metal where available, and on CPU +(with Accelerate/BLAS) everywhere else. Downloads are atomic and checksummed, so +an interrupted download cannot leave a truncated file that looks valid. + +| Model | Size | Min RAM | Use for | +| -------------------- | ------- | ------- | ----------------------------------- | +| Qwen2.5-Coder-0.5B | 0.4 GB | 2 GB | CI on small runners, lint-grade | +| Qwen2.5-Coder-1.5B ★ | 1.1 GB | 4 GB | Default — balanced | +| Qwen2.5-Coder-7B | 4.7 GB | 10 GB | Noticeably better security analysis | +| Qwen2.5-Coder-32B | 20.0 GB | 40 GB | Maximum, if you have the machine | + +`3b` and `14b` also exist. `diffmind download` gives an interactive picker with +a hardware check; `--model 1.5b --verify` checks an existing download. + +### Your own model server + +"Local" doesn't have to mean "in this process". If you already run Ollama, vLLM +or LM Studio, point diffmind at it — your code still never leaves your machine +or your network, and you get a model class diffmind will never ship as a 20 GB +download. ```bash -diffmind describe # PR title, summary, and test plan -diffmind commit # conventional commit message for staged changes -diffmind commit --apply -diffmind rules init # scaffold .diffmind/rules/default.md -diffmind rules list # show which rule sets load -diffmind index # build the symbol index -diffmind stats # cost and signal over recorded runs -diffmind stats --clear # drop run snapshots (verdicts are kept) -diffmind cache show # cache location and size -diffmind cache clear +diffmind --backend ollama --backend-model qwen2.5-coder:14b + +# anything speaking the OpenAI chat API — vLLM, LM Studio, llama.cpp, LiteLLM +diffmind --backend openai-compatible --backend-url http://localhost:8000/v1 --backend-model my-model ``` ---- +API keys are read from the environment only (`DIFFMIND_API_KEY`, override with +`--backend-api-key-env`) — never from the config file, which gets committed. -## All Options +> **What you trade for a bigger model.** The constrained JSON decoder hooks the +> bundled sampler directly, so remote backends cannot use it: you get better +> judgement and lose the same-diff-same-result guarantee. Token counts reported +> by remote endpoints are estimates, marked `~`. Reproducibility is a property +> of the local path. -``` -Usage: diffmind [OPTIONS] [FILES]... [COMMAND] - -Commands: - download Download or refresh the local AI model files - index Build a symbol index for context-aware reviews - describe Generate a PR title and description - commit Suggest a conventional commit message - baseline Record current findings as accepted - rules Manage the prose rule sets in .diffmind/rules/ - stats Findings, cost and accept/wrong ratio over recorded runs - install-hooks Install git hooks - serve Keep the model resident between runs - cache Inspect or clear the review cache - -Options: - -b, --branch Base branch [default: the repo's default branch] - -m, --model 0.5b, 1.5b, 3b, 7b, 14b, 32b [default: 1.5b] - -l, --last Review the last commit only - --staged Review staged changes only - --stdin Read the diff from stdin - --range Review an explicit revision range, e.g. v1.2.0..HEAD - -t, --tui Launch the interactive TUI - --ticket Acceptance criteria to check against - --min-severity Minimum severity to report [default: low] - --min-confidence Minimum confidence to report, 0.0-1.0 - --fail-on Severity causing exit 1 [default: --min-severity] - -f, --format text, json, sarif, markdown [default: text] - -o, --output Write the report to a file - --max-tokens Output tokens per chunk [default: 1024] - --triage auto, on, off [default: auto] - --temperature 0 is greedy and reproducible [default: 0] - --seed Sampling seed - --no-cache Skip the result cache - --no-baseline Ignore .diffmind/baseline.json - --no-daemon Don't use a running daemon - --device auto, cpu, metal [default: auto] - --backend local, ollama, openai-compatible - --backend-url Remote backend base URL - --backend-model Model name on the remote backend - --debug Print raw model output to stderr +Hosted backends are planned on the same terms — additive, opt-in, never the +default, and subject to the same trade-off above. + +### Daemon mode + +Every invocation otherwise pays the model-load cost — seconds, every time. + +```bash +diffmind serve # loads the model, unloads after 10 idle minutes +diffmind serve --status +diffmind serve --stop ``` ---- +Reviews automatically use a running daemon whose model and device match; +`--no-daemon` opts out. It listens on `127.0.0.1` only and requires a +per-instance token in a `0600` file, so no other user on the machine can submit +work to it. -## How It Works +--- -1. **Parse** — the diff is parsed once into typed per-file hunks with real pre/post-image line numbers. -2. **Pre-filter** — lockfiles, `linguist-generated` paths, files carrying a `@generated` banner, minified bundles, assets, snapshots, your `ignore` globs, and hunks that only change whitespace are dropped. Costs nothing, typically removes most of a real branch, and the counts are reported rather than silently applied. Whitespace inside a string literal counts as content, and indentation is never dismissed in Python or YAML. -3. **Deterministic detectors** — commented-out code, removed-but-used declarations, and your regex rules. No model involved. -4. **Context** — the enclosing function of each hunk (plus definitions of referenced symbols) is pulled from `.diffmind/symbols.json`, assembled per chunk so one file's edit does not invalidate another's cached result. -5. **Triage** — on large diffs, a cheap first pass decides which files carry real risk. -6. **Review units** — hunks are grouped into regions of a file rather than cut wherever a line budget ran out, so related hunks are read together and an edit in one function only re-reviews that function. Units are sized to the backend's *actual* context window, read from the GGUF metadata. -7. **Constrained decoding** — the sampler consults a JSON state machine before committing each token, so the model cannot emit a preamble, an unbalanced brace, or a truncated string. Output that hits the token cap is repaired rather than discarded. -8. **Anchoring** — findings pointing at a file not in the diff are dropped; off-by-N line numbers snap to the nearest changed line. -9. **Suppression** — inline directives, the baseline, and `--min-confidence` are applied, then results are deduplicated and sorted. +## How it works + +1. **Parse** — the diff becomes typed per-file hunks with real pre/post-image + line numbers. +2. **Pre-filter** — lockfiles, `linguist-generated` paths, `@generated` banners, + minified bundles, assets, snapshots, your `ignore` globs and whitespace-only + hunks are dropped. Costs nothing, typically removes most of a real branch, + and the counts are reported rather than silently applied: + `312 hunks → 74 reviewable (238 filtered: lockfiles, generated, formatting)`. + Whitespace inside a string literal counts as content; indentation is never + dismissed in Python or YAML. +3. **Deterministic detectors** — `DM001`, `DM002` and your regex rules. No model + involved. +4. **Context** — assembled per unit from `.diffmind/graph.db`: the enclosing + definition, the callers of every changed symbol, definitions of referenced + symbols, and the corresponding test file. Bounded by a byte budget, so + context does not grow with the repository. Bodies are read from the working + tree, so a snippet can never disagree with the file being reviewed. +5. **Triage** — on large diffs, a cheap first pass decides which files carry real + risk. +6. **Review units** — hunks are grouped into regions of a file rather than cut + wherever a line budget ran out, so related hunks are read together and an + edit in one function only re-reviews that function. When a changed symbol and + a changed caller both appear, the two are merged into one unit and reviewed + together. Units are sized to the backend's *actual* context window, read from + the GGUF metadata. +7. **Constrained decoding** — the sampler consults a JSON state machine before + committing each token, so the model cannot emit a preamble, an unbalanced + brace or a truncated string. Output that hits the token cap is repaired + rather than discarded. +8. **Anchoring** — findings pointing at a file not in the diff are dropped; + off-by-N line numbers snap to the nearest changed line. +9. **Suppression** — inline directives, the baseline and `--min-confidence` are + applied, then results are deduplicated and sorted. + +The **code graph** behind step 4 is the tree-sitter symbol index from +[Language support](#language-support), kept in `.diffmind/graph.db` and updated +incrementally by mtime. Build it with `diffmind index`. --- -## Project Structure +## Configuration -``` -diffmind/ -├── action.yml # GitHub Action -├── .pre-commit-hooks.yaml # pre-commit framework integration -├── install.sh # one-line installer (checksum-verified) -├── packages/core-engine/src/ -│ ├── analyzer.rs # orchestration: chunking, triage, cache, anchoring -│ ├── backend/ # candle (local GGUF) and remote (Ollama / OpenAI) -│ ├── detectors.rs # deterministic rules -│ ├── diff.rs # unified-diff parser, chunking, finding anchoring -│ ├── json_guard.rs # constrained-decoding state machine -│ ├── prefilter.rs # deterministic noise removal, with counts -│ ├── prompt.rs # prompt construction -│ ├── rulebook.rs # prose rule sets (.diffmind/rules/*.md) -│ ├── unit.rs # hunks → review units (the cached, reviewed thing) -│ ├── sarif.rs # SARIF 2.1.0 output -│ └── suppression.rs # inline directives and baselines -└── apps/tui-cli/src/ - ├── main.rs # entry point and review pipeline - ├── daemon.rs # diffmind serve - ├── settings.rs # CLI + config resolution - ├── output.rs # text / JSON / SARIF / Markdown rendering - ├── indexer.rs # symbol indexer - └── tui.rs # interactive browser +`.diffmind/config.toml` — precedence is CLI flag > config file > default. +Unknown keys are reported rather than silently ignored. + +```toml +[review] +branch = "develop" +model = "3b" +min_severity = "low" # what gets reported +fail_on = "high" # what fails the build +min_confidence = 0.0 # detectors score 0.9+; unscored model findings 0.5 +triage = "auto" # two-pass triage on large diffs +cache = true +temperature = 0.0 # 0 = greedy and reproducible +max_tokens = 1024 +ignore = ["**/legacy/**", "*.generated.ts"] # on top of the built-in noise rules + +[backend] +kind = "local" # or "ollama" / "openai-compatible" +url = "http://localhost:11434" +model = "qwen2.5-coder:14b" +api_key_env = "DIFFMIND_API_KEY" ``` +### Options that matter + +| Flag | Effect | +| ----------------------- | -------------------------------------------------- | +| `-b, --branch` | Base branch [default: the repo's default branch] | +| `-m, --model` | `0.5b` … `32b` [default: `1.5b`] | +| `-l, --last` / `--staged` / `--range` / `--stdin` | What to review | +| `-t, --tui` | Launch the cockpit | +| `--min-severity` | Minimum severity to report [default: `low`] | +| `--fail-on` | Severity causing exit 1 [default: `--min-severity`] | +| `--min-confidence` | Minimum confidence to report, 0.0–1.0 | +| `-f, --format` | `text`, `json`, `sarif`, `markdown` | +| `-o, --output` | Write the report to a file | +| `--ticket ` | Check the diff against acceptance criteria | +| `--backend` / `--backend-url` / `--backend-model` | Use your own model server | +| `--no-cache` / `--no-baseline` / `--no-daemon` | Opt out | + +`--triage`, `--temperature`, `--seed`, `--max-tokens`, `--device` and `--debug` +are in `diffmind --help`. + +The binary also carries two conveniences that are not the point of the tool: +`diffmind describe` (a PR title and summary) and `diffmind commit` (a +conventional commit message for staged changes). Use them if they're handy; they +are not what diffmind is for. + --- ## Roadmap -- [ ] Auto-fix patches (`diffmind fix`) — gated on remote backends landing, since a 1.5B's patches are not trustworthy enough to apply -- [ ] Cross-file impact analysis — find callers of deleted or renamed functions -- [ ] VS Code / JetBrains extensions, talking to the daemon -- [ ] Homebrew tap and scoop manifest -- [ ] Fine-tuned review model +- Hosted backends — opt-in only, with the local model still the default +- Auto-fix patches (`diffmind fix`) — gated on those backends, since a 1.5B's + patches are not trustworthy enough to apply +- Cheap-model triage feeding a strong-model deep pass, and prompt-prefix caching +- Learning from verdicts: marking a finding wrong should suppress its kind next + time, not merely count it +- VS Code / JetBrains extensions, talking to the daemon +- Homebrew tap and scoop manifest --- ## Contributing -Issues and pull requests welcome at [github.com/thinkgrid-labs/diffmind](https://github.com/thinkgrid-labs/diffmind). +Issues and pull requests welcome at +[github.com/thinkgrid-labs/diffmind](https://github.com/thinkgrid-labs/diffmind). ```bash cargo test --workspace --all-targets @@ -561,14 +533,6 @@ cargo clippy --workspace --all-targets -- -D warnings cargo fmt --all ``` ---- - -## License - MIT — see [LICENSE](LICENSE). ---- - -> **Diffmind** — AI-powered local code review. Private by design. Free forever. - -Built with ❤️ by Tech Lead, for Tech Leads. +> **Diffmind** — review that runs where your code already is. diff --git a/apps/tui-cli/Cargo.toml b/apps/tui-cli/Cargo.toml index 23369d5..c8c2c7f 100644 --- a/apps/tui-cli/Cargo.toml +++ b/apps/tui-cli/Cargo.toml @@ -26,3 +26,18 @@ sysinfo = { workspace = true } toml = { workspace = true } # Model download checksums and the daemon's auth token. sha2 = { workspace = true } +tree-sitter = "0.26" +tree-sitter-rust = "0.24" +tree-sitter-python = "0.25" +tree-sitter-javascript = "0.25" +tree-sitter-typescript = "0.23" +rusqlite = { version = "0.40", features = ["bundled"] } +streaming-iterator = "0.1.9" +tree-sitter-go = "0.25" +tree-sitter-java = "0.23" +tree-sitter-ruby = "0.23" +tree-sitter-php = "0.24" +tree-sitter-c-sharp = "0.23" +tree-sitter-scala = "0.26" +tree-sitter-c = "0.24" +tree-sitter-cpp = "0.23" diff --git a/apps/tui-cli/src/graph/extract.rs b/apps/tui-cli/src/graph/extract.rs new file mode 100644 index 0000000..ff8b79d --- /dev/null +++ b/apps/tui-cli/src/graph/extract.rs @@ -0,0 +1,923 @@ +//! Tree-sitter symbol extraction: source text in, definitions and references out. +//! +//! This replaces a pile of regexes that could only see `pub`/`export` symbols +//! and had no concept of a reference at all. The regex index could answer "where +//! is `validateToken` defined"; it could never answer "what calls it", which is +//! the question a reviewer actually has about a changed function. +//! +//! # Adding a language +//! +//! One entry in [`LANGUAGES`], and nothing else. Add the grammar crate, then: +//! +//! ```ignore +//! LangSpec { +//! name: "kotlin", +//! extensions: &["kt", "kts"], +//! grammar: || tree_sitter_kotlin::LANGUAGE.into(), +//! defs: &[def("function", "(function_declaration ...) @def")], +//! refs: &["(call_expression ...) @ref"], +//! } +//! ``` +//! +//! A definition pattern captures the whole declaration as `@def` and its name as +//! `@name`; the range comes from `@def` so "which symbol encloses line N" has a +//! real body to test against. Reference patterns capture one `@ref` each and +//! should stay narrow — call targets, constructors, type positions. Capturing +//! every bare identifier makes `callers_of` return most of the repository, and a +//! blast radius that includes everything is the same as none. +//! +//! # Tolerant at runtime, strict in tests +//! +//! Each pattern is compiled on its own and one that fails is skipped rather than +//! taking the whole language down with it. Grammar crates rename nodes between +//! versions, and losing one kind of definition is far better than losing every +//! symbol in the repository the day a dependency is bumped. +//! `every_pattern_compiles` then fails loudly in CI, so the degradation is never +//! silent — and a contributor's new pattern is checked against the real grammar. + +// Consumed by the graph store. +#![allow(dead_code)] + +use std::path::Path; +use streaming_iterator::StreamingIterator; +use tree_sitter::{Node, Parser, Query, QueryCursor}; + +/// A definition pattern: `@def` is the declaration, `@name` is its name. +pub struct DefPattern { + kind: &'static str, + query: &'static str, +} + +const fn def(kind: &'static str, query: &'static str) -> DefPattern { + DefPattern { kind, query } +} + +/// Everything needed to extract symbols from one language. +pub struct LangSpec { + pub name: &'static str, + pub extensions: &'static [&'static str], + grammar: fn() -> tree_sitter::Language, + defs: &'static [DefPattern], + refs: &'static [&'static str], +} + +/// A language this build can parse. Cheap to copy — it is a pointer to a +/// [`LangSpec`] in [`LANGUAGES`]. +#[derive(Clone, Copy)] +pub struct Lang(&'static LangSpec); + +impl std::fmt::Debug for Lang { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0.name) + } +} + +impl Lang { + pub fn for_path(path: &Path) -> Option { + Self::for_extension(path.extension()?.to_str()?) + } + + pub fn for_extension(ext: &str) -> Option { + LANGUAGES + .iter() + .find(|spec| spec.extensions.contains(&ext)) + .map(Lang) + } + + pub fn name(self) -> &'static str { + self.0.name + } + + fn grammar(self) -> tree_sitter::Language { + (self.0.grammar)() + } + + /// Every language this build can parse. + pub fn all() -> impl Iterator { + LANGUAGES.iter().map(Lang) + } + + /// Names of every supported language, for `--help` and the README. + pub fn names() -> Vec<&'static str> { + LANGUAGES.iter().map(|s| s.name).collect() + } +} + +impl PartialEq for Lang { + fn eq(&self, other: &Self) -> bool { + std::ptr::eq(self.0, other.0) + } +} +impl Eq for Lang {} + +/// A symbol declared in a file. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Definition { + pub name: String, + pub kind: &'static str, + /// 1-based, inclusive. + pub start_line: u32, + pub end_line: u32, +} + +/// A mention of a name — a call, a constructor, a type position. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Reference { + pub name: String, + pub line: u32, +} + +#[derive(Debug, Default)] +pub struct Extracted { + pub definitions: Vec, + pub references: Vec, +} + +// ─── The table ─────────────────────────────────────────────────────────────── + +pub static LANGUAGES: &[LangSpec] = &[ + LangSpec { + name: "rust", + extensions: &["rs"], + grammar: || tree_sitter_rust::LANGUAGE.into(), + defs: &[ + def("function", "(function_item name: (identifier) @name) @def"), + def("struct", "(struct_item name: (type_identifier) @name) @def"), + def("enum", "(enum_item name: (type_identifier) @name) @def"), + def("trait", "(trait_item name: (type_identifier) @name) @def"), + def("type", "(type_item name: (type_identifier) @name) @def"), + def("const", "(const_item name: (identifier) @name) @def"), + def("static", "(static_item name: (identifier) @name) @def"), + def("macro", "(macro_definition name: (identifier) @name) @def"), + def("module", "(mod_item name: (identifier) @name) @def"), + ], + refs: &[ + "(call_expression function: (identifier) @ref)", + "(call_expression function: (field_expression field: (field_identifier) @ref))", + "(call_expression function: (scoped_identifier name: (identifier) @ref))", + "(macro_invocation macro: (identifier) @ref)", + "(type_identifier) @ref", + ], + }, + LangSpec { + name: "typescript", + extensions: &["ts", "mts", "cts"], + grammar: || tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into(), + defs: TS_DEFS, + refs: TS_REFS, + }, + LangSpec { + name: "tsx", + extensions: &["tsx"], + grammar: || tree_sitter_typescript::LANGUAGE_TSX.into(), + defs: TS_DEFS, + refs: TS_REFS, + }, + LangSpec { + name: "javascript", + extensions: &["js", "jsx", "mjs", "cjs"], + grammar: || tree_sitter_javascript::LANGUAGE.into(), + defs: &[ + def( + "function", + "(function_declaration name: (identifier) @name) @def", + ), + def("class", "(class_declaration name: (identifier) @name) @def"), + def( + "method", + "(method_definition name: (property_identifier) @name) @def", + ), + def( + "function", + "(variable_declarator name: (identifier) @name value: (arrow_function)) @def", + ), + def( + "function", + "(variable_declarator name: (identifier) @name value: (function_expression)) @def", + ), + ], + // No `type_identifier`: that node does not exist in the JavaScript + // grammar, and including it would fail to compile. + refs: &[ + "(call_expression function: (identifier) @ref)", + "(call_expression function: (member_expression property: (property_identifier) @ref))", + "(new_expression constructor: (identifier) @ref)", + ], + }, + LangSpec { + name: "python", + extensions: &["py", "pyi"], + grammar: || tree_sitter_python::LANGUAGE.into(), + defs: &[ + def( + "function", + "(function_definition name: (identifier) @name) @def", + ), + def("class", "(class_definition name: (identifier) @name) @def"), + ], + refs: &[ + "(call function: (identifier) @ref)", + "(call function: (attribute attribute: (identifier) @ref))", + ], + }, + LangSpec { + name: "go", + extensions: &["go"], + grammar: || tree_sitter_go::LANGUAGE.into(), + defs: &[ + def( + "function", + "(function_declaration name: (identifier) @name) @def", + ), + def( + "method", + "(method_declaration name: (field_identifier) @name) @def", + ), + def("type", "(type_spec name: (type_identifier) @name) @def"), + ], + refs: &[ + "(call_expression function: (identifier) @ref)", + "(call_expression function: (selector_expression field: (field_identifier) @ref))", + "(type_identifier) @ref", + ], + }, + LangSpec { + name: "java", + extensions: &["java"], + grammar: || tree_sitter_java::LANGUAGE.into(), + defs: &[ + def("class", "(class_declaration name: (identifier) @name) @def"), + def( + "interface", + "(interface_declaration name: (identifier) @name) @def", + ), + def("enum", "(enum_declaration name: (identifier) @name) @def"), + def( + "record", + "(record_declaration name: (identifier) @name) @def", + ), + def( + "method", + "(method_declaration name: (identifier) @name) @def", + ), + def( + "constructor", + "(constructor_declaration name: (identifier) @name) @def", + ), + ], + refs: &[ + "(method_invocation name: (identifier) @ref)", + "(object_creation_expression type: (type_identifier) @ref)", + "(type_identifier) @ref", + ], + }, + LangSpec { + name: "c#", + extensions: &["cs"], + grammar: || tree_sitter_c_sharp::LANGUAGE.into(), + defs: &[ + def("class", "(class_declaration name: (identifier) @name) @def"), + def( + "interface", + "(interface_declaration name: (identifier) @name) @def", + ), + def( + "struct", + "(struct_declaration name: (identifier) @name) @def", + ), + def("enum", "(enum_declaration name: (identifier) @name) @def"), + def( + "record", + "(record_declaration name: (identifier) @name) @def", + ), + def( + "method", + "(method_declaration name: (identifier) @name) @def", + ), + def( + "constructor", + "(constructor_declaration name: (identifier) @name) @def", + ), + def( + "property", + "(property_declaration name: (identifier) @name) @def", + ), + ], + refs: &[ + "(invocation_expression function: (identifier) @ref)", + "(invocation_expression function: (member_access_expression name: (identifier) @ref))", + ], + }, + LangSpec { + name: "ruby", + extensions: &["rb", "rake"], + grammar: || tree_sitter_ruby::LANGUAGE.into(), + defs: &[ + def("method", "(method name: (identifier) @name) @def"), + def("method", "(singleton_method name: (identifier) @name) @def"), + def("class", "(class name: (constant) @name) @def"), + def("module", "(module name: (constant) @name) @def"), + ], + // A paren-less Ruby call (`check`) is grammatically identical to a local + // variable reference, so only explicit calls — `check(...)` or + // `obj.check` — become edges. Capturing bare identifiers instead would + // make every local variable look like a call. + refs: &["(call method: (identifier) @ref)"], + }, + LangSpec { + name: "php", + extensions: &["php"], + grammar: || tree_sitter_php::LANGUAGE_PHP.into(), + defs: &[ + def("function", "(function_definition name: (name) @name) @def"), + def("method", "(method_declaration name: (name) @name) @def"), + def("class", "(class_declaration name: (name) @name) @def"), + def( + "interface", + "(interface_declaration name: (name) @name) @def", + ), + def("trait", "(trait_declaration name: (name) @name) @def"), + ], + refs: &[ + "(function_call_expression function: (name) @ref)", + "(member_call_expression name: (name) @ref)", + ], + }, + LangSpec { + name: "c", + extensions: &["c", "h"], + grammar: || tree_sitter_c::LANGUAGE.into(), + defs: &[ + def( + "function", + "(function_definition declarator: (function_declarator declarator: (identifier) @name)) @def", + ), + def( + "struct", + "(struct_specifier name: (type_identifier) @name) @def", + ), + def( + "enum", + "(enum_specifier name: (type_identifier) @name) @def", + ), + def( + "type", + "(type_definition declarator: (type_identifier) @name) @def", + ), + ], + refs: &[ + "(call_expression function: (identifier) @ref)", + "(type_identifier) @ref", + ], + }, + LangSpec { + name: "c++", + extensions: &["cpp", "cc", "cxx", "hpp", "hh"], + grammar: || tree_sitter_cpp::LANGUAGE.into(), + defs: &[ + def( + "function", + "(function_definition declarator: (function_declarator declarator: (identifier) @name)) @def", + ), + def( + "class", + "(class_specifier name: (type_identifier) @name) @def", + ), + def( + "struct", + "(struct_specifier name: (type_identifier) @name) @def", + ), + def( + "enum", + "(enum_specifier name: (type_identifier) @name) @def", + ), + ], + refs: &[ + "(call_expression function: (identifier) @ref)", + "(call_expression function: (field_expression field: (field_identifier) @ref))", + "(type_identifier) @ref", + ], + }, + LangSpec { + name: "scala", + extensions: &["scala", "sc"], + grammar: || tree_sitter_scala::LANGUAGE.into(), + defs: &[ + def( + "function", + "(function_definition name: (identifier) @name) @def", + ), + def("class", "(class_definition name: (identifier) @name) @def"), + def( + "object", + "(object_definition name: (identifier) @name) @def", + ), + def("trait", "(trait_definition name: (identifier) @name) @def"), + ], + refs: &["(call_expression function: (identifier) @ref)"], + }, +]; + +/// TypeScript and TSX differ only in grammar, never in what a symbol looks like. +static TS_DEFS: &[DefPattern] = &[ + def( + "function", + "(function_declaration name: (identifier) @name) @def", + ), + def( + "class", + "(class_declaration name: (type_identifier) @name) @def", + ), + def( + "interface", + "(interface_declaration name: (type_identifier) @name) @def", + ), + def( + "type", + "(type_alias_declaration name: (type_identifier) @name) @def", + ), + def("enum", "(enum_declaration name: (identifier) @name) @def"), + def( + "method", + "(method_definition name: (property_identifier) @name) @def", + ), + // `const handler = () => {}` is how most TypeScript functions are actually + // written; missing it would miss most of a codebase. + def( + "function", + "(variable_declarator name: (identifier) @name value: (arrow_function)) @def", + ), + def( + "function", + "(variable_declarator name: (identifier) @name value: (function_expression)) @def", + ), +]; + +static TS_REFS: &[&str] = &[ + "(call_expression function: (identifier) @ref)", + "(call_expression function: (member_expression property: (property_identifier) @ref))", + "(new_expression constructor: (identifier) @ref)", + "(type_identifier) @ref", +]; + +fn def_patterns(lang: Lang) -> &'static [DefPattern] { + lang.0.defs +} + +fn ref_patterns(lang: Lang) -> &'static [&'static str] { + lang.0.refs +} + +/// Parse one file. Returns `None` when the language is unsupported or the +/// parser cannot produce a tree at all; a file with syntax errors still yields +/// whatever tree-sitter could recover, which is the point of using it. +pub fn extract(lang: Lang, source: &str) -> Option { + let grammar = lang.grammar(); + let mut parser = Parser::new(); + parser.set_language(&grammar).ok()?; + let tree = parser.parse(source, None)?; + let root = tree.root_node(); + let bytes = source.as_bytes(); + + let mut out = Extracted::default(); + + for pattern in def_patterns(lang) { + let Ok(query) = Query::new(&grammar, pattern.query) else { + continue; + }; + let (Some(name_ix), Some(def_ix)) = ( + query.capture_index_for_name("name"), + query.capture_index_for_name("def"), + ) else { + continue; + }; + + let mut cursor = QueryCursor::new(); + let mut matches = cursor.matches(&query, root, bytes); + while let Some(m) = matches.next() { + let name = capture(m, name_ix).and_then(|n| text(n, bytes)); + let body = capture(m, def_ix); + if let (Some(name), Some(body)) = (name, body) { + out.definitions.push(Definition { + name, + kind: pattern.kind, + start_line: line_of(body.start_position().row), + end_line: line_of(body.end_position().row), + }); + } + } + } + + for pattern in ref_patterns(lang) { + let Ok(query) = Query::new(&grammar, pattern) else { + continue; + }; + let Some(ref_ix) = query.capture_index_for_name("ref") else { + continue; + }; + + let mut cursor = QueryCursor::new(); + let mut matches = cursor.matches(&query, root, bytes); + while let Some(m) = matches.next() { + if let Some(node) = capture(m, ref_ix) + && let Some(name) = text(node, bytes) + { + out.references.push(Reference { + name, + line: line_of(node.start_position().row), + }); + } + } + } + + // A name matched by two overlapping patterns is one symbol, not two. + out.definitions + .sort_by(|a, b| (a.start_line, &a.name, a.kind).cmp(&(b.start_line, &b.name, b.kind))); + out.definitions + .dedup_by(|a, b| a.name == b.name && a.start_line == b.start_line); + + out.references + .sort_by(|a, b| (a.line, &a.name).cmp(&(b.line, &b.name))); + out.references.dedup(); + + Some(out) +} + +fn capture<'a>(m: &tree_sitter::QueryMatch<'a, 'a>, index: u32) -> Option> { + m.captures.iter().find(|c| c.index == index).map(|c| c.node) +} + +fn text(node: Node<'_>, bytes: &[u8]) -> Option { + node.utf8_text(bytes).ok().map(str::to_string) +} + +/// tree-sitter rows are 0-based; every line number diffmind handles is 1-based. +fn line_of(row: usize) -> u32 { + row as u32 + 1 +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Look a language up the way production does — by extension, from the + /// table. Keeps the tests honest about the one source of truth. + fn lang(ext: &str) -> Lang { + Lang::for_extension(ext).unwrap_or_else(|| panic!("no language for .{ext}")) + } + + fn names(defs: &[Definition]) -> Vec<&str> { + defs.iter().map(|d| d.name.as_str()).collect() + } + + fn refs(e: &Extracted) -> Vec<&str> { + e.references.iter().map(|r| r.name.as_str()).collect() + } + + /// A grammar bump that renames a node would otherwise silently stop + /// extracting one kind of symbol. Runtime skips the broken pattern; this + /// makes CI say so. + /// A grammar bump that renames a node would otherwise silently stop + /// extracting one kind of symbol. Runtime skips the broken pattern; this + /// makes CI say so — and checks a contributor's new language for real. + #[test] + fn every_pattern_compiles() { + for lang in Lang::all() { + let grammar = lang.grammar(); + for p in def_patterns(lang) { + assert!( + Query::new(&grammar, p.query).is_ok(), + "{}: definition pattern failed to compile: {}", + lang.name(), + p.query + ); + } + for p in ref_patterns(lang) { + assert!( + Query::new(&grammar, p).is_ok(), + "{}: reference pattern failed to compile: {p}", + lang.name() + ); + } + } + } + + /// Every language in the table must actually declare an extension, or it is + /// unreachable — `for_path` is the only way in. + #[test] + fn every_language_is_reachable_by_extension() { + for spec in LANGUAGES { + assert!( + !spec.extensions.is_empty(), + "{} has no extensions and can never be selected", + spec.name + ); + for ext in spec.extensions { + assert_eq!( + Lang::for_extension(ext).map(|l| l.name()), + Some(spec.name), + ".{ext} should map to {}", + spec.name + ); + } + } + } + + #[test] + fn no_two_languages_claim_the_same_extension() { + let mut seen = std::collections::HashMap::new(); + for spec in LANGUAGES { + for ext in spec.extensions { + if let Some(other) = seen.insert(*ext, spec.name) { + panic!("both {other} and {} claim .{ext}", spec.name); + } + } + } + } + + #[test] + fn rust_definitions_and_calls() { + let src = "\ +pub fn validate_token(t: &str) -> bool { true } + +struct Session { id: u32 } + +pub fn login() { + let ok = validate_token(\"x\"); + Session { id: 1 }; +} +"; + let e = extract(lang("rs"), src).unwrap(); + assert!(names(&e.definitions).contains(&"validate_token")); + assert!(names(&e.definitions).contains(&"login")); + assert!(names(&e.definitions).contains(&"Session")); + assert!( + refs(&e).contains(&"validate_token"), + "the call site is the whole point: {:?}", + refs(&e) + ); + } + + #[test] + fn private_rust_functions_are_found() { + // The regex indexer only ever saw `pub fn`, so a private helper — the + // most likely thing to change — was invisible. + let e = extract(lang("rs"), "fn helper() {}\n").unwrap(); + assert_eq!(names(&e.definitions), ["helper"]); + } + + #[test] + fn a_definition_spans_its_whole_body() { + let src = "fn outer() {\n let a = 1;\n let b = 2;\n}\n"; + let d = &extract(lang("rs"), src).unwrap().definitions[0]; + assert_eq!((d.start_line, d.end_line), (1, 4)); + } + + #[test] + fn typescript_arrow_consts_count_as_functions() { + // Most TypeScript functions are written this way; missing them would + // miss most of a real codebase. + let src = "\ +export const handler = async (req: Request) => { return check(req); }; +export function check(r: Request): boolean { return true; } +export interface Options { a: string } +type Alias = string; +class Service { run() { this.helper(); } helper() {} } +"; + let e = extract(lang("ts"), src).unwrap(); + let n = names(&e.definitions); + for expected in [ + "handler", "check", "Options", "Alias", "Service", "run", "helper", + ] { + assert!(n.contains(&expected), "missing {expected} in {n:?}"); + } + assert!(refs(&e).contains(&"check")); + assert!(refs(&e).contains(&"helper")); + } + + #[test] + fn tsx_parses_as_tsx_not_typescript() { + // The TS grammar rejects JSX; using the wrong one silently yields a + // tree full of errors and almost no symbols. + let src = "export const View = () =>
hi
;\n"; + let e = extract(lang("tsx"), src).unwrap(); + assert!(names(&e.definitions).contains(&"View")); + } + + #[test] + fn python_definitions_and_calls() { + let src = "\ +def validate(token): + return True + +class Session: + def start(self): + validate(self.token) +"; + let e = extract(lang("py"), src).unwrap(); + let n = names(&e.definitions); + assert!(n.contains(&"validate")); + assert!(n.contains(&"Session")); + assert!(n.contains(&"start")); + assert!(refs(&e).contains(&"validate")); + } + + #[test] + fn javascript_methods_and_news() { + let src = "class A { go() { new Session(); helper(); } }\n"; + let e = extract(lang("js"), src).unwrap(); + assert!(names(&e.definitions).contains(&"A")); + assert!(names(&e.definitions).contains(&"go")); + assert!(refs(&e).contains(&"Session")); + assert!(refs(&e).contains(&"helper")); + } + + #[test] + fn a_file_with_a_syntax_error_still_yields_what_parsed() { + // Reviewing a branch mid-refactor is normal; refusing to index a file + // that does not compile would blank the graph exactly when it is needed. + let src = "fn good() {}\nfn broken( {\n"; + let e = extract(lang("rs"), src).unwrap(); + assert!(names(&e.definitions).contains(&"good")); + } + + /// A pattern can compile against a grammar and still match nothing. Every + /// language must actually produce a definition *and* a call edge, or the + /// blast radius silently does not work for it. + #[test] + fn every_language_extracts_a_definition_and_a_call() { + struct Case { + ext: &'static str, + source: &'static str, + wants_def: &'static str, + wants_ref: &'static str, + } + let cases = [ + Case { + ext: "rs", + wants_def: "login", + wants_ref: "check", + source: "fn check() -> bool { true }\npub fn login() { check(); }\n", + }, + Case { + ext: "ts", + wants_def: "login", + wants_ref: "check", + source: "function check(): boolean { return true; }\nexport function login() { check(); }\n", + }, + Case { + ext: "tsx", + wants_def: "View", + wants_ref: "check", + source: "export const View = () => { check(); return
; };\n", + }, + Case { + ext: "js", + wants_def: "login", + wants_ref: "check", + source: "function check() { return true; }\nfunction login() { check(); }\n", + }, + Case { + ext: "py", + wants_def: "login", + wants_ref: "check", + source: "def check():\n return True\n\ndef login():\n check()\n", + }, + Case { + ext: "go", + wants_def: "Login", + wants_ref: "Check", + source: "package main\nfunc Check() bool { return true }\nfunc Login() { Check() }\n", + }, + Case { + ext: "java", + wants_def: "login", + wants_ref: "check", + source: "class A { boolean check() { return true; } void login() { check(); } }\n", + }, + Case { + ext: "cs", + wants_def: "Login", + wants_ref: "Check", + source: "class A { bool Check() { return true; } void Login() { Check(); } }\n", + }, + Case { + ext: "rb", + wants_def: "login", + wants_ref: "check", + // An explicit receiver. A paren-less `check` is grammatically a + // local variable reference in Ruby — see the note on its refs. + source: "def login\n Validator.check(token)\nend\n", + }, + Case { + ext: "php", + wants_def: "login", + wants_ref: "check", + source: " 20, + "expected many symbols, got {}", + e.definitions.len() + ); + assert!( + refs(&e).contains(&"line_of"), + "internal calls must be visible, or callers_of can never work" + ); + } + + #[test] + fn an_empty_file_is_not_an_error() { + let e = extract(lang("rs"), "").unwrap(); + assert!(e.definitions.is_empty()); + assert!(e.references.is_empty()); + } + + #[test] + fn duplicate_matches_collapse_to_one_definition() { + // `const f = () => {}` can match more than one pattern. + let e = extract(lang("ts"), "const f = () => {};\n").unwrap(); + assert_eq!( + e.definitions.iter().filter(|d| d.name == "f").count(), + 1, + "one symbol, not one per matching pattern" + ); + } +} diff --git a/apps/tui-cli/src/graph/link.rs b/apps/tui-cli/src/graph/link.rs new file mode 100644 index 0000000..cf6c766 --- /dev/null +++ b/apps/tui-cli/src/graph/link.rs @@ -0,0 +1,296 @@ +//! Linking review units the graph says belong together. +//! +//! The engine groups hunks by file and adjacency, which is everything a diff on +//! its own can tell it. It cannot know that the function changed in `auth.rs` +//! and the block changed in `api.rs` are two halves of one edit. +//! +//! Reviewed apart, the model is asked to judge an interaction while seeing only +//! one side of it as background — and pays for the other side's context twice. +//! Reviewed together it is one call, one context, and the actual question. That +//! is the rare change that is both more accurate *and* cheaper. + +use core_engine::ReviewUnit; +use std::collections::HashSet; + +use super::Graph; + +/// Ceiling on a merged unit, in lines. Two related units are worth reading +/// together; six are a second diff, and the model's attention degrades long +/// before the context window does. +const MAX_MERGED_LINES: usize = 600; +/// Ceiling on how many units may fold into one. +const MAX_MERGED_UNITS: usize = 3; + +/// Merge units connected by a call edge, when both ends changed in this diff. +pub fn link_related(units: Vec, graph: &Graph) -> Vec { + if units.len() < 2 { + return units; + } + + // What each unit declares, and what its added lines mention. + let profiles: Vec = units.iter().map(|u| profile(u, graph)).collect(); + + let mut merged: Vec = Vec::new(); + let mut consumed = vec![false; units.len()]; + + for i in 0..units.len() { + if consumed[i] { + continue; + } + consumed[i] = true; + let mut unit = units[i].clone(); + let mut combined = profiles[i].clone(); + let mut count = 1; + + for j in (i + 1)..units.len() { + if consumed[j] || count >= MAX_MERGED_UNITS { + continue; + } + // Same file is already the engine's job; only cross-file links are + // new information. + if units[j].files.iter().any(|f| unit.files.contains(f)) { + continue; + } + if !related(&combined, &profiles[j]) { + continue; + } + if unit.line_count() + units[j].line_count() > MAX_MERGED_LINES { + continue; + } + + unit = unit.merged_with(&units[j]); + combined.absorb(&profiles[j]); + consumed[j] = true; + count += 1; + } + + merged.push(unit); + } + + merged +} + +#[derive(Clone, Default)] +struct Profile { + /// Symbols declared inside this unit's changed region. + declares: HashSet, + /// Names its added lines mention. + mentions: HashSet, +} + +impl Profile { + fn absorb(&mut self, other: &Profile) { + self.declares.extend(other.declares.iter().cloned()); + self.mentions.extend(other.mentions.iter().cloned()); + } +} + +/// Two units are related when one declares something the other calls. The check +/// runs both ways: whether the caller or the callee is listed first in the diff +/// is an accident of path ordering. +fn related(a: &Profile, b: &Profile) -> bool { + a.declares.iter().any(|d| b.mentions.contains(d)) + || b.declares.iter().any(|d| a.mentions.contains(d)) +} + +fn profile(unit: &ReviewUnit, graph: &Graph) -> Profile { + let mut declares = HashSet::new(); + let mut mentions = HashSet::new(); + + // Anything the graph knows is declared over the unit's changed span. + for line in unit.new_start..=unit.new_end { + if let Some(def) = graph.enclosing(unit.file(), line) { + declares.insert(def.name); + } + } + + for line in unit.text.lines() { + let Some(added) = line.strip_prefix('+') else { + continue; + }; + if added.starts_with("++") { + continue; + } + for word in added.split(|c: char| !(c.is_alphanumeric() || c == '_' || c == '$')) { + if word.len() >= 3 { + mentions.insert(word.to_string()); + } + } + } + + Profile { declares, mentions } +} + +#[cfg(test)] +mod tests { + use super::*; + use core_engine::build_units; + use std::path::{Path, PathBuf}; + + fn project(name: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("diffmind-link-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(d.join("src")).unwrap(); + d + } + + fn write(root: &Path, rel: &str, body: &str) { + let p = root.join(rel); + if let Some(parent) = p.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(p, body).unwrap(); + } + + fn indexed(root: &Path) -> Graph { + let mut g = Graph::open(root).unwrap(); + g.index(root, &|_| {}).unwrap(); + g + } + + /// The change this exists for: a signature and the call site updated + /// together, in different files. + const BOTH_SIDES: &str = "\ +diff --git a/src/auth.rs b/src/auth.rs ++++ b/src/auth.rs +@@ -1,3 +1,3 @@ +-pub fn validate_token(t: &str) -> bool { ++pub fn validate_token(t: &str, strict: bool) -> bool { + !t.is_empty() + } +diff --git a/src/api.rs b/src/api.rs ++++ b/src/api.rs +@@ -1,3 +1,3 @@ + pub fn login(t: &str) -> bool { +- validate_token(t) ++ validate_token(t, true) + } +"; + + fn setup(name: &str) -> (PathBuf, Graph) { + let root = project(name); + write( + &root, + "src/auth.rs", + "pub fn validate_token(t: &str, strict: bool) -> bool {\n !t.is_empty()\n}\n", + ); + write( + &root, + "src/api.rs", + "pub fn login(t: &str) -> bool {\n validate_token(t, true)\n}\n", + ); + let g = indexed(&root); + (root, g) + } + + #[test] + fn a_changed_symbol_and_its_changed_caller_become_one_unit() { + let (root, graph) = setup("pair"); + let units = build_units(BOTH_SIDES, 1000); + assert_eq!(units.len(), 2, "the engine sees two files, two units"); + + let linked = link_related(units, &graph); + assert_eq!(linked.len(), 1, "the graph sees one change"); + + let unit = &linked[0]; + assert_eq!(unit.files.len(), 2); + assert!(unit.text.contains("src/auth.rs")); + assert!(unit.text.contains("src/api.rs")); + assert!(unit.text.contains("strict: bool")); + assert!(unit.text.contains("validate_token(t, true)")); + + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn unrelated_files_are_left_alone() { + let root = project("unrelated"); + write(&root, "src/a.rs", "pub fn alpha() {}\n"); + write(&root, "src/b.rs", "pub fn beta() {}\n"); + let graph = indexed(&root); + + let diff = "\ +diff --git a/src/a.rs b/src/a.rs ++++ b/src/a.rs +@@ -1,1 +1,1 @@ ++pub fn alpha() { let x = 1; } +diff --git a/src/b.rs b/src/b.rs ++++ b/src/b.rs +@@ -1,1 +1,1 @@ ++pub fn beta() { let y = 2; } +"; + let units = build_units(diff, 1000); + assert_eq!( + link_related(units, &graph).len(), + 2, + "no call edge, no merge" + ); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn merging_never_loses_a_hunk() { + let (root, graph) = setup("lossless"); + let before = build_units(BOTH_SIDES, 1000); + let before_hunks: usize = before.iter().map(|u| u.hunk_count).sum(); + + let after = link_related(before, &graph); + let after_hunks: usize = after.iter().map(|u| u.hunk_count).sum(); + + assert_eq!(before_hunks, after_hunks, "a merge must not drop a hunk"); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_merged_unit_gets_its_own_identity() { + // The cache key is derived from the unit; a merged unit reviewed as one + // call must not collide with either half reviewed alone. + let (root, graph) = setup("identity"); + let units = build_units(BOTH_SIDES, 1000); + let (a, b) = (units[0].id.clone(), units[1].id.clone()); + + let merged = link_related(units, &graph); + assert_ne!(merged[0].id, a); + assert_ne!(merged[0].id, b); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn an_oversized_pair_is_left_apart() { + let root = project("oversize"); + let big = " let x = 1;\n".repeat(400); + write(&root, "src/a.rs", &format!("pub fn target() {{\n{big}}}\n")); + write( + &root, + "src/b.rs", + &format!("pub fn user() {{\n target();\n{big}}}\n"), + ); + let graph = indexed(&root); + + let diff = format!( + "diff --git a/src/a.rs b/src/a.rs\n+++ b/src/a.rs\n@@ -1,401 +1,401 @@\n+pub fn target() {{\n{}\ + diff --git a/src/b.rs b/src/b.rs\n+++ b/src/b.rs\n@@ -1,402 +1,402 @@\n+pub fn user() {{\n+ target();\n{}", + "+ let x = 1;\n".repeat(400), + "+ let x = 1;\n".repeat(400) + ); + let units = build_units(&diff, 2000); + let linked = link_related(units, &graph); + assert!( + linked.len() >= 2, + "merging two huge units would blow the model's attention, not just its window" + ); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn an_empty_or_single_unit_diff_is_untouched() { + let root = project("trivial"); + let graph = indexed(&root); + assert!(link_related(vec![], &graph).is_empty()); + + let diff = + "diff --git a/src/a.rs b/src/a.rs\n+++ b/src/a.rs\n@@ -1,1 +1,1 @@\n+let x = 1;\n"; + assert_eq!(link_related(build_units(diff, 1000), &graph).len(), 1); + let _ = std::fs::remove_dir_all(&root); + } +} diff --git a/apps/tui-cli/src/graph/mod.rs b/apps/tui-cli/src/graph/mod.rs new file mode 100644 index 0000000..17f07d0 --- /dev/null +++ b/apps/tui-cli/src/graph/mod.rs @@ -0,0 +1,14 @@ +//! The code graph: what is defined where, and what refers to it. +//! +//! Two layers. `extract` turns source text into definitions and references with +//! tree-sitter, and knows nothing about storage. `store` keeps them in SQLite +//! and answers the questions a reviewer has — what encloses this line, what does +//! this name refer to, and above all **what calls this**, which the regex index +//! it replaces could never answer at all. + +pub mod extract; +pub mod link; +pub mod store; + +pub use link::link_related; +pub use store::{Def, Graph}; diff --git a/apps/tui-cli/src/graph/store.rs b/apps/tui-cli/src/graph/store.rs new file mode 100644 index 0000000..7eeedce --- /dev/null +++ b/apps/tui-cli/src/graph/store.rs @@ -0,0 +1,593 @@ +//! The graph on disk — `.diffmind/graph.db`. +//! +//! SQLite rather than the JSON file it replaces, for one reason: references. +//! Definitions are small enough to hold in memory, but a repository of any size +//! has an order of magnitude more *mentions* of symbols than declarations of +//! them, and the whole point of the graph is the reverse lookup — "what calls +//! this?" — which needs an index, not a linear scan of a map loaded wholesale +//! on every review. +//! +//! Bodies are **not** stored. A definition records its line range and the source +//! is read from the working tree when needed, so the database stays small and +//! can never serve a snippet that no longer matches the file. + +use anyhow::{Context, Result}; +use rusqlite::{Connection, params}; +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use super::extract::{self, Lang}; + +/// Bumped when the schema changes. A mismatch rebuilds from scratch rather than +/// querying a shape this build does not understand. +const SCHEMA_VERSION: i64 = 1; + +/// Directories never worth walking. +const IGNORE_DIRS: &[&str] = &[ + "node_modules", + "target", + "dist", + "build", + ".next", + ".cache", + "vendor", + "__pycache__", + ".venv", + "venv", + "pkg", +]; + +/// A symbol, located. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Def { + pub path: String, + pub name: String, + pub kind: String, + pub start_line: u32, + pub end_line: u32, +} + +impl Def { + /// The declaration's source, read from the working tree and capped. + /// + /// Read rather than stored: a snippet in the database is a copy that goes + /// stale, and staleness here means showing the model code that is not what + /// it is reviewing. + pub fn source(&self, project_root: &Path, max_lines: usize) -> Option { + let text = std::fs::read_to_string(project_root.join(&self.path)).ok()?; + let lines: Vec<&str> = text.lines().collect(); + let start = (self.start_line as usize).saturating_sub(1); + if start >= lines.len() { + return None; + } + let end = (self.end_line as usize).min(lines.len()); + let end = end.min(start + max_lines); + Some(lines[start..end].join("\n")) + } + + fn span(&self) -> u32 { + self.end_line.saturating_sub(self.start_line) + } +} + +#[derive(Debug, Default, Clone, Copy)] +pub struct IndexStats { + pub files_indexed: usize, + pub files_unchanged: usize, + pub files_removed: usize, + pub definitions: usize, + pub references: usize, +} + +pub struct Graph { + conn: Connection, +} + +impl Graph { + pub fn path(project_root: &Path) -> PathBuf { + project_root.join(".diffmind").join("graph.db") + } + + /// Open (creating if needed). A schema from another version is discarded — + /// rebuilding costs seconds; querying a shape we misunderstand costs trust. + pub fn open(project_root: &Path) -> Result { + let path = Self::path(project_root); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + let conn = Connection::open(&path) + .with_context(|| format!("could not open {}", path.display()))?; + + let version: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?; + if version != SCHEMA_VERSION { + conn.execute_batch( + "DROP TABLE IF EXISTS refs; + DROP TABLE IF EXISTS defs; + DROP TABLE IF EXISTS files;", + )?; + } + + conn.execute_batch(&format!( + "PRAGMA journal_mode = WAL; + PRAGMA synchronous = NORMAL; + CREATE TABLE IF NOT EXISTS files ( + path TEXT PRIMARY KEY, + mtime REAL NOT NULL + ); + CREATE TABLE IF NOT EXISTS defs ( + path TEXT NOT NULL, + name TEXT NOT NULL, + kind TEXT NOT NULL, + start_line INTEGER NOT NULL, + end_line INTEGER NOT NULL + ); + CREATE TABLE IF NOT EXISTS refs ( + path TEXT NOT NULL, + name TEXT NOT NULL, + line INTEGER NOT NULL + ); + CREATE INDEX IF NOT EXISTS defs_name ON defs(name); + CREATE INDEX IF NOT EXISTS defs_path ON defs(path); + CREATE INDEX IF NOT EXISTS refs_name ON refs(name); + CREATE INDEX IF NOT EXISTS refs_path ON refs(path); + PRAGMA user_version = {SCHEMA_VERSION};" + ))?; + + Ok(Graph { conn }) + } + + /// Walk the project and bring the graph up to date, reparsing only files + /// whose mtime moved. + pub fn index(&mut self, project_root: &Path, on_file: &dyn Fn(usize)) -> Result { + let mut stats = IndexStats::default(); + let mut seen: HashSet = HashSet::new(); + let tx = self.conn.transaction()?; + + for entry in walkdir::WalkDir::new(project_root) + .into_iter() + .filter_entry(|e| { + if e.depth() == 0 { + return true; + } + let name = e.file_name().to_string_lossy(); + !IGNORE_DIRS.contains(&name.as_ref()) && !name.starts_with('.') + }) + .filter_map(|e| e.ok()) + .filter(|e| e.file_type().is_file()) + { + let Some(lang) = Lang::for_path(entry.path()) else { + continue; + }; + let Ok(relative) = entry.path().strip_prefix(project_root) else { + continue; + }; + let path = relative.to_string_lossy().replace('\\', "/"); + + let mtime = entry + .metadata() + .ok() + .and_then(|m| m.modified().ok()) + .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) + .map(|d| d.as_secs_f64()) + .unwrap_or(0.0); + + seen.insert(path.clone()); + + let known: Option = tx + .query_row("SELECT mtime FROM files WHERE path = ?1", [&path], |r| { + r.get(0) + }) + .ok(); + if known == Some(mtime) { + stats.files_unchanged += 1; + continue; + } + + let Ok(source) = std::fs::read_to_string(entry.path()) else { + continue; + }; + let Some(extracted) = extract::extract(lang, &source) else { + continue; + }; + + // Replace this file's rows wholesale. Anything subtler would have + // to diff two symbol sets, and a stale definition is worse than a + // re-inserted one. + tx.execute("DELETE FROM defs WHERE path = ?1", [&path])?; + tx.execute("DELETE FROM refs WHERE path = ?1", [&path])?; + + { + let mut insert_def = tx.prepare( + "INSERT INTO defs (path, name, kind, start_line, end_line) \ + VALUES (?1, ?2, ?3, ?4, ?5)", + )?; + for d in &extracted.definitions { + insert_def.execute(params![path, d.name, d.kind, d.start_line, d.end_line])?; + } + let mut insert_ref = + tx.prepare("INSERT INTO refs (path, name, line) VALUES (?1, ?2, ?3)")?; + for r in &extracted.references { + insert_ref.execute(params![path, r.name, r.line])?; + } + } + + tx.execute( + "INSERT INTO files (path, mtime) VALUES (?1, ?2) \ + ON CONFLICT(path) DO UPDATE SET mtime = excluded.mtime", + params![path, mtime], + )?; + + stats.files_indexed += 1; + stats.definitions += extracted.definitions.len(); + stats.references += extracted.references.len(); + on_file(stats.files_indexed); + } + + // Files that have gone. Left behind, they would keep answering queries + // about code that no longer exists. + let stale: Vec = { + let mut q = tx.prepare("SELECT path FROM files")?; + let rows = q.query_map([], |r| r.get::<_, String>(0))?; + rows.filter_map(|r| r.ok()) + .filter(|p| !seen.contains(p)) + .collect() + }; + for path in &stale { + tx.execute("DELETE FROM defs WHERE path = ?1", [path])?; + tx.execute("DELETE FROM refs WHERE path = ?1", [path])?; + tx.execute("DELETE FROM files WHERE path = ?1", [path])?; + } + stats.files_removed = stale.len(); + + tx.commit()?; + Ok(stats) + } + + /// Definitions of `name`, preferring one in `near` when the name is + /// ambiguous — a local declaration is far likelier to be the referent. + pub fn definitions_of(&self, name: &str, near: Option<&str>, limit: usize) -> Vec { + let mut defs = self.query_defs( + "SELECT path, name, kind, start_line, end_line FROM defs WHERE name = ?1 LIMIT ?2", + params![name, limit as i64], + ); + if let Some(near) = near { + defs.sort_by_key(|d| d.path != near); + } + defs + } + + /// The innermost definition whose body contains `line`. + pub fn enclosing(&self, path: &str, line: u32) -> Option { + self.query_defs( + "SELECT path, name, kind, start_line, end_line FROM defs \ + WHERE path = ?1 AND start_line <= ?2 AND end_line >= ?2 \ + ORDER BY (end_line - start_line) ASC LIMIT 1", + params![path, line], + ) + .into_iter() + .next() + } + + /// The definitions that mention `name` — the reverse edge, and the reason + /// this file exists. + /// + /// A reference sitting inside nested declarations belongs to the innermost + /// one; SQLite's bare-column rule hands back the row matching `MIN`. + /// Self-references are excluded, or every symbol would appear to call itself. + pub fn callers_of(&self, name: &str, limit: usize) -> Vec { + let mut callers = self.query_defs( + "SELECT d.path, d.name, d.kind, d.start_line, d.end_line, \ + MIN(d.end_line - d.start_line) \ + FROM refs r \ + JOIN defs d ON d.path = r.path AND r.line >= d.start_line AND r.line <= d.end_line \ + WHERE r.name = ?1 AND d.name != ?1 \ + GROUP BY r.rowid \ + LIMIT ?2", + params![name, limit as i64], + ); + // One caller referencing a symbol five times is still one caller. + callers.sort_by(|a, b| { + (&a.path, a.start_line, &a.name).cmp(&(&b.path, b.start_line, &b.name)) + }); + callers + .dedup_by(|a, b| a.path == b.path && a.start_line == b.start_line && a.name == b.name); + callers.sort_by_key(|d| d.span()); + callers + } + + pub fn counts(&self) -> (usize, usize, usize) { + let one = |sql: &str| -> usize { + self.conn + .query_row(sql, [], |r| r.get::<_, i64>(0)) + .unwrap_or(0) as usize + }; + ( + one("SELECT COUNT(*) FROM files"), + one("SELECT COUNT(*) FROM defs"), + one("SELECT COUNT(*) FROM refs"), + ) + } + + pub fn is_empty(&self) -> bool { + self.counts().1 == 0 + } + + fn query_defs(&self, sql: &str, args: impl rusqlite::Params) -> Vec { + let Ok(mut stmt) = self.conn.prepare(sql) else { + return Vec::new(); + }; + let rows = stmt.query_map(args, |r| { + Ok(Def { + path: r.get(0)?, + name: r.get(1)?, + kind: r.get(2)?, + start_line: r.get(3)?, + end_line: r.get(4)?, + }) + }); + match rows { + Ok(rows) => rows.filter_map(|r| r.ok()).collect(), + Err(_) => Vec::new(), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn project(name: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("diffmind-graph-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(d.join("src")).unwrap(); + d + } + + fn write(root: &Path, rel: &str, body: &str) { + let p = root.join(rel); + if let Some(parent) = p.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(p, body).unwrap(); + } + + fn indexed(root: &Path) -> Graph { + let mut g = Graph::open(root).unwrap(); + g.index(root, &|_| {}).unwrap(); + g + } + + const LIB: &str = "\ +pub fn validate_token(t: &str) -> bool { + !t.is_empty() +} +"; + + const CALLER: &str = "\ +pub fn login(token: &str) -> bool { + validate_token(token) +} + +pub fn refresh(token: &str) -> bool { + validate_token(token) +} +"; + + /// The question the regex index could never answer. + #[test] + fn callers_are_found_across_files() { + let root = project("callers"); + write(&root, "src/lib.rs", LIB); + write(&root, "src/api.rs", CALLER); + let g = indexed(&root); + + let callers = g.callers_of("validate_token", 10); + let names: Vec<&str> = callers.iter().map(|d| d.name.as_str()).collect(); + assert!(names.contains(&"login"), "got {names:?}"); + assert!(names.contains(&"refresh"), "got {names:?}"); + assert!( + callers.iter().all(|d| d.path == "src/api.rs"), + "callers should be located where they are, not where the symbol is" + ); + + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_symbol_does_not_count_as_its_own_caller() { + let root = project("self"); + write( + &root, + "src/a.rs", + "pub fn recurse(n: u32) { recurse(n - 1) }\n", + ); + let g = indexed(&root); + assert!( + g.callers_of("recurse", 10).is_empty(), + "self-reference is not a blast radius" + ); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_caller_referencing_a_symbol_twice_is_still_one_caller() { + let root = project("dedup"); + write( + &root, + "src/a.rs", + "fn target() {}\nfn user() { target(); target(); target(); }\n", + ); + let g = indexed(&root); + assert_eq!(g.callers_of("target", 10).len(), 1); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn the_innermost_definition_owns_a_reference() { + let root = project("innermost"); + write( + &root, + "src/a.rs", + "mod outer {\n pub fn inner() { helper(); }\n}\nfn helper() {}\n", + ); + let g = indexed(&root); + let callers = g.callers_of("helper", 10); + assert_eq!(callers[0].name, "inner", "not the enclosing module"); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn enclosing_finds_the_symbol_a_line_sits_in() { + let root = project("enclosing"); + write(&root, "src/a.rs", CALLER); + let g = indexed(&root); + assert_eq!( + g.enclosing("src/a.rs", 2).map(|d| d.name), + Some("login".into()) + ); + assert_eq!( + g.enclosing("src/a.rs", 6).map(|d| d.name), + Some("refresh".into()) + ); + assert!(g.enclosing("src/a.rs", 999).is_none()); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn reindexing_skips_files_that_have_not_moved() { + let root = project("incremental"); + write(&root, "src/a.rs", LIB); + let mut g = Graph::open(&root).unwrap(); + + let first = g.index(&root, &|_| {}).unwrap(); + assert_eq!(first.files_indexed, 1); + + let second = g.index(&root, &|_| {}).unwrap(); + assert_eq!(second.files_indexed, 0, "nothing changed"); + assert_eq!(second.files_unchanged, 1); + + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn an_edited_file_is_refreshed_rather_than_duplicated() { + let root = project("refresh"); + write(&root, "src/a.rs", "pub fn before() {}\n"); + let mut g = Graph::open(&root).unwrap(); + g.index(&root, &|_| {}).unwrap(); + + std::thread::sleep(std::time::Duration::from_millis(20)); + write(&root, "src/a.rs", "pub fn after() {}\n"); + g.index(&root, &|_| {}).unwrap(); + + assert!( + g.definitions_of("before", None, 10).is_empty(), + "stale symbol survived" + ); + assert_eq!(g.definitions_of("after", None, 10).len(), 1); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_deleted_file_drops_out_of_the_graph() { + let root = project("deleted"); + write(&root, "src/gone.rs", "pub fn vanishing() {}\n"); + let mut g = Graph::open(&root).unwrap(); + g.index(&root, &|_| {}).unwrap(); + assert_eq!(g.definitions_of("vanishing", None, 10).len(), 1); + + std::fs::remove_file(root.join("src/gone.rs")).unwrap(); + let stats = g.index(&root, &|_| {}).unwrap(); + + assert_eq!(stats.files_removed, 1); + assert!( + g.definitions_of("vanishing", None, 10).is_empty(), + "a deleted file must stop answering queries" + ); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_local_definition_is_preferred_when_a_name_is_ambiguous() { + let root = project("ambiguous"); + write(&root, "src/a.rs", "pub struct Config { a: u32 }\n"); + write(&root, "src/b.rs", "pub struct Config { b: u32 }\n"); + let g = indexed(&root); + + let defs = g.definitions_of("Config", Some("src/b.rs"), 10); + assert_eq!(defs.len(), 2, "both are real and both are kept"); + assert_eq!(defs[0].path, "src/b.rs", "the local one comes first"); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn source_is_read_from_the_working_tree_not_the_database() { + let root = project("source"); + write(&root, "src/a.rs", "pub fn f() {\n let x = 1;\n}\n"); + let g = indexed(&root); + let def = g.definitions_of("f", None, 1).remove(0); + + assert!(def.source(&root, 100).unwrap().contains("let x = 1;")); + + // Edit without reindexing: the body must reflect the file, since that is + // what the reviewer is actually looking at. + write(&root, "src/a.rs", "pub fn f() {\n let x = 99;\n}\n"); + assert!(def.source(&root, 100).unwrap().contains("let x = 99;")); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_huge_definition_is_capped() { + let root = project("cap"); + let body = format!("pub fn big() {{\n{}}}\n", " let x = 1;\n".repeat(500)); + write(&root, "src/a.rs", &body); + let g = indexed(&root); + let def = g.definitions_of("big", None, 1).remove(0); + assert_eq!(def.source(&root, 10).unwrap().lines().count(), 10); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn ignored_directories_are_not_walked() { + let root = project("ignored"); + write(&root, "src/real.rs", "pub fn real() {}\n"); + write(&root, "node_modules/dep/index.js", "function fake() {}\n"); + write(&root, "target/debug/gen.rs", "pub fn generated() {}\n"); + let g = indexed(&root); + + assert_eq!(g.definitions_of("real", None, 10).len(), 1); + assert!(g.definitions_of("fake", None, 10).is_empty()); + assert!(g.definitions_of("generated", None, 10).is_empty()); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_schema_from_another_version_is_rebuilt_rather_than_queried() { + let root = project("schema"); + write(&root, "src/a.rs", LIB); + { + let mut g = Graph::open(&root).unwrap(); + g.index(&root, &|_| {}).unwrap(); + } + // Simulate an older build's schema marker. + { + let conn = Connection::open(Graph::path(&root)).unwrap(); + conn.execute_batch("PRAGMA user_version = 999;").unwrap(); + } + let g = Graph::open(&root).unwrap(); + assert!( + g.is_empty(), + "a shape we do not understand must be discarded" + ); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn an_empty_project_is_not_an_error() { + let root = project("empty"); + let g = indexed(&root); + assert_eq!(g.counts(), (0, 0, 0)); + assert!(g.callers_of("anything", 10).is_empty()); + let _ = std::fs::remove_dir_all(&root); + } +} diff --git a/apps/tui-cli/src/indexer.rs b/apps/tui-cli/src/indexer.rs deleted file mode 100644 index 15a4f38..0000000 --- a/apps/tui-cli/src/indexer.rs +++ /dev/null @@ -1,552 +0,0 @@ -use anyhow::Result; -use chrono::Utc; -use regex::Regex; -use serde::{Deserialize, Serialize}; -use std::collections::{HashMap, HashSet}; -use std::fs; -use std::path::{Path, PathBuf}; -use walkdir::WalkDir; - -/// Bumped when the on-disk shape changes so a stale index is rebuilt rather -/// than half-deserialized. -const INDEX_VERSION: &str = "2.0.0"; - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct SymbolDefinition { - pub name: String, - pub file: String, - pub line: usize, - /// Last line of the definition, used to find the symbol enclosing a hunk. - #[serde(default)] - pub end_line: usize, - pub snippet: String, - pub r#type: String, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct SymbolIndex { - pub version: String, - pub project_root: String, - pub updated_at: String, - /// Symbol name → every definition with that name. - /// - /// This was a flat `HashMap` where the first - /// definition encountered won, so a common name like `Config` or `handler` - /// silently shadowed every other one in the repo and the model was handed - /// the wrong body. - pub symbols: HashMap>, - pub file_mtimes: HashMap, -} - -impl SymbolIndex { - pub fn symbol_count(&self) -> usize { - self.symbols.values().map(Vec::len).sum() - } - - /// Definitions of `name`, preferring one in `near_file` when the name is - /// ambiguous — a local definition is far likelier to be the referent. - pub fn lookup(&self, name: &str, near_file: Option<&str>) -> Option<&SymbolDefinition> { - let defs = self.symbols.get(name)?; - if let Some(file) = near_file - && let Some(local) = defs.iter().find(|d| d.file == file) - { - return Some(local); - } - defs.first() - } - - /// The definition whose body contains `line` in `file` — the function a - /// diff hunk landed inside. - pub fn enclosing(&self, file: &str, line: usize) -> Option<&SymbolDefinition> { - self.symbols - .values() - .flatten() - .filter(|d| d.file == file && d.line <= line && line <= d.end_line.max(d.line)) - // Innermost wins when definitions nest. - .min_by_key(|d| d.end_line.saturating_sub(d.line)) - } -} - -lazy_static::lazy_static! { - pub static ref IGNORE_DIRS: HashSet<&'static str> = vec![ - "node_modules", ".git", "dist", "pkg", ".diffmind", "target", - "build", ".next", ".cache", "vendor", "__pycache__", ".venv", "venv", - ].into_iter().collect(); - - pub static ref EXTENSIONS: HashSet<&'static str> = vec![ - "ts", "tsx", "js", "jsx", "go", "py", "rs" - ].into_iter().collect(); - - pub static ref COMMON_KEYWORDS: HashSet<&'static str> = vec![ - "if", "else", "for", "while", "return", "const", "let", "var", - "function", "class", "interface", "type", "import", "export", - "from", "async", "await", "true", "false", "null", "undefined", - "string", "number", "boolean", "any", "void", "Promise", - "self", "this", "new", "match", "impl", "fn", "pub", "mut", "struct", - "trait", "enum", "use", "mod", "def", "pass", "None", "True", "False", - ].into_iter().collect(); -} - -/// Definition patterns, compiled once rather than on every file. -struct Patterns { - entries: Vec<(&'static str, Regex)>, -} - -impl Patterns { - fn new() -> Result { - let entries = vec![ - // TypeScript / JavaScript - ( - "function", - Regex::new(r"export\s+(?:async\s+)?function\s+([a-zA-Z0-9_$]+)")?, - ), - ( - "class", - Regex::new(r"export\s+(?:abstract\s+)?class\s+([a-zA-Z0-9_$]+)")?, - ), - ( - "interface", - Regex::new(r"export\s+interface\s+([a-zA-Z0-9_$]+)")?, - ), - ("type", Regex::new(r"export\s+type\s+([a-zA-Z0-9_$]+)")?), - ( - "const", - Regex::new(r"export\s+(?:const|let|var)\s+([a-zA-Z0-9_$]+)")?, - ), - // Go - ( - "function", - Regex::new(r"(?m)^func\s+(?:\([^)]*\)\s*)?([A-Z][a-zA-Z0-9_$]*)")?, - ), - ( - "interface", - Regex::new(r"(?m)^type\s+([A-Z][a-zA-Z0-9_$]*)\s+interface")?, - ), - ( - "class", - Regex::new(r"(?m)^type\s+([A-Z][a-zA-Z0-9_$]*)\s+struct")?, - ), - // Python - ("function", Regex::new(r"(?m)^\s*def\s+([a-zA-Z0-9_]+)\(")?), - ("class", Regex::new(r"(?m)^\s*class\s+([a-zA-Z0-9_]+)[(:]")?), - // Rust - ( - "function", - Regex::new(r"pub\s+(?:async\s+)?fn\s+([a-z0-9_]+)")?, - ), - ("class", Regex::new(r"pub\s+struct\s+([A-Z][a-zA-Z0-9]*)")?), - ("enum", Regex::new(r"pub\s+enum\s+([A-Z][a-zA-Z0-9]*)")?), - ( - "interface", - Regex::new(r"pub\s+trait\s+([A-Z][a-zA-Z0-9]*)")?, - ), - ]; - Ok(Patterns { entries }) - } -} - -pub struct Indexer { - project_root: PathBuf, - symbols: HashMap>, - patterns: Patterns, -} - -impl Indexer { - pub fn new(project_root: PathBuf) -> Result { - Ok(Self { - project_root, - symbols: HashMap::new(), - patterns: Patterns::new()?, - }) - } - - pub fn build_index(&mut self, existing: Option) -> Result { - let mut file_mtimes = HashMap::new(); - - // Only reuse a prior index if it was written by this format version. - let existing = existing.filter(|i| i.version == INDEX_VERSION); - - if let Some(ref idx) = existing { - self.symbols = idx.symbols.clone(); - } - let old_mtimes = existing.map(|i| i.file_mtimes).unwrap_or_default(); - - for entry in WalkDir::new(&self.project_root) - .into_iter() - .filter_entry(|e| { - let name = e.file_name().to_string_lossy(); - !IGNORE_DIRS.contains(name.as_ref()) && !name.starts_with('.') || e.depth() == 0 - }) - .filter_map(|e| e.ok()) - { - if !entry.file_type().is_file() { - continue; - } - let ext = entry - .path() - .extension() - .and_then(|s| s.to_str()) - .unwrap_or(""); - if !EXTENSIONS.contains(ext) { - continue; - } - - let Ok(relative) = entry.path().strip_prefix(&self.project_root) else { - continue; - }; - let relative_path = relative.to_string_lossy().replace('\\', "/"); - - let Ok(metadata) = entry.metadata() else { - continue; - }; - let Ok(modified) = metadata.modified() else { - continue; - }; - let Ok(since_epoch) = modified.duration_since(std::time::UNIX_EPOCH) else { - continue; - }; - let mtime = since_epoch.as_secs_f64(); - - file_mtimes.insert(relative_path.clone(), mtime); - - if Some(&mtime) != old_mtimes.get(&relative_path) { - // Drop this file's previous symbols before reparsing. - // - // Without this, `parse_file` skipped any name already in the - // map — which after loading the existing index meant *every* - // name — so a changed definition was never refreshed and the - // only fix was deleting symbols.json by hand. - self.forget_file(&relative_path); - if let Err(e) = self.parse_file(entry.path(), &relative_path) { - eprintln!(" ! could not index {relative_path}: {e}"); - } - } - } - - // Clean up deleted files. - for defs in self.symbols.values_mut() { - defs.retain(|d| file_mtimes.contains_key(&d.file)); - } - self.symbols.retain(|_, defs| !defs.is_empty()); - - Ok(SymbolIndex { - version: INDEX_VERSION.to_string(), - project_root: self.project_root.to_string_lossy().to_string(), - updated_at: Utc::now().to_rfc3339(), - symbols: self.symbols.clone(), - file_mtimes, - }) - } - - fn forget_file(&mut self, relative_path: &str) { - for defs in self.symbols.values_mut() { - defs.retain(|d| d.file != relative_path); - } - self.symbols.retain(|_, defs| !defs.is_empty()); - } - - fn parse_file(&mut self, absolute_path: &Path, relative_path: &str) -> Result<()> { - let content = fs::read_to_string(absolute_path)?; - let lines: Vec<&str> = content.lines().collect(); - - for (i, line) in lines.iter().enumerate() { - // Cheap pre-filter: skip lines that cannot be a definition. - if !line.contains("export") - && !line.contains("pub ") - && !line.trim_start().starts_with("def ") - && !line.trim_start().starts_with("class ") - && !line.starts_with("func ") - && !line.starts_with("type ") - { - continue; - } - - for (kind, re) in &self.patterns.entries { - for cap in re.captures_iter(line) { - let name = &cap[1]; - let (snippet, end_line) = extract_snippet(&lines, i); - - let defs = self.symbols.entry(name.to_string()).or_default(); - // Same name, same file, same line = the same symbol matched - // by two overlapping patterns. - if defs - .iter() - .any(|d| d.file == relative_path && d.line == i + 1) - { - continue; - } - defs.push(SymbolDefinition { - name: name.to_string(), - file: relative_path.to_string(), - line: i + 1, - end_line: end_line + 1, - r#type: kind.to_string(), - snippet, - }); - } - } - } - - Ok(()) - } - - pub fn save(&self, index: &SymbolIndex) -> Result<()> { - let dir = PathBuf::from(&index.project_root).join(".diffmind"); - fs::create_dir_all(&dir)?; - let path = dir.join("symbols.json"); - let tmp = dir.join("symbols.json.tmp"); - fs::write(&tmp, serde_json::to_string(index)?)?; - fs::rename(&tmp, &path)?; - Ok(()) - } - - pub fn load(project_root: &Path) -> Option { - let path = project_root.join(".diffmind").join("symbols.json"); - let raw = fs::read_to_string(path).ok()?; - let index: SymbolIndex = serde_json::from_str(&raw).ok()?; - // An index from an older layout is worse than none: it would feed the - // model definitions in a shape this build no longer understands. - (index.version == INDEX_VERSION).then_some(index) - } -} - -/// Body of the definition starting at `start_line`, plus the line it ends on. -/// -/// Brace-counting handles the C family and Go; indentation handles Python, -/// which has no braces to count and previously fell through to a 40-line -/// window regardless of the function's real size. -fn extract_snippet(lines: &[&str], start_line: usize) -> (String, usize) { - const MAX_LINES: usize = 60; - - let is_python_style = { - let t = lines[start_line].trim_start(); - (t.starts_with("def ") || t.starts_with("class ")) - && lines[start_line].trim_end().ends_with(':') - }; - - let end_line = if is_python_style { - python_block_end(lines, start_line, MAX_LINES) - } else { - brace_block_end(lines, start_line, MAX_LINES) - }; - - (lines[start_line..=end_line].join("\n"), end_line) -} - -fn brace_block_end(lines: &[&str], start_line: usize, max_lines: usize) -> usize { - let mut depth = 0i32; - let mut opened = false; - let mut end = start_line; - - for (i, line) in lines.iter().enumerate().skip(start_line).take(max_lines) { - let (delta, has_open) = count_braces_in_line(line); - depth += delta; - if has_open { - opened = true; - } - end = i; - if opened && depth <= 0 { - break; - } - // A single-line declaration with no braces at all (a type alias, a - // const) ends where it starts. - if !opened && i > start_line && line.trim().is_empty() { - end = i - 1; - break; - } - } - end -} - -fn python_block_end(lines: &[&str], start_line: usize, max_lines: usize) -> usize { - let indent_of = |l: &str| l.len() - l.trim_start().len(); - let base = indent_of(lines[start_line]); - let mut end = start_line; - - for (i, line) in lines - .iter() - .enumerate() - .skip(start_line + 1) - .take(max_lines) - { - if line.trim().is_empty() { - continue; - } - if indent_of(line) <= base { - break; - } - end = i; - } - end -} - -fn count_braces_in_line(line: &str) -> (i32, bool) { - let mut delta = 0; - let mut has_open_brace = false; - let mut in_string = false; - let mut string_char = ' '; - let mut escaped = false; - - for ch in line.chars() { - if in_string { - if escaped { - escaped = false; - continue; - } - if ch == '\\' { - escaped = true; - continue; - } - if ch == string_char { - in_string = false; - } - continue; - } - if ch == '"' || ch == '\'' || ch == '`' { - in_string = true; - string_char = ch; - continue; - } - if ch == '{' { - delta += 1; - has_open_brace = true; - } else if ch == '}' { - delta -= 1; - } - } - - (delta, has_open_brace) -} - -#[cfg(test)] -mod tests { - use super::*; - - fn tmpdir(name: &str) -> PathBuf { - let d = std::env::temp_dir().join(format!("diffmind-idx-{name}-{}", std::process::id())); - let _ = fs::remove_dir_all(&d); - fs::create_dir_all(&d).unwrap(); - d - } - - #[test] - fn a_changed_definition_is_refreshed_on_reindex() { - let dir = tmpdir("refresh"); - let file = dir.join("a.ts"); - fs::write(&file, "export function greet() {\n return 'v1';\n}\n").unwrap(); - - let mut indexer = Indexer::new(dir.clone()).unwrap(); - let first = indexer.build_index(None).unwrap(); - assert!(first.lookup("greet", None).unwrap().snippet.contains("v1")); - - // Rewrite with a distinct mtime. - std::thread::sleep(std::time::Duration::from_millis(20)); - fs::write(&file, "export function greet() {\n return 'v2';\n}\n").unwrap(); - - let mut indexer = Indexer::new(dir.clone()).unwrap(); - let second = indexer.build_index(Some(first)).unwrap(); - assert!( - second.lookup("greet", None).unwrap().snippet.contains("v2"), - "an incremental reindex must pick up the new body" - ); - - let _ = fs::remove_dir_all(&dir); - } - - #[test] - fn same_name_in_two_files_keeps_both_definitions() { - let dir = tmpdir("collide"); - fs::create_dir_all(dir.join("a")).unwrap(); - fs::create_dir_all(dir.join("b")).unwrap(); - fs::write(dir.join("a/x.ts"), "export type Config = { a: string };\n").unwrap(); - fs::write(dir.join("b/y.ts"), "export type Config = { b: number };\n").unwrap(); - - let mut indexer = Indexer::new(dir.clone()).unwrap(); - let index = indexer.build_index(None).unwrap(); - - let defs = index - .symbols - .get("Config") - .expect("Config should be indexed"); - assert_eq!(defs.len(), 2, "one definition must not shadow the other"); - - // Lookup prefers a definition in the file we are reviewing. - let near = index.lookup("Config", Some("b/y.ts")).unwrap(); - assert_eq!(near.file, "b/y.ts"); - - let _ = fs::remove_dir_all(&dir); - } - - #[test] - fn deleted_files_drop_out_of_the_index() { - let dir = tmpdir("delete"); - fs::write(dir.join("gone.ts"), "export const willVanish = 1;\n").unwrap(); - - let mut indexer = Indexer::new(dir.clone()).unwrap(); - let first = indexer.build_index(None).unwrap(); - assert!(first.lookup("willVanish", None).is_some()); - - fs::remove_file(dir.join("gone.ts")).unwrap(); - let mut indexer = Indexer::new(dir.clone()).unwrap(); - let second = indexer.build_index(Some(first)).unwrap(); - assert!(second.lookup("willVanish", None).is_none()); - - let _ = fs::remove_dir_all(&dir); - } - - #[test] - fn enclosing_finds_the_function_a_line_sits_in() { - let dir = tmpdir("enclosing"); - fs::write( - dir.join("a.rs"), - "pub fn outer() {\n let x = 1;\n let y = 2;\n}\n\npub fn other() {\n ok();\n}\n", - ) - .unwrap(); - - let mut indexer = Indexer::new(dir.clone()).unwrap(); - let index = indexer.build_index(None).unwrap(); - - assert_eq!( - index.enclosing("a.rs", 2).map(|d| d.name.as_str()), - Some("outer") - ); - assert_eq!( - index.enclosing("a.rs", 7).map(|d| d.name.as_str()), - Some("other") - ); - - let _ = fs::remove_dir_all(&dir); - } - - #[test] - fn python_snippets_end_at_the_dedent() { - let src = vec![ - "def handler(req):", - " a = 1", - " return a", - "", - "def other():", - " pass", - ]; - let (snippet, end) = extract_snippet(&src, 0); - assert_eq!(end, 2, "should stop before the blank line and next def"); - assert!(snippet.contains("return a")); - assert!(!snippet.contains("def other")); - } - - #[test] - fn an_index_from_an_older_version_is_rejected() { - let dir = tmpdir("version"); - fs::create_dir_all(dir.join(".diffmind")).unwrap(); - fs::write( - dir.join(".diffmind/symbols.json"), - r#"{"version":"1.1.0","project_root":"/x","updated_at":"","symbols":{},"file_mtimes":{}}"#, - ) - .unwrap(); - assert!( - Indexer::load(&dir).is_none(), - "a stale layout must be rebuilt, not half-read" - ); - let _ = fs::remove_dir_all(&dir); - } -} diff --git a/apps/tui-cli/src/main.rs b/apps/tui-cli/src/main.rs index a8f2342..3826960 100644 --- a/apps/tui-cli/src/main.rs +++ b/apps/tui-cli/src/main.rs @@ -22,8 +22,8 @@ mod config; mod daemon; mod download; mod git; +mod graph; mod hooks; -mod indexer; mod output; mod rag; mod rules; @@ -32,7 +32,7 @@ mod settings; mod tui; use crate::cli::OutputFormat; -use crate::indexer::Indexer; +use crate::graph::Graph; use crate::settings::{BackendChoice, Settings}; /// Exit codes. Distinguishing "found problems" from "could not run" is what @@ -175,18 +175,22 @@ fn run_command( } cli::Commands::Index { rebuild } => { - let mut indexer = Indexer::new(project_root.to_path_buf())?; - let existing = if rebuild { - None - } else { - Indexer::load(project_root) - }; - let index = indexer.build_index(existing)?; - indexer.save(&index)?; + if rebuild { + let _ = std::fs::remove_file(Graph::path(project_root)); + } + let mut graph = Graph::open(project_root)?; + let spinner = make_spinner("Indexing...", false); + let stats = graph.index(project_root, &|n| { + spinner.set_message(format!("Indexing... {n} files")) + })?; + spinner.finish_and_clear(); + runs::ensure_gitignore(project_root); + + let (files, defs, refs) = graph.counts(); println!( - "Index updated: {} symbols across {} files", - index.symbol_count(), - index.file_mtimes.len() + "Indexed {files} file(s): {defs} definitions, {refs} references \ + ({} reparsed, {} unchanged, {} removed)", + stats.files_indexed, stats.files_unchanged, stats.files_removed ); Ok(0) } @@ -570,6 +574,7 @@ pub fn build_analyzer( project: ProjectRules, ) -> ReviewAnalyzer { let mut analyzer = ReviewAnalyzer::new(backend) + .with_unit_grouper(unit_grouper(project_root)) .with_languages(detect_languages(diff)) .with_custom_rules(project.custom) .with_rulebooks(project.books) @@ -720,12 +725,23 @@ fn read_head(path: &Path) -> std::io::Result { /// a symbol lookup rather than a re-read of `symbols.json`. Returning a closure /// (instead of one context string for the whole diff) is what keeps each /// chunk's cache key independent of the other files in the diff. +/// Merge units the code graph says are two halves of one change. Without a +/// graph this is the identity function, so behaviour is unchanged. +pub fn unit_grouper(project_root: &Path) -> core_engine::analyzer::UnitGrouper { + let graph = Graph::open(project_root).ok().filter(|g| !g.is_empty()); + Box::new(move |units| match &graph { + Some(g) => graph::link_related(units, g), + None => units, + }) +} + pub fn context_builder(project_root: &Path, budget: usize) -> impl Fn(&str) -> String { - let index = Indexer::load(project_root); + let graph = Graph::open(project_root).ok().filter(|g| !g.is_empty()); + let root = project_root.to_path_buf(); move |chunk: &str| { - index + graph .as_ref() - .and_then(|index| rag::build_context(chunk, index, budget)) + .and_then(|g| rag::build_context(chunk, g, &root, budget)) .unwrap_or_default() } } diff --git a/apps/tui-cli/src/rag.rs b/apps/tui-cli/src/rag.rs index 7ce6a31..3b3c778 100644 --- a/apps/tui-cli/src/rag.rs +++ b/apps/tui-cli/src/rag.rs @@ -1,70 +1,142 @@ -//! Context assembly for the review prompt. +//! Context assembly — deciding what the model gets to see besides the diff. //! //! Reviewing a bare diff is why LLM reviewers hallucinate: the model cannot see -//! the function a hunk landed in, so it guesses at invariants. Two sources fix +//! the function a hunk landed in, so it guesses at invariants. Four sources fix //! that, in priority order: //! -//! 1. the *enclosing* definition of each changed hunk — the single most -//! useful thing to show, and previously not provided at all; -//! 2. definitions of symbols the diff references but does not contain. +//! 1. the **enclosing** definition of each changed hunk — what the change is +//! part of; +//! 2. **callers** of the changed symbols — the blast radius. A signature or +//! contract change is only reviewable against the code that depends on it, +//! and this is the edge the old regex index could not produce at all; +//! 3. definitions of symbols the diff **references** but does not contain; +//! 4. the **test file** for the changed file, which states the intended +//! behaviour more precisely than any amount of surrounding code. +//! +//! Everything is bounded. Context that grows with the repository would defeat +//! the point: the budget is spent on the few facts that bear on this hunk, not +//! on a summary of everything. -use crate::indexer::{COMMON_KEYWORDS, SymbolIndex}; +use crate::graph::{Def, Graph}; use core_engine::diff::{FileDiff, parse_diff}; -use regex::Regex; use std::collections::HashSet; +use std::path::Path; -/// How many enclosing bodies to include before the budget is better spent on -/// referenced symbols. -const MAX_ENCLOSING: usize = 6; -/// Cap on referenced-symbol definitions. -const MAX_REFERENCED: usize = 8; +/// Enclosing bodies to include before the budget is better spent elsewhere. +const MAX_ENCLOSING: usize = 4; +/// Callers per changed symbol. Past a couple, they stop being evidence and +/// start being a directory listing. +const MAX_CALLERS_PER_SYMBOL: usize = 3; +const MAX_CALLERS_TOTAL: usize = 6; +const MAX_REFERENCED: usize = 6; +/// Lines of any single definition. A 400-line function contributes its opening +/// contract, not its whole body. +const MAX_DEF_LINES: usize = 40; /// Build the context block for a diff. Returns `None` when nothing useful was /// found, so the caller can omit the section entirely. -pub fn build_context(diff: &str, index: &SymbolIndex, max_bytes: usize) -> Option { +pub fn build_context( + diff: &str, + graph: &Graph, + project_root: &Path, + max_bytes: usize, +) -> Option { let files = parse_diff(diff); let mut out = String::new(); - let mut included: HashSet<(String, usize)> = HashSet::new(); + let mut included: HashSet<(String, u32)> = HashSet::new(); - // 1. Enclosing definitions. + // 1. What each hunk is part of, and which symbols the change touches. + // + // Asked about the lines that actually changed, not `hunk.new_start` — a + // hunk begins three lines of context *above* the edit, which for a change + // to a function's first line resolves to whatever sits between functions, + // i.e. nothing. That silently emptied the blast radius for exactly the + // change it matters most for: an altered signature. + let mut changed_symbols: Vec = Vec::new(); let mut enclosing_count = 0; - 'outer: for file in &files { + 'files: for file in &files { for hunk in &file.hunks { + let touched = hunk.changed_new_lines(); + let lines = if touched.is_empty() { + // A pure deletion has no post-image line to ask about. + vec![hunk.new_start] + } else { + touched + }; + + let Some(def) = lines.iter().find_map(|l| graph.enclosing(&file.path, *l)) else { + continue; + }; + if !changed_symbols.iter().any(|d| d == &def) { + changed_symbols.push(def.clone()); + } if enclosing_count >= MAX_ENCLOSING { - break 'outer; + continue; + } + if !included.insert((def.path.clone(), def.start_line)) { + continue; } - let Some(def) = index.enclosing(&file.path, hunk.new_start as usize) else { + let Some(body) = def.source(project_root, MAX_DEF_LINES) else { continue; }; - if !included.insert((def.file.clone(), def.line)) { + let entry = format!( + "\n--- Enclosing {} `{}` ({}:{}) ---\n{body}\n", + def.kind, def.name, def.path, def.start_line + ); + if out.len() + entry.len() > max_bytes { + break 'files; + } + out.push_str(&entry); + enclosing_count += 1; + } + } + + // 2. Blast radius. Who depends on what just changed. + let mut caller_count = 0; + 'callers: for symbol in &changed_symbols { + for caller in graph.callers_of(&symbol.name, MAX_CALLERS_PER_SYMBOL) { + if caller_count >= MAX_CALLERS_TOTAL { + break 'callers; + } + if !included.insert((caller.path.clone(), caller.start_line)) { continue; } + let Some(body) = caller.source(project_root, MAX_DEF_LINES) else { + continue; + }; let entry = format!( - "\n--- Enclosing {} `{}` ({}:{}) ---\n{}\n", - def.r#type, def.name, def.file, def.line, def.snippet + "\n--- Caller of `{}`: {} `{}` ({}:{}) ---\n{body}\n", + symbol.name, caller.kind, caller.name, caller.path, caller.start_line ); if out.len() + entry.len() > max_bytes { - break 'outer; + break 'callers; } out.push_str(&entry); - enclosing_count += 1; + caller_count += 1; } } - // 2. Symbols referenced by added lines but defined elsewhere. - for (name, near_file) in referenced_symbols(&files, index) + // 3. Symbols the added lines mention but do not define here. + for (name, near) in referenced_symbols(&files, graph) .into_iter() .take(MAX_REFERENCED) { - let Some(def) = index.lookup(&name, near_file.as_deref()) else { + let Some(def) = graph + .definitions_of(&name, near.as_deref(), 1) + .into_iter() + .next() + else { continue; }; - if !included.insert((def.file.clone(), def.line)) { + if !included.insert((def.path.clone(), def.start_line)) { continue; } + let Some(body) = def.source(project_root, MAX_DEF_LINES) else { + continue; + }; let entry = format!( - "\n--- Definition of `{}` ({}:{}) ---\n{}\n", - def.name, def.file, def.line, def.snippet + "\n--- Definition of `{}` ({}:{}) ---\n{body}\n", + def.name, def.path, def.start_line ); if out.len() + entry.len() > max_bytes { break; @@ -72,144 +144,279 @@ pub fn build_context(diff: &str, index: &SymbolIndex, max_bytes: usize) -> Optio out.push_str(&entry); } + // 4. The test file, which says what the code is *supposed* to do. + for file in &files { + let Some(test) = test_file_for(&file.path, project_root) else { + continue; + }; + let Ok(body) = std::fs::read_to_string(project_root.join(&test)) else { + continue; + }; + let head: String = body + .lines() + .take(MAX_DEF_LINES) + .collect::>() + .join("\n"); + let entry = format!("\n--- Tests for {} ({}) ---\n{head}\n", file.path, test); + if out.len() + entry.len() > max_bytes { + break; + } + out.push_str(&entry); + // One is evidence of intent; several is a second diff to read. + break; + } + (!out.trim().is_empty()).then_some(out) } -/// Identifiers appearing on added lines that the index knows about, paired with -/// the file they were seen in so lookup can prefer a local definition. -fn referenced_symbols(files: &[FileDiff], index: &SymbolIndex) -> Vec<(String, Option)> { - let re = Regex::new(r"[a-zA-Z_$][a-zA-Z0-9_$]*").expect("static pattern"); +/// The test file for `path`, by convention. No graph needed — conventions are +/// how humans find tests, and they are right often enough to be worth 20 lines. +fn test_file_for(path: &str, project_root: &Path) -> Option { + let (dir, file) = match path.rsplit_once('/') { + Some((d, f)) => (d.to_string(), f.to_string()), + None => (String::new(), path.to_string()), + }; + let (stem, ext) = file.rsplit_once('.')?; + // Already a test; it is its own intent. + if stem.ends_with("_test") || stem.ends_with(".test") || stem.ends_with(".spec") { + return None; + } + + let joined = |d: &str, f: &str| { + if d.is_empty() { + f.to_string() + } else { + format!("{d}/{f}") + } + }; + let candidates = [ + joined(&dir, &format!("{stem}.test.{ext}")), + joined(&dir, &format!("{stem}.spec.{ext}")), + joined(&dir, &format!("{stem}_test.{ext}")), + joined(&dir, &format!("__tests__/{stem}.test.{ext}")), + joined(&dir, &format!("test_{stem}.{ext}")), + format!("tests/{stem}.{ext}"), + ]; + candidates + .into_iter() + .find(|c| project_root.join(c).is_file()) +} + +/// Names the added lines mention that the graph knows about, paired with the +/// file they were seen in so lookup can prefer a local definition. +fn referenced_symbols(files: &[FileDiff], graph: &Graph) -> Vec<(String, Option)> { let mut seen = HashSet::new(); let mut out = Vec::new(); for file in files { for hunk in &file.hunks { for line in hunk.added() { - for m in re.find_iter(&line.text) { - let word = m.as_str(); - if COMMON_KEYWORDS.contains(word) || word.len() < 3 { + for word in line + .text + .split(|c: char| !(c.is_alphanumeric() || c == '_' || c == '$')) + { + if word.len() < 3 || !seen.insert(word.to_string()) { continue; } - if !index.symbols.contains_key(word) { + // A name defined in the diff's own file is already visible. + if graph.definitions_of(word, None, 1).is_empty() { continue; } - if seen.insert(word.to_string()) { - out.push((word.to_string(), Some(file.path.clone()))); - } + out.push((word.to_string(), Some(file.path.clone()))); } } } } - out } #[cfg(test)] mod tests { use super::*; - use crate::indexer::{Indexer, SymbolIndex}; use std::path::PathBuf; - /// `name` must be unique per test: these run in parallel, and a shared - /// temp directory means one test deletes another's fixture mid-run. - fn index_from(name: &str, files: &[(&str, &str)]) -> (SymbolIndex, PathBuf) { - let dir = std::env::temp_dir().join(format!("diffmind-rag-{name}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); - for (name, body) in files { - let path = dir.join(name); - if let Some(p) = path.parent() { - std::fs::create_dir_all(p).unwrap(); - } - std::fs::write(path, body).unwrap(); + fn project(name: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("diffmind-rag-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(d.join("src")).unwrap(); + d + } + + fn write(root: &Path, rel: &str, body: &str) { + let p = root.join(rel); + if let Some(parent) = p.parent() { + std::fs::create_dir_all(parent).unwrap(); } - let mut indexer = Indexer::new(dir.clone()).unwrap(); - (indexer.build_index(None).unwrap(), dir) + std::fs::write(p, body).unwrap(); } + fn indexed(root: &Path) -> Graph { + let mut g = Graph::open(root).unwrap(); + g.index(root, &|_| {}).unwrap(); + g + } + + /// The whole reason for the graph: a changed contract is only reviewable + /// against the code that depends on it. #[test] - fn includes_the_enclosing_function_of_a_hunk() { - let (index, dir) = index_from( - "enclosing", - &[( - "a.rs", - "pub fn transfer(amount: u64) {\n check();\n apply();\n log();\n}\n", - )], + fn context_includes_callers_of_a_changed_function() { + let root = project("callers"); + write( + &root, + "src/auth.rs", + "pub fn validate_token(t: &str) -> bool {\n !t.is_empty()\n}\n", ); + write( + &root, + "src/api.rs", + "pub fn login(t: &str) -> bool {\n validate_token(t)\n}\n", + ); + let graph = indexed(&root); - let diff = "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -2,2 +2,2 @@\n- check();\n+ // check();\n"; - let ctx = build_context(diff, &index, 4000).expect("should find the enclosing fn"); - assert!(ctx.contains("Enclosing"), "{ctx}"); + let diff = "\ +diff --git a/src/auth.rs b/src/auth.rs ++++ b/src/auth.rs +@@ -1,3 +1,3 @@ +-pub fn validate_token(t: &str) -> bool { ++pub fn validate_token(t: &str) -> Result<(), Error> { +"; + let ctx = build_context(diff, &graph, &root, 8000).expect("should build context"); + assert!(ctx.contains("Enclosing function `validate_token`")); assert!( - ctx.contains("transfer"), - "the model needs the whole function: {ctx}" + ctx.contains("Caller of `validate_token`"), + "the blast radius is the point:\n{ctx}" ); assert!( - ctx.contains("apply()"), - "including the lines the diff did not touch" + ctx.contains("fn login"), + "the caller's body should be shown" ); - - let _ = std::fs::remove_dir_all(&dir); + let _ = std::fs::remove_dir_all(&root); } + /// The regression that only showed up at real scale: git puts three lines + /// of context above an edit, so a change to a function's *first* line has a + /// hunk starting outside the function entirely. #[test] - fn includes_definitions_of_referenced_symbols() { - let (index, dir) = index_from( - "referenced", - &[ - ( - "lib.ts", - "export function validateToken(t: string) {\n return t.length > 0;\n}\n", - ), - ("app.ts", "export const unused = 1;\n"), - ], + fn a_hunk_whose_context_starts_above_the_function_still_finds_its_callers() { + let root = project("hunkstart"); + write( + &root, + "src/glob.rs", + "const A: u32 = 1;\nconst B: u32 = 2;\n\npub fn matches(p: &str) -> bool {\n !p.is_empty()\n}\n", ); + write( + &root, + "src/user.rs", + "pub fn filter_paths(p: &str) -> bool {\n matches(p)\n}\n", + ); + let graph = indexed(&root); - let diff = "diff --git a/app.ts b/app.ts\n--- a/app.ts\n+++ b/app.ts\n@@ -1 +1,2 @@\n+ if (validateToken(x)) { go(); }\n"; - let ctx = build_context(diff, &index, 4000).expect("should resolve validateToken"); - assert!(ctx.contains("Definition of `validateToken`"), "{ctx}"); - assert!(ctx.contains("t.length > 0")); + // Hunk starts at line 1 (context), but the change is on line 4. + let diff = "\ +diff --git a/src/glob.rs b/src/glob.rs ++++ b/src/glob.rs +@@ -1,6 +1,6 @@ + const A: u32 = 1; + const B: u32 = 2; - let _ = std::fs::remove_dir_all(&dir); +-pub fn matches(p: &str) -> bool { ++pub fn matches(p: &str, strict: bool) -> bool { + !p.is_empty() + } +"; + let ctx = build_context(diff, &graph, &root, 8000).expect("context"); + assert!( + ctx.contains("Caller of `matches`"), + "a changed signature must bring its callers:\n{ctx}" + ); + assert!(ctx.contains("filter_paths")); + let _ = std::fs::remove_dir_all(&root); } #[test] - fn respects_the_byte_budget() { - let (index, dir) = index_from( - "budget", - &[( - "a.rs", - &format!("pub fn big() {{\n{}\n}}\n", " let x = 1;\n".repeat(50)), - )], + fn the_test_file_is_included_when_convention_finds_one() { + let root = project("tests"); + write( + &root, + "src/math.ts", + "export function add(a, b) { return a + b; }\n", ); - let diff = - "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -2,1 +2,1 @@\n+ let x = 2;\n"; - - let ctx = build_context(diff, &index, 50); - assert!( - ctx.as_ref().is_none_or(|c| c.len() <= 200), - "context must not blow past its budget: {ctx:?}" + write( + &root, + "src/math.test.ts", + "test('adds', () => { expect(add(1,2)).toBe(3); });\n", ); + let graph = indexed(&root); + + let diff = "\ +diff --git a/src/math.ts b/src/math.ts ++++ b/src/math.ts +@@ -1,1 +1,1 @@ ++export function add(a, b) { return a - b; } +"; + let ctx = build_context(diff, &graph, &root, 8000).unwrap(); + assert!(ctx.contains("Tests for src/math.ts"), "got:\n{ctx}"); + assert!(ctx.contains("expect(add(1,2))")); + let _ = std::fs::remove_dir_all(&root); + } - let _ = std::fs::remove_dir_all(&dir); + #[test] + fn a_test_file_does_not_look_for_its_own_test_file() { + let root = project("selftest"); + assert_eq!(test_file_for("src/a.test.ts", &root), None); + assert_eq!(test_file_for("src/a_test.rs", &root), None); + let _ = std::fs::remove_dir_all(&root); } #[test] - fn returns_none_when_nothing_is_known() { - let (index, dir) = index_from("nothing", &[("a.rs", "// nothing exported\n")]); - let diff = "diff --git a/z.rs b/z.rs\n--- a/z.rs\n+++ b/z.rs\n@@ -1 +1,2 @@\n+let q = 1;\n"; - assert!(build_context(diff, &index, 4000).is_none()); - let _ = std::fs::remove_dir_all(&dir); + fn context_is_bounded_by_the_byte_budget() { + let root = project("budget"); + let big = format!("pub fn huge() {{\n{}}}\n", " let x = 1;\n".repeat(200)); + write(&root, "src/a.rs", &big); + let graph = indexed(&root); + + let diff = + "diff --git a/src/a.rs b/src/a.rs\n+++ b/src/a.rs\n@@ -2,1 +2,1 @@\n+ let x = 2;\n"; + let ctx = build_context(diff, &graph, &root, 200); + assert!(ctx.as_deref().map(str::len).unwrap_or(0) <= 400); + let _ = std::fs::remove_dir_all(&root); } #[test] - fn common_keywords_are_not_treated_as_symbols() { - let (index, dir) = index_from("keywords", &[("a.ts", "export const type = 1;\n")]); + fn an_unindexed_project_yields_no_context_rather_than_failing() { + let root = project("bare"); + let graph = Graph::open(&root).unwrap(); let diff = - "diff --git a/b.ts b/b.ts\n--- a/b.ts\n+++ b/b.ts\n@@ -1 +1,2 @@\n+const type = 2;\n"; - // `type` is a keyword; pulling its "definition" in would be noise. - let ctx = build_context(diff, &index, 4000); - assert!(ctx.is_none() || !ctx.unwrap().contains("Definition of `type`")); - let _ = std::fs::remove_dir_all(&dir); + "diff --git a/src/a.rs b/src/a.rs\n+++ b/src/a.rs\n@@ -1,1 +1,1 @@\n+let x = 1;\n"; + assert!(build_context(diff, &graph, &root, 8000).is_none()); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_symbol_is_never_included_twice() { + // A function that is both the enclosing definition and a referenced + // symbol should appear once, not burn the budget twice. + let root = project("dupe"); + write( + &root, + "src/a.rs", + "pub fn helper() {}\npub fn caller() {\n helper();\n}\n", + ); + let graph = indexed(&root); + let diff = "\ +diff --git a/src/a.rs b/src/a.rs ++++ b/src/a.rs +@@ -2,3 +2,3 @@ ++pub fn caller() { ++ helper(); ++} +"; + let ctx = build_context(diff, &graph, &root, 8000).unwrap(); + assert_eq!( + ctx.matches("pub fn helper()").count(), + 1, + "duplicated context wastes the budget:\n{ctx}" + ); + let _ = std::fs::remove_dir_all(&root); } } diff --git a/apps/tui-cli/src/runs.rs b/apps/tui-cli/src/runs.rs index 11869ed..0829939 100644 --- a/apps/tui-cli/src/runs.rs +++ b/apps/tui-cli/src/runs.rs @@ -308,6 +308,9 @@ pub fn ensure_gitignore(project_root: &Path) { cache/ runs/ models/ +graph.db +graph.db-wal +graph.db-shm symbols.json daemon.json "; diff --git a/packages/core-engine/src/analyzer.rs b/packages/core-engine/src/analyzer.rs index 453f98c..a1d8265 100644 --- a/packages/core-engine/src/analyzer.rs +++ b/packages/core-engine/src/analyzer.rs @@ -106,8 +106,19 @@ pub struct ReviewAnalyzer { seed: u64, /// Accumulated across every backend call in one `analyze`, including triage. metering: Metering, + /// Optional regrouping of units before review. + unit_grouper: Option, } +/// Merges units the engine has no way to know are related. +/// +/// The engine groups by file and adjacency, which is all a diff can tell it. +/// Whether two units in *different* files are one review — a changed function +/// and a caller that changed with it — is a question only the code graph can +/// answer, and the graph lives in the application. +pub type UnitGrouper = + Box) -> Vec + Send>; + /// What one run cost. Accumulated on the analyzer because `analyze_chunk` /// recurses when a unit overruns the window, and each level must add to the /// same total. @@ -136,9 +147,16 @@ impl ReviewAnalyzer { temperature: defaults.temperature, seed: defaults.seed, metering: Metering::default(), + unit_grouper: None, } } + /// Let the caller merge related units before review. See [`UnitGrouper`]. + pub fn with_unit_grouper(mut self, grouper: UnitGrouper) -> Self { + self.unit_grouper = Some(grouper); + self + } + /// Run the backend, recording what it cost. fn generate_metered( &mut self, @@ -375,6 +393,9 @@ impl ReviewAnalyzer { &reviewable, self.max_chunk_lines(max_tokens_per_chunk as usize), ); + if let Some(group) = &self.unit_grouper { + units = group(std::mem::take(&mut units)); + } stats.units_total = units.len(); // Cloned once per run: `analyze_chunk` needs `&mut self`, so the @@ -386,11 +407,11 @@ impl ReviewAnalyzer { // a backend that can reuse a prompt prefix gets the longest possible // run of hits. Findings are sorted before output regardless, so this // changes only the order work is done in. - units.sort_by_key(|u| rulebook::group_key(&rulebook::applicable(&rulebooks, &u.file))); + units.sort_by_key(|u| rulebook::group_key(&applicable_to_unit(&rulebooks, u))); for (i, unit) in units.iter().enumerate() { on_progress(i + 1, units.len()); - let books = rulebook::applicable(&rulebooks, &unit.file); + let books = applicable_to_unit(&rulebooks, unit); match self.analyze_chunk(&unit.text, context_for, &books, max_tokens_per_chunk, 0) { Ok((unit_summary, cached)) => { @@ -417,7 +438,7 @@ impl ReviewAnalyzer { } Err(EngineError::SerializationError(e)) => { if self.debug { - eprintln!("[debug] unit {} ({}) unparseable: {e}", i + 1, unit.file); + eprintln!("[debug] unit {} ({}) unparseable: {e}", i + 1, unit.file()); } stats.units_unparseable += 1; } @@ -721,6 +742,23 @@ impl ReviewAnalyzer { } } +/// Rule sets governing any file in the unit. A merged unit spans more than one +/// file, and a rule scoped to either of them still applies. +fn applicable_to_unit<'a>( + books: &'a [Rulebook], + unit: &crate::unit::ReviewUnit, +) -> Vec<&'a Rulebook> { + let mut out: Vec<&Rulebook> = Vec::new(); + for file in &unit.files { + for b in rulebook::applicable(books, file) { + if !out.iter().any(|existing| existing.id == b.id) { + out.push(b); + } + } + } + out +} + /// Where a batch of findings came from, so `finalize` can stamp provenance and /// validate any rule set the model claimed. Bundled because these two always /// travel together and are meaningless apart. diff --git a/packages/core-engine/src/unit.rs b/packages/core-engine/src/unit.rs index 6776810..4712864 100644 --- a/packages/core-engine/src/unit.rs +++ b/packages/core-engine/src/unit.rs @@ -34,7 +34,9 @@ const MAX_UNITS_PER_FILE: usize = 8; /// One reviewable region of one file. #[derive(Debug, Clone)] pub struct ReviewUnit { - pub file: String, + /// Files this unit covers. Usually one; more when the graph linked a + /// changed symbol to callers that changed in the same diff. + pub files: Vec, /// Self-contained diff text: the file's header, then this unit's hunks. /// Original bytes, so hunk headers and line numbers are untouched. pub text: String, @@ -49,9 +51,53 @@ pub struct ReviewUnit { } impl ReviewUnit { - /// Does this unit cover `line` in `file`? + /// The file this unit is primarily about — the one its line span refers to. + pub fn file(&self) -> &str { + self.files.first().map(String::as_str).unwrap_or("") + } + + /// Does this unit cover `line` in `file`? Answers for the primary region; + /// a merged unit's other files are carried in `files`. pub fn covers(&self, file: &str, line: u32) -> bool { - self.file == file && line >= self.new_start && line <= self.new_end + self.file() == file && line >= self.new_start && line <= self.new_end + } + + /// Combine two units into one review. + /// + /// Used when a changed symbol and code that calls it were **both** edited. + /// Reviewed apart, the model judges an interaction while seeing only one + /// side of it as background — and pays for the other side's context twice. + /// Together it is one call, one context, and the actual question. + pub fn merged_with(&self, other: &ReviewUnit) -> ReviewUnit { + let mut files = self.files.clone(); + for f in &other.files { + if !files.contains(f) { + files.push(f.clone()); + } + } + + let text = format!("{}{}", self.text, other.text); + let mut h = Sha256::new(); + for f in &files { + h.update(f.as_bytes()); + h.update(b"\x00"); + } + h.update(text.as_bytes()); + + ReviewUnit { + files, + text, + id: format!("{:x}", h.finalize())[..16].to_string(), + hunk_count: self.hunk_count + other.hunk_count, + // The span still describes the primary file; a range across two + // files would not mean anything. + new_start: self.new_start, + new_end: self.new_end, + } + } + + pub fn line_count(&self) -> usize { + self.text.lines().count() } } @@ -178,7 +224,7 @@ fn assemble(file: &RawFile, group: &[usize]) -> ReviewUnit { let id = format!("{:x}", h.finalize())[..16].to_string(); ReviewUnit { - file: file.path.clone(), + files: vec![file.path.clone()], text, id, hunk_count: group.len(),