From 158ad246fe8c03f885d2aaa6c1967c9aead04045 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasilev Date: Tue, 22 Sep 2026 13:41:34 +0700 Subject: [PATCH] fix(queen): a required check that refuses the pull request takes the acceptance back Measured 2026-09-22 on gHashTag/t27: the review accepted #4385, publish opened #4578, and the required parse-ratchet refused it ("parse error in fn 'is_coq' near line 17"). The pull request sat red for ever, the issue stayed open with an accept on it, and the Queen skipped it every round as "the work already landed". 42 issues were in that state while the swarm ran 2 bees of 20, and no bee ever saw the error. Each round now asks GitHub about up to eight accepted issues, oldest-asked first: is there an open pull request for the branch, and did a REQUIRED check (read from the base branch's rules) come back red on its head? If so the accept becomes a sendBack whose note is the check's own ##[error] line, which the next bee reads in its brief (judged_note). The second refusal escalates, the review's own ceiling. It decides nothing on a running or advisory check, takes nothing back when the required list cannot be read, leaves closed and merged pull requests alone, and runs as housekeeping: a failure is logged and the round goes on to start bees. Verified against live gHashTag/t27: required checks read as validate, check-linked-issue, parse-ratchet; queen-4385 -> #4578 open, parse-ratchet red, error line extracted; a merged pull request and a missing branch are left alone. Queen suites: 628 pass (614 before + 14 new), 0 fail. Co-Authored-By: Claude Opus 5 --- .../src/api/services/queen-ci-verdict.ts | 303 ++++++++++++++++++ .../server/src/api/services/queen-tick.ts | 20 ++ .../server/tests/api/queen-ci-verdict.test.ts | 215 +++++++++++++ 3 files changed, 538 insertions(+) create mode 100644 trios/agent-server/apps/server/src/api/services/queen-ci-verdict.ts create mode 100644 trios/agent-server/apps/server/tests/api/queen-ci-verdict.test.ts diff --git a/trios/agent-server/apps/server/src/api/services/queen-ci-verdict.ts b/trios/agent-server/apps/server/src/api/services/queen-ci-verdict.ts new file mode 100644 index 0000000000..f5268a16c3 --- /dev/null +++ b/trios/agent-server/apps/server/src/api/services/queen-ci-verdict.ts @@ -0,0 +1,303 @@ +/** + * A REQUIRED CHECK THAT REFUSES THE PULL REQUEST TAKES THE ACCEPTANCE BACK. + * + * The review accepts a bee's work in the container; the work lands only when + * its pull request passes the repository's required checks. Nothing connected + * the two. Measured 2026-09-22 on gHashTag/t27: the review accepted #4385 + * ("t27c 0.2.0", machineUnmet=0), publish opened #4578 for it, and the + * required `parse-ratchet` refused it - + * + * this spec parsed at the base and does not now -- Parse error in fn + * 'is_coq' near line 17: unexpected token after expression statement + * + * - so the pull request sat red for ever, the issue stayed open with an + * `accept` on it, and the Queen skipped it every round as "the work already + * landed". 42 issues were in that state while the swarm ran 2 bees of 20. No + * bee ever saw the error, because nothing handed it to one. + * + * So each round asks GitHub about a few accepted issues: is there an open + * pull request for the branch, and did a REQUIRED check on its head come back + * red? If so the acceptance becomes a send-back whose note is the check's own + * error line, which the next bee reads in its brief (`judged_note`). The + * required checks decide because they are what stops the work landing - an + * acceptance that cannot land is not an acceptance. + * + * What it will not do: + * - decide on a check still running, or on a check that is not required + * (dozens of advisory checks are red on most pull requests); + * - take anything back when the required list cannot be read - a gate it + * cannot see is not a gate it may enforce; + * - touch a closed or merged pull request; + * - loop for ever: the second refusal escalates to a person, the same + * ceiling the review uses (QueenReviewDecision.maximumSendBacks = 2). + */ +import type { Pool } from 'pg' +import { logger } from '../../lib/logger' + +/** Accepted issues asked about per round. Two GitHub reads each, plus a log per red check. */ +export const CI_CHECKS_PER_ROUND = 8 + +/** The review's own ceiling: the send-back that reaches it escalates instead. */ +export const CI_MAXIMUM_SEND_BACKS = 2 + +/** What the next bee's brief shows of a note (`PREVIOUS_REVIEW_MAX_CHARS`). */ +const NOTE_MAX = 1500 +const ERROR_MAX = 600 + +const RED = new Set([ + 'failure', + 'timed_out', + 'cancelled', + 'action_required', + 'startup_failure', +]) + +export interface CheckRun { + id: number + name: string + status: string + conclusion: string | null + url: string | null +} + +export interface PullRequest { + number: number + state: string + merged: boolean + headSha: string +} + +/** + * The required checks that came back red on this head. A re-run leaves two runs + * of one name; the newest decides. A check still running decides nothing. + */ +export function refusedRequired( + runs: CheckRun[], + required: readonly string[], +): CheckRun[] { + const wanted = new Set(required) + const newest = new Map() + for (const run of runs) { + if (!wanted.has(run.name)) continue + const seen = newest.get(run.name) + if (!seen || run.id > seen.id) newest.set(run.name, run) + } + return [...newest.values()].filter( + (run) => + run.status === 'completed' && + run.conclusion !== null && + RED.has(run.conclusion), + ) +} + +/** + * What the check said, from its Actions log: the `##[error]` lines, without + * the runner's own "Process completed with exit code" epilogue, which names + * no defect. + */ +export function errorLinesOf(log: string): string { + return log + .split('\n') + .map((line) => line.replace(/^\s*\d{4}-\d\d-\d\dT[\d:.]+Z\s?/, '').trim()) + .filter((line) => line.startsWith('##[error]')) + .map((line) => line.slice('##[error]'.length).trim()) + .filter((line) => line && !/^Process completed with exit code/.test(line)) + .join(' | ') + .slice(0, ERROR_MAX) +} + +/** The note the next bee reads. The check's words first; ours are only the frame. */ +export function ciRefusalNote( + pull: number, + branch: string, + refused: Array<{ name: string; error: string; url: string | null }>, +): string { + const lines = refused.map( + (r) => + `- ${r.name}: ${r.error || 'failed; its log held no error line'}` + + (r.url ? ` (${r.url})` : ''), + ) + return [ + `A required check refused pull request #${pull} for ${branch}:`, + ...lines, + 'The review had accepted this work, but the required checks are what let ' + + 'it land, so they decide. Fix it on the same branch; the pull request ' + + 'runs its checks again on the next push.', + ] + .join('\n') + .slice(0, NOTE_MAX) +} + +export interface CiDeps { + /** Names of the required checks on the base branch, or null when unreadable. */ + requiredChecks(): Promise + /** The pull requests whose head is this branch, or null when unreadable. */ + pullsForBranch(branch: string): Promise + checkRuns(sha: string): Promise + jobLog(jobId: number): Promise +} + +export interface TakenBack { + issue: number + pull: number + state: 'sendBack' | 'escalate' + checks: string[] +} + +export async function takeBackRefusedAcceptances( + pool: Pool, + deps: CiDeps, + limit = CI_CHECKS_PER_ROUND, +): Promise { + const required = await deps.requiredChecks() + if (!required || required.length === 0) return [] + + // Oldest-asked first, so eight a round walks the whole accepted set rather + // than asking about the same eight for ever. + const rows = await pool.query( + `SELECT issue, branch, send_backs + FROM queen_dispatch + WHERE review_state = 'accept' AND finished_at IS NOT NULL + ORDER BY ci_checked_at ASC NULLS FIRST, issue + LIMIT $1`, + [limit], + ) + + const taken: TakenBack[] = [] + for (const row of rows.rows) { + const issue = Number(row.issue) + const branch = String(row.branch || `queen-${issue}`) + await pool.query( + `UPDATE queen_dispatch SET ci_checked_at = now() WHERE issue = $1`, + [issue], + ) + + const pulls = await deps.pullsForBranch(branch) + const open = pulls?.find((p) => p.state === 'open' && !p.merged) + if (!open) continue + const runs = await deps.checkRuns(open.headSha) + if (!runs) continue + const red = refusedRequired(runs, required) + if (red.length === 0) continue + + const refused: Array<{ name: string; error: string; url: string | null }> = + [] + for (const run of red) { + const log = await deps.jobLog(run.id) + refused.push({ + name: run.name, + error: log ? errorLinesOf(log) : '', + url: run.url, + }) + } + const note = ciRefusalNote(open.number, branch, refused) + const sendBacks = Number(row.send_backs ?? 0) + 1 + const state: 'sendBack' | 'escalate' = + sendBacks >= CI_MAXIMUM_SEND_BACKS ? 'escalate' : 'sendBack' + + // Guarded on the state it read, so a review that moved the row in the + // meantime is never overwritten. + const updated = await pool.query( + `UPDATE queen_dispatch + SET review_state = $2, review_note = $3, judged_note = $3, + reviewed_at = now(), send_backs = $4::integer + WHERE issue = $1 AND review_state = 'accept'`, + [issue, state, note, sendBacks], + ) + if (!updated.rowCount) continue + const checks = red.map((r) => r.name) + taken.push({ issue, pull: open.number, state, checks }) + logger.info('Queen took back an acceptance a required check refused', { + issue, + pull: open.number, + state, + checks, + }) + } + return taken +} + +/** + * GitHub, read with the round's token. Every failure is null - "could not + * ask" - never an empty list, which would read as "asked, and found nothing". + */ +export function githubCiDeps( + repo: string, + baseBranch: string, + headers: Record, +): CiDeps { + const api = `https://api.github.com/repos/${repo}` + const owner = repo.split('/')[0] + const json = async (url: string): Promise => { + try { + const response = await fetch(url, { headers }) + return response.ok ? await response.json() : null + } catch { + return null + } + } + return { + async requiredChecks() { + const rules = (await json( + `${api}/rules/branches/${encodeURIComponent(baseBranch)}`, + )) as Array<{ + type?: string + parameters?: { required_status_checks?: Array<{ context?: string }> } + }> | null + if (!Array.isArray(rules)) return null + return rules + .filter((rule) => rule.type === 'required_status_checks') + .flatMap((rule) => rule.parameters?.required_status_checks ?? []) + .map((check) => check.context) + .filter((name): name is string => typeof name === 'string') + }, + async pullsForBranch(branch) { + const pulls = (await json( + `${api}/pulls?state=all&per_page=5&head=${encodeURIComponent(`${owner}:${branch}`)}`, + )) as Array<{ + number: number + state: string + merged_at: string | null + head: { sha: string } + }> | null + if (!Array.isArray(pulls)) return null + return pulls.map((p) => ({ + number: p.number, + state: p.state, + merged: p.merged_at !== null, + headSha: p.head.sha, + })) + }, + async checkRuns(sha) { + const body = (await json( + `${api}/commits/${sha}/check-runs?per_page=100`, + )) as { + check_runs?: Array<{ + id: number + name: string + status: string + conclusion: string | null + html_url: string | null + }> + } | null + if (!body?.check_runs) return null + return body.check_runs.map((run) => ({ + id: run.id, + name: run.name, + status: run.status, + conclusion: run.conclusion, + url: run.html_url, + })) + }, + async jobLog(jobId) { + try { + const response = await fetch(`${api}/actions/jobs/${jobId}/logs`, { + headers, + }) + return response.ok ? await response.text() : null + } catch { + return null + } + }, + } +} diff --git a/trios/agent-server/apps/server/src/api/services/queen-tick.ts b/trios/agent-server/apps/server/src/api/services/queen-tick.ts index 4cc99de5da..8a7ab44a02 100644 --- a/trios/agent-server/apps/server/src/api/services/queen-tick.ts +++ b/trios/agent-server/apps/server/src/api/services/queen-tick.ts @@ -42,6 +42,7 @@ import { createQueenPool } from '../../lib/db/queen-pool' import { logger } from '../../lib/logger' import { startModelProbes, workerModelRanking } from '../../lib/model-ranking' import { outstandingEscalations } from '../routes/queen-needs-you' +import { githubCiDeps, takeBackRefusedAcceptances } from './queen-ci-verdict' import { type CriterionRun, criteriaCounts, @@ -51,6 +52,7 @@ import { parseCriterionChecks, } from './queen-criteria-run' import { + baseRef, DISPATCH_OUTCOME_LABELS, dispatchBee, reapDispatchesFromPreviousBoot, @@ -464,6 +466,10 @@ async function ensureQueenColumns(pool: Pool): Promise { ADD COLUMN IF NOT EXISTS judged_head text, ADD COLUMN IF NOT EXISTS judged_conversation text, ADD COLUMN IF NOT EXISTS judged_note text, + -- When the round last asked GitHub whether an accepted issue's pull + -- request passed its required checks (queen-ci-verdict.ts), so a few a + -- round walk the whole accepted set instead of the same few for ever. + ADD COLUMN IF NOT EXISTS ci_checked_at timestamptz, -- The issue's own criterion commands, run by the Queen on the commit, -- keyed like the reviewer cache (branch head, merge base, criteria). A -- wait row is re-read every 60 s, and a measurement is a temporary @@ -1553,6 +1559,20 @@ export async function runRound( logger.info('Queen reviewed her own work', { verdicts: reviewed.acted }) } + // An acceptance whose pull request a REQUIRED check refused is not one: + // the work cannot land, and the issue would otherwise be skipped as "the + // work already landed" for ever (queen-ci-verdict.ts). Housekeeping: a + // failure here is logged and the round goes on to start bees. + await takeBackRefusedAcceptances( + pool, + githubCiDeps(repo, baseRef().replace(/^origin\//, ''), githubReadHeaders()), + ).catch((error) => { + logger.warn('Queen could not ask CI about accepted work', { + error: error instanceof Error ? error.message : String(error), + }) + return [] + }) + const reaped = await reapStalledDispatches(pool) if (reaped.length > 0) { logger.info('Queen tick reaped stalled dispatches', { issues: reaped }) diff --git a/trios/agent-server/apps/server/tests/api/queen-ci-verdict.test.ts b/trios/agent-server/apps/server/tests/api/queen-ci-verdict.test.ts new file mode 100644 index 0000000000..4168342b3e --- /dev/null +++ b/trios/agent-server/apps/server/tests/api/queen-ci-verdict.test.ts @@ -0,0 +1,215 @@ +import { describe, expect, it } from 'bun:test' +import type { Pool } from 'pg' +import { + type CheckRun, + type CiDeps, + ciRefusalNote, + errorLinesOf, + type PullRequest, + refusedRequired, + takeBackRefusedAcceptances, +} from '../../src/api/services/queen-ci-verdict' + +/** + * An acceptance whose pull request a required check refused is taken back. + * + * 2026-09-22, gHashTag/t27: #4385 was accepted, publish opened #4578, and the + * required `parse-ratchet` refused it. The pull request sat red, the issue + * was skipped as "the work already landed", and no bee ever saw the error. + */ + +const REQUIRED = ['validate', 'check-linked-issue', 'parse-ratchet'] + +// The shape of the real log of #4578's parse-ratchet job. +const PARSE_RATCHET_LOG = [ + '2026-09-22T06:18:01.1023264Z ##[group]Run python3 tools/ci/check_specs_still_parse.py', + '2026-09-22T06:18:04.0000000Z changed specs: 1; newly unparseable: 1; repaired: 0', + "2026-09-22T06:18:04.1000000Z ##[error]this spec parsed at the base and does not now -- Error: Parse error: parse error in fn 'is_coq' near line 17: unexpected token after expression statement: Ident", + '2026-09-22T06:18:04.2000000Z A spec that does not parse generates nothing, so every test it carries', + '2026-09-22T06:18:04.3000000Z ##[error]Process completed with exit code 1.', +].join('\n') + +const run = ( + id: number, + name: string, + conclusion: string | null, + status = 'completed', +): CheckRun => ({ id, name, status, conclusion, url: `https://ci/${id}` }) + +describe('which required checks refused', () => { + it('counts only required checks that completed red', () => { + const red = refusedRequired( + [ + run(1, 'parse-ratchet', 'failure'), + run(2, 'validate', 'success'), + run(3, 'coverage', 'failure'), // advisory, red on most pull requests + run(4, 'check-linked-issue', null, 'in_progress'), + ], + REQUIRED, + ) + expect(red.map((r) => r.name)).toEqual(['parse-ratchet']) + }) + + it('lets the newest run of a check decide, so a green re-run clears a red one', () => { + expect( + refusedRequired( + [ + run(10, 'parse-ratchet', 'failure'), + run(11, 'parse-ratchet', 'success'), + ], + REQUIRED, + ), + ).toEqual([]) + expect( + refusedRequired( + [ + run(11, 'parse-ratchet', 'success'), + run(12, 'parse-ratchet', 'failure'), + ], + REQUIRED, + ).map((r) => r.id), + ).toEqual([12]) + }) +}) + +describe('what the check said', () => { + it('keeps the error line and drops the runner epilogue', () => { + const error = errorLinesOf(PARSE_RATCHET_LOG) + expect(error).toContain("parse error in fn 'is_coq' near line 17") + expect(error).not.toContain('Process completed with exit code') + expect(error).not.toContain('2026-09-22T') + }) + + it('puts the error first in the note the next bee reads', () => { + const note = ciRefusalNote(4578, 'queen-4385', [ + { + name: 'parse-ratchet', + error: errorLinesOf(PARSE_RATCHET_LOG), + url: null, + }, + ]) + expect(note.split('\n')[0]).toBe( + 'A required check refused pull request #4578 for queen-4385:', + ) + expect(note).toContain('- parse-ratchet: this spec parsed at the base') + expect(note.length).toBeLessThanOrEqual(1500) + }) +}) + +/** A pool that answers the SELECT with `rows` and records every statement. */ +function fakePool( + rows: Array<{ issue: number; branch: string; send_backs: number }>, +) { + const statements: Array<{ sql: string; params: unknown[] }> = [] + const pool = { + async query(sql: string, params: unknown[] = []) { + statements.push({ sql, params }) + if (/^\s*SELECT/.test(sql)) return { rows, rowCount: rows.length } + return { rows: [], rowCount: 1 } + }, + } + return { pool: pool as unknown as Pool, statements } +} + +function deps(over: Partial = {}): CiDeps { + const pull: PullRequest = { + number: 4578, + state: 'open', + merged: false, + headSha: 'abc', + } + return { + requiredChecks: async () => REQUIRED, + pullsForBranch: async () => [pull], + checkRuns: async () => [ + run(7, 'parse-ratchet', 'failure'), + run(8, 'validate', 'success'), + ], + jobLog: async () => PARSE_RATCHET_LOG, + ...over, + } +} + +const verdictWrites = (statements: Array<{ sql: string; params: unknown[] }>) => + statements.filter((s) => /SET review_state/.test(s.sql)) + +describe('taking an acceptance back', () => { + it('turns a refused acceptance into a send-back carrying the check error', async () => { + const { pool, statements } = fakePool([ + { issue: 4385, branch: 'queen-4385', send_backs: 0 }, + ]) + const taken = await takeBackRefusedAcceptances(pool, deps()) + expect(taken).toEqual([ + { issue: 4385, pull: 4578, state: 'sendBack', checks: ['parse-ratchet'] }, + ]) + const [write] = verdictWrites(statements) + expect(write.params[0]).toBe(4385) + expect(write.params[1]).toBe('sendBack') + expect(String(write.params[2])).toContain("fn 'is_coq' near line 17") + expect(write.params[3]).toBe(1) + // Guarded on the state it read: a row the review moved is not overwritten. + expect(write.sql).toContain("review_state = 'accept'") + }) + + it('escalates the refusal that reaches the send-back ceiling', async () => { + const { pool } = fakePool([ + { issue: 4385, branch: 'queen-4385', send_backs: 1 }, + ]) + const [taken] = await takeBackRefusedAcceptances(pool, deps()) + expect(taken.state).toBe('escalate') + }) + + it('takes nothing back when the required checks cannot be read', async () => { + const { pool, statements } = fakePool([ + { issue: 4385, branch: 'queen-4385', send_backs: 0 }, + ]) + const taken = await takeBackRefusedAcceptances( + pool, + deps({ requiredChecks: async () => null }), + ) + expect(taken).toEqual([]) + expect(statements).toEqual([]) + }) + + for (const [what, over] of [ + [ + 'a green pull request', + { checkRuns: async () => [run(7, 'parse-ratchet', 'success')] }, + ], + [ + 'a pending check', + { checkRuns: async () => [run(7, 'parse-ratchet', null, 'queued')] }, + ], + [ + 'a merged pull request', + { + pullsForBranch: async () => [ + { number: 1, state: 'closed', merged: true, headSha: 'x' }, + ], + }, + ], + ['no pull request', { pullsForBranch: async () => [] }], + ['an unreadable pull request list', { pullsForBranch: async () => null }], + ['unreadable check runs', { checkRuns: async () => null }], + ] as Array<[string, Partial]>) { + it(`leaves the acceptance alone for ${what}, and remembers it asked`, async () => { + const { pool, statements } = fakePool([ + { issue: 4385, branch: 'queen-4385', send_backs: 0 }, + ]) + expect(await takeBackRefusedAcceptances(pool, deps(over))).toEqual([]) + expect(verdictWrites(statements)).toEqual([]) + expect( + statements.some((s) => /ci_checked_at = now\(\)/.test(s.sql)), + ).toBe(true) + }) + } + + it('still takes it back when the log cannot be read, and says so', async () => { + const { pool, statements } = fakePool([ + { issue: 4385, branch: 'queen-4385', send_backs: 0 }, + ]) + await takeBackRefusedAcceptances(pool, deps({ jobLog: async () => null })) + const [write] = verdictWrites(statements) + expect(String(write.params[2])).toContain('its log held no error line') + }) +})