diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 20ab58f..7446b17 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -53,10 +53,12 @@ A cron job runs the sweep every minute (`*/1 * * * *`); the sweep self-locks so failed heads are throttled in local state instead.) The marker check falls back to "any comment" when `gh api user` is unavailable (a GitHub-App integration 403s on it), so dedup never silently re-reviews forever. 4. **Build memory** from the remaining comments (see below). -5. **Review.** The _runner_ fetches the diff (`gh pr diff`) and feeds it to `codex exec` over **stdin**, in a - `workspace-write` sandbox restricted to `/tmp` with **network off and no `gh`**. Codex reads the rubric + - corpus + the inlined diff and writes the review to a temp file ending in the marker; the _runner_, not Codex, - posts it with `gh pr comment`. +5. **Review.** The _runner_ fetches the diff via GitHub's compare API (`baseRefOid...headRefOid`), so stacked PRs + whose base is another feature branch diff against that base, not `main`. It spins a detached worktree at the PR + head SHA (`$STUPIFY_HOME/worktrees/-`) so codex reads the same tree the diff describes, then feeds the + diff to `codex exec` over **stdin**, in a `workspace-write` sandbox restricted to `/tmp` with **network off and + no `gh`**. Codex reads the rubric + corpus + the inlined diff and writes the review to a temp file ending in the + marker; the _runner_, not Codex, posts it with `gh pr comment`. Candidates are collected serially (all the cheap gh gates), then reviewed by a pool of up to `CODEX_JOBS` (default 3) concurrent codex runs — a busy sweep's wall-clock is the slowest review, not the sum of them. A quota wall from any run stops new launches while in-flight runs drain. diff --git a/src/cli.ts b/src/cli.ts index 473ec12..fe2704c 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -8,7 +8,7 @@ * `stupify run [--dry]` → run one review sweep right now. */ import { spawnSync } from 'node:child_process' -import { copyFileSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { appendFileSync, copyFileSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { homedir } from 'node:os' import { dirname, join, relative, resolve } from 'node:path' import { fileURLToPath } from 'node:url' @@ -74,6 +74,20 @@ function die(message: string): never { process.exit(1) } +/** Trust an extra directory in ~/.codex/config.toml (stacked-PR worktrees live outside the main checkout). */ +function appendCodexProjectTrust(dir: string): void { + const file = join(process.env.CODEX_HOME ?? join(homedir(), '.codex'), 'config.toml') + if (!existsSync(file)) { + return + } + const body = readFileSync(file, 'utf8') + const key = `[projects."${dir}"]` + if (body.includes(key)) { + return + } + appendFileSync(file, `\n${key}\ntrust_level = "trusted"\n`) +} + async function installSweepEngine(dest = join(HOME, 'review-sweep.ts')): Promise { const built = await Bun.build({ entrypoints: [join(PKG_DIR, 'review-sweep.ts')], @@ -450,10 +464,13 @@ async function setup(argv: { .join('\n') writeFileSync(join(HOME, 'config.env'), `${cfg}\n`) if (host) { + const worktrees = join(HOME, 'worktrees') + mkdirSync(worktrees, { recursive: true }) writeCodexGatewayConfig({ ...(argv.codexHost === undefined ? {} : { gatewayHost: argv.codexHost }), trustDir: join(HOME, 'repo'), }) + appendCodexProjectTrust(worktrees) } // exe.dev VM: route Codex through the no-key exe-llm gateway try { installCron({ diff --git a/src/review-sweep.test.ts b/src/review-sweep.test.ts index 4acf8f6..9139220 100644 --- a/src/review-sweep.test.ts +++ b/src/review-sweep.test.ts @@ -66,9 +66,11 @@ const cfg = (): Config => ({ codexJobs: 3, }) -const pr = (number: number, sha: string): Pr => ({ +const pr = (number: number, sha: string, base = 'main', baseSha = 'a'.repeat(40)): Pr => ({ number, headRefOid: sha, + baseRefOid: baseSha, + baseRefName: base, isDraft: false, author: { login: 'someone', is_bot: false }, labels: [{ name: 'codex-review' }], diff --git a/src/sweep/codex.ts b/src/sweep/codex.ts index 590fa77..b330a32 100644 --- a/src/sweep/codex.ts +++ b/src/sweep/codex.ts @@ -79,7 +79,9 @@ export async function runReview( priorThread: string, diff: string, dismissed: string[] = [], + workDir?: string, ): Promise { + const cwd = workDir ?? cfg.repoDir const outPath = reviewOutPath(cfg, pr) rmSync(outPath, { force: true }) // clear any stale file so we never read a previous run's review const schemaPath = join(cfg.stateDir, 'review-schema.json') @@ -87,7 +89,7 @@ export async function runReview( const codexArgs = [ 'exec', '--cd', - cfg.repoDir, + cwd, '--output-schema', schemaPath, // the provider enforces ReviewOutput on the final message... '--output-last-message', @@ -110,7 +112,7 @@ export async function runReview( codexArgs.push('-') // read the prompt from STDIN, not argv — the inlined corpus + diff would blow ARG_MAX (E2BIG) const cx = await execAsync('codex', codexArgs, { - cwd: cfg.repoDir, + cwd, timeoutMs: 1_200_000, input: reviewPrompt(cfg, pr, priorThread, diff, dismissed), }) diff --git a/src/sweep/diff.ts b/src/sweep/diff.ts index 8150385..9935842 100644 --- a/src/sweep/diff.ts +++ b/src/sweep/diff.ts @@ -1,14 +1,15 @@ // Fetching and measuring PR diffs. The RUNNER fetches the diff (not codex) so codex needs no network or gh — -// it reviews the diff straight from the prompt, sandboxed. +// it reviews the diff straight from the prompt, sandboxed. Stacked PRs diff base..head via the compare API, not main. import { exec } from '@bevyl-ai/agent-tools' import { type Config } from './config' +import { type Pr } from './prs' // GitHub's diff endpoint 406s past EITHER of these, so a big enough PR can't even be MEASURED. They are GitHub's // limits, not ours — DIFF_LINE_CAP can be set above the line one but never reached. export const GH_DIFF_LIMITS = '20000-line / 300-file' -/** A `gh pr diff` failure that is GitHub's size refusal rather than a transient error — retrying can never fix it. +/** A compare/diff failure that is GitHub's size refusal rather than a transient error — retrying can never fix it. * Matches on gh's stable `too_large` code first: the prose differs per limit (lines vs files) and can be reworded, * but both variants carry the code. Missing one variant is what kept #8338/#8241 looping after the first fix. */ export const isDiffTooLarge = (output: string): boolean => @@ -19,8 +20,14 @@ export const isDiffTooLarge = (output: string): boolean => // 'too-large' is terminal, and conflating them is what left oversized PRs re-fetched every 60s forever. type DiffRead = { ok: true; diff: string } | { ok: false; reason: 'unreadable' | 'too-large' } -export function getDiff(cfg: Config, number: number): DiffRead { - const r = exec('gh', ['pr', 'diff', String(number), '--repo', cfg.slug]) +/** Diff the PR's head against its base (not defaultBranch) — correct for stacked PRs. */ +export function getDiff(cfg: Config, pr: Pick): DiffRead { + const r = exec('gh', [ + 'api', + `repos/${cfg.slug}/compare/${pr.baseRefOid}...${pr.headRefOid}`, + '-H', + 'Accept: application/vnd.github.diff', + ]) if (r.ok) { return { ok: true, diff: r.stdout } } diff --git a/src/sweep/prs.ts b/src/sweep/prs.ts index 3ed7a4e..1146d76 100644 --- a/src/sweep/prs.ts +++ b/src/sweep/prs.ts @@ -10,6 +10,8 @@ import { type Config, log } from './config' export const Pr = z.object({ number: z.number(), headRefOid: z.string(), + baseRefOid: z.string(), + baseRefName: z.string(), isDraft: z.boolean(), author: z.object({ login: z.string(), is_bot: z.boolean() }).nullable(), // is_bot flags GitHub App bots (app/dependabot) the [bot] suffix misses labels: z.array(z.object({ name: z.string() })), @@ -26,7 +28,7 @@ const PR_LIST_LIMIT = 500 export function listPrs(cfg: Config): Pr[] | null { // Filter the PR list directly rather than `gh pr list --label` — that search index lags behind labelling. - const fields = 'number,headRefOid,isDraft,author,labels,title,body' + const fields = 'number,headRefOid,baseRefOid,baseRefName,isDraft,author,labels,title,body' const r = exec('gh', [ 'pr', 'list', diff --git a/src/sweep/review-one.ts b/src/sweep/review-one.ts index 094aa07..781b325 100644 --- a/src/sweep/review-one.ts +++ b/src/sweep/review-one.ts @@ -1,8 +1,9 @@ // `stupify review ` — review ONE pull request on demand (no cron, no checkout, no lock) and print it, // or `--post` it. Always a FRESH perspective: no prior-review memory, so you get the full take. +import { existsSync } from 'node:fs' import { join } from 'node:path' -import { exec } from '@bevyl-ai/agent-tools' +import { detectRepo, exec } from '@bevyl-ai/agent-tools' import { runReview } from './codex' import { type Config } from './config' @@ -10,6 +11,7 @@ import { getDiff, GH_DIFF_LIMITS } from './diff' import { postReview } from './github' import { hasMachinery } from './prompt' import { Pr } from './prs' +import { prepareHeadWorktree, removeHeadWorktree } from './worktree' /** Accepts a PR URL or `owner/repo#123` (the CLI resolves a bare `#123` against the cwd repo before calling here). */ export async function reviewOne(cfg: Config, ref: string, post: boolean): Promise { @@ -29,15 +31,25 @@ export async function reviewOne(cfg: Config, ref: string, post: boolean): Promis console.error('stupify review: no taste found. Run `stupify taste` (or add a .review/ to this repo) first.') process.exit(1) } - // No checkout for an ad-hoc review — codex reviews from the inlined diff. Run it in the current directory (codex - // needs a real workspace to operate in); if you're standing in the target repo it gets useful file context for free. + // Ad-hoc review runs in cwd. When cwd is the target repo we spin a head worktree for file context; otherwise codex + // reviews from the inlined diff alone (cross-repo refs can't fetch into a foreign checkout). cfg.repoDir = process.cwd() - const head = exec('gh', ['pr', 'view', String(number), '--repo', slug, '--json', 'headRefOid,title,body']) + const head = exec('gh', [ + 'pr', + 'view', + String(number), + '--repo', + slug, + '--json', + 'headRefOid,baseRefOid,baseRefName,title,body', + ]) if (!head.ok) { console.error(`stupify review: couldn't read ${slug}#${number} via gh (auth? does it exist?).`) process.exit(1) } - const meta = Pr.pick({ headRefOid: true, title: true, body: true }).parse(JSON.parse(head.stdout)) + const meta = Pr.pick({ headRefOid: true, baseRefOid: true, baseRefName: true, title: true, body: true }).parse( + JSON.parse(head.stdout), + ) const pr = { number, ...meta, @@ -45,7 +57,7 @@ export async function reviewOne(cfg: Config, ref: string, post: boolean): Promis author: { login: '', is_bot: false }, labels: [], } - const read = getDiff(cfg, number) + const read = getDiff(cfg, pr) if (!read.ok) { console.error( read.reason === 'too-large' @@ -55,8 +67,26 @@ export async function reviewOne(cfg: Config, ref: string, post: boolean): Promis process.exit(1) } const { diff } = read - console.error(`reviewing ${slug}#${number} …`) // progress on stderr; stdout stays just the review - const r = await runReview(cfg, pr, '', diff) // no memory: a manual review is always a fresh, full take + const localRepo = detectRepo() + const canWorktree = + localRepo !== null && localRepo.toLowerCase() === slug.toLowerCase() && existsSync(join(cfg.repoDir, '.git')) + console.error(`reviewing ${slug}#${number} (base ${pr.baseRefName}) …`) // progress on stderr; stdout stays just the review + let workDir: string | undefined + if (canWorktree) { + workDir = prepareHeadWorktree(cfg.repoDir, pr) ?? undefined + if (workDir === undefined) { + console.error(`stupify review: couldn't checkout head for ${slug}#${number} (git fetch/worktree failed).`) + process.exit(1) + } + } + let r + try { + r = await runReview(cfg, pr, '', diff, [], workDir) + } finally { + if (canWorktree && workDir !== undefined) { + removeHeadWorktree(cfg.repoDir, pr) + } + } if (r.kind === 'limit' || r.kind === 'fail') { console.error( `stupify review: ${r.kind === 'limit' ? 'codex is out of credits / rate-limited' : "codex couldn't produce a review"} — ${r.reason}`, diff --git a/src/sweep/review-pr.ts b/src/sweep/review-pr.ts index 6679e59..9065dd4 100644 --- a/src/sweep/review-pr.ts +++ b/src/sweep/review-pr.ts @@ -8,6 +8,7 @@ import { type Config, log } from './config' import { postNote, postReview, resolveThreads } from './github' import { type Pr } from './prs' import { FIXED_NOTE, STILL_NOTE } from './verdict' +import { prepareHeadWorktree, removeHeadWorktree } from './worktree' // A posted review carries its token spend and its blocking-finding count — zero blocking reads as a green status. export type SweepReviewResult = { tokens: number; blocking: number } | 'limit' | 'clean' | 'fixed' | 'open' | null @@ -47,8 +48,18 @@ export async function reviewPr( openThreadIds: string[], dismissed: string[], ): Promise { - log(`reviewing PR #${pr.number} @ ${pr.headRefOid.slice(0, 8)}`) - const r = await runReview(cfg, pr, priorThread, diff, dismissed) + log(`reviewing PR #${pr.number} @ ${pr.headRefOid.slice(0, 8)} (base ${pr.baseRefName})`) + const workDir = prepareHeadWorktree(cfg.repoDir, pr) + if (workDir === null) { + log(` review FAILED for #${pr.number} — couldn't checkout head for file context`) + return null + } + let r + try { + r = await runReview(cfg, pr, priorThread, diff, dismissed, workDir) + } finally { + removeHeadWorktree(cfg.repoDir, pr) + } if (r.kind === 'limit' || r.kind === 'fail') { log(` review FAILED for #${pr.number} — ${r.reason}`) if (r.kind === 'limit') { diff --git a/src/sweep/sweep.ts b/src/sweep/sweep.ts index 17be025..b52c0d8 100644 --- a/src/sweep/sweep.ts +++ b/src/sweep/sweep.ts @@ -118,7 +118,7 @@ export function collectCandidates( } // Fetch the diff once, here in the runner — codex reviews it from the prompt with no network/gh of its own. - const read = getDiff(cfg, pr.number) + const read = getDiff(cfg, pr) if (!read.ok && read.reason === 'too-large') { // Terminal: gh will never hand us this diff, so there is nothing to retry and nothing to measure. Say so // plainly — the old wording promised a retry that could not possibly succeed. diff --git a/src/sweep/worktree.ts b/src/sweep/worktree.ts new file mode 100644 index 0000000..50a0c1f --- /dev/null +++ b/src/sweep/worktree.ts @@ -0,0 +1,32 @@ +// Detached worktree at a PR's head SHA so codex reads the same tree the diff describes — required for stacked +// PRs whose base is not main (the shared checkout stays on defaultBranch for refreshRepo). +import { rmSync } from 'node:fs' +import { dirname, join } from 'node:path' + +import { exec } from '@bevyl-ai/agent-tools' + +import { type Pr } from './prs' + +export function headWorktreePath(repoDir: string, pr: Pr): string { + return join(dirname(repoDir), 'worktrees', `${pr.number}-${pr.headRefOid.slice(0, 8)}`) +} + +/** Fetch base+head and add a detached worktree at the PR head. Returns null on failure. */ +export function prepareHeadWorktree(repoDir: string, pr: Pr): string | null { + const dir = headWorktreePath(repoDir, pr) + rmSync(dir, { recursive: true, force: true }) + exec('git', ['worktree', 'prune'], { cwd: repoDir }) + if ( + !exec('git', ['fetch', '-q', 'origin', pr.baseRefOid, pr.headRefOid], { cwd: repoDir }).ok || + !exec('git', ['worktree', 'add', '--detach', dir, pr.headRefOid], { cwd: repoDir }).ok + ) { + return null + } + return dir +} + +export function removeHeadWorktree(repoDir: string, pr: Pr): void { + const dir = headWorktreePath(repoDir, pr) + exec('git', ['worktree', 'remove', '--force', dir], { cwd: repoDir }) + rmSync(dir, { recursive: true, force: true }) +}