Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions README.md

Large diffs are not rendered by default.

57 changes: 49 additions & 8 deletions packages/engine/src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,47 @@ export class GitHubClient {
return data.merge_base_commit.sha;
}

/**
* Fetches the diff a head commit makes against its merge base with a
* base commit, the way the pull request's own diff is taken; null when
* GitHub cannot serve the comparison: it no longer has either commit,
* such as one a force-push left behind, or the two share no history.
*/
async getChangeDiff(ref: PullRequestRef, base: string, head: string): Promise<string | null> {
let response;
try {
response = await this.octokit.repos.compareCommitsWithBasehead({
owner: ref.owner,
repo: ref.repo,
basehead: `${base}...${head}`,
mediaType: { format: 'diff' },
});
} catch (error) {
if (isNotFound(error) || (error as { status?: unknown }).status === 422) return null;
throw error;
}
return diffText(response.data);
}

/**
* The commit and time of the signed-in reviewer's last submitted review
* of the pull request; null when they have submitted none. A pending
* review is not submitted, so it never counts.
*/
async getLastReviewedCommit(ref: PullRequestRef): Promise<{ commit: string; at: string } | null> {
const { data: user } = await this.octokit.users.getAuthenticated();
const reviews = await this.octokit.paginate(this.octokit.pulls.listReviews, {
owner: ref.owner,
repo: ref.repo,
pull_number: ref.number,
per_page: 100,
});
const last = reviews
.filter((review) => review.user?.login === user.login && review.state !== 'PENDING' && review.commit_id && review.submitted_at)
.at(-1);
return last === undefined ? null : { commit: last.commit_id!, at: last.submitted_at! };
}

/**
* Streams the gzipped tarball of one commit. The archive is downloaded,
* never checked out, and the body is handed over unparsed so a large
Expand Down Expand Up @@ -255,14 +296,7 @@ export class GitHubClient {
pull_number: ref.number,
mediaType: { format: 'diff' },
});
// The diff media type is not JSON; the client hands over raw bytes.
if (typeof response.data === 'string') {
return response.data;
}
if (response.data instanceof ArrayBuffer) {
return new TextDecoder().decode(response.data);
}
return String(response.data);
return diffText(response.data);
}

/**
Expand Down Expand Up @@ -493,6 +527,13 @@ export interface CheckRunListing {
app: string | null;
}

/** A diff as text: the diff media type is not JSON, so the client hands over raw bytes. */
function diffText(data: unknown): string {
if (typeof data === 'string') return data;
if (data instanceof ArrayBuffer) return new TextDecoder().decode(data);
return String(data);
}

/** True when the error is the endpoint's plain 404. */
function isNotFound(error: unknown): boolean {
return (
Expand Down
1 change: 1 addition & 0 deletions packages/engine/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,5 @@ export * from './criteria.js';
export * from './criteria-mapping.js';
export * from './draft-comment.js';
export * from './reviewed-marks.js';
export * from './last-look.js';
export { runCli } from './cli.js';
158 changes: 158 additions & 0 deletions packages/engine/src/last-look.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
import { randomBytes } from 'node:crypto';
import { mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises';
import { join } from 'node:path';
import { pullRequestCacheDir } from './cache.js';
import { parseDiff } from './diff.js';
import type { GitHubClient, PullRequestRef } from './github.js';
import type { Hunk, Part, PullRequestSummary, SinceLastLook } from './protocol.js';
import { lineContent, pieceHashes } from './reviewed-marks.js';

/** The file in a pull request's cache folder that records the reviewer's looks. */
export const LAST_LOOK_FILE = 'last-look.json';

/** One look: the head commit a review was opened at, and when. */
export interface Look {
commit: string;
/** When the review was opened, as an ISO 8601 timestamp. */
at: string;
}

/**
* The store's record: the latest look, and the look before it at another
* commit, so opening the review again at the same commit still shows
* what changed since the look before.
*/
interface LookRecord {
version: 1;
last: Look;
before?: Look;
}

const COMMIT = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/;

function isLook(value: unknown): value is Look {
if (typeof value !== 'object' || value === null) return false;
const { commit, at } = value as Record<string, unknown>;
return typeof commit === 'string' && COMMIT.test(commit) && typeof at === 'string';
}

function lookPath(cacheDir: string, ref: PullRequestRef): string {
return join(pullRequestCacheDir(cacheDir, ref), LAST_LOOK_FILE);
}

/**
* Reads the record of the reviewer's looks at a pull request; null when
* none was written yet or it is not a record, which is then left out
* rather than trusted.
*/
export async function readLooks(cacheDir: string, ref: PullRequestRef): Promise<LookRecord | null> {
let text: string;
try {
text = await readFile(lookPath(cacheDir, ref), 'utf8');
} catch (error) {
if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null;
throw error;
}
let stored: unknown;
try {
stored = JSON.parse(text);
} catch {
return null;
}
const { last, before } = (stored ?? {}) as Partial<LookRecord>;
if (!isLook(last)) return null;
return { version: 1, last, ...(isLook(before) && before.commit !== last.commit ? { before } : {}) };
}

/**
* Records a look at a pull request: it becomes the latest, and the
* latest before it becomes the look before when it was at another
* commit. The record lands whole: written beside the store, then renamed
* over it.
*/
export async function recordLook(cacheDir: string, ref: PullRequestRef, look: Look): Promise<void> {
const previous = await readLooks(cacheDir, ref);
const before = previous === null ? undefined : previous.last.commit === look.commit ? previous.before : previous.last;
const record: LookRecord = { version: 1, last: look, ...(before === undefined ? {} : { before }) };
const path = lookPath(cacheDir, ref);
await mkdir(pullRequestCacheDir(cacheDir, ref), { recursive: true });
const partial = `${path}.partial-${randomBytes(6).toString('hex')}`;
try {
await writeFile(partial, `${JSON.stringify(record, null, 2)}\n`, 'utf8');
await rename(partial, path);
} catch (error) {
await rm(partial, { force: true });
throw error;
}
}

/** A hunk's changed lines alone, so context that upstream changed around them leaves the piece alone. */
function changedLines(hunk: Hunk): string[] {
return hunk.lines.filter((line) => line.kind !== 'context').map(lineContent);
}

/**
* The content hashes of a part's pieces as the comparison with the last
* look reads them: each hunk by its changed lines alone, without their
* numbers or context, and each file without hunks by its blob ids.
* Identical hunks share a hash, so a piece reads the same whichever part
* of its file holds it.
*/
export function changePieces(part: Part): string[] {
return pieceHashes(part, changedLines, false);
}

/** Whether a part changed since the reviewer's last look: every part did when the changes could not be compared. */
export function changedSinceLastLook(part: Part, since: SinceLastLook): boolean {
if (since.outcome === 'not compared') return true;
const changed = new Set(since.changed);
return changePieces(part).some((piece) => changed.has(piece));
}

/** What the comparison needs: the GitHub client, the cache folder and the pull request as it is now. */
export interface LastLookOptions {
client: GitHubClient;
cacheDir: string;
ref: PullRequestRef;
pullRequest: PullRequestSummary;
/** The pull request's full diff now. */
diff: string;
/** When this look happens. */
now?: Date;
}

/**
* Compares the change with the reviewer's last look, then records this
* look. The last look is the latest one the local record holds at
* another commit, or, with none, the commit of the reviewer's last
* submitted GitHub review; with neither, this is their first look and
* there is nothing to compare. The change at that commit is taken
* against its merge base with the base now — the way the pull request's
* own diff is — so after a rebase or a force-push the two changes are
* compared themselves, and upstream commits mix in nothing. When the
* change at that commit cannot be fetched — its commit gone from
* GitHub, or sharing no history with the head — every part counts as
* changed.
*/
export async function lookSinceLastLook(options: LastLookOptions): Promise<SinceLastLook | undefined> {
const { client, cacheDir, ref, pullRequest } = options;
const head = pullRequest.headSha;
const looks = await readLooks(cacheDir, ref);
const local = looks === null ? undefined : looks.last.commit !== head ? looks.last : looks.before;
const reviewed = local === undefined ? await client.getLastReviewedCommit(ref) : null;
const last = local ?? reviewed ?? undefined;
const since = last === undefined ? undefined : await compareWith(last, local === undefined ? 'github review' : 'local record', options);
await recordLook(cacheDir, ref, { commit: head, at: (options.now ?? new Date()).toISOString() });
return since;
}

async function compareWith(last: Look, from: SinceLastLook['from'], options: LastLookOptions): Promise<SinceLastLook> {
const { client, ref, pullRequest } = options;
const look = { commit: last.commit, from, at: last.at };
if (last.commit === pullRequest.headSha) return { ...look, outcome: 'compared', changed: [] };
const before = await client.getChangeDiff(ref, pullRequest.baseCommit, last.commit);
if (before === null) return { ...look, outcome: 'not compared', changed: [] };
const held = new Set(parseDiff(before).files.flatMap(changePieces));
const now = parseDiff(options.diff).files.flatMap(changePieces);
return { ...look, outcome: 'compared', changed: [...new Set(now.filter((piece) => !held.has(piece)))] };
}
33 changes: 31 additions & 2 deletions packages/engine/src/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import type { AgentStamp } from './agent.js';

/** Version of the review result schema. */
export const REVIEW_RESULT_VERSION = 15 as const;
export const REVIEW_RESULT_VERSION = 16 as const;

/**
* Version 2 added the head commit's SHA and each part's noise assessment;
Expand Down Expand Up @@ -40,7 +40,8 @@ export const REVIEW_RESULT_VERSION = 15 as const;
* that the diff does not contain; version 15 added each acceptance
* criterion's verdict — met, partly met, not met, can't tell or needs
* manual check — with the code, the tests and the manual checks that
* show it, and the criteria's mapping.
* show it, and the criteria's mapping; version 16 added what changed
* since the reviewer's last look.
*/
export type ReviewResultVersion = typeof REVIEW_RESULT_VERSION;

Expand Down Expand Up @@ -215,6 +216,32 @@ export interface ViewedFiles {
paths: string[];
}

/**
* What changed since the reviewer's last look: the head commit they last
* opened a review at, or, with no local record of one, the commit of
* their last submitted GitHub review, and the pieces of this change the
* change at that commit did not hold. The two changes are compared
* themselves, each against its own merge base, so a rebase or a
* force-push mixes in nothing from upstream.
*/
export interface SinceLastLook {
/** The head commit at the last look. */
commit: string;
/** Where the last look comes from: the local record of the reviews opened, or the reviewer's last submitted GitHub review. */
from: 'local record' | 'github review';
/** When the last look was, as an ISO 8601 timestamp. */
at: string;
/**
* **compared** when the change at that commit was compared with this
* one; **not compared** when it could not be — that commit gone from
* GitHub, or no longer related to the head — so every part counts as
* changed.
*/
outcome: 'compared' | 'not compared';
/** The content hashes of this change's pieces — each hunk's changed lines, or a file without hunks — the change at the last look did not hold; empty when the changes could not be compared. */
changed: string[];
}

/** The review result the engine produces for one pull request. */
export interface ReviewResult {
/** Schema version; compare against {@link REVIEW_RESULT_VERSION}. */
Expand Down Expand Up @@ -255,6 +282,8 @@ export interface ReviewResult {
pipeline: PipelineReport;
/** The CI the companion read at the head commit; absent when the review read none, such as an offline replay. */
ci?: CiResults;
/** What changed since the reviewer's last look; absent on their first look, or when the review was not opened by them, such as an offline replay. */
sinceLastLook?: SinceLastLook;
}

/**
Expand Down
16 changes: 12 additions & 4 deletions packages/engine/src/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import { validateCoverage } from './coverage.js';
import { parseDiff, type ParsedDiff } from './diff.js';
import { GitHubClient, parsePullRequestUrl } from './github.js';
import { groupingItems, groupWithAgent } from './grouping.js';
import { lookSinceLastLook } from './last-look.js';
import { offerLibraryFetches } from './library-fetch.js';
import { confirmLockfileNoise } from './lockfile.js';
import { applyNoiseRules } from './noise.js';
import { groupParts } from './parts.js';
import { pipelineClaims, readPipelineReport } from './pipeline.js';
import { REVIEW_RESULT_VERSION } from './protocol.js';
import type { ChangeCopies, CiResults, Criteria, Part, PullRequestSummary, ReviewResult } from './protocol.js';
import type { ChangeCopies, CiResults, Criteria, Part, PullRequestSummary, ReviewResult, SinceLastLook } from './protocol.js';
import { rankParts } from './rank.js';
import {
RANKING_PROMPT_VERSION,
Expand Down Expand Up @@ -51,6 +52,8 @@ export interface ReviewOptions {
criteriaHeading?: string;
/** Asks the agent to group and rank the parts, write the story, compare the change with its description and issues, list the claims and judge them, and map the acceptance criteria too, after the plain pass; see {@link reviewChange}. */
agentStage?: AgentStageOptions;
/** True when the reviewer opened this review: it is compared with their last look, then recorded as the latest; see {@link lookSinceLastLook}. */
lastLook?: boolean;
}

/**
Expand All @@ -71,6 +74,8 @@ export interface ReviewInput {
ci?: CiResults;
/** The acceptance criteria of the linked issues; absent when none were read, such as an offline replay. */
criteria?: Criteria;
/** What changed since the reviewer's last look; absent on their first look, or when none was asked for. */
sinceLastLook?: SinceLastLook;
}

/**
Expand All @@ -96,7 +101,8 @@ export async function reviewPullRequest(
* checkout), read-only copies of the base and head versions, the CI at
* the head commit — each failed job's log trimmed to its failing step —
* and the acceptance criteria of the issues the pull request links,
* quoted from the checklist under the configured heading.
* quoted from the checklist under the configured heading; and, when the
* reviewer opened the review, what changed since their last look.
*/
export async function fetchChange(url: string, options: ReviewOptions): Promise<ReviewInput> {
const ref = parsePullRequestUrl(url);
Expand Down Expand Up @@ -126,13 +132,14 @@ export async function fetchChange(url: string, options: ReviewOptions): Promise<
download: (wanted) => client.downloadTarball(ref, wanted),
});
const heading = options.criteriaHeading?.trim();
const [base, head, ci, criteria] = await Promise.all([
const [base, head, ci, criteria, sinceLastLook] = await Promise.all([
copy(mergeBase),
copy(pullRequest.headSha),
readCi(client, ref, pullRequest.headSha, mergeCommit),
readCriteria(client, ref, pullRequest.base, heading === undefined || heading === '' ? DEFAULT_CRITERIA_HEADING : heading),
options.lastLook ? lookSinceLastLook({ client, cacheDir: options.cacheDir, ref, pullRequest, diff }) : undefined,
]);
return { pullRequest, diff, gitAttributes, copies: { base, head }, ci, criteria };
return { pullRequest, diff, gitAttributes, copies: { base, head }, ci, criteria, ...(sinceLastLook ? { sinceLastLook } : {}) };
}

/**
Expand Down Expand Up @@ -228,6 +235,7 @@ export async function reviewChange(
...(input.criteria ? { criteria: input.criteria } : {}),
pipeline: readPipelineReport(input.pullRequest.description, input.pullRequest.headSha),
...(input.ci ? { ci: input.ci } : {}),
...(input.sinceLastLook ? { sinceLastLook: input.sinceLastLook } : {}),
};
if (!agentStage) return plain;
const ranked = await groupAndRank(plain, agentStage, input, parsed, files);
Expand Down
Loading
Loading