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
33 changes: 33 additions & 0 deletions packages/cli/src/commands/review/agent-prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,39 @@ describe('--roster — every prompt the plan requires, in one call', () => {
}
});

it('drops the adversarial personas when the plan records medium effort', () => {
// The wiring under test: the capturing command writes `effort` into the plan,
// and the roster reads it from there — no `--effort` flag on THIS command.
// A `medium` plan must build the reduced set (personas gone). If this reddens
// back to nine, `check-coverage` and `compose-review` — which read the same
// `plan.effort` — would flag the personas missing and escalate medium to high
// on every run. This is the boundary the pure-function test cannot reach.
const dir = mkdtempSync(join(tmpdir(), 'ap-roster-med-'));
try {
const plan = join(dir, 'plan.json');
writeFileSync(plan, JSON.stringify({ ...PLAN, effort: 'medium' }));
(agentPromptCommand.handler as (a: unknown) => void)({
plan,
roster: true,
});
const recorded = readRecordedPrompts(plan);
expect([...recorded.keys()].sort()).toEqual([
'1a',
'1b',
'2',
'3',
'4',
'5',
]);
const printed = (writeStdoutLine as unknown as Mock).mock
.calls[0][0] as string;
expect(printed).toContain('6 agents required');
expect(printed).not.toMatch(/Agent 6[abc]:/);
} finally {
rmSync(dir, { recursive: true, force: true });
}
});

it('a whole block copied lazily — separator line included — still delivers', () => {
// The point of one call is that the compliant move is mechanical. An
// orchestrator that copies from one ───── line to the next has copied an
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/commands/review/agent-prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,9 @@ function rosterLabel(req: RequiredAgent): string {
* because both come from `requiredAgents(plan)`.
*/
function runRoster(report: PlanReport, planPath: string, rules?: string): void {
// The roster reads `plan.effort` (written by the capturing command), so a
// `medium` plan builds the reduced set here without an `--effort` flag — and
// `check-coverage` holds the run to that same set from the same field.
const roster = requiredAgents(report as RosterPlan);
const blocks = roster.map((req, i) => {
const { key, prompt } = buildLaunch(
Expand Down
14 changes: 14 additions & 0 deletions packages/cli/src/commands/review/capture-local.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { mkdirSync, writeFileSync } from 'node:fs';
import { dirname, resolve } from 'node:path';
import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
import { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js';
import type { ReviewEffort } from './parse-args.js';
import { captureLocalDiff, type SkippedFile } from './lib/local-diff.js';
import { buildDiffPlan, READ_FILE_CHAR_CAP } from './lib/diff-plan.js';
import {
Expand All @@ -35,9 +36,12 @@ interface CaptureLocalArgs {
file?: string;
target: string;
untracked: boolean;
effort?: ReviewEffort;
}

type CaptureLocalResult = PlanReport & {
/** The review's effort, recorded so the roster reads one value everywhere. */
effort?: ReviewEffort;
diffPath: string;
diffPathAbsolute: string;
/** Untracked files whose contents are in the diff — `git diff` shows none. */
Expand Down Expand Up @@ -92,6 +96,7 @@ function runCaptureLocal(args: CaptureLocalArgs): void {
...buildPlanReport(plan, null),
untrackedFiles: capture.untracked,
skippedFiles: capture.skipped,
...(args.effort ? { effort: args.effort } : {}),
};

writeFileSync(out, stringifyPlanReport(result), 'utf8');
Expand Down Expand Up @@ -171,6 +176,15 @@ export const captureLocalCommand: CommandModule = {
default: true,
describe:
'Include untracked, non-ignored files. On by default: `git diff` cannot see them, so without this a brand-new file goes unreviewed.',
})
.option('effort', {
type: 'string',
choices: ['low', 'medium', 'high'],
describe:
'The review effort. `medium` (balanced) drops the adversarial ' +
'personas from the required roster; recorded in the plan so ' +
'check-coverage, agent-prompt --roster and compose-review all read ' +
'one value. Omit for the full (high) roster.',
}),
handler: (argv) => {
runCaptureLocal(argv as unknown as CaptureLocalArgs);
Expand Down
37 changes: 37 additions & 0 deletions packages/cli/src/commands/review/check-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,43 @@ describe('the roster — who should have been here', () => {
expect(gap).not.toContain('every dimension');
});

it('reads the effort from the plan: medium drops the personas, high still requires them', () => {
// coverageFromTranscripts passes the WHOLE plan to requiredAgents, which reads
// plan.effort. A medium run that launched the reduced set (no 6a/6b/6c) must
// pass; the SAME records under a high plan must fail for the missing personas.
// Drop the effort read and the medium case demands the personas too and exits 3,
// halting every medium review — this A/B is what would redden.
const p = join(dir, 'plan.json');
const base = {
diffPathAbsolute: DIFF,
srcDiffLines: 200,
diffLines: 300,
prNumber: '6766',
ownerRepo: 'QwenLM/qwen-code',
worktreePath: '.qwen/tmp/review-pr-6766',
files: [{ path: 'a.ts', kind: 'source', removedLines: 0, heavy: false }],
chunks: [
{ id: 1, startLine: 1, endLine: 100 },
{ id: 2, startLine: 101, endLine: 200 },
],
};
const backdate = () =>
utimesSync(p, new Date(2020, 0, 1), new Date(2020, 0, 1));

// Medium: satisfyRoster launches exactly the reduced roster (personas dropped).
writeFileSync(p, JSON.stringify({ ...base, effort: 'medium' }));
satisfyRoster(p);
backdate();
expect(coverageFromTranscripts(p, ENV).missingRoles).toEqual([]);

// The SAME records, now a high plan: the personas are required and were never
// launched, so they are missing — proving the medium pass was the effort, not luck.
writeFileSync(p, JSON.stringify({ ...base, effort: 'high' }));
backdate();
const high = coverageFromTranscripts(p, ENV).missingRoles.join(' ');
expect(high).toMatch(/mindset|Undirected audit/);
});

it('tells the operator where it looked, so a wrong --plan is not a missing file', () => {
// "The builder never ran" and "the builder ran against a different --plan" reach
// this check as the same thing: an absent record. They are fixed differently, so
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/commands/review/check-coverage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ interface CheckCoverageArgs {
function runCheckCoverage(args: CheckCoverageArgs): void {
let report;
try {
report = coverageFromTranscripts(args.plan);
report = coverageFromTranscripts(args.plan, process.env);
} catch (err) {
if (err instanceof TranscriptsUnavailableError) {
// Infrastructure, not a verdict. A read-only HOME or a sandbox leaves no
Expand Down
58 changes: 56 additions & 2 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,13 @@ const DIFF = '/abs/diff.txt';
* satisfies that one. A plan that requires nothing is not a plan any capture
* command writes, and coverage now reads the roster out of it.
*/
function plan(opts: { step45?: boolean; han?: boolean } = {}): string {
function plan(
opts: {
step45?: boolean;
han?: boolean;
effort?: 'low' | 'medium' | 'high';
} = {},
): string {
const p = join(dir, 'plan.json');
writeFileSync(
p,
Expand All @@ -68,6 +74,9 @@ function plan(opts: { step45?: boolean; han?: boolean } = {}): string {
// What fetch-pr records when the PR description contains Han
// characters — the deterministic bilingual-body switch.
...(opts.han ? { prDescriptionHasHan: true } : {}),
// The effort the capturing command recorded — the roster and the
// reverse-audit floor both read it from here.
...(opts.effort ? { effort: opts.effort } : {}),
srcDiffLines: 5000,
diffLines: 5000,
files: [{ path: 'a.ts', kind: 'source', removedLines: 0, heavy: false }],
Expand Down Expand Up @@ -282,7 +291,7 @@ function blindPrompt(chunk: number): string {
*/
function coveredPlan(
step45Keys: string[] = ['verify', 'reverse-audit'],
planOpts: { han?: boolean } = {},
planOpts: { han?: boolean; effort?: 'low' | 'medium' | 'high' } = {},
): string {
transcript('a1', goodPrompt(1), { toolCalls: 3 });
transcript('a2', goodPrompt(2), { toolCalls: 2 });
Expand Down Expand Up @@ -1643,6 +1652,51 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff
);
});

it('does not require the reverse audit at medium effort — a by-design Comment cap, no FIX line', () => {
// The balanced tier skips Step 5 deliberately. A clean medium review still caps
// at Comment (it cannot certify the diff the way high does), but the reverse
// audit must NOT be flagged as a repairable gap: the FIX line telling the
// orchestrator to run it made the one mandated repair round rebuild the full
// high pipeline and escalate every medium review back to high.
const r = composeReview({
criticalsInline: 0,
suggestionsInline: 1,
// verify ran; reverse audit absent BY DESIGN (plan records medium).
planPath: coveredPlan(['verify'], { effort: 'medium' }),
env: ENV,
modelId: MODEL,
});
expect(r.event).toBe('COMMENT');
expect(r.cappedBy).toContain('unreviewed-dimension');
// The disclosure reads as by-design, not as a failure the author must chase.
expect(r.body).toContain(
'the balanced (medium) tier skips the second-look pass',
);
expect(r.body).not.toMatch(
/no auditor was launched with a prompt this skill builds/,
);
// And crucially: no reverse-audit FIX line, so nothing escalates medium to high.
expect(r.remediation.join(' ')).not.toContain('reverse audit:');
});

it('still requires the verifier at medium — an unverified blocker must not post', () => {
// Medium runs Step 4. A Critical it did not verify is still held back from
// becoming a public blocker, exactly as at high — but no reverse-audit
// remediation appears, because medium never owed it.
const r = composeReview({
criticalsInline: 1,
suggestionsInline: 0,
planPath: coveredPlan([], { effort: 'medium' }),
env: ENV,
modelId: MODEL,
});
expect(r.event).toBe('COMMENT');
expect(r.cappedBy).toContain('criticals-unverified');
const fixes = r.remediation.join(' ');
Comment thread
wenshao marked this conversation as resolved.
expect(fixes).toContain('--role verify');
expect(fixes).not.toContain('--role reverse-audit');
});

it('says one sentence when verify and the reverse audit failed the same way', () => {
// #7268's posted body carried the two `rewritten` sentences back to back,
// near-identical but for the tail. Both steps down the same way is one
Expand Down
7 changes: 4 additions & 3 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,10 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult {
// Step 4 (verify) and Step 5 (reverse audit) ran, and read their briefs?
// `check-coverage` proves Step 3, but it runs at Step 3D — before these exist —
// and their count is not in the plan, so its roster cannot reach them. This is
// the floor that does, and only `compose-review` asks it, which runs only at
// high effort — the only effort at which verify and reverse audit run at all.
// Reverse audit is required on every high-effort review; verify once the review
// the floor that does, and only `compose-review` asks it, which runs at high
// and medium effort. Reverse audit is required only at high; medium skips it by
// design, and `verificationGaps` caps a clean medium verdict at Comment instead
// of flagging it as missing. Verify runs at both, once the review
// has non-deterministic findings to verify. Deterministic `[build]`/`[test]`
// findings are pre-confirmed and skip verification by design, so they do not
// demand a verifier — including a body Critical that carries their source tag.
Expand Down
14 changes: 14 additions & 0 deletions packages/cli/src/commands/review/fetch-pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { dirname, resolve } from 'node:path';
import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
import { createReviewWorktreeLease } from '../../services/review-worktree-lease.js';
import { ensureAuthenticated, gh, setGhHost } from './lib/gh.js';
import type { ReviewEffort } from './parse-args.js';
import { git, gitOpt, gitRaw, refExists, releaseWorktree } from './lib/git.js';
import { PINNED_DIFF_CONFIG, PINNED_DIFF_FLAGS } from './lib/diff-flags.js';
import {
Expand Down Expand Up @@ -73,9 +74,12 @@ interface FetchPrArgs {
host?: string;
/** yargs camelCases `--max-chunk-lines`; the snake_case form does not exist. */
maxChunkLines: number;
effort?: ReviewEffort;
}

type FetchPrResult = PlanReport & {
/** The review's effort, recorded so the roster reads one value everywhere. */
effort?: ReviewEffort;
prNumber: string;
ownerRepo: string;
remote: string;
Expand Down Expand Up @@ -373,6 +377,7 @@ async function runFetchPr(args: FetchPrArgs): Promise<void> {
diffPath,
diffPathAbsolute,
prDescriptionHasHan: /\p{Script=Han}/u.test(meta.body ?? ''),
...(args.effort ? { effort: args.effort } : {}),
...buildPlanReport(plan, (path) => fileLineCount(fetchedSha, path)),
};

Expand Down Expand Up @@ -438,6 +443,15 @@ export const fetchPrCommand: CommandModule = {
default: DEFAULT_MAX_CHUNK_LINES,
describe:
'Target size, in diff lines, of each review chunk. A chunk boundary falls on a hunk boundary; a hunk larger than this is split only at a top-level declaration, never inside a function.',
})
.option('effort', {
type: 'string',
choices: ['low', 'medium', 'high'],
describe:
'The review effort. `medium` (balanced) drops the adversarial ' +
'personas from the required roster; recorded in the plan so ' +
'check-coverage, agent-prompt --roster and compose-review all read ' +
'one value. Omit for the full (high) roster.',
}),
handler: async (argv) => {
setGhHost((argv as { host?: string }).host);
Expand Down
44 changes: 38 additions & 6 deletions packages/cli/src/commands/review/lib/coverage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,10 @@ export function coverageFromTranscripts(
// roster collapses to one line covering the whole run, and repeating "none was
// built" once per chunk transcript would put N more copies of the same fact
// into the posted body, right next to the line that already states it.
// The roster reads the effort from the plan itself (`plan.effort`, written by
// the capturing command), so this recomputation — and `compose-review`'s, which
// calls the same helper with no effort argument — agree with `check-coverage`
// on a medium run automatically. No effort is threaded through here.
const rosterForRun = requiredAgents(plan as unknown as RosterPlan);
// ONE predicate for "was this prompt built", everywhere. A partial write can
// leave a zero-byte record, and the Step 4/5 classifier already reads that as
Expand Down Expand Up @@ -1069,9 +1073,11 @@ export interface VerificationReport {
* two, so its roster (`requiredAgents`) cannot reach them. And their count is not
* in the plan: verify shards on the finding count (`ceil(N/8)`), reverse audit
* loops until it goes dry. So this is not an exact roster — it is a floor, and it
* is asked only by `compose-review`, which runs only at high effort. A low/medium
* quick pass has no verify and no reverse audit, and never reaches here (it emits
* no verdict, so it calls no `compose-review`).
* is asked only by `compose-review`, which runs at high AND medium effort. High
* requires both steps; medium runs verify but skips the reverse audit by design
* (see `balancedMedium` below), so at medium the reverse-audit floor becomes a
* Comment cap, not a repairable gap. Low emits no verdict, calls no
* `compose-review`, and never reaches here.
*
* The floor is deliberately one agent per step, for the failure it exists to catch:
* the step skipped **wholesale**, or run with agents that never opened their brief —
Expand All @@ -1095,6 +1101,14 @@ export function verificationGaps(
const built = readRecordedPrompts(planPath);
const gaps: VerificationReport['gaps'] = [];
const remediation: string[] = [];
// The balanced (medium) tier deliberately skips Step 5 (reverse audit). Read
// the effort from the plan, so this reader and the roster agree. At medium the
// absent reverse audit is a by-design omission that caps the verdict at Comment
// — NOT a gap to repair: flagging it missing, and emitting a FIX line telling
// the orchestrator to run it, made the one mandated repair round rebuild the
// full high pipeline and escalate every medium review back to high. Verify
Comment thread
wenshao marked this conversation as resolved.
// (Step 4) still runs at medium, so its floor below is untouched.
const balancedMedium = (plan as { effort?: unknown }).effort === 'medium';

// How a step's agents actually got their prompt. The floor needs the four shapes
// apart, not one boolean, because the fix for each is different — and a refusal
Expand Down Expand Up @@ -1156,7 +1170,9 @@ export function verificationGaps(
(k) => k === 'reverse-audit' || k.startsWith('reverse-audit--'),
);
const reverse = bestDelivery(reverseKeys);
if (reverse !== 'ok') {
// A repairable reverse-audit gap only at high: medium is complete without it.
const reverseGap = !balancedMedium && reverse !== 'ok';
if (reverseGap) {
// The fix template carries `--plan <plan>`; a literal `<plan>` pasted into a
// POSIX shell parses as input redirection, so the one repair round Step 6
// prescribes could never run. This function is handed the real path.
Expand Down Expand Up @@ -1203,15 +1219,15 @@ export function verificationGaps(
// keeps its own precise text. The remediation above stays per-role either
// way — the two rebuild commands differ, and the combined sentence lands in
// the posted body while the fixes land on stderr.
if (reverse !== 'ok' && verify !== null && verify === reverse) {
if (reverseGap && verify !== null && verify === reverse) {
gaps.push({
subject: 'verification and reverse audit',
reason: COMBINED_STEP45_GAP[reverse].en,
subjectZh: '验证与反向审计',
reasonZh: COMBINED_STEP45_GAP[reverse].zh,
});
} else {
if (reverse !== 'ok') {
if (reverseGap) {
gaps.push({
subject: 'reverse audit',
reason: REVERSE_AUDIT_GAP[reverse].gap,
Expand All @@ -1228,6 +1244,22 @@ export function verificationGaps(
});
}
}
// Medium discloses the reverse audit as a by-design omission — no FIX line
// (above), honest wording here — and lets it stand as the one coverage entry
// that caps a clean medium verdict at Comment, which is exactly what the tier
// promises. A medium review is complete without the second look; it simply does
// not certify the diff the way a high review does.
Comment thread
wenshao marked this conversation as resolved.
if (balancedMedium) {
gaps.push({
subject: 'reverse audit',
reason:
'not run — the balanced (medium) tier skips the second-look pass, so ' +
'this verdict is capped at Comment rather than Approve',
subjectZh: '反向审计',
reasonZh:
'未运行——均衡(medium)档跳过二次审查步骤,因此本次判定上限为 Comment,不会 Approve',
});
}

return { ok: gaps.length === 0, gaps, remediation, unverifiedFindings };
}
Expand Down
Loading
Loading