diff --git a/CONTEXT.md b/CONTEXT.md index 453b5c5..b86f101 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -76,6 +76,10 @@ _Avoid_: Supported model, certified model A fixed, typed request the reviewer makes about one part for deeper analysis, such as "explain" or "verify this claim". _Avoid_: Chat, prompt, question +**Reviewed mark**: +A part the reviewer has checked off, stored locally per pull request and keyed by the part's content, cleared automatically when that content changes, and mirrored to GitHub's **Viewed** only for whole files when the opt-in setting is on. +_Avoid_: Tick, checkmark, done flag + **Comment**: A review comment the reviewer sends to GitHub from the companion, as part of one pending GitHub review. _Avoid_: Bot comment, annotation diff --git a/README.md b/README.md index e41f7b8..69cb872 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ Second Look is a VS Code companion for human pull request review: it ranks the c The repository is a TypeScript workspace with three packages: - `packages/engine` — the engine: a separate local process that fetches a pull request, parses its full diff into files and hunks, reads the changed files' syntax trees from read-only copies of the change, reads the pipeline report in the description, the CI at the head commit and the acceptance criteria of the issues the pull request links, and offers its result two ways: printed as typed, versioned JSON by the review command, and over a JSON-RPC protocol on stdio by the serve command (ADR 0005). It also reads portable PDB files, the .NET debug files that record each source file's hash and Source Link URL. It drives the reviewer's installed coding agent, Pi or Claude Code, through one adapter interface (ADR 0004). -- `packages/extension` — the VS Code extension: a thin client that starts the engine as its own process, talks the JSON-RPC protocol to it after a version handshake, and shows the result as the ranked review tree, with each part readable in the editor's multi-file diff over read-only copies, and the review's overview with the story, the acceptance criteria of the linked issues, the unexplained changes, the claims and their verdicts, the pipeline report and CI, and the description, and the findings as the companion's own comment threads on the diff. Its settings pick the agent, the model, the account label and the heading the acceptance-criteria checklist sits under; the status bar shows the agent, model and account. +- `packages/extension` — the VS Code extension: a thin client that starts the engine as its own process, talks the JSON-RPC protocol to it after a version handshake, and shows the result as the ranked review tree, with each part readable in the editor's multi-file diff over read-only copies, and the review's overview with the story, the acceptance criteria of the linked issues, the unexplained changes, the claims and their verdicts, the pipeline report and CI, and the description, and the findings as the companion's own comment threads on the diff. Its settings pick the agent, the model, the account label, the heading the acceptance-criteria checklist sits under and whether reviewed marks are mirrored to GitHub's "Viewed"; the status bar shows the agent, model and account. - `packages/evaluation` — the evaluation: runs the engine offline over recorded pull requests and scores it against a stored baseline; see [its README](packages/evaluation/README.md). The protocol types live in `packages/engine/src/protocol.ts` and `packages/engine/src/rpc.ts`, carry their versions, and are shared by all three packages. @@ -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, 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, 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`. @@ -194,9 +194,9 @@ The extension adds a **Second Look: Review pull request** command and a review t 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. +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. 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. 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 one write — submitting the review — 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. 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; 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/diff.ts b/packages/engine/src/diff.ts index 12102e4..b0b353d 100644 --- a/packages/engine/src/diff.ts +++ b/packages/engine/src/diff.ts @@ -15,6 +15,8 @@ interface CurrentFile { newHeaderPath?: string; /** True while skipping the base85 payload of a `GIT binary patch`. */ inBinaryPatch: boolean; + /** The blob ids the `index` line names, kept on the part only when it is binary. */ + blobs?: { old: string; new: string }; } const DIFF_GIT = /^diff --git (.*)$/; @@ -22,6 +24,7 @@ const OLD_MODE = /^old mode (\d+)$/; const NEW_MODE = /^new mode (\d+)$/; const DELETED_FILE_MODE = /^deleted file mode (\d+)$/; const NEW_FILE_MODE = /^new file mode (\d+)$/; +const INDEX = /^index ([0-9a-f]+)\.\.([0-9a-f]+)(?: \d+)?$/; const RENAME_FROM = /^rename from (.+)$/; const RENAME_TO = /^rename to (.+)$/; const COPY_FROM = /^copy from (.+)$/; @@ -306,7 +309,9 @@ export function parseDiff(diff: string): ParsedDiff { } if (/^(similarity|dissimilarity) index /.test(line) || line.startsWith('index ')) { - // Carried in the diff but not needed by any part field yet. + // Only a binary file needs its blob ids: no hunk shows its change. + match = INDEX.exec(line); + if (match) current.blobs = { old: match[1]!, new: match[2]! }; hunk = undefined; lastDiffLine = undefined; continue; @@ -482,6 +487,7 @@ function syntaxNotYetRun(): PartSyntax { /** Applies the header paths to a finished file, in precedence order. */ function finishFile(current: CurrentFile): void { const { part } = current; + if (part.isBinary && current.blobs !== undefined) part.blobs = current.blobs; if (current.newHeaderPath !== undefined) { part.path = current.newHeaderPath; } else if (part.changeKind === 'deletion' && current.oldHeaderPath !== undefined) { diff --git a/packages/engine/src/github.ts b/packages/engine/src/github.ts index c9de899..c17b1ca 100644 --- a/packages/engine/src/github.ts +++ b/packages/engine/src/github.ts @@ -51,7 +51,9 @@ const silentLog = { /** * The engine's view of GitHub, through the official client: read for the * review, its checks and their failed jobs' logs included, and one write - * — submitting the review — when the reviewer sends it (ADR 0002). + * — submitting the review — when the reviewer sends it (ADR 0002), besides + * marking files "Viewed" when the reviewer's opt-in setting mirrors their + * reviewed marks there. * * The token lives only in the Octokit instance's memory: the client writes * it nowhere and echoes it in no error or log line. @@ -301,6 +303,26 @@ export class GitHubClient { return { url: data.html_url }; } + /** + * Marks files of the pull request "Viewed" on GitHub, the field the + * GitHub Pull Requests extension syncs too: one GraphQL mutation per + * path, after one query for the pull request's id. Only the reviewer's + * opt-in setting asks for it, and only for files whose every part they + * reviewed. Nothing is ever unmarked. + */ + async markFilesAsViewed(ref: PullRequestRef, paths: readonly string[]): Promise { + if (paths.length === 0) return; + const answer = graphqlData( + await this.requestGraphql(PULL_REQUEST_ID_QUERY, { owner: ref.owner, name: ref.repo, number: ref.number }), + 'pull-request-id query', + ) as { repository?: { pullRequest?: { id?: unknown } | null } | null }; + const id = answer.repository?.pullRequest?.id; + if (typeof id !== 'string') throw new Error(`GitHub has no pull request ${ref.owner}/${ref.repo}#${ref.number}`); + for (const path of paths) { + graphqlData(await this.requestGraphql(MARK_FILE_AS_VIEWED_MUTATION, { id, path }), 'mark-as-viewed mutation'); + } + } + /** * Reads the repository's root `.gitattributes` as stored at the given * commit, without a checkout: the contents endpoint serves the blob at @@ -384,17 +406,32 @@ const LINKED_ISSUES_QUERY = `query($owner: String!, $name: String!, $number: Int /** A GraphQL answer that failed, with the message of its first error. */ function linkedAnswer(response: unknown): GraphQLData { + return graphqlData(response, 'linked-issues query') as GraphQLData; +} + +/** A GraphQL answer's data, or a plain error naming the request when GitHub reports one. */ +function graphqlData(response: unknown, request: string): unknown { const body = (response as GraphQLAnswer) ?? {}; const errors = body.errors; if (errors !== undefined && errors.length > 0) { const message = errors[0]?.message; throw new Error( - `GitHub's linked-issues query failed${typeof message === 'string' ? `: ${message}` : ''}`, + `GitHub's ${request} failed${typeof message === 'string' ? `: ${message}` : ''}`, ); } return body.data ?? {}; } +/** The query that reads the pull request's GraphQL id, which the mark-as-viewed mutation names it by. */ +const PULL_REQUEST_ID_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { pullRequest(number: $number) { id } } +}`; + +/** The mutation that marks one file of the pull request "Viewed" for the signed-in reviewer. */ +const MARK_FILE_AS_VIEWED_MUTATION = `mutation($id: ID!, $path: String!) { + markFileAsViewed(input: { pullRequestId: $id, path: $path }) { clientMutationId } +}`; + /** One linked issue, read defensively: anything GitHub leaves out reads as absent. */ function linkedIssue(node: unknown, link: LinkedIssue['link']): LinkedIssue | undefined { if (typeof node !== 'object' || node === null) return undefined; diff --git a/packages/engine/src/grouping.ts b/packages/engine/src/grouping.ts index 720ce60..e9bacd3 100644 --- a/packages/engine/src/grouping.ts +++ b/packages/engine/src/grouping.ts @@ -172,18 +172,22 @@ export function groupingPrompt( /** * Checks an answer against the offered changes: every part needs a name - * and at least one hunk, and every id must be one that was offered, named - * once. Returns the problems; an empty list means the answer is usable. - * Ids the answer leaves out are not a problem here: the coverage rule - * collects them. + * and at least one hunk, no two parts share a name — including the one + * the part of left-out hunks takes, since the reviewed marks key on it — + * and every id must be one that was offered, named once. Returns the + * problems; an empty list means the answer is usable. Ids the answer + * leaves out are not a problem here: the coverage rule collects them. */ export function groupingProblems(items: readonly GroupingItem[], answer: GroupingAnswer): string[] { const offered = new Set(items.map((item) => item.id)); const placed = new Set(); + const names = new Set(); const problems: string[] = []; answer.parts.forEach((part, index) => { const name = part.name.replace(/\s+/g, ' ').trim(); if (name === '') problems.push(`part ${index + 1} has no name`); + else if (names.has(name)) problems.push(`part ${index + 1}'s name is used by more than one part`); + else names.add(name); if (name.length > MAX_NAME_LENGTH) problems.push(`part ${index + 1}'s name is over ${MAX_NAME_LENGTH} characters`); if (part.hunks.length === 0) problems.push(`part ${index + 1} has no hunks`); for (const id of part.hunks) { @@ -192,6 +196,9 @@ export function groupingProblems(items: readonly GroupingItem[], answer: Groupin placed.add(id); } }); + if (names.has(NOT_GROUPED_BY_AGENT) && items.some((item) => !placed.has(item.id))) { + problems.push(`a part is named ${JSON.stringify(NOT_GROUPED_BY_AGENT)}, which is reserved for the hunks left out`); + } return problems; } @@ -249,10 +256,11 @@ export interface AgentGroupingResult { /** * Asks the agent to group the files' hunks into parts, and checks its - * answer. An answer that misses the schema or names an id that was not - * offered, or one id twice, is retried once and then reported, and the - * plain grouping stays; the hunks a valid answer leaves out go to a part - * marked {@link NOT_GROUPED_BY_AGENT}. Sinking noise keeps its plain parts. + * answer. An answer that misses the schema, names an id that was not + * offered or one id twice, or names two parts alike, is retried once and + * then reported, and the plain grouping stays; the hunks a valid answer + * leaves out go to a part marked {@link NOT_GROUPED_BY_AGENT}. Sinking + * noise keeps its plain parts. */ export async function groupWithAgent( files: readonly Part[], diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 8721fba..a628cf6 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -47,4 +47,5 @@ export * from './ci.js'; export * from './criteria.js'; export * from './criteria-mapping.js'; export * from './draft-comment.js'; +export * from './reviewed-marks.js'; export { runCli } from './cli.js'; diff --git a/packages/engine/src/parts.ts b/packages/engine/src/parts.ts index a2c0a40..c5f2b92 100644 --- a/packages/engine/src/parts.ts +++ b/packages/engine/src/parts.ts @@ -6,7 +6,8 @@ const NAMED_ENTITIES = 3; /** The group every hunk outside all entities joins, one per file. */ const TOP_LEVEL = 'top level'; -function entityKey(entity: Entity): string { +/** An entity's key: its kind with its qualified name, which same-named entities of different kinds never share. */ +export function entityKey(entity: Entity): string { return `${entity.kind} ${entity.name}`; } diff --git a/packages/engine/src/protocol.ts b/packages/engine/src/protocol.ts index bf5618c..a948ac5 100644 --- a/packages/engine/src/protocol.ts +++ b/packages/engine/src/protocol.ts @@ -178,6 +178,43 @@ export interface DraftComment { stamp: AgentStamp; } +/** + * One reviewed mark: the reviewer ticked a part's checkbox. The local + * per-pull-request store keys it by the part's content hash when it was + * marked, and it covers the content hashes of the part's pieces — each + * hunk, or a file without hunks — so a part whose content changes is + * unmarked, and a regrouped part whose every piece was marked stays + * reviewed. + */ +export interface ReviewedMark { + /** The part's content hash when the reviewer marked it: the store's key. */ + hash: string; + /** The part's identity when marked — its name with its files and entity kinds — so a part that changed since can say so. */ + name: string; + /** The content hashes of the pieces the mark covers. */ + pieces: string[]; + /** When the reviewer marked it, as an ISO 8601 timestamp. */ + markedAt: string; +} + +/** The reviewed marks of one pull request, as its local store holds them. */ +export interface ReviewedMarks { + marks: ReviewedMark[]; +} + +/** + * Where a part stands against the reviewed marks: **reviewed** when every + * piece of it is marked; **changed since marked** when the reviewer marked + * it, or some of it, and its content changed since; **not reviewed** + * otherwise. + */ +export type ReviewedState = 'reviewed' | 'changed since marked' | 'not reviewed'; + +/** The files marked "Viewed" on GitHub, by their paths. */ +export interface ViewedFiles { + paths: string[]; +} + /** The review result the engine produces for one pull request. */ export interface ReviewResult { /** Schema version; compare against {@link REVIEW_RESULT_VERSION}. */ @@ -1078,6 +1115,12 @@ export interface Part { changeKind: ChangeKind; /** True when the changed content is binary, so there are no hunks to read. */ isBinary: boolean; + /** + * The abbreviated blob ids of the old and new sides, from the diff's + * `index` line; kept only for a binary file, whose change no hunk + * shows, so its content hash still changes with its content. + */ + blobs?: { old: string; new: string }; /** File mode on the old side, when the diff reports one. */ oldMode?: string; /** File mode on the new side, when the diff reports one. */ diff --git a/packages/engine/src/reviewed-marks.ts b/packages/engine/src/reviewed-marks.ts new file mode 100644 index 0000000..ba338ff --- /dev/null +++ b/packages/engine/src/reviewed-marks.ts @@ -0,0 +1,250 @@ +import { createHash, 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 type { PullRequestRef } from './github.js'; +import { entityKey, filesOfPart } from './parts.js'; +import type { 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'; + +/** The marks of a pull request nobody has marked yet. */ +export const NO_MARKS: ReviewedMarks = { marks: [] }; + +const SHA256 = /^[0-9a-f]{64}$/; + +function sha256(text: string): string { + return createHash('sha256').update(text).digest('hex'); +} + +/** What every piece of a file carries about the file itself: where it lives and how it changed. */ +function fileIdentity(file: FileSlice): unknown[] { + return [file.path, file.previousPath ?? null, file.changeKind, file.oldMode ?? null, file.newMode ?? null]; +} + +/** 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\\' : ''}`); +} + +/** + * The content hashes of a part's pieces, in order: one per hunk of each + * of its files — the file's path, change and modes, and the hunk's lines + * without their numbers — and one for a file without hunks, such as a + * binary, whose blob ids stand for its content. Identical hunks of one + * file are told apart by how many came before. + */ +export function partPieces(part: Part): string[] { + return filesOfPart(part).flatMap((file) => { + const identity = fileIdentity(file); + if (file.hunks.length === 0) { + return [sha256(JSON.stringify([...identity, file.isBinary, file.blobs ?? null]))]; + } + const seen = new Map(); + return file.hunks.map((hunk) => { + const content = JSON.stringify([...identity, hunkContent(hunk)]); + const occurrence = seen.get(content) ?? 0; + seen.set(content, occurrence + 1); + return sha256(`${content}#${occurrence}`); + }); + }); +} + +/** A part's content hash: the hash of its pieces' hashes, in order. */ +export function partContentHash(part: Part): string { + return sha256(partPieces(part).join('\n')); +} + +/** + * The identity a mark records for a part: its name, or its path when it + * has none, with the sorted paths of its files and the sorted keys — + * kind with qualified name — of the entities its hunks touch, so two + * parts that share a display name never share the identity the store + * matches marks on. + */ +function markName(part: Part): string { + const files = filesOfPart(part); + return JSON.stringify([ + part.name ?? part.path, + files.map((file) => file.path).sort(), + [...new Set(files.flatMap((file) => file.hunks.flatMap((hunk) => hunk.entities.map(entityKey))))].sort(), + ]); +} + +/** + * Where a part stands against the marks: reviewed when every one of its + * pieces is marked, changed since marked when only some are, or when a + * mark of the same identity covers other content, and not reviewed otherwise. + */ +export function reviewedState(part: Part, marks: ReviewedMarks): ReviewedState { + const marked = new Set(marks.marks.flatMap((mark) => mark.pieces)); + const pieces = partPieces(part); + if (pieces.every((piece) => marked.has(piece))) return 'reviewed'; + const name = markName(part); + if (pieces.some((piece) => marked.has(piece)) || marks.marks.some((mark) => mark.name === name)) { + return 'changed since marked'; + } + return 'not reviewed'; +} + +/** How many parts are left to review: every part not reviewed. */ +export function partsLeft(parts: readonly Part[], marks: ReviewedMarks): number { + return parts.filter((part) => reviewedState(part, marks) !== 'reviewed').length; +} + +/** + * The paths among those given whose every part is reviewed: a file split + * across parts, or shared by a part across files, counts only once all of + * them are marked, so no file is ever only partly reviewed. + */ +export function wholeFilesReviewed(parts: readonly Part[], marks: ReviewedMarks, paths: readonly string[]): string[] { + return [...new Set(paths)].filter((path) => { + const holding = parts.filter((part) => filesOfPart(part).some((file) => file.path === path)); + return holding.length > 0 && holding.every((part) => reviewedState(part, marks) === 'reviewed'); + }); +} + +/** What the store needs of the part the reviewer marks or unmarks: its identity — its name with its files and entity kinds — and its pieces. */ +export interface MarkedPart { + name: string; + pieces: string[]; +} + +/** The part as the store records it, from the part itself. */ +export function markedPart(part: Part): MarkedPart { + return { name: markName(part), pieces: partPieces(part) }; +} + +/** + * The marks after the reviewer ticks or clears a part's checkbox. Ticking + * it replaces every mark of the same identity with one keyed by the + * part's content hash now; clearing it removes the part's pieces from + * every mark and every mark of the same identity, so a regrouped part + * clears exactly the content it shows. + */ +export function applyMark(marks: ReviewedMarks, part: MarkedPart, reviewed: boolean, now: Date): ReviewedMarks { + const pieces = new Set(part.pieces); + const kept = marks.marks.filter((mark) => mark.name !== part.name); + if (reviewed) { + const hash = sha256(part.pieces.join('\n')); + return { marks: [...kept.filter((mark) => mark.hash !== hash), { hash, name: part.name, pieces: [...part.pieces], markedAt: now.toISOString() }] }; + } + return { + marks: kept + .map((mark) => ({ ...mark, pieces: mark.pieces.filter((piece) => !pieces.has(piece)) })) + .filter((mark) => mark.pieces.length > 0), + }; +} + +/** Whether a value is a part the store can record: a name and at least one piece hash. */ +export function isMarkedPart(value: unknown): value is MarkedPart { + if (typeof value !== 'object' || value === null) return false; + const { name, pieces } = value as Record; + return ( + typeof name === 'string' && + name.length > 0 && + Array.isArray(pieces) && + pieces.length > 0 && + pieces.every((piece) => typeof piece === 'string' && SHA256.test(piece)) + ); +} + +function isReviewedMark(value: unknown): value is ReviewedMark { + if (!isMarkedPart(value)) return false; + const { hash, markedAt } = value as unknown as Record; + return typeof hash === 'string' && SHA256.test(hash) && typeof markedAt === 'string'; +} + +/** The store file's shape: the marks keyed by the part's content hash when marked. */ +interface StoredMarks { + version: 1; + marks: Record>; +} + +function marksPath(cacheDir: string, ref: PullRequestRef): string { + return join(pullRequestCacheDir(cacheDir, ref), REVIEWED_MARKS_FILE); +} + +/** + * Reads a pull request's reviewed marks from its local store, which + * outlives the engine. A store not written yet holds no marks, and an + * entry that is not a mark is left out rather than trusted. + */ +export async function readReviewedMarks(cacheDir: string, ref: PullRequestRef): Promise { + let text: string; + try { + text = await readFile(marksPath(cacheDir, ref), 'utf8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return NO_MARKS; + throw error; + } + let stored: unknown; + try { + stored = JSON.parse(text); + } catch { + return NO_MARKS; + } + const entries = (stored as Partial | null)?.marks; + if (typeof entries !== 'object' || entries === null) return NO_MARKS; + return { + marks: Object.entries(entries) + .map(([hash, mark]) => ({ ...(mark as object), hash })) + .filter(isReviewedMark), + }; +} + +/** Each store's latest write, so marks made in quick succession apply in order. */ +const writes = new Map>(); + +/** + * Ticks or clears one part's checkbox in a pull request's local store and + * answers with the marks as they now stand. Writes to one store apply one + * after another, and each lands whole: written beside the store, then + * renamed over it. + */ +export function saveReviewedMark( + cacheDir: string, + ref: PullRequestRef, + part: MarkedPart, + reviewed: boolean, + now: Date = new Date(), +): Promise { + const path = marksPath(cacheDir, ref); + const previous = writes.get(path) ?? Promise.resolve(); + const next = previous.then( + () => writeMark(cacheDir, ref, path, part, reviewed, now), + () => writeMark(cacheDir, ref, path, part, reviewed, now), + ); + writes.set(path, next); + void next.finally(() => { + if (writes.get(path) === next) writes.delete(path); + }).catch(() => undefined); + return next; +} + +async function writeMark( + cacheDir: string, + ref: PullRequestRef, + path: string, + part: MarkedPart, + reviewed: boolean, + now: Date, +): Promise { + const marks = applyMark(await readReviewedMarks(cacheDir, ref), part, reviewed, now); + const stored: StoredMarks = { + version: 1, + marks: Object.fromEntries(marks.marks.map(({ hash, ...mark }) => [hash, mark])), + }; + await mkdir(pullRequestCacheDir(cacheDir, ref), { recursive: true }); + const partial = `${path}.partial-${randomBytes(6).toString('hex')}`; + try { + await writeFile(partial, `${JSON.stringify(stored, null, 2)}\n`, 'utf8'); + await rename(partial, path); + } catch (error) { + await rm(partial, { force: true }); + throw error; + } + return marks; +} diff --git a/packages/engine/src/rpc.ts b/packages/engine/src/rpc.ts index 779cf59..9034d4d 100644 --- a/packages/engine/src/rpc.ts +++ b/packages/engine/src/rpc.ts @@ -12,10 +12,13 @@ * * After the handshake, {@link REVIEW_METHOD} reviews a pull request, * {@link FETCH_LIBRARY_METHOD} presses one finding's library fetch, - * {@link DRAFT_COMMENT_METHOD} drafts a comment from one finding, and + * {@link DRAFT_COMMENT_METHOD} drafts a comment from one finding, * {@link SEND_REVIEW_METHOD} sends the pending review to GitHub as one - * review — the protocol's one write, asked for only when the reviewer - * presses send (ADR 0002). A review request also carries the reviewer's + * review — the protocol's one write of the review, asked for only when the + * reviewer presses send (ADR 0002) — {@link REVIEWED_MARKS_METHOD} and + * {@link MARK_REVIEWED_METHOD} read and change the reviewed marks in the + * pull request's local store, and {@link MARK_VIEWED_METHOD} marks files + * "Viewed" on GitHub when the reviewer's opt-in setting mirrors them. A review request also carries the reviewer's * agent choice — which installed agent runs the review's agent passes, * with which model — and the reviewer's label for the account it bills, * so switching the choice in the editor's settings reaches the next @@ -23,7 +26,8 @@ */ import type { AgentName } from './agents.js'; -import type { DraftComment, FindingRef, PendingReview, ReviewResult, SentReview } from './protocol.js'; +import type { DraftComment, FindingRef, PendingReview, ReviewedMarks, ReviewResult, SentReview, ViewedFiles } from './protocol.js'; +import type { MarkedPart } from './reviewed-marks.js'; /** Version of the JSON-RPC protocol between the extension and the engine. */ export const ENGINE_PROTOCOL_VERSION = 1 as const; @@ -200,6 +204,58 @@ export interface SendReviewParams { /** The send request's result: the review's link on GitHub. */ export type SendReviewRpcResult = SentReview; +/** The request that reads a pull request's reviewed marks from its local store. */ +export const REVIEWED_MARKS_METHOD = 'reviewedMarks' as const; + +/** One marks request: the pull request whose marks to read. */ +export interface ReviewedMarksParams { + /** The pull request's HTML URL. */ + url: string; +} + +/** The marks request's result: the marks as the store holds them. */ +export type ReviewedMarksRpcResult = ReviewedMarks; + +/** + * The request that ticks or clears one part's reviewed checkbox in the + * pull request's local store, keyed by the part's content hash, which the + * engine computes from the part's pieces. + */ +export const MARK_REVIEWED_METHOD = 'markReviewed' as const; + +/** One mark request: the pull request, the part and whether it is now reviewed. */ +export interface MarkReviewedParams { + /** The pull request's HTML URL. */ + url: string; + /** The part: its identity — its name with its files and entity kinds — and the content hashes of its pieces. */ + part: MarkedPart; + /** True to tick the part's checkbox, false to clear it. */ + reviewed: boolean; +} + +/** The mark request's result: the marks as they now stand. */ +export type MarkReviewedRpcResult = ReviewedMarks; + +/** + * The request that marks whole files "Viewed" on GitHub, sent only when + * the reviewer's opt-in setting mirrors the reviewed marks there, and only + * for files whose every part is reviewed. Nothing is unmarked. + */ +export const MARK_VIEWED_METHOD = 'markViewed' as const; + +/** One mirror request; the token travels with the request, never stored. */ +export interface MarkViewedParams { + /** The pull request's HTML URL. */ + url: string; + /** The GitHub token for this one request, from VS Code's GitHub sign-in. */ + token: string; + /** The files to mark, by their paths in the pull request. */ + paths: string[]; +} + +/** The mirror request's result: the files marked. */ +export type MarkViewedRpcResult = ViewedFiles; + /** * The notification the engine sends while a review request is still * running: the result so far is ready and a further stage, such as the diff --git a/packages/engine/src/server.ts b/packages/engine/src/server.ts index 99ad1f2..1cd391b 100644 --- a/packages/engine/src/server.ts +++ b/packages/engine/src/server.ts @@ -3,11 +3,12 @@ import { DEFAULT_AGENT_SETTINGS, type AgentAdapter, type AgentSettings } from '. import { AGENT_NAMES, isAgentName, type AgentName } from './agents.js'; import { pullRequestCacheDir } from './cache.js'; import { draftComment, draftFinding, isFindingRef } from './draft-comment.js'; -import { parsePullRequestUrl } from './github.js'; +import { GitHubClient, parsePullRequestUrl } from './github.js'; import { pressLibraryFetch } from './library-verdicts.js'; import { FINDING_REF_KINDS, type ReviewResult } from './protocol.js'; import { reviewPullRequest } from './review.js'; import { sendReview } from './send.js'; +import { isMarkedPart, readReviewedMarks, saveReviewedMark, wholeFilesReviewed } from './reviewed-marks.js'; import type { TestedRanking } from './ranking.js'; import { DRAFT_COMMENT_METHOD, @@ -19,7 +20,10 @@ import { JSON_RPC_INVALID_REQUEST, JSON_RPC_METHOD_NOT_FOUND, JSON_RPC_PARSE_ERROR, + MARK_REVIEWED_METHOD, + MARK_VIEWED_METHOD, NOT_INITIALIZED_CODE, + REVIEWED_MARKS_METHOD, REVIEW_METHOD, REVIEW_STAGE_METHOD, SEND_REVIEW_METHOD, @@ -29,8 +33,11 @@ import { type DraftCommentParams, type FetchLibraryParams, type InitializeParams, + type MarkReviewedParams, + type MarkViewedParams, type ReviewAgentChoice, type ReviewParams, + type ReviewedMarksParams, type ReviewStageParams, type RpcNotification, type RpcResponse, @@ -109,7 +116,11 @@ export interface RpcServerDeps { * `draftComment` drafts a comment from one finding of that latest * result, only when the reviewer asks for it, and answers with the * checked draft, which the reviewer edits and adds to the pending review - * or discards; nothing sends it. + * or discards; nothing sends it. `reviewedMarks` and `markReviewed` + * read and change the reviewed marks in the pull request's local store, + * which outlives the engine, and `markViewed` marks the whole files of + * the latest review that every mark covers "Viewed" on GitHub, only for + * the reviewer's opt-in mirror. * * With an agent, a review arrives in stages: as soon as the plain result * is ready the engine sends it in a {@link REVIEW_STAGE_METHOD} @@ -176,12 +187,24 @@ export async function runRpcServer( running.push(send(value.params, value.id, sink, initialized, deps)); continue; } + if (value.method === REVIEWED_MARKS_METHOD) { + running.push(reviewedMarks(value.params, value.id, sink, initialized, deps)); + continue; + } + if (value.method === MARK_REVIEWED_METHOD) { + running.push(markReviewed(value.params, value.id, sink, initialized, deps)); + continue; + } + if (value.method === MARK_VIEWED_METHOD) { + running.push(markViewed(value.params, value.id, sink, initialized, deps, reviews)); + continue; + } respond( sink, failure( value.id, JSON_RPC_METHOD_NOT_FOUND, - `unknown method: ${value.method}; this engine speaks ${INITIALIZE_METHOD}, ${REVIEW_METHOD}, ${FETCH_LIBRARY_METHOD}, ${DRAFT_COMMENT_METHOD} and ${SEND_REVIEW_METHOD}`, + `unknown method: ${value.method}; this engine speaks ${INITIALIZE_METHOD}, ${REVIEW_METHOD}, ${FETCH_LIBRARY_METHOD}, ${DRAFT_COMMENT_METHOD}, ${SEND_REVIEW_METHOD}, ${REVIEWED_MARKS_METHOD}, ${MARK_REVIEWED_METHOD} and ${MARK_VIEWED_METHOD}`, ), ); } @@ -527,6 +550,96 @@ async function send( } } +/** Reads one pull request's reviewed marks from its local store. */ +async function reviewedMarks( + params: unknown, + id: number, + sink: RpcLineSink, + initialized: boolean, + deps: RpcServerDeps, +): Promise { + if (!initialized) { + respond(sink, failure(id, NOT_INITIALIZED_CODE, `the protocol starts with a version handshake: ${INITIALIZE_METHOD} before ${REVIEWED_MARKS_METHOD}`)); + return; + } + const { url } = (params ?? {}) as Partial; + const ref = typeof url === 'string' ? parsePullRequestUrl(url) : null; + if (ref === null) { + respond(sink, failure(id, JSON_RPC_INVALID_PARAMS, `${REVIEWED_MARKS_METHOD} needs params: { "url": string }`)); + return; + } + try { + respond(sink, { jsonrpc: '2.0', id, result: await readReviewedMarks(deps.cacheDir, ref) }); + } catch (error) { + respond(sink, failure(id, ENGINE_FAILED_CODE, error instanceof Error ? error.message : String(error))); + } +} + +/** Ticks or clears one part's checkbox in the pull request's local store, answering with the marks as they now stand. */ +async function markReviewed( + params: unknown, + id: number, + sink: RpcLineSink, + initialized: boolean, + deps: RpcServerDeps, +): Promise { + if (!initialized) { + respond(sink, failure(id, NOT_INITIALIZED_CODE, `the protocol starts with a version handshake: ${INITIALIZE_METHOD} before ${MARK_REVIEWED_METHOD}`)); + return; + } + const { url, part, reviewed } = (params ?? {}) as Partial; + const ref = typeof url === 'string' ? parsePullRequestUrl(url) : null; + if (ref === null || !isMarkedPart(part) || typeof reviewed !== 'boolean') { + respond(sink, failure(id, JSON_RPC_INVALID_PARAMS, `${MARK_REVIEWED_METHOD} needs params: { "url": string, "part": { "name": string, "pieces": [sha256 hex string, ...] }, "reviewed": boolean }`)); + return; + } + try { + respond(sink, { jsonrpc: '2.0', id, result: await saveReviewedMark(deps.cacheDir, ref, part, reviewed) }); + } catch (error) { + respond(sink, failure(id, ENGINE_FAILED_CODE, error instanceof Error ? error.message : String(error))); + } +} + +/** + * Marks files "Viewed" on GitHub for the reviewer's opt-in mirror: only + * the asked-for files whose every part in the pull request's latest review + * the local store holds as reviewed, so no file is marked while part of it + * is left. The token is used for this request only and redacted from any + * error. + */ +async function markViewed( + params: unknown, + id: number, + sink: RpcLineSink, + initialized: boolean, + deps: RpcServerDeps, + reviews: Map, +): Promise { + if (!initialized) { + respond(sink, failure(id, NOT_INITIALIZED_CODE, `the protocol starts with a version handshake: ${INITIALIZE_METHOD} before ${MARK_VIEWED_METHOD}`)); + return; + } + const { url, token, paths } = (params ?? {}) as Partial; + const ref = typeof url === 'string' ? parsePullRequestUrl(url) : null; + if (ref === null || typeof url !== 'string' || typeof token !== 'string' || token.length === 0 || !Array.isArray(paths) || !paths.every((path) => typeof path === 'string')) { + respond(sink, failure(id, JSON_RPC_INVALID_PARAMS, `${MARK_VIEWED_METHOD} needs params: { "url": string, "token": string, "paths": string[] }`)); + return; + } + const result = reviews.get(url); + if (result === undefined) { + respond(sink, failure(id, ENGINE_FAILED_CODE, `this engine has no finished review of ${url} to mirror; review the pull request again`)); + return; + } + try { + const whole = wholeFilesReviewed(result.parts, await readReviewedMarks(deps.cacheDir, ref), paths); + await new GitHubClient({ token, ...(deps.fetch ? { fetch: deps.fetch } : {}) }).markFilesAsViewed(ref, whole); + respond(sink, { jsonrpc: '2.0', id, result: { paths: whole } }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + respond(sink, failure(id, ENGINE_FAILED_CODE, redactToken(message, token))); + } +} + function failure(id: number | null, code: number, message: string): RpcResponse { return { jsonrpc: '2.0', id, error: { code, message } }; } diff --git a/packages/engine/test/grouping.test.ts b/packages/engine/test/grouping.test.ts index 6c7ace0..5b4eda3 100644 --- a/packages/engine/test/grouping.test.ts +++ b/packages/engine/test/grouping.test.ts @@ -135,6 +135,26 @@ describe('groupingProblems', () => { expect(groupingProblems(items, { parts: [{ name: 'a', hunks: ['h1'] }] })).toEqual([]); }); + it('names every part whose name an earlier part already took, even one that differs only in whitespace', () => { + const answer: GroupingAnswer = { + parts: [ + { name: 'helpers', hunks: ['h1'] }, + { name: ' helpers ', hunks: ['h2'] }, + ], + }; + expect(groupingProblems(items, answer)).toEqual(["part 2's name is used by more than one part"]); + }); + + it('names a part that takes the name kept for the hunks left out, but not one that leaves none out', () => { + const collision: GroupingAnswer = { parts: [{ name: NOT_GROUPED_BY_AGENT, hunks: ['h1'] }] }; + expect(groupingProblems(items, collision)).toEqual([ + 'a part is named "not grouped by the agent", which is reserved for the hunks left out', + ]); + + const everyHunkPlaced: GroupingAnswer = { parts: [{ name: NOT_GROUPED_BY_AGENT, hunks: ['h1', 'h2'] }] }; + expect(groupingProblems(items, everyHunkPlaced)).toEqual([]); + }); + it('names every id that was not offered or is placed twice, and every empty part', () => { const answer: GroupingAnswer = { parts: [ @@ -233,6 +253,25 @@ describe('reviewChange with the agent grouping stage', () => { expect(result.grouping.agent!.detail).toMatch(/^the agent gave no usable answer \(invalid-answer: /); }); + it('keeps the plain grouping when two parts share a name, after retrying the answer once', async () => { + const input = await pull7Input(); + const duplicateName = JSON.stringify({ + parts: [ + { name: 'cleanup', hunks: [HUNKS.applyDiscount, HUNKS.reformat] }, + { name: 'cleanup', hunks: [HUNKS.deploy, HUNKS.greeter] }, + ], + }); + const agent = scriptedAgent([duplicateName, duplicateName]); + const plain = await reviewChange(input); + + const result = await reviewChange(input, { story: false, unexplained: false, claims: false, adapter: agent }); + + expect(agent.requests).toHaveLength(2); + expect(agent.requests[1]!.prompt).toContain("- part 2's name is used by more than one part"); + expect(result.parts).toEqual(plain.parts); + expect(result.grouping).toMatchObject({ by: 'plain', agent: { outcome: 'fell back', leftOut: 0 } }); + }); + it('uses an answer corrected on the retry', async () => { const input = await pull7Input(); const agent = scriptedAgent(['Here are the parts!', JSON.stringify(GOOD_ANSWER)]); diff --git a/packages/engine/test/reviewed-marks.test.ts b/packages/engine/test/reviewed-marks.test.ts new file mode 100644 index 0000000..f446ad3 --- /dev/null +++ b/packages/engine/test/reviewed-marks.test.ts @@ -0,0 +1,352 @@ +import { readFile, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { pullRequestCacheDir, removeCopy } from '../src/cache.js'; +import { parseDiff } from '../src/diff.js'; +import type { PullRequestRef } from '../src/github.js'; +import { groupParts } from '../src/parts.js'; +import type { Part, ReviewedMarks } from '../src/protocol.js'; +import { + NO_MARKS, + REVIEWED_MARKS_FILE, + applyMark, + isMarkedPart, + markedPart, + partContentHash, + partPieces, + partsLeft, + readReviewedMarks, + reviewedState, + saveReviewedMark, + wholeFilesReviewed, +} from '../src/reviewed-marks.js'; +import { temporaryCacheDir } from './helpers.js'; + +const NOW = new Date('2026-10-06T12:00:00Z'); + +/** The diff before the push: a cart total edited, a helper added, and a two-hunk config change. */ +const BEFORE = `diff --git a/web/cart.ts b/web/cart.ts +index 1111111..2222222 100644 +--- a/web/cart.ts ++++ b/web/cart.ts +@@ -10,3 +10,3 @@ export class Cart { + total(): number { +- return this.items.length; ++ return this.items.reduce((sum, item) => sum + item.price, 0); + } +diff --git a/web/money.ts b/web/money.ts +new file mode 100644 +index 0000000..3333333 +--- /dev/null ++++ b/web/money.ts +@@ -0,0 +1,2 @@ ++export const cents = (value: number): number => Math.round(value * 100); ++export const euros = (value: number): number => value / 100; +diff --git a/config/app.json b/config/app.json +index 4444444..5555555 100644 +--- a/config/app.json ++++ b/config/app.json +@@ -2,3 +2,3 @@ + "name": "shop", +- "currency": "USD", ++ "currency": "EUR", + "debug": false +@@ -20,3 +20,3 @@ + "cache": { +- "ttl": 60 ++ "ttl": 300 + } +`; + +/** The same change after a push that edits only the cart total, and adds lines above it. */ +const AFTER = `diff --git a/web/cart.ts b/web/cart.ts +index 1111111..6666666 100644 +--- a/web/cart.ts ++++ b/web/cart.ts +@@ -12,3 +12,3 @@ export class Cart { + total(): number { +- return this.items.length; ++ return this.items.reduce((sum, item) => sum + item.price * item.quantity, 0); + } +diff --git a/web/money.ts b/web/money.ts +new file mode 100644 +index 0000000..3333333 +--- /dev/null ++++ b/web/money.ts +@@ -0,0 +1,2 @@ ++export const cents = (value: number): number => Math.round(value * 100); ++export const euros = (value: number): number => value / 100; +diff --git a/config/app.json b/config/app.json +index 4444444..5555555 100644 +--- a/config/app.json ++++ b/config/app.json +@@ -40,3 +40,3 @@ + "name": "shop", +- "currency": "USD", ++ "currency": "EUR", + "debug": false +@@ -58,3 +58,3 @@ + "cache": { +- "ttl": 60 ++ "ttl": 300 + } +`; + +/** Names each parsed file as its part, the way the engine does before printing. */ +function partsOf(diff: string): Part[] { + return parseDiff(diff).files.map((file) => ({ ...file, name: `top-level code in ${file.path}` })); +} + +/** Splits a file's part into one part per hunk, each named after the file and the hunk's place. */ +function splitByHunk(part: Part): Part[] { + return part.hunks.map((hunk, index) => ({ ...part, name: `${part.path} hunk ${index + 1}`, hunks: [hunk] })); +} + +function mark(marks: ReviewedMarks, part: Part, reviewed = true): ReviewedMarks { + return applyMark(marks, markedPart(part), reviewed, NOW); +} + +function markAll(parts: readonly Part[]): ReviewedMarks { + return parts.reduce((marks, part) => mark(marks, part), NO_MARKS); +} + +describe('content hashes', () => { + it('leave a part alone when only its line numbers moved', () => { + const before = partsOf(BEFORE); + const after = partsOf(AFTER); + + expect(partContentHash(after[1]!)).toBe(partContentHash(before[1]!)); + expect(partContentHash(after[2]!)).toBe(partContentHash(before[2]!)); + }); + + it('change when a part’s content changes', () => { + expect(partContentHash(partsOf(AFTER)[0]!)).not.toBe(partContentHash(partsOf(BEFORE)[0]!)); + }); + + it('give each hunk its own piece, and tell identical hunks of one file apart', () => { + const config = partsOf(BEFORE)[2]!; + const twice = { ...config, hunks: [config.hunks[1]!, config.hunks[1]!] }; + + expect(partPieces(config)).toHaveLength(2); + expect(new Set(partPieces(twice)).size).toBe(2); + }); + + it('follow a binary file’s blob ids, since no hunk shows its change', () => { + const binary = (index: string): Part => + partsOf(`diff --git a/assets/logo.png b/assets/logo.png\nindex ${index} 100644\nBinary files a/assets/logo.png and b/assets/logo.png differ\n`)[0]!; + + expect(binary('1234567..89abcde').blobs).toEqual({ old: '1234567', new: '89abcde' }); + expect(partContentHash(binary('1234567..89abcde'))).toBe(partContentHash(binary('1234567..89abcde'))); + expect(partContentHash(binary('1234567..fedcba9'))).not.toBe(partContentHash(binary('1234567..89abcde'))); + }); + + it('change with the file’s path or mode, which every piece carries', () => { + const cart = partsOf(BEFORE)[0]!; + + expect(partContentHash({ ...cart, path: 'web/basket.ts' })).not.toBe(partContentHash(cart)); + expect(partContentHash({ ...cart, newMode: '100755' })).not.toBe(partContentHash(cart)); + }); +}); + +describe('reviewed state', () => { + it('unmarks only the part a push changed, which says it changed since it was marked', () => { + const marks = markAll(partsOf(BEFORE)); + const after = partsOf(AFTER); + + expect(after.map((part) => reviewedState(part, marks))).toEqual(['changed since marked', 'reviewed', 'reviewed']); + expect(partsLeft(after, marks)).toBe(1); + }); + + it('reads an unmarked part as not reviewed', () => { + const [cart, money] = partsOf(BEFORE); + const marks = mark(NO_MARKS, cart!); + + expect(reviewedState(money!, marks)).toBe('not reviewed'); + expect(partsLeft(partsOf(BEFORE), marks)).toBe(2); + }); + + it('says a marked part changed when the push adds a hunk to it', () => { + const config = partsOf(BEFORE)[2]!; + const marks = mark(NO_MARKS, { ...config, hunks: [config.hunks[0]!] }); + + expect(reviewedState(config, { marks: marks.marks.map((each) => ({ ...each, name: 'another name' })) })).toBe('changed since marked'); + }); + + it('keeps a regrouped part reviewed when every piece of it was marked', () => { + const config = partsOf(BEFORE)[2]!; + const marks = markAll(splitByHunk(config)); + + expect(reviewedState(config, marks)).toBe('reviewed'); + }); + + it('clears exactly the content a part shows when it is unmarked', () => { + const parts = partsOf(BEFORE); + const marks = mark(markAll(parts), parts[0]!, false); + + expect(parts.map((part) => reviewedState(part, marks))).toEqual(['not reviewed', 'reviewed', 'reviewed']); + }); + + it('replaces a part’s earlier mark when it is marked again after a change', () => { + const marks = mark(markAll(partsOf(BEFORE)), partsOf(AFTER)[0]!); + + expect(marks.marks).toHaveLength(3); + expect(marks.marks.find((each) => each.name === markedPart(partsOf(AFTER)[0]!).name)?.hash).toBe(partContentHash(partsOf(AFTER)[0]!)); + expect(reviewedState(partsOf(BEFORE)[0]!, marks)).toBe('changed since marked'); + }); +}); + +describe('parts that share a display name', () => { + /** A TypeScript file whose two hunks touch an interface and a class of one name, as the syntax pass names them. */ + function mergedConfig(interfaceHead: string, classHead: string): Part { + const hunk = (start: number, oldText: string, newText: string, kind: 'interface' | 'class') => ({ + oldStart: start, + oldLines: 1, + newStart: start, + newLines: 1, + lines: [ + { kind: 'deletion' as const, oldLineNumber: start, text: oldText }, + { kind: 'addition' as const, newLineNumber: start, text: newText }, + ], + entities: [{ kind, name: 'Config', public: true, change: 'body' as const }], + }); + return { + path: 'src/config.ts', + changeKind: 'modification', + isBinary: false, + oldMissingFinalNewline: false, + newMissingFinalNewline: false, + hunks: [hunk(3, ' interface body;', interfaceHead, 'interface'), hunk(21, ' class body;', classHead, 'class')], + additions: 2, + deletions: 2, + syntax: { language: 'typescript', formattingOnly: { status: 'not-checked', reason: '' }, checksNotRun: [] }, + }; + } + + it('keeps the marks of an interface and a class of one name in one file', () => { + const parts = groupParts([mergedConfig(' debug?: boolean;', ' currency = "USD";')]); + expect(parts.map((part) => part.name)).toEqual(['Config in src/config.ts', 'Config in src/config.ts']); + + const marks = markAll(parts); + expect(marks.marks).toHaveLength(2); + expect(parts.map((part) => reviewedState(part, marks))).toEqual(['reviewed', 'reviewed']); + expect(partsLeft(parts, marks)).toBe(0); + }); + + it('still says changed since marked when one of the same-named parts changes', () => { + const marks = markAll(groupParts([mergedConfig(' debug?: boolean;', ' currency = "USD";')])); + const after = groupParts([mergedConfig(' debug?: boolean;', ' currency = "EUR";')]); + + expect(after.map((part) => reviewedState(part, marks))).toEqual(['reviewed', 'changed since marked']); + }); + + it('keeps the marks of an agent part named like a noise part', () => { + const file = mergedConfig(' debug?: boolean;', ' currency = "USD";'); + const agent: Part = { ...file, name: 'web/deps.lock', origin: 'agent', hunks: [file.hunks[0]!] }; + const noise: Part = { + ...file, + path: 'web/deps.lock', + name: 'web/deps.lock', + origin: 'plain', + hunks: [{ ...file.hunks[1]!, entities: [] }], + }; + + const marks = markAll([agent, noise]); + expect(marks.marks).toHaveLength(2); + expect(reviewedState(agent, marks)).toBe('reviewed'); + expect(reviewedState(noise, marks)).toBe('reviewed'); + }); +}); + +describe('wholeFilesReviewed', () => { + it('names a file only once every part holding it is reviewed', () => { + const [config1, config2] = splitByHunk(partsOf(BEFORE)[2]!); + const half = mark(NO_MARKS, config1!); + + expect(wholeFilesReviewed([config1!, config2!], half, ['config/app.json'])).toEqual([]); + expect(wholeFilesReviewed([config1!, config2!], mark(half, config2!), ['config/app.json'])).toEqual(['config/app.json']); + }); + + it('counts a part across files toward each of its files', () => { + const [cart, money] = partsOf(BEFORE); + const across: Part = { ...cart!, name: 'Cart.total across files', otherFiles: [money!] }; + const marks = mark(NO_MARKS, across); + + expect(wholeFilesReviewed([across], marks, ['web/cart.ts', 'web/money.ts', 'web/cart.ts'])).toEqual(['web/cart.ts', 'web/money.ts']); + }); + + it('never names a file the parts do not hold', () => { + expect(wholeFilesReviewed(partsOf(BEFORE), markAll(partsOf(BEFORE)), ['elsewhere.ts'])).toEqual([]); + }); +}); + +describe('isMarkedPart', () => { + it('accepts a name with sha256 piece hashes, and nothing else', () => { + const part = markedPart(partsOf(BEFORE)[0]!); + + expect(isMarkedPart(part)).toBe(true); + expect(isMarkedPart({ ...part, pieces: [] })).toBe(false); + expect(isMarkedPart({ ...part, pieces: ['../../etc'] })).toBe(false); + expect(isMarkedPart({ ...part, name: '' })).toBe(false); + }); +}); + +describe('the local per-pull-request store', () => { + const ref: PullRequestRef = { owner: 'example-org', repo: 'example-repo', number: 42 }; + let cacheDir: string; + + beforeAll(() => { + cacheDir = temporaryCacheDir(); + }); + + afterAll(async () => { + await removeCopy(cacheDir); + }); + + it('holds no marks before any is made', async () => { + expect(await readReviewedMarks(temporaryCacheDir(), ref)).toEqual(NO_MARKS); + }); + + it('keeps marks across restarts, keyed by the part’s content hash', async () => { + const [cart, money] = partsOf(BEFORE); + await saveReviewedMark(cacheDir, ref, markedPart(cart!), true, NOW); + const answered = await saveReviewedMark(cacheDir, ref, markedPart(money!), true, NOW); + + // A fresh read stands for an engine started again: only the file is shared. + const read = await readReviewedMarks(cacheDir, ref); + expect(read).toEqual(answered); + expect(read.marks.map((each) => each.name)).toEqual([markedPart(cart!).name, markedPart(money!).name]); + const stored = JSON.parse(await readFile(join(pullRequestCacheDir(cacheDir, ref), REVIEWED_MARKS_FILE), 'utf8')) as { + version: number; + marks: Record; + }; + expect(stored.version).toBe(1); + expect(stored.marks[partContentHash(cart!)]).toMatchObject({ name: markedPart(cart!).name, markedAt: NOW.toISOString() }); + }); + + it('applies marks made in quick succession one after another', async () => { + const other = { ...ref, number: 43 }; + const parts = partsOf(BEFORE); + await Promise.all(parts.map((part) => saveReviewedMark(cacheDir, other, markedPart(part), true, NOW))); + + expect(partsLeft(parts, await readReviewedMarks(cacheDir, other))).toBe(0); + }); + + it('reads a store it cannot parse as holding no marks', async () => { + const broken = { ...ref, number: 44 }; + await saveReviewedMark(cacheDir, broken, markedPart(partsOf(BEFORE)[0]!), true, NOW); + await writeFile(join(pullRequestCacheDir(cacheDir, broken), REVIEWED_MARKS_FILE), '{ not json', 'utf8'); + + expect(await readReviewedMarks(cacheDir, broken)).toEqual(NO_MARKS); + }); + + it('leaves out an entry that is not a mark', async () => { + const odd = { ...ref, number: 45 }; + const saved = await saveReviewedMark(cacheDir, odd, markedPart(partsOf(BEFORE)[0]!), true, NOW); + const path = join(pullRequestCacheDir(cacheDir, odd), REVIEWED_MARKS_FILE); + const stored = JSON.parse(await readFile(path, 'utf8')) as { marks: Record }; + stored.marks['not-a-hash'] = { name: 'x', pieces: [], markedAt: NOW.toISOString() }; + await writeFile(path, JSON.stringify(stored), 'utf8'); + + expect(await readReviewedMarks(cacheDir, odd)).toEqual(saved); + }); +}); diff --git a/packages/engine/test/server.test.ts b/packages/engine/test/server.test.ts index cf73ef7..313ff71 100644 --- a/packages/engine/test/server.test.ts +++ b/packages/engine/test/server.test.ts @@ -18,6 +18,8 @@ import { LIBRARY_VERDICTS_INSTRUCTIONS } from '../src/library-verdicts.js'; 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 type { ReviewResult } from '../src/protocol.js'; import { removeCopy } from '../src/cache.js'; import { PR_7_URL, @@ -219,7 +221,7 @@ describe('runRpcServer', () => { expect(responses[0]!.id).toBeNull(); expect(responses[0]!.error!.message).toContain('not JSON'); expect(responses[1]!.error!.message).toContain('unknown method: start'); - expect(responses[1]!.error!.message).toContain('initialize, review, fetchLibrary, draftComment and sendReview'); + expect(responses[1]!.error!.message).toContain('initialize, review, fetchLibrary, draftComment, sendReview, reviewedMarks, markReviewed and markViewed'); expect(responses[2]!.result).toEqual({ protocolVersion: ENGINE_PROTOCOL_VERSION }); }); @@ -824,3 +826,141 @@ describe('runRpcServer fetching a library', () => { expect(state.pypi.requests).toEqual([]); }); }); + +describe('runRpcServer keeping reviewed marks', () => { + const GRAPHQL = 'https://api.github.com/graphql'; + + /** The recorded pull request, plus GitHub's GraphQL side of the "Viewed" mirror, recording each file marked. */ + function viewedFetch(): { fetch: typeof fetch; marked: string[]; tokens: (string | null)[] } { + const transport = fixtureFetch(); + const marked: string[] = []; + const tokens: (string | null)[] = []; + const fetchImpl: typeof fetch = async (input, init) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url; + const body = typeof init?.body === 'string' ? (JSON.parse(init.body) as { query: string; variables: Record }) : undefined; + if (url === GRAPHQL && body?.query.includes('markFileAsViewed')) { + expect(body.variables['id']).toBe('PR_kwDOfixture42'); + marked.push(body.variables['path'] as string); + tokens.push(new Headers(init?.headers).get('authorization')); + return Response.json({ data: { markFileAsViewed: { clientMutationId: null } } }); + } + if (url === GRAPHQL && body?.query.includes('pullRequest(number: $number) { id }')) { + return Response.json({ data: { repository: { pullRequest: { id: 'PR_kwDOfixture42' } } } }); + } + return transport.fetch(input, init); + }; + return { fetch: fetchImpl, marked, tokens }; + } + + /** Serves the lines one at a time, each sent once the one before it is answered. */ + async function serveInOrder(lines: string[], deps: { fetch?: typeof fetch; cacheDir: string }): Promise { + const written: Response[] = []; + let index = 0; + let answered: () => void = () => undefined; + await runRpcServer( + { + readLine: async () => { + if (index >= lines.length) return null; + while (written.length < index) await new Promise((resolve) => (answered = resolve)); + return lines[index++]!; + }, + }, + { + writeLine: (line) => { + const response = JSON.parse(line) as Response; + if (response.id === null || response.id === undefined) return; + written.push(response); + answered(); + }, + }, + deps, + ); + return written; + } + + const initialize = request('initialize', { protocolVersion: ENGINE_PROTOCOL_VERSION }); + + 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'))] }; + const first = await serveInOrder( + [initialize, request('markReviewed', { url: PR_URL, part, reviewed: true }, 2), request('reviewedMarks', { url: PR_URL }, 3)], + { cacheDir: store }, + ); + const again = await serveInOrder([initialize, request('reviewedMarks', { url: PR_URL }, 2)], { cacheDir: store }); + + const marks = first[1]!.result as { marks: { name: string; pieces: string[]; hash: string }[] }; + expect(marks.marks).toEqual([expect.objectContaining({ name: part.name, pieces: part.pieces, hash: createHash('sha256').update(part.pieces.join('\n')).digest('hex') })]); + expect(first[2]!.result).toEqual(marks); + expect(again[1]!.result).toEqual(marks); + + const cleared = await serveInOrder([initialize, request('markReviewed', { url: PR_URL, part, reviewed: false }, 2)], { cacheDir: store }); + expect(cleared[1]!.result).toEqual({ marks: [] }); + await removeCopy(store); + }); + + it('refuses a mark whose part is not a name with content hashes', async () => { + const responses = await serveInOrder( + [initialize, request('markReviewed', { url: PR_URL, part: { name: 'x', pieces: ['../escape'] }, reviewed: true }, 2), request('reviewedMarks', { url: 'https://example.com/x' }, 3)], + { cacheDir }, + ); + + expect(responses[1]!.error!.code).toBe(JSON_RPC_INVALID_PARAMS); + expect(responses[2]!.error!.code).toBe(JSON_RPC_INVALID_PARAMS); + }); + + it('marks on GitHub only the files whose every part is reviewed, with the request’s token', async () => { + const store = temporaryCacheDir(); + const transport = viewedFetch(); + const reviewed = await serveInOrder([initialize, request('review', { url: PR_URL, token: TOKEN }, 2)], { fetch: transport.fetch, cacheDir: store }); + const parts = (reviewed[1]!.result as ReviewResult).parts; + const [first, second] = parts; + const lines = [ + initialize, + request('review', { url: PR_URL, token: TOKEN }, 2), + request('markReviewed', { url: PR_URL, part: markedPart(first!), reviewed: true }, 3), + request('markViewed', { url: PR_URL, token: TOKEN, paths: [first!.path, second!.path] }, 4), + ]; + const responses = await serveInOrder(lines, { fetch: transport.fetch, cacheDir: store }); + + // Each recorded file is one part: the first is reviewed, the second is not. + expect(responses[3]!.result).toEqual({ paths: [first!.path] }); + expect(transport.marked).toEqual([first!.path]); + expect(transport.tokens.every((header) => header === `token ${TOKEN}`)).toBe(true); + await removeCopy(store); + }); + + it('refuses to mirror a pull request it holds no finished review of, and calls GitHub for nothing', async () => { + const transport = viewedFetch(); + const responses = await serveInOrder([initialize, request('markViewed', { url: PR_URL, token: TOKEN, paths: ['web/cart.ts'] }, 2)], { fetch: transport.fetch, cacheDir }); + + expect(responses[1]!.error!.code).toBe(ENGINE_FAILED_CODE); + expect(responses[1]!.error!.message).toContain('review the pull request again'); + expect(transport.marked).toEqual([]); + }); + + it('redacts the token from a mirror GitHub refused', async () => { + const store = temporaryCacheDir(); + const transport = fixtureFetch(); + const refusing: typeof fetch = async (input, init) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url; + const body = typeof init?.body === 'string' ? init.body : ''; + if (url === GRAPHQL && body.includes('{ id }')) return Response.json({ data: null, errors: [{ message: `bad credentials ${TOKEN}` }] }); + return transport.fetch(input, init); + }; + const reviewed = await serveInOrder([initialize, request('review', { url: PR_URL, token: TOKEN }, 2)], { fetch: refusing, cacheDir: store }); + const parts = (reviewed[1]!.result as ReviewResult).parts; + const lines = [ + initialize, + request('review', { url: PR_URL, token: TOKEN }, 2), + ...parts.map((part, index) => request('markReviewed', { url: PR_URL, part: markedPart(part), reviewed: true }, 3 + index)), + request('markViewed', { url: PR_URL, token: TOKEN, paths: [parts[0]!.path] }, 3 + parts.length), + ]; + const responses = await serveInOrder(lines, { fetch: refusing, cacheDir: store }); + + const failed = responses.at(-1)!; + expect(failed.error!.message).toContain("GitHub's pull-request-id query failed: bad credentials [REDACTED]"); + expect(failed.error!.message).not.toContain(TOKEN); + await removeCopy(store); + }); +}); diff --git a/packages/extension/package.json b/packages/extension/package.json index e33d276..849ee85 100644 --- a/packages/extension/package.json +++ b/packages/extension/package.json @@ -247,6 +247,11 @@ "default": "", "description": "A label for the account or subscription the agent bills, such as `Claude Max (work address)` or `Pi personal key`, stamped on every agent-produced result of a review and shown in the status bar beside the agent and model. The companion never reads the agent's login: this label is what you tell it. For Claude Code, the status bar warns when an inherited ANTHROPIC_API_KEY overrides the Claude subscription." }, + "second-look.mirrorViewedToGitHub": { + "type": "boolean", + "default": false, + "description": "Mirror the reviewed marks to GitHub's \"Viewed\" checkbox: once every part in a file is marked reviewed, the companion marks that whole file \"Viewed\" on the pull request, and never marks a file only partly reviewed or unmarks one. Off by default because the GitHub Pull Requests extension syncs the same field. The reviewed marks themselves are always kept locally, per pull request." + }, "second-look.criteriaHeading": { "type": "string", "default": "Acceptance criteria", diff --git a/packages/extension/src/engine-client.ts b/packages/extension/src/engine-client.ts index e937520..608ef44 100644 --- a/packages/extension/src/engine-client.ts +++ b/packages/extension/src/engine-client.ts @@ -7,18 +7,34 @@ import { ENGINE_PROTOCOL_VERSION, FETCH_LIBRARY_METHOD, INITIALIZE_METHOD, + MARK_REVIEWED_METHOD, + MARK_VIEWED_METHOD, + REVIEWED_MARKS_METHOD, REVIEW_METHOD, REVIEW_STAGE_METHOD, SEND_REVIEW_METHOD, type DraftComment, type FindingRef, type InitializeResult, + type MarkedPart, type PendingReview, type ReviewAgentChoice, + type ReviewedMarks, type ReviewResult, type SentReview, + type ViewedFiles, } from '@second-look/engine'; -import { DraftProtocolError, ProtocolError, SendProtocolError, isDraftComment, isReviewResult, isSentReview } from './protocol.js'; +import { + DraftProtocolError, + MarksProtocolError, + ProtocolError, + SendProtocolError, + isDraftComment, + isReviewResult, + isReviewedMarks, + isSentReview, + isViewedFiles, +} from './protocol.js'; /** * Creates the engine process this client talks to. Tests inject their own @@ -80,6 +96,12 @@ const DRAFT_COMMENT_TIMEOUT_MS = 720_000; /** How long one send request may take before the engine is given up on. */ const SEND_REVIEW_TIMEOUT_MS = 60_000; +/** How long reading or changing the reviewed marks in the local store may take. */ +const MARKS_TIMEOUT_MS = 10_000; + +/** How long marking files "Viewed" on GitHub may take before the engine is given up on. */ +const MARK_VIEWED_TIMEOUT_MS = 60_000; + /** How long a stalled engine gets to die from SIGTERM before it is killed outright. */ const KILL_GRACE_MS = 2_000; @@ -300,6 +322,49 @@ export class EngineClient { return result; } + /** Reads the pull request's reviewed marks from the engine's local store. */ + async reviewedMarks(url: string): Promise { + if (!this.handshaken) { + throw new Error('the engine has not completed its handshake yet'); + } + const result = await this.request(REVIEWED_MARKS_METHOD, { url }, MARKS_TIMEOUT_MS); + if (!isReviewedMarks(result)) { + throw new MarksProtocolError(); + } + return result; + } + + /** + * Ticks or clears one part's reviewed checkbox in the engine's local + * store. Resolves with the marks as they now stand. + */ + async markReviewed(url: string, part: MarkedPart, reviewed: boolean): Promise { + if (!this.handshaken) { + throw new Error('the engine has not completed its handshake yet'); + } + const result = await this.request(MARK_REVIEWED_METHOD, { url, part, reviewed }, MARKS_TIMEOUT_MS); + if (!isReviewedMarks(result)) { + throw new MarksProtocolError(); + } + return result; + } + + /** + * Marks whole files "Viewed" on GitHub for the reviewer's opt-in mirror, + * with the token VS Code's GitHub sign-in gave; the engine marks only the + * files whose every part is reviewed. Resolves with the files marked. + */ + async markViewed(url: string, token: string, paths: string[]): Promise { + if (!this.handshaken) { + throw new Error('the engine has not completed its handshake yet'); + } + const result = await this.request(MARK_VIEWED_METHOD, { url, token, paths }, MARK_VIEWED_TIMEOUT_MS); + if (!isViewedFiles(result)) { + throw new MarksProtocolError(); + } + return result; + } + /** Stops the engine process, if one was started. Safe to call twice. */ dispose(): void { this.failPending(new Error('the engine was stopped')); diff --git a/packages/extension/src/extension.ts b/packages/extension/src/extension.ts index d90db10..ee252c3 100644 --- a/packages/extension/src/extension.ts +++ b/packages/extension/src/extension.ts @@ -28,6 +28,7 @@ import { anchorOf, buildTree, findAnchor, + reviewBadge, reviewStatus, pendingReviewSection, type TreeComment, @@ -40,7 +41,20 @@ import { isSubmitKind, SendReviewPage } from './send-page.js'; import { OverviewPanel } from './overview.js'; import { AgentStatusBar } from './agent-status.js'; import { readAgentSettings, reviewAgentChoice } from './agent-settings.js'; -import { draftFinding, isFindingRef, type LibraryFetchOffer, type Part, type PendingReview, type ReviewResult } from '@second-look/engine'; +import { + NO_MARKS, + draftFinding, + filesOfPart, + isFindingRef, + markedPart, + parsePullRequestUrl, + wholeFilesReviewed, + type LibraryFetchOffer, + type Part, + type PendingReview, + type ReviewedMarks, + type ReviewResult, +} from '@second-look/engine'; export { ADD_COMMENT_COMMAND, @@ -122,7 +136,7 @@ function carriedPart(arg: unknown): Part | undefined { * The side-bar tree: importance groups in order with the reason beside * each part and the signals in its tooltip, the noise last, and the * pending review gathering above them all. Clicking a part opens it in - * the diff editor. + * the diff editor, and its checkbox marks it reviewed. */ class ReviewTreeProvider implements vscode.TreeDataProvider { private readonly change = new vscode.EventEmitter(); @@ -156,6 +170,8 @@ class ReviewTreeProvider implements vscode.TreeDataProvider { item.contextValue = node.kind; if (node.kind !== 'comment' && node.part !== undefined) { item.id = `part:${JSON.stringify(anchorOf(node.part))}`; + item.checkboxState = + node.reviewed === 'reviewed' ? vscode.TreeItemCheckboxState.Checked : vscode.TreeItemCheckboxState.Unchecked; item.command = { command: OPEN_PART_COMMAND, title: 'Open part in the diff editor', @@ -199,6 +215,12 @@ class ReviewTreeProvider implements vscode.TreeDataProvider { * overview opens with its first result, without taking the focus from the * tree, and follows every stage, as do the findings — the refuted and * unverifiable claims — shown as the companion's own threads on the diff. + * + * Each part's checkbox marks it reviewed in the engine's local store for + * the pull request, which outlives the editor; the tree view's badge + * 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. */ class ReviewSession { private readonly tree: ReviewTreeProvider; @@ -224,6 +246,10 @@ class ReviewSession { ); /** The review's findings, its refuted and unverifiable claims, as threads on the diff. */ private readonly findings = new FindingThreads(); + /** The reviewed marks the engine's local store holds, with the pull request they belong to. */ + 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(); constructor( tree: ReviewTreeProvider, @@ -278,22 +304,34 @@ class ReviewSession { const review = ++this.reviews; const current = (): boolean => review === this.reviews; let shown = false; + // The marks are read while the engine reviews, and the review finishes + // only once they are in. + let marksRead: Promise = Promise.resolve(); this.running = true; this.treeView.message = undefined; try { const result = await vscode.window.withProgress( { location: { viewId: REVIEW_TREE_VIEW }, title: 'Reading the pull request…' }, () => - this.engineReview(url.trim(), accessToken, (stage) => { - if (!current()) return; - void this.show(stage.result, shown, stage.running); - shown = true; - this.treeView.message = reviewStatus(stage.result, stage.running); - }), + this.engineReview( + url.trim(), + accessToken, + (stage) => { + if (!current()) return; + 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; 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.treeView.message = undefined; @@ -301,6 +339,7 @@ class ReviewSession { error instanceof Error ? error.message : String(error), ); } finally { + await marksRead; if (current()) this.running = false; } } @@ -334,8 +373,9 @@ class ReviewSession { this.page?.dispose(); this.page = undefined; this.comments.setReview(result); + this.mirrorWaiting.clear(); } - this.tree.setSections(this.sections()); + this.render(); this.overview.update(result, running); this.findings.show(result); if (!update) { @@ -360,14 +400,121 @@ class ReviewSession { const pending = this.comments.pending(); return [ ...(pending.length > 0 ? [pendingReviewSection(pending)] : []), - ...buildTree(this.result), + ...buildTree(this.result, this.marks()), ]; } - /** Rebuilds the tree's sections after the pending review changed. */ + /** Shows the tree's sections 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()); + } + + /** The reviewed marks of the pull request shown; none until the store's are read. */ + private marks(): ReviewedMarks { + const shown = this.result === undefined ? null : parsePullRequestUrl(this.result.pullRequest.url); + const stored = this.stored === undefined ? null : parsePullRequestUrl(this.stored.url); + const same = + shown !== null && + stored !== null && + shown.owner === stored.owner && + shown.repo === stored.repo && + shown.number === stored.number; + return same ? this.stored!.marks : NO_MARKS; + } + + /** Rebuilds the tree's sections after the pending review or the marks changed. */ private refreshTree(): void { if (this.result !== undefined) { - this.tree.setSections(this.sections()); + this.render(); + } + } + + /** + * Reads the pull request's reviewed marks from the engine's local store, + * for the review that asked; a failure only warns, and the parts show + * unmarked. + */ + private async readMarks(engine: EngineClient, review: number, url: string): Promise { + try { + const marks = await engine.reviewedMarks(url); + if (review !== this.reviews) return; + this.stored = { url, marks }; + this.refreshTree(); + } catch (error) { + if (review !== this.reviews) return; + vscode.window.showWarningMessage( + `The reviewed marks could not be read: ${error instanceof Error ? error.message : String(error)}`, + ); + } + } + + /** + * Ticks or clears the reviewed checkboxes the reviewer changed, one part + * at a time, in the engine's local store, then shows the marks as they + * now stand and mirrors the whole files they complete when the setting + * asks for it. A review started meanwhile keeps its own marks. + */ + async markParts(changes: readonly (readonly [TreeNode, vscode.TreeItemCheckboxState])[]): Promise { + const url = this.url; + if (url === undefined) return; + const review = this.reviews; + try { + const engine = await this.readyEngine(); + for (const [node, state] of changes) { + if (isSection(node) || node.kind === 'comment' || node.part === undefined) continue; + const reviewed = state === vscode.TreeItemCheckboxState.Checked; + const marks = await engine.markReviewed(url, markedPart(node.part), reviewed); + if (review !== this.reviews) return; + this.stored = { url, marks }; + if (reviewed) for (const file of filesOfPart(node.part)) this.mirrorWaiting.add(file.path); + } + } catch (error) { + if (review === this.reviews) { + vscode.window.showErrorMessage( + `The reviewed mark was not saved: ${error instanceof Error ? error.message : String(error)}`, + ); + } + } + if (review !== this.reviews) return; + this.refreshTree(); + await this.mirrorViewed(); + } + + /** + * Marks "Viewed" on GitHub the files of the parts just marked whose + * every part is now reviewed — only with the opt-in mirror setting on, + * off by default because the GitHub Pull Requests extension syncs the + * same field, and only once the review finished, so the engine checks + * every file against the parts it holds. A file only partly reviewed is + * never marked, and nothing is unmarked. + */ + private async mirrorViewed(): Promise { + if (!vscode.workspace.getConfiguration('second-look').get('mirrorViewedToGitHub', false)) { + this.mirrorWaiting.clear(); + return; + } + if (this.running || this.result === undefined || this.url === undefined) return; + const paths = wholeFilesReviewed(this.result.parts, this.marks(), [...this.mirrorWaiting]); + this.mirrorWaiting.clear(); + if (paths.length === 0) return; + const url = this.url; + let session: vscode.AuthenticationSession | undefined; + try { + session = await vscode.authentication.getSession('github', ['repo'], { createIfNone: false }); + } catch { + session = undefined; + } + if (!session) { + vscode.window.showWarningMessage('Sign in to GitHub to mark reviewed files "Viewed" there.'); + return; + } + try { + await (await this.readyEngine()).markViewed(url, session.accessToken, paths); + } catch (error) { + vscode.window.showErrorMessage( + `The reviewed files were not marked "Viewed" on GitHub: ${error instanceof Error ? error.message : String(error)}`, + ); } } @@ -693,10 +840,15 @@ class ReviewSession { return engine.sendReview(url, token, review); } + /** + * Sends the review request, then hands the engine over for the request + * that reads the pull request's reviewed marks, so they arrive alongside. + */ private async engineReview( url: string, token: string, onStage: (stage: ReviewStageUpdate) => void, + onSent: (engine: EngineClient) => void, ): Promise { const engine = await this.readyEngine(); // The heading the acceptance criteria checklist sits under travels @@ -707,7 +859,9 @@ class ReviewSession { .getConfiguration('second-look') .get('criteriaHeading', '') .trim(); - return engine.review(url, token, reviewAgentChoice(readAgentSettings()), onStage, heading); + const reviewed = engine.review(url, token, reviewAgentChoice(readAgentSettings()), onStage, heading); + onSent(engine); + return reviewed; } /** @@ -751,11 +905,13 @@ class ReviewSession { * the commands that open the review's overview — at the story's start, or * at one part as its "why this matters" — the commands that draft a * comment from a finding and add the draft to the pending review or - * discard it, and the status bar entry that - * shows the agent and model in use. + * discard it, the parts' reviewed checkboxes, and the status bar entry + * that shows the agent and model in use. * Nothing here runs anything from the workspace — the engine is started * from the companion's own install, reads GitHub, and writes only the - * one review the reviewer sends. + * one review the reviewer sends — and, only with the opt-in mirror + * setting on, the "Viewed" mark of each file whose every part they + * reviewed. * * Returns the review tree's data provider, so a test running in a real * editor can read the tree the command filled. @@ -767,6 +923,8 @@ export function activate( const tree = new ReviewTreeProvider(); const treeView = vscode.window.createTreeView(REVIEW_TREE_VIEW, { treeDataProvider: tree, + // A part's checkbox is its own: a section has none to tick it with. + manageCheckboxStateManually: true, }); const copies = new ChangeCopiesProvider(); const marker = new PartMarker(); @@ -776,6 +934,7 @@ export function activate( agentStatusBar.refresh(); context.subscriptions.push( treeView, + treeView.onDidChangeCheckboxState((event) => void session.markParts(event.items)), marker, comments, { dispose: () => session.dispose() }, diff --git a/packages/extension/src/protocol.ts b/packages/extension/src/protocol.ts index 27f140d..5048640 100644 --- a/packages/extension/src/protocol.ts +++ b/packages/extension/src/protocol.ts @@ -21,9 +21,11 @@ import { type PartOrigin, type PartRank, type PartRole, + type ReviewedMarks, type ReviewResult, type SentReview, type SyntaxCheck, + type ViewedFiles, } from '@second-look/engine'; /** How the pull request links an issue, as the result names it. */ @@ -241,6 +243,8 @@ function isFileSlice(value: unknown): boolean { return false; } if (typeof value['isBinary'] !== 'boolean') return false; + const blobs = value['blobs']; + if (blobs !== undefined && !(isRecord(blobs) && isString(blobs['old']) && isString(blobs['new']))) return false; if ( typeof value['oldMissingFinalNewline'] !== 'boolean' || typeof value['newMissingFinalNewline'] !== 'boolean' @@ -805,6 +809,40 @@ export function isSentReview(value: unknown): value is SentReview { ); } +/** Error thrown when a marks answer is not the reviewed marks. */ +export class MarksProtocolError extends Error { + constructor() { + super(`the engine's answer is not the pull request's reviewed marks`); + this.name = 'MarksProtocolError'; + } +} + +const SHA256 = /^[0-9a-f]{64}$/; + +/** + * Checks that a value read over the protocol is the reviewed marks: each + * mark keyed by a content hash, with the part's name, its pieces' hashes + * and when it was marked. + */ +export function isReviewedMarks(value: unknown): value is ReviewedMarks { + if (!isRecord(value) || !Array.isArray(value['marks'])) return false; + return value['marks'].every( + (mark) => + isRecord(mark) && + isString(mark['hash']) && + SHA256.test(mark['hash']) && + isString(mark['name']) && + isString(mark['markedAt']) && + Array.isArray(mark['pieces']) && + mark['pieces'].every((piece) => isString(piece) && SHA256.test(piece)), + ); +} + +/** Checks that a value read over the protocol lists the files marked "Viewed" on GitHub. */ +export function isViewedFiles(value: unknown): value is ViewedFiles { + return isRecord(value) && Array.isArray(value['paths']) && value['paths'].every(isString); +} + /** * Reads the JSON the engine printed and returns it as a review result, * throwing {@link ProtocolError} when it does not match the shared, diff --git a/packages/extension/src/tree.ts b/packages/extension/src/tree.ts index 5304525..c936602 100644 --- a/packages/extension/src/tree.ts +++ b/packages/extension/src/tree.ts @@ -1,10 +1,13 @@ import { IMPORTANCE_ORDER, + NO_MARKS, claimCounts, filesOfPart, findingCounts, isLabelledNoise, noiseSinks, + partsLeft, + reviewedState, unexplainedReasons, type Comment, type FileSlice, @@ -12,6 +15,8 @@ import { type LabelledNoise, type Part, type Ranking, + type ReviewedMarks, + type ReviewedState, type ReviewResult, } from '@second-look/engine'; import { commentLocation } from './comments.js'; @@ -32,6 +37,8 @@ export interface TreePart { findings?: number; /** Why neither the description nor a linked issue explains the part; absent when the comparison does not flag it. */ unexplained?: string; + /** Where the part stands against the reviewed marks, which its checkbox shows. */ + reviewed?: ReviewedState; /** The part itself, which clicking opens in the diff editor. */ part?: Part; } @@ -91,9 +98,11 @@ const SECTION_TOOLTIPS: Record = { * attached to shows their count beside it, and a badge counting its * findings once the claims are judged. A part neither the description nor * a linked issue explains carries the unexplained badge, its one-line - * reason in the tooltip. + * 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. */ -export function buildTree(result: ReviewResult): TreeSection[] { +export function buildTree(result: ReviewResult, marks: ReviewedMarks = NO_MARKS): TreeSection[] { const grouped = new Map( IMPORTANCE_ORDER.map((importance) => [importance, []]), ); @@ -105,7 +114,10 @@ export function buildTree(result: ReviewResult): TreeSection[] { const judged = result.claims?.judging?.outcome === 'judged'; const unexplained = unexplainedReasons(result.unexplained, result.parts.length); const withBadges = (node: TreePart, index: number): TreePart => - withUnexplained(withClaims(node, counts[index]!, findings[index]!, judged), unexplained[index]); + withReviewed( + withUnexplained(withClaims(node, counts[index]!, findings[index]!, judged), unexplained[index]), + reviewedState(result.parts[index]!, marks), + ); result.parts.forEach((part, index) => { const assessment = part.noise; if (assessment && isLabelledNoise(assessment) && noiseSinks(assessment)) { @@ -245,6 +257,35 @@ function withClaims(node: TreePart, count: number, findings: number, judged: boo }; } +/** What a part whose content changed since the reviewer marked it says. */ +export const CHANGED_SINCE_MARKED = 'changed since you marked it'; + +/** + * A part's node with its reviewed state, and, when its content changed + * since the reviewer marked it, the note saying so first beside the label + * and in the tooltip. + */ +function withReviewed(node: TreePart, reviewed: ReviewedState): TreePart { + if (reviewed !== 'changed since marked') return { ...node, reviewed }; + const line = `Unmarked: its content changed since you marked it reviewed.`; + return { + ...node, + reviewed, + description: node.description === undefined ? CHANGED_SINCE_MARKED : `${CHANGED_SINCE_MARKED} · ${node.description}`, + tooltip: node.tooltip === undefined ? line : `${node.tooltip}\n${line}`, + }; +} + +/** + * The tree view's badge: how many parts are left to review, with its + * tooltip; absent once every part is reviewed. + */ +export function reviewBadge(result: ReviewResult, marks: ReviewedMarks): { value: number; tooltip: string } | undefined { + const left = partsLeft(result.parts, marks); + if (left === 0) return undefined; + return { value: left, tooltip: `${left} of ${result.parts.length} part${result.parts.length === 1 ? '' : 's'} left to review` }; +} + /** The badge of a part neither the description nor a linked issue explains. */ export const UNEXPLAINED_BADGE = '? unexplained'; diff --git a/packages/extension/test/engine-client.test.ts b/packages/extension/test/engine-client.test.ts index 2d4c7b5..4325b28 100644 --- a/packages/extension/test/engine-client.test.ts +++ b/packages/extension/test/engine-client.test.ts @@ -32,6 +32,7 @@ interface FakeEngineOptions { fetchError?: string; draftResult?: unknown; draftError?: string; + viewedError?: string; } /** Starts the fake engine as a separate process, speaking real stdio. */ @@ -59,6 +60,7 @@ function fakeEngine(options: FakeEngineOptions = {}): ChildProcessWithoutNullStr ...(options.fetchError !== undefined ? { FAKE_ENGINE_FETCH_ERROR: options.fetchError } : {}), ...(options.draftResult !== undefined ? { FAKE_ENGINE_DRAFT_RESULT: JSON.stringify(options.draftResult) } : {}), ...(options.draftError !== undefined ? { FAKE_ENGINE_DRAFT_ERROR: options.draftError } : {}), + ...(options.viewedError !== undefined ? { FAKE_ENGINE_VIEWED_ERROR: options.viewedError } : {}), }, }); } @@ -192,6 +194,51 @@ describe('EngineClient against a fake engine', () => { malformed.dispose(); }); + it('ticks and clears a part’s reviewed checkbox, and reads the marks back', async () => { + const client = new EngineClient(() => fakeEngine({ logName: 'reviewed-marks.log' })); + const part = { name: 'Cart.total in web/cart.ts', pieces: ['a'.repeat(64), 'b'.repeat(64)] }; + + await client.initialize(); + expect(await client.reviewedMarks(PR_URL)).toEqual({ marks: [] }); + const marked = await client.markReviewed(PR_URL, part, true); + expect(marked.marks).toEqual([expect.objectContaining({ name: part.name, pieces: part.pieces })]); + expect(await client.reviewedMarks(PR_URL)).toEqual(marked); + expect(await client.markReviewed(PR_URL, part, false)).toEqual({ marks: [] }); + + const requests = loggedRequests('reviewed-marks.log') as { method: string; params: unknown }[]; + // Marks stay local: no request about them carries a token. + expect(requests.filter((each) => each.method === 'markReviewed').map((each) => each.params)).toEqual([ + { url: PR_URL, part, reviewed: true }, + { url: PR_URL, part, reviewed: false }, + ]); + client.dispose(); + }); + + it('marks files "Viewed" with the token of that one request, and reads a refusal as its plain message', async () => { + const client = new EngineClient(() => fakeEngine({ logName: 'mark-viewed.log' })); + await client.initialize(); + + expect(await client.markViewed(PR_URL, TOKEN, ['web/cart.ts'])).toEqual({ paths: ['web/cart.ts'] }); + const request = loggedRequests('mark-viewed.log').find((each) => (each as { method: string }).method === 'markViewed') as { params: unknown }; + expect(request.params).toEqual({ url: PR_URL, token: TOKEN, paths: ['web/cart.ts'] }); + client.dispose(); + + const failing = new EngineClient(() => fakeEngine({ viewedError: 'GitHub refused' })); + await failing.initialize(); + await expect(failing.markViewed(PR_URL, TOKEN, ['web/cart.ts'])).rejects.toThrow('GitHub refused'); + failing.dispose(); + }); + + it('refuses a marks answer that is not the reviewed marks', async () => { + const client = new EngineClient(() => fakeEngine()); + await client.initialize(); + + await expect(client.markReviewed(PR_URL, { name: 'x', pieces: ['not a hash'] }, true)).rejects.toThrow( + "the engine's answer is not the pull request's reviewed marks", + ); + client.dispose(); + }); + it('carries the agent, model and account choice with the review request', async () => { const client = new EngineClient(() => fakeEngine({ result: mixedResult(), logName: 'agent-choice.log' })); diff --git a/packages/extension/test/fixtures/fake-engine.mjs b/packages/extension/test/fixtures/fake-engine.mjs index 2712411..63599fd 100644 --- a/packages/extension/test/fixtures/fake-engine.mjs +++ b/packages/extension/test/fixtures/fake-engine.mjs @@ -21,9 +21,15 @@ // review/stage notification before the answer // FAKE_ENGINE_STAGE_ONLY send the stage notification, then never answer // FAKE_ENGINE_ANSWER_DELAY_MS wait this long after the stage before answering +// FAKE_ENGINE_VIEWED_ERROR answer markViewed with this plain error message +// +// The reviewed marks live in the fake's memory: markReviewed ticks or +// clears a part by its name, reviewedMarks reads them back, and markViewed +// answers with the paths it was asked to mark. // // Every request it receives is appended to the log, so a test can prove // what reached the engine, including the token carried per request. +import { createHash } from 'node:crypto'; import { appendFileSync } from 'node:fs'; const protocolVersion = Number(process.env.FAKE_ENGINE_PROTOCOL_VERSION ?? '1'); @@ -46,6 +52,8 @@ const log = process.env.FAKE_ENGINE_LOG; const stage = process.env.FAKE_ENGINE_STAGE ? JSON.parse(process.env.FAKE_ENGINE_STAGE) : null; const stageOnly = Boolean(process.env.FAKE_ENGINE_STAGE_ONLY); const answerDelayMs = Number(process.env.FAKE_ENGINE_ANSWER_DELAY_MS ?? '0'); +const viewedError = process.env.FAKE_ENGINE_VIEWED_ERROR; +let marks = []; if (process.env.FAKE_ENGINE_IGNORE_SIGTERM) { process.on('SIGTERM', () => { @@ -128,6 +136,25 @@ function handle(line) { else send({ jsonrpc: '2.0', id: request.id, result: draftResult }); return; } + if (request.method === 'reviewedMarks') { + send({ jsonrpc: '2.0', id: request.id, result: { marks } }); + return; + } + if (request.method === 'markReviewed') { + const { part, reviewed } = request.params; + marks = marks.filter((mark) => mark.name !== part.name); + if (reviewed) { + const hash = createHash('sha256').update(part.pieces.join('\n')).digest('hex'); + marks.push({ hash, name: part.name, pieces: part.pieces, markedAt: '2026-10-06T00:00:00.000Z' }); + } + send({ jsonrpc: '2.0', id: request.id, result: { marks } }); + return; + } + if (request.method === 'markViewed') { + if (viewedError) fail(request.id, -32002, viewedError); + else send({ jsonrpc: '2.0', id: request.id, result: { paths: request.params.paths } }); + return; + } fail(request.id, -32601, `unknown method: ${request.method}`); } diff --git a/packages/extension/test/integration/extension.test.ts b/packages/extension/test/integration/extension.test.ts index 97a540d..8eed18a 100644 --- a/packages/extension/test/integration/extension.test.ts +++ b/packages/extension/test/integration/extension.test.ts @@ -27,9 +27,11 @@ 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 { OVERVIEW_VIEW_TYPE } from '../../src/overview.js'; import { Range, + TreeItemCheckboxState, stub, stubContext, workspace, @@ -806,7 +808,7 @@ describe('the overview', () => { expect(offered).toContain('command:second-look.fetchLibrary'); const logged = (): { method: string; params: unknown }[] => readFileSync(join(workDir, 'fetch-library.log'), 'utf8').split('\n').filter(Boolean).map((line) => JSON.parse(line) as { method: string; params: unknown }); - expect(logged().map((request) => request.method)).toEqual(['initialize', 'review']); + expect(logged().map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); await registeredCommands().get(FETCH_LIBRARY_COMMAND)!(2); @@ -909,7 +911,7 @@ describe('the pending review and sending it', () => { { label: 'src/retry.py:5', description: 'this retry loop needs a cap', tooltip: 'this retry loop needs a cap', contextValue: 'comment' }, { label: 'src/retry.py (part)', description: 'the loop reads well overall', tooltip: 'the loop reads well overall', contextValue: 'comment' }, ]); - expect(engineRequests('send.log').map((request) => request.method)).toEqual(['initialize', 'review']); + expect(engineRequests('send.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); expect(stub.sessionRequests).toHaveLength(sessionRequestsBefore); // Submit review… opens the Send review page: both comments together, @@ -1008,7 +1010,7 @@ describe('the pending review and sending it', () => { thread.comments[0]!.body = 'The docstring says three attempts, but `src/retry.py:6` loops five times. Which is meant?'; await registeredCommands().get(ADD_DRAFT_COMMAND)!(thread.comments[0]); expect(renderedTree(view)[1]).toMatchObject({ label: 'src/retry.py:3', contextValue: 'comment' }); - expect(engineRequests('draft.log').map((request) => request.method)).toEqual(['initialize', 'review', 'draftComment']); + expect(engineRequests('draft.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks', 'draftComment']); // Sending stays the Send review page's one press. await registeredCommands().get(SUBMIT_REVIEW_COMMAND)!() as Promise; @@ -1055,7 +1057,7 @@ describe('the pending review and sending it', () => { await registeredCommands().get(DRAFT_COMMENT_COMMAND)!({ kind: 'described change', index: 0 }); expect(sendPage().webview.posted.at(-1)).toMatchObject({ type: 'state', body: 'The description says failed sends are retried and logged, but nothing logs a retry.', drafts: [] }); - expect(engineRequests('draft-overall.log').map((request) => request.method)).toEqual(['initialize', 'review', 'draftComment', 'draftComment']); + expect(engineRequests('draft-overall.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks', 'draftComment', 'draftComment']); }); it('shows and sends a draft with injected markup escaped, so it never renders', async () => { @@ -1120,7 +1122,7 @@ describe('the pending review and sending it', () => { await registeredCommands().get(DRAFT_COMMENT_COMMAND)!({ kind: 'claim', index: 0 }); expect(stub.warningMessages).toEqual(['This finding cannot be drafted from; review the pull request again.']); - expect(engineRequests('draft-none.log').map((request) => request.method)).toEqual(['initialize', 'review']); + expect(engineRequests('draft-none.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); }); it('drops a comment on the page instead of sending it', async () => { @@ -1352,7 +1354,7 @@ describe('the pending review and sending it', () => { page.dispose(); // The reviewer closes the page's tab. await new Promise((resolve) => setTimeout(resolve, 0)); - expect(engineRequests('closed-page.log').map((request) => request.method)).toEqual(['initialize', 'review']); + expect(engineRequests('closed-page.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); expect(stub.sessionRequests).toHaveLength(1); // Only the review's own. expect(stub.progressTitles).toHaveLength(1); expect(stub.commentControllers[0]!.threads).toContain(thread); @@ -1382,7 +1384,7 @@ describe('the pending review and sending it', () => { expect(stub.warningMessages).toEqual([ 'Nothing to send yet: write a comment or an overall comment, or approve.', ]); - expect(engineRequests('empty-send.log').map((request) => request.method)).toEqual(['initialize', 'review']); + expect(engineRequests('empty-send.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); }); it('refuses an empty request-changes review; an empty approve still sends', async () => { @@ -1398,7 +1400,7 @@ describe('the pending review and sending it', () => { expect(stub.warningMessages).toEqual([ 'Nothing to send yet: write a comment or an overall comment, or approve.', ]); - expect(engineRequests('empty-request-changes.log').map((request) => request.method)).toEqual(['initialize', 'review']); + expect(engineRequests('empty-request-changes.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); expect(sendPages()).toContain(page); // The page keeps the choice. drive(page, { type: 'kind', submit: 'approve' }); @@ -1429,7 +1431,7 @@ describe('the pending review and sending it', () => { expect(stub.warningMessages).toEqual([ 'One comment is empty: write it or drop it before sending.', ]); - expect(engineRequests('blank-comment.log').map((request) => request.method)).toEqual(['initialize', 'review']); + expect(engineRequests('blank-comment.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); expect(stub.commentControllers[0]!.threads).toContain(thread); }); @@ -1447,7 +1449,7 @@ describe('the pending review and sending it', () => { expect(stub.warningMessages).toEqual(['Sign in to GitHub to send the review.']); expect(renderedTree(view)[1]).toMatchObject({ label: 'src/retry.py:5' }); - expect(engineRequests('no-sign-in-send.log').map((request) => request.method)).toEqual(['initialize', 'review']); + expect(engineRequests('no-sign-in-send.log').map((request) => request.method)).toEqual(['initialize', 'review', 'reviewedMarks']); }); it('closes the page when a new review starts', async () => { @@ -1466,7 +1468,9 @@ describe('the pending review and sending it', () => { expect(engineRequests('new-review-page.log').map((request) => request.method)).toEqual([ 'initialize', 'review', + 'reviewedMarks', 'review', + 'reviewedMarks', ]); }); @@ -1489,3 +1493,67 @@ describe('the pending review and sending it', () => { expect(stub.commentControllers[0]!.threads).not.toContain(onBase); }); }); + +describe('reviewed marks', () => { + /** The tree's parts, each with the item the view renders for it. */ + function partNodes(view: StubTreeView): { node: unknown; label?: string; checkboxState?: number }[] { + const provider = providerOf(view); + return provider + .getChildren() + .flatMap((section) => provider.getChildren(section)) + .map((node) => ({ node, item: provider.getTreeItem(node) as ReturnType & { checkboxState?: number } })) + .filter(({ item }) => item.contextValue === 'part' || item.contextValue === 'noise') + .map(({ node, item }) => ({ node, label: item.label, checkboxState: item.checkboxState })); + } + + function loggedRequests(logName: string): { method: string; params?: Record }[] { + return readFileSync(join(workDir, logName), 'utf8') + .split('\n') + .filter((line) => line !== '') + .map((line) => JSON.parse(line) as { method: string; params?: Record }); + } + + it("gives every part a checkbox, keeps a tick in the engine's store, and counts the parts left in the view's badge", async () => { + const view = await reviewWithFakeEngine({ result: mixedResult(), logName: 'marks.log' }); + const parts = partNodes(view); + + expect(view.options).toMatchObject({ manageCheckboxStateManually: true }); + expect(parts).toHaveLength(7); + expect(parts.every((each) => each.checkboxState === TreeItemCheckboxState.Unchecked)).toBe(true); + expect(view.badge).toEqual({ value: 7, tooltip: '7 of 7 parts left to review' }); + + view.fireCheckboxChange([[parts[0]!.node, TreeItemCheckboxState.Checked]]); + await until('the mark to be kept', () => view.badge?.value === 6); + + expect(partNodes(view)[0]).toMatchObject({ label: 'src/retry.py', checkboxState: TreeItemCheckboxState.Checked }); + const marks = loggedRequests('marks.log').filter((request) => request.method === 'markReviewed'); + expect(marks.map((request) => request.params)).toEqual([ + { url: PR_URL, part: { name: markedPart(mixedResult().parts[0]!).name, pieces: [expect.stringMatching(/^[0-9a-f]{64}$/)] }, reviewed: true }, + ]); + // The mirror is off by default: nothing asked GitHub, and no sign-in beyond the review's own. + expect(loggedRequests('marks.log').map((request) => request.method)).not.toContain('markViewed'); + expect(stub.sessionRequests).toHaveLength(1); + + view.fireCheckboxChange([[partNodes(view)[0]!.node, TreeItemCheckboxState.Unchecked]]); + await until('the mark to be cleared', () => view.badge?.value === 7); + expect(partNodes(view)[0]!.checkboxState).toBe(TreeItemCheckboxState.Unchecked); + expect(stub.errorMessages).toEqual([]); + }); + + it('with the mirror setting on, marks a file "Viewed" on GitHub once its every part is reviewed, never on a clear', async () => { + stub.configuration['second-look.mirrorViewedToGitHub'] = true; + const view = await reviewWithFakeEngine({ result: mixedResult(), logName: 'mirror.log' }); + + view.fireCheckboxChange([[partNodes(view)[0]!.node, TreeItemCheckboxState.Checked]]); + await until('the file to be mirrored', () => loggedRequests('mirror.log').some((request) => request.method === 'markViewed')); + + const viewed = loggedRequests('mirror.log').filter((request) => request.method === 'markViewed'); + expect(viewed.map((request) => request.params)).toEqual([{ url: PR_URL, token: TOKEN, paths: ['src/retry.py'] }]); + expect(stub.sessionRequests.at(-1)).toEqual({ id: 'github', scopes: ['repo'], createIfNone: false }); + + view.fireCheckboxChange([[partNodes(view)[0]!.node, TreeItemCheckboxState.Unchecked]]); + await until('the mark to be cleared', () => view.badge?.value === 7); + expect(loggedRequests('mirror.log').filter((request) => request.method === 'markViewed')).toHaveLength(1); + expect(stub.errorMessages).toEqual([]); + }); +}); diff --git a/packages/extension/test/package-smoke/run.ts b/packages/extension/test/package-smoke/run.ts index a6a707d..999a105 100644 --- a/packages/extension/test/package-smoke/run.ts +++ b/packages/extension/test/package-smoke/run.ts @@ -202,7 +202,7 @@ export async function run(): Promise { criteriaHeading?: string; }; }); - deepStrictEqual(requests.length, 2); + deepStrictEqual(requests.length, 3); ok(requests[0] && requests[0].method === 'initialize'); ok(requests[1] && requests[1].method === 'review'); // The request carries the agent choice and the criteria heading the @@ -215,6 +215,12 @@ export async function run(): Promise { agent: { agent: 'pi', model: '', account: '' }, criteriaHeading: 'Acceptance criteria', }); + // The review's marks are read from the engine's local store as soon + // as the review is under way, so the tree can show what the reviewer + // had already marked — the round trip's third and last request, and + // nothing else reaches the engine. + ok(requests[2] && requests[2].method === 'reviewedMarks'); + deepStrictEqual(requests[2]?.params, { url: PR_URL }); } finally { delete process.env['SECOND_LOOK_ENGINE_ENTRY']; delete process.env['FAKE_ENGINE_RESULT']; diff --git a/packages/extension/test/real-host/run.ts b/packages/extension/test/real-host/run.ts index 0d2402e..ce6af7c 100644 --- a/packages/extension/test/real-host/run.ts +++ b/packages/extension/test/real-host/run.ts @@ -291,7 +291,7 @@ export async function run(): Promise { .split('\n') .filter((line) => line !== '') .map((line) => JSON.parse(line) as EngineRequest); - deepStrictEqual(requests.length, 2); + deepStrictEqual(requests.length, 3); ok(requests[0] && requests[0].method === 'initialize'); ok(requests[1] && requests[1].method === 'review'); // The request carries the agent choice and the criteria heading the @@ -304,6 +304,12 @@ export async function run(): Promise { agent: { agent: 'pi', model: '', account: '' }, criteriaHeading: 'Acceptance criteria', }); + // The review's marks are read from the engine's local store as soon + // as the review is under way, so the tree can show what the reviewer + // had already marked — the round trip's third and last request, and + // nothing else reaches the engine. + ok(requests[2] && requests[2].method === 'reviewedMarks'); + deepStrictEqual(requests[2]?.params, { url: PR_URL }); // Reading a part: clicking it opens the multi-file diff with exactly // its files, read-only from the cached copies, scrolled to the part's diff --git a/packages/extension/test/tree.test.ts b/packages/extension/test/tree.test.ts index 23d7628..59ff899 100644 --- a/packages/extension/test/tree.test.ts +++ b/packages/extension/test/tree.test.ts @@ -1,18 +1,21 @@ import { describe, expect, it } from 'vitest'; -import type { AgentGrouping, AgentRanking, FileSlice, Hunk } from '@second-look/engine'; +import { NO_MARKS, applyMark, markedPart, type AgentGrouping, type AgentRanking, type FileSlice, type Hunk, type Part, type ReviewedMarks } from '@second-look/engine'; import { anchorOf, buildTree, + CHANGED_SINCE_MARKED, claimCountText, findingBadge, findAnchor, NOISE, NOT_RANKED_YET, partsInReadingOrder, + reviewBadge, reviewStatus, UNEXPLAINED_BADGE, } from '../src/tree.js'; import { claimsResult, judgedResult, mixedResult, part, result, unexplainedResult } from './results.js'; +import type { TreePart } from '../src/tree.js'; /** A hunk adding one line at the given place, on both sides. */ function hunkAt(oldStart: number, newStart: number): Hunk { @@ -361,3 +364,47 @@ describe('the ranking a tooltip names', () => { expect(buildTree(fellBack)[0]!.parts[0]!.tooltip).toMatch(/\nPlain ranking$/); }); }); + +describe('reviewed marks in the tree', () => { + const NOW = new Date('2026-10-06T12:00:00Z'); + 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)] }); + + function marked(...parts: Part[]): ReviewedMarks { + return parts.reduce((marks, each) => applyMark(marks, markedPart(each), true, NOW), NO_MARKS); + } + + function nodes(marks: ReviewedMarks, parts: Part[] = [cart, money]): TreePart[] { + return buildTree(result(parts), marks) + .flatMap((section) => section.parts) + .filter((node): node is TreePart => node.kind !== 'comment'); + } + + it('gives every part its reviewed state for its checkbox', () => { + expect(nodes(NO_MARKS).map((node) => node.reviewed)).toEqual(['not reviewed', 'not reviewed']); + expect(nodes(marked(cart)).map((node) => node.reviewed)).toEqual(['reviewed', 'not reviewed']); + }); + + it('says a part changed since it was marked, beside its label and in its tooltip', () => { + const edited = { ...cart, hunks: [{ ...hunkAt(10, 10), lines: [{ kind: 'addition' as const, newLineNumber: 10, text: 'edited' }] }] }; + const [node, other] = nodes(marked(cart, money), [edited, money]); + + expect(node!.reviewed).toBe('changed since marked'); + expect(node!.description).toBe(CHANGED_SINCE_MARKED); + expect(node!.tooltip).toContain('its content changed since you marked it reviewed'); + expect(other!.reviewed).toBe('reviewed'); + expect(other!.description).toBeUndefined(); + }); + + it('leaves a part whose lines only moved reviewed', () => { + const moved = { ...cart, hunks: [hunkAt(30, 30)] }; + + expect(nodes(marked(cart), [moved, money])[0]!.reviewed).toBe('reviewed'); + }); + + it('counts the parts left to review in the view badge, and shows none once all are reviewed', () => { + expect(reviewBadge(result([cart, money]), NO_MARKS)).toEqual({ value: 2, tooltip: '2 of 2 parts left to review' }); + expect(reviewBadge(result([cart, money]), marked(cart))).toEqual({ value: 1, tooltip: '1 of 2 parts left to review' }); + expect(reviewBadge(result([cart, money]), marked(cart, money))).toBeUndefined(); + }); +}); diff --git a/packages/extension/test/vscode-stub.ts b/packages/extension/test/vscode-stub.ts index 9caba3d..dab9d30 100644 --- a/packages/extension/test/vscode-stub.ts +++ b/packages/extension/test/vscode-stub.ts @@ -29,6 +29,12 @@ export interface StubTreeView { selection: unknown[]; /** The status line the extension shows above the tree. */ message?: string; + /** The badge the extension shows on the view. */ + badge?: { value: number; tooltip: string }; + /** The options the view was created with. */ + options?: Record; + /** Fires the checkbox change the way the editor does when the reviewer ticks or clears parts. */ + fireCheckboxChange(items: [unknown, number][]): void; } /** A sign-in session VS Code's authentication API returned. */ @@ -268,6 +274,7 @@ export class TreeItem { contextValue?: string; collapsibleState?: number; command?: { command: string; title: string; arguments?: unknown[] }; + checkboxState?: number; constructor(label?: string, collapsibleState?: number) { this.label = label; this.collapsibleState = collapsibleState; @@ -281,6 +288,12 @@ export const TreeItemCollapsibleState = { Expanded: 2, } as const; +/** The checkbox states a tree item shows. */ +export const TreeItemCheckboxState = { + Unchecked: 0, + Checked: 1, +} as const; + /** The event emitter the tree provider signals changes with. */ export class EventEmitter { private readonly listeners = new Set<(value: T) => void>(); @@ -525,12 +538,25 @@ export const window = { }, createTreeView(id: string, options: { treeDataProvider: unknown }): StubTreeView & StubDisposable { const revealed: { element: unknown; options?: unknown }[] = []; - const view: StubTreeView & StubDisposable & { reveal(element: unknown, options?: unknown): Promise } = { + const checkboxListeners = new Set<(event: { items: [unknown, number][] }) => void>(); + const view: StubTreeView & + StubDisposable & { + reveal(element: unknown, options?: unknown): Promise; + onDidChangeCheckboxState(listener: (event: { items: [unknown, number][] }) => void): StubDisposable; + } = { id, provider: options.treeDataProvider, revealed, selection: [], message: undefined, + options, + fireCheckboxChange: (items: [unknown, number][]): void => { + for (const listener of checkboxListeners) listener({ items }); + }, + onDidChangeCheckboxState: (listener: (event: { items: [unknown, number][] }) => void): StubDisposable => { + checkboxListeners.add(listener); + return { dispose: () => checkboxListeners.delete(listener) }; + }, reveal: (element: unknown, revealOptions?: unknown): Promise => { revealed.push({ element, options: revealOptions }); if ((revealOptions as { select?: boolean } | undefined)?.select) view.selection = [element];