diff --git a/packages/daemon/src/lib/space/runtime/built-in-validators/gh-lookup-helpers.ts b/packages/daemon/src/lib/space/runtime/built-in-validators/gh-lookup-helpers.ts new file mode 100644 index 000000000..21a3dcb76 --- /dev/null +++ b/packages/daemon/src/lib/space/runtime/built-in-validators/gh-lookup-helpers.ts @@ -0,0 +1,426 @@ +/** + * Shared GitHub-lookup helpers for built-in hook validators. + * + * Both the pre-review `pr_ready` validator and the post-approval `pr_merged` + * validator shell out to `gh` to inspect PR state and must classify GitHub + * rate-limit errors as retryable. These primitives — `gh` execution with + * rate-limit detection, a `/rate_limit` reset probe, a restricted GitHub-only + * env builder, and a current-branch PR-URL fallback — are extracted here so the + * two validators share one implementation instead of drifting. + */ + +import { collectWithMaxBuffer, parseJsonStdout } from '../gate-script-executor'; +import { + computeRateLimitRetryMs, + isRateLimitError, + isSecondaryRateLimitError, + RATE_LIMIT_MIN_BACKOFF_MS, +} from '../rate-limit-detector'; +import type { HookExecutorContext } from '../hook-executor'; +import type { WorkflowHookResult } from '@hyperneo/shared'; + +export const MAX_BUFFER_BYTES = 1_048_576; + +export const GITHUB_LOOKUP_ENV_KEYS = new Set([ + 'GH_TOKEN', + 'GITHUB_TOKEN', + 'GH_ENTERPRISE_TOKEN', + 'GITHUB_ENTERPRISE_TOKEN', + 'GH_HOST', + 'GH_REPO', + 'GH_CONFIG_DIR', +]); + +export const BASIC_ENV_KEYS = new Set([ + 'PATH', + 'HOME', + 'USER', + 'SHELL', + 'LANG', + 'TERM', + 'TMPDIR', + 'XDG_CONFIG_HOME', + 'AppData', + 'HTTPS_PROXY', + 'https_proxy', + 'HTTP_PROXY', + 'http_proxy', + 'ALL_PROXY', + 'all_proxy', + 'NO_PROXY', + 'no_proxy', + 'SSL_CERT_FILE', + 'SSL_CERT_DIR', + 'GIT_SSL_CAINFO', +]); + +/** + * Failure shape returned by `runCommand`. + * + * - `rateLimited: true` when stderr matched GitHub rate-limit patterns. The + * caller converts this into a `retryable_block` so the workflow engine backs + * off rather than re-running the validator on every action dispatch. + * - `retryAfterMs` is derived from a follow-up `gh api /rate_limit` probe + * (when reachable) and bounded by `RATE_LIMIT_MIN_BACKOFF_MS`. + */ +export type CommandFailure = { + success: false; + error: string; + rateLimited?: boolean; + retryAfterMs?: number; +}; +export type CommandSuccess = { success: true; data: T }; +export type CommandOutcome = CommandSuccess | CommandFailure; + +export interface RateLimitPayload { + resources?: { + core?: { reset?: number }; + graphql?: { reset?: number }; + }; +} + +export function remainingTimeoutMs(deadlineMs: number): number { + return Math.max(1, deadlineMs - Date.now()); +} + +export function buildGitHubLookupEnv(): Record { + const env: Record = {}; + for (const key of [...BASIC_ENV_KEYS, ...GITHUB_LOOKUP_ENV_KEYS]) { + const value = process.env[key]; + if (value !== undefined) env[key] = value; + } + return env; +} + +export function parseGitRemoteHost(remoteUrl: string): string | undefined { + const trimmed = remoteUrl.trim(); + if (!trimmed) return undefined; + try { + const url = new URL(trimmed); + return url.hostname || undefined; + } catch { + // scp-like SSH remote: git@github.example.com:owner/repo.git + const match = trimmed.match(/^[^@]+@([^:]+):/); + return match?.[1]; + } +} + +export async function inferGitHubHost( + cwd: string, + spawnImpl: typeof Bun.spawn, + deadlineMs: number +): Promise { + if (process.env.GH_HOST) return process.env.GH_HOST; + if (process.env.GH_REPO) { + const parts = process.env.GH_REPO.split('/'); + if (parts.length >= 3 && parts[0]) return parts[0]; + } + const originUrl = await runTextCommand( + ['git', 'config', '--get', 'remote.origin.url'], + cwd, + Math.min(remainingTimeoutMs(deadlineMs), 2_000), + spawnImpl + ); + if (!originUrl) return undefined; + return parseGitRemoteHost(originUrl); +} + +export async function runTextCommand( + args: string[], + cwd: string, + timeoutMs: number, + spawnImpl: typeof Bun.spawn +): Promise { + let proc; + try { + proc = spawnImpl(args, { + cwd, + env: buildGitHubLookupEnv(), + stdout: 'pipe', + stderr: 'pipe', + }); + } catch { + return undefined; + } + + const killTimer = setTimeout(() => { + try { + proc.kill('SIGKILL'); + } catch { + // ignore + } + }, timeoutMs); + + const [stdoutResult, exitCode] = await Promise.all([ + collectWithMaxBuffer(proc.stdout, MAX_BUFFER_BYTES), + proc.exited, + ]); + clearTimeout(killTimer); + if (exitCode !== 0) return undefined; + return stdoutResult.text.trim() || undefined; +} + +/** + * Picks the appropriate reset epoch from a `/rate_limit` payload. + * + * When `resource` is 'core' or 'graphql', returns that specific window's reset + * (or null if missing). When undefined, returns the earliest finite reset across + * both windows as a conservative fallback for cases where the caller doesn't + * know which resource was exhausted. + */ +export function pickRateLimitResetEpoch( + payload: RateLimitPayload, + resource?: 'core' | 'graphql' +): number | null { + if (resource === 'core') { + const coreReset = payload.resources?.core?.reset; + return typeof coreReset === 'number' && Number.isFinite(coreReset) ? coreReset : null; + } + if (resource === 'graphql') { + const graphqlReset = payload.resources?.graphql?.reset; + return typeof graphqlReset === 'number' && Number.isFinite(graphqlReset) ? graphqlReset : null; + } + // Fallback: pick the earliest available reset when resource is unknown. + const resets: number[] = []; + const coreReset = payload.resources?.core?.reset; + const graphqlReset = payload.resources?.graphql?.reset; + if (typeof coreReset === 'number' && Number.isFinite(coreReset)) { + resets.push(coreReset); + } + if (typeof graphqlReset === 'number' && Number.isFinite(graphqlReset)) { + resets.push(graphqlReset); + } + if (resets.length === 0) return null; + return Math.min(...resets); +} + +/** + * Probes `gh api /rate_limit` to discover the current window's reset epoch. + * + * Uses `runCommandRaw` (not `runCommand`) so a persistent rate limit does not + * cause infinite recursion. When `host` is provided (e.g. a GitHub Enterprise + * hostname from the rate-limited request), it is forwarded via `--hostname` so + * the probe queries the same host instead of `github.com`. + */ +export async function fetchRateLimitResetEpoch( + cwd: string, + spawnImpl: typeof Bun.spawn, + deadlineMs: number, + host?: string, + resource?: 'core' | 'graphql' +): Promise { + const args = ['gh', 'api']; + if (host) args.push('--hostname', host); + args.push('/rate_limit'); + const result = await runCommandRaw( + args, + cwd, + Math.min(remainingTimeoutMs(deadlineMs), 5_000), + spawnImpl + ); + if (!result.success) return null; + return pickRateLimitResetEpoch(result.data, resource); +} + +/** + * Spawns a `gh` command, captures stdout/stderr, and parses JSON stdout. + * + * This is the inner primitive — it does NOT interpret rate-limit errors. + * Callers that want rate-limit awareness should use `runCommand` instead. + */ +export async function runCommandRaw( + args: string[], + cwd: string, + timeoutMs: number, + spawnImpl: typeof Bun.spawn +): Promise> { + let proc; + try { + proc = spawnImpl(args, { + cwd, + env: buildGitHubLookupEnv(), + stdout: 'pipe', + stderr: 'pipe', + }); + } catch (err) { + return { success: false, error: err instanceof Error ? err.message : String(err) }; + } + + const killTimer = setTimeout(() => { + try { + proc.kill('SIGKILL'); + } catch { + // ignore + } + }, timeoutMs); + + const [stdoutResult, stderrResult, exitCode] = await Promise.all([ + collectWithMaxBuffer(proc.stdout, MAX_BUFFER_BYTES), + collectWithMaxBuffer(proc.stderr, MAX_BUFFER_BYTES), + proc.exited, + ]); + + clearTimeout(killTimer); + + if (exitCode !== 0) { + return { success: false, error: stderrResult.text.trim() || `gh exited with code ${exitCode}` }; + } + + const parsed = parseJsonStdout(stdoutResult.text); + if (!parsed) { + return { success: false, error: 'gh produced empty or non-JSON stdout' }; + } + + return { success: true, data: parsed as T }; +} + +/** + * Runs a `gh` command, classifying non-zero exits as rate-limited when the + * stderr matches GitHub's rate-limit error patterns. + * + * On rate-limit detection, performs a follow-up `gh api /rate_limit` probe to + * compute `retryAfterMs` (bounded by `RATE_LIMIT_MIN_BACKOFF_MS`). + * `options.hostHint` is forwarded to the probe so an Enterprise rate-limit is + * measured against the right host. All other failures pass through unchanged. + */ +export async function runCommand( + args: string[], + cwd: string, + timeoutMs: number, + spawnImpl: typeof Bun.spawn, + options?: { hostHint?: string; resourceHint?: 'core' | 'graphql' } +): Promise> { + const outcome = await runCommandRaw(args, cwd, timeoutMs, spawnImpl); + if (outcome.success) return outcome; + if (!isRateLimitError(outcome.error)) return outcome; + // Secondary rate limits don't update /rate_limit — skip the probe and use minimum backoff. + if (isSecondaryRateLimitError(outcome.error)) { + return { + success: false, + error: outcome.error, + rateLimited: true, + retryAfterMs: RATE_LIMIT_MIN_BACKOFF_MS, + }; + } + const resetEpoch = await fetchRateLimitResetEpoch( + cwd, + spawnImpl, + Date.now() + timeoutMs, + options?.hostHint, + options?.resourceHint + ); + return { + success: false, + error: outcome.error, + rateLimited: true, + retryAfterMs: computeRateLimitRetryMs(resetEpoch), + }; +} + +/** + * Wraps a failed `runCommand` outcome into the right `WorkflowHookResult`. + * + * Rate-limited failures become `retryable_block` so the workflow engine defers + * the next attempt past the reset window. All other failures pass through as + * `block` with the original `prefix` framing. + */ +export function commandFailureToHookResult( + failure: CommandFailure, + prefix: string +): WorkflowHookResult { + if (failure.rateLimited) { + return { + type: 'retryable_block', + reason: `${prefix}: GitHub rate limited — ${failure.error}`, + retryAfterMs: failure.retryAfterMs ?? RATE_LIMIT_MIN_BACKOFF_MS, + }; + } + return { type: 'block', reason: `${prefix}: ${failure.error}` }; +} + +/** Extracts a `pr_url` string from a params/rawParams `data` record. */ +export function extractPrUrlFromParams(params: Record): string | undefined { + const data = params.data; + if ( + typeof data === 'object' && + data !== null && + typeof (data as Record).pr_url === 'string' + ) { + return (data as Record).pr_url as string; + } + return undefined; +} + +/** Extracts a `pr_url` string from a hook definition's bounded `templateData`. */ +export function extractTemplatePrUrl(context: HookExecutorContext): string | undefined { + const templateData = context.templateData; + if ( + typeof templateData === 'object' && + templateData !== null && + typeof templateData.pr_url === 'string' + ) { + return templateData.pr_url as string; + } + return undefined; +} + +/** + * Extracts the most recent PR URL from the run's artifacts. + * + * `currentArtifacts` is sorted most-recent-first by the hook engine, so the + * first hit wins. Mirrors the artifact scan in + * `SpaceRuntimeService.dispatchPostApproval`, accepting both camelCase `prUrl` + * and snake_case `pr_url`. + */ +export function extractPrUrlFromArtifacts( + artifacts: HookExecutorContext['currentArtifacts'] +): string | undefined { + for (const artifact of artifacts) { + const data = artifact.data; + if (!data || typeof data !== 'object' || Array.isArray(data)) continue; + const record = data as Record; + const candidate = + (typeof record.prUrl === 'string' && record.prUrl) || + (typeof record.pr_url === 'string' && record.pr_url); + if (candidate) return candidate; + } + return undefined; +} + +/** + * Resolves the PR URL for the current branch via `gh pr view --json url`. + * + * Shared last-resort fallback used by validators that have no explicit PR URL + * in their params. The `graphql` resource window backs the CLI's PR finder. + */ +export async function resolveCurrentBranchPrUrl( + context: HookExecutorContext, + spawnImpl: typeof Bun.spawn, + deadlineMs: number +): Promise< + | { success: true; prUrl: string } + | ({ success: false; error: string } & Pick) +> { + const currentBranchPr = await runCommand<{ url?: string }>( + ['gh', 'pr', 'view', '--json', 'url'], + context.workspacePath, + remainingTimeoutMs(deadlineMs), + spawnImpl, + { + resourceHint: 'graphql', + hostHint: await inferGitHubHost(context.workspacePath, spawnImpl, deadlineMs), + } + ); + if (!currentBranchPr.success) { + return { + success: false, + error: `current-branch PR discovery failed: ${currentBranchPr.error}`, + rateLimited: currentBranchPr.rateLimited, + retryAfterMs: currentBranchPr.retryAfterMs, + }; + } + if (typeof currentBranchPr.data.url !== 'string' || currentBranchPr.data.url.length === 0) { + return { success: false, error: 'current-branch PR discovery returned no URL' }; + } + return { success: true, prUrl: currentBranchPr.data.url }; +} diff --git a/packages/daemon/src/lib/space/runtime/built-in-validators/pr-merged-validator.ts b/packages/daemon/src/lib/space/runtime/built-in-validators/pr-merged-validator.ts new file mode 100644 index 000000000..10cb9ce2c --- /dev/null +++ b/packages/daemon/src/lib/space/runtime/built-in-validators/pr-merged-validator.ts @@ -0,0 +1,186 @@ +/** + * PR Merged Built-in Validator + * + * The symmetric counterpart to `pr_ready`. `pr_ready` blocks the + * coder→reviewer handoff until the PR is *ready* (open + mergeable + clean). + * `pr_merged` blocks the post-approval reviewer's `mark_complete` + * (`approved → done`) until the PR is actually *merged* — i.e. until the + * delivery to the base branch is confirmed. + * + * This enforces the merge-conflict routing contract deterministically: a task + * can never be marked complete for an unmerged PR. When the PR has unresolved + * merge conflicts, the block remediation directs the reviewer to route the + * conflict back to the coder (the upstream implementation node) rather than + * escalating routine coder-owned work to a human. + */ + +import type { WorkflowHookResult } from '@hyperneo/shared'; +import type { HookExecutorContext } from '../hook-executor'; +import { parsePrUrl } from '../parse-pr-url'; +import { + commandFailureToHookResult, + extractPrUrlFromArtifacts, + extractPrUrlFromParams, + extractTemplatePrUrl, + remainingTimeoutMs, + resolveCurrentBranchPrUrl, + runCommand, + type CommandFailure, +} from './gh-lookup-helpers'; + +const DEFAULT_TIMEOUT_MS = 30_000; + +interface PrMergedView { + url: string; + state: string; + mergeable: string; + mergeStateStatus: string; +} + +const BLOCK_PREFIX = 'PR is not merged'; + +/** States that indicate the PR head conflicts with the base branch. */ +const CONFLICT_MERGE_STATE = new Set(['DIRTY']); +const CONFLICT_MERGEABLE = new Set(['CONFLICTING']); + +export function createPrMergedValidator( + spawnImpl: typeof Bun.spawn = Bun.spawn +): (context: HookExecutorContext) => Promise { + return async (context: HookExecutorContext): Promise => { + const deadlineMs = Date.now() + DEFAULT_TIMEOUT_MS; + const prUrlResult = await resolvePrUrl(context, spawnImpl, deadlineMs); + if (!prUrlResult.success) { + return commandFailureToHookResult(prUrlResult, BLOCK_PREFIX); + } + const prUrl = prUrlResult.prUrl; + + const prMeta = parsePrUrl(prUrl); + if (!prMeta) { + return { + type: 'block', + reason: `${BLOCK_PREFIX}: unable to parse GitHub PR URL: ${prUrl}`, + }; + } + + // gh pr view fields are GraphQL PullRequest fields in the GitHub CLI, so a + // rate-limit probe uses the `graphql` resource window rather than REST `core`. + const prView = await runCommand( + ['gh', 'pr', 'view', prUrl, '--json', 'url,state,mergeable,mergeStateStatus'], + context.workspacePath, + remainingTimeoutMs(deadlineMs), + spawnImpl, + { hostHint: prMeta.host, resourceHint: 'graphql' } + ); + if (!prView.success) { + return commandFailureToHookResult(prView, BLOCK_PREFIX); + } + + const prJson = prView.data; + + if (prJson.state === 'MERGED') { + return { + type: 'allow', + data: { pr_url: prJson.url, merged: true }, + }; + } + + // A CLOSED PR is a definitive terminal state — it will never be merged via + // this PR, so block regardless of the (possibly transient UNKNOWN) mergeable + // field. CLOSED must be checked before UNKNOWN, otherwise a closed PR whose + // mergeability GitHub hasn't finished computing would retryable-block and + // loop on the 30s backoff instead of failing fast. + if (prJson.state === 'CLOSED') { + return { + type: 'block', + reason: `${BLOCK_PREFIX}: PR is CLOSED without being merged — investigate why the PR was closed instead of merged before completing the task.`, + }; + } + + // GitHub computes mergeability asynchronously; a transient UNKNOWN right + // after a push/merge attempt should back off, not hard-block. + if (prJson.mergeable === 'UNKNOWN') { + return { + type: 'retryable_block', + reason: 'Waiting for GitHub mergeability/checks', + retryAfterMs: 30_000, + }; + } + + const hasConflict = + CONFLICT_MERGEABLE.has(prJson.mergeable) || CONFLICT_MERGE_STATE.has(prJson.mergeStateStatus); + if (hasConflict) { + return { + type: 'block', + reason: + `${BLOCK_PREFIX}: PR has unresolved merge conflicts ` + + `(mergeable: ${prJson.mergeable ?? 'unknown'}, mergeStateStatus: ${prJson.mergeStateStatus ?? 'unknown'}). ` + + 'Do NOT mark the task complete and do NOT escalate a routine merge conflict to a human — ' + + 'merge conflicts are coder-owned work. Route the conflict back to the upstream implementation ' + + 'node (Coding in a Coding workflow, Research in a Research workflow) with the conflicting files ' + + 'and retry instructions: rebase onto the latest base branch, resolve the listed conflicts, run ' + + 'the affected tests, then push to update the PR branch, and report back to Review for a re-merge.', + }; + } + + // Open, not merged, no conflict — the reviewer has not run the merge yet. + return { + type: 'block', + reason: + `${BLOCK_PREFIX}: PR state is ${prJson.state ?? 'unknown'} ` + + `(mergeStateStatus: ${prJson.mergeStateStatus ?? 'unknown'}). ` + + 'Merge the PR (gh pr merge --squash) before marking the task complete.', + }; + }; +} + +/** + * Resolves the PR URL to verify. `mark_complete` carries no `pr_url` param, so + * unlike `pr_ready` this falls back to the run's artifacts (where the reviewer + * records the PR URL before approval) before the current branch. + * + * Single-PR assumption: this reads the most-recent `pr_url`/`prUrl` artifact. + * `dispatchPostApproval` resolves the merge kickoff's `{{pr_url}}` from the + * last-*created* artifact, while the hook engine supplies artifacts ordered by + * `updatedAt`. For the standard append-based per-cycle flow — each review cycle + * creates a fresh result artifact via `append: true` and never upserts it — + * `createdAt` and `updatedAt` move together, so both orderings select the same + * (latest-cycle) PR, and the URL validated here is the one the reviewer was told + * to merge. The step-6 merge audit artifact uses `merged_pr_url` (not `pr_url`), + * so it is skipped. Only a run carrying multiple distinct `pr_url` values with + * out-of-order updates could make the two selections diverge; that is not a + * supported configuration for these workflows. + */ +async function resolvePrUrl( + context: HookExecutorContext, + spawnImpl: typeof Bun.spawn, + deadlineMs: number +): Promise< + | { success: true; prUrl: string } + | ({ success: false; error: string } & Pick) +> { + const boundedPrUrl = extractPrUrlFromParams(context.params); + if (boundedPrUrl) return { success: true, prUrl: boundedPrUrl }; + + const rawPrUrl = context.rawParams ? extractPrUrlFromParams(context.rawParams) : undefined; + if (rawPrUrl) return { success: true, prUrl: rawPrUrl }; + + const templatePrUrl = extractTemplatePrUrl(context); + if (templatePrUrl) return { success: true, prUrl: templatePrUrl }; + + const artifactPrUrl = extractPrUrlFromArtifacts(context.currentArtifacts); + if (artifactPrUrl) return { success: true, prUrl: artifactPrUrl }; + + // Last resort: the current branch's PR. Fail-closed if nothing is resolvable + // — a Review-node mark_complete should always have a PR, so an unresolvable + // URL surfaces a real problem rather than silently completing the task. + const fallback = await resolveCurrentBranchPrUrl(context, spawnImpl, deadlineMs); + if (!fallback.success) { + return { + success: false, + error: `no PR URL resolved to verify merge (${fallback.error})`, + rateLimited: fallback.rateLimited, + retryAfterMs: fallback.retryAfterMs, + }; + } + return { success: true, prUrl: fallback.prUrl }; +} diff --git a/packages/daemon/src/lib/space/runtime/built-in-validators/pr-ready-validator.ts b/packages/daemon/src/lib/space/runtime/built-in-validators/pr-ready-validator.ts index c18f47813..a85771a68 100644 --- a/packages/daemon/src/lib/space/runtime/built-in-validators/pr-ready-validator.ts +++ b/packages/daemon/src/lib/space/runtime/built-in-validators/pr-ready-validator.ts @@ -8,7 +8,6 @@ import type { WorkflowHookResult } from '@hyperneo/shared'; import type { HookExecutorContext } from '../hook-executor'; -import { collectWithMaxBuffer, parseJsonStdout } from '../gate-script-executor'; import { parsePrUrl } from '../parse-pr-url'; import { computeRateLimitRetryMs, @@ -16,41 +15,18 @@ import { isSecondaryRateLimitError, RATE_LIMIT_MIN_BACKOFF_MS, } from '../rate-limit-detector'; +import { + commandFailureToHookResult, + extractPrUrlFromParams, + extractTemplatePrUrl, + fetchRateLimitResetEpoch, + remainingTimeoutMs, + resolveCurrentBranchPrUrl, + runCommand, + type CommandFailure, +} from './gh-lookup-helpers'; const DEFAULT_TIMEOUT_MS = 30_000; -const MAX_BUFFER_BYTES = 1_048_576; - -const GITHUB_LOOKUP_ENV_KEYS = new Set([ - 'GH_TOKEN', - 'GITHUB_TOKEN', - 'GH_ENTERPRISE_TOKEN', - 'GITHUB_ENTERPRISE_TOKEN', - 'GH_HOST', - 'GH_REPO', - 'GH_CONFIG_DIR', -]); -const BASIC_ENV_KEYS = new Set([ - 'PATH', - 'HOME', - 'USER', - 'SHELL', - 'LANG', - 'TERM', - 'TMPDIR', - 'XDG_CONFIG_HOME', - 'AppData', - 'HTTPS_PROXY', - 'https_proxy', - 'HTTP_PROXY', - 'http_proxy', - 'ALL_PROXY', - 'all_proxy', - 'NO_PROXY', - 'no_proxy', - 'SSL_CERT_FILE', - 'SSL_CERT_DIR', - 'GIT_SSL_CAINFO', -]); interface PrViewResult { url: string; @@ -211,35 +187,19 @@ async function resolvePrUrl( const templatePrUrl = extractTemplatePrUrl(context); if (templatePrUrl) return { success: true, prUrl: templatePrUrl, shouldPatchPrUrl: true }; - // Run gh pr view for current branch. The URL field is resolved via GitHub - // CLI's GraphQL PR finder, so a rate-limit probe uses the `graphql` resource - // window rather than REST `core`. When GH_HOST points to an Enterprise host, - // forward it so the probe queries the same host. - const currentBranchPr = await runCommand<{ url?: string }>( - ['gh', 'pr', 'view', '--json', 'url'], - context.workspacePath, - remainingTimeoutMs(deadlineMs), - spawnImpl, - { - resourceHint: 'graphql', - hostHint: await inferGitHubHost(context.workspacePath, spawnImpl, deadlineMs), - } - ); - if (!currentBranchPr.success) { - return { - success: false, - error: `no PR URL provided and current-branch PR discovery failed: ${currentBranchPr.error}`, - rateLimited: currentBranchPr.rateLimited, - retryAfterMs: currentBranchPr.retryAfterMs, - }; - } - if (typeof currentBranchPr.data.url !== 'string' || currentBranchPr.data.url.length === 0) { + // Fall back to the current branch's PR. When GH_HOST points to an Enterprise + // host, the shared resolver forwards it so a rate-limit probe queries the + // same host. + const fallback = await resolveCurrentBranchPrUrl(context, spawnImpl, deadlineMs); + if (!fallback.success) { return { success: false, - error: 'no PR URL provided and current-branch PR discovery returned no URL', + error: `no PR URL provided and ${fallback.error}`, + rateLimited: fallback.rateLimited, + retryAfterMs: fallback.retryAfterMs, }; } - return { success: true, prUrl: currentBranchPr.data.url, shouldPatchPrUrl: true }; + return { success: true, prUrl: fallback.prUrl, shouldPatchPrUrl: true }; } function extractDataRecord(context: HookExecutorContext): Record { @@ -247,98 +207,6 @@ function extractDataRecord(context: HookExecutorContext): Record { - if (process.env.GH_HOST) return process.env.GH_HOST; - if (process.env.GH_REPO) { - const parts = process.env.GH_REPO.split('/'); - if (parts.length >= 3 && parts[0]) return parts[0]; - } - const originUrl = await runTextCommand( - ['git', 'config', '--get', 'remote.origin.url'], - cwd, - Math.min(remainingTimeoutMs(deadlineMs), 2_000), - spawnImpl - ); - if (!originUrl) return undefined; - return parseGitRemoteHost(originUrl); -} - -function parseGitRemoteHost(remoteUrl: string): string | undefined { - const trimmed = remoteUrl.trim(); - if (!trimmed) return undefined; - try { - const url = new URL(trimmed); - return url.hostname || undefined; - } catch { - // scp-like SSH remote: git@github.example.com:owner/repo.git - const match = trimmed.match(/^[^@]+@([^:]+):/); - return match?.[1]; - } -} - -async function runTextCommand( - args: string[], - cwd: string, - timeoutMs: number, - spawnImpl: typeof Bun.spawn -): Promise { - let proc; - try { - proc = spawnImpl(args, { - cwd, - env: buildGitHubLookupEnv(), - stdout: 'pipe', - stderr: 'pipe', - }); - } catch { - return undefined; - } - - const killTimer = setTimeout(() => { - try { - proc.kill('SIGKILL'); - } catch { - // ignore - } - }, timeoutMs); - - const [stdoutResult, exitCode] = await Promise.all([ - collectWithMaxBuffer(proc.stdout, MAX_BUFFER_BYTES), - proc.exited, - ]); - clearTimeout(killTimer); - if (exitCode !== 0) return undefined; - return stdoutResult.text.trim() || undefined; -} - -function extractTemplatePrUrl(context: HookExecutorContext): string | undefined { - const templateData = context.templateData; - if ( - typeof templateData === 'object' && - templateData !== null && - typeof templateData.pr_url === 'string' - ) { - return templateData.pr_url; - } - return undefined; -} - -function extractPrUrlFromParams(params: Record): string | undefined { - const data = params.data; - if ( - typeof data === 'object' && - data !== null && - typeof (data as Record).pr_url === 'string' - ) { - return (data as Record).pr_url as string; - } - return undefined; -} - async function runReviewThreadsQuery( meta: { host: string; owner: string; repo: string; number: string }, cwd: string, @@ -451,221 +319,3 @@ async function runReviewThreadsQuery( return { success: true, unresolvedUrls }; } - -function remainingTimeoutMs(deadlineMs: number): number { - return Math.max(1, deadlineMs - Date.now()); -} - -function buildGitHubLookupEnv(): Record { - const env: Record = {}; - for (const key of [...BASIC_ENV_KEYS, ...GITHUB_LOOKUP_ENV_KEYS]) { - const value = process.env[key]; - if (value !== undefined) env[key] = value; - } - return env; -} - -/** - * Failure shape returned by `runCommand`. - * - * - `rateLimited: true` when stderr matched GitHub rate-limit patterns. The - * caller converts this into a `retryable_block` so the workflow engine - * backs off rather than re-running the validator on every action dispatch. - * - `retryAfterMs` is derived from a follow-up `gh api /rate_limit` probe - * (when reachable) and bounded by `RATE_LIMIT_MIN_BACKOFF_MS`. - */ -type CommandFailure = { - success: false; - error: string; - rateLimited?: boolean; - retryAfterMs?: number; -}; -type CommandSuccess = { success: true; data: T }; -type CommandOutcome = CommandSuccess | CommandFailure; - -interface RateLimitPayload { - resources?: { - core?: { reset?: number }; - graphql?: { reset?: number }; - }; -} - -/** - * Picks the appropriate reset epoch from a `/rate_limit` payload. - * - * When `resource` is 'core' or 'graphql', returns that specific window's reset - * (or null if missing). When undefined, returns the earliest finite reset across - * both windows as a conservative fallback for cases where the caller doesn't - * know which resource was exhausted. - */ -function pickRateLimitResetEpoch( - payload: RateLimitPayload, - resource?: 'core' | 'graphql' -): number | null { - if (resource === 'core') { - const coreReset = payload.resources?.core?.reset; - return typeof coreReset === 'number' && Number.isFinite(coreReset) ? coreReset : null; - } - if (resource === 'graphql') { - const graphqlReset = payload.resources?.graphql?.reset; - return typeof graphqlReset === 'number' && Number.isFinite(graphqlReset) ? graphqlReset : null; - } - // Fallback: pick the earliest available reset when resource is unknown. - const resets: number[] = []; - const coreReset = payload.resources?.core?.reset; - const graphqlReset = payload.resources?.graphql?.reset; - if (typeof coreReset === 'number' && Number.isFinite(coreReset)) { - resets.push(coreReset); - } - if (typeof graphqlReset === 'number' && Number.isFinite(graphqlReset)) { - resets.push(graphqlReset); - } - if (resets.length === 0) return null; - return Math.min(...resets); -} - -/** - * Probes `gh api /rate_limit` to discover the current window's reset epoch. - * - * Uses `runCommandRaw` (not `runCommand`) so a persistent rate limit does - * not cause infinite recursion. When `host` is provided (e.g. a GitHub - * Enterprise hostname from the rate-limited request), it is forwarded via - * `--hostname` so the probe queries the same host instead of `github.com`. - * When `resource` is 'core' or 'graphql', returns that specific window's - * reset; otherwise returns the earliest available reset as a fallback. - * Returns null when the probe itself fails — callers fall back to - * `RATE_LIMIT_MIN_BACKOFF_MS`. - */ -async function fetchRateLimitResetEpoch( - cwd: string, - spawnImpl: typeof Bun.spawn, - deadlineMs: number, - host?: string, - resource?: 'core' | 'graphql' -): Promise { - const args = ['gh', 'api']; - if (host) args.push('--hostname', host); - args.push('/rate_limit'); - const result = await runCommandRaw( - args, - cwd, - Math.min(remainingTimeoutMs(deadlineMs), 5_000), - spawnImpl - ); - if (!result.success) return null; - return pickRateLimitResetEpoch(result.data, resource); -} - -/** - * Spawns a `gh` command, captures stdout/stderr, and parses JSON stdout. - * - * This is the inner primitive — it does NOT interpret rate-limit errors. - * Callers that want rate-limit awareness should use `runCommand` instead. - */ -async function runCommandRaw( - args: string[], - cwd: string, - timeoutMs: number, - spawnImpl: typeof Bun.spawn -): Promise> { - let proc; - try { - proc = spawnImpl(args, { - cwd, - env: buildGitHubLookupEnv(), - stdout: 'pipe', - stderr: 'pipe', - }); - } catch (err) { - return { success: false, error: err instanceof Error ? err.message : String(err) }; - } - - const killTimer = setTimeout(() => { - try { - proc.kill('SIGKILL'); - } catch { - // ignore - } - }, timeoutMs); - - const [stdoutResult, stderrResult, exitCode] = await Promise.all([ - collectWithMaxBuffer(proc.stdout, MAX_BUFFER_BYTES), - collectWithMaxBuffer(proc.stderr, MAX_BUFFER_BYTES), - proc.exited, - ]); - - clearTimeout(killTimer); - - if (exitCode !== 0) { - return { success: false, error: stderrResult.text.trim() || `gh exited with code ${exitCode}` }; - } - - const parsed = parseJsonStdout(stdoutResult.text); - if (!parsed) { - return { success: false, error: 'gh produced empty or non-JSON stdout' }; - } - - return { success: true, data: parsed as T }; -} - -/** - * Runs a `gh` command, classifying non-zero exits as rate-limited when the - * stderr matches GitHub's rate-limit error patterns. - * - * On rate-limit detection, performs a follow-up `gh api /rate_limit` probe - * to compute `retryAfterMs` (bounded by `RATE_LIMIT_MIN_BACKOFF_MS`). - * `options.hostHint` is forwarded to the probe so an Enterprise rate-limit - * is measured against the right host. All other failures pass through - * unchanged so existing block-path behavior is preserved. - */ -async function runCommand( - args: string[], - cwd: string, - timeoutMs: number, - spawnImpl: typeof Bun.spawn, - options?: { hostHint?: string; resourceHint?: 'core' | 'graphql' } -): Promise> { - const outcome = await runCommandRaw(args, cwd, timeoutMs, spawnImpl); - if (outcome.success) return outcome; - if (!isRateLimitError(outcome.error)) return outcome; - // Secondary rate limits don't update /rate_limit — skip the probe and use minimum backoff. - if (isSecondaryRateLimitError(outcome.error)) { - return { - success: false, - error: outcome.error, - rateLimited: true, - retryAfterMs: RATE_LIMIT_MIN_BACKOFF_MS, - }; - } - const resetEpoch = await fetchRateLimitResetEpoch( - cwd, - spawnImpl, - Date.now() + timeoutMs, - options?.hostHint, - options?.resourceHint - ); - return { - success: false, - error: outcome.error, - rateLimited: true, - retryAfterMs: computeRateLimitRetryMs(resetEpoch), - }; -} - -/** - * Wraps a failed `runCommand` outcome into the right `WorkflowHookResult`. - * - * Rate-limited failures become `retryable_block` so the workflow engine - * defers the next attempt past the reset window. All other failures pass - * through as `block` with the original `prefix` framing. - */ -function commandFailureToHookResult(failure: CommandFailure, prefix: string): WorkflowHookResult { - if (failure.rateLimited) { - return { - type: 'retryable_block', - reason: `${prefix}: GitHub rate limited — ${failure.error}`, - retryAfterMs: failure.retryAfterMs ?? RATE_LIMIT_MIN_BACKOFF_MS, - }; - } - return { type: 'block', reason: `${prefix}: ${failure.error}` }; -} diff --git a/packages/daemon/src/lib/space/runtime/hook-executor.ts b/packages/daemon/src/lib/space/runtime/hook-executor.ts index 78cf2b72d..1851793a1 100644 --- a/packages/daemon/src/lib/space/runtime/hook-executor.ts +++ b/packages/daemon/src/lib/space/runtime/hook-executor.ts @@ -23,6 +23,7 @@ import { homedir, tmpdir } from 'os'; import { join } from 'path'; import { validateWorkflowHookResult } from '../workflow-hook-validation'; import { createPrReadyValidator } from './built-in-validators/pr-ready-validator'; +import { createPrMergedValidator } from './built-in-validators/pr-merged-validator'; // --------------------------------------------------------------------------- // Public types @@ -178,6 +179,7 @@ registerBuiltInValidator('task_reported_status', async () => ({ reason: NOT_IMPLEMENTED, })); registerBuiltInValidator('pr_ready', createPrReadyValidator()); +registerBuiltInValidator('pr_merged', createPrMergedValidator()); // --------------------------------------------------------------------------- // Environment builder diff --git a/packages/daemon/src/lib/space/runtime/post-approval-router.ts b/packages/daemon/src/lib/space/runtime/post-approval-router.ts index 0e3f40fc9..de90f7ae2 100644 --- a/packages/daemon/src/lib/space/runtime/post-approval-router.ts +++ b/packages/daemon/src/lib/space/runtime/post-approval-router.ts @@ -195,19 +195,62 @@ export type PostApprovalRouteResult = // Event shapes (§2.3) // --------------------------------------------------------------------------- -const POST_APPROVAL_COMPLETION_INSTRUCTIONS = +const POST_APPROVAL_COMPLETION_HEAD = `When the post-approval work is finished, call mark_complete to transition the\n` + - `task from \`approved\` to \`done\`. If you are blocked and cannot complete the\n` + - `work, do NOT call mark_complete — the post-approval node-agent surface has no\n` + - `request-human tool, so surface the blocker via send_message(target="space-agent")\n` + - `and save a NON-result artifact describing the block (e.g. type:"blocked"). A\n` + - `"result" artifact would be picked up as the task result on a later mark_complete,\n` + - `poisoning completion. Then stop.\n\n` + + `task from \`approved\` to \`done\`.`; + +// Appended only for PR-merge routes — workflows that declare a `pr_merged` +// mark_complete hook. Non-PR routes (deployers, custom agents, etc.) have +// nothing to merge and no pr_merged hook, so the merge guidance must not appear +// for them. See `appendPostApprovalCompletionInstructions`. +const POST_APPROVAL_PR_MERGE_COMPLETION_INSTRUCTIONS = + `A mark_complete hook (pr_merged) verifies the PR is actually merged before\n` + + `allowing completion — it BLOCKS if the PR is not merged, and on a merge\n` + + `conflict it tells you to route the conflict back to the coder. So only call\n` + + `mark_complete after a successful merge; if you hit a block, follow its\n` + + `remediation rather than retrying mark_complete.`; + +const POST_APPROVAL_COMPLETION_TAIL = + `If you are blocked and cannot complete the work, do NOT call mark_complete — the\n` + + `post-approval node-agent surface has no request-human tool, so surface the\n` + + `blocker via send_message(target="space-agent") and save a NON-result artifact\n` + + `describing the block (e.g. type:"blocked"). A "result" artifact would be picked\n` + + `up as the task result on a later mark_complete, poisoning completion. Then stop.\n\n` + `Do NOT call approve_task; the task has already been approved upstream.`; -export function appendPostApprovalCompletionInstructions(interpolatedInstructions: string): string { +/** + * Appends the runtime-owned completion instructions shared by every post-approval + * route. The generic guidance (call mark_complete when done, surface blockers via + * space-agent, do not call approve_task) always applies. The `pr_merged` merge + * guidance is appended only when `hasPrMergedHook` is true — i.e. the workflow + * declares a `pr_merged` mark_complete hook, so the spawned session is gated on a + * real merge. Non-PR routes pass false and get no merge guidance. + */ +export function appendPostApprovalCompletionInstructions( + interpolatedInstructions: string, + hasPrMergedHook = false +): string { const trimmed = interpolatedInstructions.trim(); - return `${trimmed}\n\n${POST_APPROVAL_COMPLETION_INSTRUCTIONS}`; + const mergePart = hasPrMergedHook ? `\n\n${POST_APPROVAL_PR_MERGE_COMPLETION_INSTRUCTIONS}` : ''; + return `${trimmed}\n\n${POST_APPROVAL_COMPLETION_HEAD}${mergePart}\n\n${POST_APPROVAL_COMPLETION_TAIL}`; +} + +/** + * Whether the workflow declares a `pr_merged` mark_complete hook — the signal + * that a post-approval route is PR-merge-based and the spawned session will be + * gated on a real merge. Used to scope the merge-specific completion guidance to + * PR routes only, so non-PR routes (deployers, custom agents) don't read it. + */ +function workflowHasPrMergedHook(workflow: SpaceWorkflow | null): boolean { + return ( + workflow?.hooks?.some( + (h) => + h.enabled !== false && + h.method === 'mark_complete' && + h.validator.kind === 'built_in' && + h.validator.id === 'pr_merged' + ) ?? false + ); } function resolvePostApprovalRoute( @@ -372,7 +415,10 @@ export class PostApprovalRouter { clearPendingCompletionState(this.deps.taskRepo, task.id); return { mode: 'skipped', reason }; } - const kickoffMessage = appendPostApprovalCompletionInstructions(interpolatedInstructions); + const kickoffMessage = appendPostApprovalCompletionInstructions( + interpolatedInstructions, + workflowHasPrMergedHook(workflow) + ); const startedAt = Date.now(); const { sessionId } = await this.deps.spawner.spawnPostApprovalSubSession({ diff --git a/packages/daemon/src/lib/space/runtime/workflow-hook-engine.ts b/packages/daemon/src/lib/space/runtime/workflow-hook-engine.ts index 885b17373..45a23167d 100644 --- a/packages/daemon/src/lib/space/runtime/workflow-hook-engine.ts +++ b/packages/daemon/src/lib/space/runtime/workflow-hook-engine.ts @@ -904,7 +904,7 @@ export class WorkflowHookEngine { const permittedExternalLookups: string[] = hook.validator.kind === 'script' ? (hook.validator.externalLookups ?? []) - : hook.validator.id === 'pr_ready' + : hook.validator.id === 'pr_ready' || hook.validator.id === 'pr_merged' ? ['github'] : []; diff --git a/packages/daemon/src/lib/space/workflow-hook-validation.ts b/packages/daemon/src/lib/space/workflow-hook-validation.ts index 733e4ac96..e8a4650d2 100644 --- a/packages/daemon/src/lib/space/workflow-hook-validation.ts +++ b/packages/daemon/src/lib/space/workflow-hook-validation.ts @@ -13,9 +13,10 @@ const VALID_METHODS = new Set([ 'submit_for_approval', 'approve_task', ]); -// Built-in validators are not yet implemented; reject them at validation time -// so workflows cannot declare IDs that would unconditionally block at runtime. -const VALID_BUILT_IN_VALIDATORS = new Set(['pr_ready']); +// Implemented built-in validators. IDs absent from this set are rejected at +// validation time so workflows cannot declare validators that would +// unconditionally block at runtime. +const VALID_BUILT_IN_VALIDATORS = new Set(['pr_ready', 'pr_merged']); const VALID_RESULT_TYPES = new Set([ 'allow', 'block', diff --git a/packages/daemon/src/lib/space/workflows/built-in-workflows.ts b/packages/daemon/src/lib/space/workflows/built-in-workflows.ts index ec5a96f08..fe4f6a2fe 100644 --- a/packages/daemon/src/lib/space/workflows/built-in-workflows.ts +++ b/packages/daemon/src/lib/space/workflows/built-in-workflows.ts @@ -441,6 +441,9 @@ const REVIEW_REVIEW_NODE = 'tpl-review-review'; * Two-node iterative graph: Coding ↔ Review (with cycle). * - Coding → Review: validated by a `send_message` hook (`pr_ready`) that checks * the PR is open, mergeable, and has no unresolved review threads. + * - Review → done: validated by a `mark_complete` hook (`pr_merged`) that blocks + * the post-approval reviewer from closing the task until the PR is actually + * merged; on a merge conflict it routes the conflict back to the coder. * - Coding → Validation Complete: gated by `validation-complete-gate` — accepts * no-code-change validation evidence (`completion_mode: "validation_only"`, * `changed_files: 0`, `validation_outcome`) and bypasses PR validation. @@ -586,6 +589,22 @@ export const CODING_WORKFLOW: SpaceWorkflow = { validator: { kind: 'built_in', id: 'pr_ready' }, authorizedCallers: [{ sourceNode: 'Coding', agentSlots: ['coder'] }], }, + { + // Blocks the post-approval reviewer's mark_complete (approved → done) + // until the PR is actually merged, so a task can never be closed for an + // unmerged PR. On a merge conflict the block routes the conflict back to + // the coder instead of letting the reviewer complete or escalate routine + // coder-owned work. Symmetric to code-pr-ready on the handoff side. + id: 'coding-pr-merged', + enabled: true, + label: 'PR Merged', + sourceNode: 'Review', + method: 'mark_complete', + classification: 'validation', + order: 0, + validator: { kind: 'built_in', id: 'pr_merged' }, + authorizedCallers: [{ sourceNode: 'Review', agentSlots: ['reviewer'] }], + }, ], gates: [ { @@ -777,6 +796,20 @@ export const RESEARCH_WORKFLOW: SpaceWorkflow = { validator: { kind: 'built_in', id: 'pr_ready' }, authorizedCallers: [{ sourceNode: 'Research', agentSlots: ['research'] }], }, + { + // Blocks the post-approval reviewer's mark_complete (approved → done) + // until the PR is actually merged; on a merge conflict it routes the + // conflict back to Research instead of completing or escalating. + id: 'research-pr-merged', + enabled: true, + label: 'PR Merged', + sourceNode: 'Review', + method: 'mark_complete', + classification: 'validation', + order: 0, + validator: { kind: 'built_in', id: 'pr_merged' }, + authorizedCallers: [{ sourceNode: 'Review', agentSlots: ['reviewer'] }], + }, ], channels: [ { @@ -1206,6 +1239,22 @@ export const FULLSTACK_QA_LOOP_WORKFLOW: SpaceWorkflow = { validator: { kind: 'built_in', id: 'pr_ready' }, authorizedCallers: [{ sourceNode: 'Coding', agentSlots: ['coder'] }], }, + { + // Blocks the post-approval reviewer's mark_complete (approved → done) + // until the PR is actually merged; on a merge conflict it routes the + // conflict back to Coding instead of completing or escalating. The + // post-approval reviewer session (targetAgent 'reviewer') inherits the + // Review node identity, so sourceNode is 'Review'. + id: 'fullstack-pr-merged', + enabled: true, + label: 'PR Merged', + sourceNode: 'Review', + method: 'mark_complete', + classification: 'validation', + order: 0, + validator: { kind: 'built_in', id: 'pr_merged' }, + authorizedCallers: [{ sourceNode: 'Review', agentSlots: ['reviewer'] }], + }, ], gates: [ { diff --git a/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts b/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts index b53217416..e1516209a 100644 --- a/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts @@ -209,6 +209,10 @@ describe('PostApprovalRouter.route', () => { // result artifact would poison a later successful completion. expect(delegates.spawned[0].kickoffMessage).toMatch(/NON-result artifact/); expect(delegates.spawned[0].kickoffMessage).toContain('type:"blocked"'); + // Non-PR route (no pr_merged hook) must not get the PR-merge guidance — a + // deployer has nothing to merge, so it must not be told to merge first. + expect(delegates.spawned[0].kickoffMessage).not.toContain('pr_merged'); + expect(delegates.spawned[0].kickoffMessage).not.toContain('actually merged'); const final = taskRepo.getTask(task.id); expect(final?.postApprovalSessionId).toBe('spawned-session-1'); @@ -217,6 +221,47 @@ describe('PostApprovalRouter.route', () => { expect(final?.status).toBe('approved'); }); + test('PR-merge route (workflow declares pr_merged hook) appends merge guidance', async () => { + const task = makeApprovedTask(taskRepo); + const delegates = makeDelegates(); + const router = new PostApprovalRouter({ + taskRepo, + spawner: delegates.spawner, + livenessProbe: delegates.liveness, + }); + const workflow = stubWorkflow({ + postApproval: { + targetAgent: 'reviewer', + instructions: 'Merge PR {{pr_url}}.', + }, + hooks: [ + { + id: 'coding-pr-merged', + enabled: true, + sourceNode: 'Review', + method: 'mark_complete', + classification: 'validation', + order: 0, + validator: { kind: 'built_in', id: 'pr_merged' }, + authorizedCallers: [{ sourceNode: 'Review', agentSlots: ['reviewer'] }], + }, + ], + }); + const result = await router.route(task, workflow, { + approvalSource: 'agent', + pr_url: 'https://github.com/acme/corp/pull/42', + }); + + expect(result.mode).toBe('spawn'); + // A PR-merge route declares a pr_merged hook, so the spawned reviewer is + // gated on a real merge and must receive the merge guidance. + expect(delegates.spawned[0].kickoffMessage).toContain('pr_merged'); + expect(delegates.spawned[0].kickoffMessage).toContain('actually merged'); + // Generic guidance still applies. + expect(delegates.spawned[0].kickoffMessage).toContain('mark_complete'); + expect(delegates.spawned[0].kickoffMessage).toContain('Do NOT call approve_task'); + }); + test('node-level postApproval on submitting node overrides legacy workflow route', async () => { const task = makeApprovedTask(taskRepo); const delegates = makeDelegates(); diff --git a/packages/daemon/tests/unit/5-space/runtime/pr-merged-validator.test.ts b/packages/daemon/tests/unit/5-space/runtime/pr-merged-validator.test.ts new file mode 100644 index 000000000..9af44a3ca --- /dev/null +++ b/packages/daemon/tests/unit/5-space/runtime/pr-merged-validator.test.ts @@ -0,0 +1,391 @@ +/** + * Unit tests for pr-merged built-in validator. + * + * Covers: + * - missing PR URL (no params, artifacts, or current-branch PR) + * - PR URL resolved from artifacts (camelCase + snake_case) + * - PR URL from templateData / params + * - merged PR → allow + * - unknown mergeability → retryable + * - merge conflict (CONFLICTING / DIRTY) → block routing back to coder + * - open but not merged, no conflict → block (merge first) + * - closed without merge → block + * - gh CLI error + * - rate-limit handling + * - restricted GitHub-only env + */ + +import { describe, test, expect } from 'bun:test'; +import { createPrMergedValidator } from '../../../../src/lib/space/runtime/built-in-validators/pr-merged-validator'; +import type { HookExecutorContext } from '../../../../src/lib/space/runtime/hook-executor'; +import { RATE_LIMIT_MIN_BACKOFF_MS } from '../../../../src/lib/space/runtime/rate-limit-detector'; + +function streamFromString(text: string): ReadableStream { + return new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(text)); + controller.close(); + }, + }); +} + +function makeMockSpawn( + results: Array<{ stdout: string; stderr: string; exitCode: number }>, + calls?: Array<{ cmd: string[]; options?: unknown }> +): typeof Bun.spawn { + let callIndex = 0; + return ((cmd: string[], options?: unknown) => { + calls?.push({ cmd, options }); + if (cmd[0] === 'git' && cmd[1] === 'config') { + return { + stdout: streamFromString('git@github.com:acme/corp.git\n'), + stderr: streamFromString(''), + exited: Promise.resolve(0), + pid: 12345, + kill() {}, + } as unknown as ReturnType; + } + const result = results[callIndex++] ?? { stdout: '', stderr: '', exitCode: 1 }; + return { + stdout: streamFromString(result.stdout), + stderr: streamFromString(result.stderr), + exited: Promise.resolve(result.exitCode), + pid: 12345, + kill() {}, + } as unknown as ReturnType; + }) as unknown as typeof Bun.spawn; +} + +interface MakeContextOpts { + prUrl?: string; + artifacts?: HookExecutorContext['currentArtifacts']; + templateData?: Record; +} + +function makeContext(opts: MakeContextOpts = {}): HookExecutorContext { + return { + workspacePath: '/tmp', + runId: 'run-1', + hookId: 'hook-1', + methodName: 'mark_complete', + params: opts.prUrl ? { goal_update: { data: { pr_url: opts.prUrl } } } : {}, + nodeId: 'node-review-1', + nodeName: 'Review', + sessionId: 'sess-1', + taskId: 'task-1', + hookLocalState: {}, + currentArtifacts: opts.artifacts ?? [], + permittedExternalLookups: ['github'], + ...(opts.templateData ? { templateData: opts.templateData } : {}), + }; +} + +const MERGED_PR_VIEW = { + url: 'https://github.com/acme/corp/pull/42', + state: 'MERGED', + mergeable: 'MERGEABLE', + mergeStateStatus: 'CLEAN', +}; + +describe('pr-merged validator', () => { + test('missing PR URL and no artifacts and no current branch PR → block', async () => { + const validator = createPrMergedValidator( + makeMockSpawn([{ stdout: '', stderr: 'no pull requests found', exitCode: 1 }]) + ); + const result = await validator(makeContext()); + expect(result.type).toBe('block'); + expect((result as { reason: string }).reason).toContain('no PR URL resolved to verify merge'); + expect((result as { reason: string }).reason).toContain('current-branch PR discovery failed'); + expect((result as { reason: string }).reason).toContain('no pull requests found'); + }); + + test('PR URL resolved from artifacts (camelCase prUrl) → merged → allow', async () => { + const spawn = makeMockSpawn([ + { stdout: JSON.stringify(MERGED_PR_VIEW), stderr: '', exitCode: 0 }, + ]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('allow'); + expect((result as { data?: Record }).data?.pr_url).toBe( + 'https://github.com/acme/corp/pull/42' + ); + expect((result as { data?: Record }).data?.merged).toBe(true); + }); + + test('PR URL resolved from artifacts (snake_case pr_url)', async () => { + const spawn = makeMockSpawn([ + { stdout: JSON.stringify(MERGED_PR_VIEW), stderr: '', exitCode: 0 }, + ]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'progress', data: { pr_url: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('allow'); + }); + + test('artifact lookup prefers most-recent first (first hit wins)', async () => { + // currentArtifacts is sorted most-recent-first by the hook engine; the + // first artifact carrying a PR URL should win. + const spawn = makeMockSpawn([ + { + stdout: JSON.stringify({ ...MERGED_PR_VIEW, url: 'https://github.com/acme/corp/pull/99' }), + stderr: '', + exitCode: 0, + }, + ]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [ + { type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/99' } }, + { type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }, + ], + }) + ); + expect(result.type).toBe('allow'); + expect((result as { data?: Record }).data?.pr_url).toBe( + 'https://github.com/acme/corp/pull/99' + ); + }); + + test('PR URL from templateData → merged → allow', async () => { + const spawn = makeMockSpawn([ + { stdout: JSON.stringify(MERGED_PR_VIEW), stderr: '', exitCode: 0 }, + ]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ templateData: { pr_url: 'https://github.com/acme/corp/pull/42' } }) + ); + expect(result.type).toBe('allow'); + }); + + test('unknown mergeability → retryable_block', async () => { + const prView = { + ...MERGED_PR_VIEW, + state: 'OPEN', + mergeable: 'UNKNOWN', + mergeStateStatus: 'UNKNOWN', + }; + const spawn = makeMockSpawn([{ stdout: JSON.stringify(prView), stderr: '', exitCode: 0 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('retryable_block'); + expect((result as { reason: string }).reason).toContain('Waiting for GitHub mergeability'); + expect((result as { retryAfterMs?: number }).retryAfterMs).toBe(30_000); + }); + + test('merge conflict (mergeable CONFLICTING) → block routing back to coder', async () => { + const prView = { + ...MERGED_PR_VIEW, + state: 'OPEN', + mergeable: 'CONFLICTING', + mergeStateStatus: 'BLOCKED', + }; + const spawn = makeMockSpawn([{ stdout: JSON.stringify(prView), stderr: '', exitCode: 0 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('block'); + const reason = (result as { reason: string }).reason; + expect(reason).toContain('unresolved merge conflicts'); + expect(reason).toContain('CONFLICTING'); + expect(reason).toContain('Route the conflict back'); + expect(reason).toContain('coder'); + }); + + test('merge conflict (mergeStateStatus DIRTY) → block routing back to coder', async () => { + const prView = { + ...MERGED_PR_VIEW, + state: 'OPEN', + mergeable: 'MERGEABLE', + mergeStateStatus: 'DIRTY', + }; + const spawn = makeMockSpawn([{ stdout: JSON.stringify(prView), stderr: '', exitCode: 0 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('block'); + const reason = (result as { reason: string }).reason; + expect(reason).toContain('unresolved merge conflicts'); + expect(reason).toContain('DIRTY'); + expect(reason).toContain('Route the conflict back'); + }); + + test('open but not merged and no conflict → block (merge first)', async () => { + const prView = { + ...MERGED_PR_VIEW, + state: 'OPEN', + mergeable: 'MERGEABLE', + mergeStateStatus: 'BEHIND', + }; + const spawn = makeMockSpawn([{ stdout: JSON.stringify(prView), stderr: '', exitCode: 0 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('block'); + const reason = (result as { reason: string }).reason; + expect(reason).toContain('PR is not merged'); + expect(reason).toContain('Merge the PR'); + expect(reason).toContain('gh pr merge'); + }); + + test('closed without merge → block', async () => { + const prView = { + ...MERGED_PR_VIEW, + state: 'CLOSED', + mergeable: 'MERGEABLE', + mergeStateStatus: 'CLEAN', + }; + const spawn = makeMockSpawn([{ stdout: JSON.stringify(prView), stderr: '', exitCode: 0 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('block'); + expect((result as { reason: string }).reason).toContain('CLOSED without being merged'); + }); + + test('closed PR with transient UNKNOWN mergeability → block (CLOSED checked before UNKNOWN)', async () => { + // A closed PR is terminal — it must hard-block even if GitHub hasn't + // finished computing mergeability (UNKNOWN), rather than looping on + // retryable_block. + const prView = { + ...MERGED_PR_VIEW, + state: 'CLOSED', + mergeable: 'UNKNOWN', + mergeStateStatus: 'UNKNOWN', + }; + const spawn = makeMockSpawn([{ stdout: JSON.stringify(prView), stderr: '', exitCode: 0 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('block'); + expect((result as { reason: string }).reason).toContain('CLOSED without being merged'); + }); + + test('gh CLI error → block with stderr reason', async () => { + const spawn = makeMockSpawn([{ stdout: '', stderr: 'gh: not authenticated', exitCode: 1 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('block'); + expect((result as { reason: string }).reason).toContain('not authenticated'); + }); + + test('rate-limited gh pr view → retryable_block with backoff from /rate_limit', async () => { + const resetEpochSeconds = Math.floor((Date.now() + 90_000) / 1000); + const spawn = makeMockSpawn([ + { + stdout: '', + stderr: + 'HTTP 403: rate limit exceeded (https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting)', + exitCode: 1, + }, + { + stdout: JSON.stringify({ resources: { graphql: { reset: resetEpochSeconds } } }), + stderr: '', + exitCode: 0, + }, + ]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('retryable_block'); + expect((result as { reason: string }).reason).toContain('rate limited'); + expect((result as { retryAfterMs?: number }).retryAfterMs).toBeGreaterThanOrEqual(60_000); + }); + + test('secondary rate-limit error skips probe and returns min backoff', async () => { + const calls: Array<{ cmd: string[] }> = []; + const spawn = makeMockSpawn( + [{ stdout: '', stderr: 'HTTP 403: You have exceeded a secondary rate limit', exitCode: 1 }], + calls + ); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('retryable_block'); + expect((result as { retryAfterMs?: number }).retryAfterMs).toBe(RATE_LIMIT_MIN_BACKOFF_MS); + // Only one gh call (the pr view); no /rate_limit probe for secondary limits. + expect(calls.filter((c) => c.cmd[0] === 'gh').length).toBe(1); + }); + + test('non-rate-limit gh error hard-blocks (no retryable promotion)', async () => { + const spawn = makeMockSpawn([{ stdout: '', stderr: 'HTTP 404: Not Found', exitCode: 1 }]); + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('block'); + expect((result as { reason: string }).reason).toContain('Not Found'); + }); + + test('gh subprocess receives restricted GitHub-only env', async () => { + const calls: Array<{ cmd: string[]; options?: unknown }> = []; + const spawn = makeMockSpawn( + [{ stdout: JSON.stringify(MERGED_PR_VIEW), stderr: '', exitCode: 0 }], + calls + ); + const previousAnthropicKey = process.env.ANTHROPIC_API_KEY; + const previousClaudeToken = process.env.CLAUDE_CODE_OAUTH_TOKEN; + const previousGhToken = process.env.GH_TOKEN; + process.env.ANTHROPIC_API_KEY = 'anthropic-secret'; + process.env.CLAUDE_CODE_OAUTH_TOKEN = 'claude-secret'; + process.env.GH_TOKEN = 'github-secret'; + try { + const validator = createPrMergedValidator(spawn); + const result = await validator( + makeContext({ + artifacts: [{ type: 'result', data: { prUrl: 'https://github.com/acme/corp/pull/42' } }], + }) + ); + expect(result.type).toBe('allow'); + const env = (calls[0].options as { env?: Record }).env ?? {}; + expect(env.GH_TOKEN).toBe('github-secret'); + expect(env.ANTHROPIC_API_KEY).toBeUndefined(); + expect(env.CLAUDE_CODE_OAUTH_TOKEN).toBeUndefined(); + } finally { + if (previousAnthropicKey === undefined) delete process.env.ANTHROPIC_API_KEY; + else process.env.ANTHROPIC_API_KEY = previousAnthropicKey; + if (previousClaudeToken === undefined) delete process.env.CLAUDE_CODE_OAUTH_TOKEN; + else process.env.CLAUDE_CODE_OAUTH_TOKEN = previousClaudeToken; + if (previousGhToken === undefined) delete process.env.GH_TOKEN; + else process.env.GH_TOKEN = previousGhToken; + } + }); +}); diff --git a/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts b/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts index 9e70b13f1..8fc704da5 100644 --- a/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts +++ b/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts @@ -282,6 +282,19 @@ describe('CODING_WORKFLOW template', () => { expect(hook!.authorizedCallers).toEqual([{ sourceNode: 'Coding', agentSlots: ['coder'] }]); }); + test('has a mark_complete hook on Review using pr_merged validator', () => { + const hooks = CODING_WORKFLOW.hooks ?? []; + const hook = hooks.find((h) => h.id === 'coding-pr-merged'); + expect(hook).toBeDefined(); + expect(hook!.sourceNode).toBe('Review'); + expect(hook!.targetNode).toBeUndefined(); + expect(hook!.method).toBe('mark_complete'); + expect(hook!.validator).toEqual({ kind: 'built_in', id: 'pr_merged' }); + expect(hook!.enabled).toBe(true); + expect(hook!.classification).toBe('validation'); + expect(hook!.authorizedCallers).toEqual([{ sourceNode: 'Review', agentSlots: ['reviewer'] }]); + }); + test('review feedback cycle is gated by fresh GitHub review evidence', () => { const channel = CODING_WORKFLOW.channels!.find( (c) => c.from === 'Review' && c.to === 'Coding' @@ -753,6 +766,18 @@ describe('RESEARCH_WORKFLOW template', () => { expect(hook!.enabled).toBe(true); }); + test('has a mark_complete hook on Review using pr_merged validator', () => { + const hooks = RESEARCH_WORKFLOW.hooks ?? []; + const hook = hooks.find((h) => h.id === 'research-pr-merged'); + expect(hook).toBeDefined(); + expect(hook!.sourceNode).toBe('Review'); + expect(hook!.targetNode).toBeUndefined(); + expect(hook!.method).toBe('mark_complete'); + expect(hook!.validator).toEqual({ kind: 'built_in', id: 'pr_merged' }); + expect(hook!.enabled).toBe(true); + expect(hook!.authorizedCallers).toEqual([{ sourceNode: 'Review', agentSlots: ['reviewer'] }]); + }); + test('channel from/to references match node names', () => { const nodeNames = new Set(RESEARCH_WORKFLOW.nodes.map((n) => n.name)); for (const ch of RESEARCH_WORKFLOW.channels!) { @@ -5635,7 +5660,9 @@ describe('Reviewer Terminal Action Pre-conditions (Task #136 regression)', () => templateName: FULLSTACK_QA_LOOP_WORKFLOW.name, templateGates: FULLSTACK_QA_LOOP_WORKFLOW.gates ?? [], }).workflow; - const hook = workflow.hooks?.find((h) => h.sourceNode === 'Review'); + const hook = workflow.hooks?.find( + (h) => h.sourceNode === 'Review' && h.validator.kind === 'script' + ); expect(hook).toBeDefined(); const source = hook?.validator.kind === 'script' ? hook.validator.source : ''; expect(source).toContain('-lt 300 '); @@ -5767,7 +5794,9 @@ describe('Reviewer Terminal Action Pre-conditions (Task #136 regression)', () => templateName: FULLSTACK_QA_LOOP_WORKFLOW.name, templateGates: FULLSTACK_QA_LOOP_WORKFLOW.gates ?? [], }).workflow; - const hook = workflow.hooks?.find((h) => h.sourceNode === 'Review'); + const hook = workflow.hooks?.find( + (h) => h.sourceNode === 'Review' && h.validator.kind === 'script' + ); expect(hook?.validator.kind).toBe('script'); if (hook?.validator.kind === 'script') { expect(hook.validator.externalLookups).toEqual(['github']); @@ -5790,7 +5819,9 @@ describe('Reviewer Terminal Action Pre-conditions (Task #136 regression)', () => templateName: FULLSTACK_QA_LOOP_WORKFLOW.name, templateGates: FULLSTACK_QA_LOOP_WORKFLOW.gates ?? [], }).workflow; - const initialHook = initial.hooks?.find((h) => h.sourceNode === 'Review'); + const initialHook = initial.hooks?.find( + (h) => h.sourceNode === 'Review' && h.validator.kind === 'script' + ); expect(initialHook?.validator.kind).toBe('script'); if (initialHook?.validator.kind === 'script') { expect(initialHook.validator.source).toContain('-lt 300 '); @@ -5807,7 +5838,9 @@ describe('Reviewer Terminal Action Pre-conditions (Task #136 regression)', () => templateName: FULLSTACK_QA_LOOP_WORKFLOW.name, templateGates: FULLSTACK_QA_LOOP_WORKFLOW.gates ?? [], }).workflow; - const reMigratedHook = reMigrated.hooks?.find((h) => h.sourceNode === 'Review'); + const reMigratedHook = reMigrated.hooks?.find( + (h) => h.sourceNode === 'Review' && h.validator.kind === 'script' + ); expect(reMigratedHook?.validator.kind).toBe('script'); if (reMigratedHook?.validator.kind === 'script') { expect(reMigratedHook.validator.source).toContain('-lt 900 '); @@ -5828,7 +5861,9 @@ describe('Reviewer Terminal Action Pre-conditions (Task #136 regression)', () => templateName: FULLSTACK_QA_LOOP_WORKFLOW.name, templateGates: FULLSTACK_QA_LOOP_WORKFLOW.gates ?? [], }).workflow; - const initialHook = initial.hooks?.find((h) => h.sourceNode === 'Review'); + const initialHook = initial.hooks?.find( + (h) => h.sourceNode === 'Review' && h.validator.kind === 'script' + ); if (initialHook?.validator.kind === 'script') { expect(initialHook.validator.source).toContain('-lt 300 '); } @@ -5846,7 +5881,9 @@ describe('Reviewer Terminal Action Pre-conditions (Task #136 regression)', () => templateName: FULLSTACK_QA_LOOP_WORKFLOW.name, templateGates: FULLSTACK_QA_LOOP_WORKFLOW.gates ?? [], }).workflow; - const reMigratedHook = reMigrated.hooks?.find((h) => h.sourceNode === 'Review'); + const reMigratedHook = reMigrated.hooks?.find( + (h) => h.sourceNode === 'Review' && h.validator.kind === 'script' + ); expect(reMigratedHook?.validator.kind).toBe('script'); if (reMigratedHook?.validator.kind === 'script') { expect(reMigratedHook.validator.source).toContain('-lt 7200 '); @@ -6045,6 +6082,18 @@ test('FULLSTACK_QA_LOOP_WORKFLOW has a send_message hook for Coding → Review u expect(hook!.enabled).toBe(true); }); +test('FULLSTACK_QA_LOOP_WORKFLOW has a mark_complete hook on Review using pr_merged validator', () => { + const hooks = FULLSTACK_QA_LOOP_WORKFLOW.hooks ?? []; + const hook = hooks.find((h) => h.id === 'fullstack-pr-merged'); + expect(hook).toBeDefined(); + expect(hook!.sourceNode).toBe('Review'); + expect(hook!.targetNode).toBeUndefined(); + expect(hook!.method).toBe('mark_complete'); + expect(hook!.validator).toEqual({ kind: 'built_in', id: 'pr_merged' }); + expect(hook!.enabled).toBe(true); + expect(hook!.authorizedCallers).toEqual([{ sourceNode: 'Review', agentSlots: ['reviewer'] }]); +}); + test('FULLSTACK_QA_LOOP_WORKFLOW coder prompt uses behavioral hook handoff wording', () => { const codingNode = FULLSTACK_QA_LOOP_WORKFLOW.nodes.find((n) => n.name === 'Coding')!; const prompt = codingNode.agents[0].customPrompt!.value; diff --git a/packages/shared/src/types/space.ts b/packages/shared/src/types/space.ts index 9badd6350..e5d643031 100644 --- a/packages/shared/src/types/space.ts +++ b/packages/shared/src/types/space.ts @@ -1549,6 +1549,7 @@ export type WorkflowHookValidatorId = | 'pr_open' | 'pr_mergeable' | 'pr_ready' + | 'pr_merged' | 'github_review_approved' | 'codex_review_approved' | 'artifact_exists' diff --git a/packages/web/src/components/space/visual-editor/HookEditorPanel.tsx b/packages/web/src/components/space/visual-editor/HookEditorPanel.tsx index 454eba126..69d3e51e5 100644 --- a/packages/web/src/components/space/visual-editor/HookEditorPanel.tsx +++ b/packages/web/src/components/space/visual-editor/HookEditorPanel.tsx @@ -29,6 +29,7 @@ const BUILT_IN_VALIDATORS: WorkflowHookValidatorId[] = [ 'pr_open', 'pr_mergeable', 'pr_ready', + 'pr_merged', 'github_review_approved', 'codex_review_approved', 'artifact_exists', @@ -39,6 +40,7 @@ const BUILT_IN_VALIDATOR_COPY: Record = { pr_open: 'PR-ready block: requires an open pull request URL before this hook can pass.', pr_mergeable: 'PR-ready block: requires GitHub to report the pull request as mergeable.', pr_ready: 'PR-ready block: requires an open, mergeable pull request with approval checks passed.', + pr_merged: 'PR-merged block: requires the pull request to be merged before completion can pass.', github_review_approved: 'Requires an approved GitHub review on the pull request.', codex_review_approved: 'Codex retry: blocks until Codex approval is available, then retry safely.',