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
12 changes: 8 additions & 4 deletions README.md

Large diffs are not rendered by default.

26 changes: 25 additions & 1 deletion packages/engine/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const USAGE = `second-look-engine — the engine of the Second Look reviewer's c
Usage:
second-look-engine review <pull-request-url> [--agent <pi|claude-code>]
[--model <model>] [--effort <level>] [--agent-timeout <seconds>]
[--token <token>] [--cache-dir <dir>]
[--criteria-heading <heading>] [--token <token>] [--cache-dir <dir>]
second-look-engine probe <pull-request-url> [--agent <pi|claude-code>] [--target <path-or-url>]...
[--model <model>] [--effort <level>] [--agent-timeout <seconds>]
[--agent-concurrency <n>] [--token <token>] [--cache-dir <dir>]
Expand Down Expand Up @@ -79,6 +79,16 @@ archives, in a per-pull-request cache: --cache-dir, else the
SECOND_LOOK_CACHE_DIR environment variable, else the platform's per-user
cache folder. Nothing is checked out and nothing from the pull request runs.

The review also reads the issues the pull request links — the closing
references GitHub returns, which cover the description's closing keywords
and the sidebar's "will close" links in this repository or another, and
the issues referencing the pull request — and lists each acceptance
criterion from the checklist under a heading, quoted and not checked.
Issue text is untrusted: it is parsed and never followed. The heading is
"Acceptance criteria" unless --criteria-heading names another; GitHub
returns no closing references for a pull request into a non-default
branch, and the result says so.

The GitHub token is passed in by the caller, either with --token or through
the GITHUB_TOKEN environment variable. It is used only for the GitHub
request, and is never written to disk or logs.
Expand Down Expand Up @@ -140,6 +150,9 @@ export interface CliDeps {
/** Flags that take a value, beyond --token and --cache-dir. */
const AGENT_FLAGS = ['--agent', '--target', '--model', '--effort', '--agent-timeout', '--agent-concurrency'];

/** The heading the acceptance criteria checklist sits under, as the review command names it. */
const CRITERIA_HEADING_FLAG = '--criteria-heading';

/**
* Runs the command line. Returns the process exit code: 0 on success,
* 1 on any error. Never throws, never prints the token.
Expand All @@ -153,6 +166,7 @@ export async function runCli(
const positional: string[] = [];
let tokenFlag: string | undefined;
let cacheDirFlag: string | undefined;
let criteriaHeading: string | undefined;
const agentFlags: Record<string, string[]> = {};
for (let i = 0; i < argv.length; i++) {
const arg = argv[i]!;
Expand All @@ -178,6 +192,15 @@ export async function runCli(
cacheDirFlag = value;
continue;
}
if (arg === CRITERIA_HEADING_FLAG) {
const value = argv[++i];
if (value === undefined || value.trim() === '') {
streams.err.write(`second-look-engine: ${CRITERIA_HEADING_FLAG} needs a heading\n`);
return 1;
}
criteriaHeading = value;
continue;
}
if (AGENT_FLAGS.includes(arg)) {
const value = argv[++i];
if (value === undefined) {
Expand Down Expand Up @@ -270,6 +293,7 @@ export async function runCli(
token,
fetch: deps.fetch,
cacheDir: cacheDirFlag ?? defaultCacheDir(env),
...(criteriaHeading !== undefined ? { criteriaHeading } : {}),
...(agentFlags['--agent'] !== undefined
? {
agentStage: {
Expand Down
144 changes: 144 additions & 0 deletions packages/engine/src/criteria.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
import type { GitHubClient, PullRequestRef } from './github.js';
import type { AcceptanceCriterion, Criteria, LinkedIssue } from './protocol.js';

/**
* The criteria pass: reads the issues the pull request links and lists
* each condition from the checklist under the configured heading, quoted
* and not checked. Model-free, like every plain pass. Issue text is
* untrusted: it is parsed and never followed, and the hidden content
* GitHub does not show stays in the quotes for the panel to flag.
*/

/** The heading the criteria checklist is read from under, unless configured otherwise. */
export const DEFAULT_CRITERIA_HEADING = 'Acceptance criteria';

/** The longest criterion quote kept, so an issue cannot flood the result. */
const MAX_QUOTE = 2000;

/** An ATX heading line, its text captured, closing hashes dropped. */
const HEADING = /^ {0,3}#{1,6}[ \t]+(.+?)[ \t]*#*[ \t]*$/;

/** One task-list item, its checkbox mark and its text captured. */
const TASK_ITEM = /^[ \t]*(?:[-*+]|\d+[.)])[ \t]+\[([ xX])\][ \t]*(.*)$/;

/** A heading's text, as the heading is matched against the configured one. */
function headingText(line: string): string | undefined {
return HEADING.exec(line)?.[1]?.trim();
}

/**
* Reads the checklist under one heading in an issue's body: every task
* item under the first matching heading that lists any, in the body's
* order — nested items and loose lists included — with room for plain
* words before the list starts. The section ends at another heading, or
* at the first line that is neither a task item nor blank once the list
* has started. A matching heading that lists nothing is passed over, so
* an issue that repeats the heading keeps the checklist that follows its
* later occurrence. Nothing in the body is followed; only its task items
* are quoted.
*/
export function checklistUnder(body: string, heading: string): { text: string; line: number }[] {
const lines = body.split(/\r?\n/);
const wanted = heading.trim().toLowerCase();
for (let at = 0; at < lines.length; at++) {
if (headingText(lines[at]!)?.toLowerCase() !== wanted) continue;
const items: { text: string; line: number }[] = [];
for (let scan = at + 1; scan < lines.length; scan++) {
const line = lines[scan]!;
// Blank lines run between the heading and its list, and between
// the items of a loose list.
if (line.trim() === '') continue;
// Another heading ends the section, before the list as after it.
if (headingText(line) !== undefined) break;
const match = TASK_ITEM.exec(line);
// Plain words before the first item introduce the list; after it,
// the section ends. A checkbox with no words lists nothing.
if (match === null) {
if (items.length > 0) break;
continue;
}
const text = match[2]?.replace(/\s+/g, ' ').trim();
if (text !== undefined && text !== '') items.push({ text, line: scan + 1 });
}
if (items.length > 0) return items;
}
return [];
}

/** One issue's checklist as criteria, each quoted and not checked. */
function criteriaOf(issue: number, body: string, heading: string): AcceptanceCriterion[] {
return checklistUnder(body, heading).map(({ text, line }) => ({
quote: text.length > MAX_QUOTE ? `${text.slice(0, MAX_QUOTE - 1)}…` : text,
issue,
line,
verdict: { kind: 'not checked' },
}));
}

/** Plain words for why something could not be read, the token never among them. */
function reason(error: unknown): string {
const status = typeof error === 'object' && error !== null ? (error as { status?: unknown }).status : undefined;
if (typeof status === 'number') return `GitHub answered ${status}`;
return error instanceof Error ? error.message : String(error);
}

/** How many of the linked issues the pull request closes. */
function closingCount(issues: readonly LinkedIssue[]): number {
return issues.filter((issue) => issue.link === 'closes').length;
}

/** One plain line saying what was read of the pull request's linked issues. */
function detailOf(issues: readonly LinkedIssue[], base: string, defaultBranch: string): string {
const closing = closingCount(issues);
const referencing = issues.length - closing;
const plural = (count: number, one: string, many: string): string => `${count} ${count === 1 ? one : many}`;
if (closing === 0 && base !== defaultBranch) {
const beside =
referencing === 0
? ', and no issue references it'
: `, while ${plural(referencing, 'issue', 'issues')} still reference${referencing === 1 ? 's' : ''} it`;
return `GitHub returns no closing references for a pull request into ${base}, not the repository's default branch ${defaultBranch}${beside}`;
}
if (closing === 0) {
return referencing === 0
? 'this pull request links no issue'
: `${plural(referencing, 'issue references', 'issues reference')} this pull request and none will be closed by it`;
}
const beside = referencing === 0 ? '' : ` and ${plural(referencing, 'issue', 'issues')} that reference${referencing === 1 ? 's' : ''} it`;
return `${plural(closing, 'issue', 'issues')} this pull request closes${beside}`;
}

/**
* Reads the acceptance criteria of one pull request: the issues it links
* — closing references, sidebar links and issues in other repositories —
* and each checklist under the heading. Issues that cannot be read never
* fail the review: the result says why. Each criterion starts as not
* checked; judging them is a later pass.
*/
export async function readCriteria(
client: GitHubClient,
ref: PullRequestRef,
base: string,
heading: string,
): Promise<Criteria> {
let linked: { defaultBranch: string; issues: LinkedIssue[] };
try {
linked = await client.getLinkedIssues(ref);
} catch (error) {
return {
outcome: 'unreadable',
detail: `the linked issues could not be read: ${reason(error)}`,
heading,
issues: [],
criteria: [],
};
}
const issues = linked.issues;
return {
outcome: 'read',
detail: detailOf(issues, base, linked.defaultBranch),
heading,
issues,
criteria: issues.flatMap((issue, index) => criteriaOf(index, issue.body, heading)),
};
}
158 changes: 157 additions & 1 deletion packages/engine/src/github.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { Octokit } from '@octokit/rest';
import type { PositionedComment } from './positions.js';
import type { CheckAnnotation, PullRequestSummary, SentReview, SubmitKind } from './protocol.js';
import type {
CheckAnnotation,
LinkedIssue,
PullRequestSummary,
SentReview,
SubmitKind,
} from './protocol.js';

/** The parts of a pull request URL the engine needs. */
export interface PullRequestRef {
Expand Down Expand Up @@ -152,6 +158,55 @@ export class GitHubClient {
throw new Error(`the log of job ${jobId} arrived without a body`);
}

/**
* Reads the issues the pull request links, through GitHub's GraphQL
* side, with one query: the closing references — which cover the
* description's closing keywords and the sidebar's "will close"
* links, in this repository or another, and which GitHub returns only
* for a pull request into the repository's default branch — and the
* issues the pull request's own timeline shows referencing it, a
* sidebar link or a mention. An issue both closes and is referenced
* reads once, as a closing reference. Nothing is written.
*/
async getLinkedIssues(ref: PullRequestRef): Promise<LinkedReferences> {
const answer = linkedAnswer(
await this.requestGraphql(LINKED_ISSUES_QUERY, {
owner: ref.owner,
name: ref.repo,
number: ref.number,
}),
);
const pullRequest = answer.repository?.pullRequest;
const branch = answer.repository?.defaultBranchRef?.name;
const issues: LinkedIssue[] = [];
const seen = new Set<string>();
for (const node of pullRequest?.closingIssuesReferences?.nodes ?? []) {
const issue = linkedIssue(node, 'closes');
if (issue === undefined) continue;
seen.add(`${issue.repository}#${issue.number}`);
issues.push(issue);
}
for (const node of pullRequest?.timelineItems?.nodes ?? []) {
const issue = linkedIssue(node?.source, 'references');
if (issue === undefined || seen.has(`${issue.repository}#${issue.number}`)) continue;
issues.push(issue);
}
return { defaultBranch: typeof branch === 'string' ? branch : '', issues };
}

/**
* Sends one GraphQL query through the same client, so it carries the
* same token, the same silent log and the same fetch as every REST
* call, and hands back its parsed answer as it arrived: the envelope
* with the query's `data` and any `errors`, so a query that failed —
* GitHub answers some failures as HTTP 200 with `data: null` — is
* seen by whoever reads the answer.
*/
private async requestGraphql(query: string, variables: Record<string, unknown>): Promise<unknown> {
const response: { data: unknown } = await this.octokit.request('POST /graphql', { query, variables });
return response.data;
}

/**
* Finds the merge base of the base and head commits: the version the
* pull request's diff is computed against, so the base copy is taken
Expand Down Expand Up @@ -286,6 +341,107 @@ export class GitHubClient {
/** The most annotations the companion reads of one check run. */
const MAX_ANNOTATIONS = 50;

/** The most closing issues one query reads; a pull request closing more lists none beyond these. */
const MAX_CLOSING_ISSUES = 50;

/** The most cross-references one query reads from the pull request's own timeline. */
const MAX_TIMELINE_LINKS = 100;

/**
* The issues the pull request links, with the repository's default
* branch: GitHub returns closing references only for a pull request
* into that branch, so a review needs the branch to say why none came
* back.
*/
export interface LinkedReferences {
/** The repository's default branch, as the answer names it. */
defaultBranch: string;
/** The linked issues: closing references first, then the issues referencing the pull request, in GitHub's order. */
issues: LinkedIssue[];
}

/**
* The query that reads the pull request's closing references and the
* issues referencing it in one GraphQL round trip. The closing
* references cover the description's closing keywords and the sidebar's
* "will close" links, in this repository or another; the timeline's
* cross-references are the issues that reference the pull request
* without closing, such as a plain sidebar link.
*/
const LINKED_ISSUES_QUERY = `query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
defaultBranchRef { name }
pullRequest(number: $number) {
closingIssuesReferences(first: ${MAX_CLOSING_ISSUES}) {
nodes { number title url body repository { nameWithOwner } }
}
timelineItems(first: ${MAX_TIMELINE_LINKS}, itemTypes: CROSS_REFERENCED_EVENT) {
nodes { ... on CrossReferencedEvent { source { ... on Issue { number title url body repository { nameWithOwner } } } } }
}
}
}
}`;

/** A GraphQL answer that failed, with the message of its first error. */
function linkedAnswer(response: unknown): GraphQLData {
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}` : ''}`,
);
}
return body.data ?? {};
}

/** 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;
const value = node as Record<string, unknown>;
const repository =
typeof value['repository'] === 'object' && value['repository'] !== null
? (value['repository'] as { nameWithOwner?: unknown }).nameWithOwner
: undefined;
if (
typeof value['number'] !== 'number' ||
typeof value['title'] !== 'string' ||
typeof value['url'] !== 'string' ||
typeof repository !== 'string'
) {
return undefined;
}
return {
number: value['number'],
title: value['title'],
url: value['url'],
repository,
body: typeof value['body'] === 'string' ? value['body'] : '',
link,
};
}

/** The data the linked-issues query asks for, each field optional as GitHub leaves it. */
interface GraphQLData {
repository?: {
defaultBranchRef?: { name?: unknown };
pullRequest?: {
closingIssuesReferences?: { nodes?: unknown[] };
timelineItems?: { nodes?: ({ source?: unknown } | undefined)[] };
};
};
}

/**
* The envelope one GraphQL query answers with: the query's data, and
* the errors that make it fail — a rate limit or a permission error
* among them — which GitHub can send with HTTP 200 and `data: null`.
*/
interface GraphQLAnswer {
errors?: { message?: unknown }[];
data?: GraphQLData | null;
}

/** One check run as GitHub lists it, before its annotations and log are read. */
export interface CheckRunListing {
id: number;
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 @@ -43,4 +43,5 @@ export * from './repository-fetch.js';
export * from './library-verdicts.js';
export * from './pipeline.js';
export * from './ci.js';
export * from './criteria.js';
export { runCli } from './cli.js';
Loading
Loading