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
2 changes: 1 addition & 1 deletion CONTEXT.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ _Avoid_: Dependency sync, auto-fetch, install
### Acting on the review

**Acceptance criterion**:
One condition from the linked issue that the change must meet, proven by code, automated tests, or a manual check the PR reports.
One condition from the linked issue that the change must meet, proven by code, automated tests, or a manual check the PR reports; its verdict is **met**, **partly met**, **not met**, **can't tell**, or **needs manual check**, always with the code, tests and manual checks that show it.
_Avoid_: Requirement, AC item

**Manual check**:
Expand Down
14 changes: 8 additions & 6 deletions README.md

Large diffs are not rendered by default.

19 changes: 13 additions & 6 deletions packages/engine/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,18 @@ behaves — from the description, the docstrings and comments the change
adds, and the story — each quoted from its source and attached to a part;
a quote not found in its source rejects the answer, which is retried once
before the result says why the agent listed none. A fresh pipeline
report's open findings are claims too, listed first by the engine. Last,
report's open findings are claims too, listed first by the engine. Then
the agent judges each claim against the change, the head copy and, when
a check failed, its trimmed CI log: verified, refuted or unverifiable,
with its evidence source and the lines it cites, each of which the engine
re-reads; a citation that does not match, or the
model's memory alone, keeps a claim from verified. Each stage is
announced on stderr while the agent works.
model's memory alone, keeps a claim from verified. Last, the agent maps
each acceptance criterion of the linked issues to the change: met, partly
met, not met, can't tell or needs manual check, with the code that
implements it and the tests that cover it, each a line the engine
re-reads, and the manual checks the description reports, each a quote
the engine finds there; one that does not match makes the criterion
can't tell. Each stage is announced on stderr while the agent works.

It keeps read-only copies of the base and head versions, downloaded as
archives, in a per-pull-request cache: --cache-dir, else the
Expand All @@ -90,8 +95,9 @@ 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
criterion from the checklist under a heading, quoted and not checked
until the agent maps it. Issue text is untrusted: it is parsed and never
followed, and the agent reads it only as marked untrusted text. 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.
Expand Down Expand Up @@ -132,7 +138,8 @@ 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 judged result. Each review
judges them, then the result with the verdicts in another while the agent
maps the acceptance criteria, then the mapped result. Each review
request may also carry the reviewer's agent choice — the agent, model and
account from the editor's settings — which runs that review's agent passes
and stamps the account label on their results, replacing this command's
Expand Down
378 changes: 378 additions & 0 deletions packages/engine/src/criteria-mapping.ts

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions packages/engine/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,5 @@ export * from './library-verdicts.js';
export * from './pipeline.js';
export * from './ci.js';
export * from './criteria.js';
export * from './criteria-mapping.js';
export { runCli } from './cli.js';
94 changes: 83 additions & 11 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 = 14 as const;
export const REVIEW_RESULT_VERSION = 15 as const;

/**
* Version 2 added the head commit's SHA and each part's noise assessment;
Expand All @@ -37,7 +37,10 @@ export const REVIEW_RESULT_VERSION = 14 as const;
* under the configured heading and not checked; version 14 added the
* unexplained changes in both directions: the parts neither the
* description nor a linked issue explains, and the changes they describe
* that the diff does not contain.
* 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.
*/
export type ReviewResultVersion = typeof REVIEW_RESULT_VERSION;

Expand Down Expand Up @@ -170,8 +173,8 @@ export interface ReviewResult {
unexplained?: UnexplainedChanges;
/**
* The acceptance criteria read from the issues the pull request links,
* each quoted and not checked; absent when the review read none, such
* as an offline replay.
* each quoted, with its verdict once the agent mapped it to the change;
* absent when the review read none, such as an offline replay.
*/
criteria?: Criteria;
/** The no-mistakes report the description carries, and whether it is trusted. */
Expand Down Expand Up @@ -203,8 +206,8 @@ export interface LinkedIssue {
/**
* One condition from a linked issue that the change must meet (the
* glossary's acceptance criterion), quoted from the checklist under the
* configured heading. Criteria start as not checked; judging them
* against the change is a later pass.
* configured heading. Criteria start as not checked, and keep that
* verdict until the agent maps them to the change.
*/
export interface AcceptanceCriterion {
/**
Expand All @@ -217,8 +220,74 @@ export interface AcceptanceCriterion {
issue: number;
/** The 1-based line of the issue's body the quote sits on. */
line: number;
/** The criterion's verdict: not checked until a later pass judges it. */
verdict: { kind: 'not checked' };
/** The criterion's verdict: not checked until the agent maps it to the change. */
verdict: CriterionVerdict;
}

/** A mapped criterion's verdict kind: what judging a criterion against the change can come to. */
export type CriterionVerdictKind = 'met' | 'partly met' | 'not met' | "can't tell" | 'needs manual check';

/** The mapped criterion verdict kinds, the order a reviewer reads them in: what needs them first. */
export const CRITERION_VERDICT_KINDS: readonly CriterionVerdictKind[] = ['not met', 'partly met', 'needs manual check', "can't tell", 'met'];

/**
* A manual check (the glossary's): verification a person performed and
* the pull request reports, such as steps followed, what they saw, or a
* measurement, quoted from the description where it is reported.
*/
export interface ManualCheck {
/**
* The report, exactly as the description has it, on one line: runs of
* white space as one space, and each line's leading quote marker
* dropped.
*/
quote: string;
/** The 1-based line of the description the quote starts on. */
line: number;
}

/**
* Whether the change meets an acceptance criterion, and where. A
* criterion is listed before the agent maps it, so it starts as not
* checked; once mapped, it is met, partly met, not met, can't tell or
* needs manual check, always with its reason and the evidence that shows
* it: the code that implements it and the automated tests that cover it,
* each a line of the head copy the engine re-read, and the manual checks
* the description reports, each a quote the engine found there.
*/
export type CriterionVerdict =
| { kind: 'not checked' }
| {
kind: CriterionVerdictKind;
/** One plain line saying why the criterion has this verdict. */
reason: string;
/** The lines of the head copy that implement the criterion, each re-checked by the engine. */
code: Citation[];
/** The lines of the head copy's automated tests that cover it, each re-checked by the engine. */
tests: Citation[];
/** The manual checks the description reports for it, each found there by the engine. */
manualChecks: ManualCheck[];
/** Why the engine dropped the agent's verdict to can't tell, when it did. */
recheck?: string;
};

/**
* The criteria mapping's outcome: the agent judged each criterion against
* the change, the read-only copy and the description, and the engine
* re-checked every citation and manual check.
*/
export interface CriteriaMapping {
/** The version of the criteria-mapping prompt. */
promptVersion: string;
/**
* `mapped` when the criteria carry the agent's verdicts, as the re-check
* left them; `fell back` when its answer was missing or invalid, so
* every criterion stays not checked.
*/
outcome: 'mapped' | 'fell back';
/** One plain line: how the verdicts were checked, or why there are none. */
detail: string;
stamp: AgentStamp;
}

/**
Expand All @@ -227,8 +296,9 @@ export interface AcceptanceCriterion {
* request's own closing keywords and the sidebar's "will close" links,
* and the issues its timeline shows referencing it, in this repository
* or another — and lists each criterion found in the checklist under
* the configured heading, quoted and not checked. Model-free: issue
* text is parsed, never followed.
* the configured heading, quoted and not checked. Reading them is
* model-free: issue text is parsed, never followed. The agent then maps
* each criterion to the change, and its mapping says what came of it.
*/
export interface Criteria {
/** `read` when the linked issues were read; `unreadable` when GitHub refused or failed. */
Expand All @@ -239,8 +309,10 @@ export interface Criteria {
heading: string;
/** The issues the pull request links, closing references first, in GitHub's order; empty when none was read. */
issues: LinkedIssue[];
/** The criteria, each quoted and not checked; empty when no checklist was found. */
/** The criteria, each quoted, and not checked until mapped; empty when no checklist was found. */
criteria: AcceptanceCriterion[];
/** What came of asking the agent to map the criteria to the change; absent until it was asked. */
mapping?: CriteriaMapping;
}

/**
Expand Down
57 changes: 46 additions & 11 deletions packages/engine/src/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { ensureCopy } from './cache.js';
import { readCi } from './ci.js';
import { findClaims } from './claims.js';
import { DEFAULT_CRITERIA_HEADING, readCriteria } from './criteria.js';
import { mapCriteria } from './criteria-mapping.js';
import { validateCoverage } from './coverage.js';
import { parseDiff, type ParsedDiff } from './diff.js';
import { GitHubClient, parsePullRequestUrl } from './github.js';
Expand Down Expand Up @@ -48,7 +49,7 @@ export interface ReviewOptions {
cacheDir: string;
/** The heading the acceptance criteria checklist sits under in a linked issue; "Acceptance criteria" when absent. */
criteriaHeading?: string;
/** Asks the agent to group and rank the parts, write the story, compare the change with its description and issues, and list the claims and judge them too, after the plain pass; see {@link reviewChange}. */
/** 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;
}

Expand Down Expand Up @@ -136,9 +137,10 @@ export async function fetchChange(url: string, options: ReviewOptions): Promise<

/**
* The agent stages, grouping, ranking, the story, the unexplained
* changes, the claims then their verdicts, when a review asks the agent
* to group and rank the parts, write the story, compare the change with
* its description and linked issues, list the claims and judge them too.
* changes, the claims then their verdicts, and the criteria mapping, when
* a review asks the agent to group and rank the parts, write the story,
* compare the change with its description and linked issues, list the
* claims and judge them, and map the acceptance criteria to the change too.
*/
export interface AgentStageOptions {
adapter: AgentAdapter;
Expand All @@ -153,8 +155,10 @@ export interface AgentStageOptions {
unexplained?: boolean;
/** Whether the agent lists the claims; true when absent. */
claims?: boolean;
/** Whether the agent judges the claims it listed, last; true when absent. */
/** Whether the agent judges the claims it listed; true when absent. */
verdicts?: boolean;
/** Whether the agent maps the acceptance criteria to the change, last; true when absent. */
criteria?: boolean;
}

/** A stage of the review starting, with the result so far. */
Expand All @@ -163,7 +167,7 @@ export interface ReviewStage {
running: string;
/** The stage ends within this many milliseconds. */
timeoutMs: number;
/** The result so far: the plain pass's, then the grouping, ranking, story, unexplained changes, claims and verdicts stages' in turn. */
/** The result so far: the plain pass's, then the grouping, ranking, story, unexplained changes, claims, verdicts and criteria stages' in turn. */
result: ReviewResult;
}

Expand Down Expand Up @@ -193,8 +197,9 @@ function coverageProblems(diff: ParsedDiff, parts: Part[]): string | undefined {
* writes their story, see {@link storyStage}, while it compares the
* change with its description and linked issues, see
* {@link unexplainedStage}, while it lists the claims the change makes,
* see {@link claimsStage}, and last while it judges them, see
* {@link verdictsStage}.
* see {@link claimsStage}, while it judges them, see
* {@link verdictsStage}, and last while it maps the acceptance criteria
* to the change, see {@link criteriaStage}.
*/
export async function reviewChange(
input: ReviewInput,
Expand Down Expand Up @@ -228,9 +233,9 @@ export async function reviewChange(
const ranked = await groupAndRank(plain, agentStage, input, parsed, files);
const told = agentStage.story === false ? ranked : await storyStage(ranked, agentStage, input);
const compared = agentStage.unexplained === false ? told : await unexplainedStage(told, agentStage, input);
if (agentStage.claims === false) return compared;
const claimed = await claimsStage(compared, agentStage, input);
return agentStage.verdicts === false ? claimed : verdictsStage(claimed, agentStage, input);
const claimed = agentStage.claims === false ? compared : await claimsStage(compared, agentStage, input);
const judged = agentStage.claims === false || agentStage.verdicts === false ? claimed : await verdictsStage(claimed, agentStage, input);
return agentStage.criteria === false ? judged : criteriaStage(judged, agentStage, input);
}

/**
Expand Down Expand Up @@ -443,3 +448,33 @@ async function verdictsStage(
const offered = await offerLibraryFetches(judged.claims, input.copies.head.path);
return { ...shown, claims: { ...claims, ...judged, claims: offered } };
}

/**
* The criteria stage, last: the agent maps each acceptance criterion read
* from the linked issues to the change — met, partly met, not met, can't
* tell or needs manual check — citing the code that implements it and the
* tests that cover it, which the engine re-reads in the head copy, and
* quoting the manual checks the description reports, which the engine
* finds there. No criteria, or a change with no parts, need no mapping.
*/
async function criteriaStage(
shown: ReviewResult,
agentStage: AgentStageOptions,
input: ReviewInput,
): Promise<ReviewResult> {
const criteria = shown.criteria;
if (criteria === undefined || criteria.criteria.length === 0 || shown.parts.length === 0) return shown;
const settings = agentStage.settings ?? DEFAULT_AGENT_SETTINGS;
agentStage.onStage?.({
running: `mapping the acceptance criteria with ${agentStage.adapter.agent}`,
timeoutMs: agentStageTimeoutMs(settings),
result: shown,
});
const mapped = await mapCriteria(shown.parts, criteria, {
adapter: agentStage.adapter,
settings,
root: input.copies.head.path,
pullRequest: input.pullRequest,
});
return { ...shown, criteria: { ...criteria, criteria: mapped.criteria, mapping: mapped.mapping } };
}
8 changes: 5 additions & 3 deletions packages/engine/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,11 @@ export interface RpcServerDeps {
* 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, and the review's response carries the result with
* the agent's parts, ranking, story, unexplained changes and judged
* claims, or the plain parts and ranking with the reason they stayed.
* agent judges them, then the result with the verdicts in another while
* the agent maps the acceptance criteria, and the review's response
* carries the result with the agent's parts, ranking, story, unexplained
* changes, judged claims and mapped criteria, or the plain parts and
* ranking with the reason they stayed.
*/
export async function runRpcServer(
source: RpcLineSource,
Expand Down
4 changes: 2 additions & 2 deletions packages/engine/test/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('runCli review', () => {
expect(code).toBe(0);
expect(err.text).toBe('');
const result = JSON.parse(out.text) as { version: number; parts: unknown[] };
expect(result.version).toBe(14);
expect(result.version).toBe(15);
expect(result.parts).toHaveLength(11);
});

Expand All @@ -56,7 +56,7 @@ describe('runCli review', () => {
version: number;
copies: { head: { path: string } };
};
expect(result.version).toBe(14);
expect(result.version).toBe(15);
expect(result.copies.head.path.startsWith(cacheDir)).toBe(true);
});

Expand Down
Loading
Loading