diff --git a/.no-mistakes.yaml b/.no-mistakes.yaml index 26148bd..7bd2639 100644 --- a/.no-mistakes.yaml +++ b/.no-mistakes.yaml @@ -36,10 +36,9 @@ document: Each class of fact has exactly one owner document. README.md owns the introduction, install, the reader-facing feature list and the CLI table. skills/thurview/SKILL.md owns the agent workflow for authoring a document, - skills/forge-review/SKILL.md the workflow for reviewing a change request - and posting it back, with references/forges.md owning the two forges' - differences and references/security-surfaces.md the per-surface checklist; - and the thurview references own + skills/review-fix/SKILL.md the workflow for reviewing a change and fixing + what it finds, with references/forges.md owning the two forges' + differences; and the thurview references own the contracts: references/document-authoring.md the document rules, references/components.md every data.yaml shape and fenced component, references/lifecycle.md statuses, storage and threads, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 88eae9d..217117d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,8 +36,8 @@ locally. - `src/ui/`: the browser app, vanilla TypeScript bundled by esbuild. - `skills/thurview/`: the agent skill and its references, the single source for every authored file's shape. -- `skills/forge-review/`: the skill that reviews a pull or merge request and - posts it back through `thurview forge`. +- `skills/review-fix/`: the skill that reviews a change with the code graph, + commits the fixes that pass, and can post the rest through `thurview forge`. - `test/e2e.test.ts`: the suite, driving the CLI and the server end to end. - `test/forge.test.ts`: the same, with a fake `gh` and `glab` on PATH, so both forge adapters are driven rather than asserted. @@ -55,7 +55,7 @@ temporary directory. Re-record it when the UI or the CLI output changes. ## The skills and the command `skills/` holds two skills - `thurview` authors and publishes the document, -`forge-review` posts a review of a change request back to its forge. Both are +`review-fix` reviews a change and commits the fixes that pass. Both are installed three ways, and only one of them keeps the skill and the command in step: diff --git a/README.md b/README.md index e1ef0d4..5dec4c1 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ reads the Agent Skills format: ```sh npx skills add Thurbeen/thurview --skill thurview -npx skills add Thurbeen/thurview --skill forge-review # optional, see below +npx skills add Thurbeen/thurview --skill review-fix # optional, see below ``` That form tracks this repository's default branch: `skills update` takes @@ -178,7 +178,7 @@ bar. | `thurview open --review ID [--view T]` | Start the server if needed and open the browser | | `thurview wait --review ID [--timeout S]` | Block until the reader needs the agent | | `thurview threads list\|get\|reply\|resolve` | Read and answer threads | -| `thurview graph interfaces\|impact\|callers\|tests-for\|architecture` | Ask the code graph at the pinned commits | +| `thurview graph interfaces\|impact\|callers\|tests-for\|architecture` | Ask the code graph at a review's pins, or at `--base`/`--head` | | `thurview forge status\|prior\|submit\|reply` | Read a change request through its forge, and post the review back | | `thurview serve` / `thurview stop` | Run the server in the foreground / stop the background one | | `thurview setup hooks\|skill\|status` | Session hooks, agent skill, install state | @@ -194,11 +194,26 @@ arguments shows live state for the current directory instead of a manual. `thurview --help` is the fallback. Progress and diagnostics go to stderr. -## Posting the review to the forge +## Review and fix -A thurview review is read in the browser. When the change is a pull request on -GitHub or a merge request on GitLab, the `forge-review` skill posts it there as -well: inline comments anchored to lines, a summary, and a verdict. +The `review-fix` skill reviews a branch, a commit range or a pull or merge +request, fixes what it is sure of and reports the rest, with no browser and no +approval step. For each changed symbol it asks the code graph who calls it and +which tests reach it, so a finding can name a caller the diff never shows. +Fixes that pass the repository's own tests and lint land as one local commit; +nothing is pushed unless you ask. + +```sh +thurview graph impact --head HEAD # changed symbols, the callers they left alone, the tests +thurview graph callers discount --head HEAD # every call site of one symbol +``` + +`--base` and `--head` ask about two commits directly; `--head` alone diffs +from where it forked from trunk. Each caller in `impact.reach` carries the line +of its call and whether any test reaches it. + +With `--post`, the skill posts the findings it did not fix as inline comments +on the change request, through `thurview forge`: ```sh thurview forge status --change 123 # what CI actually did, and whether it is a gate at all @@ -224,7 +239,7 @@ and gitlab.com are matched against what those CLIs are authenticated for, and an unmatched host is refused rather than guessed. The differences that survive the seam - GitLab has no changes-requested state, no atomic review and no multi-line comment anchor - are listed in -[skills/forge-review/references/forges.md](skills/forge-review/references/forges.md). +[skills/review-fix/references/forges.md](skills/review-fix/references/forges.md). ## Authoring format diff --git a/skills/forge-review/SKILL.md b/skills/forge-review/SKILL.md deleted file mode 100644 index adc7449..0000000 --- a/skills/forge-review/SKILL.md +++ /dev/null @@ -1,276 +0,0 @@ ---- -name: forge-review -description: Review a pull or merge request and post the review back to the forge - inline comments anchored to lines, a summary, a verdict, and a re-review that answers the previous pass point by point. Use when the user asks to review a PR or MR and post the comments, to re-review after a push, to approve or request changes on a pull or merge request, or invokes /forge-review. Built on thurview for the evidence, with the forge as the destination. -user-invocable: true -argument-hint: "[] [--forge github|gitlab]" ---- - -# forge-review - -thurview authors the evidence; the forge is where the review lands. This skill -is the bridge between them, and it is the only one that posts. - -```mermaid -flowchart LR - A[forge status: what CI really did] --> B[forge prior: the previous pass] - B --> C[scaffold + graph: the evidence] - C --> D[publish: the reader approves the review before it is posted] - D --> E[forge submit: comments, summary, verdict] - E --> F[forge reply --resolve: only what is verified] - F -->|author pushes| A -``` - -Run the CLI as `thurview`, or `npx -y thurview` when it is not on PATH. Every -command prints TOON on stdout, errors are structured on stdout too, exit code -2 is a usage error. If a command answers `unknown command` or `unknown flag` -for something below, the installed CLI is older than this skill: run -`thurview update` and retry once. - -## Request - -$ARGUMENTS - -A number or URL names the change request. Empty means the change request open -from the current branch - `thurview forge status` with no `--change` finds it -through the active review's binding, and when there is none, ask which one -rather than guessing. - -## The word - -GitHub calls it a pull request, GitLab a merge request. Everything here calls -it a **change request** and the CLI takes `--change ` on either -forge. Use the forge's own word only when you are writing to the author on -that forge. - -Forge coverage, and what a third forge would need, is in -[Forges](references/forges.md). Read it when a call fails or the forge is not -github.com. - -## What this never does - -Never merge, never close, never push to the branch under review. You usually -cannot push to a fork at all, so **every finding is a comment**. Merging is -the maintainer's decision and the CLI has no command for it. - -## Before you start - -Read `~/.agent-rules/VOICE.md` **on every run**. Every comment, reply and -summary is published on the operator's account, and that file is the spec for -how they read - the sign-off line and its exact wording, the length budget per -comment, severity prefixes, and when to ask instead of assert. It changes; do -not work from memory of it, and do not copy it in here. - -Two consequences of it shape everything below, so they are worth saying once: -a comment is a few lines and no more, and it ends with the sign-off as a plain -last line. A finding that will not fit is **two comments**, or one comment -carrying the claim and one suggestion with the evidence behind a permalink - -never a wall of prose on a line of someone's diff. - -## Workflow - -### 1. Ask the forge what it knows, before forming any opinion - -```sh -thurview forge status --change -``` - -Record `change.head`. That commit is what this pass reviews, and a later pass -diffs against it to find what moved. Note `change.fromFork` and -`change.author`: a change request from outside the organisation is the case -every rule here was learned on. - -### 2. Establish what CI actually ran - -`ci.verdict` is one sentence you can quote. `ci.trustworthy` is the only field -that means "the tests really passed here". - -Two failures this answers, both of which shipped real bugs: - -- **A fork change request has almost no CI.** `ci.baselineRan` is what the - target branch's own tip runs. One check here against twenty there means the - pipeline is not a gate on this change - the review is. -- **A green tick can mean "never ran".** `passed`, `failed`, `cancelled`, - `skipped` and `running` are counted separately because a cancelled job - asserted nothing while showing no failure. A title-gate failure that - cancels the test matrix leaves exactly that shape. - -When `ci.trustworthy` is false, **say so in the summary comment in your own -words, with the counts**. An unstated gap is one the maintainer will assume -you checked. - -### 3. Read the previous pass back, point by point - -```sh -thurview forge prior --change # add --mine for this account's threads -``` - -`summary.passes` of 0 means this is a first pass; skip to step 4. - -Otherwise this is a **re-review, and answering the prior pass is the most -important thing you will do here**. A point raised and then silently dropped -teaches the author that review is noise. - -Go through every open thread and give it exactly one of three words: - -| Word | What you do | -| ------------------- | --------------------------------------------------------------- | -| addressed | Verify it at the current head, then reply and resolve (step 8) | -| partially addressed | Reply saying which part is done and which is not; leave it open | -| untouched | Reply asking for it again, or say why you are dropping it | - -`threads[].atHead` is false when the thread was written against an older -commit, and `outdated` when the code under it moved. Neither means the point -was fixed - only reading the code at the current head means that. - -### 4. Diff only what moved - -On a re-review, read the new work rather than the whole change again: - -```sh -git range-diff ... ... -``` - -The previous head is the one you recorded last pass, or `review.pinnedHead` -from `forge status`. Read the full diff only on a first pass. - -### 5. Get the evidence from thurview - -```sh -thurview scaffold --pr # pins base and head from the forge -thurview graph interfaces --review # what the change moved in the visible surface -thurview graph impact --review # what it reaches that the diff does not show -thurview graph callers --review -thurview graph tests-for --review -``` - -The thurview skill owns these in full - `thurview skill` prints the path to -its SKILL.md, and its references sit beside it. Read them when you author the -document in step 6; do not re-derive structure from hunks. - -Read every line you are about to comment on **at the pinned head**, with -`git show :`, never from the working tree. - -### 6. Decide who reads the review before the forge does - -Default: **a human approves the pass before it is posted.** Author the -thurview document as the thurview skill describes, publish it, and wait: - -```sh -thurview publish --review --open -thurview wait --review --timeout -``` - -The reader sees every finding against its anchored code and approves or sends -it back; `wait.reason` of `accepted` is your signal to post. This is the whole -reason to go through thurview rather than straight to `gh`: comments land on -the operator's account, on a contributor's work, and are read as the -maintainer's word. - -Post without that gate only when the user asked for an unattended run. Say in -the handover which of the two happened. - -**Never put the thurview URL in a forge comment.** That server is local to -this machine; the author cannot open it, and the link leaks a path. Evidence -that must travel goes in a permalink - `status.permalink` shows the shape for -this forge, with the pinned head already in it. - -### 7. Write the pass - -One JSON file, which is also what a human can read before it is posted: - -```json -{ - "verdict": "request-changes", - "body": "", - "comments": [ - { - "path": "src/clip.rs", - "line": 44, - "startLine": 40, - "side": "head", - "body": "" - } - ] -} -``` - -`line` is the LAST line of the range and `startLine` the first. `side` is -`head` unless you are commenting on a line the change deleted. Both forges -refuse a comment on a line the diff does not touch, so anchor inside a hunk. - -Per comment: one point, stated as a problem then a concrete suggestion, -within VOICE.md's length budget, sign-off last. **A finding you cannot -reproduce is a question, not an assertion** - give the mechanism and the -evidence, say what you could not reproduce, and ask the author to confirm. -That is the correct form, not a weaker one. - -The summary body carries what has no line: what CI did and did not establish -(step 2), the security result (step 5 of -[Security surfaces](references/security-surfaces.md)), and who does what next. - -Check it before it goes anywhere: - -```sh -thurview forge submit --change --file pass.json --dry-run -``` - -`warnings` names every comment past the line budget. Split those, do not -shorten by deleting the suggestion. - -### 8. Submit - -```sh -thurview forge submit --change --file pass.json -``` - -`verdict` is `comment`, `request-changes` or `approve`. - -**Approving is a state change with consequences, and you say them out loud -before you do it.** It dismisses a standing request for changes, which is what -makes the change request mergeable, and where auto-merge is armed it merges -the code with no further human read. The CLI refuses an approve without -`--confirm` for that reason; the flag is not a formality, it is the point at -which you have told the user. - -Record `submitted.head`. That is the commit the next pass diffs against. - -### 9. Resolve only what you verified - -```sh -thurview forge reply --change --body "" --resolve --at -``` - -`--at` must be the current head, and the CLI refuses any other. Resolving a -thread tells the author a point was accepted; doing it without reading the -code at that head is worse than leaving it open. A thread deferred by -agreement may be resolved only when the agreement is written in the thread. - -Reply without `--resolve` for a point that is partially addressed. - -### 10. Hand over - -Tell the user, in a few lines: - -- the change request, its head, and the verdict you posted -- what CI established, in one clause, when `ci.trustworthy` was false -- how many comments, and how many prior threads you resolved -- whether a human approved the pass first, or it was unattended -- what you are waiting for now - the author's push, or the maintainer's merge - -## Re-review after a push - -Start at step 1 again. The head will have moved; `forge status` says so and -`review.pinnedHead` holds what you reviewed last. Re-pin with -`thurview scaffold --update --review `, range-diff from the old head, and -answer the prior pass before reading anything new. - -## Completion criteria - -Report completion only when all of these hold: - -- `forge status` was read and its CI verdict is reflected in the summary. -- Every prior thread is marked addressed, partially addressed or untouched. -- Security is stated explicitly, findings or none. -- The pass is posted, with a verdict, and its head is recorded. -- Every thread you resolved was verified at the current head. -- Nothing was merged, closed or pushed. diff --git a/skills/forge-review/references/security-surfaces.md b/skills/forge-review/references/security-surfaces.md deleted file mode 100644 index 3e16e0a..0000000 --- a/skills/forge-review/references/security-surfaces.md +++ /dev/null @@ -1,54 +0,0 @@ -# Security surfaces - -A generic security pass produces generic findings. The checklist has to come -from what the change actually touches, so derive it from the diff and say what -you derived it from. - -## How to derive it - -1. List the surfaces the diff crosses. A surface is a place where the change - meets something it does not control - input it did not produce, a file - system, another process, a network peer, a platform API, a log. -2. For each surface, take its questions from the table below. -3. Ask each question against the code at the pinned head, not against the - hunk. A missing cleanup path is invisible in a diff that only adds lines. -4. Anchor every finding to the line that answers it. -5. **State the result explicitly, findings or none.** "No security findings" - is a result; an absent section is not one, and the maintainer cannot tell - the two apart. - -## Surfaces and their questions - -| The change touches | Ask | -| -------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | -| Text that reaches a model | Prompt injection - whose text is it, what can it make the agent do, what is quoted versus interpreted | -| Temporary files | Predictable name, permissions at creation, symlink races, TOCTOU between check and use, cleanup on every error path as well as the happy one | -| Attacker-controlled names | Path traversal, absolute paths, shell metacharacters, leading dashes read as flags, Unicode that normalises to something else | -| Reading untrusted bytes | Unbounded reads, allocation from a length the input chose, decompression ratios, what happens at the size limit rather than below it | -| Running another process | Argument construction, quoting per platform, whether a shell is involved at all, what the environment carries, working directory | -| Process or PTY lifetime | Orphans on error, exit status that can be forged or lost, signals, what happens when the child outlives the parent | -| Logs and telemetry | What reaches a log that should not - secrets, tokens, file contents, personal data - and whether log lines can be forged by input | -| Platform branches | Each branch checked separately; a guard that holds on Linux and not on Windows is the common shape | -| Authentication or identity | What is trusted, what is verified, what a caller can assert about itself | -| Serialised data | Deserialisation of attacker-controlled shapes, schema validation before use, defaults that silently accept | - -## Two worked examples - -**A clipboard copy through a temporary file.** Surfaces: text that reaches a -model, temporary files, attacker-controlled names, unbounded reads, running -another process, platform branches, logs. Which yields, concretely - is the -temp file name predictable; what mode is it created with; is there a window -between creating and writing it; is it removed when the copy fails and not -only when it succeeds; can the copied text be read back by another user; does -the platform helper get its argument as an argument or through a shell; does -the content reach a log. - -**A window lifecycle change that spawns a process.** Surfaces: running another -process, process lifetime, attacker-controlled names, platform branches. -Which yields - how the command line is built and escaped on each platform; -whether a window name is interpolated into it; whether the child is reaped; -whether its exit status can be forged by something the child does not control; -what the code does when the platform branch it was not written for runs. - -The pattern in both: the surfaces come from the diff, the questions come from -the surfaces, and the answers come from the code at head. diff --git a/skills/review-fix/SKILL.md b/skills/review-fix/SKILL.md new file mode 100644 index 0000000..601f1c0 --- /dev/null +++ b/skills/review-fix/SKILL.md @@ -0,0 +1,165 @@ +--- +name: review-fix +description: Review a change and fix what the review finds - bugs, regressions for callers, missing or broken tests, security issues - with thurview's code graph showing the callers and tests a diff does not. Commits only the fixes that pass the repository's own tests and lint, and reports the rest. Use when the user asks to review and fix a branch, a commit range or a pull or merge request, to find and fix bugs in a change, or invokes /review-fix. +user-invocable: true +argument-hint: "[ | .. | ] [--post]" +--- + +# review-fix + +Review a change, fix what you are sure of, report the rest. The diff shows what +changed; thurview's code graph shows what depends on it, which is where a +change breaks code the diff never shows. + +```mermaid +flowchart LR + A[Scope: pin base and head] --> B[Graph: who reaches the change] + B --> C[Findings] + C --> D[Fix, then the repo's tests and lint] + D -->|green| E[One fix commit] + D -->|red| F[Undo that fix, report why] + E --> G[Report] + F --> G + G -.->|--post| H[Unfixed findings as inline comments] +``` + +Run the CLI as `thurview`, or `npx -y thurview` when it is not on PATH. It +prints TOON, and exit code 2 is a usage error. If it answers `unknown flag` for +something below, run `thurview update` and retry once. + +## Request + +$ARGUMENTS + +## Never + +- Never push, force-push or post unless this request asks for it. `--post` + asks to post comments; pushing the fix commit needs its own ask. +- Never rewrite the branch's history. Fixes are a new commit on top. +- Never report style. A finding is a bug, a regression for a caller, a missing + or broken test, or a security issue. + +## 1. Scope + +Start from a clean tree (`git status --porcelain` prints nothing), otherwise +ask: the fixes get committed. Check out the head, then pin both commits with +one graph call: + +| Request | Check out | Pin with | +| ------------------ | ---------------------------------------------- | --------------------------------------------------------------------------------- | +| empty | the current branch | `thurview graph impact --head HEAD` | +| a branch | `git switch ` | `thurview graph impact --head HEAD` | +| `..` | the branch at `` | `thurview graph impact --base --head HEAD` | +| a PR or MR ref/URL | `gh pr checkout ` or `glab mr checkout ` | `thurview graph impact --base $(git merge-base origin/ HEAD) --head HEAD` | + +With `--head` alone the base is where head forked from trunk. For a change +request, `thurview forge status --change ` names `` as +`change.baseBranch`. + +The output's `base` and `head` are the pins. Pass `--base --head ` +to every later graph command, as its `help` lines do, so your fix commit does +not move what you are reviewing. + +## 2. Ask the graph + +```sh +thurview graph impact --base --head # changed symbols, who reaches them, tests +thurview graph interfaces --base --head # exports and signatures that moved +thurview graph callers --base --head # every call site; --graph base for before +thurview graph tests-for --base --head +``` + +What to take from them: + +- **`impact.reach`** lists code that calls a changed symbol and was not changed + itself - what the author may have forgotten. `at` is the line of the call, + `via` the changed symbol it reaches. Read each call site against the new + behaviour: this is the finding a diff cannot give you. +- **`reach[].tested: false`**: no test reaches that caller, so nothing catches + a break there. +- **`interfaces`** rows `changed` or `removed`: run `callers` on each, with + `--graph base` for a removed one, since head no longer has its callers. +- **`impact.untested`**: changed symbols no test reaches. +- **`unresolved`, `truncated`**: references the graph could not resolve and + files past its cap. "No callers" is only as true as those allow; say so when + a finding rests on it. + +Then read the diff (`git diff `) and every call site the rows name. + +## 3. Findings + +For each one, record: + +- `file:line` at ``, now, before a fix moves lines +- severity: `high` (wrong behaviour on a normal path, data loss, security), + `medium` (a likely bug, or a risky path no test reaches), `low` (real but + narrow) +- why, in one line +- the graph evidence when there is some, e.g. + `reach: checkout src/cart.js:5 via discount, tested false` + +Security means input crossing a trust boundary: a shell command, query or path +built from it, a secret reaching a log, an authorization check the change +skips. + +A problem that is just as present at `` is not this change's finding. +Leave it unfixed and list it after the report's table. + +## 4. Fix + +Find the repository's own test and lint commands - `CONTRIBUTING.md`, +`AGENTS.md`, the package manifest's scripts, a `Makefile`, the CI config - and +run them once before editing. What is already red at head is not yours: note +it, and judge each fix by not making it worse. + +For each finding whose fix is local and whose right behaviour is unambiguous: + +1. For a bug, first add or extend a test and run it: it must fail, for the + reason the finding gives. +2. Edit, then run the tests and lint. +3. Green: keep it. Red: undo only that fix (`git restore `, and delete + files it created) and mark the finding unfixed, with the failure as why. + +Leave a finding unfixed when it needs a decision the code cannot make, changes +an interface used outside this repository, or no test can show it. + +Once every kept fix passes together, commit them as one. Follow the +repository's commit convention when it has one; otherwise: + +```sh +git add +git commit -m "fix: address review findings" -m "" +``` + +## 5. Report + +One table - severity, `file:line`, the finding, its graph evidence, and the fix +commit or why it is unfixed - then the tests and lint after the commit, and +what the graph could not see. Do not push; offer to. + +## 6. Post, with `--post` only + +Only on a change request, only the unfixed findings, and before the fix commit +is pushed, so the lines still match the forge's head. Write `pass.json`: + +```json +{ + "verdict": "comment", + "body": "", + "comments": [{ "path": "src/cart.js", "line": 5, "body": "" }] +} +``` + +A comment must sit on a line the change request's diff touches, or the forge +refuses it; a finding on an untouched caller goes in `body`, with a link in the +`permalink` shape `forge status` prints. Keep each comment to a few lines, and +follow the user's own rules for text posted in their name when they keep any. +Check, then post: + +```sh +thurview forge submit --change --file pass.json --dry-run +thurview forge submit --change --file pass.json +``` + +The verdict stays `comment`: approving is the maintainer's call. How GitHub and +GitLab differ is in [Forges](references/forges.md). diff --git a/skills/forge-review/references/forges.md b/skills/review-fix/references/forges.md similarity index 100% rename from skills/forge-review/references/forges.md rename to skills/review-fix/references/forges.md diff --git a/skills/thurview/SKILL.md b/skills/thurview/SKILL.md index 5a36763..01c4678 100644 --- a/skills/thurview/SKILL.md +++ b/skills/thurview/SKILL.md @@ -19,9 +19,9 @@ request asks for. states what it did not examine. Read [Code explainer](references/code-explainer.md) and follow that instead. -Posting a review back to a forge - inline comments on a pull or merge request, -a verdict, a re-review that answers the previous one - is the `forge-review` -skill, not this one. This skill's document is read in the browser. +Reviewing a change and fixing what the review finds - and posting what stays +unfixed to a pull or merge request - is the `review-fix` skill, not this one. +This skill's document is read in the browser. The agent studies the change and writes a short document in which every claim about code is anchored to an exact file and line range at a pinned commit. diff --git a/src/cli.ts b/src/cli.ts index 284bb98..a9b2362 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -269,26 +269,56 @@ async function ensureServer(): Promise<{ port: number; hosts: string[] }> { function reviewUrl(base: string, id: string, view?: string): string { return `${base}/review/${id}${view ? `#/${view}` : ""}`; } +/** Two commits in a worktree, and the directory their graphs are cached under. */ +interface Pinned { + worktree: string; + pins: { base: string; head: string }; + dir: string; +} + +function pinnedOf(review: ReviewState): Pinned { + return { worktree: review.worktree, pins: review.pins, dir: reviewDir(review.id) }; +} + +/** + * Base and head as `--base` and `--head` name them. Head defaults to HEAD and + * base to where head forked from trunk, so a branch is diffed against what it + * branched from rather than against wherever trunk has moved since. + */ +async function pinRange( + worktree: string, + base: string | undefined, + head: string | undefined, + usage: string, +): Promise { + try { + const h = await g.revParse(worktree, head ?? "HEAD"); + const b = base + ? await g.revParse(worktree, base) + : await g.mergeBase(worktree, await g.trunkRef(worktree), h); + return { base: b, head: h }; + } catch (e) { + throw new AxiError((e as Error).message, "VALIDATION_ERROR", [ + `Pass resolvable refs: \`${usage}\``, + ]); + } +} + /** - * The interface delta at a review's pins. The graphs are cached per commit under - * the review directory, so publish and `graph interfaces` build them once between + * The interface delta at two pinned commits. The graphs are cached per commit + * under `at.dir`, so publish and `graph interfaces` build them once between * them; both modules load lazily to keep tree-sitter off every other command's path. */ -async function deltaFor( - review: ReviewState, - base?: CodeGraph, - head?: CodeGraph, -): Promise { +async function deltaFor(at: Pinned, base?: CodeGraph, head?: CodeGraph): Promise { const graph = await import("./graph.js"); const { interfaceDelta } = await import("./interfaces.js"); - const dir = reviewDir(review.id); - const b = base ?? (await graph.graphAt(review.worktree, review.pins.base, dir)); - const h = head ?? (await graph.graphAt(review.worktree, review.pins.head, dir)); - const changes = await g.lineChanges(review.worktree, review.pins.base, review.pins.head); - const changed = await g.changedFiles(review.worktree, review.pins.base, review.pins.head); + const b = base ?? (await graph.graphAt(at.worktree, at.pins.base, at.dir)); + const h = head ?? (await graph.graphAt(at.worktree, at.pins.head, at.dir)); + const changes = await g.lineChanges(at.worktree, at.pins.base, at.pins.head); + const changed = await g.changedFiles(at.worktree, at.pins.base, at.pins.head); return interfaceDelta({ - cwd: review.worktree, - pins: review.pins, + cwd: at.worktree, + pins: at.pins, base: b, head: h, impact: graph.impact(b, h, changes, 1), @@ -508,12 +538,18 @@ const SPECS: Record< args: "interfaces|impact|callers |tests-for |architecture", flags: { review: { kind: "string", help: "review id prefix" }, + base: { + kind: "string", + help: "instead of --review: base revision (default: trunk fork point)", + }, + head: { kind: "string", help: "instead of --review: head revision (default: HEAD)" }, graph: { kind: "string", help: "callers, tests-for: head or base", default: "head" }, depth: { kind: "string", help: "how many caller hops to follow", default: "2" }, }, examples: [ "thurview graph interfaces", "thurview graph impact", + "thurview graph impact --base main --head HEAD", "thurview graph callers login", "thurview graph tests-for login --graph base", "thurview graph architecture", @@ -662,16 +698,12 @@ const commands: Record Promise> = { b?.kind === "range" && !str(p, "base") && !str(p, "head") ? b.name.split("..") : [str(p, "base"), str(p, "head")]; - try { - head = await g.revParse(worktree, hh ?? "HEAD"); - base = bb - ? await g.revParse(worktree, bb) - : await g.mergeBase(worktree, await g.trunkRef(worktree), head); - } catch (e) { - throw new AxiError((e as Error).message, "VALIDATION_ERROR", [ - "Pass resolvable refs: `thurview scaffold --base --head `", - ]); - } + ({ base, head } = await pinRange( + worktree, + bb, + hh, + "thurview scaffold --base --head ", + )); binding = { kind: "range", name: `${base.slice(0, 12)}..${head.slice(0, 12)}` }; } else { const branch = b?.kind === "branch" ? b.name : await g.currentBranch(worktree); @@ -987,7 +1019,7 @@ const commands: Record Promise> = { let interfaces: InterfaceDelta | null = null; if (kind === "review") { try { - interfaces = await deltaFor(review); + interfaces = await deltaFor(pinnedOf(review)); } catch (e) { diags.push({ level: "warning", @@ -1327,9 +1359,29 @@ const commands: Record Promise> = { const side = str(p, "graph") ?? "head"; if (side !== "head" && side !== "base") throw new AxiError("--graph must be head or base", "VALIDATION_ERROR", help); + const baseRef = str(p, "base"); + const headRef = str(p, "head"); + const commits = baseRef !== undefined || headRef !== undefined; + if (commits && str(p, "review")) + throw new AxiError("pass --review or --base/--head, not both", "VALIDATION_ERROR", help); const graph = await import("./graph.js"); - const review = await resolveReview(str(p, "review")); - if (kindOf(review) === "explainer" && (sub === "interfaces" || sub === "impact")) + const review = commits ? null : await resolveReview(str(p, "review")); + let t: Pinned; + if (review) t = pinnedOf(review); + else { + const worktree = await worktreeOf(process.cwd()); + if (!worktree) + throw new AxiError("not inside a git repository", "VALIDATION_ERROR", [ + "Run inside the source worktree, or pass --review ", + ]); + // No review directory owns these graphs, and a commit's graph is the same + // whoever asks, so they share one cache under the thurview home. + const pins = await pinRange(worktree, baseRef, headRef, "thurview graph impact --base "); + t = { worktree, pins, dir: home() }; + } + // A next step has to name the same commits, or it answers about another change. + const again = review ? "" : ` --base ${short(t.pins.base)} --head ${short(t.pins.head)}`; + if (review && kindOf(review) === "explainer" && (sub === "interfaces" || sub === "impact")) throw new AxiError( `graph ${sub} compares two commits; an explainer is pinned to one`, "VALIDATION_ERROR", @@ -1338,10 +1390,9 @@ const commands: Record Promise> = { `Run \`thurview graph callers --review ${short(review.id)}\` to follow one symbol`, ], ); - const dir = reviewDir(review.id); - const at = (commit: string) => graph.graphAt(review.worktree, commit, dir); + const at = (commit: string) => graph.graphAt(t.worktree, commit, t.dir); if (sub === "callers" || sub === "tests-for") { - const g = await at(side === "base" ? review.pins.base : review.pins.head); + const g = await at(side === "base" ? t.pins.base : t.pins.head); const pins = { graph: side, commit: short(g.commit), @@ -1353,25 +1404,25 @@ const commands: Record Promise> = { ...pins, symbol: name, callers: graph.callers(g, name!), - help: [`Run \`thurview graph tests-for ${name}\` to see what exercises it`], + help: [`Run \`thurview graph tests-for ${name}${again}\` to see what exercises it`], }; return { ...pins, symbol: name, depth, tests: graph.testsFor(g, name!, depth), - help: [`Run \`thurview graph callers ${name}\` for every reference`], + help: [`Run \`thurview graph callers ${name}${again}\` for every reference`], }; } - const base = await at(review.pins.base); - const head = await at(review.pins.head); + const base = await at(t.pins.base); + const head = await at(t.pins.head); const pins = { base: short(base.commit), head: short(head.commit), languages: graph.LANGUAGES.join(","), }; if (sub === "interfaces") { - const delta = await deltaFor(review, base, head); + const delta = await deltaFor(t, base, head); return { ...pins, verdict: delta.verdict, @@ -1389,29 +1440,31 @@ const commands: Record Promise> = { unreadable: delta.unreadable, truncated: delta.truncated, help: [ - "Write one capability line per entry in data.yaml under `interfaces`, keyed by id", - "Run `thurview graph callers ` to see who a removed or changed interface reached", + ...(review + ? ["Write one capability line per entry in data.yaml under `interfaces`, keyed by id"] + : []), + `Run \`thurview graph callers ${again}\` to see who a removed or changed interface reached`, ], }; } if (sub === "impact") { - const changes = await g.lineChanges(review.worktree, review.pins.base, review.pins.head); + const changes = await g.lineChanges(t.worktree, t.pins.base, t.pins.head); return { ...pins, depth, ...graph.impact(base, head, changes, depth), help: [ - "Run `thurview graph callers ` to follow one symbol", - "Run `thurview graph architecture` for the module structure and its diff", + `Run \`thurview graph callers ${again}\` to follow one symbol`, + `Run \`thurview graph architecture${again}\` for the module structure and its diff`, ], }; } // An explainer is scoped to a path, so its structure is that path's, not the // repository's: the same bound the Coverage tab accounts for. - if (kindOf(review) === "explainer") { + if (review && kindOf(review) === "explainer") { const scope = review.binding.name; const g0 = scopeGraph(head, scope); - const allFiles = await g.listFiles(review.worktree, review.pins.head); + const allFiles = await g.listFiles(t.worktree, t.pins.head); const { diff: _diff, truncated: _truncated, ...rest } = graph.architecture(g0, g0); return { commit: short(head.commit), @@ -1924,7 +1977,7 @@ const commands: Record Promise> = { return { skill: installed, help: [ - "Invoke them as /thurview and /forge-review in Claude Code, or by name in other agents", + "Invoke them as /thurview and /review-fix in Claude Code, or by name in other agents", "Run `thurview setup hooks` for ambient context at session start", ], }; diff --git a/src/graph.ts b/src/graph.ts index 87f4b04..5fe1c60 100644 --- a/src/graph.ts +++ b/src/graph.ts @@ -335,11 +335,14 @@ export function callers(g: CodeGraph, name: string): Caller[] { } export interface Reach { + id: string; symbol: string; file: string; line: number; depth: number; via: string; + /** The line in `symbol` that references `via`. */ + at: number; } /** Symbols that transitively reference any of `roots`, up to `depth` hops, nearest first. */ @@ -356,7 +359,15 @@ export function reach(g: CodeGraph, roots: string[], depth: number): Reach[] { seen.add(e.from); next.push(e.from); const s = byId.get(e.from)!; - out.push({ symbol: s.name, file: s.file, line: s.line, depth: d, via: id }); + out.push({ + id: e.from, + symbol: s.name, + file: s.file, + line: s.line, + depth: d, + via: id, + at: e.at, + }); } frontier = next; } @@ -389,7 +400,11 @@ export interface Changed { export interface Impact { changed: Changed[]; edges: { added: string[]; removed: string[] }; - reach: Reach[]; + /** + * Code that depends on the change without being part of it - every changed + * symbol is a root, so none reappears here - and whether a test reaches it. + */ + reach: (Omit & { tested: boolean })[]; tests: { file: string; covers: string[] }[]; untested: string[]; unresolved: { base: number; head: number }; @@ -486,10 +501,32 @@ export function impact( } } const tested = new Set([...covers.values()].flatMap((s) => [...s])); + // What the tests exercise within `depth` calls, walking forward from every test + // symbol: a caller no test reaches is where a regression ships unnoticed. + const calls = new Map(); + for (const e of head.edges) { + const list = calls.get(e.from) ?? []; + list.push(e.to); + calls.set(e.from, list); + } + const exercised = new Set(); + let front = head.symbols.filter((s) => isTestFile(s.file)).map((s) => s.id); + for (let d = 1; d <= depth && front.length; d++) { + const next: string[] = []; + for (const id of front) + for (const to of calls.get(id) ?? []) + if (!exercised.has(to)) { + exercised.add(to); + next.push(to); + } + front = next; + } return { changed, edges: { added, removed }, - reach: r.filter((x) => !isTestFile(x.file)), + reach: r + .filter((x) => !isTestFile(x.file)) + .map(({ id, ...x }) => ({ ...x, tested: exercised.has(id) })), tests: [...covers] .map(([file, set]) => ({ file, covers: [...set].sort() })) .sort((a, b) => a.file.localeCompare(b.file)), diff --git a/test/e2e.test.ts b/test/e2e.test.ts index f16dd21..cbcda8b 100644 --- a/test/e2e.test.ts +++ b/test/e2e.test.ts @@ -264,6 +264,38 @@ describe("thurview end to end", () => { expect(badSide["code"]).toBe("VALIDATION_ERROR"); }); + it("answers on two commits without a review, pinned the way scaffold pins them", async () => { + const main = (await git("rev-parse", "main")).stdout.trim(); + const feature = (await git("rev-parse", "feature")).stdout.trim(); + const impact = await cli(["graph", "impact", "--base", "main", "--head", "feature"]); + expect(main.startsWith(impact["base"])).toBe(true); + expect(feature.startsWith(impact["head"])).toBe(true); + expect((impact["changed"] as Out[]).map((s) => `${s["symbol"]}:${s["change"]}`).sort()).toEqual( + ["audit:added", "login:modified"], + ); + // --head alone diffs from the trunk fork point + const forked = await cli(["graph", "impact", "--head", "feature"]); + expect(forked["base"]).toBe(impact["base"]); + const callers = await cli(["graph", "callers", "audit", "--base", "main", "--head", "feature"]); + expect(callers["callers"]).toEqual([{ symbol: "login", file: "src/auth.ts", line: 3, at: 4 }]); + const before = await cli(["graph", "callers", "check", "--head", "feature", "--graph", "base"]); + expect(before["callers"]).toEqual([{ symbol: "login", file: "src/auth.ts", line: 1, at: 2 }]); + const surface = await cli(["graph", "interfaces", "--base", "feature", "--head", "surface"]); + expect((surface["interfaces"] as Out[]).map((r) => `${r["change"]} ${r["id"]}`)).toEqual([ + "removed src/audit.ts:audit", + "changed src/auth.ts:login", + "added src/audit.ts:record", + ]); + // with no review there is no data.yaml to write, so no step may point at one + expect(String(surface["help"])).not.toMatch(/data\.yaml/); + const both = await cli(["graph", "impact", "--review", reviewId, "--base", "main"], { + expectCode: 2, + }); + expect(both["code"]).toBe("VALIDATION_ERROR"); + const unknown = await cli(["graph", "impact", "--base", "no-such-ref"], { expectCode: 2 }); + expect(unknown["code"]).toBe("VALIDATION_ERROR"); + }, 30_000); + it("rejects a document whose anchors do not resolve", async () => { expect(reviewDir).toBeTruthy(); await writeFile( diff --git a/test/graph.test.ts b/test/graph.test.ts index 03a5369..3bd14cb 100644 --- a/test/graph.test.ts +++ b/test/graph.test.ts @@ -135,6 +135,44 @@ describe("impact", () => { expect(byName.get("bar")).toBe("modified"); expect(result.truncated).toEqual({ base: false, head: false }); }); + + it("gives each caller the change left alone its call site and whether a test reaches it", async () => { + const { dir, git } = await repo(); + await mkdir(join(dir, "test")); + const caller = (fn: string, arg: string) => + `import { discount } from "./price";\n\nexport function ${fn}(total: number) {\n return discount(total, ${arg});\n}\n`; + await writeFile( + join(dir, "src", "price.ts"), + `export function discount(total: number, percent: number) {\n return total - (total * percent) / 100;\n}\n`, + ); + await writeFile(join(dir, "src", "cart.ts"), caller("checkout", "15")); + await writeFile(join(dir, "src", "invoice.ts"), caller("invoice", "5")); + await writeFile( + join(dir, "test", "invoice.test.ts"), + `import { invoice } from "../src/invoice";\n\nexport function invoicesFive() {\n return invoice(100) === 95;\n}\n`, + ); + await git("add", "."); + await git("commit", "-q", "-m", "base"); + const base = (await git("rev-parse", "HEAD")).stdout.trim(); + // discount takes a rate now, and neither caller outside the diff was told + await writeFile( + join(dir, "src", "price.ts"), + `export function discount(total: number, rate: number) {\n return total - total * rate;\n}\n`, + ); + await git("commit", "-q", "-am", "take a rate"); + const head = (await git("rev-parse", "HEAD")).stdout.trim(); + const result = impact( + await buildGraph(dir, base), + await buildGraph(dir, head), + await lineChanges(dir, base, head), + 2, + ); + const via = "src/price.ts:discount"; + expect([...result.reach].sort((a, b) => a.symbol.localeCompare(b.symbol))).toEqual([ + { symbol: "checkout", file: "src/cart.ts", line: 3, depth: 1, via, at: 4, tested: false }, + { symbol: "invoice", file: "src/invoice.ts", line: 3, depth: 1, via, at: 4, tested: true }, + ]); + }); }); describe("architecture", () => { diff --git a/test/skill-frontmatter.test.ts b/test/skill-frontmatter.test.ts index c543891..64e9c9e 100644 --- a/test/skill-frontmatter.test.ts +++ b/test/skill-frontmatter.test.ts @@ -33,6 +33,11 @@ describe("SKILL.md frontmatter", () => { expect(files).toContain("skills/thurview/SKILL.md"); }); + it("ships one review skill, the one that fixes what it finds", () => { + expect(files).toContain("skills/review-fix/SKILL.md"); + expect(files).not.toContain("skills/forge-review/SKILL.md"); + }); + it.each(files)("%s parses as YAML and names the skill", (path) => { const fm = loadFrontmatter(readFileSync(`${repoRoot}${path}`, "utf8")); expect(typeof fm["name"]).toBe("string");