ReviewOS
Reload

also looking at this

stacks/buddy

fix: report skipped gates as skipped, and honour ai.majorUpgrades.maxAttempts

#1433
Merged glennmichael123 wants to merge fix/gate-neutral into main
9 files +257 -8
bin/cli.tsmodified+2-2
@@ -3029,7 +3029,7 @@ cli
30293029 }
30303030
30313031 const provider = await providerFor(config, 'running gates', logger)
3032 const { findLinkedIssues, runAiGates, runGates, summarizeGates } = await import('../src/gates')
3032 const { findLinkedIssues, runAiGates, runGates, gateIcon, summarizeGates } = await import('../src/gates')
30333033 const { parseManifest } = await import('../src/pr/pr-manifest')
30343034
30353035 const prs = await provider.getPullRequests('open')
@@ -3105,7 +3105,7 @@ cli
31053105 const summary = summarizeGates(results)
31063106
31073107 for (const result of results)
3108 logger.info(`${result.passed ? '✅' : result.mode === 'error' ? '❌' : '⚠️'} ${result.name}: ${result.summary}`)
3108 logger.info(`${gateIcon(result)} ${result.name}: ${result.summary}`)
31093109
31103110 const blocking = summary.conclusion === 'failure'
31113111
src/gates/ai-checks.tsmodified+3-1
@@ -43,7 +43,9 @@ const VERDICT_SCHEMA = {
4343
4444/** A gate that could not run, reported as neutral rather than as a pass. */
4545function neutral(name: string, summary: string): GateResult {
46 return { name, mode: 'warning', passed: true, summary, detail: 'This check did not run.' }
46 // `passed: true` so a skipped gate never blocks; `neutral: true` so it is
47 // never rendered, counted or titled as though it had run and found nothing.
48 return { name, mode: 'warning', passed: true, neutral: true, summary, detail: 'This check did not run.' }
4749}
4850
4951/**
src/gates/checks.tsmodified+38-2
@@ -9,6 +9,14 @@ export interface GateResult {
99 /** `error` blocks the merge under branch protection; `warning` does not */
1010 mode: GateMode
1111 passed: boolean
12 /**
13 * The gate did not run — no AI configured, nothing to check against, the
14 * assessment failed. Distinct from `passed`, which it used to be folded
15 * into: a gate that could not run was rendered with the same green tick as
16 * one that ran and found nothing, and a check run made of nothing but
17 * skipped gates reported "All checks passed".
18 */
19 neutral?: boolean
1220 /** What failed, or what passed */
1321 summary: string
1422 /** Extra explanation shown under the summary */
@@ -199,17 +207,31 @@ export function summarizeGates(results: GateResult[]): {
199207
200208 const blocking = results.filter(result => !result.passed && result.mode === 'error')
201209 const warnings = results.filter(result => !result.passed && result.mode === 'warning')
210 const skipped = results.filter(result => result.neutral)
202211
203212 const lines = results.map((result) => {
204 const icon = result.passed ? '✅' : result.mode === 'error' ? '❌' : '⚠️'
213 const icon = gateIcon(result)
205214 return `${icon} **${result.name}** — ${result.summary}${result.detail ? `\n\n${result.detail}\n` : ''}`
206215 })
207216
217 // Every gate skipped is not a pass. It is the "no checks configured" case
218 // arrived at a different way, and gets the same conclusion.
219 if (skipped.length === results.length) {
220 return {
221 conclusion: 'neutral',
222 title: `${skipped.length} check(s) did not run`,
223 summary: lines.join('\n\n'),
224 }
225 }
226
227 const ran = results.length - skipped.length
208228 const title = blocking.length > 0
209229 ? `${blocking.length} check(s) failed`
210230 : warnings.length > 0
211231 ? `${warnings.length} warning(s)`
212 : 'All checks passed'
232 : skipped.length > 0
233 ? `${ran} check(s) passed, ${skipped.length} did not run`
234 : 'All checks passed'
213235
214236 return {
215237 conclusion: blocking.length > 0 ? 'failure' : 'success',
@@ -218,6 +240,20 @@ export function summarizeGates(results: GateResult[]): {
218240 }
219241}
220242
243/**
244 * The icon a gate result renders with.
245 *
246 * Shared with the CLI so a skipped gate looks the same in a terminal as in
247 * the check run.
248 *
249 * @param result - A gate result
250 */
251export function gateIcon(result: GateResult): string {
252 if (result.neutral)
253 return '⏭️'
254 return result.passed ? '✅' : result.mode === 'error' ? '❌' : '⚠️'
255}
256
221257function escapeRegExp(value: string): string {
222258 return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
223259}
src/gates/index.tsmodified+1-0
@@ -3,6 +3,7 @@ export {
33 checkDescription,
44 checkTitleFormat,
55 runGates,
6 gateIcon,
67 summarizeGates,
78} from './checks'
89export type { GateConfig, GateInput, GateMode, GateResult } from './checks'
src/upgrades/migrate.tsmodified+42-3
@@ -38,6 +38,16 @@ export interface UpgradeOptions {
3838 autoMigrate?: boolean
3939 /** Open as draft below this confidence (default: high) */
4040 draftBelowConfidence?: MigrationConfidence
41 /**
42 * Agent runs allowed before the migration is reported as failed (default: 1).
43 *
44 * Declared on `ai.majorUpgrades` and documented, and the agent ran exactly
45 * once whatever it said. A second attempt is handed the first one's output
46 * so it continues rather than starting over.
47 */
48 maxAttempts?: number
49 /** The agent runner; injectable so attempts can be tested without a model */
50 agent?: typeof runAgent
4151 logger?: Logger
4252}
4353
@@ -116,9 +126,13 @@ export async function attemptMajorUpgrade(options: UpgradeOptions): Promise<Upgr
116126 return { plan, report: renderMigrationReport(plan, undefined, usage), draft: false, status: 'analysis-only' }
117127 }
118128
119 const result = await runAgent(options.ai, {
129 const run = options.agent ?? runAgent
130 const maxAttempts = Math.max(1, Math.floor(options.maxAttempts ?? 1))
131 const task = buildMigrationTask(plan)
132
133 let result = await run(options.ai, {
120134 mode: implementMode,
121 task: buildMigrationTask(plan),
135 task,
122136 context: {
123137 workspace: options.workspace,
124138 baseBranch: options.baseBranch,
@@ -126,11 +140,36 @@ export async function attemptMajorUpgrade(options: UpgradeOptions): Promise<Upgr
126140 },
127141 logger,
128142 })
143 let attempts = 1
144 let applied = result.toolCalls > 0
145
146 // A run that stopped short — out of tool calls, out of time, gave up — is
147 // handed its own output and asked to continue from the workspace as it now
148 // stands. Starting over would redo the work that did land.
149 while (result.stopReason !== 'completed' && attempts < maxAttempts) {
150 attempts++
151 logger.info(`🔧 Migration attempt ${attempts} of ${maxAttempts} (previous run ended: ${result.stopReason})`)
152
153 result = await run(options.ai, {
154 mode: implementMode,
155 task: `${task}\n\nA previous attempt ended (${result.stopReason}) with this output:\n\n${
156 result.output || '(no output)'
157 }\n\nContinue from the workspace as it stands now. Do not redo work that is already done.`,
158 context: {
159 workspace: options.workspace,
160 baseBranch: options.baseBranch,
161 ...(options.branch ? { branch: options.branch } : {}),
162 },
163 logger,
164 })
165 applied = applied || result.toolCalls > 0
166 }
129167
130168 const verified = result.stopReason === 'completed'
131169 const outcome: MigrationOutcome = {
132 applied: result.toolCalls > 0,
170 applied,
133171 verified,
172 attempts,
134173 changedFiles: knownFiles,
135174 unresolved: verified ? [] : plan.changes.filter(change => !change.automatable).map(change => change.action),
136175 ...(verified ? {} : { verificationOutput: result.output }),
src/upgrades/report.tsmodified+2-0
@@ -8,6 +8,8 @@ export interface MigrationOutcome {
88 applied: boolean
99 /** Whether the repository's own verification passed afterwards */
1010 verified: boolean
11 /** Agent runs it took, when more than one was allowed */
12 attempts?: number
1113 /** Files the migration touched */
1214 changedFiles: string[]
1315 /** Verification output, when it ran */
src/upgrades/wire.tsmodified+1-0
@@ -124,6 +124,7 @@ export async function analyzeGroupMajors(options: GroupUpgradeOptions): Promise<
124124 baseBranch: options.baseBranch,
125125 ai: options.ai,
126126 ...(autoMigrate !== undefined ? { autoMigrate } : {}),
127 ...(settings.maxAttempts !== undefined ? { maxAttempts: settings.maxAttempts } : {}),
127128 ...(settings.draftBelowConfidence ? { draftBelowConfidence: settings.draftBelowConfidence } : {}),
128129 logger,
129130 })
test/ai-gates.test.tsmodified+48-0
@@ -9,6 +9,7 @@ import {
99 isMergeEvent,
1010 runAiGates,
1111 runPostMerge,
12 summarizeGates,
1213} from '../src/gates'
1314import {
1415 clearQuickSelection,
@@ -486,3 +487,50 @@ describe('EOL feeds the dependency gate', () => {
486487 expect(checkDependencies([{ name: 'node', version: '22' }], { mode: 'error' }).passed).toBe(true)
487488 })
488489})
490
491/**
492 * `neutral()` in the assertion gates returned `passed: true`, and `GateResult`
493 * had no other state — so a gate that could not run was rendered with the same
494 * green tick as one that ran and found nothing, and a check run made of
495 * nothing but skipped gates was titled "All checks passed".
496 */
497describe('neutral gate results', () => {
498 const passed = { name: 'deps', mode: 'error' as const, passed: true, summary: 'fine' }
499 const skipped = { name: 'linked-issue', mode: 'warning' as const, passed: true, neutral: true, summary: 'No AI provider configured' }
500 const failed = { name: 'size', mode: 'error' as const, passed: false, summary: 'too big' }
501
502 it('failure case - all gates skipped is not "all checks passed"', () => {
503 const summary = summarizeGates([skipped])
504
505 expect(summary.conclusion).toBe('neutral')
506 expect(summary.title).toContain('did not run')
507 expect(summary.title).not.toContain('passed')
508 })
509
510 it('success case - a skipped gate beside a real pass is counted apart', () => {
511 const summary = summarizeGates([passed, skipped])
512
513 expect(summary.conclusion).toBe('success')
514 expect(summary.title).toBe('1 check(s) passed, 1 did not run')
515 })
516
517 it('success case - a skipped gate renders as skipped, not as a tick', () => {
518 const { summary } = summarizeGates([passed, skipped])
519
520 expect(summary).toContain('⏭️ **linked-issue**')
521 expect(summary).toContain('✅ **deps**')
522 })
523
524 it('failure case - a skipped gate never blocks', () => {
525 expect(summarizeGates([skipped, failed]).conclusion).toBe('failure')
526 expect(summarizeGates([skipped, passed]).conclusion).toBe('success')
527 })
528
529 it('success case - a gate that could not run is flagged neutral', async () => {
530 // No AI is the commonest reason. It says so rather than passing.
531 const result = await checkLinkedIssue(null, { diff: 'x', linkedIssues: [{ number: 1, title: 't', body: 'b' }] } as never, 'error')
532
533 expect(result.neutral).toBe(true)
534 expect(result.passed).toBe(true)
535 })
536})
test/upgrade-attempts.test.tsadded+120-0
@@ -0,0 +1,120 @@
1import type { AiClient } from '../src/ai/types'
2import type { AgentRunResult } from '../src/agent/types'
3import type { PackageUpdate } from '../src/types'
4import { describe, expect, it } from 'bun:test'
5import { attemptMajorUpgrade } from '../src/upgrades/migrate'
6import { Logger } from '../src/utils/logger'
7
8const PLAN = {
9 changes: [{ description: 'the legacy import was removed', action: 'import from the root', affectedFiles: [], automatable: true }],
10 confidence: 'high',
11 effort: 2,
12 risks: [],
13}
14
15/** An analysis client that always returns the same plan. */
16const ai = {
17 provider: 'anthropic',
18 model: 'test',
19 tokensUsed: 0,
20 async complete() {
21 return { text: JSON.stringify(PLAN), toolCalls: [], json: PLAN, stopReason: 'end', usage: { inputTokens: 1, outputTokens: 1 }, model: 'test' }
22 },
23} as unknown as AiClient
24
25const update: PackageUpdate = {
26 name: 'react',
27 currentVersion: '17.0.2',
28 newVersion: '19.0.0',
29 updateType: 'major',
30 dependencyType: 'dependencies',
31 file: 'package.json',
32} as PackageUpdate
33
34/** A scripted agent: one result per call, recording the tasks it was given. */
35function scriptedAgent(results: Array<Partial<AgentRunResult>>) {
36 const tasks: string[] = []
37 let call = 0
38 const agent = async (_ai: unknown, options: { task: string }): Promise<AgentRunResult> => {
39 tasks.push(options.task)
40 const scripted = results[Math.min(call++, results.length - 1)]
41 return { output: '', stopReason: 'completed', transcript: [], outputTokens: 1, toolCalls: 1, ...scripted }
42 }
43 return { agent: agent as never, tasks }
44}
45
46function attempt(maxAttempts: number | undefined, results: Array<Partial<AgentRunResult>>) {
47 const { agent, tasks } = scriptedAgent(results)
48 return attemptMajorUpgrade({
49 update,
50 releaseNotes: 'removed the legacy API',
51 files: [],
52 workspace: '/tmp',
53 baseBranch: 'main',
54 ai,
55 autoMigrate: true,
56 ...(maxAttempts === undefined ? {} : { maxAttempts }),
57 agent,
58 logger: Logger.silent(),
59 }).then(result => ({ result, tasks }))
60}
61
62/**
63 * `ai.majorUpgrades.maxAttempts` was declared, documented as "maximum agent
64 * attempts per upgrade", and the agent ran exactly once whatever it said.
65 */
66describe('migration attempts', () => {
67 it('success case - a run that completes takes one attempt', async () => {
68 const { result, tasks } = await attempt(3, [{ stopReason: 'completed' }])
69
70 expect(tasks).toHaveLength(1)
71 expect(result.outcome?.attempts).toBe(1)
72 expect(result.status).toBe('migrated')
73 })
74
75 it('success case - a run that stops short is retried up to the limit', async () => {
76 const { result, tasks } = await attempt(3, [
77 { stopReason: 'max_tool_calls', output: 'got halfway' },
78 { stopReason: 'completed' },
79 ])
80
81 expect(tasks).toHaveLength(2)
82 expect(result.outcome?.attempts).toBe(2)
83 expect(result.status).toBe('migrated')
84 })
85
86 it('success case - the retry is told what happened and to continue', async () => {
87 // Starting over would redo the work that did land; the second run is
88 // handed the first one's output and pointed at the workspace as it is.
89 const { tasks } = await attempt(2, [
90 { stopReason: 'timeout', output: 'renamed two imports' },
91 { stopReason: 'completed' },
92 ])
93
94 expect(tasks[1]).toContain('renamed two imports')
95 expect(tasks[1]).toContain('Continue from the workspace')
96 })
97
98 it('failure case - the limit is respected', async () => {
99 const { result, tasks } = await attempt(2, [{ stopReason: 'max_tool_calls' }])
100
101 expect(tasks).toHaveLength(2)
102 expect(result.status).toBe('migration-failed')
103 expect(result.draft).toBe(true)
104 })
105
106 it('edge case - unset means one attempt, as before', async () => {
107 const { tasks } = await attempt(undefined, [{ stopReason: 'error' }])
108
109 expect(tasks).toHaveLength(1)
110 })
111
112 it('edge case - applied stays true if any attempt changed code', async () => {
113 const { result } = await attempt(2, [
114 { stopReason: 'max_tool_calls', toolCalls: 3 },
115 { stopReason: 'error', toolCalls: 0 },
116 ])
117
118 expect(result.outcome?.applied).toBe(true)
119 })
120})