From 246600bd0924d8626cf9826017b7627d90022673 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Sun, 9 Aug 2026 20:56:13 +0100 Subject: [PATCH 1/2] fix(review): make template fixture Biome-safe ## Summary - construct the template-literal fixture from string fragments - preserve the exact tested fixture bytes - restore a clean full-repository Biome gate on origin/main ## Root cause PR #370 added a deliberately literal `${sideEffect()}` fixture inside a single-quoted string. Biome correctly flags that representation as noTemplateCurlyInString, and PR #371 made warnings fatal, so every unrelated devkit ship became blocked. ## Validation - focused diff-focus Vitest: 30 passed - Biome check for the changed file: clean - matcher preflight: 0 new candidates - commit guard: PASS - GitNexus: exercised review-cache path is high reach, but production code and fixture value are unchanged Tracks autonomous report 741fde8b-e5ef-41d5-aaa6-df158dd673ce. --- gate-engine/judge/__tests__/diff-focus.test.mts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gate-engine/judge/__tests__/diff-focus.test.mts b/gate-engine/judge/__tests__/diff-focus.test.mts index b601173..38cada6 100644 --- a/gate-engine/judge/__tests__/diff-focus.test.mts +++ b/gate-engine/judge/__tests__/diff-focus.test.mts @@ -233,7 +233,7 @@ describe('diffCacheIdentity — conservative matching + function anchors', () => "import { captureException } from '../utils/presentry-shim';", ], ['a nested call smuggled as the argument', 'Sentry.captureException(mutateGlobalState(err));'], - ['a template-literal argument', 'Sentry.captureMessage(`${sideEffect()}`);'], + ['a template-literal argument', ['Sentry.captureMessage(`', '$', '{sideEffect()}`);'].join('')], ])('does NOT strip %s — the restage re-reviews', (_label, line) => { const fixed = edit(10, ['handle();']); const d1 = gitDiffOf(BASE, fixed); From f8c40b82f3dc1f91f029ad67d8c01d6d79cdcb89 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Sun, 9 Aug 2026 21:36:45 +0100 Subject: [PATCH 2/2] fix(review): resolve checklist assets across providers (sc-1397) ## Summary - resolve checklist scripts from the provider-projected root - keep prompts, Bash allowlists, retries, and identities on the same root - make the correctness skill and reviewer brief portable across Codex, Claude, and Cursor projections - extract cascade and asset resolution helpers to respect the size ratchet - add regression coverage for provider-specific checklist resolution ## Root cause The correctness review runtime and generated assets assumed a .claude projection. Consumers with the valid .agents projection could therefore invoke a nonexistent checklist path and fail with MODULE_NOT_FOUND. ## Validation - reproduced the failure shape in Frink and verified portable resolution selects .agents/skills/correctness/scripts/checklist.mjs - review orchestration: 123 tests passed - provider and asset suites: 106 tests passed - full review cluster: 663 tests passed before the behavior-preserving extraction - correctness review: all four lenses passed - Biome, ESLint, git diff check, and size gate passed - matcher preflight: 0 candidates; commit-guard passed ## Shared checkout note The shared checkout TypeScript run is blocked only by unrelated concurrent duplicate imports in setup-manifest.mts and setup-runtime.mts. The isolated ship excludes those changes. ## Dependency Stacked on #374, which fixes the pre-existing origin/main Biome warning that otherwise blocks every devkit ship. --- .claude/agents/correctness-reviewer.md | 24 +- .claude/skills/correctness/SKILL.md | 17 +- .cursor/agents/correctness-reviewer.md | 24 +- .cursor/skills/correctness/SKILL.md | 17 +- agents/correctness-reviewer.md | 24 +- cli/__tests__/review-asset-runtime.test.mts | 15 + dist/agents/correctness-reviewer.md | 24 +- dist/skills/correctness/SKILL.md | 17 +- eslint/baselines/size-lines.json | 3 +- .../__tests__/consumer-identity.test.mts | 23 +- .../review/__tests__/reviewers.test.mts | 25 +- .../review/cascade/consumer-assets.mts | 32 ++ gate-engine/review/cascade/reviewer.mts | 235 +++++++++++++++ gate-engine/review/reviewers.mts | 10 +- gate-engine/review/run-review.mts | 273 +----------------- gate-engine/review/runtime.mts | 23 +- skills/correctness/SKILL.md | 17 +- 17 files changed, 492 insertions(+), 311 deletions(-) create mode 100644 gate-engine/review/cascade/consumer-assets.mts create mode 100644 gate-engine/review/cascade/reviewer.mts diff --git a/.claude/agents/correctness-reviewer.md b/.claude/agents/correctness-reviewer.md index 74bd111..bbc1088 100644 --- a/.claude/agents/correctness-reviewer.md +++ b/.claude/agents/correctness-reviewer.md @@ -67,10 +67,26 @@ performance issue as "correctness" to justify a FAIL. -## 1. Read skill for detailed rules: -- `.claude/skills/correctness/SKILL.md` - -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +## 1. Resolve the installed skill and read it for detailed rules + +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" + +Read `$CORRECTNESS_SKILL/SKILL.md` before continuing. ## 2. Generate the checklist ```bash diff --git a/.claude/skills/correctness/SKILL.md b/.claude/skills/correctness/SKILL.md index 747aa02..737b45d 100644 --- a/.claude/skills/correctness/SKILL.md +++ b/.claude/skills/correctness/SKILL.md @@ -8,7 +8,22 @@ description: Correctness bug hunting for a finished diff. Use when reviewing cha ## Review Script ```bash -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" node $SCRIPT generate # Enumerate review items from staged source files (all declared roots) node $SCRIPT status # Show progress diff --git a/.cursor/agents/correctness-reviewer.md b/.cursor/agents/correctness-reviewer.md index 74bd111..bbc1088 100644 --- a/.cursor/agents/correctness-reviewer.md +++ b/.cursor/agents/correctness-reviewer.md @@ -67,10 +67,26 @@ performance issue as "correctness" to justify a FAIL. -## 1. Read skill for detailed rules: -- `.claude/skills/correctness/SKILL.md` - -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +## 1. Resolve the installed skill and read it for detailed rules + +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" + +Read `$CORRECTNESS_SKILL/SKILL.md` before continuing. ## 2. Generate the checklist ```bash diff --git a/.cursor/skills/correctness/SKILL.md b/.cursor/skills/correctness/SKILL.md index 747aa02..737b45d 100644 --- a/.cursor/skills/correctness/SKILL.md +++ b/.cursor/skills/correctness/SKILL.md @@ -8,7 +8,22 @@ description: Correctness bug hunting for a finished diff. Use when reviewing cha ## Review Script ```bash -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" node $SCRIPT generate # Enumerate review items from staged source files (all declared roots) node $SCRIPT status # Show progress diff --git a/agents/correctness-reviewer.md b/agents/correctness-reviewer.md index 74bd111..bbc1088 100644 --- a/agents/correctness-reviewer.md +++ b/agents/correctness-reviewer.md @@ -67,10 +67,26 @@ performance issue as "correctness" to justify a FAIL. -## 1. Read skill for detailed rules: -- `.claude/skills/correctness/SKILL.md` - -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +## 1. Resolve the installed skill and read it for detailed rules + +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" + +Read `$CORRECTNESS_SKILL/SKILL.md` before continuing. ## 2. Generate the checklist ```bash diff --git a/cli/__tests__/review-asset-runtime.test.mts b/cli/__tests__/review-asset-runtime.test.mts index b91ada4..31a768b 100644 --- a/cli/__tests__/review-asset-runtime.test.mts +++ b/cli/__tests__/review-asset-runtime.test.mts @@ -133,6 +133,21 @@ describe('packaged reviewer asset runtime', () => { } }); + it('makes correctness checklist discovery portable across agent providers', () => { + const sources = [ + readFileSync(join(HERE, '../../agents/correctness-reviewer.md'), 'utf8'), + readFileSync(join(HERE, '../../skills/correctness/SKILL.md'), 'utf8'), + ]; + + for (const source of sources) { + expect(source).toContain('.agents/skills/correctness'); + expect(source).toContain('.claude/skills/correctness'); + expect(source).toContain('.cursor/skills/correctness'); + expect(source).toContain('[ -f "$candidate/scripts/checklist.mjs" ]'); + expect(source).toContain('Correctness Review checklist unavailable: run devkit sync-skills'); + } + }); + it('copies only registered assets, dereferences links, preserves executability, and keeps preflight identity', () => { const source = packageFixture('devkit review package '); const originalBrief = join(source, 'agents/api-security-reviewer.md'); diff --git a/dist/agents/correctness-reviewer.md b/dist/agents/correctness-reviewer.md index 74bd111..bbc1088 100644 --- a/dist/agents/correctness-reviewer.md +++ b/dist/agents/correctness-reviewer.md @@ -67,10 +67,26 @@ performance issue as "correctness" to justify a FAIL. -## 1. Read skill for detailed rules: -- `.claude/skills/correctness/SKILL.md` - -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +## 1. Resolve the installed skill and read it for detailed rules + +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" + +Read `$CORRECTNESS_SKILL/SKILL.md` before continuing. ## 2. Generate the checklist ```bash diff --git a/dist/skills/correctness/SKILL.md b/dist/skills/correctness/SKILL.md index 747aa02..737b45d 100644 --- a/dist/skills/correctness/SKILL.md +++ b/dist/skills/correctness/SKILL.md @@ -8,7 +8,22 @@ description: Correctness bug hunting for a finished diff. Use when reviewing cha ## Review Script ```bash -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" node $SCRIPT generate # Enumerate review items from staged source files (all declared roots) node $SCRIPT status # Show progress diff --git a/eslint/baselines/size-lines.json b/eslint/baselines/size-lines.json index c215b1a..84b1261 100644 --- a/eslint/baselines/size-lines.json +++ b/eslint/baselines/size-lines.json @@ -15,7 +15,6 @@ "gate-engine/review/__tests__/run-review.test.mts": 2461, "gate-engine/review/eval/bench.mts": 878, "gate-engine/review/eval/conventions/bench.mts": 988, - "gate-engine/review/eval/reviewers/bench.mts": 881, - "gate-engine/review/run-review.mts": 547 + "gate-engine/review/eval/reviewers/bench.mts": 881 } } diff --git a/gate-engine/review/__tests__/consumer-identity.test.mts b/gate-engine/review/__tests__/consumer-identity.test.mts index 17cc1cf..00b64e2 100644 --- a/gate-engine/review/__tests__/consumer-identity.test.mts +++ b/gate-engine/review/__tests__/consumer-identity.test.mts @@ -6,11 +6,12 @@ * ship-mode identities are two incomparable namespaces and every cross-mode rate is a blend. * 2. It never throws. It feeds telemetry, and telemetry must never fail a gate. */ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, renameSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { afterEach, describe, expect, it } from 'vitest'; import { resolveGuardConfig } from '../../config.mts'; +import { consumerChecklistAssetRoot } from '../cascade/consumer-assets.mts'; import { checklistAssetPath, hasChecklist, @@ -82,6 +83,26 @@ describe('consumerReviewerIdentity', () => { } }); + it('uses a provider-projected skill root for execution identity when Claude skills are absent', () => { + const { root, packaged } = consumerFixture(); + mkdirSync(join(root, '.agents'), { recursive: true }); + renameSync(join(root, '.claude/skills'), join(root, '.agents/skills')); + const cfg = resolveGuardConfig(root); + const selected: ReviewerSelection[] = REVIEWERS.map((reviewer) => ({ + reviewer, + files: ['src/example.ts'], + })); + const packagedIdentities = preflightReviewAssets(packaged, selected, cfg); + + for (const reviewer of REVIEWERS) { + if (hasChecklist(reviewer)) + expect(consumerChecklistAssetRoot(root, reviewer)).toBe('.agents'); + expect(consumerReviewerIdentity(root, cfg, reviewer)).toBe( + packagedIdentities.get(reviewer.name), + ); + } + }); + it('changes when the brief changes, and only for that reviewer', () => { const { root } = consumerFixture(); const cfg = resolveGuardConfig(root); diff --git a/gate-engine/review/__tests__/reviewers.test.mts b/gate-engine/review/__tests__/reviewers.test.mts index 451e119..988563a 100644 --- a/gate-engine/review/__tests__/reviewers.test.mts +++ b/gate-engine/review/__tests__/reviewers.test.mts @@ -270,6 +270,11 @@ describe('allowedToolsFor', () => { expect(tools).toContain(',mcp__codebase__searchCode'); expect(tools).toContain('Bash(node .claude/skills/commit-guard/scripts/checklist.mjs:*)'); }); + it('grants a provider-projected checklist path when the consumer resolver supplies one', () => { + const tools = allowedToolsFor(REVIEWERS[0], cfg, '.agents'); + expect(tools).toContain('Bash(node .agents/skills/api-security/scripts/checklist.mjs:*)'); + expect(tools).not.toContain('Bash(node .claude/skills/api-security/scripts/checklist.mjs:*)'); + }); it('a skill-less reviewer (conventions) gets EXACTLY Read,Grep,Glob — no Bash at all, per its AC', () => { const conv = REVIEWERS.find((r) => r.name === 'conventions-reviewer'); expect(allowedToolsFor(conv, cfg)).toBe('Read,Grep,Glob'); @@ -416,16 +421,34 @@ describe('wrapPrompt / escalatePrompt / stripFrontmatter', () => { expect(p).toContain('check-item --pass'); expect(p).toContain('Never delete the checklist artifact'); }); + it('rewrites ordinary commit/ship prompts to the resolved consumer checklist root', () => { + const p = wrapPrompt( + 'Try .agents/skills/api-security/SKILL.md, .claude/skills/api-security/SKILL.md, then .cursor/skills/api-security/SKILL.md.', + REVIEWERS[0], + ['src/main/a.ts'], + undefined, + undefined, + undefined, + '.agents', + ); + expect(p).toContain('node .agents/skills/api-security/scripts/checklist.mjs generate'); + expect(p.match(/\.agents\/skills\/api-security/g)).toHaveLength(6); + expect(p).not.toContain('.claude/skills/api-security'); + expect(p).not.toContain('.cursor/skills/api-security'); + expect(p).toContain('MANDATORY CHECKLIST WORKFLOW'); + }); it('lets the packaged brief own enumeration and rewrites its skill paths in review mode', () => { const guard = REVIEWERS.find((r) => r.name === 'commit-guard'); const p = wrapPrompt( - 'Read .claude/skills/commit-guard/SKILL.md.', + 'Try .agents/skills/commit-guard/SKILL.md, .claude/skills/commit-guard/SKILL.md, then .cursor/skills/commit-guard/SKILL.md.', guard, ['src/a.ts'], '/tmp/devkit-review-assets', ); expect(p).toContain('/tmp/devkit-review-assets/skills/commit-guard/SKILL.md'); expect(p).not.toContain('.claude/skills/commit-guard/SKILL.md'); + expect(p).not.toContain('.agents/skills/commit-guard'); + expect(p).not.toContain('.cursor/skills/commit-guard'); expect(p).toContain('The reviewer brief owns checklist enumeration'); expect(p).not.toContain('check-file '); }); diff --git a/gate-engine/review/cascade/consumer-assets.mts b/gate-engine/review/cascade/consumer-assets.mts new file mode 100644 index 0000000..79cd047 --- /dev/null +++ b/gate-engine/review/cascade/consumer-assets.mts @@ -0,0 +1,32 @@ +import { existsSync, readFileSync } from 'node:fs'; +import path from 'node:path'; +import type { GuardConfig } from '../../config.mts'; +import { checklistAssetPath, hasChecklist, type Reviewer } from '../reviewers.mts'; + +const CONSUMER_SKILL_ROOTS = ['.claude', '.agents', '.cursor'] as const; + +/** Resolve the provider-projected checklist root actually present in a consumer checkout. */ +export function consumerChecklistAssetRoot(cwd: string, reviewer: Reviewer): string { + if (!hasChecklist(reviewer)) return '.claude'; + const relativePath = checklistAssetPath(reviewer); + return ( + CONSUMER_SKILL_ROOTS.find((root) => existsSync(path.resolve(cwd, root, relativePath))) ?? + '.claude' + ); +} + +/** Read one package-relative asset from its consumer-projected brief or skill root. */ +export function readConsumerReviewAsset( + cwd: string, + cfg: GuardConfig, + skillRoot: string, + relativePath: string, +): Buffer { + const agentsPrefix = 'agents/'; + if (relativePath.startsWith(agentsPrefix)) { + const dir = cfg.review.agentsDir; + const base = path.isAbsolute(dir) ? dir : path.resolve(cwd, dir); + return readFileSync(path.join(base, relativePath.slice(agentsPrefix.length))); + } + return readFileSync(path.resolve(cwd, skillRoot, relativePath)); +} diff --git a/gate-engine/review/cascade/reviewer.mts b/gate-engine/review/cascade/reviewer.mts new file mode 100644 index 0000000..680de66 --- /dev/null +++ b/gate-engine/review/cascade/reviewer.mts @@ -0,0 +1,235 @@ +import type { GuardConfig } from '../../config.mts'; +import { JUDGE_ISOLATION } from '../../judge/judge-isolation.mts'; +import { DEEP_JUDGE_TIMEOUT_MS, execJudgeAsync } from '../../judge/run-judge.mts'; +import { renderGoverningClaudeMd } from '../claude-md.mts'; +import { buildCappedDiffEvidence } from '../diff-evidence.mts'; +import { attachItems } from '../evidence/items.mts'; +import { gitCached } from '../evidence/staged-git.mts'; +import { applyOverrideValve } from '../overrides.mts'; +import { + allowedToolsFor, + escalatePrompt, + hasChecklist, + type PromptExtras, + parseReviewVerdict, + type ReviewerSelection, + wrapConventionsPrompt, + wrapPrompt, +} from '../reviewers.mts'; +import { + agentBody, + cleanupChecklistState, + enforceChecklistContract, + initializeCommitGuardChecklist, + type ReviewOutcome, + readChecklistState, + withStagedFiles, +} from '../runtime.mts'; +import { consumerChecklistAssetRoot } from './consumer-assets.mts'; + +/** One reviewer cascade outcome, including its persisted transcript when a judge ran. */ +export type CascadeResult = ReviewOutcome; + +/** Orchestration inputs threaded through one reviewer's cascade. */ +export interface CascadeOpts { + cwd: string; + cfg: GuardConfig; + exec?: typeof execJudgeAsync; + firstModel?: string; + retryFirst?: boolean; + assetRoot?: string; + judgeEnv?: NodeJS.ProcessEnv; + checklistRecoveryReason?: string; + promptExtras?: PromptExtras; + /** Review-only recovery scheduling; commit/ship never retries a checklist-contract miss. */ + recovery?: 'defer' | 'final'; +} + +/** Run one reviewer with checklist verification, override handling, and cleanup. */ +export async function runCascade( + sel: ReviewerSelection, + opts: CascadeOpts, +): Promise { + const { cwd } = opts; + const checklistRoot = opts.assetRoot ?? consumerChecklistAssetRoot(cwd, sel.reviewer); + cleanupChecklistState(cwd, sel.reviewer); + try { + initializeCommitGuardChecklist(cwd, sel.reviewer, checklistRoot, opts.judgeEnv); + let res = await cascadeVerdict(sel, opts, checklistRoot); + res = await enforceChecklistContract(sel, res, cwd, opts.assetRoot, async (reason) => { + if (opts.recovery === 'defer') + return { ...res, status: 'inconclusive', reason, retryable: reason } as CascadeResult; + if (opts.recovery === 'final') + return { + ...res, + status: 'error', + reason: `reviewer checklist contract failed after one retry — ${reason}`, + } as CascadeResult; + throw new Error(`checklist recovery has no scheduling mode — ${reason}`); + }); + const disposition = applyOverrideValve(sel, res, cwd, { + readState: () => readChecklistState(cwd, sel.reviewer), + stagedDiff: () => gitCached(cwd, [], sel.files), + }); + attachItems(res, readChecklistState(cwd, sel.reviewer), disposition); + return res; + } finally { + cleanupChecklistState(cwd, sel.reviewer); + } +} + +async function cascadeVerdict( + { reviewer, files }: ReviewerSelection, + { + cwd, + cfg, + exec = execJudgeAsync, + firstModel = 'haiku', + retryFirst = false, + assetRoot, + judgeEnv, + checklistRecoveryReason, + promptExtras, + }: CascadeOpts, + checklistRoot: string, +): Promise { + const env = withStagedFiles(judgeEnv ?? process.env, reviewer, files); + const body = agentBody(cwd, cfg, reviewer.name, assetRoot); + if (body === null) + return { + name: reviewer.name, + status: 'inconclusive', + reason: `agent brief ${reviewer.name}.md missing under ${cfg.review.agentsDir} — run devkit sync-agents && devkit sync-skills`, + escalated: false, + }; + const stat = gitCached(cwd, ['--stat'], files); + const prompt = hasChecklist(reviewer) + ? wrapPrompt( + body, + reviewer, + files, + assetRoot, + checklistRecoveryReason, + promptExtras, + checklistRoot, + ) + : wrapConventionsPrompt(body, files, renderGoverningClaudeMd(cwd, files), promptExtras); + const input = buildCappedDiffEvidence(gitCached(cwd, [], files), stat); + const args = (promptBody: string, model: string): string[] => [ + '-p', + promptBody, + '--model', + model, + ...JUDGE_ISOLATION, + '--allowedTools', + allowedToolsFor(reviewer, cfg, checklistRoot), + ]; + const passModel = reviewer.model ?? firstModel; + let firstOutage: 'timeout' | 'transient' | 'empty' | undefined; + const firstOpts = { + label: `review:${reviewer.name}`, + args: args(prompt, passModel), + input, + timeout: DEEP_JUDGE_TIMEOUT_MS, + cwd, + transcript: false, + env, + onOutage: (kind: 'timeout' | 'transient' | 'empty') => { + firstOutage = kind; + }, + }; + let first = await exec(firstOpts); + if (first === null && retryFirst && firstOutage !== 'timeout') { + console.error( + `guard-review: ${reviewer.name}: judge run failed (${firstOutage ?? 'transient'}), retrying once…`, + ); + cleanupChecklistState(cwd, reviewer); + initializeCommitGuardChecklist(cwd, reviewer, checklistRoot, judgeEnv); + first = await exec(firstOpts); + } + if (first === null) + return { + name: reviewer.name, + status: 'inconclusive', + reason: firstOutage === 'timeout' ? 'judge timed out' : 'judge outage', + escalated: false, + model: passModel, + }; + const firstVerdict = parseReviewVerdict(first); + if (firstVerdict.verdict === 'PASS') + return { + name: reviewer.name, + status: 'pass', + reason: firstVerdict.reason, + escalated: false, + model: passModel, + transcript: first, + }; + if (firstVerdict.verdict === null) + return { + name: reviewer.name, + status: 'inconclusive', + reason: 'no VERDICT line', + escalated: false, + model: passModel, + transcript: first, + }; + if (reviewer.model) + return { + name: reviewer.name, + status: 'fail', + reason: firstVerdict.reason, + escalated: false, + model: passModel, + transcript: first, + }; + let secondOutage: 'timeout' | 'transient' | 'empty' | undefined; + const second = await exec({ + label: `review:${reviewer.name}:escalate`, + args: args(escalatePrompt(prompt, first), 'opus'), + input, + timeout: DEEP_JUDGE_TIMEOUT_MS, + cwd, + transcript: false, + env, + onOutage: (kind: 'timeout' | 'transient' | 'empty') => { + secondOutage = kind; + }, + }); + if (second === null) + return { + name: reviewer.name, + status: 'inconclusive', + reason: secondOutage === 'timeout' ? 'escalation timed out' : 'escalation outage', + escalated: true, + model: passModel, + transcript: first, + }; + const finalVerdict = parseReviewVerdict(second); + if (finalVerdict.verdict === 'FAIL') + return { + name: reviewer.name, + status: 'fail', + reason: finalVerdict.reason, + escalated: true, + model: passModel, + transcript: second, + }; + if (finalVerdict.verdict === 'PASS') + return { + name: reviewer.name, + status: 'pass', + reason: finalVerdict.reason, + escalated: true, + model: passModel, + transcript: second, + }; + return { + name: reviewer.name, + status: 'inconclusive', + reason: 'no VERDICT line', + escalated: true, + model: passModel, + transcript: second, + }; +} diff --git a/gate-engine/review/reviewers.mts b/gate-engine/review/reviewers.mts index 3c48834..13a316f 100644 --- a/gate-engine/review/reviewers.mts +++ b/gate-engine/review/reviewers.mts @@ -294,11 +294,13 @@ export function wrapPrompt( assetRoot?: string, checklistRecoveryReason?: string, { targetsBlock = '', commitMsgBlock = '' }: PromptExtras = {}, + checklistRoot = assetRoot ?? '.claude', ): string { - const effectiveAssetRoot = assetRoot ?? '.claude'; - const brief = stripFrontmatter(agentBody).replaceAll( - '.claude/skills/', - `${effectiveAssetRoot.replace(TRAILING_SLASH_RE, '')}/skills/`, + const effectiveAssetRoot = checklistRoot; + const skillPrefix = `${effectiveAssetRoot.replace(TRAILING_SLASH_RE, '')}/skills/`; + const brief = ['.agents/skills/', '.claude/skills/', '.cursor/skills/'].reduce( + (body, providerPrefix) => body.replaceAll(providerPrefix, skillPrefix), + stripFrontmatter(agentBody), ); const script = checklistScriptAt(reviewer, effectiveAssetRoot); const checklistContract = checklistContractFor(reviewer, script, assetRoot); diff --git a/gate-engine/review/run-review.mts b/gate-engine/review/run-review.mts index adff4db..3eda32b 100644 --- a/gate-engine/review/run-review.mts +++ b/gate-engine/review/run-review.mts @@ -32,19 +32,15 @@ import { envFlag, type GuardConfig, resolveGuardConfig } from '../config.mts'; import { emitCacheHit } from '../judge/gate-events.mts'; -import { JUDGE_ISOLATION } from '../judge/judge-isolation.mts'; import { reportGateInfraFailure } from '../judge/odb-probe.mts'; -import { DEEP_JUDGE_TIMEOUT_MS, execJudgeAsync, strictRemedy } from '../judge/run-judge.mts'; +import { execJudgeAsync, strictRemedy } from '../judge/run-judge.mts'; import { loadCache } from './cache.mts'; -import { renderGoverningClaudeMd } from './claude-md.mts'; -import { buildCappedDiffEvidence } from './diff-evidence.mts'; +import { type CascadeResult, runCascade } from './cascade/reviewer.mts'; import { loadReviewerContext } from './evidence/commit-message.mts'; -import { attachItems } from './evidence/items.mts'; import { emitReviewScope, emitReviewSkipped, reportNonRuns } from './evidence/scope.mts'; import { gitCached, stagedFiles } from './evidence/staged-git.mts'; import { reviewerTargetSalts } from './evidence/targets-block.mts'; import { emitMergedLensResults, mapLimit, planReviewWork, taskLabel } from './lens/split.mts'; -import { applyOverrideValve } from './overrides.mts'; import { clearProgress, writeProgress } from './progress.mts'; import { type ParkedRecovery, @@ -54,284 +50,27 @@ import { settleReviewOutcome, } from './recovery/settle.mts'; import { - allowedToolsFor, cacheKey, effectiveReviewConfig, - escalatePrompt, - hasChecklist, - type PromptExtras, - parseReviewVerdict, type ReviewerSelection, selectReviewers, - wrapConventionsPrompt, - wrapPrompt, } from './reviewers.mts'; import { - agentBody, - cleanupChecklistState, - enforceChecklistContract, gateJudgeEnv, - initializeCommitGuardChecklist, passAssetVerifier, preflightReviewAssets, - type ReviewOutcome, - readChecklistState, resolveReviewerIdentities, skippedReviewers, - withStagedFiles, } from './runtime.mts'; import { ReviewGateTiming, reviewConcurrency } from './telemetry/timing.mts'; -/** One reviewer's cascade outcome. `transcript` rides along on EVERY judged outcome (pass/fail/ - * no-VERDICT) — the FAIL loop prints it (a block whose evidence was discarded is undebuggable) and - * every outcome persists it as a fetchable transcript (saveTranscript) so a passing reviewer's - * reasoning is showcasable, not thrown away. Absent only when no judge ran (a hard outage). */ -type CascadeResult = ReviewOutcome; +export { runCascade }; + // A missing brief / missing checklist artifact is a SYNC gap, not an auth/quota outage — the strict -// remedy branches on it (see the inconclusive loop). Matches the reasons set in cascadeVerdict -// (`agent brief …`) and verifyChecklist (`checklist artifact missing …`). +// remedy branches on it (see the inconclusive loop). const SYNC_INCONCLUSIVE_RE = /^agent brief |^checklist artifact missing/; -// A cap kill, likewise, is the gate's OWN contention kill — not auth/quota. Matches the reasons -// cascadeVerdict sets from the judge's outage KIND (`judge timed out` / `escalation timed out`). +// A cap kill, likewise, is the gate's OWN contention kill — not auth/quota. const TIMEOUT_INCONCLUSIVE_RE = /timed out$/; -/** Orchestration inputs threaded through a cascade (config + the injectable judge runner). */ -interface CascadeOpts { - cwd: string; - cfg: GuardConfig; - exec?: typeof execJudgeAsync; - firstModel?: string; - retryFirst?: boolean; - assetRoot?: string; - judgeEnv?: NodeJS.ProcessEnv; - checklistRecoveryReason?: string; - promptExtras?: PromptExtras; - /** Checklist-contract recovery scheduling (sc-1476, REVIEW-ONLY — commit/ship have no retry): - * 'defer' parks a voided PASS for the post-wave serial phase instead of retrying inline; - * 'final' marks the deferred attempt itself — a repeated miss is terminal, never re-retried. */ - recovery?: 'defer' | 'final'; -} - -// Every pass here — first, strict first, opus escalation — runs on the SHARED DEEP_JUDGE_TIMEOUT_MS -// (judge/run-judge.mts), as does the commit-msg completeness judge; the 30-min rationale lives with -// the constant. Three same-valued locals here is exactly how it drifted from completeness (sc-1227). -// Budget arithmetic — the ship ceiling bounds the WHOLE hook chain, not this gate alone: deterministic -// prefix ~240s + decisions ≤60s (both ≈0 on a cache hit) + this cascade gate + completeness on the same -// cap. PER-CASCADE worst ≈ 1800 (first) + 1800 (escalate) = 3600s; under the concurrency cap (default -// 2, see the docblock) cascades run in ceil(N/K) WAVES, so the theoretical worst far exceeds -// SHIP_COMMIT_TIMEOUT (3600s) — by design: a killed ship CONVERGES on re-run because PASSes checkpoint -// per-completion and the caches skip what was earned (docs/decisions/ship-gates-converge-not-restart.md). -// Only correctness nears the cap; the rest finish <300s, so a real ship is one slow wave + fast waves. - -/** - * One reviewer's cascade → {name, status: 'pass'|'fail'|'inconclusive', reason, escalated}. - * `exec` is injectable for tests; the gate always passes execJudgeAsync. - * - * Wraps the verdict cascade with the checklist-artifact contract: the state file is cleaned - * BEFORE the judge runs (a stale artifact from an interactive session must never satisfy the - * gate), a PASS is voided to inconclusive when the artifact is missing/incomplete/inconsistent - * (verifyChecklist), and the artifact is removed afterwards either way. - */ -export async function runCascade( - sel: ReviewerSelection, - opts: CascadeOpts, -): Promise { - const { cwd } = opts; - cleanupChecklistState(cwd, sel.reviewer); - try { - initializeCommitGuardChecklist(cwd, sel.reviewer, opts.assetRoot, opts.judgeEnv); - let res = await cascadeVerdict(sel, opts); - res = await enforceChecklistContract(sel, res, cwd, opts.assetRoot, async (reason) => { - // sc-1476: under 'defer', the contract miss is PARKED for the post-wave serial phase (haiku - // compliance degrades under concurrent load — retrying inside the same wave re-fails). - // Under 'final' (the deferred attempt), a repeated miss is terminal. One attempt total. - if (opts.recovery === 'defer') - return { ...res, status: 'inconclusive', reason, retryable: reason } as CascadeResult; - if (opts.recovery === 'final') - return { - ...res, - status: 'error', - reason: `reviewer checklist contract failed after one retry — ${reason}`, - } as CascadeResult; - // Unreachable: only the review lane sets assetRoot (the callback's gate), always with a mode. - throw new Error(`checklist recovery has no scheduling mode — ${reason}`); - }); - const disposition = applyOverrideValve(sel, res, cwd, { - readState: () => readChecklistState(cwd, sel.reviewer), - stagedDiff: () => gitCached(cwd, [], sel.files), - }); - attachItems(res, readChecklistState(cwd, sel.reviewer), disposition); - return res; - } finally { - cleanupChecklistState(cwd, sel.reviewer); - } -} - -async function cascadeVerdict( - { reviewer, files }: ReviewerSelection, - { - cwd, - cfg, - exec = execJudgeAsync, - firstModel = 'haiku', - retryFirst = false, - assetRoot, - judgeEnv, - checklistRecoveryReason, - promptExtras, - }: CascadeOpts, -): Promise { - const env = withStagedFiles(judgeEnv ?? process.env, reviewer, files); // sc-1439 - const body = agentBody(cwd, cfg, reviewer.name, assetRoot); - if (body === null) - // A missing brief must never be judged as an EMPTY brief (a wrapper-only prompt fake-passes): - // inconclusive → fail-open on a normal commit, fail-closed on a ship — exactly the loudness - // an updated-CLI-but-unsynced-agents consumer needs. - return { - name: reviewer.name, - status: 'inconclusive', - reason: `agent brief ${reviewer.name}.md missing under ${cfg.review.agentsDir} — run devkit sync-agents && devkit sync-skills`, - escalated: false, - }; - // A skill-less reviewer (no checklist, no Bash) gets its evidence PRE-RENDERED instead of a - // "fetch it yourself" instruction: the capped diff (diff-evidence.mts) rides on stdin exactly - // like completeness.mts's judge, and the governing CLAUDE.md rules (claude-md.mts) are baked - // into the prompt itself. - const stat = gitCached(cwd, ['--stat'], files); - const prompt = hasChecklist(reviewer) - ? wrapPrompt(body, reviewer, files, assetRoot, checklistRecoveryReason, promptExtras) - : wrapConventionsPrompt(body, files, renderGoverningClaudeMd(cwd, files), promptExtras); - // sc-1441: every judge gets capped per-file evidence on stdin, not a bare stat — a judge that - // reads real hunks up front misses less; the caps are NAMED and Bash still fetches full hunks. - const input = buildCappedDiffEvidence(gitCached(cwd, [], files), stat); - const args = (p: string, model: string): string[] => [ - '-p', - p, - '--model', - model, - ...JUDGE_ISOLATION, - '--allowedTools', - allowedToolsFor(reviewer, cfg, assetRoot), - ]; - // A model-pinned reviewer (correctness, conventions) runs single-pass at its pinned model — no escalation. - const passModel = reviewer.model ?? firstModel; - let firstOutage: 'timeout' | 'transient' | 'empty' | undefined; - const firstOpts = { - label: `review:${reviewer.name}`, - args: args(prompt, passModel), - input, - timeout: DEEP_JUDGE_TIMEOUT_MS, - cwd, - transcript: false, // this gate persists its own review- transcript — don't store twice - env, - onOutage: (kind: 'timeout' | 'transient' | 'empty') => { - firstOutage = kind; - }, - }; - let first = await exec(firstOpts); - if (first === null && retryFirst && firstOutage !== 'timeout') { - // Strict (ship) runs get ONE first-pass retry — a TRANSIENT/empty failure must not fail a ship - // closed. A TIMEOUT is NOT retried: the pass already had the full DEEP_JUDGE_TIMEOUT_MS (a - // contended judge got its time UP FRONT), so a re-run burns the same budget again past the ship - // ceiling. The escalation pass never retries: outage stays inconclusive. - // Colon (not " — ") on purpose: the ship timeout banner's awk reads ` — ` as COMPLETED. - console.error( - `guard-review: ${reviewer.name}: judge run failed (${firstOutage ?? 'transient'}), retrying once…`, - ); - cleanupChecklistState(cwd, reviewer); // a dead first pass may have left partial rows - initializeCommitGuardChecklist(cwd, reviewer, assetRoot, judgeEnv); - first = await exec(firstOpts); - } - if (first === null) - return { - name: reviewer.name, - status: 'inconclusive', - // The CAUSE rides in the reason so the strict remedy can name it (sc-1227): a cap kill is - // not an auth/quota outage, and that remedy wastes the operator's time on a healthy CLI. - reason: firstOutage === 'timeout' ? 'judge timed out' : 'judge outage', - escalated: false, - model: passModel, - }; - const firstVerdict = parseReviewVerdict(first); - if (firstVerdict.verdict === 'PASS') - // Keep the judge's one-line PASS reason (the tail of its VERDICT line) instead of dropping it — - // it flows to the telemetry event + the terminal line, and `first` is persisted as a transcript. - return { - name: reviewer.name, - status: 'pass', - reason: firstVerdict.reason, - escalated: false, - model: passModel, - transcript: first, - }; - if (firstVerdict.verdict === null) - return { - name: reviewer.name, - status: 'inconclusive', - reason: 'no VERDICT line', - escalated: false, - model: passModel, - transcript: first, - }; - // Single-pass (model-pinned) reviewer: this FAIL is final — no opus escalation to second-guess it. - if (reviewer.model) - return { - name: reviewer.name, - status: 'fail', - reason: firstVerdict.reason, - escalated: false, - model: passModel, - transcript: first, - }; - let secondOutage: 'timeout' | 'transient' | 'empty' | undefined; - const second = await exec({ - label: `review:${reviewer.name}:escalate`, - args: args(escalatePrompt(prompt, first), 'opus'), - input, - timeout: DEEP_JUDGE_TIMEOUT_MS, // opus re-investigation; only fires pre-block, never retried - cwd, - transcript: false, // this gate persists its own review- transcript — don't store twice - env, - onOutage: (kind: 'timeout' | 'transient' | 'empty') => { - secondOutage = kind; - }, - }); - if (second === null) - return { - name: reviewer.name, - status: 'inconclusive', - reason: secondOutage === 'timeout' ? 'escalation timed out' : 'escalation outage', - escalated: true, - model: passModel, - transcript: first, // the first-pass FAIL evidence survives even when opus was dark - }; - const finalVerdict = parseReviewVerdict(second); - if (finalVerdict.verdict === 'FAIL') - return { - name: reviewer.name, - status: 'fail', - reason: finalVerdict.reason, - escalated: true, - model: passModel, - transcript: second, - }; - if (finalVerdict.verdict === 'PASS') - return { - name: reviewer.name, - status: 'pass', - reason: finalVerdict.reason, - escalated: true, - model: passModel, - transcript: second, - }; - return { - name: reviewer.name, - status: 'inconclusive', - reason: 'no VERDICT line', - escalated: true, - model: passModel, - transcript: second, - }; -} /** * The gate → exit code (see module contract). Selected reviewers run concurrently but BOUNDED to diff --git a/gate-engine/review/runtime.mts b/gate-engine/review/runtime.mts index 4c26f18..333daea 100644 --- a/gate-engine/review/runtime.mts +++ b/gate-engine/review/runtime.mts @@ -3,6 +3,7 @@ import { createHash } from 'node:crypto'; import { readFileSync, rmSync } from 'node:fs'; import path from 'node:path'; import type { GuardConfig } from '../config.mts'; +import { consumerChecklistAssetRoot, readConsumerReviewAsset } from './cascade/consumer-assets.mts'; import type { RecordedWaiver } from './overrides.mts'; import { checklistAssetPath, @@ -248,21 +249,6 @@ export function preflightReviewAssets( return identities; } -/** - * The SYNCED consumer copy of a packaged asset. `reviewerAssetPaths` names package-relative paths; - * a consumer keeps its briefs wherever `review.agentsDir` points (configurable) and every skill - * asset under `.claude/` — devkit's own sync convention, the same one `checklistScript` encodes. - */ -function readConsumerReviewAsset(cwd: string, cfg: GuardConfig, relativePath: string): Buffer { - const AGENTS_PREFIX = 'agents/'; - if (relativePath.startsWith(AGENTS_PREFIX)) { - const dir = cfg.review.agentsDir; - const base = path.isAbsolute(dir) ? dir : path.resolve(cwd, dir); - return readFileSync(path.join(base, relativePath.slice(AGENTS_PREFIX.length))); - } - return readFileSync(path.resolve(cwd, '.claude', relativePath)); -} - /** * Per-reviewer prompt identity for the ordinary commit/ship path, where there is no packaged asset * root and `preflightReviewAssets` therefore never runs. This is what makes a production verdict @@ -309,7 +295,12 @@ export function consumerReviewerIdentity( reviewer: Reviewer, ): string | null { try { - return hashReviewerIdentity((rel) => readConsumerReviewAsset(cwd, cfg, rel), reviewer, cfg); + const skillRoot = consumerChecklistAssetRoot(cwd, reviewer); + return hashReviewerIdentity( + (rel) => readConsumerReviewAsset(cwd, cfg, skillRoot, rel), + reviewer, + cfg, + ); } catch { return null; } diff --git a/skills/correctness/SKILL.md b/skills/correctness/SKILL.md index 747aa02..737b45d 100644 --- a/skills/correctness/SKILL.md +++ b/skills/correctness/SKILL.md @@ -8,7 +8,22 @@ description: Correctness bug hunting for a finished diff. Use when reviewing cha ## Review Script ```bash -SCRIPT=".claude/skills/correctness/scripts/checklist.mjs" +CORRECTNESS_SKILL="" +for candidate in \ + .agents/skills/correctness \ + .claude/skills/correctness \ + .cursor/skills/correctness +do + if [ -f "$candidate/scripts/checklist.mjs" ]; then + CORRECTNESS_SKILL="$candidate" + break + fi +done +if [ -z "$CORRECTNESS_SKILL" ]; then + echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2 + exit 2 +fi +SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs" node $SCRIPT generate # Enumerate review items from staged source files (all declared roots) node $SCRIPT status # Show progress