Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
100 changes: 96 additions & 4 deletions crux/commands/pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,8 @@ async function detect(_args: string[], options: CommandOptions): Promise<Command
* --body="PR body" Body as inline string (vulnerable to shell expansion).
* --body-file=<path> Body from a file (safe for markdown with backticks).
* --base=main Base branch (default: main).
* --draft Create as draft PR.
* --draft Create as draft PR (default: true).
* --no-draft Create as ready PR (not draft).
*
* If a PR already exists for this branch, reports it instead of creating a duplicate.
*/
Expand All @@ -210,7 +211,8 @@ async function create(_args: string[], options: CommandOptions): Promise<Command
const bodyFile = (options.bodyFile ?? options['body-file']) as string | undefined;
let body = options.body as string | undefined;
const base = (options.base as string) || 'main';
const draft = Boolean(options.draft);
// Default to draft unless --no-draft is explicitly passed
const draft = options.noDraft === true || options['no-draft'] === true ? false : true;

// --body-file takes precedence (avoids shell expansion of backticks in markdown)
if (bodyFile) {
Expand Down Expand Up @@ -602,12 +604,97 @@ async function resolveConflictsCmd(_args: string[], options: CommandOptions): Pr
return { output, exitCode: failed > 0 ? 1 : 0 };
}

/**
* Mark the current branch's PR as ready for review.
*
* Validates eligibility (CI green, no conflicts, no unresolved threads,
* no unchecked checkboxes) before converting from draft to ready.
*
* Options:
* --pr=N Target a specific PR number instead of auto-detecting.
* --force Skip eligibility checks and mark as ready anyway.
*/
async function ready(_args: string[], options: CommandOptions): Promise<CommandResult> {
const log = createLogger(Boolean(options.ci));
const c = log.colors;
const force = Boolean(options.force);

let prNum: number | null = options.pr ? parseInt(String(options.pr), 10) : null;

if (!prNum) {
const branch = currentBranch();
const prs = await githubApi<GitHubPR[]>(
`/repos/${REPO}/pulls?head=quantified-uncertainty:${branch}&state=open`
);
if (!prs.length) {
return {
output: `${c.red}No open PR found for branch ${currentBranch()}.${c.reset}\n`,
exitCode: 1,
};
}
prNum = prs[0].number;
}

// Fetch full PR details via GraphQL for eligibility checks
const { checkMergeEligibility, fetchSinglePr } = await import('../pr-patrol/index.ts');
const prNode = await fetchSinglePr(prNum);

if (!prNode) {
return {
output: `${c.red}Could not fetch PR #${prNum} details.${c.reset}\n`,
exitCode: 1,
};
}

// Check eligibility (unless --force)
if (!force) {
const eligibility = checkMergeEligibility(prNode);
// Filter out ci-pending (normal for newly-pushed PRs) and is-draft
// (the whole point of this command is to undraft — it's not a block reason here)
const blockReasons = eligibility.blockReasons.filter(
(r: string) => r !== 'ci-pending' && r !== 'is-draft',
);

if (blockReasons.length > 0) {
let output = `${c.red}✗ PR #${prNum} is not eligible to be marked ready:${c.reset}\n`;
for (const reason of blockReasons) {
output += ` - ${reason}\n`;
}
output += `\n Use --force to mark ready anyway.\n`;
return { output, exitCode: 1 };
}
}

// Convert from draft to ready via GraphQL mutation
// (GitHub REST API doesn't support undrafting — only GraphQL works)
try {
const { githubGraphQL } = await import('../lib/github.ts');
const prData = await githubApi<{ node_id: string }>(`/repos/${REPO}/pulls/${prNum}`);
await githubGraphQL(
`mutation($id: ID!) { markPullRequestReadyForReview(input: { pullRequestId: $id }) { pullRequest { isDraft } } }`,
{ id: prData.node_id },
);
} catch (e: unknown) {
const msg = e instanceof Error ? e.message : String(e);
return {
output: `${c.red}Failed to mark PR #${prNum} as ready: ${msg}${c.reset}\n`,
exitCode: 1,
};
}

let output = `${c.green}✓${c.reset} PR #${prNum} marked as ready for review.\n`;
if (force) output += ` ${c.yellow}(eligibility checks skipped with --force)${c.reset}\n`;

return { output, exitCode: 0 };
}

// ---------------------------------------------------------------------------
// Domain entry point (required by crux.mjs dispatch)
// ---------------------------------------------------------------------------

export const commands = {
create,
ready,
detect,
'fix-body': fixBody,
'rebase-all': rebaseAll,
Expand All @@ -620,7 +707,8 @@ export function getHelp(): string {
PR Domain — GitHub Pull Request utilities

Commands:
create Create a PR for the current branch (corruption-safe).
create Create a draft PR for the current branch (corruption-safe).
ready [--pr=N] Mark PR as ready (validates eligibility, removes draft status).
detect Detect open PR for current branch (returns URL + number).
fix-body [--pr=N] Detect and repair literal \\n in the current branch's PR body.
rebase-all Rebase all open non-draft PRs onto main (used by CI).
Expand All @@ -632,11 +720,15 @@ Options (create):
--body="..." PR body (inline — avoid for multi-line bodies; use --body-file or stdin).
--body-file=<path> PR body from file (safe for markdown with backticks).
--base=main Base branch (default: main).
--draft Create as draft PR.
--no-draft Create as ready PR (default: draft).
--allow-empty-body Allow creating PR without a description (not recommended).
--skip-test-plan Skip test plan validation (not recommended).
(stdin) If --body and --body-file are absent and stdin is a pipe, body is read from stdin.

Options (ready):
--pr=N Target a specific PR number instead of auto-detecting.
--force Skip eligibility checks and mark as ready anyway.

Options (detect):
--ci JSON output.

Expand Down
32 changes: 32 additions & 0 deletions crux/pr-patrol/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ function makePrNode(overrides: Partial<GqlPrNode> = {}): GqlPrNode {
title: 'Test PR',
headRefName: 'claude/test',
mergeable: 'MERGEABLE',
isDraft: false,
createdAt: '2026-01-01T00:00:00Z',
updatedAt: '2026-03-05T00:00:00Z',
body: '## Summary\n\n- [x] Task done\n\n## Test plan\n\n- [x] Tests pass\n\nCloses #1',
Expand Down Expand Up @@ -283,6 +284,20 @@ describe('checkMergeEligibility', () => {
expect(result.blockReasons).toContain('claude-working');
});

it('blocks when PR is a draft', () => {
const result = checkMergeEligibility(
makePrNode({ isDraft: true }),
);
expect(result.eligible).toBe(false);
expect(result.blockReasons).toContain('is-draft');
});

it('does NOT block when PR is not a draft', () => {
const result = checkMergeEligibility(makePrNode({ isDraft: false }));
expect(result.eligible).toBe(true);
expect(result.blockReasons).not.toContain('is-draft');
});

it('returns multiple block reasons when several checks fail', () => {
const result = checkMergeEligibility(
makePrNode({
Expand Down Expand Up @@ -349,6 +364,23 @@ describe('findMergeCandidates', () => {
expect(result.find((c) => c.number === 1)?.eligible).toBe(true);
expect(result.find((c) => c.number === 2)?.eligible).toBe(false);
});

it('marks draft PRs as blocked with is-draft reason', () => {
const draftPr = makePrNode({ number: 1, isDraft: true });
const result = findMergeCandidates([draftPr]);
expect(result).toHaveLength(1);
expect(result[0].eligible).toBe(false);
expect(result[0].blockReasons).toContain('is-draft');
});

it('draft PR with only is-draft block is undraft-eligible', () => {
const draftPr = makePrNode({ number: 1, isDraft: true });
const result = findMergeCandidates([draftPr]);
const undraftEligible = result.filter(
(c) => !c.eligible && c.blockReasons.length === 1 && c.blockReasons[0] === 'is-draft',
);
expect(undraftEligible).toHaveLength(1);
});
});

// ── detectIssues (regression test after refactor) ────────────────────────────
Expand Down
111 changes: 107 additions & 4 deletions crux/pr-patrol/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ export type MergeBlockReason =
| 'ci-pending'
| 'unresolved-threads'
| 'unchecked-items'
| 'claude-working';
| 'claude-working'
| 'is-draft';

export interface MergeCandidate {
number: number;
Expand Down Expand Up @@ -486,7 +487,7 @@ const PR_QUERY = `query($owner: String!, $name: String!) {
repository(owner: $owner, name: $name) {
pullRequests(first: 50, states: [OPEN], orderBy: {field: UPDATED_AT, direction: DESC}) {
nodes {
number title headRefName mergeable createdAt updatedAt body
number title headRefName mergeable isDraft createdAt updatedAt body
labels(first: 20) { nodes { name } }
commits(last: 1) { nodes { commit { statusCheckRollup {
contexts(first: 50) { nodes {
Expand Down Expand Up @@ -525,6 +526,7 @@ export interface GqlPrNode {
title: string;
headRefName: string;
mergeable: string;
isDraft: boolean;
createdAt: string;
updatedAt: string;
body: string | null;
Expand Down Expand Up @@ -625,6 +627,42 @@ export async function fetchOpenPrs(config: PatrolConfig): Promise<GqlPrNode[]> {
return prs;
}

const SINGLE_PR_QUERY = `query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
number title headRefName mergeable isDraft createdAt updatedAt body
labels(first: 20) { nodes { name } }
commits(last: 1) { nodes { commit { statusCheckRollup {
contexts(first: 50) { nodes {
... on CheckRun { conclusion }
... on StatusContext { state }
}}
}}}}
reviewThreads(first: 50) { nodes {
isResolved isOutdated path line startLine
comments(first: 3) { nodes {
author { login }
body
}}
}}
}
}
}`;

/** Fetch a single PR by number. Used by `crux pr ready` for eligibility checks. */
export async function fetchSinglePr(prNumber: number): Promise<GqlPrNode | null> {
const [owner, name] = REPO.split('/');
try {
const data = await githubGraphQL<{
repository: { pullRequest: GqlPrNode | null };
}>(SINGLE_PR_QUERY, { owner, name, number: prNumber });
return data.repository.pullRequest;
} catch (e) {
log(`Warning: could not fetch PR #${prNumber}: ${e instanceof Error ? e.message : String(e)}`);
return null;
}
}

function detectAllPrIssuesFromNodes(
prs: GqlPrNode[],
config: PatrolConfig,
Expand All @@ -635,6 +673,8 @@ function detectAllPrIssuesFromNodes(
.filter((pr) => {
const labels = pr.labels.nodes.map((l) => l.name);
if (labels.includes('claude-working')) return false;
// Skip draft PRs — they're not ready for automated fixes
if (pr.isDraft) return false;
return true;
})
.map((pr) => {
Expand Down Expand Up @@ -688,6 +728,10 @@ export function checkMergeEligibility(pr: GqlPrNode): MergeCandidate {
const blockReasons: MergeBlockReason[] = [];
const labels = pr.labels.nodes.map((l) => l.name);

if (pr.isDraft) {
blockReasons.push('is-draft');
}

if (labels.includes('claude-working')) {
blockReasons.push('claude-working');
}
Expand Down Expand Up @@ -759,6 +803,41 @@ export function findMergeCandidates(prs: GqlPrNode[]): MergeCandidate[] {
);
}

// ── Undraft execution ────────────────────────────────────────────────────────

async function undraftPr(prNum: number, config: PatrolConfig): Promise<boolean> {
log(`→ Undrafting PR #${prNum} (all eligibility checks pass)`);

try {
// GitHub REST API doesn't support undrafting — must use GraphQL mutation
const prData = await githubApi<{ node_id: string }>(
`/repos/${config.repo}/pulls/${prNum}`,
);
await githubGraphQL(
`mutation($id: ID!) { markPullRequestReadyForReview(input: { pullRequestId: $id }) { pullRequest { isDraft } } }`,
{ id: prData.node_id },
);

log(`✓ PR #${prNum} marked as ready for review`);
appendJsonl(JSONL_FILE, {
type: 'undraft_result',
pr_num: prNum,
outcome: 'undrafted',
});
return true;
} catch (e) {
const msg = e instanceof Error ? e.message : String(e);
log(`✗ Failed to undraft PR #${prNum}: ${msg}`);
appendJsonl(JSONL_FILE, {
type: 'undraft_result',
pr_num: prNum,
outcome: 'error',
reason: msg,
});
return false;
}
}

// ── Merge execution ─────────────────────────────────────────────────────────

async function mergePr(
Expand Down Expand Up @@ -1298,9 +1377,33 @@ async function runCheckCycle(
}
}

// ── Merge phase ────────────────────────────────────────────────────
// ── Undraft phase ──────────────────────────────────────────────────
// Auto-undraft draft PRs that are otherwise eligible for merge.
// A PR is auto-undrafted when its only block reason is 'is-draft'.

const mergeCandidates = findMergeCandidates(allPrs);
const draftCandidates = findMergeCandidates(allPrs).filter(
(c) => !c.eligible && c.blockReasons.length === 1 && c.blockReasons[0] === 'is-draft',
);

const undraftedNumbers = new Set<number>();
for (const candidate of draftCandidates) {
if (config.dryRun) {
log(` [DRY RUN] Would undraft PR #${candidate.number} (all other checks pass)`);
} else {
const success = await undraftPr(candidate.number, config);
if (success) undraftedNumbers.add(candidate.number);
}
}

// ── Merge phase ────────────────────────────────────────────────────
// Re-evaluate after undrafting (only successfully undrafted PRs become eligible)
const mergeCandidates = findMergeCandidates(allPrs).map((c) => {
if (undraftedNumbers.has(c.number)) {
const updated = { ...c, blockReasons: c.blockReasons.filter((r) => r !== 'is-draft') };
return { ...updated, eligible: updated.blockReasons.length === 0 };
}
return c;
});
const eligibleForMerge = mergeCandidates.filter((c) => c.eligible);
const blockedForMerge = mergeCandidates.filter((c) => !c.eligible);
let mergedPr: number | null = null;
Expand Down
Loading