From 63b5ec39cd27a9efb37caa107f1d1df57b598752 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 16:29:09 -0500 Subject: [PATCH 1/8] fix(automerge): fail closed on CI -- read check runs, require a known mergeable state, pin the head SHA (0.9.5) The gate read only the legacy combined-status API, which omits GitHub Actions check runs and reports pending/0 on Actions-only repos; that was treated as "no CI" and passed, so a docs/tests/research PR whose checks all failed could be auto-merged. Now every status and check run on the head commit must pass and at least one must exist; mergeable null waits; the merge is pinned to the checked SHA. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 + web/package.json | 2 +- web/src/github.ts | 35 ++++++- web/src/kernel/scheduled/pr-automerge.ts | 64 +++++++++--- web/tests/pr-automerge-ci-gate.test.ts | 125 +++++++++++++++++++++++ 5 files changed, 214 insertions(+), 17 deletions(-) create mode 100644 web/tests/pr-automerge-ci-gate.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index b1df300..f05144b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 0.9.5 (2026-10-16) + +### Fixed +- **PR auto-merge no longer merges PRs whose GitHub Actions checks failed.** The CI gate read only the legacy combined-status API, which doesn't include Actions check runs and reports `pending` with zero statuses on an Actions-only repo; that was treated as "no CI configured" and passed. The gate now reads the check runs too and fails closed: every commit status must be `success`, every check run completed with `success`, `neutral` or `skipped`, and a PR with no CI at all is not auto-merged. It also waits while GitHub reports mergeability as unknown (it only refused `false`), checks CI on the head commit SHA rather than the branch name, and pins the merge to that SHA so a commit pushed after the checks can't be merged unchecked. New `getCheckRuns()` and `PullRequestStats.headSha`; `mergePullRequest()` takes an optional `sha`. + ## 0.9.4 (2026-09-24) ### Added diff --git a/web/package.json b/web/package.json index ff8db2f..f8882d5 100755 --- a/web/package.json +++ b/web/package.json @@ -1,6 +1,6 @@ { "name": "@stackbilt/aegis-core", - "version": "0.9.4", + "version": "0.9.5", "description": "Persistent AI agent framework for Cloudflare Workers. Multi-tier memory, autonomous goals, dreaming cycles, MCP native.", "license": "Apache-2.0", "publishConfig": { diff --git a/web/src/github.ts b/web/src/github.ts index ba69bde..d7549e2 100755 --- a/web/src/github.ts +++ b/web/src/github.ts @@ -597,6 +597,33 @@ export async function getCombinedStatus( }; } +// ─── Check Runs ────────────────────────────────────────────── + +export interface CheckRunSummary { + total: number; + runs: { name: string; status: string; conclusion: string | null }[]; +} + +/** + * GitHub Actions (and other Checks API) results for a commit. The legacy + * combined-status endpoint above does not include them. + */ +export async function getCheckRuns( + token: string, + repo: string, + ref: string, +): Promise { + const res = await ghFetch(token, `/repos/${repo}/commits/${encodeURIComponent(ref)}/check-runs?per_page=100`); + const data = (await res.json()) as { + total_count: number; + check_runs: { name: string; status: string; conclusion: string | null }[]; + }; + return { + total: data.total_count, + runs: data.check_runs.map((r) => ({ name: r.name, status: r.status, conclusion: r.conclusion })), + }; +} + // ─── PR Stats ──────────────────────────────────────────────── export interface PullRequestStats { @@ -607,6 +634,8 @@ export interface PullRequestStats { deletions: number; changed_files: number; head: string; + /** Head commit SHA: what CI ran on, and what a merge must pin to. */ + headSha: string; base: string; } @@ -623,7 +652,7 @@ export async function getPullRequestStats( additions: number; deletions: number; changed_files: number; - head: { ref: string }; + head: { ref: string; sha: string }; base: { ref: string }; }; return { @@ -634,6 +663,7 @@ export async function getPullRequestStats( deletions: data.deletions, changed_files: data.changed_files, head: data.head.ref, + headSha: data.head.sha, base: data.base.ref, }; } @@ -658,9 +688,12 @@ export async function mergePullRequest( pullNumber: number, method: 'merge' | 'squash' | 'rebase' = 'squash', commitMessage?: string, + /** Merge only if the PR head is still this commit (GitHub answers 409 otherwise). */ + sha?: string, ): Promise<{ sha: string; merged: boolean; message: string }> { const body: Record = { merge_method: method }; if (commitMessage) body.commit_message = commitMessage; + if (sha) body.sha = sha; const res = await ghFetch(token, `/repos/${repo}/pulls/${pullNumber}/merge`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, diff --git a/web/src/kernel/scheduled/pr-automerge.ts b/web/src/kernel/scheduled/pr-automerge.ts index 1a11163..b19d54c 100644 --- a/web/src/kernel/scheduled/pr-automerge.ts +++ b/web/src/kernel/scheduled/pr-automerge.ts @@ -4,16 +4,19 @@ // // Gates: // 1. Task category is auto_safe (docs, tests, research) -// 2. CI status is green (or no CI configured) +// 2. CI is green: every commit status and check run on the head commit +// succeeded, and there is at least one (no CI means no auto-merge) // 3. Diff is under LOC cap // 4. No sensitive files touched -// 5. PR is open and mergeable +// 5. PR is open and GitHub reports it mergeable (unknown is not enough); +// the merge is pinned to the head commit that was checked // // Runs in Phase 1 (free -- GitHub API only, no LLM). import { type EdgeEnv } from '../dispatch.js'; import { getCombinedStatus, + getCheckRuns, getPullRequestStats, getPullRequestFiles, mergePullRequest, @@ -163,8 +166,8 @@ async function evaluateAndMerge( return { merged: false, reason: `PR state: ${stats.state}` }; } - if (stats.mergeable === false) { - return { merged: false, reason: 'not mergeable (conflicts)' }; + if (stats.mergeable !== true) { + return { merged: false, reason: stats.mergeable === false ? 'not mergeable (conflicts)' : 'mergeability not computed yet' }; } const totalLoc = stats.additions + stats.deletions; @@ -182,17 +185,14 @@ async function evaluateAndMerge( } } - // Gate 3: CI status - const ciStatus = await getCombinedStatus(githubToken, candidate.ghRepo, stats.head); - - if (ciStatus.state === 'failure' || ciStatus.state === 'error') { - return { merged: false, reason: `CI ${ciStatus.state}` }; - } - - // CI pending is OK for repos without CI configured (total=0) - // But if there ARE statuses and they're pending, wait - if (ciStatus.state === 'pending' && ciStatus.total > 0) { - return { merged: false, reason: 'CI pending' }; + // Gate 3: CI on the exact head commit -- legacy statuses and check runs + const [ciStatus, checkRuns] = await Promise.all([ + getCombinedStatus(githubToken, candidate.ghRepo, stats.headSha), + getCheckRuns(githubToken, candidate.ghRepo, stats.headSha), + ]); + const ci = ciVerdict(ciStatus, checkRuns); + if (!ci.ok) { + return { merged: false, reason: ci.reason }; } // All gates passed -- merge with squash @@ -203,6 +203,7 @@ async function evaluateAndMerge( candidate.prNumber, 'squash', commitMsg, + stats.headSha, ); if (!mergeResult.merged) { @@ -211,3 +212,36 @@ async function evaluateAndMerge( return { merged: true }; } + +// --- CI verdict --- + +const PASSING_CONCLUSIONS = new Set(['success', 'neutral', 'skipped']); + +/** + * Fail closed. GitHub Actions report through the Checks API, not commit + * statuses, and the combined status reads "pending" with zero statuses when + * a repo only uses Actions -- which used to pass. Now every status must be + * success, every check run completed with a passing conclusion, and at least + * one of either must exist. + */ +export function ciVerdict( + status: { total: number; statuses: { context: string; state: string }[] }, + checks: { total: number; runs: { name: string; status: string; conclusion: string | null }[] }, +): { ok: true } | { ok: false; reason: string } { + if (status.total === 0 && checks.total === 0) { + return { ok: false, reason: 'no CI statuses or check runs on head commit' }; + } + if (checks.total > checks.runs.length) { + return { ok: false, reason: `too many check runs to verify (${checks.total})` }; + } + for (const s of status.statuses) { + if (s.state !== 'success') return { ok: false, reason: `status ${s.context}: ${s.state}` }; + } + for (const r of checks.runs) { + if (r.status !== 'completed') return { ok: false, reason: `check ${r.name}: ${r.status}` }; + if (!PASSING_CONCLUSIONS.has(r.conclusion ?? '')) { + return { ok: false, reason: `check ${r.name}: ${r.conclusion ?? 'no conclusion'}` }; + } + } + return { ok: true }; +} diff --git a/web/tests/pr-automerge-ci-gate.test.ts b/web/tests/pr-automerge-ci-gate.test.ts new file mode 100644 index 0000000..34b17cc --- /dev/null +++ b/web/tests/pr-automerge-ci-gate.test.ts @@ -0,0 +1,125 @@ +// PR auto-merge CI gate: fail closed. +// GitHub Actions results come from the Checks API, not commit statuses. The +// old gate read only the combined status, which is "pending" with zero +// statuses on an Actions-only repo, and treated that as "no CI configured" +// -- so a PR whose checks all failed could be auto-merged. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { ciVerdict, runPrAutomerge } from '../src/kernel/scheduled/pr-automerge.js'; + +const noStatuses = { total: 0, statuses: [] }; +const run = (name: string, status: string, conclusion: string | null) => ({ name, status, conclusion }); + +describe('ciVerdict', () => { + it('refuses when there is no CI at all', () => { + expect(ciVerdict(noStatuses, { total: 0, runs: [] })).toEqual({ ok: false, reason: 'no CI statuses or check runs on head commit' }); + }); + + it('refuses a failed check run even when legacy statuses are empty (the old hole)', () => { + const v = ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'failure')] }); + expect(v.ok).toBe(false); + }); + + it('waits for in-progress check runs', () => { + expect(ciVerdict(noStatuses, { total: 1, runs: [run('test', 'in_progress', null)] }).ok).toBe(false); + }); + + it('refuses a failed or pending legacy status', () => { + expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'failure' }] }, { total: 0, runs: [] }).ok).toBe(false); + expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'pending' }] }, { total: 0, runs: [] }).ok).toBe(false); + }); + + it('refuses when not every check run could be read', () => { + expect(ciVerdict(noStatuses, { total: 150, runs: [run('a', 'completed', 'success')] }).ok).toBe(false); + }); + + it('passes when every status and check run succeeded (skipped and neutral allowed)', () => { + expect(ciVerdict( + { total: 1, statuses: [{ context: 'ci', state: 'success' }] }, + { total: 3, runs: [run('a', 'completed', 'success'), run('b', 'completed', 'skipped'), run('c', 'completed', 'neutral')] }, + )).toEqual({ ok: true }); + }); +}); + +describe('runPrAutomerge end to end (mocked GitHub)', () => { + const HEAD_SHA = 'abc123def456'; + let merges: Array>; + let checkRuns: unknown; + let mergeable: boolean | null; + + const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); + + function env() { + const updates: unknown[][] = []; + const db = { + prepare: (sql: string) => ({ + bind: (...args: unknown[]) => ({ + all: async () => ({ + results: sql.includes('FROM cc_tasks') + ? [{ id: 'task-1234abcd', title: 'docs: fix typo', repo: 'demo', category: 'docs', pr_url: 'https://github.com/ExampleOrg/demo/pull/7' }] + : [], + }), + run: async () => { updates.push(args); return { success: true }; }, + }), + }), + }; + return { env: { githubToken: 'test-token', db } as never, updates }; + } + + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-10-16T22:00:00Z')); // even hour: the task runs + merges = []; + mergeable = true; + checkRuns = { total_count: 1, check_runs: [run('typecheck', 'completed', 'success')] }; + vi.stubGlobal('fetch', vi.fn(async (url: string, init?: RequestInit) => { + const u = String(url); + if (u.endsWith('/pulls/7')) { + return json({ number: 7, state: 'open', mergeable, additions: 3, deletions: 1, changed_files: 1, head: { ref: 'auto/task1234', sha: HEAD_SHA }, base: { ref: 'main' } }); + } + if (u.endsWith('/pulls/7/files?per_page=100') || u.includes('/pulls/7/files')) return json([{ filename: 'docs/readme.md' }]); + if (u.includes(`/commits/${HEAD_SHA}/status`)) return json({ state: 'pending', total_count: 0, statuses: [] }); + if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) return json(checkRuns); + if (u.endsWith('/pulls/7/merge') && init?.method === 'PUT') { + merges.push(JSON.parse(String(init.body))); + return json({ sha: 'merged-sha', merged: true, message: 'Pull Request successfully merged' }); + } + return new Response('not mocked: ' + u, { status: 404 }); + })); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it('does not merge an Actions-only PR whose check failed', async () => { + checkRuns = { total_count: 1, check_runs: [run('typecheck', 'completed', 'failure')] }; + const { env: e, updates } = env(); + await runPrAutomerge(e); + expect(merges).toEqual([]); + expect(updates).toEqual([]); + }); + + it('does not merge a PR with no CI at all', async () => { + checkRuns = { total_count: 0, check_runs: [] }; + const { env: e } = env(); + await runPrAutomerge(e); + expect(merges).toEqual([]); + }); + + it('does not merge while GitHub has not computed mergeability', async () => { + mergeable = null; + const { env: e } = env(); + await runPrAutomerge(e); + expect(merges).toEqual([]); + }); + + it('merges a green PR, pinned to the head commit that was checked', async () => { + const { env: e, updates } = env(); + await runPrAutomerge(e); + expect(merges).toHaveLength(1); + expect(merges[0]).toMatchObject({ merge_method: 'squash', sha: HEAD_SHA }); + expect(updates).toEqual([['https://github.com/ExampleOrg/demo/pull/7/merged', 'task-1234abcd']]); + }); +}); From f855822d5832a47cb0adfb4a4fac4e549b14d2c0 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 16:29:29 -0500 Subject: [PATCH 2/8] chore: version.ts 0.9.5 Co-Authored-By: Claude Opus 5.5 --- web/src/version.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/version.ts b/web/src/version.ts index 6486056..590f4dd 100755 --- a/web/src/version.ts +++ b/web/src/version.ts @@ -1,3 +1,3 @@ // Semantic version - bump on each deploy // Used in /health endpoint and changelog tracking -export const VERSION = '0.9.4'; +export const VERSION = '0.9.5'; From e6263f1f617b14994be564399c71e18b9d814278 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 17:22:43 -0500 Subject: [PATCH 3/8] fix(automerge): require configured checks per repo; read all statuses; skipped isn't passed Review of #89: one green signal isn't CI passing (a job waiting on another, a path-filtered workflow, a fast external status), and these repos have no branch protection to name the checks that matter. The operator config now lists required checks per repo (integrations.github.automerge.requiredChecks); each must succeed on the head commit and a repo with no entry never auto-merges. Commit statuses are read 100 per page and the gate refuses when there are more; a skipped or neutral required check doesn't count. Tests assert the logged skip reason, not just the absence of a merge. Co-Authored-By: Claude Opus 5.5 --- web/src/github.ts | 2 +- web/src/kernel/scheduled/pr-automerge.ts | 27 ++++++-- web/src/operator/config.example.ts | 6 +- web/src/operator/types.ts | 12 +++- web/tests/pr-automerge-ci-gate.test.ts | 88 +++++++++++++++++++----- 5 files changed, 108 insertions(+), 27 deletions(-) diff --git a/web/src/github.ts b/web/src/github.ts index d7549e2..f9be7f9 100755 --- a/web/src/github.ts +++ b/web/src/github.ts @@ -580,7 +580,7 @@ export async function getCombinedStatus( repo: string, ref: string, ): Promise { - const res = await ghFetch(token, `/repos/${repo}/commits/${encodeURIComponent(ref)}/status`); + const res = await ghFetch(token, `/repos/${repo}/commits/${encodeURIComponent(ref)}/status?per_page=100`); const data = (await res.json()) as { state: string; total_count: number; diff --git a/web/src/kernel/scheduled/pr-automerge.ts b/web/src/kernel/scheduled/pr-automerge.ts index b19d54c..1abde1e 100644 --- a/web/src/kernel/scheduled/pr-automerge.ts +++ b/web/src/kernel/scheduled/pr-automerge.ts @@ -4,8 +4,10 @@ // // Gates: // 1. Task category is auto_safe (docs, tests, research) -// 2. CI is green: every commit status and check run on the head commit -// succeeded, and there is at least one (no CI means no auto-merge) +// 2. CI is green: every check the operator config requires for the repo +// (integrations.github.automerge.requiredChecks) succeeded on the head +// commit, and every other status and check run there passed. A repo with +// no required checks is never auto-merged. // 3. Diff is under LOC cap // 4. No sensitive files touched // 5. PR is open and GitHub reports it mergeable (unknown is not enough); @@ -21,6 +23,7 @@ import { getPullRequestFiles, mergePullRequest, } from '../../github.js'; +import { operatorConfig } from '../../operator/index.js'; import { AUTOMERGE_SAFE_CATEGORIES } from '../../schema-enums.js'; @@ -190,7 +193,8 @@ async function evaluateAndMerge( getCombinedStatus(githubToken, candidate.ghRepo, stats.headSha), getCheckRuns(githubToken, candidate.ghRepo, stats.headSha), ]); - const ci = ciVerdict(ciStatus, checkRuns); + const required = operatorConfig.integrations.github.automerge?.requiredChecks?.[candidate.ghRepo] ?? []; + const ci = ciVerdict(ciStatus, checkRuns, required); if (!ci.ok) { return { merged: false, reason: ci.reason }; } @@ -227,13 +231,26 @@ const PASSING_CONCLUSIONS = new Set(['success', 'neutral', 'skipped']); export function ciVerdict( status: { total: number; statuses: { context: string; state: string }[] }, checks: { total: number; runs: { name: string; status: string; conclusion: string | null }[] }, + required: string[], ): { ok: true } | { ok: false; reason: string } { - if (status.total === 0 && checks.total === 0) { - return { ok: false, reason: 'no CI statuses or check runs on head commit' }; + // "Some check is green" is not "CI passed": a job waiting on another job, a + // path-filtered workflow or a slow integration may not have reported yet. + // Without branch protection nothing else knows which checks matter, so the + // operator names them per repo; no names, no auto-merge. + if (required.length === 0) { + return { ok: false, reason: 'no required checks configured for this repo' }; } if (checks.total > checks.runs.length) { return { ok: false, reason: `too many check runs to verify (${checks.total})` }; } + if (status.total > status.statuses.length) { + return { ok: false, reason: `too many commit statuses to verify (${status.total})` }; + } + for (const name of required) { + const passed = checks.runs.some((r) => r.name === name && r.status === 'completed' && r.conclusion === 'success') + || status.statuses.some((s) => s.context === name && s.state === 'success'); + if (!passed) return { ok: false, reason: `required check ${name} has not passed` }; + } for (const s of status.statuses) { if (s.state !== 'success') return { ok: false, reason: `status ${s.context}: ${s.state}` }; } diff --git a/web/src/operator/config.example.ts b/web/src/operator/config.example.ts index 727b46c..10d7c0c 100755 --- a/web/src/operator/config.example.ts +++ b/web/src/operator/config.example.ts @@ -41,7 +41,11 @@ const config: OperatorConfig = { fallbackUrl: 'https://your-bizops.example.com/mcp', toolPrefix: 'BizOps Copilot', }, - github: { enabled: true }, + github: { + enabled: true, + // PR auto-merge: only repos listed here, and only when every named check passed. + // automerge: { requiredChecks: { 'ExampleOrg/my-project': ['typecheck', 'test'] } }, + }, brave: { enabled: true }, email: { profiles: { diff --git a/web/src/operator/types.ts b/web/src/operator/types.ts index 316e9cf..ab6e53d 100755 --- a/web/src/operator/types.ts +++ b/web/src/operator/types.ts @@ -28,7 +28,17 @@ export interface OperatorConfig { selfModel: SelfModel; integrations: { bizops: { enabled: boolean; fallbackUrl: string; toolPrefix: string }; - github: { enabled: boolean }; + github: { + enabled: boolean; + /** + * PR auto-merge for completed docs/tests/research tasks. A repo merges + * only when every check named here passed on the PR's head commit + * (plus every other status and check run that exists). A repo with no + * entry is never auto-merged. Keys are `owner/name`; names are check + * run names (GitHub Actions job names) or commit status contexts. + */ + automerge?: { requiredChecks: Record }; + }; brave: { enabled: boolean }; email: { profiles: Record; diff --git a/web/tests/pr-automerge-ci-gate.test.ts b/web/tests/pr-automerge-ci-gate.test.ts index 34b17cc..ebfa8ec 100644 --- a/web/tests/pr-automerge-ci-gate.test.ts +++ b/web/tests/pr-automerge-ci-gate.test.ts @@ -2,50 +2,73 @@ // GitHub Actions results come from the Checks API, not commit statuses. The // old gate read only the combined status, which is "pending" with zero // statuses on an Actions-only repo, and treated that as "no CI configured" -// -- so a PR whose checks all failed could be auto-merged. +// -- so a PR whose checks all failed could be auto-merged. Without branch +// protection, the operator config names the checks each repo must pass. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { ciVerdict, runPrAutomerge } from '../src/kernel/scheduled/pr-automerge.js'; +import { operatorConfig, setOperatorConfig } from '../src/operator/index.js'; const noStatuses = { total: 0, statuses: [] }; const run = (name: string, status: string, conclusion: string | null) => ({ name, status, conclusion }); +const REQ = ['typecheck']; describe('ciVerdict', () => { - it('refuses when there is no CI at all', () => { - expect(ciVerdict(noStatuses, { total: 0, runs: [] })).toEqual({ ok: false, reason: 'no CI statuses or check runs on head commit' }); + it('refuses a repo with no required checks configured, even when everything is green', () => { + expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'success')] }, [])) + .toEqual({ ok: false, reason: 'no required checks configured for this repo' }); + }); + + it('refuses when the required check has not reported yet, even if another check is green', () => { + expect(ciVerdict(noStatuses, { total: 1, runs: [run('lint', 'completed', 'success')] }, REQ)) + .toEqual({ ok: false, reason: 'required check typecheck has not passed' }); + expect(ciVerdict({ total: 1, statuses: [{ context: 'deploy-preview', state: 'success' }] }, { total: 0, runs: [] }, REQ).ok).toBe(false); + }); + + it('refuses a required check that was skipped or neutral', () => { + expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'skipped')] }, REQ).ok).toBe(false); + expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'neutral')] }, REQ).ok).toBe(false); }); it('refuses a failed check run even when legacy statuses are empty (the old hole)', () => { - const v = ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'failure')] }); - expect(v.ok).toBe(false); + expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'failure')] }, REQ).ok).toBe(false); }); - it('waits for in-progress check runs', () => { - expect(ciVerdict(noStatuses, { total: 1, runs: [run('test', 'in_progress', null)] }).ok).toBe(false); + it('refuses when any other check failed or is still running', () => { + const ok = run('typecheck', 'completed', 'success'); + expect(ciVerdict(noStatuses, { total: 2, runs: [ok, run('test', 'completed', 'failure')] }, REQ).ok).toBe(false); + expect(ciVerdict(noStatuses, { total: 2, runs: [ok, run('test', 'in_progress', null)] }, REQ).ok).toBe(false); + expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'pending' }] }, { total: 1, runs: [ok] }, REQ).ok).toBe(false); }); - it('refuses a failed or pending legacy status', () => { - expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'failure' }] }, { total: 0, runs: [] }).ok).toBe(false); - expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'pending' }] }, { total: 0, runs: [] }).ok).toBe(false); + it('refuses when not every check run or status could be read (pagination)', () => { + const ok = run('typecheck', 'completed', 'success'); + expect(ciVerdict(noStatuses, { total: 150, runs: [ok] }, REQ).ok).toBe(false); + const thirty = Array.from({ length: 30 }, (_, i) => ({ context: `s${i}`, state: 'success' })); + expect(ciVerdict({ total: 31, statuses: thirty }, { total: 1, runs: [ok] }, REQ)) + .toEqual({ ok: false, reason: 'too many commit statuses to verify (31)' }); }); - it('refuses when not every check run could be read', () => { - expect(ciVerdict(noStatuses, { total: 150, runs: [run('a', 'completed', 'success')] }).ok).toBe(false); + it('accepts a required check reported as a commit status', () => { + expect(ciVerdict({ total: 1, statuses: [{ context: 'typecheck', state: 'success' }] }, { total: 0, runs: [] }, REQ)).toEqual({ ok: true }); }); - it('passes when every status and check run succeeded (skipped and neutral allowed)', () => { + it('passes when every required check succeeded and the rest passed (skipped extras allowed)', () => { expect(ciVerdict( { total: 1, statuses: [{ context: 'ci', state: 'success' }] }, - { total: 3, runs: [run('a', 'completed', 'success'), run('b', 'completed', 'skipped'), run('c', 'completed', 'neutral')] }, + { total: 3, runs: [run('typecheck', 'completed', 'success'), run('b', 'completed', 'skipped'), run('c', 'completed', 'neutral')] }, + REQ, )).toEqual({ ok: true }); }); }); describe('runPrAutomerge end to end (mocked GitHub)', () => { const HEAD_SHA = 'abc123def456'; + const original = operatorConfig; let merges: Array>; let checkRuns: unknown; let mergeable: boolean | null; + let logs: string[]; const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); @@ -66,19 +89,30 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { return { env: { githubToken: 'test-token', db } as never, updates }; } + function requireChecks(map: Record) { + setOperatorConfig({ + ...original, + integrations: { ...original.integrations, github: { ...original.integrations.github, automerge: { requiredChecks: map } } }, + } as never); + } + beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(new Date('2026-10-16T22:00:00Z')); // even hour: the task runs merges = []; + logs = []; mergeable = true; checkRuns = { total_count: 1, check_runs: [run('typecheck', 'completed', 'success')] }; + requireChecks({ 'ExampleOrg/demo': ['typecheck'] }); + vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { logs.push(a.join(' ')); }); + vi.spyOn(console, 'warn').mockImplementation((...a: unknown[]) => { logs.push('WARN ' + a.join(' ')); }); vi.stubGlobal('fetch', vi.fn(async (url: string, init?: RequestInit) => { const u = String(url); if (u.endsWith('/pulls/7')) { return json({ number: 7, state: 'open', mergeable, additions: 3, deletions: 1, changed_files: 1, head: { ref: 'auto/task1234', sha: HEAD_SHA }, base: { ref: 'main' } }); } - if (u.endsWith('/pulls/7/files?per_page=100') || u.includes('/pulls/7/files')) return json([{ filename: 'docs/readme.md' }]); - if (u.includes(`/commits/${HEAD_SHA}/status`)) return json({ state: 'pending', total_count: 0, statuses: [] }); + if (u.includes('/pulls/7/files')) return json([{ filename: 'docs/readme.md' }]); + if (u.includes(`/commits/${HEAD_SHA}/status?per_page=100`)) return json({ state: 'pending', total_count: 0, statuses: [] }); if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) return json(checkRuns); if (u.endsWith('/pulls/7/merge') && init?.method === 'PUT') { merges.push(JSON.parse(String(init.body))); @@ -89,23 +123,37 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { }); afterEach(() => { + setOperatorConfig(original as never); vi.useRealTimers(); + vi.restoreAllMocks(); vi.unstubAllGlobals(); }); - it('does not merge an Actions-only PR whose check failed', async () => { + const skipped = (reason: string) => logs.some((l) => l.includes('Skipped ExampleOrg/demo#7') && l.includes(reason)); + + it('does not merge an Actions-only PR whose required check failed', async () => { checkRuns = { total_count: 1, check_runs: [run('typecheck', 'completed', 'failure')] }; const { env: e, updates } = env(); await runPrAutomerge(e); expect(merges).toEqual([]); expect(updates).toEqual([]); + expect(skipped('required check typecheck has not passed')).toBe(true); + }); + + it('does not merge while only some other check has reported', async () => { + checkRuns = { total_count: 1, check_runs: [run('lint', 'completed', 'success')] }; + const { env: e } = env(); + await runPrAutomerge(e); + expect(merges).toEqual([]); + expect(skipped('required check typecheck has not passed')).toBe(true); }); - it('does not merge a PR with no CI at all', async () => { - checkRuns = { total_count: 0, check_runs: [] }; + it('does not merge a repo with no required checks configured', async () => { + requireChecks({}); const { env: e } = env(); await runPrAutomerge(e); expect(merges).toEqual([]); + expect(skipped('no required checks configured')).toBe(true); }); it('does not merge while GitHub has not computed mergeability', async () => { @@ -113,6 +161,7 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { const { env: e } = env(); await runPrAutomerge(e); expect(merges).toEqual([]); + expect(skipped('mergeability not computed yet')).toBe(true); }); it('merges a green PR, pinned to the head commit that was checked', async () => { @@ -121,5 +170,6 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { expect(merges).toHaveLength(1); expect(merges[0]).toMatchObject({ merge_method: 'squash', sha: HEAD_SHA }); expect(updates).toEqual([['https://github.com/ExampleOrg/demo/pull/7/merged', 'task-1234abcd']]); + expect(logs.some((l) => l.startsWith('WARN'))).toBe(false); }); }); From 324c08e17152ff5a4fe397bcb8a28eecd2dba7f9 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 17:23:04 -0500 Subject: [PATCH 4/8] docs(changelog): 0.9.5 required checks per repo Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f05144b..db97f28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## 0.9.5 (2026-10-16) ### Fixed -- **PR auto-merge no longer merges PRs whose GitHub Actions checks failed.** The CI gate read only the legacy combined-status API, which doesn't include Actions check runs and reports `pending` with zero statuses on an Actions-only repo; that was treated as "no CI configured" and passed. The gate now reads the check runs too and fails closed: every commit status must be `success`, every check run completed with `success`, `neutral` or `skipped`, and a PR with no CI at all is not auto-merged. It also waits while GitHub reports mergeability as unknown (it only refused `false`), checks CI on the head commit SHA rather than the branch name, and pins the merge to that SHA so a commit pushed after the checks can't be merged unchecked. New `getCheckRuns()` and `PullRequestStats.headSha`; `mergePullRequest()` takes an optional `sha`. +- **PR auto-merge no longer merges PRs whose GitHub Actions checks failed.** The CI gate read only the legacy combined-status API, which doesn't include Actions check runs and reports `pending` with zero statuses on an Actions-only repo; that was treated as "no CI configured" and passed. The gate now reads the check runs too and fails closed. Without branch protection nothing else knows which checks matter, so the operator config names them per repo (`integrations.github.automerge.requiredChecks`, `owner/name` → check names): each must have succeeded on the head commit (skipped or neutral don't count), every other status and check run there must have passed, and a repo with no entry is never auto-merged. **Existing deployments stop auto-merging until they add this config.** Statuses and check runs are read up to 100 each, and the gate refuses when there are more. It also waits while GitHub reports mergeability as unknown (it only refused `false`), checks CI on the head commit SHA rather than the branch name, and pins the merge to that SHA so a commit pushed after the checks can't be merged unchecked. New `getCheckRuns()` and `PullRequestStats.headSha`; `mergePullRequest()` takes an optional `sha`. ## 0.9.4 (2026-09-24) From 48101a46c22ceed3a403f9b4bd8855fd6b20b21a Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 17:31:03 -0500 Subject: [PATCH 5/8] fix(automerge): a PR can't rewrite what judges it; required checks must be Actions runs (0.10.0) Second review of #89: the gate matched required checks by name, and the PR controls what runs under that name. - Per-category file allowlist replaces the sensitive-file list: docs may touch Markdown and docs/ text/images, research research/ and Markdown, tests may only add new test files. .github/, dotfiles, existing tests and config mean no auto-merge. Renames are judged by both paths; refuse when not every changed file could be read. - A required check counts only as a completed github-actions check run concluding success (not a status or another app's run). - 0.10.0 with a Breaking note: auto-merge is off until configured. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 +++- web/package.json | 2 +- web/src/github.ts | 30 +++++++- web/src/kernel/scheduled/pr-automerge.ts | 90 ++++++++++++++++-------- web/src/version.ts | 2 +- web/tests/pr-automerge-ci-gate.test.ts | 73 +++++++++++++++++-- 6 files changed, 168 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db97f28..0ddee1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,18 @@ # Changelog -## 0.9.5 (2026-10-16) +## 0.10.0 (2026-10-16) + +### Breaking +- **PR auto-merge is off until you configure it.** It now merges a repo's task PRs only when the operator config lists that repo's required checks (`integrations.github.automerge.requiredChecks`: `owner/name` → GitHub Actions job names). Existing deployments stop auto-merging until they add the entry. Consumers with their own copy of `OperatorConfig` need the new optional `automerge` field. ### Fixed -- **PR auto-merge no longer merges PRs whose GitHub Actions checks failed.** The CI gate read only the legacy combined-status API, which doesn't include Actions check runs and reports `pending` with zero statuses on an Actions-only repo; that was treated as "no CI configured" and passed. The gate now reads the check runs too and fails closed. Without branch protection nothing else knows which checks matter, so the operator config names them per repo (`integrations.github.automerge.requiredChecks`, `owner/name` → check names): each must have succeeded on the head commit (skipped or neutral don't count), every other status and check run there must have passed, and a repo with no entry is never auto-merged. **Existing deployments stop auto-merging until they add this config.** Statuses and check runs are read up to 100 each, and the gate refuses when there are more. It also waits while GitHub reports mergeability as unknown (it only refused `false`), checks CI on the head commit SHA rather than the branch name, and pins the merge to that SHA so a commit pushed after the checks can't be merged unchecked. New `getCheckRuns()` and `PullRequestStats.headSha`; `mergePullRequest()` takes an optional `sha`. +- **PR auto-merge no longer merges PRs whose CI failed or didn't run.** The gate read only the legacy combined-status API, which doesn't include GitHub Actions check runs and reports `pending` with zero statuses on an Actions-only repo; that was treated as "no CI configured" and passed. Now, on the PR's head commit: + - every required check must be a completed GitHub Actions check run (app `github-actions`) concluding `success` — skipped, neutral, a commit status or another app's check run of the same name don't count; + - every other commit status must be `success` and every other check run completed with `success`, `neutral` or `skipped`; + - statuses, check runs and changed files are each read up to 100, and the gate refuses if there are more; + - GitHub must report the PR mergeable (unknown no longer passes), and the merge is pinned to the checked head SHA. +- **A task PR can't rewrite what judges it.** The sensitive-file list is replaced by a per-category allowlist: docs PRs may touch Markdown and `docs/` text and images; research PRs `research/` and Markdown; tests PRs may only add new test files. Anything else — `.github/`, dotfiles, existing tests, any config — means no auto-merge. Renames are judged by both paths. +- New `getCheckRuns()`, `getPullRequestFileDetails()`, `PullRequestStats.headSha`; `mergePullRequest()` takes an optional `sha`; `getCombinedStatus()` reads 100 statuses. ## 0.9.4 (2026-09-24) diff --git a/web/package.json b/web/package.json index f8882d5..ff3c98f 100755 --- a/web/package.json +++ b/web/package.json @@ -1,6 +1,6 @@ { "name": "@stackbilt/aegis-core", - "version": "0.9.5", + "version": "0.10.0", "description": "Persistent AI agent framework for Cloudflare Workers. Multi-tier memory, autonomous goals, dreaming cycles, MCP native.", "license": "Apache-2.0", "publishConfig": { diff --git a/web/src/github.ts b/web/src/github.ts index f9be7f9..ee80bae 100755 --- a/web/src/github.ts +++ b/web/src/github.ts @@ -601,7 +601,7 @@ export async function getCombinedStatus( export interface CheckRunSummary { total: number; - runs: { name: string; status: string; conclusion: string | null }[]; + runs: { name: string; status: string; conclusion: string | null; app: string | null }[]; } /** @@ -616,11 +616,14 @@ export async function getCheckRuns( const res = await ghFetch(token, `/repos/${repo}/commits/${encodeURIComponent(ref)}/check-runs?per_page=100`); const data = (await res.json()) as { total_count: number; - check_runs: { name: string; status: string; conclusion: string | null }[]; + check_runs: { name: string; status: string; conclusion: string | null; app?: { slug?: string } | null }[]; }; + // GitHub's default filter=latest keeps only the most recent run per check + // name, so a passing re-run replaces a failed attempt. filter=all would make + // every re-run failure block forever. return { total: data.total_count, - runs: data.check_runs.map((r) => ({ name: r.name, status: r.status, conclusion: r.conclusion })), + runs: data.check_runs.map((r) => ({ name: r.name, status: r.status, conclusion: r.conclusion, app: r.app?.slug ?? null })), }; } @@ -680,6 +683,27 @@ export async function getPullRequestFiles( return data.map(f => f.filename); } +export interface PullRequestFile { + filename: string; + status: string; // added | modified | removed | renamed | copied | changed | unchanged + previousFilename?: string; +} + +/** Changed files with their status and, for renames, the old path. Reads up to 100. */ +export async function getPullRequestFileDetails( + token: string, + repo: string, + pullNumber: number, +): Promise { + const res = await ghFetch(token, `/repos/${repo}/pulls/${pullNumber}/files?per_page=100`); + const data = (await res.json()) as { filename: string; status: string; previous_filename?: string }[]; + return data.map((f) => ({ + filename: f.filename, + status: f.status, + ...(f.previous_filename ? { previousFilename: f.previous_filename } : {}), + })); +} + // ─── PR Merge ──────────────────────────────────────────────── export async function mergePullRequest( diff --git a/web/src/kernel/scheduled/pr-automerge.ts b/web/src/kernel/scheduled/pr-automerge.ts index 1abde1e..2d1e72e 100644 --- a/web/src/kernel/scheduled/pr-automerge.ts +++ b/web/src/kernel/scheduled/pr-automerge.ts @@ -9,7 +9,9 @@ // commit, and every other status and check run there passed. A repo with // no required checks is never auto-merged. // 3. Diff is under LOC cap -// 4. No sensitive files touched +// 4. Every changed file is one the category may touch (docs: Markdown and +// docs/; research: research/ and Markdown; tests: new test files only), +// so a PR can't edit CI, tests or config that judge it // 5. PR is open and GitHub reports it mergeable (unknown is not enough); // the merge is pinned to the head commit that was checked // @@ -20,7 +22,8 @@ import { getCombinedStatus, getCheckRuns, getPullRequestStats, - getPullRequestFiles, + getPullRequestFileDetails, + type PullRequestFile, mergePullRequest, } from '../../github.js'; import { operatorConfig } from '../../operator/index.js'; @@ -32,16 +35,42 @@ import { AUTOMERGE_SAFE_CATEGORIES } from '../../schema-enums.js'; /** Max total LOC changed (additions + deletions) */ const MAX_LOC = 500; -/** Files that block auto-merge if touched */ -const SENSITIVE_PATTERNS = [ - /\.env/i, - /secret/i, - /credential/i, - /wrangler\.toml$/, - /package\.json$/, - /package-lock\.json$/, - /pnpm-lock\.yaml$/, -]; +/** + * What each category's PR may touch. The PR controls the workflows, tests and + * config that produce its own checks, so anything outside these is refused -- + * including .github/, existing tests and every config file. + */ +const MARKDOWN = /\.(md|mdx)$/i; +const DOCS_ASSET = /\.(md|mdx|txt|png|jpe?g|gif|svg|webp)$/i; +const TEST_FILE = /(^|\/)(tests?|__tests__)\/.+\.(test|spec)\.[cm]?[jt]sx?$|\.(test|spec)\.[cm]?[jt]sx?$/; + +function fileAllowed(category: string, file: PullRequestFile): boolean { + const paths = [file.filename, ...(file.previousFilename ? [file.previousFilename] : [])]; + if (paths.some((p) => p.startsWith('.github/') || /(^|\/)\./.test(p))) return false; + switch (category) { + case 'docs': + return paths.every((p) => MARKDOWN.test(p) || (p.startsWith('docs/') && DOCS_ASSET.test(p))); + case 'research': + return paths.every((p) => p.startsWith('research/') || MARKDOWN.test(p)); + case 'tests': + // New test files only: editing or deleting an existing test could weaken + // the very checks that are about to judge this PR. + return file.status === 'added' && TEST_FILE.test(file.filename); + default: + return false; + } +} + +/** First out-of-scope file for the category, as a skip reason, or null. */ +export function filesOutsideCategory(category: string, files: PullRequestFile[]): string | null { + for (const f of files) { + if (!fileAllowed(category, f)) { + const from = f.previousFilename ? ` (renamed from ${f.previousFilename})` : ''; + return `${category} PR touches ${f.filename}${from} (${f.status}), outside what ${category} tasks may change`; + } + } + return null; +} /** Max PRs to process per run (rate limiting) */ const MAX_PER_RUN = 3; @@ -162,7 +191,7 @@ async function evaluateAndMerge( const { githubToken } = env; if (!githubToken) return { merged: false, reason: 'no token' }; - // Gate 1: PR stats (open, mergeable, LOC) + // Gate: PR stats (open, mergeable, LOC) const stats = await getPullRequestStats(githubToken, candidate.ghRepo, candidate.prNumber); if (stats.state !== 'open') { @@ -178,17 +207,17 @@ async function evaluateAndMerge( return { merged: false, reason: `LOC ${totalLoc} > ${MAX_LOC} cap` }; } - // Gate 2: Sensitive files - const files = await getPullRequestFiles(githubToken, candidate.ghRepo, candidate.prNumber); - for (const file of files) { - for (const pattern of SENSITIVE_PATTERNS) { - if (pattern.test(file)) { - return { merged: false, reason: `sensitive file: ${file}` }; - } - } + // Gate: every changed file is one the category may touch + const files = await getPullRequestFileDetails(githubToken, candidate.ghRepo, candidate.prNumber); + if (stats.changed_files > files.length) { + return { merged: false, reason: `too many changed files to verify (${stats.changed_files})` }; + } + const outOfScope = filesOutsideCategory(candidate.category, files); + if (outOfScope) { + return { merged: false, reason: outOfScope }; } - // Gate 3: CI on the exact head commit -- legacy statuses and check runs + // Gate: CI on the exact head commit -- legacy statuses and check runs const [ciStatus, checkRuns] = await Promise.all([ getCombinedStatus(githubToken, candidate.ghRepo, stats.headSha), getCheckRuns(githubToken, candidate.ghRepo, stats.headSha), @@ -224,13 +253,16 @@ const PASSING_CONCLUSIONS = new Set(['success', 'neutral', 'skipped']); /** * Fail closed. GitHub Actions report through the Checks API, not commit * statuses, and the combined status reads "pending" with zero statuses when - * a repo only uses Actions -- which used to pass. Now every status must be - * success, every check run completed with a passing conclusion, and at least - * one of either must exist. + * a repo only uses Actions -- which used to pass. Now: the repo must have + * required checks configured; each must be a completed GitHub Actions check + * run concluding `success` (skipped or neutral don't count); every other + * commit status must be `success` and every other check run completed with + * `success`, `neutral` or `skipped`; and everything must have been readable + * in one page of 100. */ export function ciVerdict( status: { total: number; statuses: { context: string; state: string }[] }, - checks: { total: number; runs: { name: string; status: string; conclusion: string | null }[] }, + checks: { total: number; runs: { name: string; status: string; conclusion: string | null; app?: string | null }[] }, required: string[], ): { ok: true } | { ok: false; reason: string } { // "Some check is green" is not "CI passed": a job waiting on another job, a @@ -247,8 +279,10 @@ export function ciVerdict( return { ok: false, reason: `too many commit statuses to verify (${status.total})` }; } for (const name of required) { - const passed = checks.runs.some((r) => r.name === name && r.status === 'completed' && r.conclusion === 'success') - || status.statuses.some((s) => s.context === name && s.state === 'success'); + // Only a GitHub Actions check run counts: a commit status or another + // app's check run can be posted under any name by anything with a token. + const passed = checks.runs.some((r) => + r.name === name && r.app === 'github-actions' && r.status === 'completed' && r.conclusion === 'success'); if (!passed) return { ok: false, reason: `required check ${name} has not passed` }; } for (const s of status.statuses) { diff --git a/web/src/version.ts b/web/src/version.ts index 590f4dd..f9532fa 100755 --- a/web/src/version.ts +++ b/web/src/version.ts @@ -1,3 +1,3 @@ // Semantic version - bump on each deploy // Used in /health endpoint and changelog tracking -export const VERSION = '0.9.5'; +export const VERSION = '0.10.0'; diff --git a/web/tests/pr-automerge-ci-gate.test.ts b/web/tests/pr-automerge-ci-gate.test.ts index ebfa8ec..25ebc46 100644 --- a/web/tests/pr-automerge-ci-gate.test.ts +++ b/web/tests/pr-automerge-ci-gate.test.ts @@ -6,11 +6,11 @@ // protection, the operator config names the checks each repo must pass. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { ciVerdict, runPrAutomerge } from '../src/kernel/scheduled/pr-automerge.js'; +import { ciVerdict, filesOutsideCategory, runPrAutomerge } from '../src/kernel/scheduled/pr-automerge.js'; import { operatorConfig, setOperatorConfig } from '../src/operator/index.js'; const noStatuses = { total: 0, statuses: [] }; -const run = (name: string, status: string, conclusion: string | null) => ({ name, status, conclusion }); +const run = (name: string, status: string, conclusion: string | null, app: string | null = 'github-actions') => ({ name, status, conclusion, app }); const REQ = ['typecheck']; describe('ciVerdict', () => { @@ -49,8 +49,9 @@ describe('ciVerdict', () => { .toEqual({ ok: false, reason: 'too many commit statuses to verify (31)' }); }); - it('accepts a required check reported as a commit status', () => { - expect(ciVerdict({ total: 1, statuses: [{ context: 'typecheck', state: 'success' }] }, { total: 0, runs: [] }, REQ)).toEqual({ ok: true }); + it('only a GitHub Actions check run satisfies a required check, not a status or another app', () => { + expect(ciVerdict({ total: 1, statuses: [{ context: 'typecheck', state: 'success' }] }, { total: 0, runs: [] }, REQ).ok).toBe(false); + expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'success', 'some-other-app')] }, REQ).ok).toBe(false); }); it('passes when every required check succeeded and the rest passed (skipped extras allowed)', () => { @@ -62,6 +63,39 @@ describe('ciVerdict', () => { }); }); +describe('filesOutsideCategory', () => { + const f = (filename: string, status = 'modified', previousFilename?: string) => ({ filename, status, ...(previousFilename ? { previousFilename } : {}) }); + + it('docs may touch Markdown and docs/ assets only', () => { + expect(filesOutsideCategory('docs', [f('README.md'), f('docs/guide.md'), f('docs/img/a.png', 'added')])).toBeNull(); + expect(filesOutsideCategory('docs', [f('docs/build.config.js')])).toMatch(/outside what docs tasks may change/); + expect(filesOutsideCategory('docs', [f('src/index.ts')])).not.toBeNull(); + }); + + it('no category may touch .github/ or dotfiles, even as Markdown', () => { + expect(filesOutsideCategory('docs', [f('.github/workflows/ci.yml')])).not.toBeNull(); + expect(filesOutsideCategory('docs', [f('.github/PULL_REQUEST_TEMPLATE.md')])).not.toBeNull(); + expect(filesOutsideCategory('research', [f('.ai/notes.md')])).not.toBeNull(); + }); + + it('tests may only add new test files, never edit or delete existing ones or config', () => { + expect(filesOutsideCategory('tests', [f('tests/new.test.ts', 'added'), f('src/__tests__/x.spec.ts', 'added')])).toBeNull(); + expect(filesOutsideCategory('tests', [f('tests/gate.test.ts', 'modified')])).not.toBeNull(); + expect(filesOutsideCategory('tests', [f('tests/gate.test.ts', 'removed')])).not.toBeNull(); + expect(filesOutsideCategory('tests', [f('vitest.config.ts', 'modified')])).not.toBeNull(); + expect(filesOutsideCategory('tests', [f('tests/helpers.ts', 'added')])).not.toBeNull(); + }); + + it('a rename is judged by both paths', () => { + expect(filesOutsideCategory('docs', [f('docs/notes.md', 'renamed', 'wrangler.toml')])).toMatch(/renamed from wrangler\.toml/); + }); + + it('research may touch research/ and Markdown', () => { + expect(filesOutsideCategory('research', [f('research/2026-10-16-x.md', 'added'), f('research/data.json', 'added')])).toBeNull(); + expect(filesOutsideCategory('research', [f('src/x.ts')])).not.toBeNull(); + }); +}); + describe('runPrAutomerge end to end (mocked GitHub)', () => { const HEAD_SHA = 'abc123def456'; const original = operatorConfig; @@ -69,6 +103,8 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { let checkRuns: unknown; let mergeable: boolean | null; let logs: string[]; + let files: unknown[]; + let changedFiles: number; const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); @@ -102,6 +138,8 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { merges = []; logs = []; mergeable = true; + files = [{ filename: 'docs/readme.md', status: 'modified' }]; + changedFiles = 1; checkRuns = { total_count: 1, check_runs: [run('typecheck', 'completed', 'success')] }; requireChecks({ 'ExampleOrg/demo': ['typecheck'] }); vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { logs.push(a.join(' ')); }); @@ -109,11 +147,15 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { vi.stubGlobal('fetch', vi.fn(async (url: string, init?: RequestInit) => { const u = String(url); if (u.endsWith('/pulls/7')) { - return json({ number: 7, state: 'open', mergeable, additions: 3, deletions: 1, changed_files: 1, head: { ref: 'auto/task1234', sha: HEAD_SHA }, base: { ref: 'main' } }); + return json({ number: 7, state: 'open', mergeable, additions: 3, deletions: 1, changed_files: changedFiles, head: { ref: 'auto/task1234', sha: HEAD_SHA }, base: { ref: 'main' } }); } - if (u.includes('/pulls/7/files')) return json([{ filename: 'docs/readme.md' }]); + if (u.includes('/pulls/7/files')) return json(files); if (u.includes(`/commits/${HEAD_SHA}/status?per_page=100`)) return json({ state: 'pending', total_count: 0, statuses: [] }); - if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) return json(checkRuns); + if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) { + // The API reports the producing app as { slug }. + const cr = checkRuns as { total_count: number; check_runs: Array<{ app: string | null }> }; + return json({ ...cr, check_runs: cr.check_runs.map((r) => ({ ...r, app: r.app ? { slug: r.app } : null })) }); + } if (u.endsWith('/pulls/7/merge') && init?.method === 'PUT') { merges.push(JSON.parse(String(init.body))); return json({ sha: 'merged-sha', merged: true, message: 'Pull Request successfully merged' }); @@ -164,6 +206,23 @@ describe('runPrAutomerge end to end (mocked GitHub)', () => { expect(skipped('mergeability not computed yet')).toBe(true); }); + it('does not merge a docs PR that edits a workflow, even with a green required check', async () => { + files = [{ filename: 'docs/readme.md', status: 'modified' }, { filename: '.github/workflows/ci.yml', status: 'modified' }]; + changedFiles = 2; + const { env: e } = env(); + await runPrAutomerge(e); + expect(merges).toEqual([]); + expect(skipped('touches .github/workflows/ci.yml')).toBe(true); + }); + + it('does not merge when not every changed file could be read', async () => { + changedFiles = 101; + const { env: e } = env(); + await runPrAutomerge(e); + expect(merges).toEqual([]); + expect(skipped('too many changed files to verify (101)')).toBe(true); + }); + it('merges a green PR, pinned to the head commit that was checked', async () => { const { env: e, updates } = env(); await runPrAutomerge(e); From f7ff02b61913ba183f6fcd72e24978a25d0f99f9 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 17:57:15 -0500 Subject: [PATCH 6/8] feat!: remove PR auto-merge; flag green task PRs ready for a human merge (0.10.0) Four review rounds on a hardened auto-merge gate showed no file category is safe to merge by rule without branch protection: docs steer later agents, Markdown is served on some sites, and new tests are code that runs in CI beside deploy credentials. The pr-automerge heartbeat is replaced by pr-ready, which labels a PR aegis:ready-to-merge when it comes from an auto/ branch in the same repo, targets the default branch, is mergeable, and its CI on the head commit passed (at least one successful GitHub Actions run; optional per-repo required checks). It never merges. Tasks are read newest first and closed PRs are marked done, ending starvation. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 17 +- README.md | 2 +- web/src/github.ts | 31 +-- web/src/kernel/scheduled/index.ts | 4 +- web/src/kernel/scheduled/pr-automerge.ts | 298 ----------------------- web/src/kernel/scheduled/pr-ready.ts | 186 ++++++++++++++ web/src/operator/config.example.ts | 4 +- web/src/operator/types.ts | 11 +- web/src/schema-enums.ts | 3 - web/tests/pr-automerge-ci-gate.test.ts | 234 ------------------ web/tests/pr-ready.test.ts | 161 ++++++++++++ 11 files changed, 373 insertions(+), 578 deletions(-) delete mode 100644 web/src/kernel/scheduled/pr-automerge.ts create mode 100644 web/src/kernel/scheduled/pr-ready.ts delete mode 100644 web/tests/pr-automerge-ci-gate.test.ts create mode 100644 web/tests/pr-ready.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ddee1f..9e20a11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,16 +3,15 @@ ## 0.10.0 (2026-10-16) ### Breaking -- **PR auto-merge is off until you configure it.** It now merges a repo's task PRs only when the operator config lists that repo's required checks (`integrations.github.automerge.requiredChecks`: `owner/name` → GitHub Actions job names). Existing deployments stop auto-merging until they add the entry. Consumers with their own copy of `OperatorConfig` need the new optional `automerge` field. +- **PR auto-merge is removed.** The `pr-automerge` heartbeat merged completed docs/tests/research task PRs, and its CI gate read only the legacy combined-status API, which doesn't include GitHub Actions check runs and reports `pending` with zero statuses on an Actions-only repo. That was treated as "no CI configured", so a PR whose checks all failed could be merged. Four review rounds on a hardened gate showed that no file category is safe to merge by rule without branch protection: docs steer later agents (`CLAUDE.md`, `AGENTS.md`, prompts, skills), Markdown is served as pages on some sites, and a new test file is code that runs in CI next to deploy credentials. A human merge is the control. -### Fixed -- **PR auto-merge no longer merges PRs whose CI failed or didn't run.** The gate read only the legacy combined-status API, which doesn't include GitHub Actions check runs and reports `pending` with zero statuses on an Actions-only repo; that was treated as "no CI configured" and passed. Now, on the PR's head commit: - - every required check must be a completed GitHub Actions check run (app `github-actions`) concluding `success` — skipped, neutral, a commit status or another app's check run of the same name don't count; - - every other commit status must be `success` and every other check run completed with `success`, `neutral` or `skipped`; - - statuses, check runs and changed files are each read up to 100, and the gate refuses if there are more; - - GitHub must report the PR mergeable (unknown no longer passes), and the merge is pinned to the checked head SHA. -- **A task PR can't rewrite what judges it.** The sensitive-file list is replaced by a per-category allowlist: docs PRs may touch Markdown and `docs/` text and images; research PRs `research/` and Markdown; tests PRs may only add new test files. Anything else — `.github/`, dotfiles, existing tests, any config — means no auto-merge. Renames are judged by both paths. -- New `getCheckRuns()`, `getPullRequestFileDetails()`, `PullRequestStats.headSha`; `mergePullRequest()` takes an optional `sha`; `getCombinedStatus()` reads 100 statuses. +### Added +- **PR ready flag** (`pr-ready` heartbeat, every 2 h). It labels a completed task's PR `aegis:ready-to-merge` when: + - it is open, comes from an `auto/` branch in the same repo, and targets the repo's default branch; + - GitHub reports it mergeable; + - on the head commit, every commit status is `success`, every check run completed and passing, and at least one GitHub Actions check run succeeded. + Optional per-repo required checks (`integrations.github.prReady.requiredChecks`: `owner/name` → GitHub Actions job names) must each be successful Actions runs. Statuses and check runs are read up to 100 each; more is refused. Tasks are read newest first, and closed or merged PRs are marked done on the task row (`/closed`), so old PRs no longer starve new ones. +- `getCheckRuns()`, `PullRequestStats.headSha`, `headRepo` and `baseDefaultBranch`; `mergePullRequest()` takes an optional `sha` to pin the head; `getCombinedStatus()` reads 100 statuses. ## 0.9.4 (2026-09-24) diff --git a/README.md b/README.md index 6d59bd9..3105be5 100644 --- a/README.md +++ b/README.md @@ -187,7 +187,7 @@ AEGIS runs 26 tasks on an hourly cron, split into heartbeat (always-run) and tim | CI Watcher | Hourly | Monitor GitHub Actions runs | | ARGUS Notify | Hourly | Classify webhook events, route alerts | | Cognitive Metrics | Daily | Classifier accuracy, dispatch cost tracking | -| PR Automerge | Hourly | Auto-merge approved docs/tests PRs | +| PR ready flag | Every 2 h | Labels green autonomous-task PRs `aegis:ready-to-merge` for a human merge (never merges) | | Consolidation | Hourly | Memory dedup, decay, promotion, dynamic tool GC | | Heartbeat | 6h | System health + email digest | | Product Health | Hourly | Worker availability checks | diff --git a/web/src/github.ts b/web/src/github.ts index ee80bae..c2507fa 100755 --- a/web/src/github.ts +++ b/web/src/github.ts @@ -639,7 +639,11 @@ export interface PullRequestStats { head: string; /** Head commit SHA: what CI ran on, and what a merge must pin to. */ headSha: string; + /** `owner/name` of the head branch's repo; null if the fork was deleted. */ + headRepo: string | null; base: string; + /** The base repo's default branch. */ + baseDefaultBranch: string; } export async function getPullRequestStats( @@ -655,8 +659,8 @@ export async function getPullRequestStats( additions: number; deletions: number; changed_files: number; - head: { ref: string; sha: string }; - base: { ref: string }; + head: { ref: string; sha: string; repo: { full_name: string } | null }; + base: { ref: string; repo: { default_branch: string } }; }; return { number: data.number, @@ -667,7 +671,9 @@ export async function getPullRequestStats( changed_files: data.changed_files, head: data.head.ref, headSha: data.head.sha, + headRepo: data.head.repo?.full_name ?? null, base: data.base.ref, + baseDefaultBranch: data.base.repo.default_branch, }; } @@ -683,27 +689,6 @@ export async function getPullRequestFiles( return data.map(f => f.filename); } -export interface PullRequestFile { - filename: string; - status: string; // added | modified | removed | renamed | copied | changed | unchanged - previousFilename?: string; -} - -/** Changed files with their status and, for renames, the old path. Reads up to 100. */ -export async function getPullRequestFileDetails( - token: string, - repo: string, - pullNumber: number, -): Promise { - const res = await ghFetch(token, `/repos/${repo}/pulls/${pullNumber}/files?per_page=100`); - const data = (await res.json()) as { filename: string; status: string; previous_filename?: string }[]; - return data.map((f) => ({ - filename: f.filename, - status: f.status, - ...(f.previous_filename ? { previousFilename: f.previous_filename } : {}), - })); -} - // ─── PR Merge ──────────────────────────────────────────────── export async function mergePullRequest( diff --git a/web/src/kernel/scheduled/index.ts b/web/src/kernel/scheduled/index.ts index 8ec47bf..e2a3eb0 100755 --- a/web/src/kernel/scheduled/index.ts +++ b/web/src/kernel/scheduled/index.ts @@ -18,7 +18,7 @@ import { runArgusNotifications } from './argus-notify.js'; import { runArgusHeartbeat } from './argus-heartbeat.js'; import { runCognitiveMetrics } from './cognitive-metrics.js'; import { runArgusAnalytics } from './argus-analytics.js'; -import { runPrAutomerge } from './pr-automerge.js'; +import { runPrReadyFlag } from './pr-ready.js'; import { runFeedWatcher } from './feed-watcher.js'; import { runCostReport } from './cost-report.js'; import { runBoardSync } from './board-sync.js'; @@ -134,7 +134,7 @@ async function runHeartbeatPhase(env: EdgeEnv): Promise { await runTask(env, 'argus-notify', 'heartbeat', runArgusNotifications); await runTask(env, 'argus-heartbeat', 'heartbeat', runArgusHeartbeat); await runTask(env, 'cognitive-metrics', 'heartbeat', runCognitiveMetrics); - await runTask(env, 'pr-automerge', 'heartbeat', runPrAutomerge); + await runTask(env, 'pr-ready', 'heartbeat', runPrReadyFlag); // Agent inbox -- process unread messages from peer agents await runTask(env, 'inbox', 'heartbeat', runInboxProcessor); diff --git a/web/src/kernel/scheduled/pr-automerge.ts b/web/src/kernel/scheduled/pr-automerge.ts deleted file mode 100644 index 2d1e72e..0000000 --- a/web/src/kernel/scheduled/pr-automerge.ts +++ /dev/null @@ -1,298 +0,0 @@ -// --- PR Auto-Merge --- -// Closes the self-improvement loop by auto-merging PRs from -// autonomous tasks that meet safety criteria. -// -// Gates: -// 1. Task category is auto_safe (docs, tests, research) -// 2. CI is green: every check the operator config requires for the repo -// (integrations.github.automerge.requiredChecks) succeeded on the head -// commit, and every other status and check run there passed. A repo with -// no required checks is never auto-merged. -// 3. Diff is under LOC cap -// 4. Every changed file is one the category may touch (docs: Markdown and -// docs/; research: research/ and Markdown; tests: new test files only), -// so a PR can't edit CI, tests or config that judge it -// 5. PR is open and GitHub reports it mergeable (unknown is not enough); -// the merge is pinned to the head commit that was checked -// -// Runs in Phase 1 (free -- GitHub API only, no LLM). - -import { type EdgeEnv } from '../dispatch.js'; -import { - getCombinedStatus, - getCheckRuns, - getPullRequestStats, - getPullRequestFileDetails, - type PullRequestFile, - mergePullRequest, -} from '../../github.js'; -import { operatorConfig } from '../../operator/index.js'; - -import { AUTOMERGE_SAFE_CATEGORIES } from '../../schema-enums.js'; - -// --- Configuration --- - -/** Max total LOC changed (additions + deletions) */ -const MAX_LOC = 500; - -/** - * What each category's PR may touch. The PR controls the workflows, tests and - * config that produce its own checks, so anything outside these is refused -- - * including .github/, existing tests and every config file. - */ -const MARKDOWN = /\.(md|mdx)$/i; -const DOCS_ASSET = /\.(md|mdx|txt|png|jpe?g|gif|svg|webp)$/i; -const TEST_FILE = /(^|\/)(tests?|__tests__)\/.+\.(test|spec)\.[cm]?[jt]sx?$|\.(test|spec)\.[cm]?[jt]sx?$/; - -function fileAllowed(category: string, file: PullRequestFile): boolean { - const paths = [file.filename, ...(file.previousFilename ? [file.previousFilename] : [])]; - if (paths.some((p) => p.startsWith('.github/') || /(^|\/)\./.test(p))) return false; - switch (category) { - case 'docs': - return paths.every((p) => MARKDOWN.test(p) || (p.startsWith('docs/') && DOCS_ASSET.test(p))); - case 'research': - return paths.every((p) => p.startsWith('research/') || MARKDOWN.test(p)); - case 'tests': - // New test files only: editing or deleting an existing test could weaken - // the very checks that are about to judge this PR. - return file.status === 'added' && TEST_FILE.test(file.filename); - default: - return false; - } -} - -/** First out-of-scope file for the category, as a skip reason, or null. */ -export function filesOutsideCategory(category: string, files: PullRequestFile[]): string | null { - for (const f of files) { - if (!fileAllowed(category, f)) { - const from = f.previousFilename ? ` (renamed from ${f.previousFilename})` : ''; - return `${category} PR touches ${f.filename}${from} (${f.status}), outside what ${category} tasks may change`; - } - } - return null; -} - -/** Max PRs to process per run (rate limiting) */ -const MAX_PER_RUN = 3; - -/** Repos where auto-merge is disabled (production deploy targets) */ -const PROTECTED_REPOS = new Set([ - // Add your production repos here, e.g.: - // 'ExampleOrg/my-production-app', -]); - -// --- Types --- - -interface MergeCandidate { - taskId: string; - title: string; - repo: string; - category: string; - prUrl: string; - prNumber: number; - ghRepo: string; -} - -// --- Main --- - -export async function runPrAutomerge(env: EdgeEnv): Promise { - if (!env.githubToken) return; - - // Time guard: run every 2 hours (00, 02, 04, ..., 22 UTC) - const hour = new Date().getUTCHours(); - if (hour % 2 !== 0) return; - - // Find completed tasks with PRs in safe categories - const rows = await env.db.prepare(` - SELECT id, title, repo, category, pr_url - FROM cc_tasks - WHERE status = 'completed' - AND pr_url IS NOT NULL - AND category IN ('docs', 'tests', 'research') - AND pr_url NOT LIKE '%/merged%' - ORDER BY completed_at ASC - LIMIT ? - `).bind(MAX_PER_RUN * 2).all<{ - id: string; - title: string; - repo: string; - category: string; - pr_url: string; - }>(); - - if (rows.results.length === 0) return; - - // Parse PR URLs into merge candidates - const candidates: MergeCandidate[] = []; - for (const row of rows.results) { - const match = row.pr_url.match(/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)/); - if (!match) continue; - - const ghRepo = match[1]; - const prNumber = parseInt(match[2], 10); - - // Skip protected repos - if (PROTECTED_REPOS.has(ghRepo)) { - console.log(`[pr-automerge] Skipped ${ghRepo}#${prNumber} -- protected repo`); - continue; - } - - // Double-check category - if (!AUTOMERGE_SAFE_CATEGORIES.has(row.category as any)) continue; - - candidates.push({ - taskId: row.id, - title: row.title, - repo: row.repo, - category: row.category, - prUrl: row.pr_url, - prNumber, - ghRepo, - }); - } - - let merged = 0; - - for (const candidate of candidates.slice(0, MAX_PER_RUN)) { - try { - const result = await evaluateAndMerge(env, candidate); - if (result.merged) { - merged++; - // Mark in cc_tasks so we don't re-process - await env.db.prepare( - `UPDATE cc_tasks SET pr_url = ? WHERE id = ?` - ).bind(`${candidate.prUrl}/merged`, candidate.taskId).run(); - console.log(`[pr-automerge] Merged ${candidate.ghRepo}#${candidate.prNumber} -- "${candidate.title}"`); - } else { - console.log(`[pr-automerge] Skipped ${candidate.ghRepo}#${candidate.prNumber}: ${result.reason}`); - } - } catch (err) { - console.warn(`[pr-automerge] Error processing ${candidate.ghRepo}#${candidate.prNumber}:`, - err instanceof Error ? err.message : String(err)); - } - } - - if (merged > 0) { - console.log(`[pr-automerge] Merged ${merged} PR(s) this cycle`); - } -} - -// --- Evaluation --- - -interface EvalResult { - merged: boolean; - reason?: string; -} - -async function evaluateAndMerge( - env: EdgeEnv, - candidate: MergeCandidate, -): Promise { - const { githubToken } = env; - if (!githubToken) return { merged: false, reason: 'no token' }; - - // Gate: PR stats (open, mergeable, LOC) - const stats = await getPullRequestStats(githubToken, candidate.ghRepo, candidate.prNumber); - - if (stats.state !== 'open') { - return { merged: false, reason: `PR state: ${stats.state}` }; - } - - if (stats.mergeable !== true) { - return { merged: false, reason: stats.mergeable === false ? 'not mergeable (conflicts)' : 'mergeability not computed yet' }; - } - - const totalLoc = stats.additions + stats.deletions; - if (totalLoc > MAX_LOC) { - return { merged: false, reason: `LOC ${totalLoc} > ${MAX_LOC} cap` }; - } - - // Gate: every changed file is one the category may touch - const files = await getPullRequestFileDetails(githubToken, candidate.ghRepo, candidate.prNumber); - if (stats.changed_files > files.length) { - return { merged: false, reason: `too many changed files to verify (${stats.changed_files})` }; - } - const outOfScope = filesOutsideCategory(candidate.category, files); - if (outOfScope) { - return { merged: false, reason: outOfScope }; - } - - // Gate: CI on the exact head commit -- legacy statuses and check runs - const [ciStatus, checkRuns] = await Promise.all([ - getCombinedStatus(githubToken, candidate.ghRepo, stats.headSha), - getCheckRuns(githubToken, candidate.ghRepo, stats.headSha), - ]); - const required = operatorConfig.integrations.github.automerge?.requiredChecks?.[candidate.ghRepo] ?? []; - const ci = ciVerdict(ciStatus, checkRuns, required); - if (!ci.ok) { - return { merged: false, reason: ci.reason }; - } - - // All gates passed -- merge with squash - const commitMsg = `${candidate.title} (#${candidate.prNumber})\n\nAuto-merged by AEGIS (category: ${candidate.category}, task: ${candidate.taskId.slice(0, 8)})`; - const mergeResult = await mergePullRequest( - githubToken, - candidate.ghRepo, - candidate.prNumber, - 'squash', - commitMsg, - stats.headSha, - ); - - if (!mergeResult.merged) { - return { merged: false, reason: `merge failed: ${mergeResult.message}` }; - } - - return { merged: true }; -} - -// --- CI verdict --- - -const PASSING_CONCLUSIONS = new Set(['success', 'neutral', 'skipped']); - -/** - * Fail closed. GitHub Actions report through the Checks API, not commit - * statuses, and the combined status reads "pending" with zero statuses when - * a repo only uses Actions -- which used to pass. Now: the repo must have - * required checks configured; each must be a completed GitHub Actions check - * run concluding `success` (skipped or neutral don't count); every other - * commit status must be `success` and every other check run completed with - * `success`, `neutral` or `skipped`; and everything must have been readable - * in one page of 100. - */ -export function ciVerdict( - status: { total: number; statuses: { context: string; state: string }[] }, - checks: { total: number; runs: { name: string; status: string; conclusion: string | null; app?: string | null }[] }, - required: string[], -): { ok: true } | { ok: false; reason: string } { - // "Some check is green" is not "CI passed": a job waiting on another job, a - // path-filtered workflow or a slow integration may not have reported yet. - // Without branch protection nothing else knows which checks matter, so the - // operator names them per repo; no names, no auto-merge. - if (required.length === 0) { - return { ok: false, reason: 'no required checks configured for this repo' }; - } - if (checks.total > checks.runs.length) { - return { ok: false, reason: `too many check runs to verify (${checks.total})` }; - } - if (status.total > status.statuses.length) { - return { ok: false, reason: `too many commit statuses to verify (${status.total})` }; - } - for (const name of required) { - // Only a GitHub Actions check run counts: a commit status or another - // app's check run can be posted under any name by anything with a token. - const passed = checks.runs.some((r) => - r.name === name && r.app === 'github-actions' && r.status === 'completed' && r.conclusion === 'success'); - if (!passed) return { ok: false, reason: `required check ${name} has not passed` }; - } - for (const s of status.statuses) { - if (s.state !== 'success') return { ok: false, reason: `status ${s.context}: ${s.state}` }; - } - for (const r of checks.runs) { - if (r.status !== 'completed') return { ok: false, reason: `check ${r.name}: ${r.status}` }; - if (!PASSING_CONCLUSIONS.has(r.conclusion ?? '')) { - return { ok: false, reason: `check ${r.name}: ${r.conclusion ?? 'no conclusion'}` }; - } - } - return { ok: true }; -} diff --git a/web/src/kernel/scheduled/pr-ready.ts b/web/src/kernel/scheduled/pr-ready.ts new file mode 100644 index 0000000..adddd96 --- /dev/null +++ b/web/src/kernel/scheduled/pr-ready.ts @@ -0,0 +1,186 @@ +// --- PR Ready Flag --- +// Labels autonomous-task PRs that are ready for a human merge. It never +// merges: without branch protection and sandboxed CI, no file category is +// safe to merge by rule -- docs steer later agents, Markdown is served on +// some sites, and new tests are code that runs in CI next to deploy +// credentials. (This replaces PR auto-merge, removed in 0.10.0.) +// +// A PR is flagged when: +// 1. it belongs to a completed task and is still open +// 2. it comes from an `auto/` branch in the same repo and targets the +// repo's default branch +// 3. GitHub reports it mergeable (no conflicts) +// 4. CI on the head commit passed: every commit status is `success`, +// every check run completed and passing, at least one GitHub Actions +// check run succeeded, and every check the operator config requires +// for the repo (integrations.github.prReady.requiredChecks) is a +// successful GitHub Actions run +// +// Closed or merged PRs are marked done on the task row so the heartbeat +// moves on to newer ones. +// +// Runs in Phase 1 (free -- GitHub API only, no LLM). + +import type { EdgeEnv } from '../dispatch.js'; +import { + addLabelsToIssue, + getCombinedStatus, + getCheckRuns, + getPullRequestStats, +} from '../../github.js'; +import { operatorConfig } from '../../operator/index.js'; + +// --- Configuration --- + +/** Label added to PRs that are ready for a human merge. */ +export const READY_LABEL = 'aegis:ready-to-merge'; + +/** Max PRs to evaluate per run (rate limiting) */ +const MAX_PER_RUN = 5; + +// --- Types --- + +interface Candidate { + taskId: string; + title: string; + prUrl: string; + prNumber: number; + ghRepo: string; +} + +type Outcome = + | { kind: 'flagged' } + | { kind: 'closed'; state: string } + | { kind: 'not_ready'; reason: string }; + +// --- Main --- + +export async function runPrReadyFlag(env: EdgeEnv): Promise { + if (!env.githubToken) return; + + // Time guard: run every 2 hours (00, 02, 04, ..., 22 UTC) + const hour = new Date().getUTCHours(); + if (hour % 2 !== 0) return; + + // Newest first, so new PRs are seen even when old ones are still open. + // '/merged' and '/closed' suffixes mark PRs already finished. + const rows = await env.db.prepare(` + SELECT id, title, pr_url + FROM cc_tasks + WHERE status = 'completed' + AND pr_url IS NOT NULL + AND pr_url NOT LIKE '%/merged' + AND pr_url NOT LIKE '%/closed' + ORDER BY completed_at DESC + LIMIT ? + `).bind(MAX_PER_RUN).all<{ id: string; title: string; pr_url: string }>(); + + for (const row of rows.results) { + const match = row.pr_url.match(/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)$/); + if (!match) continue; + const candidate: Candidate = { + taskId: row.id, + title: row.title, + prUrl: row.pr_url, + ghRepo: match[1], + prNumber: parseInt(match[2], 10), + }; + + try { + const outcome = await evaluate(env.githubToken, candidate); + const ref = `${candidate.ghRepo}#${candidate.prNumber}`; + if (outcome.kind === 'closed') { + await env.db.prepare(`UPDATE cc_tasks SET pr_url = ? WHERE id = ?`) + .bind(`${candidate.prUrl}/closed`, candidate.taskId).run(); + console.log(`[pr-ready] ${ref} is ${outcome.state}; no longer tracked`); + } else if (outcome.kind === 'flagged') { + console.log(`[pr-ready] Flagged ${ref} ready to merge -- "${candidate.title}"`); + } else { + console.log(`[pr-ready] Not ready ${ref}: ${outcome.reason}`); + } + } catch (err) { + console.warn(`[pr-ready] Error evaluating ${candidate.ghRepo}#${candidate.prNumber}:`, + err instanceof Error ? err.message : String(err)); + } + } +} + +// --- Evaluation --- + +async function evaluate(token: string, candidate: Candidate): Promise { + const stats = await getPullRequestStats(token, candidate.ghRepo, candidate.prNumber); + + if (stats.state !== 'open') return { kind: 'closed', state: stats.state }; + + // Provenance: the task's own branch in this repo, aimed at the default branch. + if (stats.headRepo !== candidate.ghRepo) { + return { kind: 'not_ready', reason: `head is from ${stats.headRepo ?? 'a deleted fork'}, not ${candidate.ghRepo}` }; + } + if (!stats.head.startsWith('auto/')) { + return { kind: 'not_ready', reason: `head branch ${stats.head} is not an auto/ task branch` }; + } + if (stats.base !== stats.baseDefaultBranch) { + return { kind: 'not_ready', reason: `targets ${stats.base}, not ${stats.baseDefaultBranch}` }; + } + + if (stats.mergeable !== true) { + return { kind: 'not_ready', reason: stats.mergeable === false ? 'merge conflicts' : 'mergeability not computed yet' }; + } + + const [status, checks] = await Promise.all([ + getCombinedStatus(token, candidate.ghRepo, stats.headSha), + getCheckRuns(token, candidate.ghRepo, stats.headSha), + ]); + const required = operatorConfig.integrations.github.prReady?.requiredChecks?.[candidate.ghRepo] ?? []; + const ci = ciVerdict(status, checks, required); + if (!ci.ok) return { kind: 'not_ready', reason: ci.reason }; + + await addLabelsToIssue(token, candidate.ghRepo, candidate.prNumber, [READY_LABEL]); + return { kind: 'flagged' }; +} + +// --- CI verdict --- + +const PASSING_CONCLUSIONS = new Set(['success', 'neutral', 'skipped']); + +/** + * Fail closed. GitHub Actions report through the Checks API, not commit + * statuses, and the combined status reads "pending" with zero statuses when + * a repo only uses Actions. Passing means: every commit status is `success`; + * every check run completed with `success`, `neutral` or `skipped`; at least + * one GitHub Actions check run concluded `success`; each required check is a + * GitHub Actions run concluding `success`; and everything fit in one page of + * 100 (more is refused rather than judged on a partial list). + */ +export function ciVerdict( + status: { total: number; statuses: { context: string; state: string }[] }, + checks: { total: number; runs: { name: string; status: string; conclusion: string | null; app?: string | null }[] }, + required: string[], +): { ok: true } | { ok: false; reason: string } { + if (checks.total > checks.runs.length) { + return { ok: false, reason: `too many check runs to verify (${checks.total})` }; + } + if (status.total > status.statuses.length) { + return { ok: false, reason: `too many commit statuses to verify (${status.total})` }; + } + const actionsPassed = (r: { status: string; conclusion: string | null; app?: string | null }) => + r.app === 'github-actions' && r.status === 'completed' && r.conclusion === 'success'; + if (!checks.runs.some(actionsPassed)) { + return { ok: false, reason: 'no successful GitHub Actions check run on head commit' }; + } + for (const name of required) { + if (!checks.runs.some((r) => r.name === name && actionsPassed(r))) { + return { ok: false, reason: `required check ${name} has not passed` }; + } + } + for (const s of status.statuses) { + if (s.state !== 'success') return { ok: false, reason: `status ${s.context}: ${s.state}` }; + } + for (const r of checks.runs) { + if (r.status !== 'completed') return { ok: false, reason: `check ${r.name}: ${r.status}` }; + if (!PASSING_CONCLUSIONS.has(r.conclusion ?? '')) { + return { ok: false, reason: `check ${r.name}: ${r.conclusion ?? 'no conclusion'}` }; + } + } + return { ok: true }; +} diff --git a/web/src/operator/config.example.ts b/web/src/operator/config.example.ts index 10d7c0c..4877cf2 100755 --- a/web/src/operator/config.example.ts +++ b/web/src/operator/config.example.ts @@ -43,8 +43,8 @@ const config: OperatorConfig = { }, github: { enabled: true, - // PR auto-merge: only repos listed here, and only when every named check passed. - // automerge: { requiredChecks: { 'ExampleOrg/my-project': ['typecheck', 'test'] } }, + // PR ready flag: also require these GitHub Actions jobs before labelling a PR ready. + // prReady: { requiredChecks: { 'ExampleOrg/my-project': ['typecheck', 'test'] } }, }, brave: { enabled: true }, email: { diff --git a/web/src/operator/types.ts b/web/src/operator/types.ts index ab6e53d..c822309 100755 --- a/web/src/operator/types.ts +++ b/web/src/operator/types.ts @@ -31,13 +31,12 @@ export interface OperatorConfig { github: { enabled: boolean; /** - * PR auto-merge for completed docs/tests/research tasks. A repo merges - * only when every check named here passed on the PR's head commit - * (plus every other status and check run that exists). A repo with no - * entry is never auto-merged. Keys are `owner/name`; names are check - * run names (GitHub Actions job names) or commit status contexts. + * The PR ready flag labels green autonomous-task PRs for a human merge + * (nothing is merged automatically). Optionally require named checks + * per repo: keys are `owner/name`, values are GitHub Actions job + * names, each of which must have succeeded on the PR's head commit. */ - automerge?: { requiredChecks: Record }; + prReady?: { requiredChecks?: Record }; }; brave: { enabled: boolean }; email: { diff --git a/web/src/schema-enums.ts b/web/src/schema-enums.ts index 634e099..7d2381a 100644 --- a/web/src/schema-enums.ts +++ b/web/src/schema-enums.ts @@ -84,9 +84,6 @@ export const AUTO_SAFE_CATEGORIES = new Set(['docs', 'tests', 'res /** Categories that require approval before execution. */ export const PROPOSED_CATEGORIES = new Set(['bugfix', 'feature']); -/** Subset safe for PR automerge. */ -export const AUTOMERGE_SAFE_CATEGORIES = new Set(['docs', 'tests', 'research']); - // ─── Narratives ───────────────────────────────────────────── export const MEMORY_GOAL_STATUSES = ['active', 'resolved', 'stalled', 'abandoned'] as const; diff --git a/web/tests/pr-automerge-ci-gate.test.ts b/web/tests/pr-automerge-ci-gate.test.ts deleted file mode 100644 index 25ebc46..0000000 --- a/web/tests/pr-automerge-ci-gate.test.ts +++ /dev/null @@ -1,234 +0,0 @@ -// PR auto-merge CI gate: fail closed. -// GitHub Actions results come from the Checks API, not commit statuses. The -// old gate read only the combined status, which is "pending" with zero -// statuses on an Actions-only repo, and treated that as "no CI configured" -// -- so a PR whose checks all failed could be auto-merged. Without branch -// protection, the operator config names the checks each repo must pass. - -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { ciVerdict, filesOutsideCategory, runPrAutomerge } from '../src/kernel/scheduled/pr-automerge.js'; -import { operatorConfig, setOperatorConfig } from '../src/operator/index.js'; - -const noStatuses = { total: 0, statuses: [] }; -const run = (name: string, status: string, conclusion: string | null, app: string | null = 'github-actions') => ({ name, status, conclusion, app }); -const REQ = ['typecheck']; - -describe('ciVerdict', () => { - it('refuses a repo with no required checks configured, even when everything is green', () => { - expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'success')] }, [])) - .toEqual({ ok: false, reason: 'no required checks configured for this repo' }); - }); - - it('refuses when the required check has not reported yet, even if another check is green', () => { - expect(ciVerdict(noStatuses, { total: 1, runs: [run('lint', 'completed', 'success')] }, REQ)) - .toEqual({ ok: false, reason: 'required check typecheck has not passed' }); - expect(ciVerdict({ total: 1, statuses: [{ context: 'deploy-preview', state: 'success' }] }, { total: 0, runs: [] }, REQ).ok).toBe(false); - }); - - it('refuses a required check that was skipped or neutral', () => { - expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'skipped')] }, REQ).ok).toBe(false); - expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'neutral')] }, REQ).ok).toBe(false); - }); - - it('refuses a failed check run even when legacy statuses are empty (the old hole)', () => { - expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'failure')] }, REQ).ok).toBe(false); - }); - - it('refuses when any other check failed or is still running', () => { - const ok = run('typecheck', 'completed', 'success'); - expect(ciVerdict(noStatuses, { total: 2, runs: [ok, run('test', 'completed', 'failure')] }, REQ).ok).toBe(false); - expect(ciVerdict(noStatuses, { total: 2, runs: [ok, run('test', 'in_progress', null)] }, REQ).ok).toBe(false); - expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'pending' }] }, { total: 1, runs: [ok] }, REQ).ok).toBe(false); - }); - - it('refuses when not every check run or status could be read (pagination)', () => { - const ok = run('typecheck', 'completed', 'success'); - expect(ciVerdict(noStatuses, { total: 150, runs: [ok] }, REQ).ok).toBe(false); - const thirty = Array.from({ length: 30 }, (_, i) => ({ context: `s${i}`, state: 'success' })); - expect(ciVerdict({ total: 31, statuses: thirty }, { total: 1, runs: [ok] }, REQ)) - .toEqual({ ok: false, reason: 'too many commit statuses to verify (31)' }); - }); - - it('only a GitHub Actions check run satisfies a required check, not a status or another app', () => { - expect(ciVerdict({ total: 1, statuses: [{ context: 'typecheck', state: 'success' }] }, { total: 0, runs: [] }, REQ).ok).toBe(false); - expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'success', 'some-other-app')] }, REQ).ok).toBe(false); - }); - - it('passes when every required check succeeded and the rest passed (skipped extras allowed)', () => { - expect(ciVerdict( - { total: 1, statuses: [{ context: 'ci', state: 'success' }] }, - { total: 3, runs: [run('typecheck', 'completed', 'success'), run('b', 'completed', 'skipped'), run('c', 'completed', 'neutral')] }, - REQ, - )).toEqual({ ok: true }); - }); -}); - -describe('filesOutsideCategory', () => { - const f = (filename: string, status = 'modified', previousFilename?: string) => ({ filename, status, ...(previousFilename ? { previousFilename } : {}) }); - - it('docs may touch Markdown and docs/ assets only', () => { - expect(filesOutsideCategory('docs', [f('README.md'), f('docs/guide.md'), f('docs/img/a.png', 'added')])).toBeNull(); - expect(filesOutsideCategory('docs', [f('docs/build.config.js')])).toMatch(/outside what docs tasks may change/); - expect(filesOutsideCategory('docs', [f('src/index.ts')])).not.toBeNull(); - }); - - it('no category may touch .github/ or dotfiles, even as Markdown', () => { - expect(filesOutsideCategory('docs', [f('.github/workflows/ci.yml')])).not.toBeNull(); - expect(filesOutsideCategory('docs', [f('.github/PULL_REQUEST_TEMPLATE.md')])).not.toBeNull(); - expect(filesOutsideCategory('research', [f('.ai/notes.md')])).not.toBeNull(); - }); - - it('tests may only add new test files, never edit or delete existing ones or config', () => { - expect(filesOutsideCategory('tests', [f('tests/new.test.ts', 'added'), f('src/__tests__/x.spec.ts', 'added')])).toBeNull(); - expect(filesOutsideCategory('tests', [f('tests/gate.test.ts', 'modified')])).not.toBeNull(); - expect(filesOutsideCategory('tests', [f('tests/gate.test.ts', 'removed')])).not.toBeNull(); - expect(filesOutsideCategory('tests', [f('vitest.config.ts', 'modified')])).not.toBeNull(); - expect(filesOutsideCategory('tests', [f('tests/helpers.ts', 'added')])).not.toBeNull(); - }); - - it('a rename is judged by both paths', () => { - expect(filesOutsideCategory('docs', [f('docs/notes.md', 'renamed', 'wrangler.toml')])).toMatch(/renamed from wrangler\.toml/); - }); - - it('research may touch research/ and Markdown', () => { - expect(filesOutsideCategory('research', [f('research/2026-10-16-x.md', 'added'), f('research/data.json', 'added')])).toBeNull(); - expect(filesOutsideCategory('research', [f('src/x.ts')])).not.toBeNull(); - }); -}); - -describe('runPrAutomerge end to end (mocked GitHub)', () => { - const HEAD_SHA = 'abc123def456'; - const original = operatorConfig; - let merges: Array>; - let checkRuns: unknown; - let mergeable: boolean | null; - let logs: string[]; - let files: unknown[]; - let changedFiles: number; - - const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); - - function env() { - const updates: unknown[][] = []; - const db = { - prepare: (sql: string) => ({ - bind: (...args: unknown[]) => ({ - all: async () => ({ - results: sql.includes('FROM cc_tasks') - ? [{ id: 'task-1234abcd', title: 'docs: fix typo', repo: 'demo', category: 'docs', pr_url: 'https://github.com/ExampleOrg/demo/pull/7' }] - : [], - }), - run: async () => { updates.push(args); return { success: true }; }, - }), - }), - }; - return { env: { githubToken: 'test-token', db } as never, updates }; - } - - function requireChecks(map: Record) { - setOperatorConfig({ - ...original, - integrations: { ...original.integrations, github: { ...original.integrations.github, automerge: { requiredChecks: map } } }, - } as never); - } - - beforeEach(() => { - vi.useFakeTimers(); - vi.setSystemTime(new Date('2026-10-16T22:00:00Z')); // even hour: the task runs - merges = []; - logs = []; - mergeable = true; - files = [{ filename: 'docs/readme.md', status: 'modified' }]; - changedFiles = 1; - checkRuns = { total_count: 1, check_runs: [run('typecheck', 'completed', 'success')] }; - requireChecks({ 'ExampleOrg/demo': ['typecheck'] }); - vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { logs.push(a.join(' ')); }); - vi.spyOn(console, 'warn').mockImplementation((...a: unknown[]) => { logs.push('WARN ' + a.join(' ')); }); - vi.stubGlobal('fetch', vi.fn(async (url: string, init?: RequestInit) => { - const u = String(url); - if (u.endsWith('/pulls/7')) { - return json({ number: 7, state: 'open', mergeable, additions: 3, deletions: 1, changed_files: changedFiles, head: { ref: 'auto/task1234', sha: HEAD_SHA }, base: { ref: 'main' } }); - } - if (u.includes('/pulls/7/files')) return json(files); - if (u.includes(`/commits/${HEAD_SHA}/status?per_page=100`)) return json({ state: 'pending', total_count: 0, statuses: [] }); - if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) { - // The API reports the producing app as { slug }. - const cr = checkRuns as { total_count: number; check_runs: Array<{ app: string | null }> }; - return json({ ...cr, check_runs: cr.check_runs.map((r) => ({ ...r, app: r.app ? { slug: r.app } : null })) }); - } - if (u.endsWith('/pulls/7/merge') && init?.method === 'PUT') { - merges.push(JSON.parse(String(init.body))); - return json({ sha: 'merged-sha', merged: true, message: 'Pull Request successfully merged' }); - } - return new Response('not mocked: ' + u, { status: 404 }); - })); - }); - - afterEach(() => { - setOperatorConfig(original as never); - vi.useRealTimers(); - vi.restoreAllMocks(); - vi.unstubAllGlobals(); - }); - - const skipped = (reason: string) => logs.some((l) => l.includes('Skipped ExampleOrg/demo#7') && l.includes(reason)); - - it('does not merge an Actions-only PR whose required check failed', async () => { - checkRuns = { total_count: 1, check_runs: [run('typecheck', 'completed', 'failure')] }; - const { env: e, updates } = env(); - await runPrAutomerge(e); - expect(merges).toEqual([]); - expect(updates).toEqual([]); - expect(skipped('required check typecheck has not passed')).toBe(true); - }); - - it('does not merge while only some other check has reported', async () => { - checkRuns = { total_count: 1, check_runs: [run('lint', 'completed', 'success')] }; - const { env: e } = env(); - await runPrAutomerge(e); - expect(merges).toEqual([]); - expect(skipped('required check typecheck has not passed')).toBe(true); - }); - - it('does not merge a repo with no required checks configured', async () => { - requireChecks({}); - const { env: e } = env(); - await runPrAutomerge(e); - expect(merges).toEqual([]); - expect(skipped('no required checks configured')).toBe(true); - }); - - it('does not merge while GitHub has not computed mergeability', async () => { - mergeable = null; - const { env: e } = env(); - await runPrAutomerge(e); - expect(merges).toEqual([]); - expect(skipped('mergeability not computed yet')).toBe(true); - }); - - it('does not merge a docs PR that edits a workflow, even with a green required check', async () => { - files = [{ filename: 'docs/readme.md', status: 'modified' }, { filename: '.github/workflows/ci.yml', status: 'modified' }]; - changedFiles = 2; - const { env: e } = env(); - await runPrAutomerge(e); - expect(merges).toEqual([]); - expect(skipped('touches .github/workflows/ci.yml')).toBe(true); - }); - - it('does not merge when not every changed file could be read', async () => { - changedFiles = 101; - const { env: e } = env(); - await runPrAutomerge(e); - expect(merges).toEqual([]); - expect(skipped('too many changed files to verify (101)')).toBe(true); - }); - - it('merges a green PR, pinned to the head commit that was checked', async () => { - const { env: e, updates } = env(); - await runPrAutomerge(e); - expect(merges).toHaveLength(1); - expect(merges[0]).toMatchObject({ merge_method: 'squash', sha: HEAD_SHA }); - expect(updates).toEqual([['https://github.com/ExampleOrg/demo/pull/7/merged', 'task-1234abcd']]); - expect(logs.some((l) => l.startsWith('WARN'))).toBe(false); - }); -}); diff --git a/web/tests/pr-ready.test.ts b/web/tests/pr-ready.test.ts new file mode 100644 index 0000000..dcac715 --- /dev/null +++ b/web/tests/pr-ready.test.ts @@ -0,0 +1,161 @@ +// PR ready flag: label green autonomous-task PRs for a human merge; never merge. +// Replaces PR auto-merge (0.10.0). The old gate read only the legacy +// combined-status API -- "pending" with zero statuses on an Actions-only repo +// -- and treated that as "no CI", so failed PRs could be merged. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { ciVerdict, runPrReadyFlag, READY_LABEL } from '../src/kernel/scheduled/pr-ready.js'; +import { operatorConfig, setOperatorConfig } from '../src/operator/index.js'; + +const noStatuses = { total: 0, statuses: [] }; +const run = (name: string, status: string, conclusion: string | null, app: string | null = 'github-actions') => ({ name, status, conclusion, app }); +const green = run('typecheck', 'completed', 'success'); + +describe('ciVerdict', () => { + it('needs at least one successful GitHub Actions check run (the old hole: no CI passed)', () => { + expect(ciVerdict(noStatuses, { total: 0, runs: [] }, [])).toEqual({ ok: false, reason: 'no successful GitHub Actions check run on head commit' }); + expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'success' }] }, { total: 0, runs: [] }, []).ok).toBe(false); + expect(ciVerdict(noStatuses, { total: 1, runs: [run('x', 'completed', 'success', 'other-app')] }, []).ok).toBe(false); + expect(ciVerdict(noStatuses, { total: 1, runs: [run('x', 'completed', 'skipped')] }, []).ok).toBe(false); + }); + + it('refuses a failed or unfinished check, and a non-success status', () => { + expect(ciVerdict(noStatuses, { total: 1, runs: [run('typecheck', 'completed', 'failure')] }, []).ok).toBe(false); + expect(ciVerdict(noStatuses, { total: 2, runs: [green, run('test', 'in_progress', null)] }, []).ok).toBe(false); + expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'pending' }] }, { total: 1, runs: [green] }, []).ok).toBe(false); + }); + + it('requires each configured check as a successful GitHub Actions run', () => { + expect(ciVerdict(noStatuses, { total: 1, runs: [green] }, ['test'])).toEqual({ ok: false, reason: 'required check test has not passed' }); + expect(ciVerdict(noStatuses, { total: 2, runs: [green, run('test', 'completed', 'skipped')] }, ['test']).ok).toBe(false); + expect(ciVerdict({ total: 1, statuses: [{ context: 'test', state: 'success' }] }, { total: 1, runs: [green] }, ['test']).ok).toBe(false); + }); + + it('refuses to judge a partial list (pagination)', () => { + expect(ciVerdict(noStatuses, { total: 150, runs: [green] }, []).ok).toBe(false); + const thirty = Array.from({ length: 30 }, (_, i) => ({ context: `s${i}`, state: 'success' })); + expect(ciVerdict({ total: 31, statuses: thirty }, { total: 1, runs: [green] }, [])).toEqual({ ok: false, reason: 'too many commit statuses to verify (31)' }); + }); + + it('passes when everything passed (extra skipped/neutral runs allowed)', () => { + expect(ciVerdict({ total: 1, statuses: [{ context: 'ci', state: 'success' }] }, + { total: 3, runs: [green, run('b', 'completed', 'skipped'), run('c', 'completed', 'neutral')] }, [])).toEqual({ ok: true }); + }); +}); + +describe('runPrReadyFlag (mocked GitHub)', () => { + const HEAD_SHA = 'abc123def456'; + const original = operatorConfig; + let pr: Record; + let checkRuns: { total_count: number; check_runs: ReturnType[] }; + let calls: { url: string; method: string; body?: unknown }[]; + let logs: string[]; + let sqls: string[]; + + const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); + + function env() { + const updates: unknown[][] = []; + const db = { + prepare: (sql: string) => { + sqls.push(sql); + return { + bind: (...args: unknown[]) => ({ + all: async () => ({ results: [{ id: 'task-1234abcd', title: 'docs: fix typo', pr_url: 'https://github.com/ExampleOrg/demo/pull/7' }] }), + run: async () => { updates.push(args); return { success: true }; }, + }), + }; + }, + }; + return { env: { githubToken: 'test-token', db } as never, updates }; + } + + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-10-16T22:00:00Z')); + calls = []; logs = []; sqls = []; + pr = { + number: 7, state: 'open', mergeable: true, additions: 3, deletions: 1, changed_files: 1, + head: { ref: 'auto/task1234', sha: HEAD_SHA, repo: { full_name: 'ExampleOrg/demo' } }, + base: { ref: 'main', repo: { default_branch: 'main' } }, + }; + checkRuns = { total_count: 1, check_runs: [green] }; + vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { logs.push(a.join(' ')); }); + vi.spyOn(console, 'warn').mockImplementation((...a: unknown[]) => { logs.push('WARN ' + a.join(' ')); }); + vi.stubGlobal('fetch', vi.fn(async (url: string, init?: RequestInit) => { + const u = String(url); + const method = init?.method ?? 'GET'; + calls.push({ url: u, method, body: init?.body ? JSON.parse(String(init.body)) : undefined }); + if (u.endsWith('/pulls/7') && method === 'GET') return json(pr); + if (u.includes(`/commits/${HEAD_SHA}/status?per_page=100`)) return json({ state: 'pending', total_count: 0, statuses: [] }); + if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) { + return json({ ...checkRuns, check_runs: checkRuns.check_runs.map((r) => ({ ...r, app: r.app ? { slug: r.app } : null })) }); + } + if (u.endsWith('/issues/7/labels') && method === 'POST') return json([{ name: READY_LABEL }]); + return new Response('not mocked: ' + u, { status: 404 }); + })); + }); + + afterEach(() => { + setOperatorConfig(original as never); + vi.useRealTimers(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + }); + + const labelled = () => calls.some((c) => c.url.endsWith('/issues/7/labels') && c.method === 'POST'); + const merged = () => calls.some((c) => c.url.includes('/merge')); + const notReady = (reason: string) => logs.some((l) => l.includes('Not ready ExampleOrg/demo#7') && l.includes(reason)); + + it('labels a green PR from the task branch, and never merges', async () => { + const { env: e, updates } = env(); + await runPrReadyFlag(e); + expect(calls.find((c) => c.url.endsWith('/issues/7/labels'))?.body).toEqual({ labels: [READY_LABEL] }); + expect(merged()).toBe(false); + expect(updates).toEqual([]); + }); + + it('reads newest tasks first and skips PRs already finished', async () => { + const { env: e } = env(); + await runPrReadyFlag(e); + expect(sqls[0]).toContain('ORDER BY completed_at DESC'); + expect(sqls[0]).toContain("NOT LIKE '%/closed'"); + }); + + it('marks a closed or merged PR done so it stops being re-read', async () => { + pr.state = 'closed'; + const { env: e, updates } = env(); + await runPrReadyFlag(e); + expect(updates).toEqual([['https://github.com/ExampleOrg/demo/pull/7/closed', 'task-1234abcd']]); + expect(labelled()).toBe(false); + }); + + it.each([ + ['a fork', () => { (pr.head as any).repo = { full_name: 'Outsider/demo' }; }, 'head is from Outsider/demo'], + ['a deleted fork', () => { (pr.head as any).repo = null; }, 'head is from a deleted fork'], + ['a non-task branch', () => { (pr.head as any).ref = 'feature/x'; }, 'is not an auto/ task branch'], + ['a non-default base', () => { (pr.base as any).ref = 'release'; }, 'targets release, not main'], + ['unknown mergeability', () => { pr.mergeable = null; }, 'mergeability not computed yet'], + ['conflicts', () => { pr.mergeable = false; }, 'merge conflicts'], + ['a failed check next to a green one', () => { checkRuns = { total_count: 2, check_runs: [green, run('test', 'completed', 'failure')] }; }, 'check test: failure'], + ['no CI', () => { checkRuns = { total_count: 0, check_runs: [] }; }, 'no successful GitHub Actions check run'], + ])('does not flag %s', async (_name, setup, reason) => { + setup(); + const { env: e } = env(); + await runPrReadyFlag(e); + expect(labelled()).toBe(false); + expect(merged()).toBe(false); + expect(notReady(reason)).toBe(true); + }); + + it('honours required checks from the operator config', async () => { + setOperatorConfig({ + ...original, + integrations: { ...original.integrations, github: { ...original.integrations.github, prReady: { requiredChecks: { 'ExampleOrg/demo': ['test'] } } } }, + } as never); + const { env: e } = env(); + await runPrReadyFlag(e); + expect(labelled()).toBe(false); + expect(notReady('required check test has not passed')).toBe(true); + }); +}); From 523b979e304b8d3a417727f07c587cc3aa4fe77b Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 18:04:45 -0500 Subject: [PATCH 7/8] fix(pr-ready): withdraw stale labels; refuse PRs that edit .github/; re-evaluate 25 newest Review of #89: - A flagged PR that stops qualifying (a new commit fails CI) loses the label; an already-labelled PR isn't re-labelled. - A PR touching .github/ (case-insensitive, incl. a rename's old path) is never flagged: editing its own workflows decides which checks run. Refuse when not every changed file could be read. - Re-evaluate up to 25 tracked PRs per run; rows whose pr_url isn't a PR are marked done. - Drop the unused sha parameter on mergePullRequest. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +- web/src/github.ts | 42 +++++++++++++++-- web/src/kernel/scheduled/pr-ready.ts | 70 +++++++++++++++++++++------- web/tests/pr-ready.test.ts | 39 +++++++++++++++- 4 files changed, 134 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e20a11..e02da27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,10 @@ - **PR ready flag** (`pr-ready` heartbeat, every 2 h). It labels a completed task's PR `aegis:ready-to-merge` when: - it is open, comes from an `auto/` branch in the same repo, and targets the repo's default branch; - GitHub reports it mergeable; + - it doesn't touch `.github/` (checked case-insensitively, including the old path of a rename), since a PR that edits its own workflows decides which checks run on it; - on the head commit, every commit status is `success`, every check run completed and passing, and at least one GitHub Actions check run succeeded. - Optional per-repo required checks (`integrations.github.prReady.requiredChecks`: `owner/name` → GitHub Actions job names) must each be successful Actions runs. Statuses and check runs are read up to 100 each; more is refused. Tasks are read newest first, and closed or merged PRs are marked done on the task row (`/closed`), so old PRs no longer starve new ones. -- `getCheckRuns()`, `PullRequestStats.headSha`, `headRepo` and `baseDefaultBranch`; `mergePullRequest()` takes an optional `sha` to pin the head; `getCombinedStatus()` reads 100 statuses. + Optional per-repo required checks (`integrations.github.prReady.requiredChecks`: `owner/name` → GitHub Actions job names) must each be successful Actions runs. Statuses and check runs are read up to 100 each; more is refused. A labelled PR that stops qualifying (for example a new commit fails CI) loses the label. Up to 25 tracked PRs are re-evaluated per run, newest first; closed or merged PRs, and task rows whose `pr_url` isn't a GitHub PR, are marked done (`/closed`) and no longer read. +- `getCheckRuns()`, `getPullRequestFileDetails()`, `removeLabelFromIssue()`; `PullRequestStats.headSha`, `headRepo`, `baseDefaultBranch` and `labels`; `getCombinedStatus()` reads 100 statuses. ## 0.9.4 (2026-09-24) diff --git a/web/src/github.ts b/web/src/github.ts index c2507fa..076c1c5 100755 --- a/web/src/github.ts +++ b/web/src/github.ts @@ -644,6 +644,7 @@ export interface PullRequestStats { base: string; /** The base repo's default branch. */ baseDefaultBranch: string; + labels: string[]; } export async function getPullRequestStats( @@ -661,6 +662,7 @@ export async function getPullRequestStats( changed_files: number; head: { ref: string; sha: string; repo: { full_name: string } | null }; base: { ref: string; repo: { default_branch: string } }; + labels?: { name: string }[]; }; return { number: data.number, @@ -674,6 +676,7 @@ export async function getPullRequestStats( headRepo: data.head.repo?.full_name ?? null, base: data.base.ref, baseDefaultBranch: data.base.repo.default_branch, + labels: (data.labels ?? []).map((l) => l.name), }; } @@ -689,6 +692,42 @@ export async function getPullRequestFiles( return data.map(f => f.filename); } +export interface PullRequestFile { + filename: string; + status: string; + previousFilename?: string; +} + +/** Changed files with status and, for renames, the old path. Reads up to 100. */ +export async function getPullRequestFileDetails( + token: string, + repo: string, + pullNumber: number, +): Promise { + const res = await ghFetch(token, `/repos/${repo}/pulls/${pullNumber}/files?per_page=100`); + const data = (await res.json()) as { filename: string; status: string; previous_filename?: string }[]; + return data.map((f) => ({ + filename: f.filename, + status: f.status, + ...(f.previous_filename ? { previousFilename: f.previous_filename } : {}), + })); +} + +/** Remove one label from an issue or PR; a label that isn't there is not an error. */ +export async function removeLabelFromIssue( + token: string, + repo: string, + issueNumber: number, + label: string, +): Promise { + try { + await ghFetch(token, `/repos/${repo}/issues/${issueNumber}/labels/${encodeURIComponent(label)}`, { method: 'DELETE' }); + } catch (err) { + if (err instanceof GitHubApiError && err.status === 404) return; + throw err; + } +} + // ─── PR Merge ──────────────────────────────────────────────── export async function mergePullRequest( @@ -697,12 +736,9 @@ export async function mergePullRequest( pullNumber: number, method: 'merge' | 'squash' | 'rebase' = 'squash', commitMessage?: string, - /** Merge only if the PR head is still this commit (GitHub answers 409 otherwise). */ - sha?: string, ): Promise<{ sha: string; merged: boolean; message: string }> { const body: Record = { merge_method: method }; if (commitMessage) body.commit_message = commitMessage; - if (sha) body.sha = sha; const res = await ghFetch(token, `/repos/${repo}/pulls/${pullNumber}/merge`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, diff --git a/web/src/kernel/scheduled/pr-ready.ts b/web/src/kernel/scheduled/pr-ready.ts index adddd96..bb560d6 100644 --- a/web/src/kernel/scheduled/pr-ready.ts +++ b/web/src/kernel/scheduled/pr-ready.ts @@ -10,23 +10,28 @@ // 2. it comes from an `auto/` branch in the same repo and targets the // repo's default branch // 3. GitHub reports it mergeable (no conflicts) -// 4. CI on the head commit passed: every commit status is `success`, +// 4. it doesn't touch .github/ (a PR that edits its own workflows decides +// which checks run and pass on it) +// 5. CI on the head commit passed: every commit status is `success`, // every check run completed and passing, at least one GitHub Actions // check run succeeded, and every check the operator config requires // for the repo (integrations.github.prReady.requiredChecks) is a // successful GitHub Actions run // -// Closed or merged PRs are marked done on the task row so the heartbeat -// moves on to newer ones. +// A flagged PR that stops qualifying (say, a new commit fails CI) loses the +// label. Closed or merged PRs are marked done on the task row so the +// heartbeat stops reading them. // // Runs in Phase 1 (free -- GitHub API only, no LLM). import type { EdgeEnv } from '../dispatch.js'; import { addLabelsToIssue, + removeLabelFromIssue, getCombinedStatus, getCheckRuns, getPullRequestStats, + getPullRequestFileDetails, } from '../../github.js'; import { operatorConfig } from '../../operator/index.js'; @@ -35,8 +40,12 @@ import { operatorConfig } from '../../operator/index.js'; /** Label added to PRs that are ready for a human merge. */ export const READY_LABEL = 'aegis:ready-to-merge'; -/** Max PRs to evaluate per run (rate limiting) */ -const MAX_PER_RUN = 5; +/** + * Open task PRs evaluated per run, newest first. Every tracked PR is + * re-evaluated each run (so a label can be withdrawn), which takes a few + * GitHub calls apiece; old PRs that never close drop out of the window. + */ +const MAX_PER_RUN = 25; // --- Types --- @@ -49,7 +58,7 @@ interface Candidate { } type Outcome = - | { kind: 'flagged' } + | { kind: 'flagged'; already: boolean } | { kind: 'closed'; state: string } | { kind: 'not_ready'; reason: string }; @@ -77,7 +86,11 @@ export async function runPrReadyFlag(env: EdgeEnv): Promise { for (const row of rows.results) { const match = row.pr_url.match(/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)$/); - if (!match) continue; + if (!match) { + // Not a PR URL this can judge; stop re-reading it. + await env.db.prepare(`UPDATE cc_tasks SET pr_url = ? WHERE id = ?`).bind(`${row.pr_url}/closed`, row.id).run(); + continue; + } const candidate: Candidate = { taskId: row.id, title: row.title, @@ -94,7 +107,7 @@ export async function runPrReadyFlag(env: EdgeEnv): Promise { .bind(`${candidate.prUrl}/closed`, candidate.taskId).run(); console.log(`[pr-ready] ${ref} is ${outcome.state}; no longer tracked`); } else if (outcome.kind === 'flagged') { - console.log(`[pr-ready] Flagged ${ref} ready to merge -- "${candidate.title}"`); + if (!outcome.already) console.log(`[pr-ready] Flagged ${ref} ready to merge -- "${candidate.title}"`); } else { console.log(`[pr-ready] Not ready ${ref}: ${outcome.reason}`); } @@ -112,19 +125,46 @@ async function evaluate(token: string, candidate: Candidate): Promise { if (stats.state !== 'open') return { kind: 'closed', state: stats.state }; + const labelled = stats.labels.includes(READY_LABEL); + const verdict = await judge(token, candidate, stats); + if (verdict) { + if (labelled) await removeLabelFromIssue(token, candidate.ghRepo, candidate.prNumber, READY_LABEL); + return { kind: 'not_ready', reason: labelled ? `${verdict} (label removed)` : verdict }; + } + if (!labelled) await addLabelsToIssue(token, candidate.ghRepo, candidate.prNumber, [READY_LABEL]); + return { kind: 'flagged', already: labelled }; +} + +/** Why the PR isn't ready, or null when it is. */ +async function judge( + token: string, + candidate: Candidate, + stats: Awaited>, +): Promise { + const notReady = (reason: string): string => reason; + // Provenance: the task's own branch in this repo, aimed at the default branch. if (stats.headRepo !== candidate.ghRepo) { - return { kind: 'not_ready', reason: `head is from ${stats.headRepo ?? 'a deleted fork'}, not ${candidate.ghRepo}` }; + return notReady(`head is from ${stats.headRepo ?? 'a deleted fork'}, not ${candidate.ghRepo}`); } if (!stats.head.startsWith('auto/')) { - return { kind: 'not_ready', reason: `head branch ${stats.head} is not an auto/ task branch` }; + return notReady(`head branch ${stats.head} is not an auto/ task branch`); } if (stats.base !== stats.baseDefaultBranch) { - return { kind: 'not_ready', reason: `targets ${stats.base}, not ${stats.baseDefaultBranch}` }; + return notReady(`targets ${stats.base}, not ${stats.baseDefaultBranch}`); } if (stats.mergeable !== true) { - return { kind: 'not_ready', reason: stats.mergeable === false ? 'merge conflicts' : 'mergeability not computed yet' }; + return notReady(stats.mergeable === false ? 'merge conflicts' : 'mergeability not computed yet'); + } + + const files = await getPullRequestFileDetails(token, candidate.ghRepo, candidate.prNumber); + if (stats.changed_files > files.length) { + return notReady(`too many changed files to verify (${stats.changed_files})`); + } + const workflowEdit = files.find((f) => [f.filename, f.previousFilename ?? ''].some((path) => path.toLowerCase().startsWith('.github/'))); + if (workflowEdit) { + return notReady(`touches ${workflowEdit.previousFilename?.toLowerCase().startsWith('.github/') ? workflowEdit.previousFilename : workflowEdit.filename}; a PR that edits .github/ decides its own checks`); } const [status, checks] = await Promise.all([ @@ -133,10 +173,8 @@ async function evaluate(token: string, candidate: Candidate): Promise { ]); const required = operatorConfig.integrations.github.prReady?.requiredChecks?.[candidate.ghRepo] ?? []; const ci = ciVerdict(status, checks, required); - if (!ci.ok) return { kind: 'not_ready', reason: ci.reason }; - - await addLabelsToIssue(token, candidate.ghRepo, candidate.prNumber, [READY_LABEL]); - return { kind: 'flagged' }; + if (!ci.ok) return notReady(ci.reason); + return null; } // --- CI verdict --- diff --git a/web/tests/pr-ready.test.ts b/web/tests/pr-ready.test.ts index dcac715..31f9eb3 100644 --- a/web/tests/pr-ready.test.ts +++ b/web/tests/pr-ready.test.ts @@ -51,6 +51,8 @@ describe('runPrReadyFlag (mocked GitHub)', () => { let calls: { url: string; method: string; body?: unknown }[]; let logs: string[]; let sqls: string[]; + let files: unknown[]; + let rows: Record[]; const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); @@ -61,7 +63,7 @@ describe('runPrReadyFlag (mocked GitHub)', () => { sqls.push(sql); return { bind: (...args: unknown[]) => ({ - all: async () => ({ results: [{ id: 'task-1234abcd', title: 'docs: fix typo', pr_url: 'https://github.com/ExampleOrg/demo/pull/7' }] }), + all: async () => ({ results: rows }), run: async () => { updates.push(args); return { success: true }; }, }), }; @@ -74,10 +76,13 @@ describe('runPrReadyFlag (mocked GitHub)', () => { vi.useFakeTimers(); vi.setSystemTime(new Date('2026-10-16T22:00:00Z')); calls = []; logs = []; sqls = []; + files = [{ filename: 'docs/readme.md', status: 'modified' }]; + rows = [{ id: 'task-1234abcd', title: 'docs: fix typo', pr_url: 'https://github.com/ExampleOrg/demo/pull/7' }]; pr = { number: 7, state: 'open', mergeable: true, additions: 3, deletions: 1, changed_files: 1, head: { ref: 'auto/task1234', sha: HEAD_SHA, repo: { full_name: 'ExampleOrg/demo' } }, base: { ref: 'main', repo: { default_branch: 'main' } }, + labels: [], }; checkRuns = { total_count: 1, check_runs: [green] }; vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { logs.push(a.join(' ')); }); @@ -91,7 +96,9 @@ describe('runPrReadyFlag (mocked GitHub)', () => { if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) { return json({ ...checkRuns, check_runs: checkRuns.check_runs.map((r) => ({ ...r, app: r.app ? { slug: r.app } : null })) }); } + if (u.includes('/pulls/7/files')) return json(files); if (u.endsWith('/issues/7/labels') && method === 'POST') return json([{ name: READY_LABEL }]); + if (u.endsWith(`/issues/7/labels/${encodeURIComponent(READY_LABEL)}`) && method === 'DELETE') return json([]); return new Response('not mocked: ' + u, { status: 404 }); })); }); @@ -104,6 +111,7 @@ describe('runPrReadyFlag (mocked GitHub)', () => { }); const labelled = () => calls.some((c) => c.url.endsWith('/issues/7/labels') && c.method === 'POST'); + const unlabelled = () => calls.some((c) => c.url.includes('/issues/7/labels/') && c.method === 'DELETE'); const merged = () => calls.some((c) => c.url.includes('/merge')); const notReady = (reason: string) => logs.some((l) => l.includes('Not ready ExampleOrg/demo#7') && l.includes(reason)); @@ -139,6 +147,10 @@ describe('runPrReadyFlag (mocked GitHub)', () => { ['conflicts', () => { pr.mergeable = false; }, 'merge conflicts'], ['a failed check next to a green one', () => { checkRuns = { total_count: 2, check_runs: [green, run('test', 'completed', 'failure')] }; }, 'check test: failure'], ['no CI', () => { checkRuns = { total_count: 0, check_runs: [] }; }, 'no successful GitHub Actions check run'], + ['a workflow edit', () => { files = [{ filename: '.github/workflows/ci.yml', status: 'modified' }]; }, 'touches .github/workflows/ci.yml'], + ['a workflow renamed away', () => { files = [{ filename: 'old-ci.yml', status: 'renamed', previousFilename: undefined, previous_filename: '.github/workflows/ci.yml' }]; }, 'touches .github/workflows/ci.yml'], + ['a workflow edit in a different case', () => { files = [{ filename: '.GitHub/workflows/ci.yml', status: 'added' }]; }, 'touches .GitHub/workflows/ci.yml'], + ['too many files to read', () => { pr.changed_files = 101; }, 'too many changed files to verify (101)'], ])('does not flag %s', async (_name, setup, reason) => { setup(); const { env: e } = env(); @@ -148,6 +160,31 @@ describe('runPrReadyFlag (mocked GitHub)', () => { expect(notReady(reason)).toBe(true); }); + it('withdraws the label when a flagged PR stops qualifying', async () => { + pr.labels = [{ name: READY_LABEL }]; + checkRuns = { total_count: 2, check_runs: [green, run('test', 'completed', 'failure')] }; + const { env: e } = env(); + await runPrReadyFlag(e); + expect(unlabelled()).toBe(true); + expect(labelled()).toBe(false); + expect(notReady('(label removed)')).toBe(true); + }); + + it('does not re-add the label to a PR that already has it', async () => { + pr.labels = [{ name: READY_LABEL }]; + const { env: e } = env(); + await runPrReadyFlag(e); + expect(labelled()).toBe(false); + expect(unlabelled()).toBe(false); + }); + + it('stops tracking a task whose pr_url is not a PR it can judge', async () => { + rows = [{ id: 'task-x', title: 't', pr_url: 'https://example.com/not-a-pr' }]; + const { env: e, updates } = env(); + await runPrReadyFlag(e); + expect(updates).toEqual([['https://example.com/not-a-pr/closed', 'task-x']]); + }); + it('honours required checks from the operator config', async () => { setOperatorConfig({ ...original, From f33314033f7365fe6780e910bfc725b66624df04 Mon Sep 17 00:00:00 2001 From: Kurt Overmier Date: Fri, 16 Oct 2026 18:10:31 -0500 Subject: [PATCH 8/8] fix(pr-ready): drop the label when a PR can't be re-checked or its head moved mid-check Round-six review of #89: withdrawing a label failed open (an error while re-checking left it on), and a push between the CI check and labelling left the label on an unchecked head. Remove the label on any evaluation error (best effort), and re-read the head after labelling. Docs now say what the label means: CI reported green at the last check, can lag a push by one run, and is reported by code the PR can change. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- web/src/kernel/scheduled/pr-ready.ts | 21 ++++++++++++++++--- web/tests/pr-ready.test.ts | 30 ++++++++++++++++++++++++++-- 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e02da27..e3a83b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ - GitHub reports it mergeable; - it doesn't touch `.github/` (checked case-insensitively, including the old path of a rename), since a PR that edits its own workflows decides which checks run on it; - on the head commit, every commit status is `success`, every check run completed and passing, and at least one GitHub Actions check run succeeded. - Optional per-repo required checks (`integrations.github.prReady.requiredChecks`: `owner/name` → GitHub Actions job names) must each be successful Actions runs. Statuses and check runs are read up to 100 each; more is refused. A labelled PR that stops qualifying (for example a new commit fails CI) loses the label. Up to 25 tracked PRs are re-evaluated per run, newest first; closed or merged PRs, and task rows whose `pr_url` isn't a GitHub PR, are marked done (`/closed`) and no longer read. + Optional per-repo required checks (`integrations.github.prReady.requiredChecks`: `owner/name` → GitHub Actions job names) must each be successful Actions runs. Statuses and check runs are read up to 100 each; more is refused. A labelled PR that stops qualifying (for example a new commit fails CI), or that can't be re-checked, loses the label, and a label applied while the head moved is taken back off. The label means "CI reported green on the head commit at the last check": it can lag a push by up to one run (2 h), covers the newest tracked PRs, and CI is reported by code the PR can change (package scripts, test config), so re-check CI when merging. It applies to any completed task's PR, not only docs/tests/research. Up to 25 tracked PRs are re-evaluated per run, newest first; closed or merged PRs, and task rows whose `pr_url` isn't a GitHub PR, are marked done (`/closed`) and no longer read. - `getCheckRuns()`, `getPullRequestFileDetails()`, `removeLabelFromIssue()`; `PullRequestStats.headSha`, `headRepo`, `baseDefaultBranch` and `labels`; `getCombinedStatus()` reads 100 statuses. ## 0.9.4 (2026-09-24) diff --git a/web/src/kernel/scheduled/pr-ready.ts b/web/src/kernel/scheduled/pr-ready.ts index bb560d6..c9e0aa7 100644 --- a/web/src/kernel/scheduled/pr-ready.ts +++ b/web/src/kernel/scheduled/pr-ready.ts @@ -18,8 +18,11 @@ // for the repo (integrations.github.prReady.requiredChecks) is a // successful GitHub Actions run // -// A flagged PR that stops qualifying (say, a new commit fails CI) loses the -// label. Closed or merged PRs are marked done on the task row so the +// The label means "CI reported green on the head commit at the last check": +// it can lag a new push by up to one run (2 h), covers only the newest +// tracked PRs, and CI is reported by code the PR itself can change -- so +// re-check CI when merging. A flagged PR that stops qualifying (say, a new +// commit fails CI), or can't be re-checked, loses the label. Closed or merged PRs are marked done on the task row so the // heartbeat stops reading them. // // Runs in Phase 1 (free -- GitHub API only, no LLM). @@ -114,6 +117,9 @@ export async function runPrReadyFlag(env: EdgeEnv): Promise { } catch (err) { console.warn(`[pr-ready] Error evaluating ${candidate.ghRepo}#${candidate.prNumber}:`, err instanceof Error ? err.message : String(err)); + // Couldn't confirm the PR is still ready: withdraw any label (best effort) + // rather than leave a possibly stale one. + await removeLabelFromIssue(env.githubToken, candidate.ghRepo, candidate.prNumber, READY_LABEL).catch(() => {}); } } } @@ -131,7 +137,16 @@ async function evaluate(token: string, candidate: Candidate): Promise { if (labelled) await removeLabelFromIssue(token, candidate.ghRepo, candidate.prNumber, READY_LABEL); return { kind: 'not_ready', reason: labelled ? `${verdict} (label removed)` : verdict }; } - if (!labelled) await addLabelsToIssue(token, candidate.ghRepo, candidate.prNumber, [READY_LABEL]); + if (!labelled) { + await addLabelsToIssue(token, candidate.ghRepo, candidate.prNumber, [READY_LABEL]); + // The label belongs to the PR, not the commit: if a push landed while CI + // was being checked, take it back off until the next run judges the new head. + const after = await getPullRequestStats(token, candidate.ghRepo, candidate.prNumber); + if (after.headSha !== stats.headSha) { + await removeLabelFromIssue(token, candidate.ghRepo, candidate.prNumber, READY_LABEL); + return { kind: 'not_ready', reason: `head moved to ${after.headSha.slice(0, 7)} while checking; label removed` }; + } + } return { kind: 'flagged', already: labelled }; } diff --git a/web/tests/pr-ready.test.ts b/web/tests/pr-ready.test.ts index 31f9eb3..52f13a8 100644 --- a/web/tests/pr-ready.test.ts +++ b/web/tests/pr-ready.test.ts @@ -53,6 +53,9 @@ describe('runPrReadyFlag (mocked GitHub)', () => { let sqls: string[]; let files: unknown[]; let rows: Record[]; + let prReads: number; + let movedHead: string | null; + let checksFail: boolean; const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); @@ -76,6 +79,7 @@ describe('runPrReadyFlag (mocked GitHub)', () => { vi.useFakeTimers(); vi.setSystemTime(new Date('2026-10-16T22:00:00Z')); calls = []; logs = []; sqls = []; + prReads = 0; movedHead = null; checksFail = false; files = [{ filename: 'docs/readme.md', status: 'modified' }]; rows = [{ id: 'task-1234abcd', title: 'docs: fix typo', pr_url: 'https://github.com/ExampleOrg/demo/pull/7' }]; pr = { @@ -91,7 +95,11 @@ describe('runPrReadyFlag (mocked GitHub)', () => { const u = String(url); const method = init?.method ?? 'GET'; calls.push({ url: u, method, body: init?.body ? JSON.parse(String(init.body)) : undefined }); - if (u.endsWith('/pulls/7') && method === 'GET') return json(pr); + if (u.endsWith('/pulls/7') && method === 'GET') { + prReads++; + return json(prReads > 1 && movedHead ? { ...pr, head: { ...(pr.head as object), sha: movedHead } } : pr); + } + if (u.includes('/check-runs') && checksFail) return new Response('forbidden', { status: 403 }); if (u.includes(`/commits/${HEAD_SHA}/status?per_page=100`)) return json({ state: 'pending', total_count: 0, statuses: [] }); if (u.includes(`/commits/${HEAD_SHA}/check-runs`)) { return json({ ...checkRuns, check_runs: checkRuns.check_runs.map((r) => ({ ...r, app: r.app ? { slug: r.app } : null })) }); @@ -148,7 +156,7 @@ describe('runPrReadyFlag (mocked GitHub)', () => { ['a failed check next to a green one', () => { checkRuns = { total_count: 2, check_runs: [green, run('test', 'completed', 'failure')] }; }, 'check test: failure'], ['no CI', () => { checkRuns = { total_count: 0, check_runs: [] }; }, 'no successful GitHub Actions check run'], ['a workflow edit', () => { files = [{ filename: '.github/workflows/ci.yml', status: 'modified' }]; }, 'touches .github/workflows/ci.yml'], - ['a workflow renamed away', () => { files = [{ filename: 'old-ci.yml', status: 'renamed', previousFilename: undefined, previous_filename: '.github/workflows/ci.yml' }]; }, 'touches .github/workflows/ci.yml'], + ['a workflow renamed away', () => { files = [{ filename: 'old-ci.yml', status: 'renamed', previous_filename: '.github/workflows/ci.yml' }]; }, 'touches .github/workflows/ci.yml'], ['a workflow edit in a different case', () => { files = [{ filename: '.GitHub/workflows/ci.yml', status: 'added' }]; }, 'touches .GitHub/workflows/ci.yml'], ['too many files to read', () => { pr.changed_files = 101; }, 'too many changed files to verify (101)'], ])('does not flag %s', async (_name, setup, reason) => { @@ -185,6 +193,24 @@ describe('runPrReadyFlag (mocked GitHub)', () => { expect(updates).toEqual([['https://example.com/not-a-pr/closed', 'task-x']]); }); + it('takes the label back off when the head moved while CI was being checked', async () => { + movedHead = 'fff999aaa000'; + const { env: e } = env(); + await runPrReadyFlag(e); + expect(labelled()).toBe(true); + expect(unlabelled()).toBe(true); + expect(notReady('head moved to fff999a while checking; label removed')).toBe(true); + }); + + it('withdraws the label when the PR can no longer be checked', async () => { + pr.labels = [{ name: READY_LABEL }]; + checksFail = true; + const { env: e } = env(); + await runPrReadyFlag(e); + expect(unlabelled()).toBe(true); + expect(logs.some((l) => l.startsWith('WARN') && l.includes('Error evaluating ExampleOrg/demo#7'))).toBe(true); + }); + it('honours required checks from the operator config', async () => { setOperatorConfig({ ...original,