Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions docs/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<n>-<sha>`) 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.
Expand Down
19 changes: 18 additions & 1 deletion src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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<void> {
const built = await Bun.build({
entrypoints: [join(PKG_DIR, 'review-sweep.ts')],
Expand Down Expand Up @@ -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({
Expand Down
4 changes: 3 additions & 1 deletion src/review-sweep.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' }],
Expand Down
6 changes: 4 additions & 2 deletions src/sweep/codex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,17 @@ export async function runReview(
priorThread: string,
diff: string,
dismissed: string[] = [],
workDir?: string,
): Promise<ReviewOutcome> {
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')
writeFileSync(schemaPath, JSON.stringify(REVIEW_SCHEMA))
const codexArgs = [
'exec',
'--cd',
cfg.repoDir,
cwd,
'--output-schema',
schemaPath, // the provider enforces ReviewOutput on the final message...
'--output-last-message',
Expand All @@ -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),
})
Expand Down
15 changes: 11 additions & 4 deletions src/sweep/diff.ts
Original file line number Diff line number Diff line change
@@ -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 =>
Expand All @@ -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<Pr, 'baseRefOid' | 'headRefOid'>): 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 }
}
Expand Down
4 changes: 3 additions & 1 deletion src/sweep/prs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() })),
Expand All @@ -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',
Expand Down
46 changes: 38 additions & 8 deletions src/sweep/review-one.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
// `stupify review <pr>` — 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'
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<void> {
Expand All @@ -29,23 +31,33 @@ 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(
Comment thread
Octember marked this conversation as resolved.
JSON.parse(head.stdout),
)
const pr = {
number,
...meta,
isDraft: false,
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'
Expand All @@ -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}`,
Expand Down
15 changes: 13 additions & 2 deletions src/sweep/review-pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -47,8 +48,18 @@ export async function reviewPr(
openThreadIds: string[],
dismissed: string[],
): Promise<SweepReviewResult> {
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') {
Expand Down
2 changes: 1 addition & 1 deletion src/sweep/sweep.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 src/sweep/sweep.ts:121 · bug · conf 0.98
The diff now depends on baseRefOid, but dedup/failure state is still keyed only by headRefOid; when a stacked base advances without changing this PR's head, the sweep skips the changed compare entirely.
→ Fix: key the existing review marker and local attempt state by the base+head compare identity, not head alone.

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.
Expand Down
32 changes: 32 additions & 0 deletions src/sweep/worktree.ts
Original file line number Diff line number Diff line change
@@ -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 ||

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 src/sweep/worktree.ts:20 · footgun · conf 0.93
Default pool workers call git fetch concurrently against the same checkout, contending on shared fetch metadata such as FETCH_HEAD; one preparation can fail and throttle an otherwise valid PR.
→ Fix: pass Git's existing --no-write-fetch-head flag, or perform the fetches in the existing serial candidate-collection phase.

!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 })
}