also looking at this
fix(ci): give the repair loop the guard it already had
#1407
10 files
+229
-34
| @@ -3,7 +3,7 @@ | ||
| 3 | 3 | import type { GitProvider } from '../src/git/provider' |
| 4 | 4 | import type { ReviewFinding } from '../src/review/findings' |
| 5 | 5 | import type { ReviewFormat } from '../src/review/local' |
| 6 | import type { BuddyConfig } from '../src/types' | |
| 6 | import type { BuddyConfig, PullRequest } from '../src/types' | |
| 7 | 7 | import fs from 'node:fs' |
| 8 | 8 | import process from 'node:process' |
| 9 | 9 | import { CLI } from '@stacksjs/clapp' |
| @@ -1354,10 +1354,22 @@ cli | ||
| 1354 | 1354 | } |
| 1355 | 1355 | |
| 1356 | 1356 | const { attemptFix } = await import('../src/ci') |
| 1357 | const { parseFixAttempts, upsertFixAttempts } = await import('../src/ci/attempts') | |
| 1357 | 1358 | const { createAiClient } = await import('../src/ai') |
| 1358 | 1359 | |
| 1360 | // The guard in attemptFix can only stop a loop if it is told how many | |
| 1361 | // attempts came before, and a CI job remembers nothing between runs. | |
| 1362 | const prNumber = options.pr ? Number.parseInt(options.pr, 10) : undefined | |
| 1363 | let pullRequest: PullRequest | undefined | |
| 1364 | if (prNumber) { | |
| 1365 | const open = await provider.getPullRequests('open') | |
| 1366 | pullRequest = open.find(candidate => candidate.number === prNumber) | |
| 1367 | } | |
| 1368 | const priorAttempts = parseFixAttempts(pullRequest?.body)?.attempts ?? 0 | |
| 1369 | ||
| 1359 | 1370 | const outcome = await attemptFix({ |
| 1360 | 1371 | log, |
| 1372 | priorAttempts, | |
| 1361 | 1373 | workspace: process.cwd(), |
| 1362 | 1374 | baseBranch: config.repository?.baseBranch ?? 'main', |
| 1363 | 1375 | ai: createAiClient(config, logger), |
| @@ -1384,10 +1396,23 @@ cli | ||
| 1384 | 1396 | |
| 1385 | 1397 | logger.info(`\n${outcome.report}\n`) |
| 1386 | 1398 | |
| 1387 | const prNumber = options.pr ? Number.parseInt(options.pr, 10) : undefined | |
| 1388 | if (prNumber && !options.dryRun) | |
| 1399 | if (prNumber && !options.dryRun) { | |
| 1389 | 1400 | await provider.createComment(prNumber, outcome.report) |
| 1390 | 1401 | |
| 1402 | // Only a real attempt counts. Recording the ones the guard already | |
| 1403 | // refused would march the counter up without anything being tried. | |
| 1404 | if (outcome.action !== 'skipped' && pullRequest) { | |
| 1405 | try { | |
| 1406 | await provider.updatePullRequest(prNumber, { | |
| 1407 | body: upsertFixAttempts(pullRequest.body, priorAttempts + 1), | |
| 1408 | }) | |
| 1409 | } | |
| 1410 | catch (error) { | |
| 1411 | logger.warn(`Could not record the fix attempt on PR #${prNumber}: ${error instanceof Error ? error.message : String(error)}`) | |
| 1412 | } | |
| 1413 | } | |
| 1414 | } | |
| 1415 | ||
| 1391 | 1416 | logger.success(`✅ ${outcome.action}${outcome.fixed ? ' (fixed)' : ''}`) |
| 1392 | 1417 | } |
| 1393 | 1418 | catch (error) { |
| @@ -93,9 +93,9 @@ typo cannot reach a workflow run. | ||
| 93 | 93 | |
| 94 | 94 | `apiUrl` defaults to `GITHUB_API_URL` when set — GitHub Actions exports it on |
| 95 | 95 | both github.com and Enterprise Server runners — otherwise |
| 96 | `https://api.github.com`. Set it explicitly for GitHub Enterprise Server, for | |
| 97 | example `https://github.acme.com/api/v3`. `serverUrl` defaults to | |
| 98 | `GITHUB_SERVER_URL`, otherwise `https://github.com`. | |
| 96 | `<https://api.github.com>`. Set it explicitly for GitHub Enterprise Server, for | |
| 97 | example `<https://github.acme.com/api/v3>`. `serverUrl` defaults to | |
| 98 | `GITHUB_SERVER_URL`, otherwise `<https://github.com>`. | |
| 99 | 99 | |
| 100 | 100 | ## Registries |
| 101 | 101 | |
| @@ -45,14 +45,14 @@ The repository comes from `repository.owner` and `repository.name` in `buddy.con | ||
| 45 | 45 | |
| 46 | 46 | #### Repository Actions settings |
| 47 | 47 | |
| 48 | `https://github.com/<owner>/<repo>/settings/actions` | |
| 48 | `<https://github.com/><owner>/<repo>/settings/actions` | |
| 49 | 49 | |
| 50 | 50 | - Workflow permissions — select "Read and write permissions" |
| 51 | 51 | - "Allow GitHub Actions to create and approve pull requests" |
| 52 | 52 | |
| 53 | 53 | #### Organization Actions settings |
| 54 | 54 | |
| 55 | `https://github.com/organizations/<owner>/settings/actions` | |
| 55 | `<https://github.com/organizations/><owner>/settings/actions` | |
| 56 | 56 | |
| 57 | 57 | - The same two settings one level up, which may override the repository's |
| 58 | 58 | |
| @@ -62,8 +62,8 @@ If neither the config nor the git remote identifies a repository, buddy prints b | ||
| 62 | 62 | |
| 63 | 63 | buddy does not open these, but setup points you at them: |
| 64 | 64 | |
| 65 | - Personal access tokens — `https://github.com/settings/tokens` | |
| 66 | - Repository secrets — `https://github.com/<owner>/<repo>/settings/secrets/actions` | |
| 65 | - Personal access tokens — `<https://github.com/settings/tokens>` | |
| 66 | - Repository secrets — `<https://github.com/><owner>/<repo>/settings/secrets/actions` | |
| 67 | 67 | |
| 68 | 68 | ## schedule |
| 69 | 69 | |
| @@ -80,21 +80,27 @@ features: | ||
| 80 | 80 | details: "Every run posts an outcome: what it classified, what it did, and — when it declined — why the fix was not the bot's to make." |
| 81 | 81 | --- |
| 82 | 82 | |
| 83 | ## From the thread | |
| 83 | ## From a failing run | |
| 84 | 84 | |
| 85 | ```text | |
| 86 | @buddy fix-ci | |
| 87 | ``` | |
| 85 | Repair is driven by the run that failed, because the diagnosis is a function of | |
| 86 | that job's log. The workflow generated by `buddy setup` wires this up: when a | |
| 87 | run on a Buddy branch fails, the `fix-ci` job reads its log and reports back on | |
| 88 | the pull request. | |
| 88 | 89 | |
| 89 | The most common way to reach it. The command is permission-checked, so a commenter without write access cannot make Buddy push to your branch. | |
| 90 | A comment cannot say *which* run failed, so `@buddy fix-ci` in a thread replies | |
| 91 | pointing at that job rather than guessing. | |
| 90 | 92 | |
| 91 | 93 | ## From the CLI |
| 92 | 94 | |
| 93 | 95 | ```bash |
| 94 | buddy fix-ci --pr 128 | |
| 95 | buddy fix-ci --pr 128 --dry-run # classify and report, change nothing | |
| 96 | buddy fix-ci --run-id 12345 --pr 128 | |
| 97 | buddy fix-ci --run-id 12345 --pr 128 --dry-run # classify and report, change nothing | |
| 96 | 98 | ``` |
| 97 | 99 | |
| 100 | `--run-id` supplies the log to diagnose. `--pr` is where the report goes, and | |
| 101 | also where the attempt counter lives — without it Buddy cannot tell a first | |
| 102 | attempt from a fourth. | |
| 103 | ||
| 98 | 104 | ## From a workflow |
| 99 | 105 | |
| 100 | 106 | ```yaml |
| @@ -91,11 +91,11 @@ The rebase handling lives in the unified workflow `buddy setup` writes to `.gith | ||
| 91 | 91 | name: Buddy |
| 92 | 92 | |
| 93 | 93 | on: |
| 94 | # Ticking the checkbox edits the PR body, which fires this event | |
| 94 | # Ticking the checkbox edits the PR body, which fires this event | |
| 95 | 95 | pull_request: |
| 96 | 96 | types: [edited, opened, ready_for_review, synchronize, closed] |
| 97 | 97 | |
| 98 | # The dashboard's checkboxes are the same request, made on an issue | |
| 98 | # The dashboard's checkboxes are the same request, made on an issue | |
| 99 | 99 | issues: |
| 100 | 100 | types: [edited, opened] |
| 101 | 101 | |
| @@ -76,8 +76,8 @@ link: | ||
| 76 | 76 | | --- | --- | |
| 77 | 77 | | `#123` | the same text, wrapped in code formatting | |
| 78 | 78 | | `fixes #456` | `fixes` followed by the number in code formatting | |
| 79 | | `https://github.com/org/repo/pull/123` | `org/repo#123`, as code rather than a link | | |
| 80 | | `https://github.com/org/repo/issues/456` | `org/repo#456`, as code rather than a link | | |
| 79 | | `<https://github.com/org/repo/pull/123>` | `org/repo#123`, as code rather than a link | | |
| 80 | | `<https://github.com/org/repo/issues/456>` | `org/repo#456`, as code rather than a link | | |
| 81 | 81 | |
| 82 | 82 | The information survives — you can still see which issue was fixed — but it no |
| 83 | 83 | longer cross-links into your tracker. |
| @@ -0,0 +1,81 @@ | ||
| 1 | /** | |
| 2 | * How many times Buddy has already tried to repair a pull request. | |
| 3 | * | |
| 4 | * `attemptFix` refuses to keep trying past `maxAttempts`, but it can only do | |
| 5 | * that if someone tells it how many attempts came before — and a CI job has no | |
| 6 | * memory of its own. The count therefore lives on the pull request, the same | |
| 7 | * place review state does, because it is the only store that survives the run. | |
| 8 | */ | |
| 9 | ||
| 10 | /** Marker carrying the attempt count, hidden in the pull request body. */ | |
| 11 | const MARKER_REGEX = /<!--\s*buddy:fix-ci\s+v(\d+)\s*([\s\S]*?)-->/ | |
| 12 | ||
| 13 | /** Schema version emitted by this build. */ | |
| 14 | export const FIX_ATTEMPTS_VERSION = 1 | |
| 15 | ||
| 16 | /** Attempt bookkeeping for one pull request. */ | |
| 17 | export interface FixAttemptState { | |
| 18 | /** Repairs attempted so far, successful or not */ | |
| 19 | attempts: number | |
| 20 | /** ISO timestamp of the most recent attempt */ | |
| 21 | lastAttemptAt: string | |
| 22 | } | |
| 23 | ||
| 24 | /** | |
| 25 | * Read the attempt count out of a pull request body. | |
| 26 | * | |
| 27 | * A missing or unreadable marker reads as zero attempts rather than as an | |
| 28 | * error: the guard exists to stop a loop, and refusing to act because the | |
| 29 | * bookkeeping is unparseable would be a worse failure than one extra try. | |
| 30 | * | |
| 31 | * @param body - Pull request body, possibly null | |
| 32 | * @returns The recorded state, or `null` when absent or unreadable | |
| 33 | */ | |
| 34 | export function parseFixAttempts(body: string | null | undefined): FixAttemptState | null { | |
| 35 | if (!body) | |
| 36 | return null | |
| 37 | ||
| 38 | const match = body.match(MARKER_REGEX) | |
| 39 | if (!match) | |
| 40 | return null | |
| 41 | ||
| 42 | try { | |
| 43 | const parsed = JSON.parse(match[2].trim()) as Partial<FixAttemptState> | |
| 44 | const attempts = Number(parsed.attempts) | |
| 45 | if (!Number.isFinite(attempts) || attempts < 0) | |
| 46 | return null | |
| 47 | ||
| 48 | return { | |
| 49 | attempts: Math.floor(attempts), | |
| 50 | lastAttemptAt: typeof parsed.lastAttemptAt === 'string' ? parsed.lastAttemptAt : new Date(0).toISOString(), | |
| 51 | } | |
| 52 | } | |
| 53 | catch { | |
| 54 | return null | |
| 55 | } | |
| 56 | } | |
| 57 | ||
| 58 | /** | |
| 59 | * Record an attempt on a pull request body. | |
| 60 | * | |
| 61 | * Counted per pull request rather than per commit, deliberately. Buddy's own | |
| 62 | * repair pushes a commit, so keying the count to the head sha would reset it | |
| 63 | * every time a fix landed and defeat the guard it exists to provide. | |
| 64 | * | |
| 65 | * @param body - Current pull request body | |
| 66 | * @param attempts - Total attempts including the one just made | |
| 67 | * @param now - Timestamp to record | |
| 68 | * @returns The body with exactly one, current, marker | |
| 69 | * @example | |
| 70 | * ```ts | |
| 71 | * const attempts = (parseFixAttempts(pr.body)?.attempts ?? 0) + 1 | |
| 72 | * await provider.updatePullRequest(number, { body: upsertFixAttempts(pr.body, attempts) }) | |
| 73 | * ``` | |
| 74 | */ | |
| 75 | export function upsertFixAttempts(body: string | null | undefined, attempts: number, now: Date = new Date()): string { | |
| 76 | const state: FixAttemptState = { attempts, lastAttemptAt: now.toISOString() } | |
| 77 | const marker = `<!-- buddy:fix-ci v${FIX_ATTEMPTS_VERSION}\n${JSON.stringify(state)}\n-->` | |
| 78 | const withoutMarker = (body ?? '').replace(MARKER_REGEX, '').trimEnd() | |
| 79 | ||
| 80 | return `${withoutMarker}\n\n${marker}` | |
| 81 | } | |
| @@ -1,9 +1,7 @@ | ||
| 1 | 1 | import type { GitProvider } from '../git/provider' |
| 2 | 2 | import type { BuddyConfig } from '../types' |
| 3 | 3 | import type { CommandContext, CommandHandler, CommandOutcome } from './dispatcher' |
| 4 | import process from 'node:process' | |
| 5 | 4 | import { createAiClient } from '../ai' |
| 6 | import { attemptFix } from '../ci/fix' | |
| 7 | 5 | |
| 8 | 6 | /** Everything the built-in handlers need to act. */ |
| 9 | 7 | export interface HandlerDeps { |
| @@ -90,16 +88,17 @@ export function createHandlers(deps: HandlerDeps): Record<string, CommandHandler | ||
| 90 | 88 | }, |
| 91 | 89 | |
| 92 | 90 | async 'fix-ci'(context): Promise<CommandOutcome> { |
| 93 | const ai = createAiClient(deps.config, context.logger) | |
| 94 | const outcome = await attemptFix({ | |
| 95 | log: '', | |
| 96 | workspace: process.cwd(), | |
| 97 | baseBranch: deps.config.repository?.baseBranch ?? 'main', | |
| 98 | ai, | |
| 99 | logger: context.logger, | |
| 100 | }) | |
| 101 | ||
| 102 | return { handled: true, reply: outcome.report } | |
| 91 | // This handler has no run to read. Diagnosis is entirely a function of | |
| 92 | // the failing job's log, so calling attemptFix with an empty one only | |
| 93 | // ever produced a confident-looking `unknown` classification with no | |
| 94 | // evidence behind it. Saying so is more useful than that. | |
| 95 | return { | |
| 96 | handled: false, | |
| 97 | reply: 'I diagnose a failure from its job log, and a comment does not tell me which run failed. ' | |
| 98 | + 'Buddy repairs failing checks automatically when the `fix-ci` job is enabled in your workflow — ' | |
| 99 | + 'it runs on the failed run itself and reports back here. ' | |
| 100 | + 'See https://buddy.sh/features/ci-repair', | |
| 101 | } | |
| 103 | 102 | }, |
| 104 | 103 | |
| 105 | 104 | async remember(context): Promise<CommandOutcome> { |
| @@ -1620,8 +1620,13 @@ ${generateComposerSetupSteps()} | ||
| 1620 | 1620 | - name: Install dependencies |
| 1621 | 1621 | run: bun install |
| 1622 | 1622 | |
| 1623 | # --pr is what carries the diagnosis back to the pull request, and it is | |
| 1624 | # also where the attempt counter lives: without it the anti-loop guard | |
| 1625 | # has nothing to read and every failure looks like the first one. | |
| 1626 | # A run triggered from a fork has no entry here, and the step is skipped. | |
| 1623 | 1627 | - name: Attempt a fix |
| 1624 | run: bunx @buddysh/buddy fix-ci --run-id \${{ github.event.workflow_run.id }} --verbose | |
| 1628 | if: \${{ github.event.workflow_run.pull_requests[0] }} | |
| 1629 | run: bunx @buddysh/buddy fix-ci --run-id \${{ github.event.workflow_run.id }} --pr \${{ github.event.workflow_run.pull_requests[0].number }} --verbose | |
| 1625 | 1630 | env: |
| 1626 | 1631 | GITHUB_TOKEN: \${{ secrets.GITHUB_TOKEN }} |
| 1627 | 1632 | ANTHROPIC_API_KEY: \${{ secrets.ANTHROPIC_API_KEY }} |
| @@ -0,0 +1,79 @@ | ||
| 1 | import { describe, expect, it } from 'bun:test' | |
| 2 | import { attemptFix } from '../src/ci/fix' | |
| 3 | import { parseFixAttempts, upsertFixAttempts } from '../src/ci/attempts' | |
| 4 | ||
| 5 | const TYPE_ERROR_LOG = `src/app.ts(12,5): error TS2322: Type 'string' is not assignable to type 'number'.` | |
| 6 | ||
| 7 | /** | |
| 8 | * `attemptFix` has always refused to keep trying past `maxAttempts`, but no | |
| 9 | * production caller passed `priorAttempts`, so every failure looked like the | |
| 10 | * first one and the guard never engaged. | |
| 11 | */ | |
| 12 | describe('fix attempt tracking', () => { | |
| 13 | describe('marker round-trip', () => { | |
| 14 | it('success case - a recorded attempt reads back', () => { | |
| 15 | const body = upsertFixAttempts('Original description.', 1) | |
| 16 | ||
| 17 | expect(parseFixAttempts(body)?.attempts).toBe(1) | |
| 18 | expect(body).toContain('Original description.') | |
| 19 | }) | |
| 20 | ||
| 21 | it('success case - recording again replaces rather than stacks', () => { | |
| 22 | const first = upsertFixAttempts('desc', 1) | |
| 23 | const second = upsertFixAttempts(first, 2) | |
| 24 | ||
| 25 | expect(second.match(/<!--\s*buddy:fix-ci/g)).toHaveLength(1) | |
| 26 | expect(parseFixAttempts(second)?.attempts).toBe(2) | |
| 27 | }) | |
| 28 | ||
| 29 | it('edge case - no marker means no attempts yet', () => { | |
| 30 | expect(parseFixAttempts('just a description')).toBeNull() | |
| 31 | expect(parseFixAttempts(null)).toBeNull() | |
| 32 | expect(parseFixAttempts('')).toBeNull() | |
| 33 | }) | |
| 34 | ||
| 35 | it('edge case - an unreadable marker reads as absent rather than throwing', () => { | |
| 36 | // Refusing to act because the bookkeeping is corrupt would be a worse | |
| 37 | // failure than one extra attempt. | |
| 38 | expect(() => parseFixAttempts('<!-- buddy:fix-ci v1\n{bad\n-->')).not.toThrow() | |
| 39 | expect(parseFixAttempts('<!-- buddy:fix-ci v1\n{bad\n-->')).toBeNull() | |
| 40 | }) | |
| 41 | ||
| 42 | it('edge case - a negative count is rejected', () => { | |
| 43 | expect(parseFixAttempts('<!-- buddy:fix-ci v1\n{"attempts":-3}\n-->')).toBeNull() | |
| 44 | }) | |
| 45 | ||
| 46 | it('success case - survives alongside the review marker', () => { | |
| 47 | // Both live in the pull request body; neither may eat the other. | |
| 48 | const withReview = 'desc\n\n<!-- buddy:review v1\n{"reviewedSha":"abc","fingerprints":[]}\n-->' | |
| 49 | const body = upsertFixAttempts(withReview, 1) | |
| 50 | ||
| 51 | expect(body).toContain('buddy:review') | |
| 52 | expect(parseFixAttempts(body)?.attempts).toBe(1) | |
| 53 | }) | |
| 54 | }) | |
| 55 | ||
| 56 | describe('the guard the count feeds', () => { | |
| 57 | const base = { workspace: process.cwd(), baseBranch: 'main' } | |
| 58 | ||
| 59 | it('failure case - stops once the attempts are spent', async () => { | |
| 60 | const outcome = await attemptFix({ ...base, log: TYPE_ERROR_LOG, priorAttempts: 3, maxAttempts: 3 }) | |
| 61 | ||
| 62 | expect(outcome.action).toBe('skipped') | |
| 63 | expect(outcome.report).toContain('stopping rather than looping') | |
| 64 | }) | |
| 65 | ||
| 66 | it('success case - a count below the ceiling still tries', async () => { | |
| 67 | const outcome = await attemptFix({ ...base, log: TYPE_ERROR_LOG, priorAttempts: 1, maxAttempts: 3 }) | |
| 68 | ||
| 69 | expect(outcome.action).not.toBe('skipped') | |
| 70 | }) | |
| 71 | ||
| 72 | it('failure case - a pre-existing failure on base is not this PR to fix', async () => { | |
| 73 | const outcome = await attemptFix({ ...base, log: TYPE_ERROR_LOG, failsOnBase: true }) | |
| 74 | ||
| 75 | expect(outcome.action).toBe('skipped') | |
| 76 | expect(outcome.report).toContain('base branch') | |
| 77 | }) | |
| 78 | }) | |
| 79 | }) | |