diff --git a/README.md b/README.md index 69cb872..18862d1 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ The command fetches the pull request's metadata and full diff — the descriptio Each file's hunks are grouped into parts named after the entities they touch: hunks that share an entity form one part, such as `Cart.total in web/cart.ts`, the hunks outside every entity form a `top-level code in …` part, and a file whose entities could not be named keeps its path. The engine fails the run unless every changed line belongs to exactly one part. Each part gets plain signals: new versus changed code, test versus code, its changed lines, the public entities it adds, removes or redeclares, and how many other files in the head copy mention its entity names — a name-based count, labelled so, that cannot tell a call from a same-named word. A fixed rule ranks each part **must review**, **worth reviewing** or **context** with a one-line reason citing the signals it used, keeps at most a third of the parts (rounded up) at must review, and gives the same order for the same input. Noise parts sink to the bottom, except snapshots and fixtures, which are labelled but ranked with the rest. Read the order and the reasons, and compare the parts' hunks and line counts with the GitHub page: no hunk may be missing. -The engine also keeps a read-only copy of the base version (the merge base the diff is computed against) and the head version in a per-pull-request cache, at `/github.com/{owner}/{repo}/pull-{number}/{commit}`. The copies are downloaded as archives: nothing is checked out in the reviewer's workspace, nothing from the pull request runs, and no package manager is called. A later run at the same commits reuses them. The cache folder is `--cache-dir`, else `SECOND_LOOK_CACHE_DIR`, else the platform's per-user cache folder (`~/.cache/second-look`, `~/Library/Caches/second-look` or `%LOCALAPPDATA%\second-look\cache`); its files and folders are read-only but for the reviewed marks, so remove it with `chmod -R u+w` first. +The engine also keeps a read-only copy of the base version (the merge base the diff is computed against) and the head version in a per-pull-request cache, at `/github.com/{owner}/{repo}/pull-{number}/{commit}`. The copies are downloaded as archives: nothing is checked out in the reviewer's workspace, nothing from the pull request runs, and no package manager is called. A later run at the same commits reuses them. The cache folder is `--cache-dir`, else `SECOND_LOOK_CACHE_DIR`, else the platform's per-user cache folder (`~/.cache/second-look`, `~/Library/Caches/second-look` or `%LOCALAPPDATA%\second-look\cache`); its files and folders are read-only but for the reviewed marks and the record of the reviewer's last look, so remove it with `chmod -R u+w` first. Each changed file is parsed with a tree-sitter grammar bundled as WASM — Python, C#, TypeScript, TSX, JavaScript, Go, Rust and Java. Every hunk names the entities (functions, classes, methods and the like) its changed lines touch, each with whether it is public by its language's visibility rules and whether the hunk adds it, removes it, changes its declaration or only its body, and each part says whether its change is confirmed formatting-only: the base and head syntax trees must match, nesting included, so a Python dedent that moves a statement out of a block is not formatting-only. Files in other languages still flow through at file level, and their part lists the checks that could not run and why. The result records the time spent parsing in `parseTimeMs`. @@ -193,10 +193,10 @@ The extension adds a **Second Look: Review pull request** command and a review t 3. Sign in with VS Code's built-in GitHub login when it asks. 4. Read the tree: the parts grouped by importance in order — must review, worth reviewing, context — each named by the entities it touches, with its reason beside it and in its tooltip the signals the reason cites and whether the plain or the agent ranking is shown, and the noise last with its label and whether it was confirmed or only claimed. A part that arrives without a rank sits in its own plain section above the noise, and the tree shows whatever the engine returns. 5. Watch the tree arrive in stages: the plain parts show first, with a status line above the tree naming the stage still running ("grouping related hunks with pi"), then the tree updates in place when the agent's parts arrive, and the status line says who grouped them — the agent with its model and the grouping prompt's version, or the plain pass with the reason the agent's grouping was not used. The part you had selected stays selected: the part now holding its first hunk is selected in its place, and an open diff editor stays as it is. Try a pull request where a function, its caller and its test change in different files: the plain tree shows them apart, the regrouped tree as one part. The tree then updates again when the agent's ranking arrives ("ranking the parts with pi"), and the status line says who ranked the parts, or why the plain ranking stayed; then the story ("writing the story with pi"), then the comparison with the description and the linked issues ("comparing the change with its description and issues with pi"), each part neither explains showing the **? unexplained** badge beside its name with its one-line reason in its tooltip, then the claims ("listing the claims with pi"), each part they are attached to showing its claim count beside its name, then their verdicts ("checking the claims with pi"), and the acceptance criteria's verdicts last ("mapping the acceptance criteria with pi"), shown in the overview: each part with a finding — a refuted or unverifiable claim — shows a badge such as **⚠ 1 finding** beside its name, and each finding is the companion's own comment thread on the diff, beside any thread of the GitHub pull request extension, at the line that makes the claim — a pipeline finding's thread at the line it names when the change shows that line, else on its part's first added line — or, for a claim in the description or the story, the first line of the change its verdict cites, and on its part when its verdict cites none of the change. The thread gives the verdict, its evidence source, the quote, the reason, each citation and the library whose source the claim needs; it is read-only and nothing in it reaches GitHub. When the project pins that library with its hashes, the thread offers its library fetch with its reason, a **Fetch** link naming the library and the pinned version: press it and the engine downloads that exact file, checks its hash, unpacks it read-only and judges the claim again, and the thread updates with the new verdict, each citation a link that opens the library's file read-only at the cited line, from the same cache the agent read. For a .NET package without Source Link, the thread instead offers a **Decompile** link when that version's licence allows it, or says why it is not decompiled. Review the Python canary (`packages/evaluation/cases/canary-python`): its redirect claim offers a fetch of `httpx` 0.27.2 with its reason, and pressing it turns the claim refuted, citing `httpx/_client.py` at the pinned version. Review a pull request whose description misstates what a changed function does: the claim is refuted, with its thread at the changed line the verdict cites. Review a medium pull request and compare the agent ranking with the plain one: the agent's comes only with a tested agent, model and effort, and the engine's review command without `--agent` prints the plain ranking of the same pull request. -6. Read the overview, the **Second Look: #… overview** tab that opens with the review without taking the focus from the tree: the pull request's title, where it comes from, a chip for each stage done and the one still running, the story with its stamp — each part it mentions a link that opens the part in the diff editor — the acceptance criteria — each condition from the issues the pull request links, quoted from the checklist under the heading the `second-look.criteriaHeading` setting names ("Acceptance criteria" by default), its issue a link that opens it on GitHub, hidden content shown and flagged, and its verdict — **not checked** until the agent maps it, then **met**, **partly met**, **not met**, **can't tell** or **needs manual check**, with its reason, the code and the tests it cites, each a link that opens the line read-only in the head copy, and the manual checks the description reports, each its place a link that jumps to the description, with how many criteria have each verdict beside the heading — the claims with theirs, each quoted with where it is made, the part it is attached to as a link, and its verdict with its evidence source, reason and citations (**not checked** until judged), the pipeline and CI — whether the no-mistakes report is fresh or stale, its steps and open findings, then each check run on the merge commit with its annotations and a failed job's trimmed log — the pull request's description in full, and who made each result: the plain pass, or the agent with its model and prompt version. Content GitHub hides in the description is shown and flagged: an HTML comment as written, Unicode tag characters decoded to the text they spell, zero-width characters and bidirectional controls as their code points. Review a pull request whose description holds an HTML comment: the overview shows it flagged. Review a pull request that closes an issue with an acceptance-criteria checklist: the criteria appear in the overview, each with its issue and, once mapped, its verdict; a criterion the change leaves out is **not met**, and one the description reports a manual check for cites that check. The unexplained changes follow the criteria, in both directions: each part neither the description nor a linked issue explains, a link that opens it, with its reason, then each change they describe that the diff does not contain, quoted with where it is made — a description line, or its issue as a link that opens it on GitHub — and what the diff lacks; or why there is no comparison. Review a pull request that sneaks an unrelated refactor in beside its feature: that part carries the **? unexplained** badge in the tree. Review one whose description promises a test or a document the diff does not contain: the overview lists that statement as described, not in the code. Review a pull request with a no-mistakes report, then push a commit to it and review again: the report shows as stale, and its findings are no longer claims. Review one with a failing check: its trimmed log appears under the check. Nothing in the overview renders as markup: no remote image and no link, under a content security policy that loads nothing but the page's own style and script. Each part in the tree has a **Why this matters** button beside its reason: it opens the story at the sentence that first mentions the part, or says the story does not mention it. The tree's **Open overview** title button brings the overview back. -7. Click a part: the multi-file diff editor opens with exactly that part's files, the base copy on the left and the head copy on the right, scrolled to the part's first hunk with its added and deleted lines marked. Try to edit a file: the copies are read-only, so the editor refuses. Added, deleted, renamed and binary files open sensibly too. The tree's title button, **Open all parts in order**, opens the whole change in one multi-file diff in the tree's order, noise last. Tick a part's checkbox to mark it reviewed: the mark is kept in the pull request's own folder of the engine's cache, keyed by the part's content hash, so it survives restarts, and the tree view's badge counts the parts left. Push a change to one marked part and review again: only that part is unmarked, and it says **changed since you marked it**; a part whose lines only moved keeps its mark. With the `second-look.mirrorViewedToGitHub` setting on — off by default, because the GitHub Pull Requests extension syncs the same field — a file whose every part is marked is also marked **Viewed** on GitHub; a file only partly reviewed never is, and nothing is unmarked there. +6. Read the overview, the **Second Look: #… overview** tab that opens with the review without taking the focus from the tree: the pull request's title, where it comes from, the line saying which commit the reviewer's last look was at and what changed since (nothing on a first look), a chip for each stage done and the one still running, the story with its stamp — each part it mentions a link that opens the part in the diff editor — the acceptance criteria — each condition from the issues the pull request links, quoted from the checklist under the heading the `second-look.criteriaHeading` setting names ("Acceptance criteria" by default), its issue a link that opens it on GitHub, hidden content shown and flagged, and its verdict — **not checked** until the agent maps it, then **met**, **partly met**, **not met**, **can't tell** or **needs manual check**, with its reason, the code and the tests it cites, each a link that opens the line read-only in the head copy, and the manual checks the description reports, each its place a link that jumps to the description, with how many criteria have each verdict beside the heading — the claims with theirs, each quoted with where it is made, the part it is attached to as a link, and its verdict with its evidence source, reason and citations (**not checked** until judged), the pipeline and CI — whether the no-mistakes report is fresh or stale, its steps and open findings, then each check run on the merge commit with its annotations and a failed job's trimmed log — the pull request's description in full, and who made each result: the plain pass, or the agent with its model and prompt version. Content GitHub hides in the description is shown and flagged: an HTML comment as written, Unicode tag characters decoded to the text they spell, zero-width characters and bidirectional controls as their code points. Review a pull request whose description holds an HTML comment: the overview shows it flagged. Review a pull request that closes an issue with an acceptance-criteria checklist: the criteria appear in the overview, each with its issue and, once mapped, its verdict; a criterion the change leaves out is **not met**, and one the description reports a manual check for cites that check. The unexplained changes follow the criteria, in both directions: each part neither the description nor a linked issue explains, a link that opens it, with its reason, then each change they describe that the diff does not contain, quoted with where it is made — a description line, or its issue as a link that opens it on GitHub — and what the diff lacks; or why there is no comparison. Review a pull request that sneaks an unrelated refactor in beside its feature: that part carries the **? unexplained** badge in the tree. Review one whose description promises a test or a document the diff does not contain: the overview lists that statement as described, not in the code. Review a pull request with a no-mistakes report, then push a commit to it and review again: the report shows as stale, and its findings are no longer claims. Review one with a failing check: its trimmed log appears under the check. Nothing in the overview renders as markup: no remote image and no link, under a content security policy that loads nothing but the page's own style and script. Each part in the tree has a **Why this matters** button beside its reason: it opens the story at the sentence that first mentions the part, or says the story does not mention it. The tree's **Open overview** title button brings the overview back. +7. Click a part: the multi-file diff editor opens with exactly that part's files, the base copy on the left and the head copy on the right, scrolled to the part's first hunk with its added and deleted lines marked. Try to edit a file: the copies are read-only, so the editor refuses. Added, deleted, renamed and binary files open sensibly too. The tree's title button, **Open all parts in order**, opens the whole change in one multi-file diff in the tree's order, noise last. Tick a part's checkbox to mark it reviewed: the mark is kept in the pull request's own folder of the engine's cache, keyed by the part's content hash, so it survives restarts, and the tree view's badge counts the parts left. Push a change to one marked part and review again: only that part is unmarked, and it says **changed since you marked it**; a part whose lines only moved keeps its mark. With the `second-look.mirrorViewedToGitHub` setting on — off by default, because the GitHub Pull Requests extension syncs the same field — a file whose every part is marked is also marked **Viewed** on GitHub; a file only partly reviewed never is, and nothing is unmarked there. Each review records the head commit it opened at; review again after new commits and each part changed since your last look says **changed since your last look**, the line above the tree says which commit that look was at and how many parts changed, and the tree's filter button shows only those parts. The change at that commit is compared with this one, each against its own merge base, so after a rebase onto a newer master with one real edit only the edited part is flagged; when the change at that commit cannot be compared — its commit gone from GitHub or no longer related to the head — the line says so and every part counts as changed. With no local record, the last look is the commit of your last submitted GitHub review. 8. Write comments: click the comment icon on any line a hunk covers — on either side of the diff — and type into the thread, or right-click a part in the tree and choose **Comment on this part…**. Each comment joins the pending review, which gathers in its own section at the top of the tree with where every comment points. A comment can be discarded from its thread until it is sent; nothing reaches GitHub while it waits (ADR 0002). 9. Draft a comment from a finding: press **Draft comment** in a finding's thread on the diff, or beside a finding in the overview — a refuted or unverifiable claim, an unexplained part, a change the description or an issue describes that the diff does not contain, or a not met or partly met acceptance criterion. The agent writes a short draft from the finding and its evidence, citing where the evidence is. A draft from a claim opens in a thread of its own on the claim's line, when the diff shows that line, else on its part as a whole, and a draft from an unexplained part on that part: its text is open for editing, with **Add to review**, which adds it to the pending review as you edited it, and **Discard draft**. A draft from a finding on the whole pull request, a described change or a criterion, opens for editing in an input box: Enter adds it to the overall comment on the Send review page, and Escape discards it. Nothing is sent on its own. Draft a comment from a refuted claim, edit it, add it to the pending review, and send it with the next step. 10. Press **Submit review…** (the tree's rocket button, or the Command Palette): the **Send review** page opens with every pending comment together for one last pass — each with where it points, editable or droppable in place — beneath them the overall comment on the whole pull request and the choice of **Comment**, **Approve** or **Request changes**. Nothing reaches GitHub until the page's Submit button is pressed; then the GitHub sign-in is asked for at that moment, the engine maps every line and part comment to its position in the pull request's current diff, and one request submits them as one GitHub review pinned to the head commit it read. The review's link then shows, ready to open; a send that fails keeps every comment exactly where it was. -The extension starts the engine as a separate process and speaks JSON-RPC to it over stdio, starting with a version handshake. The engine sends the plain result in a `review/stage` notification naming the next stage and its deadline, then the grouped result in another while the agent ranks, then the ranked result in another while the agent writes the story, then the result with the story in another while the agent compares the change with its description and issues, then the result with the comparison in another while the agent lists the claims, then the result with the claims in another while the agent judges them, then the result with the verdicts in another while the agent maps the acceptance criteria, then answers the review request with the final result; a new review replaces one still running. Pressing a finding's library fetch sends a `fetchLibrary` request naming the pull request and the claim in the engine's latest review of it, and the engine answers with the result holding the claim's new verdict. Pressing **Draft comment** on a finding sends a `draftComment` request naming the pull request and the finding in the engine's latest review of it — no token, since nothing of it reaches GitHub — and the engine answers with the checked draft. Ticking or clearing a part's checkbox sends a `markReviewed` request naming the pull request, the part's identity — its name with its files and entity kinds — and the content hashes of its pieces — each hunk without its line numbers, or a file without hunks — and the engine keeps the marks in the pull request's cache folder, which `reviewedMarks` reads back with each review; with the mirror setting on, a `markViewed` request carries the token and the files whose every part is marked, and the engine marks only those of its latest review. The GitHub token comes from VS Code's authentication API, travels with each review request, and is never stored by the companion. The agent, model, account and acceptance-criteria heading the settings choose travel with each review request too, so switching them needs no engine restart: the next review runs its agent passes on the chosen agent, and every agent-produced result is stamped with it. The review's one write — submitting it — asks for its token the same way, only at the moment the reviewer presses send, and the engine performs it as a single request: nothing of the review reaches GitHub before that. Progress shows in the tree while the engine works, and an engine failure reads as a plain message. The extension declares limited support for untrusted workspaces and runs nothing from the workspace: the engine is started from the extension's own install, reads GitHub, and writes only the review the reviewer sends — and, only with the opt-in mirror setting on, the **Viewed** mark of each file whose every part they reviewed. The diff editor reads the base and head content through the companion's own read-only file system (`second-look-change:` URIs) straight from the engine's cache — nothing is checked out, and every write is refused; a fetched library's files open the same way, from the pull request's library cache. +The extension starts the engine as a separate process and speaks JSON-RPC to it over stdio, starting with a version handshake. The engine sends the plain result in a `review/stage` notification naming the next stage and its deadline, then the grouped result in another while the agent ranks, then the ranked result in another while the agent writes the story, then the result with the story in another while the agent compares the change with its description and issues, then the result with the comparison in another while the agent lists the claims, then the result with the claims in another while the agent judges them, then the result with the verdicts in another while the agent maps the acceptance criteria, then answers the review request with the final result; a new review replaces one still running. Pressing a finding's library fetch sends a `fetchLibrary` request naming the pull request and the claim in the engine's latest review of it, and the engine answers with the result holding the claim's new verdict. Pressing **Draft comment** on a finding sends a `draftComment` request naming the pull request and the finding in the engine's latest review of it — no token, since nothing of it reaches GitHub — and the engine answers with the checked draft. Ticking or clearing a part's checkbox sends a `markReviewed` request naming the pull request, the part's identity — its name with its files and entity kinds — and the content hashes of its pieces — each hunk without its line numbers, or a file without hunks — and the engine keeps the marks in the pull request's cache folder, which `reviewedMarks` reads back with each review; each review compares the change with the reviewer's last look, recorded in the same folder, and its result carries what changed since (`sinceLastLook`); with the mirror setting on, a `markViewed` request carries the token and the files whose every part is marked, and the engine marks only those of its latest review. The GitHub token comes from VS Code's authentication API, travels with each review request, and is never stored by the companion. The agent, model, account and acceptance-criteria heading the settings choose travel with each review request too, so switching them needs no engine restart: the next review runs its agent passes on the chosen agent, and every agent-produced result is stamped with it. The review's one write — submitting it — asks for its token the same way, only at the moment the reviewer presses send, and the engine performs it as a single request: nothing of the review reaches GitHub before that. Progress shows in the tree while the engine works, and an engine failure reads as a plain message. The extension declares limited support for untrusted workspaces and runs nothing from the workspace: the engine is started from the extension's own install, reads GitHub, and writes only the review the reviewer sends — and, only with the opt-in mirror setting on, the **Viewed** mark of each file whose every part they reviewed. The diff editor reads the base and head content through the companion's own read-only file system (`second-look-change:` URIs) straight from the engine's cache — nothing is checked out, and every write is refused; a fetched library's files open the same way, from the pull request's library cache. diff --git a/packages/engine/src/github.ts b/packages/engine/src/github.ts index c17b1ca..c5595b3 100644 --- a/packages/engine/src/github.ts +++ b/packages/engine/src/github.ts @@ -224,6 +224,47 @@ export class GitHubClient { return data.merge_base_commit.sha; } + /** + * Fetches the diff a head commit makes against its merge base with a + * base commit, the way the pull request's own diff is taken; null when + * GitHub cannot serve the comparison: it no longer has either commit, + * such as one a force-push left behind, or the two share no history. + */ + async getChangeDiff(ref: PullRequestRef, base: string, head: string): Promise { + let response; + try { + response = await this.octokit.repos.compareCommitsWithBasehead({ + owner: ref.owner, + repo: ref.repo, + basehead: `${base}...${head}`, + mediaType: { format: 'diff' }, + }); + } catch (error) { + if (isNotFound(error) || (error as { status?: unknown }).status === 422) return null; + throw error; + } + return diffText(response.data); + } + + /** + * The commit and time of the signed-in reviewer's last submitted review + * of the pull request; null when they have submitted none. A pending + * review is not submitted, so it never counts. + */ + async getLastReviewedCommit(ref: PullRequestRef): Promise<{ commit: string; at: string } | null> { + const { data: user } = await this.octokit.users.getAuthenticated(); + const reviews = await this.octokit.paginate(this.octokit.pulls.listReviews, { + owner: ref.owner, + repo: ref.repo, + pull_number: ref.number, + per_page: 100, + }); + const last = reviews + .filter((review) => review.user?.login === user.login && review.state !== 'PENDING' && review.commit_id && review.submitted_at) + .at(-1); + return last === undefined ? null : { commit: last.commit_id!, at: last.submitted_at! }; + } + /** * Streams the gzipped tarball of one commit. The archive is downloaded, * never checked out, and the body is handed over unparsed so a large @@ -255,14 +296,7 @@ export class GitHubClient { pull_number: ref.number, mediaType: { format: 'diff' }, }); - // The diff media type is not JSON; the client hands over raw bytes. - if (typeof response.data === 'string') { - return response.data; - } - if (response.data instanceof ArrayBuffer) { - return new TextDecoder().decode(response.data); - } - return String(response.data); + return diffText(response.data); } /** @@ -493,6 +527,13 @@ export interface CheckRunListing { app: string | null; } +/** A diff as text: the diff media type is not JSON, so the client hands over raw bytes. */ +function diffText(data: unknown): string { + if (typeof data === 'string') return data; + if (data instanceof ArrayBuffer) return new TextDecoder().decode(data); + return String(data); +} + /** True when the error is the endpoint's plain 404. */ function isNotFound(error: unknown): boolean { return ( diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index a628cf6..d2cf4b4 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -48,4 +48,5 @@ export * from './criteria.js'; export * from './criteria-mapping.js'; export * from './draft-comment.js'; export * from './reviewed-marks.js'; +export * from './last-look.js'; export { runCli } from './cli.js'; diff --git a/packages/engine/src/last-look.ts b/packages/engine/src/last-look.ts new file mode 100644 index 0000000..a22baf9 --- /dev/null +++ b/packages/engine/src/last-look.ts @@ -0,0 +1,158 @@ +import { randomBytes } from 'node:crypto'; +import { mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { pullRequestCacheDir } from './cache.js'; +import { parseDiff } from './diff.js'; +import type { GitHubClient, PullRequestRef } from './github.js'; +import type { Hunk, Part, PullRequestSummary, SinceLastLook } from './protocol.js'; +import { lineContent, pieceHashes } from './reviewed-marks.js'; + +/** The file in a pull request's cache folder that records the reviewer's looks. */ +export const LAST_LOOK_FILE = 'last-look.json'; + +/** One look: the head commit a review was opened at, and when. */ +export interface Look { + commit: string; + /** When the review was opened, as an ISO 8601 timestamp. */ + at: string; +} + +/** + * The store's record: the latest look, and the look before it at another + * commit, so opening the review again at the same commit still shows + * what changed since the look before. + */ +interface LookRecord { + version: 1; + last: Look; + before?: Look; +} + +const COMMIT = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/; + +function isLook(value: unknown): value is Look { + if (typeof value !== 'object' || value === null) return false; + const { commit, at } = value as Record; + return typeof commit === 'string' && COMMIT.test(commit) && typeof at === 'string'; +} + +function lookPath(cacheDir: string, ref: PullRequestRef): string { + return join(pullRequestCacheDir(cacheDir, ref), LAST_LOOK_FILE); +} + +/** + * Reads the record of the reviewer's looks at a pull request; null when + * none was written yet or it is not a record, which is then left out + * rather than trusted. + */ +export async function readLooks(cacheDir: string, ref: PullRequestRef): Promise { + let text: string; + try { + text = await readFile(lookPath(cacheDir, ref), 'utf8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; + throw error; + } + let stored: unknown; + try { + stored = JSON.parse(text); + } catch { + return null; + } + const { last, before } = (stored ?? {}) as Partial; + if (!isLook(last)) return null; + return { version: 1, last, ...(isLook(before) && before.commit !== last.commit ? { before } : {}) }; +} + +/** + * Records a look at a pull request: it becomes the latest, and the + * latest before it becomes the look before when it was at another + * commit. The record lands whole: written beside the store, then renamed + * over it. + */ +export async function recordLook(cacheDir: string, ref: PullRequestRef, look: Look): Promise { + const previous = await readLooks(cacheDir, ref); + const before = previous === null ? undefined : previous.last.commit === look.commit ? previous.before : previous.last; + const record: LookRecord = { version: 1, last: look, ...(before === undefined ? {} : { before }) }; + const path = lookPath(cacheDir, ref); + await mkdir(pullRequestCacheDir(cacheDir, ref), { recursive: true }); + const partial = `${path}.partial-${randomBytes(6).toString('hex')}`; + try { + await writeFile(partial, `${JSON.stringify(record, null, 2)}\n`, 'utf8'); + await rename(partial, path); + } catch (error) { + await rm(partial, { force: true }); + throw error; + } +} + +/** A hunk's changed lines alone, so context that upstream changed around them leaves the piece alone. */ +function changedLines(hunk: Hunk): string[] { + return hunk.lines.filter((line) => line.kind !== 'context').map(lineContent); +} + +/** + * The content hashes of a part's pieces as the comparison with the last + * look reads them: each hunk by its changed lines alone, without their + * numbers or context, and each file without hunks by its blob ids. + * Identical hunks share a hash, so a piece reads the same whichever part + * of its file holds it. + */ +export function changePieces(part: Part): string[] { + return pieceHashes(part, changedLines, false); +} + +/** Whether a part changed since the reviewer's last look: every part did when the changes could not be compared. */ +export function changedSinceLastLook(part: Part, since: SinceLastLook): boolean { + if (since.outcome === 'not compared') return true; + const changed = new Set(since.changed); + return changePieces(part).some((piece) => changed.has(piece)); +} + +/** What the comparison needs: the GitHub client, the cache folder and the pull request as it is now. */ +export interface LastLookOptions { + client: GitHubClient; + cacheDir: string; + ref: PullRequestRef; + pullRequest: PullRequestSummary; + /** The pull request's full diff now. */ + diff: string; + /** When this look happens. */ + now?: Date; +} + +/** + * Compares the change with the reviewer's last look, then records this + * look. The last look is the latest one the local record holds at + * another commit, or, with none, the commit of the reviewer's last + * submitted GitHub review; with neither, this is their first look and + * there is nothing to compare. The change at that commit is taken + * against its merge base with the base now — the way the pull request's + * own diff is — so after a rebase or a force-push the two changes are + * compared themselves, and upstream commits mix in nothing. When the + * change at that commit cannot be fetched — its commit gone from + * GitHub, or sharing no history with the head — every part counts as + * changed. + */ +export async function lookSinceLastLook(options: LastLookOptions): Promise { + const { client, cacheDir, ref, pullRequest } = options; + const head = pullRequest.headSha; + const looks = await readLooks(cacheDir, ref); + const local = looks === null ? undefined : looks.last.commit !== head ? looks.last : looks.before; + const reviewed = local === undefined ? await client.getLastReviewedCommit(ref) : null; + const last = local ?? reviewed ?? undefined; + const since = last === undefined ? undefined : await compareWith(last, local === undefined ? 'github review' : 'local record', options); + await recordLook(cacheDir, ref, { commit: head, at: (options.now ?? new Date()).toISOString() }); + return since; +} + +async function compareWith(last: Look, from: SinceLastLook['from'], options: LastLookOptions): Promise { + const { client, ref, pullRequest } = options; + const look = { commit: last.commit, from, at: last.at }; + if (last.commit === pullRequest.headSha) return { ...look, outcome: 'compared', changed: [] }; + const before = await client.getChangeDiff(ref, pullRequest.baseCommit, last.commit); + if (before === null) return { ...look, outcome: 'not compared', changed: [] }; + const held = new Set(parseDiff(before).files.flatMap(changePieces)); + const now = parseDiff(options.diff).files.flatMap(changePieces); + return { ...look, outcome: 'compared', changed: [...new Set(now.filter((piece) => !held.has(piece)))] }; +} diff --git a/packages/engine/src/protocol.ts b/packages/engine/src/protocol.ts index a948ac5..d7f10f5 100644 --- a/packages/engine/src/protocol.ts +++ b/packages/engine/src/protocol.ts @@ -10,7 +10,7 @@ import type { AgentStamp } from './agent.js'; /** Version of the review result schema. */ -export const REVIEW_RESULT_VERSION = 15 as const; +export const REVIEW_RESULT_VERSION = 16 as const; /** * Version 2 added the head commit's SHA and each part's noise assessment; @@ -40,7 +40,8 @@ export const REVIEW_RESULT_VERSION = 15 as const; * that the diff does not contain; version 15 added each acceptance * criterion's verdict — met, partly met, not met, can't tell or needs * manual check — with the code, the tests and the manual checks that - * show it, and the criteria's mapping. + * show it, and the criteria's mapping; version 16 added what changed + * since the reviewer's last look. */ export type ReviewResultVersion = typeof REVIEW_RESULT_VERSION; @@ -215,6 +216,32 @@ export interface ViewedFiles { paths: string[]; } +/** + * What changed since the reviewer's last look: the head commit they last + * opened a review at, or, with no local record of one, the commit of + * their last submitted GitHub review, and the pieces of this change the + * change at that commit did not hold. The two changes are compared + * themselves, each against its own merge base, so a rebase or a + * force-push mixes in nothing from upstream. + */ +export interface SinceLastLook { + /** The head commit at the last look. */ + commit: string; + /** Where the last look comes from: the local record of the reviews opened, or the reviewer's last submitted GitHub review. */ + from: 'local record' | 'github review'; + /** When the last look was, as an ISO 8601 timestamp. */ + at: string; + /** + * **compared** when the change at that commit was compared with this + * one; **not compared** when it could not be — that commit gone from + * GitHub, or no longer related to the head — so every part counts as + * changed. + */ + outcome: 'compared' | 'not compared'; + /** The content hashes of this change's pieces — each hunk's changed lines, or a file without hunks — the change at the last look did not hold; empty when the changes could not be compared. */ + changed: string[]; +} + /** The review result the engine produces for one pull request. */ export interface ReviewResult { /** Schema version; compare against {@link REVIEW_RESULT_VERSION}. */ @@ -255,6 +282,8 @@ export interface ReviewResult { pipeline: PipelineReport; /** The CI the companion read at the head commit; absent when the review read none, such as an offline replay. */ ci?: CiResults; + /** What changed since the reviewer's last look; absent on their first look, or when the review was not opened by them, such as an offline replay. */ + sinceLastLook?: SinceLastLook; } /** diff --git a/packages/engine/src/review.ts b/packages/engine/src/review.ts index dca9164..095468c 100644 --- a/packages/engine/src/review.ts +++ b/packages/engine/src/review.ts @@ -13,13 +13,14 @@ import { validateCoverage } from './coverage.js'; import { parseDiff, type ParsedDiff } from './diff.js'; import { GitHubClient, parsePullRequestUrl } from './github.js'; import { groupingItems, groupWithAgent } from './grouping.js'; +import { lookSinceLastLook } from './last-look.js'; import { offerLibraryFetches } from './library-fetch.js'; import { confirmLockfileNoise } from './lockfile.js'; import { applyNoiseRules } from './noise.js'; import { groupParts } from './parts.js'; import { pipelineClaims, readPipelineReport } from './pipeline.js'; import { REVIEW_RESULT_VERSION } from './protocol.js'; -import type { ChangeCopies, CiResults, Criteria, Part, PullRequestSummary, ReviewResult } from './protocol.js'; +import type { ChangeCopies, CiResults, Criteria, Part, PullRequestSummary, ReviewResult, SinceLastLook } from './protocol.js'; import { rankParts } from './rank.js'; import { RANKING_PROMPT_VERSION, @@ -51,6 +52,8 @@ export interface ReviewOptions { criteriaHeading?: string; /** Asks the agent to group and rank the parts, write the story, compare the change with its description and issues, list the claims and judge them, and map the acceptance criteria too, after the plain pass; see {@link reviewChange}. */ agentStage?: AgentStageOptions; + /** True when the reviewer opened this review: it is compared with their last look, then recorded as the latest; see {@link lookSinceLastLook}. */ + lastLook?: boolean; } /** @@ -71,6 +74,8 @@ export interface ReviewInput { ci?: CiResults; /** The acceptance criteria of the linked issues; absent when none were read, such as an offline replay. */ criteria?: Criteria; + /** What changed since the reviewer's last look; absent on their first look, or when none was asked for. */ + sinceLastLook?: SinceLastLook; } /** @@ -96,7 +101,8 @@ export async function reviewPullRequest( * checkout), read-only copies of the base and head versions, the CI at * the head commit — each failed job's log trimmed to its failing step — * and the acceptance criteria of the issues the pull request links, - * quoted from the checklist under the configured heading. + * quoted from the checklist under the configured heading; and, when the + * reviewer opened the review, what changed since their last look. */ export async function fetchChange(url: string, options: ReviewOptions): Promise { const ref = parsePullRequestUrl(url); @@ -126,13 +132,14 @@ export async function fetchChange(url: string, options: ReviewOptions): Promise< download: (wanted) => client.downloadTarball(ref, wanted), }); const heading = options.criteriaHeading?.trim(); - const [base, head, ci, criteria] = await Promise.all([ + const [base, head, ci, criteria, sinceLastLook] = await Promise.all([ copy(mergeBase), copy(pullRequest.headSha), readCi(client, ref, pullRequest.headSha, mergeCommit), readCriteria(client, ref, pullRequest.base, heading === undefined || heading === '' ? DEFAULT_CRITERIA_HEADING : heading), + options.lastLook ? lookSinceLastLook({ client, cacheDir: options.cacheDir, ref, pullRequest, diff }) : undefined, ]); - return { pullRequest, diff, gitAttributes, copies: { base, head }, ci, criteria }; + return { pullRequest, diff, gitAttributes, copies: { base, head }, ci, criteria, ...(sinceLastLook ? { sinceLastLook } : {}) }; } /** @@ -228,6 +235,7 @@ export async function reviewChange( ...(input.criteria ? { criteria: input.criteria } : {}), pipeline: readPipelineReport(input.pullRequest.description, input.pullRequest.headSha), ...(input.ci ? { ci: input.ci } : {}), + ...(input.sinceLastLook ? { sinceLastLook: input.sinceLastLook } : {}), }; if (!agentStage) return plain; const ranked = await groupAndRank(plain, agentStage, input, parsed, files); diff --git a/packages/engine/src/reviewed-marks.ts b/packages/engine/src/reviewed-marks.ts index ba338ff..7b94f3f 100644 --- a/packages/engine/src/reviewed-marks.ts +++ b/packages/engine/src/reviewed-marks.ts @@ -4,7 +4,7 @@ import { join } from 'node:path'; import { pullRequestCacheDir } from './cache.js'; import type { PullRequestRef } from './github.js'; import { entityKey, filesOfPart } from './parts.js'; -import type { FileSlice, Hunk, Part, ReviewedMark, ReviewedMarks, ReviewedState } from './protocol.js'; +import type { DiffLine, FileSlice, Hunk, Part, ReviewedMark, ReviewedMarks, ReviewedState } from './protocol.js'; /** The file in a pull request's cache folder that holds its reviewed marks. */ export const REVIEWED_MARKS_FILE = 'reviewed-marks.json'; @@ -23,10 +23,16 @@ function fileIdentity(file: FileSlice): unknown[] { return [file.path, file.previousPath ?? null, file.changeKind, file.oldMode ?? null, file.newMode ?? null]; } +const LINE_PREFIX = { context: ' ', addition: '+', deletion: '-' } as const; + +/** One diff line as a piece's content holds it: its kind and text, without its numbers. */ +export function lineContent(line: DiffLine): string { + return `${LINE_PREFIX[line.kind]}${line.text}${line.endsWithoutNewline ? '\n\\' : ''}`; +} + /** A hunk's content without its line numbers, so an edit elsewhere in the file leaves it alone. */ function hunkContent(hunk: Hunk): string[] { - const prefix = { context: ' ', addition: '+', deletion: '-' } as const; - return hunk.lines.map((line) => `${prefix[line.kind]}${line.text}${line.endsWithoutNewline ? '\n\\' : ''}`); + return hunk.lines.map(lineContent); } /** @@ -37,6 +43,16 @@ function hunkContent(hunk: Hunk): string[] { * file are told apart by how many came before. */ export function partPieces(part: Part): string[] { + return pieceHashes(part, hunkContent); +} + +/** + * The content hashes of a part's pieces, each hunk's content read by the + * given function: one per hunk, and one per file without hunks. Identical + * hunks of one file are told apart by how many came before, unless + * `apart` is false. + */ +export function pieceHashes(part: Part, hunkLines: (hunk: Hunk) => string[], apart = true): string[] { return filesOfPart(part).flatMap((file) => { const identity = fileIdentity(file); if (file.hunks.length === 0) { @@ -44,7 +60,8 @@ export function partPieces(part: Part): string[] { } const seen = new Map(); return file.hunks.map((hunk) => { - const content = JSON.stringify([...identity, hunkContent(hunk)]); + const content = JSON.stringify([...identity, hunkLines(hunk)]); + if (!apart) return sha256(content); const occurrence = seen.get(content) ?? 0; seen.set(content, occurrence + 1); return sha256(`${content}#${occurrence}`); diff --git a/packages/engine/src/server.ts b/packages/engine/src/server.ts index 1cd391b..763032a 100644 --- a/packages/engine/src/server.ts +++ b/packages/engine/src/server.ts @@ -104,7 +104,9 @@ export interface RpcServerDeps { * is refused with a plain message. `review` then carries the pull request * URL, the GitHub token and — when the client's settings chose one — the * agent, model and account that run the review's agent passes; a request - * without a choice runs the engine's serve-time default. `sendReview` + * without a choice runs the engine's serve-time default. Each review + * compares the change with the reviewer's last look and records this one + * in the pull request's local store. `sendReview` * carries the pending review the companion gathered — the protocol's one * write — submitted as one GitHub review when the reviewer presses send. * The token arrives with each request, is used only for that request's @@ -323,6 +325,7 @@ async function review( const result = await reviewPullRequest(url, { token, cacheDir: deps.cacheDir, + lastLook: true, ...(criteriaHeading !== undefined ? { criteriaHeading } : {}), ...(deps.fetch ? { fetch: deps.fetch } : {}), ...(deps.agent diff --git a/packages/engine/test/cli.test.ts b/packages/engine/test/cli.test.ts index 8551d17..fb424ee 100644 --- a/packages/engine/test/cli.test.ts +++ b/packages/engine/test/cli.test.ts @@ -39,7 +39,7 @@ describe('runCli review', () => { expect(code).toBe(0); expect(err.text).toBe(''); const result = JSON.parse(out.text) as { version: number; parts: unknown[] }; - expect(result.version).toBe(15); + expect(result.version).toBe(16); expect(result.parts).toHaveLength(11); }); @@ -56,7 +56,7 @@ describe('runCli review', () => { version: number; copies: { head: { path: string } }; }; - expect(result.version).toBe(15); + expect(result.version).toBe(16); expect(result.copies.head.path.startsWith(cacheDir)).toBe(true); }); diff --git a/packages/engine/test/helpers.ts b/packages/engine/test/helpers.ts index 7a08d09..af370aa 100644 --- a/packages/engine/test/helpers.ts +++ b/packages/engine/test/helpers.ts @@ -251,8 +251,9 @@ function recordedBody(init: RequestInit | undefined): unknown { * stored at the head commit, the merge base from the compare endpoint, * the linked issues from the one GraphQL query the review makes, archives * of both versions, the check runs at the head commit with their - * annotations and the failed job's log when the fixture records CI, and - * one submitted review for a send. Any + * annotations and the failed job's log when the fixture records CI, the + * signed-in reviewer with no review submitted yet, a 404 for the change + * at any earlier commit, and one submitted review for a send. Any * other URL throws, so a test can never touch the live network by * accident. */ @@ -301,6 +302,9 @@ export function fixtureFetch(pull: PullFixture = pull42()): FixtureTransport { const body = pull.issues === undefined ? NO_LINKED_ISSUES : JSON.parse(fixtureText(pull.issues)); return Response.json(body); } + // The reviewer has submitted no review, so a first look compares with nothing. + if (url === 'https://api.github.com/user') return Response.json({ login: 'reviewer' }); + if (url === `${API}/pulls/${pull.number}/reviews?per_page=100`) return Response.json([]); if (url === `${API}/pulls/${pull.number}/reviews`) { if ((init?.method ?? 'GET') !== 'POST') { throw new Error(`unexpected ${init?.method ?? 'GET'} to ${url}: sending is one POST`); @@ -334,6 +338,10 @@ export function fixtureFetch(pull: PullFixture = pull42()): FixtureTransport { if (url === `${API}/compare/${meta.base.sha}...${pull.headSha}?per_page=1`) { return Response.json({ merge_base_commit: { sha: pull.mergeBase } }); } + // The change at any earlier commit: GitHub no longer has it. + if (new RegExp(`^${API}/compare/[0-9a-f]+\\.\\.\\.[0-9a-f]+$`).test(url)) { + return Response.json({ message: 'Not Found' }, { status: 404 }); + } const tarballMatch = /\/tarball\/([0-9a-f]+)$/.exec(url); if (url.startsWith(`${API}/tarball/`) && tarballMatch) { const commit = tarballMatch[1]!; diff --git a/packages/engine/test/last-look.test.ts b/packages/engine/test/last-look.test.ts new file mode 100644 index 0000000..964ce9b --- /dev/null +++ b/packages/engine/test/last-look.test.ts @@ -0,0 +1,284 @@ +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { readFile, writeFile } from 'node:fs/promises'; +import { devNull, tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { pullRequestCacheDir, removeCopy } from '../src/cache.js'; +import { parseDiff } from '../src/diff.js'; +import { GitHubClient, type PullRequestRef } from '../src/github.js'; +import { LAST_LOOK_FILE, changePieces, changedSinceLastLook, lookSinceLastLook, readLooks, recordLook } from '../src/last-look.js'; +import type { Part, PullRequestSummary, SinceLastLook } from '../src/protocol.js'; +import { temporaryCacheDir } from './helpers.js'; + +const REF: PullRequestRef = { owner: 'example-org', repo: 'example-repo', number: 42 }; +const API = 'https://api.github.com/repos/example-org/example-repo'; +const THEN = new Date('2026-10-01T09:00:00Z'); +const NOW = new Date('2026-10-07T09:00:00Z'); + +/** Forty numbered lines, so edits far apart land in separate hunks. */ +const LINES = Array.from({ length: 40 }, (_, index) => `line ${index + 1}`); + +/** + * A local git repository standing in for the pull request's repository: + * the tests build the pushes, rebases and force-pushes with real git, and + * the fake GitHub below serves its compare diffs from it. Nothing leaves + * the machine, and no global git configuration is read. + */ +class Repository { + readonly dir = mkdtempSync(join(tmpdir(), 'second-look-git-')); + + constructor() { + this.git('init', '--quiet', '--initial-branch=master'); + } + + git(...args: string[]): string { + return execFileSync( + 'git', + ['-c', 'user.name=Reviewer', '-c', 'user.email=reviewer@example.com', '-c', 'commit.gpgsign=false', '-c', 'core.autocrlf=false', ...args], + { cwd: this.dir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, GIT_CONFIG_GLOBAL: devNull, GIT_CONFIG_NOSYSTEM: '1' } }, + ); + } + + /** Writes the files, commits them and answers with the new commit. */ + commit(files: Record, message: string): string { + for (const [path, content] of Object.entries(files)) writeFileSync(join(this.dir, path), content); + this.git('add', '--all'); + this.git('commit', '--quiet', '--no-verify', '-m', message); + return this.sha('HEAD'); + } + + sha(rev: string): string { + return this.git('rev-parse', rev).trim(); + } + + /** The diff GitHub's compare endpoint serves for `base...head`; null when a commit is missing. */ + compare(base: string, head: string): string | null { + try { + this.git('cat-file', '-e', `${base}^{commit}`); + this.git('cat-file', '-e', `${head}^{commit}`); + } catch { + return null; + } + return this.git('diff', `${base}...${head}`); + } + + remove(): void { + rmSync(this.dir, { recursive: true, force: true }); + } +} + +/** The file with the given numbered lines replaced. */ +function edited(edits: Record): string { + return `${LINES.map((line, index) => edits[index + 1] ?? line).join('\n')}\n`; +} + +/** + * A fetch that plays GitHub for the repository: the compare endpoint's + * diff from local git, 404 for a commit it does not have, the signed-in + * reviewer, and their reviews of the pull request. + */ +function gitHubOf(repository: Repository, reviews: unknown[] = []): typeof fetch { + return async (input) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url; + const compare = new RegExp(`^${API}/compare/([0-9a-f]+)\\.\\.\\.([0-9a-f]+)$`).exec(url); + if (compare) { + const diff = repository.compare(compare[1]!, compare[2]!); + if (diff === null) return Response.json({ message: 'Not Found' }, { status: 404 }); + return new Response(diff, { status: 200, headers: { 'content-type': 'application/vnd.github.v3.diff' } }); + } + if (url === 'https://api.github.com/user') return Response.json({ login: 'reviewer' }); + if (url === `${API}/pulls/42/reviews?per_page=100`) return Response.json(reviews); + throw new Error(`unexpected request to ${url}: tests run against the local repository only`); + }; +} + +let cacheDir: string; +let repository: Repository; + +beforeEach(() => { + cacheDir = temporaryCacheDir(); + repository = new Repository(); +}); + +afterEach(async () => { + await removeCopy(cacheDir); + repository.remove(); +}); + +/** The pull request as GitHub would show it with the branch at its current commit, against master. */ +function pullRequestNow(): PullRequestSummary { + return { + url: 'https://github.com/example-org/example-repo/pull/42', + number: 42, + title: 'Edit the lines', + author: 'author', + description: '', + base: 'master', + head: 'feature', + baseCommit: repository.sha('master'), + headSha: repository.sha('feature'), + }; +} + +/** Opens the review now: compares with the last look and records this one. */ +async function openReview(reviews: unknown[] = [], fetch: typeof globalThis.fetch = gitHubOf(repository, reviews)): Promise<{ since: SinceLastLook | undefined; parts: Part[] }> { + const pullRequest = pullRequestNow(); + const diff = repository.compare(pullRequest.baseCommit, pullRequest.headSha)!; + const client = new GitHubClient({ token: 'test-token', fetch }); + const since = await lookSinceLastLook({ client, cacheDir, ref: REF, pullRequest, diff, now: NOW }); + // One part per hunk, so each edit shows on its own. + const parts = parseDiff(diff).files.flatMap((file) => (file.hunks.length === 0 ? [file] : file.hunks.map((hunk) => ({ ...file, hunks: [hunk] })))); + return { since, parts }; +} + +/** The changed parts, each by its file and its first new line. */ +function flagged(parts: Part[], since: SinceLastLook): string[] { + return parts.filter((part) => changedSinceLastLook(part, since)).map((part) => `${part.path}:${part.hunks[0]?.newStart ?? 0}`); +} + +/** master with two files; feature edits app.txt at lines 5 and 30, in two hunks. */ +function startPullRequest(): void { + repository.commit({ 'app.txt': edited({}), 'util.txt': edited({}) }, 'start'); + repository.git('checkout', '--quiet', '-b', 'feature'); + repository.commit({ 'app.txt': edited({ 5: 'five', 30: 'thirty' }) }, 'edit app'); +} + +/** A look recorded at the feature branch's current commit, as if the reviewer opened the review then. */ +async function lookNow(): Promise { + const commit = repository.sha('feature'); + await recordLook(cacheDir, REF, { commit, at: THEN.toISOString() }); + return commit; +} + +describe('since your last look', () => { + it('is nothing on the first look, and records it', async () => { + startPullRequest(); + const { since } = await openReview(); + expect(since).toBeUndefined(); + expect((await readLooks(cacheDir, REF))?.last).toEqual({ commit: repository.sha('feature'), at: NOW.toISOString() }); + }); + + it('flags only the part a plain push changed', async () => { + startPullRequest(); + const looked = await lookNow(); + repository.commit({ 'util.txt': edited({ 12: 'twelve' }) }, 'edit util'); + const { since, parts } = await openReview(); + expect(since).toMatchObject({ commit: looked, from: 'local record', at: THEN.toISOString(), outcome: 'compared' }); + expect(flagged(parts, since!)).toEqual(['util.txt:9']); + }); + + it('flags nothing after a rebase onto a newer master, whatever upstream changed', async () => { + startPullRequest(); + const looked = await lookNow(); + repository.git('checkout', '--quiet', 'master'); + // Upstream edits a line inside the context of the first hunk, edits + // the other file and adds one: none of it is the pull request's. + repository.commit({ 'app.txt': edited({ 8: 'eight upstream' }), 'util.txt': edited({ 20: 'upstream' }), 'new.txt': 'upstream\n' }, 'upstream'); + repository.git('checkout', '--quiet', 'feature'); + repository.git('rebase', '--quiet', 'master'); + expect(repository.sha('feature')).not.toBe(looked); + const { since, parts } = await openReview(); + expect(since).toMatchObject({ commit: looked, outcome: 'compared', changed: [] }); + expect(flagged(parts, since!)).toEqual([]); + }); + + it('flags only the edited part after a force-push that rebased and edited', async () => { + startPullRequest(); + const looked = await lookNow(); + repository.git('checkout', '--quiet', 'master'); + repository.commit({ 'util.txt': edited({ 20: 'upstream' }) }, 'upstream'); + repository.git('checkout', '--quiet', 'feature'); + repository.git('rebase', '--quiet', 'master'); + writeFileSync(join(repository.dir, 'app.txt'), edited({ 5: 'five', 30: 'thirty, edited' })); + repository.git('commit', '--quiet', '--no-verify', '--all', '--amend', '--no-edit'); + const { since, parts } = await openReview(); + expect(since).toMatchObject({ commit: looked, outcome: 'compared' }); + expect(flagged(parts, since!)).toEqual(['app.txt:27']); + }); + + it('counts every part as changed when GitHub no longer has the old commit', async () => { + startPullRequest(); + const looked = await lookNow(); + writeFileSync(join(repository.dir, 'app.txt'), edited({ 5: 'five', 30: 'thirty, edited' })); + repository.git('commit', '--quiet', '--no-verify', '--all', '--amend', '--no-edit'); + // The force-pushed commit is collected: nothing reaches it any more. + repository.git('reflog', 'expire', '--expire=now', '--all'); + repository.git('gc', '--quiet', '--prune=now'); + expect(repository.compare(repository.sha('master'), looked)).toBeNull(); + const { since, parts } = await openReview(); + expect(since).toEqual({ commit: looked, from: 'local record', at: THEN.toISOString(), outcome: 'not compared', changed: [] }); + expect(flagged(parts, since!)).toEqual(['app.txt:2', 'app.txt:27']); + }); + + it('counts every part as changed when the compare answers 422, as for commits sharing no history', async () => { + startPullRequest(); + const looked = await lookNow(); + writeFileSync(join(repository.dir, 'app.txt'), edited({ 5: 'five', 30: 'thirty, edited' })); + repository.git('commit', '--quiet', '--no-verify', '--all', '--amend', '--no-edit'); + // GitHub answers 422 rather than 404 when both commits exist but + // share no history, such as after the base branch switched to an + // unrelated one: the old commit is still there. + expect(repository.compare(repository.sha('master'), looked)).not.toBeNull(); + const unrelated: typeof fetch = async (input, init) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url; + if (new RegExp(`^${API}/compare/`).test(url)) { + return Response.json({ message: 'Validation Failed' }, { status: 422 }); + } + return gitHubOf(repository)(input, init); + }; + const { since, parts } = await openReview([], unrelated); + expect(since).toEqual({ commit: looked, from: 'local record', at: THEN.toISOString(), outcome: 'not compared', changed: [] }); + expect(flagged(parts, since!)).toEqual(['app.txt:2', 'app.txt:27']); + }); + + it("falls back to the commit of the reviewer's last submitted GitHub review", async () => { + startPullRequest(); + const reviewedAt = repository.sha('feature'); + repository.commit({ 'util.txt': edited({ 12: 'twelve' }) }, 'edit util'); + const someoneElse = repository.sha('feature'); + const reviews = [ + { user: { login: 'reviewer' }, state: 'COMMENTED', commit_id: reviewedAt, submitted_at: '2026-10-02T10:00:00Z' }, + { user: { login: 'someone-else' }, state: 'APPROVED', commit_id: someoneElse, submitted_at: '2026-10-03T10:00:00Z' }, + { user: { login: 'reviewer' }, state: 'PENDING', commit_id: someoneElse }, + ]; + const { since, parts } = await openReview(reviews); + expect(since).toMatchObject({ commit: reviewedAt, from: 'github review', at: '2026-10-02T10:00:00Z', outcome: 'compared' }); + expect(flagged(parts, since!)).toEqual(['util.txt:9']); + }); + + it('keeps comparing with the look before when the review opens again at the same commit', async () => { + startPullRequest(); + const looked = await lookNow(); + repository.commit({ 'util.txt': edited({ 12: 'twelve' }) }, 'edit util'); + await openReview(); + const { since, parts } = await openReview(); + expect(since).toMatchObject({ commit: looked, from: 'local record' }); + expect(flagged(parts, since!)).toEqual(['util.txt:9']); + }); + + it('reads a record that is not one as no look', async () => { + startPullRequest(); + await lookNow(); + await writeFile(join(pullRequestCacheDir(cacheDir, REF), LAST_LOOK_FILE), '{ not json', 'utf8'); + expect(await readLooks(cacheDir, REF)).toBeNull(); + const { since } = await openReview(); + expect(since).toBeUndefined(); + const stored = JSON.parse(await readFile(join(pullRequestCacheDir(cacheDir, REF), LAST_LOOK_FILE), 'utf8')); + expect(stored.last.commit).toBe(repository.sha('feature')); + }); + + it('hashes a hunk by its changed lines alone, so new context reads the same', () => { + const hunk = (context: string): Part => + parseDiff(`diff --git a/a.txt b/a.txt +index 1111111..2222222 100644 +--- a/a.txt ++++ b/a.txt +@@ -1,3 +1,3 @@ + ${context} +-old ++new +`).files[0]!; + expect(changePieces(hunk('before'))).toEqual(changePieces(hunk('upstream changed this'))); + }); +}); diff --git a/packages/engine/test/review.test.ts b/packages/engine/test/review.test.ts index 96c3638..80fe4b2 100644 --- a/packages/engine/test/review.test.ts +++ b/packages/engine/test/review.test.ts @@ -34,7 +34,7 @@ describe('reviewPullRequest', () => { }); expect(result.version).toBe(REVIEW_RESULT_VERSION); - expect(result.version).toBe(15); + expect(result.version).toBe(16); expect(result.pullRequest.number).toBe(42); expect(result.pullRequest.description).toHaveLength(8082); // The head commit's SHA, where the noise attributes are read. diff --git a/packages/engine/test/server.test.ts b/packages/engine/test/server.test.ts index 313ff71..f386a54 100644 --- a/packages/engine/test/server.test.ts +++ b/packages/engine/test/server.test.ts @@ -19,6 +19,7 @@ import { VERDICTS_INSTRUCTIONS } from '../src/verdicts.js'; import { DEFAULT_EFFORT } from '../src/ranking.js'; import { runRpcServer, type RpcAgentDeps } from '../src/server.js'; import { markedPart } from '../src/reviewed-marks.js'; +import { readLooks, recordLook } from '../src/last-look.js'; import type { ReviewResult } from '../src/protocol.js'; import { removeCopy } from '../src/cache.js'; import { @@ -145,18 +146,19 @@ describe('runRpcServer', () => { expect(responses[0]!.result).toEqual({ protocolVersion: ENGINE_PROTOCOL_VERSION }); const first = responses[1]!.result as { version: number; parts: unknown[] }; - expect(first.version).toBe(15); + expect(first.version).toBe(16); expect(first.parts).toHaveLength(11); const second = responses[2]!.result as { parts: unknown[] }; expect(second.parts).toHaveLength(11); // Each review asks GitHub for what it needs — the pull request twice // (metadata, diff), the attributes, the merge base, the linked issues, - // the check runs, and on the first run the two commit archives — + // the check runs, the reviewer and their reviews, for want of an + // earlier look, and on the first run the two commit archives — // always with the token its own request carried; the second review at // the same commits reuses the archives. const authorizations = transport.requests.map((request) => request.authorization); - expect(authorizations.slice(0, 8)).toEqual(Array(8).fill(`token ${TOKEN}`)); - expect(authorizations.slice(8)).toEqual(Array(6).fill('token ghp_another-token')); + expect(authorizations.slice(0, 10)).toEqual(Array(10).fill(`token ${TOKEN}`)); + expect(authorizations.slice(10)).toEqual(Array(8).fill('token ghp_another-token')); }); it('answers a failed review with the plain message, with the token redacted', async () => { const leakingFetch: typeof fetch = async (input, init) => { @@ -355,7 +357,7 @@ describe('runRpcServer with an agent', () => { id: 2, running: 'grouping related hunks with fake', timeoutMs: 660_000, - result: { version: 15, grouping: { by: 'plain' }, ranking: { by: 'plain' } }, + result: { version: 16, grouping: { by: 'plain' }, ranking: { by: 'plain' } }, }, }); // The fake agent has no tested ranking, so the story stage follows the grouping. @@ -694,7 +696,7 @@ describe('runRpcServer fetching a library', () => { }); expect(pypiBeforeFetch).toBe(0); expect(answer(3).result).toMatchObject({ - version: 15, + version: 16, claims: { claims: [ { @@ -747,7 +749,7 @@ describe('runRpcServer fetching a library', () => { expect(answer(3).error).toBeUndefined(); expect(answer(3).result).toMatchObject({ - version: 15, + version: 16, claims: { claims: [ { @@ -880,6 +882,19 @@ describe('runRpcServer keeping reviewed marks', () => { const initialize = request('initialize', { protocolVersion: ENGINE_PROTOCOL_VERSION }); + it('says what changed since the last look with each review, and records this one', async () => { + const store = temporaryCacheDir(); + const gone = 'abcdef0123456789abcdef0123456789abcdef01'; + await recordLook(store, { owner: 'example-org', repo: 'example-repo', number: 42 }, { commit: gone, at: '2026-10-01T09:00:00.000Z' }); + const answers = await serveInOrder([initialize, request('review', { url: PR_URL, token: TOKEN }, 2)], { fetch: fixtureFetch().fetch, cacheDir: store }); + + const result = answers[1]!.result as ReviewResult; + expect(result.sinceLastLook).toEqual({ commit: gone, from: 'local record', at: '2026-10-01T09:00:00.000Z', outcome: 'not compared', changed: [] }); + const looks = await readLooks(store, { owner: 'example-org', repo: 'example-repo', number: 42 }); + expect(looks).toEqual({ version: 1, last: { commit: result.pullRequest.headSha, at: expect.any(String) }, before: { commit: gone, at: '2026-10-01T09:00:00.000Z' } }); + await removeCopy(store); + }); + it('keeps a mark in the pull request’s local store, which a new engine reads back', async () => { const store = temporaryCacheDir(); const part = { name: 'Cart.total in web/cart.ts', pieces: [sha256Hex(Buffer.from('a hunk'))] }; diff --git a/packages/extension/package.json b/packages/extension/package.json index 849ee85..d3f026c 100644 --- a/packages/extension/package.json +++ b/packages/extension/package.json @@ -49,6 +49,12 @@ "title": "Open all parts in order", "category": "Second Look" }, + { + "command": "second-look.filterChangedSinceLastLook", + "title": "Show only parts changed since your last look, or all parts", + "category": "Second Look", + "icon": "$(filter)" + }, { "command": "second-look.submitReview", "title": "Submit review…", @@ -109,6 +115,11 @@ ], "menus": { "view/title": [ + { + "command": "second-look.filterChangedSinceLastLook", + "when": "view == second-look.reviewTree", + "group": "navigation" + }, { "command": "second-look.openAllParts", "when": "view == second-look.reviewTree", diff --git a/packages/extension/src/commands.ts b/packages/extension/src/commands.ts index e6fa68b..1426189 100644 --- a/packages/extension/src/commands.ts +++ b/packages/extension/src/commands.ts @@ -7,6 +7,9 @@ export const REVIEW_TREE_VIEW = 'second-look.reviewTree' as const; /** Opens one part's files in the multi-file diff editor. */ export const OPEN_PART_COMMAND = 'second-look.openPart' as const; +/** Toggles the tree between every part and only the parts changed since the reviewer's last look. */ +export const FILTER_CHANGED_COMMAND = 'second-look.filterChangedSinceLastLook' as const; + /** Opens the whole change in the multi-file diff editor, in ranked order. */ export const OPEN_ALL_PARTS_COMMAND = 'second-look.openAllParts' as const; diff --git a/packages/extension/src/extension.ts b/packages/extension/src/extension.ts index ee252c3..372ab9a 100644 --- a/packages/extension/src/extension.ts +++ b/packages/extension/src/extension.ts @@ -13,6 +13,7 @@ import { DISCARD_DRAFT_COMMAND, DRAFT_COMMENT_COMMAND, FETCH_LIBRARY_COMMAND, + FILTER_CHANGED_COMMAND, OPEN_ALL_PARTS_COMMAND, OPEN_LIBRARY_EVIDENCE_COMMAND, OPEN_OVERVIEW_COMMAND, @@ -29,7 +30,7 @@ import { buildTree, findAnchor, reviewBadge, - reviewStatus, + treeMessage, pendingReviewSection, type TreeComment, type TreePart, @@ -64,6 +65,7 @@ export { DISCARD_DRAFT_COMMAND, DRAFT_COMMENT_COMMAND, FETCH_LIBRARY_COMMAND, + FILTER_CHANGED_COMMAND, OPEN_ALL_PARTS_COMMAND, OPEN_LIBRARY_EVIDENCE_COMMAND, OPEN_OVERVIEW_COMMAND, @@ -221,6 +223,10 @@ class ReviewTreeProvider implements vscode.TreeDataProvider { * counts the parts left, and a part whose content changed since it was * marked is unmarked and says so. With the opt-in mirror setting on, a * file whose every part is reviewed is marked "Viewed" on GitHub too. + * + * The line above the tree says which commit the reviewer's last look was + * at and how many parts changed since, each flagged in the tree, and the + * filter shows only those parts. */ class ReviewSession { private readonly tree: ReviewTreeProvider; @@ -250,6 +256,10 @@ class ReviewSession { private stored: { url: string; marks: ReviewedMarks } | undefined; /** The files of parts marked while the review still runs, mirrored to GitHub once it finishes. */ private readonly mirrorWaiting = new Set(); + /** True while the tree shows only the parts changed since the reviewer's last look. */ + private onlyChanged = false; + /** The stage the review is still running, in words, for the line above the tree. */ + private stage: string | undefined; constructor( tree: ReviewTreeProvider, @@ -318,22 +328,23 @@ class ReviewSession { accessToken, (stage) => { if (!current()) return; + this.stage = stage.running; void this.show(stage.result, shown, stage.running); shown = true; - this.treeView.message = reviewStatus(stage.result, stage.running); }, (engine) => (marksRead = this.readMarks(engine, review, url.trim())), ), ); if (!current()) return; + this.stage = undefined; await this.show(result, shown); - this.treeView.message = reviewStatus(result); await marksRead; if (!current()) return; this.running = false; await this.mirrorViewed(); } catch (error) { if (!current()) return; + this.stage = undefined; this.treeView.message = undefined; vscode.window.showErrorMessage( error instanceof Error ? error.message : String(error), @@ -362,6 +373,7 @@ class ReviewSession { ? anchorOf(selected.part) : undefined; this.result = result; + const previous = this.url; this.url = result.pullRequest.url; this.copies.setCopies(result.copies); this.copies.setLibraries(result); @@ -374,6 +386,7 @@ class ReviewSession { this.page = undefined; this.comments.setReview(result); this.mirrorWaiting.clear(); + if (previous !== result.pullRequest.url) this.onlyChanged = false; } this.render(); this.overview.update(result, running); @@ -400,14 +413,37 @@ class ReviewSession { const pending = this.comments.pending(); return [ ...(pending.length > 0 ? [pendingReviewSection(pending)] : []), - ...buildTree(this.result, this.marks()), + ...buildTree(this.result, this.marks(), { onlyChangedSinceLastLook: this.onlyChanged }), ]; } - /** Shows the tree's sections and the badge counting the parts left to review. */ + /** + * Shows the tree's sections, the line above them — what changed since + * the last look and the stage still running — and the badge counting + * the parts left to review. + */ private render(): void { this.tree.setSections(this.sections()); - this.treeView.badge = this.result === undefined ? undefined : reviewBadge(this.result, this.marks()); + if (this.result === undefined) return; + this.treeView.message = treeMessage(this.result, this.stage, { onlyChangedSinceLastLook: this.onlyChanged }); + this.treeView.badge = reviewBadge(this.result, this.marks()); + } + + /** + * Toggles the tree between every part and only the parts changed since + * the reviewer's last look; a first look has nothing to filter. + */ + filterChanged(): void { + if (this.result === undefined) { + vscode.window.showWarningMessage('Review a pull request first, then filter its parts.'); + return; + } + if (this.result.sinceLastLook === undefined) { + vscode.window.showInformationMessage('This is your first look at this pull request, so no part changed since.'); + return; + } + this.onlyChanged = !this.onlyChanged; + this.render(); } /** The reviewed marks of the pull request shown; none until the store's are read. */ @@ -952,6 +988,7 @@ export function activate( part === undefined ? undefined : session.openPart(part), ), vscode.commands.registerCommand(OPEN_ALL_PARTS_COMMAND, () => session.openAllParts()), + vscode.commands.registerCommand(FILTER_CHANGED_COMMAND, () => session.filterChanged()), vscode.commands.registerCommand(SUBMIT_REVIEW_COMMAND, (submit?: unknown, body?: unknown) => session.submitReview(submit, body), ), diff --git a/packages/extension/src/overview.ts b/packages/extension/src/overview.ts index 29e8c70..9fad836 100644 --- a/packages/extension/src/overview.ts +++ b/packages/extension/src/overview.ts @@ -23,6 +23,7 @@ import { type ReviewResult, type Story, } from '@second-look/engine'; +import { sinceLastLookLine } from './tree.js'; /** The view type of the overview's one webview panel. */ export const OVERVIEW_VIEW_TYPE = 'second-look.overview' as const; @@ -294,6 +295,12 @@ function metaLine(result: ReviewResult): string { .join(' · '); } +/** The line under the meta saying which commit the reviewer's last look was at and what changed since; nothing on a first look. */ +function sinceLine(result: ReviewResult): string { + const line = sinceLastLookLine(result); + return line === undefined ? '' : `
${escapeHtml(line)}
`; +} + /** A chip for each stage done, and one for the stage still running. */ function stageChips(state: OverviewState): string { const { result } = state; @@ -931,6 +938,7 @@ export function overviewHtml(state: OverviewState, nonce: string): string {

${escapeHtml(result.pullRequest.title)}

${metaLine(result)}
+ ${sinceLine(result)}
${stageChips(state)}
${storySection(state)}
${criteriaSection(state)}
diff --git a/packages/extension/src/protocol.ts b/packages/extension/src/protocol.ts index 5048640..7d58dbb 100644 --- a/packages/extension/src/protocol.ts +++ b/packages/extension/src/protocol.ts @@ -717,6 +717,23 @@ function isChangeCopy(value: unknown): boolean { ); } +/** + * Checks what changed since the reviewer's last look: the commit it was + * at, where the look comes from, when it was, whether the changes were + * compared, and the hashes of the pieces that changed. + */ +function isSinceLastLook(value: unknown): boolean { + return ( + isRecord(value) && + isNonEmptyString(value['commit']) && + isOneOf(value['from'], ['local record', 'github review'] as const) && + isString(value['at']) && + isOneOf(value['outcome'], ['compared', 'not compared'] as const) && + Array.isArray(value['changed']) && + value['changed'].every((piece) => isString(piece) && SHA256.test(piece)) + ); +} + /** * Checks that a value read over the protocol is a review result of the * version this extension understands. The engine and the extension share @@ -739,6 +756,7 @@ export function isReviewResult(value: unknown): value is ReviewResult { if (!isPipelineReport(value['pipeline'])) return false; if (value['ci'] !== undefined && !isCiResults(value['ci'])) return false; if (value['criteria'] !== undefined && !isCriteria(value['criteria'])) return false; + if (value['sinceLastLook'] !== undefined && !isSinceLastLook(value['sinceLastLook'])) return false; const parts = value['parts']; if (!Array.isArray(parts) || !parts.every(isPart)) return false; const story = value['story']; diff --git a/packages/extension/src/tree.ts b/packages/extension/src/tree.ts index c936602..b81d87c 100644 --- a/packages/extension/src/tree.ts +++ b/packages/extension/src/tree.ts @@ -1,6 +1,7 @@ import { IMPORTANCE_ORDER, NO_MARKS, + changedSinceLastLook, claimCounts, filesOfPart, findingCounts, @@ -18,6 +19,7 @@ import { type ReviewedMarks, type ReviewedState, type ReviewResult, + type SinceLastLook, } from '@second-look/engine'; import { commentLocation } from './comments.js'; @@ -39,6 +41,8 @@ export interface TreePart { unexplained?: string; /** Where the part stands against the reviewed marks, which its checkbox shows. */ reviewed?: ReviewedState; + /** True when the part changed since the reviewer's last look. */ + changedSinceLastLook?: true; /** The part itself, which clicking opens in the diff editor. */ part?: Part; } @@ -63,6 +67,12 @@ export interface TreeSection { parts: (TreePart | TreeComment)[]; } +/** Which parts the tree shows. */ +export interface TreeFilter { + /** Only the parts that changed since the reviewer's last look, when the result knows of one. */ + onlyChangedSinceLastLook?: boolean; +} + /** The title of the section for parts that arrive without a rank. */ export const NOT_RANKED_YET = 'Not ranked yet'; @@ -100,9 +110,10 @@ const SECTION_TOOLTIPS: Record = { * a linked issue explains carries the unexplained badge, its one-line * reason in the tooltip. Every part carries its reviewed state for its * checkbox, and a part whose content changed since the reviewer marked it - * says so first. + * says so first. A part that changed since the reviewer's last look says + * so too, and the filter shows only those parts. */ -export function buildTree(result: ReviewResult, marks: ReviewedMarks = NO_MARKS): TreeSection[] { +export function buildTree(result: ReviewResult, marks: ReviewedMarks = NO_MARKS, filter: TreeFilter = {}): TreeSection[] { const grouped = new Map( IMPORTANCE_ORDER.map((importance) => [importance, []]), ); @@ -113,12 +124,15 @@ export function buildTree(result: ReviewResult, marks: ReviewedMarks = NO_MARKS) const findings = findingCounts(result.claims, result.parts.length); const judged = result.claims?.judging?.outcome === 'judged'; const unexplained = unexplainedReasons(result.unexplained, result.parts.length); - const withBadges = (node: TreePart, index: number): TreePart => - withReviewed( - withUnexplained(withClaims(node, counts[index]!, findings[index]!, judged), unexplained[index]), - reviewedState(result.parts[index]!, marks), - ); + const since = result.sinceLastLook; + const changed = result.parts.map((part) => since !== undefined && changedSinceLastLook(part, since)); + const withBadges = (node: TreePart, index: number): TreePart => { + const reviewed = reviewedState(result.parts[index]!, marks); + const badged = withUnexplained(withClaims(node, counts[index]!, findings[index]!, judged), unexplained[index]); + return withReviewed(changed[index] ? withLastLook(badged, since!, reviewed) : badged, reviewed); + }; result.parts.forEach((part, index) => { + if (filter.onlyChangedSinceLastLook && since !== undefined && !changed[index]) return; const assessment = part.noise; if (assessment && isLabelledNoise(assessment) && noiseSinks(assessment)) { noise.push(withBadges(noisePart(part, assessment), index)); @@ -276,6 +290,69 @@ function withReviewed(node: TreePart, reviewed: ReviewedState): TreePart { }; } +/** What a part that changed since the reviewer's last look says. */ +export const CHANGED_SINCE_LAST_LOOK = 'changed since your last look'; + +/** + * A part's node flagged as changed since the reviewer's last look: the + * note beside the label — unless it already says it changed since it + * was marked — and in the tooltip, with the commit the look was at. + */ +function withLastLook(node: TreePart, since: SinceLastLook, reviewed: ReviewedState): TreePart { + const line = + since.outcome === 'not compared' + ? `Counts as changed: the change could not be compared with your last look at ${shortCommit(since.commit)}, because that commit is gone or no longer related.` + : `Changed since your last look at ${shortCommit(since.commit)}.`; + const description = + reviewed === 'changed since marked' + ? node.description + : node.description === undefined + ? CHANGED_SINCE_LAST_LOOK + : `${CHANGED_SINCE_LAST_LOOK} · ${node.description}`; + return { + ...node, + changedSinceLastLook: true, + ...(description === undefined ? {} : { description }), + tooltip: node.tooltip === undefined ? line : `${node.tooltip}\n${line}`, + }; +} + +/** A commit as the reviewer reads it: its first seven characters. */ +function shortCommit(commit: string): string { + return commit.slice(0, 7); +} + +/** + * What changed since the reviewer's last look, in one line: the commit + * the look was at, where it comes from and on which day, and how many + * parts changed — or that the change could not be compared, so every + * part counts as changed. Absent on the first look. + */ +export function sinceLastLookLine(result: ReviewResult): string | undefined { + const since = result.sinceLastLook; + if (since === undefined) return undefined; + const look = since.from === 'github review' ? 'your last GitHub review' : 'your last look'; + const where = `${shortCommit(since.commit)} on ${since.at.slice(0, 10)}`; + if (since.outcome === 'not compared') { + return `The change could not be compared with ${look} at ${where}, because that commit is gone or no longer related: every part counts as changed.`; + } + const changed = result.parts.filter((part) => changedSinceLastLook(part, since)).length; + const total = result.parts.length; + return `Since ${look} at ${where}: ${changed} of ${total} part${total === 1 ? '' : 's'} changed.`; +} + +/** + * The line above the tree: what changed since the reviewer's last look, + * saying when the filter shows only those parts, then the review's + * status; see {@link reviewStatus}. + */ +export function treeMessage(result: ReviewResult, running?: string, filter: TreeFilter = {}): string | undefined { + const since = sinceLastLookLine(result); + const shown = since !== undefined && filter.onlyChangedSinceLastLook ? `${since} Showing only those.` : since; + const lines = [shown, reviewStatus(result, running)].filter((line) => line !== undefined); + return lines.length === 0 ? undefined : lines.join(' '); +} + /** * The tree view's badge: how many parts are left to review, with its * tooltip; absent once every part is reviewed. diff --git a/packages/extension/test/engine-client.test.ts b/packages/extension/test/engine-client.test.ts index 4325b28..cc780c4 100644 --- a/packages/extension/test/engine-client.test.ts +++ b/packages/extension/test/engine-client.test.ts @@ -106,7 +106,7 @@ describe('EngineClient against a fake engine', () => { await client.initialize(); const result = await client.review(PR_URL, TOKEN); - expect(result.version).toBe(15); + expect(result.version).toBe(16); expect(result.parts).toHaveLength(7); const requests = loggedRequests('round-trip.log') as { @@ -417,7 +417,7 @@ describe('EngineClient against a fake engine', () => { await timedOut; await client.initialize(); - expect(await client.review(PR_URL, TOKEN)).toMatchObject({ version: 15 }); + expect(await client.review(PR_URL, TOKEN)).toMatchObject({ version: 16 }); expect(spawns).toBe(2); client.dispose(); } finally { @@ -448,7 +448,7 @@ describe('EngineClient against a fake engine', () => { const stages: ReviewStageUpdate[] = []; await client.initialize(); - expect(await client.review(PR_URL, TOKEN, undefined, (stage) => stages.push(stage))).toMatchObject({ version: 15 }); + expect(await client.review(PR_URL, TOKEN, undefined, (stage) => stages.push(stage))).toMatchObject({ version: 16 }); expect(stages).toEqual([]); client.dispose(); }); diff --git a/packages/extension/test/fixtures/fake-engine.mjs b/packages/extension/test/fixtures/fake-engine.mjs index 63599fd..7129e0b 100644 --- a/packages/extension/test/fixtures/fake-engine.mjs +++ b/packages/extension/test/fixtures/fake-engine.mjs @@ -4,6 +4,8 @@ // environment: // // FAKE_ENGINE_RESULT JSON review result to return for review +// FAKE_ENGINE_RESULTS_BY_URL JSON { url: review result } to answer review by +// the requested URL, falling back to FAKE_ENGINE_RESULT // FAKE_ENGINE_ERROR answer review with this plain error message // FAKE_ENGINE_SEND_RESULT JSON sent review to return for sendReview // FAKE_ENGINE_SEND_ERROR answer sendReview with this plain error message @@ -36,6 +38,9 @@ const protocolVersion = Number(process.env.FAKE_ENGINE_PROTOCOL_VERSION ?? '1'); const reviewResult = process.env.FAKE_ENGINE_RESULT ? JSON.parse(process.env.FAKE_ENGINE_RESULT) : null; +const resultsByUrl = process.env.FAKE_ENGINE_RESULTS_BY_URL + ? JSON.parse(process.env.FAKE_ENGINE_RESULTS_BY_URL) + : null; const reviewError = process.env.FAKE_ENGINE_ERROR; const sendResult = process.env.FAKE_ENGINE_SEND_RESULT ? JSON.parse(process.env.FAKE_ENGINE_SEND_RESULT) @@ -111,7 +116,7 @@ function handle(line) { send({ jsonrpc: '2.0', method: 'review/stage', params: { id: request.id, ...stage } }); if (stageOnly) return; } - const answer = () => send({ jsonrpc: '2.0', id: request.id, result: reviewResult }); + const answer = () => send({ jsonrpc: '2.0', id: request.id, result: resultsByUrl?.[request.params.url] ?? reviewResult }); if (answerDelayMs > 0) setTimeout(answer, answerDelayMs); else answer(); return; diff --git a/packages/extension/test/integration/extension.test.ts b/packages/extension/test/integration/extension.test.ts index 8eed18a..f321257 100644 --- a/packages/extension/test/integration/extension.test.ts +++ b/packages/extension/test/integration/extension.test.ts @@ -13,6 +13,7 @@ import { DISCARD_DRAFT_COMMAND, DRAFT_COMMENT_COMMAND, FETCH_LIBRARY_COMMAND, + FILTER_CHANGED_COMMAND, OPEN_ALL_PARTS_COMMAND, OPEN_LIBRARY_EVIDENCE_COMMAND, OPEN_OVERVIEW_COMMAND, @@ -27,7 +28,7 @@ import { changeUri, libraryUri } from '../../src/change-copies.js'; import { escapeMarkdown } from '../../src/findings.js'; import { SEND_REVIEW_VIEW_TYPE } from '../../src/send-page.js'; import { claimsResult, criteriaResult, fetchedResult, judgedResult, mixedResult, offeredResult, storyResult, unexplainedResult } from '../results.js'; -import { markedPart } from '@second-look/engine'; +import { changePieces, markedPart } from '@second-look/engine'; import { OVERVIEW_VIEW_TYPE } from '../../src/overview.js'; import { Range, @@ -41,6 +42,7 @@ import { const FAKE_ENGINE = fileURLToPath(new URL('../fixtures/fake-engine.mjs', import.meta.url)); const PR_URL = 'https://github.com/example-org/example-repo/pull/42'; +const OTHER_PR_URL = 'https://github.com/example-org/example-repo/pull/43'; const TOKEN = 'ghp_test-token-do-not-print'; const workDir = mkdtempSync(join(tmpdir(), 'second-look-extension-')); @@ -51,6 +53,8 @@ afterAll(() => { interface FakeEngineOptions { result?: unknown; + /** Results the engine answers a review with, by the pull request URL asked about. */ + resultsByUrl?: Record; error?: string; sendError?: string; logName: string; @@ -73,6 +77,9 @@ function fakeEngine(options: FakeEngineOptions): ChildProcessWithoutNullStreams ...(options.result !== undefined ? { FAKE_ENGINE_RESULT: JSON.stringify(options.result) } : {}), + ...(options.resultsByUrl !== undefined + ? { FAKE_ENGINE_RESULTS_BY_URL: JSON.stringify(options.resultsByUrl) } + : {}), ...(options.error !== undefined ? { FAKE_ENGINE_ERROR: options.error } : {}), ...(options.stage !== undefined ? { FAKE_ENGINE_STAGE: JSON.stringify(options.stage) } : {}), ...(options.answerDelayMs !== undefined @@ -105,6 +112,7 @@ async function reviewWithFakeEngine(options: FakeEngineOptions): Promise { REVIEW_COMMAND, OPEN_PART_COMMAND, OPEN_ALL_PARTS_COMMAND, + FILTER_CHANGED_COMMAND, SUBMIT_REVIEW_COMMAND, ADD_COMMENT_COMMAND, COMMENT_ON_PART_COMMAND, @@ -1557,3 +1566,72 @@ describe('reviewed marks', () => { expect(stub.errorMessages).toEqual([]); }); }); + +describe('since your last look', () => { + /** The labels of the tree's parts, as the view renders them. */ + function partLabels(view: StubTreeView): string[] { + return renderedTree(view) + .filter((node) => node.contextValue === 'part' || node.contextValue === 'noise') + .map((node) => node.label); + } + + it('says which commit the last look was at, flags the changed part, and filters the tree to it and back', async () => { + const shown = mixedResult(); + const result = { + ...shown, + sinceLastLook: { commit: 'abcdef0123456789abcdef0123456789abcdef01', from: 'local record', at: '2026-10-01T09:00:00.000Z', outcome: 'compared', changed: changePieces(shown.parts[0]!) }, + }; + const view = await reviewWithFakeEngine({ result, logName: 'since.log' }); + + expect(view.message).toBe('Since your last look at abcdef0 on 2026-10-01: 1 of 7 parts changed.'); + expect(renderedTree(view).find((node) => node.label === 'src/retry.py')?.description).toContain('changed since your last look'); + expect(partLabels(view)).toHaveLength(7); + + await registeredCommands().get(FILTER_CHANGED_COMMAND)!(); + expect(partLabels(view)).toEqual(['src/retry.py']); + expect(view.message).toBe('Since your last look at abcdef0 on 2026-10-01: 1 of 7 parts changed. Showing only those.'); + + await registeredCommands().get(FILTER_CHANGED_COMMAND)!(); + expect(partLabels(view)).toHaveLength(7); + }); + + it('has nothing to filter on a first look', async () => { + const view = await reviewWithFakeEngine({ result: mixedResult(), logName: 'first-look.log' }); + + await registeredCommands().get(FILTER_CHANGED_COMMAND)!(); + expect(partLabels(view)).toHaveLength(7); + expect(stub.informationMessages).toContain('This is your first look at this pull request, so no part changed since.'); + }); + + it('keeps the only-changed filter across re-reviews of one pull request and clears it for another', async () => { + const shown = mixedResult(); + const sinceLastLook = { + commit: 'abcdef0123456789abcdef0123456789abcdef01', + from: 'local record', + at: '2026-10-01T09:00:00.000Z', + outcome: 'compared', + changed: changePieces(shown.parts[0]!), + }; + const otherPullRequest = { ...shown, pullRequest: { ...shown.pullRequest, url: OTHER_PR_URL, number: 43 } }; + const view = await reviewWithFakeEngine({ + result: { ...shown, sinceLastLook }, + resultsByUrl: { [OTHER_PR_URL]: { ...otherPullRequest, sinceLastLook } }, + logName: 'filter-reset.log', + }); + + await registeredCommands().get(FILTER_CHANGED_COMMAND)!(); + expect(partLabels(view)).toEqual(['src/retry.py']); + + // Reviewing the same pull request again keeps the filter the reviewer chose. + stub.inputBoxResult = PR_URL; + await registeredCommands().get(REVIEW_COMMAND)!() as Promise; + expect(partLabels(view)).toEqual(['src/retry.py']); + expect(view.message).toBe('Since your last look at abcdef0 on 2026-10-01: 1 of 7 parts changed. Showing only those.'); + + // Another pull request's review starts unfiltered. + stub.inputBoxResult = OTHER_PR_URL; + await registeredCommands().get(REVIEW_COMMAND)!() as Promise; + expect(partLabels(view)).toHaveLength(7); + expect(view.message).toBe('Since your last look at abcdef0 on 2026-10-01: 1 of 7 parts changed.'); + }); +}); diff --git a/packages/extension/test/overview.test.ts b/packages/extension/test/overview.test.ts index 3aff9a2..c8a151d 100644 --- a/packages/extension/test/overview.test.ts +++ b/packages/extension/test/overview.test.ts @@ -117,6 +117,17 @@ describe('overviewHtml', () => { expect([...order].sort((a, b) => a - b)).toEqual(order); }); + it('says under where it comes from which commit the last look was at, and nothing on a first look', () => { + const review = storyResult(); + const looked = { ...review, sinceLastLook: { commit: 'abcdef0123456789abcdef0123456789abcdef01', from: 'local record' as const, at: '2026-10-01T09:00:00.000Z', outcome: 'not compared' as const, changed: [] } }; + const html = overviewHtml({ result: looked }, 'NONCE'); + const meta = html.indexOf('
'); + const since = html.indexOf('
The change could not be compared with your last look at abcdef0 on 2026-10-01, because that commit is gone or no longer related: every part counts as changed.
'); + + expect(since).toBeGreaterThan(meta); + expect(overviewHtml({ result: review }, 'NONCE')).not.toContain('class="meta since"'); + }); + it('links each part the story mentions as a button, and sets code names as code', () => { const html = overviewHtml({ result: storyResult() }, 'NONCE'); expect(html).toContain( diff --git a/packages/extension/test/review-result.test.ts b/packages/extension/test/review-result.test.ts index 485bfa7..6f22cd1 100644 --- a/packages/extension/test/review-result.test.ts +++ b/packages/extension/test/review-result.test.ts @@ -668,6 +668,33 @@ describe('isReviewResult for the unexplained changes', () => { }); }); +describe('isReviewResult for what changed since the last look', () => { + const PIECE = 'a'.repeat(64); + const looked = (since: Record): Record => ({ + ...(JSON.parse(JSON.stringify(sampleResult())) as Record), + sinceLastLook: { commit: 'abcdef0123456789abcdef0123456789abcdef01', from: 'local record', at: '2026-10-01T09:00:00.000Z', outcome: 'compared', changed: [PIECE], ...since }, + }); + + it('accepts a compared look, one that could not be compared, one from a GitHub review, and a result without one', () => { + expect(isReviewResult(looked({}))).toBe(true); + expect(isReviewResult(looked({ outcome: 'not compared', changed: [] }))).toBe(true); + expect(isReviewResult(looked({ from: 'github review' }))).toBe(true); + expect(isReviewResult(sampleResult())).toBe(true); + }); + + it('rejects a look without its commit or time, from elsewhere, with another outcome, or with a changed piece that is no hash', () => { + const cases = [ + looked({ commit: '' }), + looked({ at: undefined }), + looked({ from: 'somewhere' }), + looked({ outcome: 'unchanged' }), + looked({ changed: ['not a hash'] }), + looked({ changed: undefined }), + ]; + for (const value of cases) expect(isReviewResult(value)).toBe(false); + }); +}); + describe('parseReviewResult', () => { it('reads the JSON the engine printed', () => { const result = parseReviewResult(JSON.stringify(sampleResult())); @@ -683,6 +710,6 @@ describe('parseReviewResult', () => { describe('the versioned protocol is shared with the engine', () => { it('uses the same version constant', () => { - expect(REVIEW_RESULT_VERSION).toBe(15); + expect(REVIEW_RESULT_VERSION).toBe(16); }); }); diff --git a/packages/extension/test/tree.test.ts b/packages/extension/test/tree.test.ts index 59ff899..62867d9 100644 --- a/packages/extension/test/tree.test.ts +++ b/packages/extension/test/tree.test.ts @@ -1,8 +1,9 @@ import { describe, expect, it } from 'vitest'; -import { NO_MARKS, applyMark, markedPart, type AgentGrouping, type AgentRanking, type FileSlice, type Hunk, type Part, type ReviewedMarks } from '@second-look/engine'; +import { NO_MARKS, applyMark, changePieces, markedPart, type AgentGrouping, type AgentRanking, type FileSlice, type Hunk, type Part, type ReviewedMarks, type ReviewResult, type SinceLastLook } from '@second-look/engine'; import { anchorOf, buildTree, + CHANGED_SINCE_LAST_LOOK, CHANGED_SINCE_MARKED, claimCountText, findingBadge, @@ -12,10 +13,12 @@ import { partsInReadingOrder, reviewBadge, reviewStatus, + sinceLastLookLine, + treeMessage, UNEXPLAINED_BADGE, } from '../src/tree.js'; import { claimsResult, judgedResult, mixedResult, part, result, unexplainedResult } from './results.js'; -import type { TreePart } from '../src/tree.js'; +import type { TreeFilter, TreePart } from '../src/tree.js'; /** A hunk adding one line at the given place, on both sides. */ function hunkAt(oldStart: number, newStart: number): Hunk { @@ -408,3 +411,71 @@ describe('reviewed marks in the tree', () => { expect(reviewBadge(result([cart, money]), marked(cart, money))).toBeUndefined(); }); }); + +describe('since your last look in the tree', () => { + const LOOKED = 'abcdef0123456789abcdef0123456789abcdef01'; + const cart = part('web/cart.ts', { name: 'Cart.total in web/cart.ts', hunks: [hunkAt(10, 10)] }); + const money = part('web/money.ts', { name: 'top-level code in web/money.ts', hunks: [hunkAt(1, 1)] }); + + /** The review with cart changed since a look at the given commit, from where it was recorded. */ + function since(overrides: Partial = {}): ReviewResult { + return { + ...result([cart, money]), + sinceLastLook: { commit: LOOKED, from: 'local record', at: '2026-10-01T09:00:00.000Z', outcome: 'compared', changed: changePieces(cart), ...overrides }, + }; + } + + function nodes(review: ReviewResult, filter: TreeFilter = {}, marks: ReviewedMarks = NO_MARKS): TreePart[] { + return buildTree(review, marks, filter) + .flatMap((section) => section.parts) + .filter((node): node is TreePart => node.kind !== 'comment'); + } + + it('flags the parts changed since the last look, beside the label and in the tooltip with its commit', () => { + const [changed, same] = nodes(since()); + + expect(changed!.changedSinceLastLook).toBe(true); + expect(changed!.description).toBe(CHANGED_SINCE_LAST_LOOK); + expect(changed!.tooltip).toBe('Changed since your last look at abcdef0.'); + expect(same!.changedSinceLastLook).toBeUndefined(); + expect(same!.description).toBeUndefined(); + }); + + it('shows only the changed parts when filtered, and every part without a last look', () => { + expect(nodes(since(), { onlyChangedSinceLastLook: true }).map((node) => node.label)).toEqual(['Cart.total in web/cart.ts']); + expect(nodes(since({ changed: [] }), { onlyChangedSinceLastLook: true })).toEqual([]); + expect(nodes(result([cart, money]), { onlyChangedSinceLastLook: true })).toHaveLength(2); + }); + + it('counts every part as changed when the change could not be compared with the last look', () => { + const notCompared = since({ outcome: 'not compared', changed: [] }); + + expect(nodes(notCompared, { onlyChangedSinceLastLook: true }).map((node) => node.changedSinceLastLook)).toEqual([true, true]); + expect(nodes(notCompared)[0]!.tooltip).toBe('Counts as changed: the change could not be compared with your last look at abcdef0, because that commit is gone or no longer related.'); + expect(sinceLastLookLine(notCompared)).toBe('The change could not be compared with your last look at abcdef0 on 2026-10-01, because that commit is gone or no longer related: every part counts as changed.'); + }); + + it('says which commit the last look was at, where it comes from, and how many parts changed', () => { + expect(sinceLastLookLine(result([cart, money]))).toBeUndefined(); + expect(sinceLastLookLine(since())).toBe('Since your last look at abcdef0 on 2026-10-01: 1 of 2 parts changed.'); + expect(sinceLastLookLine(since({ from: 'github review' }))).toBe('Since your last GitHub review at abcdef0 on 2026-10-01: 1 of 2 parts changed.'); + }); + + it('puts the last look above the review status, saying when the filter is on', () => { + expect(treeMessage(result([cart, money]))).toBeUndefined(); + expect(treeMessage(since(), 'ranking the parts with pi', { onlyChangedSinceLastLook: true })).toBe( + 'Since your last look at abcdef0 on 2026-10-01: 1 of 2 parts changed. Showing only those. Plain parts shown; ranking the parts with pi…', + ); + }); + + it('says only once that a part changed when it also changed since it was marked', () => { + const marks = applyMark(NO_MARKS, markedPart(cart), true, new Date('2026-10-01T09:00:00Z')); + const edited = { ...cart, hunks: [{ ...hunkAt(10, 10), lines: [{ kind: 'addition' as const, newLineNumber: 10, text: 'edited' }] }] }; + const review = { ...since({ changed: changePieces(edited) }), parts: [edited, money] }; + const [node] = nodes(review, {}, marks); + + expect(node!.description).toBe(CHANGED_SINCE_MARKED); + expect(node!.changedSinceLastLook).toBe(true); + expect(node!.tooltip).toContain('Changed since your last look at abcdef0.'); + }); +});