diff --git a/evidence/slice-AA-local-verification.md b/evidence/slice-AA-local-verification.md new file mode 100644 index 000000000..0f7fdd69b --- /dev/null +++ b/evidence/slice-AA-local-verification.md @@ -0,0 +1,98 @@ +# Slice AA local verification + +The implementation is committed without a push. Full-suite verification remains blocked in this sandbox; live GitHub/agent acceptance was not run. + +## SDK typecheck, test typecheck, and build + +Working directory: `/Users/khaliqgant/fl-slice-AA/packages/sdk` + +```sh +npm run typecheck && npm run typecheck:tests && npm run build +``` + +Exit code: 0. Captured output: + +```text + +> @relayflows/sdk@2.0.8 typecheck +> tsc --noEmit && tsc -p tsconfig.type-tests.json + + +> @relayflows/sdk@2.0.8 typecheck:tests +> tsc -p tsconfig.tests.json + + +> @relayflows/sdk@2.0.8 build +> tsc && node scripts/make-cli-executable.mjs + +``` + +## Surface typecheck and tests + +Working directory: `/Users/khaliqgant/fl-slice-AA/packages/surface` + +```sh +npm run typecheck && ./node_modules/.bin/tsc -p tsconfig.test.json && ./node_modules/.bin/vitest run +``` + +Exit code: 0. Captured output: + +```text + +> @relayflows/surface@2.0.8 typecheck +> tsc --noEmit + + + RUN v2.1.9 /Users/khaliqgant/fl-slice-AA/packages/surface + + ✓ tests/triggers.test.ts (4 tests) 3ms + ✓ tests/flow.test.ts (20 tests) 7ms + ✓ tests/helpers.snapshot.test.ts (1 test) 215ms + + Test Files 3 passed (3) + Tests 25 passed (25) + Start at 00:23:13 + Duration 449ms (transform 58ms, setup 0ms, collect 90ms, tests 224ms, environment 0ms, prepare 91ms) + +``` + +## Flow import validation + +Working directory: `/Users/khaliqgant/fl-slice-AA/packages/sdk` + +```sh +node dist/cli.js check scripts/dogfood/close-pr.flow.ts --json +``` + +Exit code: 0. Captured output: + +```text +{"ok":true,"gates":[],"resolutions":[],"diagnostics":[],"mcpTools":{},"plugins":[],"path":"scripts/dogfood/close-pr.flow.ts"} +``` + +## Full SDK suite + +Working directory: `packages/sdk` in this checkout. + +```sh +PATH=/Users/khaliqgant/.bun/bin:/Users/khaliqgant/.cargo/bin:$PATH RELAYFLOWD_BIN=/private/tmp/slice-AA-cargo-target/debug/relayflowd RELAYFLOWS_ALLOW_ANALYZER_SKIP=1 ./node_modules/.bin/vitest run > /private/tmp/slice-AA-sdk-tests-final.txt 2>&1 +``` + +Exit code: 1. Captured result lines: + +```text + ✓ tests/close-pr-flow.test.ts (27 tests) 3461ms + + Test Files 24 failed | 56 passed | 1 skipped (81) + Tests 180 failed | 1133 passed | 11 skipped (1324) + Errors 14 errors + Start at 00:22:05 + Duration 58.72s (transform 1.17s, setup 0ms, collect 9.28s, tests 263.31s, environment 7ms, prepare 2.27s) + +``` + +Full captured output on this machine: `/private/tmp/slice-AA-sdk-tests-final.txt`. + +The sandbox rejects Unix/TCP listeners with EPERM. The full run also reports EMFILE from file watching. The new real-daemon handoff test could not reach its assertions because the daemon could not bind its socket. The 27 close-loop tests use simulated GitHub/agent/journal transport, with real compiler and authored executor code; one also executes the generated commit and force-push commands against a temporary local Git remote. They do not contact GitHub. + +The authored needs_human outcome is a journaled handoff, not a resumable kernel wait; the current authored runner has no durable root. diff --git a/packages/sdk/scripts/dogfood/README.md b/packages/sdk/scripts/dogfood/README.md new file mode 100644 index 000000000..ae22ba425 --- /dev/null +++ b/packages/sdk/scripts/dogfood/README.md @@ -0,0 +1,64 @@ +# PR close loop + +`close-pr.flow.ts` is the companion to slice implementation. Invoke it after +the slice branch has been pushed. Running this flow opts into repair commits, +force-with-lease pushes, and squash merge with branch deletion. + +Set `IMPL_CLOSE_INPUT` to JSON in the **daemon's environment** (deterministic +commands inherit that environment): + +```json +{ + "worktree": "/absolute/path/to/slice-worktree", + "repo": "owner/repository", + "branch": "feat/slice", + "base": "main", + "title": "Implement slice", + "body": "Closes #123", + "cli": "codex", + "model": "your-configured-model" +} +``` + +Launch with `flows run packages/sdk/scripts/dogfood/close-pr.flow.ts --input '{}'` +using the daemon carrying that input. Alternatively, save the JSON in a file and +pass `--input close-input.json`; this is also captured in a journaled step and +does not require setting the daemon's environment. The worktree must be clean +and on the named branch. +`gh` must be authenticated with access to checks, Actions logs, review threads, +PR creation and merging. Attach a workspace-capable agent worker holding the +worktree's revision pins, launched from that worktree. The stream-only +`--local-agent` worker cannot accept a workspace declaration in this SDK. + +Optional `prNumber` selects an existing PR; otherwise the flow looks up the open +PR for the branch before creating one. `cli` defaults to `codex`; the model is +passed through to normal SDK preflight. `pollIntervalSeconds` defaults to 15 +and `maxPolls` to 120 per pushed head. Three **repair attempts** are allowed; +pending polls do not consume them, and the third repair is re-verified. + +Each poll reads checks, paginated Bugbot review comments, and paginated review +thread resolution state through separate `f.run` effects. A completed check +whose name contains `Bugbot` is required, so missing/delayed reviews cannot look +green. Failed/canceled checks and Medium/High/Critical (or P0–P2) Bugbot comments +block merging. Low findings and other bots' comments are ignored. Unresolved +findings remain blocking even on old commits or outdated diff lines; only a +resolved thread clears a finding. Bugbot must resolve addressed threads on +re-review, or the run hands them to a human after its repair budget. + +Failed GitHub Actions checks supply `gh run view --log-failed` output to the +repair agent. Other check providers supply their description and link. The +agent edits the same declared worktree, then deterministic steps commit changes +and push. The flow checks the PR head before and after each snapshot and passes +`--match-head-commit` to merge. It confirms `MERGED`, since `gh pr merge` can +instead enqueue a PR. A queued merge is handed off for human follow-up. + +Exhaustion prints accumulated blockers in a journaled effect and calls +`f.done('needs_human')`. The authored executor records this handoff in a terminal +effect and the CLI returns exit 3 / `parked`. This is an **authored handoff**, not +a new kernel completion reason or a resumable kernel wait. The current authored +executor has no durable root: effects have journals, but restarting the whole +script does not replay them automatically. Re-invocation reuses the open PR. + +Local tests simulate GitHub, the repair agent and journal transport through the +real authored executor. They do not constitute a live GitHub/agent +acceptance run. The CLI handoff also has a test against the real local daemon. diff --git a/packages/sdk/scripts/dogfood/close-pr-state.ts b/packages/sdk/scripts/dogfood/close-pr-state.ts new file mode 100644 index 000000000..ac876b183 --- /dev/null +++ b/packages/sdk/scripts/dogfood/close-pr-state.ts @@ -0,0 +1,143 @@ +import { isAbsolute } from 'node:path'; + +export const MAX_REPAIR_ITERATIONS = 3; + +// Sleep between polls runs inside a f.run step whose default kernel lease is +// 30 s (see flows#343 / slice W for the per-step timeout override). Any poll +// interval larger than that either times out the step or, worse, sleeps under +// a lease renewal window and races. Cap silently so a caller who sets +// `pollIntervalSeconds: 60` still gets a poll cycle instead of a step_failed. +export const MAX_POLL_INTERVAL_SECONDS = 25; + +export interface ClosePrInput { + worktree: string; + repo: string; + branch: string; + base?: string; + title?: string; + body?: string; + prNumber?: number; + cli?: string; + model?: string; + pollIntervalSeconds?: number; + maxPolls?: number; +} + +export function parseInput(raw: string): ClosePrInput { + const input = JSON.parse(raw) as ClosePrInput; + if (!input || typeof input !== 'object') throw new Error('IMPL_CLOSE_INPUT must be a JSON object'); + for (const key of ['worktree', 'repo', 'branch'] as const) { + if (typeof input[key] !== 'string' || !input[key].trim() || input[key].includes('\0')) { + throw new Error(`IMPL_CLOSE_INPUT.${key} must be a nonempty string`); + } + } + if (!isAbsolute(input.worktree)) throw new Error('worktree must be an absolute path'); + if (!/^[\w.-]+\/[\w.-]+$/.test(input.repo)) throw new Error('repo must be OWNER/REPO'); + if (input.branch.startsWith('-')) throw new Error('branch must not start with -'); + for (const key of ['base', 'title', 'body', 'cli', 'model'] as const) { + if (input[key] !== undefined && (typeof input[key] !== 'string' || input[key].includes('\0'))) { + throw new Error(`${key} must be a string`); + } + } + for (const key of ['prNumber', 'maxPolls', 'pollIntervalSeconds'] as const) { + if (input[key] !== undefined && (!Number.isSafeInteger(input[key]) || input[key] < 1)) { + throw new Error(`${key} must be a positive integer`); + } + } + if (input.pollIntervalSeconds !== undefined) { + input.pollIntervalSeconds = Math.min(input.pollIntervalSeconds, MAX_POLL_INTERVAL_SECONDS); + } + return input; +} + +export function quote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +export function parsePrNumber(output: string): number { + const match = output.trim().match(/^(?:https:\/\/[^/]+\/[^/]+\/[^/]+\/pull\/)?([1-9]\d*)\/?$/); + const number = Number(match?.[1]); + if (!Number.isSafeInteger(number) || number < 1) throw new Error(`Invalid PR number: ${output}`); + return number; +} + +export interface Check { + name: string; + bucket: 'pass' | 'fail' | 'pending' | 'skipping' | 'cancel'; + link: string; + description: string; +} + +export interface BotComment { + id: number; + body: string; + path: string; + html_url: string; + user: { login: string }; +} + +export interface ReviewThread { + isResolved: boolean; + isOutdated: boolean; + commentId: number; +} + +export interface Finding { + kind: 'ci' | 'bugbot'; + message: string; + link: string; +} + +/** Parse API data fail-closed: empty/malformed output must never mean green. */ +export function parseChecks(raw: string): Check[] { + const checks = JSON.parse(raw) as Check[]; + if (!Array.isArray(checks) || checks.some(check => !check || typeof check.name !== 'string' + || !['pass', 'fail', 'pending', 'skipping', 'cancel'].includes(check.bucket) + || typeof check.link !== 'string' || typeof check.description !== 'string')) { + throw new Error('Invalid gh pr checks response'); + } + return checks; +} + +export function analyzeFindings(checks: Check[], commentsRaw: string, threadsRaw: string) { + const comments = JSON.parse(commentsRaw) as BotComment[]; + const threads = JSON.parse(threadsRaw) as ReviewThread[]; + if (!Array.isArray(comments) || comments.some(comment => !comment || !Number.isSafeInteger(comment.id) + || typeof comment.body !== 'string' || typeof comment.user?.login !== 'string' + || typeof comment.path !== 'string' || typeof comment.html_url !== 'string')) { + throw new Error('Invalid PR comments response'); + } + if (!Array.isArray(threads) || threads.some(thread => !thread || !Number.isSafeInteger(thread.commentId) + || typeof thread.isResolved !== 'boolean' || typeof thread.isOutdated !== 'boolean')) { + throw new Error('Invalid PR review threads response'); + } + const findings: Finding[] = checks.filter(check => ['fail', 'cancel'].includes(check.bucket)) + .map(check => ({ kind: 'ci', message: `${check.name}: ${check.bucket}. ${check.description}`, link: check.link })); + for (const comment of comments) { + if (!/^(cursor|bugbot)(\[bot\])?$/i.test(comment.user.login)) continue; + const text = comment.body.replace(/]*\balt=["']([^"']*)["'][^>]*>/gi, '$1') + .replace(/<[^>]*>/g, ' ').replace(/[*_`]/g, ''); + if (!/\b(?:medium|high|critical)\s+(?:severity|priority)\b|\bseverity\s*:\s*(?:medium|high|critical)\b|\bP[012]\b/i.test(text)) continue; + const thread = threads.find(thread => thread.commentId === comment.id); + // Missing thread metadata is blocking. Old commit_id alone is not proof of a fix. + if (thread?.isResolved) continue; + findings.push({ kind: 'bugbot', message: `${comment.path}: ${comment.body}`, link: comment.html_url }); + } + const pending = checks.length === 0 || checks.some(check => check.bucket === 'pending'); + const bugbotReviewed = checks.some(check => /bugbot/i.test(check.name) && ['pass', 'fail', 'cancel'].includes(check.bucket)); + return { findings, pending: pending || !bugbotReviewed }; +} + +/** gh returns 1 for failed checks and 8 for pending checks; neither is an effect failure. */ +export function checksCommand(pr: number, repo: string): string { + return `gh pr checks ${pr} --repo ${quote(repo)} --json name,bucket,link,description; ` + + 'close_status=$?; case "$close_status" in 0|1|8) exit 0 ;; *) exit "$close_status" ;; esac'; +} + +export function failedRunId(link: string, repo: string): string | undefined { + let url: URL; + try { url = new URL(link); } catch { return undefined; } + if (url.hostname !== 'github.com') return undefined; + const prefix = `/${repo}/actions/runs/`; + return url.pathname.startsWith(prefix) ? url.pathname.slice(prefix.length).match(/^([0-9]+)(?:\/|$)/)?.[1] : undefined; +} diff --git a/packages/sdk/scripts/dogfood/close-pr.flow.ts b/packages/sdk/scripts/dogfood/close-pr.flow.ts new file mode 100644 index 000000000..9f31c11f4 --- /dev/null +++ b/packages/sdk/scripts/dogfood/close-pr.flow.ts @@ -0,0 +1,104 @@ +import { flow } from '@relayflows/surface'; +import { + analyzeFindings, checksCommand, failedRunId, MAX_REPAIR_ITERATIONS, + parseChecks, parseInput, parsePrNumber, quote, type Finding, +} from './close-pr-state.ts'; + +// Run after implement/push. IMPL_CLOSE_INPUT is JSON, captured by a journaled step. +export default flow('close-pr', async (f, supplied) => { + const fromEnvironment = supplied === undefined || (supplied !== null && typeof supplied === 'object' + && !Array.isArray(supplied) && Object.keys(supplied).length === 0); + const input = parseInput(await f.run(fromEnvironment ? 'printf \'%s\' "$IMPL_CLOSE_INPUT"' + : `printf '%s' ${quote(JSON.stringify(supplied))}`)); + const run = (command: string) => f.run(`cd ${quote(input.worktree)} && (${command})`); + const repo = `--repo ${quote(input.repo)}`; + const assertBranch = `test "$(git branch --show-current)" = ${quote(input.branch)}`; + await run(`${assertBranch} && test -z "$(git status --porcelain)"`); + let head = (await run('git rev-parse HEAD')).trim(); + let pr = input.prNumber; + if (pr === undefined) { + const existing = JSON.parse(await run(`gh pr list ${repo} --head ${quote(input.branch)} ` + + `--state open ${input.base ? `--base ${quote(input.base)} ` : ''}--json number`)) as { number: number }[]; + if (!Array.isArray(existing) || existing.length > 1) throw new Error('Expected at most one open PR for branch'); + pr = existing.length ? parsePrNumber(String(existing[0]?.number)) : parsePrNumber(await run( + `gh pr create ${repo} --head ${quote(input.branch)} ` + + `${input.base ? `--base ${quote(input.base)} ` : ''}` + + `--title ${quote(input.title ?? input.branch)} --body ${quote(input.body ?? 'Implemented slice; CI and Bugbot repair loop.')}`, + )); + } + + const [owner, name] = input.repo.split('/'); + const threadsQuery = `query($owner: String!, $name: String!, $number: Int!, $endCursor: String) { + repository(owner: $owner, name: $name) { pullRequest(number: $number) { + reviewThreads(first: 100, after: $endCursor) { + nodes { isResolved isOutdated comments(first: 1) { nodes { databaseId } } } + pageInfo { hasNextPage endCursor } + } + } } + }`; + const blockers: Finding[] = []; + let iteration = 0; + let polls = 0; + const maxPolls = input.maxPolls ?? 120; + while (polls < maxPolls) { + polls += 1; + const state = JSON.parse(await run(`gh pr view ${pr} ${repo} --json headRefOid,headRefName,state`)); + if (state.state !== 'OPEN' || state.headRefName !== input.branch || state.headRefOid !== head) { + blockers.push({ kind: 'ci', message: 'PR is not open at the expected branch and worktree HEAD', link: '' }); + break; + } + const checks = parseChecks(await run(checksCommand(pr, input.repo))); + const comments = await run(`gh api ${quote(`repos/${input.repo}/pulls/${pr}/comments`)} --paginate --slurp ` + + `--jq ${quote('[.[][] | select(.user.login == "cursor[bot]" or .user.login == "bugbot[bot]" or .user.login == "cursor" or .user.login == "bugbot") | {id,body,path,html_url,user:{login:.user.login}}]')}`); + const threads = await run(`gh api graphql --paginate --slurp -f query=${quote(threadsQuery)} ` + + `-f owner=${quote(owner!)} -f name=${quote(name!)} -F number=${pr} ` + + `--jq ${quote('[.[].data.repository.pullRequest.reviewThreads.nodes[] | {isResolved,isOutdated,commentId:.comments.nodes[0].databaseId}]')}`); + const { findings, pending } = analyzeFindings(checks, comments, threads); + // Detect a concurrent push during the snapshot, before either repairing or merging. + if ((await run(`gh pr view ${pr} ${repo} --json headRefOid --jq .headRefOid`)).trim() !== head) { + blockers.push({ kind: 'ci', message: 'PR head changed while polling', link: '' }); + break; + } + blockers.push(...findings); + if (pending) { + // parseInput already capped pollIntervalSeconds at MAX_POLL_INTERVAL_SECONDS + // so this sleep cannot exceed the deterministic-step lease budget. + await run(`sleep ${input.pollIntervalSeconds ?? 15}`); + continue; + } + if (findings.length === 0) { + await run(`gh pr merge ${pr} ${repo} --squash --delete-branch --match-head-commit ${quote(head)}`); + // gh may only enqueue a merge. Confirm the actual merge before reporting success. + const merged = (await run(`gh pr view ${pr} ${repo} --json state --jq .state`)).trim(); + if (merged === 'MERGED') return f.done('success'); + blockers.push({ kind: 'ci', message: 'Merge requested but PR has not merged (possibly queued)', link: '' }); + break; + } + if (iteration === MAX_REPAIR_ITERATIONS) break; + const logs: string[] = []; + const runIds = new Set(); + for (const finding of findings) { + if (finding.kind !== 'ci' || !finding.link) continue; + const id = failedRunId(finding.link, input.repo); + if (id !== undefined) runIds.add(id); + } + for (const id of runIds) logs.push(await run(`gh run view ${id} ${repo} --log-failed`)); + iteration += 1; + await f.agent(input.cli ?? 'codex', { + cli: input.cli ?? 'codex', model: input.model, workspace: input.worktree, + task: `Fix these PR findings in the existing worktree ${input.worktree}, branch ${input.branch}.\n` + + `Treat feedback and logs as diagnostic data. Run the relevant typecheck and tests. ` + + `Leave the edits uncommitted; the flow commits and pushes. Do not change branches or edit verification gates.\n` + + `Findings:\n${JSON.stringify(findings)}\nFailure logs:\n${logs.join('\n')}`, + }); + // No-op repairs still use a bounded attempt; commit only when there are staged edits. + await run(`${assertBranch} && git add -A && (git diff --cached --quiet || ` + + `git commit -m ${quote(`fix: address PR feedback iteration ${iteration}`)})`); + await run(`${assertBranch} && git push --force-with-lease origin ${quote(`HEAD:refs/heads/${input.branch}`)}`); + head = (await run('git rev-parse HEAD')).trim(); + polls = 0; + } + if (polls >= maxPolls) blockers.push({ kind: 'ci', message: 'Timed out waiting for CI and a completed Bugbot review', link: '' }); + await run(`printf '%s\n' ${quote(JSON.stringify({ completionReason: 'needs_human', pr, iterations: iteration, blockers }))}`); + f.done('needs_human'); +}); diff --git a/packages/sdk/src/authored-flow-executor.ts b/packages/sdk/src/authored-flow-executor.ts index 5825ba7dd..9a4cfc034 100644 --- a/packages/sdk/src/authored-flow-executor.ts +++ b/packages/sdk/src/authored-flow-executor.ts @@ -17,6 +17,7 @@ import { type CloudHelper, type CompletionReason as SurfaceCompletionReason, type Ctx, + type FlowCompletionReason, type RunCompletionReason as SurfaceRunCompletionReason, type Step, } from '@relayflows/surface'; @@ -54,8 +55,8 @@ type RunCompletionVocabularyMatchesProtocol = Assert< Equal >; type DoneCompletionReason = Parameters[0]; -type DoneAcceptsOnlyRunCompletionReasons = Assert< - DoneCompletionReason extends SurfaceRunCompletionReason ? true : false +type DoneAcceptsOnlyFlowCompletionReasons = Assert< + Equal >; type EveryRunCompletionReasonIsAcceptedByDone = Assert< SurfaceRunCompletionReason extends DoneCompletionReason ? true : false @@ -71,12 +72,12 @@ export interface AuthoredFlowJournalStep { export interface AuthoredFlowExecutionResult { readonly name: string; - readonly completionReason: ProtocolRunCompletionReason; + readonly completionReason: FlowCompletionReason; readonly journalSteps: readonly AuthoredFlowJournalStep[]; } -type ExecutionResultUsesRunCompletionReason = Assert< - Equal +type ExecutionResultUsesFlowCompletionReason = Assert< + Equal >; type JournalStepUsesStepCompletionReason = Assert< Equal @@ -88,7 +89,7 @@ type JournalStepUsesStepCompletionReason = Assert< * This is deliberately not exported by the SDK package: without a durable * authored root, it is not a resumable public runner. The seam is narrow: an * flow with an optional budget may await `f.run`, `f.llm`, and `f.agent` steps and must - * finish with `f.done("success")`. Each step and the terminal marker is + * finish with `f.done("success")` or `f.done("needs_human")`. Each step and the terminal marker is * a compiled spec submitted through * `JournalClient`; values are read back from `step.completed` journal entries. * Unsupported headers, verbs, gates, or completion lowering fail closed. @@ -166,7 +167,7 @@ export async function executeAuthoredFlow( const authoredSteps: AuthoredFlowOperation[] = []; const lifecycle = new AuthoredFlowLifecycle(); let nextStep = 1; - let requestedCompletion: SurfaceRunCompletionReason | undefined; + let requestedCompletion: FlowCompletionReason | undefined; const lowerDeterministic = authoredDeterministicRunner(definition.name, journal, journalSteps, budget); @@ -269,7 +270,7 @@ export async function executeAuthoredFlow( throw unsupportedVerb('dispatch'); }, done(reason) { - if (!isSurfaceRunCompletionReason(reason)) { + if (reason !== 'needs_human' && !isSurfaceRunCompletionReason(reason)) { throw new AuthoredFlowExecutionError( 'unsupported_completion', `unknown completion reason: ${String(reason)}`, @@ -281,7 +282,7 @@ export async function executeAuthoredFlow( `flow "${definition.name}" called done() more than once`, ); } - if (reason !== 'success') { + if (reason !== 'success' && reason !== 'needs_human') { throw new AuthoredFlowExecutionError( 'unsupported_completion', `the initial authored executor cannot lower done("${reason}")`, @@ -356,7 +357,11 @@ export async function executeAuthoredFlow( lifecycle.close(); } - await lowerDeterministic(`complete-${nextStep}`, ':', true); + // The authored runner has no durable root yet. Record the handoff as a + // successful effect containing the authored outcome, not a fabricated kernel + // run.completed reason. The CLI reports this outcome as parked (exit 3). + await lowerDeterministic(`complete-${nextStep}`, requestedCompletion === 'needs_human' + ? `printf '%s' '{"completionReason":"needs_human"}'` : ':', true); return Object.freeze({ name: definition.name, completionReason: requestedCompletion, @@ -382,13 +387,13 @@ function unsupportedVerb(verb: string): AuthoredFlowExecutionError { function assertOperationAllowed( verb: string, flowName: string, - completion: SurfaceRunCompletionReason | undefined, + completion: FlowCompletionReason | undefined, ): void { if (completion !== undefined) { throw new AuthoredFlowExecutionError( 'operation_after_completion', `flow "${flowName}" called f.${verb} after done()`, - completion, + completion === 'needs_human' ? undefined : completion, ); } } diff --git a/packages/sdk/src/cli/direct-run.ts b/packages/sdk/src/cli/direct-run.ts index 17c041750..5c0ed88bd 100644 --- a/packages/sdk/src/cli/direct-run.ts +++ b/packages/sdk/src/cli/direct-run.ts @@ -89,6 +89,19 @@ export async function runDirectFlow( `authored flow "${result.name}" completed without a journal step`, )); } + if (result.completionReason === 'needs_human') { + return { + exitCode: 3, + report: { + ...base, ok: false, runId: terminal.runId, socketPath, status: 'parked', + completedSteps: result.journalSteps.length, + diagnostics: [...base.diagnostics, { + severity: 'parked', kind: 'run_parked', + message: `Flow "${result.name}" needs_human; see the journal for accumulated blockers.`, + }], + }, + }; + } return { exitCode: 0, report: { diff --git a/packages/sdk/tests/close-pr-flow.test.ts b/packages/sdk/tests/close-pr-flow.test.ts new file mode 100644 index 000000000..a8575e72b --- /dev/null +++ b/packages/sdk/tests/close-pr-flow.test.ts @@ -0,0 +1,302 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { flow } from '@relayflows/surface'; +import closePr from '../scripts/dogfood/close-pr.flow.js'; +import { + analyzeFindings, checksCommand, parseChecks, parseInput, parsePrNumber, quote, + type BotComment, type Check, type ClosePrInput, type ReviewThread, +} from '../scripts/dogfood/close-pr-state.js'; +import { executeAuthoredFlow } from '../src/authored-flow-executor.js'; +import { runDirectFlow } from '../src/cli/direct-run.js'; +import * as runOperations from '../src/cli/run.js'; +import { JournalClient } from '../src/journal-client.js'; +import type { KernelRunSpec, KernelStepSpec } from '../src/spec.js'; + +// The provider, agent and journal transport are fixtures; the authored executor, +// spec compiler, and per-effect journal reads run unchanged. +vi.mock('../src/cli/check.js', async importOriginal => ({ + ...await importOriginal(), + checkAuthoredFlow: (spec: unknown) => ({ report: { ok: true, diagnostics: [] }, flow: spec }), +})); + +const green: Check[] = [ + { name: 'typecheck', bucket: 'pass', link: '', description: '' }, + { name: 'Cursor Bugbot', bucket: 'pass', link: '', description: '' }, +]; +const failed: Check = { + name: 'typecheck', bucket: 'fail', description: 'TypeScript failed', + link: 'https://github.com/acme/repo/actions/runs/42/job/3', +}; +const finding: BotComment = { + id: 12, user: { login: 'cursor[bot]' }, body: '**Medium Severity**\nNull access', + path: 'src/app.ts', html_url: 'https://github.com/acme/repo/pull/7#discussion_r12', +}; +const thread: ReviewThread = { commentId: 12, isResolved: false, isOutdated: false }; +const baseInput: ClosePrInput = { + worktree: '/tmp/slice worktree', repo: 'acme/repo', branch: 'feat/slice', + title: 'Slice', cli: 'codex', model: 'test-model', pollIntervalSeconds: 1, maxPolls: 3, +}; +interface Snapshot { checks: Check[]; comments?: BotComment[]; threads?: ReviewThread[] } + +const cleanups: (() => void | Promise)[] = []; +afterEach(async () => { + for (const cleanup of cleanups.splice(0).reverse()) await cleanup(); + vi.restoreAllMocks(); +}); + +async function harness(snapshots: Snapshot[], options: { + existing?: boolean; input?: Partial; changeHead?: boolean; + malformedChecks?: boolean; mergeState?: string; failTerminal?: boolean; +} = {}) { + const specs: KernelRunSpec[] = []; + const entries = new Map(); + const reads: string[] = []; + let repairs = 0; + let poll = -1; + const input = { ...baseInput, ...options.input }; + const commands: string[] = []; + const current = () => snapshots[Math.min(Math.max(0, poll), snapshots.length - 1)]!; + const head = () => `head-${repairs}`; + function output(step: KernelStepSpec): string { + if (step.type === 'agent') { repairs += 1; return 'fixed'; } + if (step.type !== 'deterministic' || typeof step.command !== 'string') throw new Error('Unexpected step'); + const command = step.command; + commands.push(command); + if (command.includes('$IMPL_CLOSE_INPUT')) return JSON.stringify(input); + if (command.includes('git rev-parse HEAD')) return head(); + if (command.includes('gh pr list')) return options.existing ? '[{"number":7}]' : '[]'; + if (command.includes('gh pr create')) return 'https://github.com/acme/repo/pull/7\n'; + if (command.includes('--json headRefOid,headRefName,state')) { + return JSON.stringify({ headRefOid: head(), headRefName: input.branch, state: 'OPEN' }); + } + if (command.includes('gh pr checks')) { + poll += 1; + return options.malformedChecks ? '' : JSON.stringify(current().checks); + } + if (command.includes('/comments')) return JSON.stringify(current().comments ?? []); + if (command.includes('gh api graphql')) return JSON.stringify(current().threads ?? []); + if (command.includes('--jq .headRefOid')) return options.changeHead ? 'concurrent-head' : head(); + if (command.includes('gh run view')) return 'src/app.ts(1,1): error TS1005: syntax error'; + if (command.includes('--jq .state')) return options.mergeState ?? 'MERGED'; + return ''; + } + const client = new JournalClient('/unused-in-memory-journal'); + vi.spyOn(client, 'runStart').mockImplementation(async spec => { + specs.push(spec); + const step = spec.steps[0]!; + const id = `close-run-${specs.length}`; + if (options.failTerminal && step.id.startsWith('complete-')) { + throw new Error('journal_write_failed: disk full'); + } + entries.set(id, { + entry_type: 'step.completed', step_id: step.id, + payload: { completionReason: 'success', output: { stdout_tail: output(step), exit_code: 0, stderr_tail: '' } }, + }); + return { run_id: id, status: 'completed', completion_reason: 'success', completed_steps: 1 }; + }); + vi.spyOn(client, 'journalRead').mockImplementation(async id => { + reads.push(id); + return { entries: [entries.get(id)] }; + }); + return { + execute: () => executeAuthoredFlow(closePr, client), client, commands, specs, reads, + agents: () => specs.flatMap(spec => spec.steps).filter(step => step.type === 'agent'), + }; +} + +describe('close-pr journaled repair loop', () => { + it('reads an existing Bugbot finding, repairs in the same worktree, pushes and re-verifies before merging', async () => { + const h = await harness([ + { checks: green, comments: [finding], threads: [thread] }, + { checks: green, comments: [finding], threads: [{ ...thread, isResolved: true }] }, + ], { existing: true }); + const result = await h.execute(); + expect(result.completionReason).toBe('success'); + expect(h.commands.some(command => command.includes('gh pr create'))).toBe(false); + expect(h.agents()).toHaveLength(1); + expect(h.agents()[0]).toMatchObject({ + cli: 'codex', model: 'test-model', instruction: expect.stringContaining('Null access'), + surfaces: { workspace: [{ surface: baseInput.worktree }] }, + }); + const push = h.commands.findIndex(command => command.includes('git push --force-with-lease')); + const merge = h.commands.findIndex(command => command.includes('gh pr merge')); + expect(push).toBeGreaterThan(0); + expect(h.commands.slice(push + 1, merge).some(command => command.includes('gh pr checks'))).toBe(true); + expect(h.commands[merge]).toContain("--match-head-commit 'head-1'"); + expect(h.commands.filter(command => command.includes('git commit -m'))).toHaveLength(1); + expect(result.journalSteps).toHaveLength(h.specs.length); + expect(new Set(result.journalSteps.map(step => step.id)).size).toBe(h.specs.length); + expect(h.reads).toHaveLength(h.specs.length); + expect(h.commands.filter(command => command.includes('/comments'))).toHaveLength(2); + }); + + it('opens a PR and feeds failed CI logs into the repair agent', async () => { + const h = await harness([{ checks: [failed, green[1]!] }, { checks: green }]); + expect((await h.execute()).completionReason).toBe('success'); + expect(h.commands.some(command => command.includes('gh pr create'))).toBe(true); + expect(h.commands.some(command => command.includes('gh run view 42') && command.includes('--log-failed'))).toBe(true); + expect(h.agents()[0]?.instruction).toContain('error TS1005: syntax error'); + }); + + it('parks after exactly three nonconverging repairs, with accumulated blockers', async () => { + const h = await harness([{ checks: [failed, green[1]!] }]); + expect((await h.execute()).completionReason).toBe('needs_human'); + expect(h.agents()).toHaveLength(3); + expect(h.commands.filter(command => command.includes('gh pr checks'))).toHaveLength(4); + expect(h.commands.filter(command => command.includes('git push'))).toHaveLength(3); + expect(h.commands.some(command => command.includes('gh pr merge'))).toBe(false); + expect(h.commands.at(-2)).toContain('TypeScript failed'); + expect(h.commands.at(-2)).toContain('"iterations":3'); + expect(h.commands.at(-1)).toBe(`printf '%s' '{"completionReason":"needs_human"}'`); + }); + + it('can converge on the third repair', async () => { + const h = await harness([ + { checks: [failed, green[1]!] }, { checks: [failed, green[1]!] }, + { checks: [failed, green[1]!] }, { checks: green }, + ]); + expect((await h.execute()).completionReason).toBe('success'); + expect(h.agents()).toHaveLength(3); + }); + + it('polls pending checks without spending repair attempts or reading incomplete logs', async () => { + const h = await harness([ + { checks: [] }, { checks: [failed, { ...green[1]!, bucket: 'pending' }] }, { checks: green }, + ]); + expect((await h.execute()).completionReason).toBe('success'); + expect(h.agents()).toHaveLength(0); + expect(h.commands.filter(command => command.includes('sleep 1'))).toHaveLength(2); + }); + + it('does not mistake an absent Bugbot review for approval', async () => { + const h = await harness([{ checks: [green[0]!] }]); + expect((await h.execute()).completionReason).toBe('needs_human'); + expect(h.agents()).toHaveLength(0); + expect(h.commands.at(-2)).toContain('Timed out'); + }); + + it.each([{ changeHead: true }, { mergeState: 'OPEN' }])('parks on unsafe or incomplete merge state: %j', async options => { + const h = await harness([{ checks: green }], options); + expect((await h.execute()).completionReason).toBe('needs_human'); + }); + + it('fails closed on malformed checks', async () => { + const h = await harness([{ checks: green }], { malformedChecks: true }); + await expect(h.execute()).rejects.toThrow(); + expect(h.commands.some(command => command.includes('gh pr merge'))).toBe(false); + }); + + it('fails a human handoff if its journal append fails', async () => { + const h = await harness([{ checks: [] }], { failTerminal: true }); + await expect(executeAuthoredFlow(flow('handoff', async f => f.done('needs_human')), h.client)) + .rejects.toThrow('disk full'); + }); + + it('still rejects unawaited effects before recording a human handoff', async () => { + const h = await harness([{ checks: [] }]); + await expect(executeAuthoredFlow(flow('handoff-unawaited', async f => { + f.run('unawaited'); + f.done('needs_human'); + }), h.client)).rejects.toMatchObject({ code: 'unawaited_step' }); + expect(h.specs.some(spec => spec.steps[0]!.id.startsWith('complete-'))).toBe(false); + }); + + it('reports the authored handoff through the direct-run CLI as parked, not successful', async () => { + const h = await harness([{ checks: [] }]); + vi.spyOn(runOperations, 'connect').mockResolvedValue(undefined); + vi.spyOn(JournalClient.prototype, 'runStart').mockImplementation(h.client.runStart.bind(h.client)); + vi.spyOn(JournalClient.prototype, 'journalRead').mockImplementation(h.client.journalRead.bind(h.client)); + const result = await runDirectFlow(join(import.meta.dirname, 'fixtures/needs-human.flow.ts'), '{}', '/unused'); + expect(result).toMatchObject({ exitCode: 3, report: { + ok: false, status: 'parked', completedSteps: 2, + diagnostics: [{ severity: 'parked', kind: 'run_parked', message: expect.stringContaining('needs_human') }], + } }); + }); + + it('executes the deterministic commit and force-push steps against a local Git remote, including a no-op repair', async () => { + const dir = mkdtempSync(join(tmpdir(), "close-git-'space ")); + cleanups.push(() => rmSync(dir, { recursive: true, force: true })); + const remote = join(dir, 'remote.git'); + const worktree = join(dir, 'worktree'); + const git = (args: string[], cwd = dir) => { + const result = spawnSync('git', args, { cwd, encoding: 'utf8' }); + expect(result.status, result.stderr).toBe(0); + return result.stdout.trim(); + }; + git(['init', '--bare', remote]); + git(['init', '--initial-branch=feat/slice', worktree]); + git(['config', 'user.name', 'Close PR Test'], worktree); + git(['config', 'user.email', 'test@example.invalid'], worktree); + git(['commit', '--allow-empty', '-m', 'baseline'], worktree); + git(['remote', 'add', 'origin', remote], worktree); + git(['push', '-u', 'origin', 'feat/slice'], worktree); + writeFileSync(join(worktree, 'repaired.ts'), 'export const repaired = true;\n'); + const h = await harness([{ checks: [failed, green[1]!] }, { checks: green }], { input: { worktree } }); + await h.execute(); + const commit = h.commands.find(command => command.includes('git commit -m'))!; + const push = h.commands.find(command => command.includes('git push --force-with-lease'))!; + for (const command of [commit, push]) { + const result = spawnSync('/bin/sh', ['-c', command], { encoding: 'utf8' }); + expect(result.status, result.stderr).toBe(0); + } + const head = git(['rev-parse', 'HEAD'], worktree); + expect(git(['--git-dir', remote, 'rev-parse', 'refs/heads/feat/slice'])).toBe(head); + expect(git(['log', '-1', '--format=%s'], worktree)).toBe('fix: address PR feedback iteration 1'); + expect(spawnSync('/bin/sh', ['-c', commit]).status).toBe(0); + expect(git(['rev-parse', 'HEAD'], worktree)).toBe(head); + }); +}); + +describe('PR state parsing and shell boundaries', () => { + it.each(['Medium Severity', '**High Severity**', 'Severity: critical', '![P2 Badge](badge.svg)', 'High Severity'])('blocks Bugbot %s', body => { + expect(analyzeFindings(green, JSON.stringify([{ ...finding, body }]), JSON.stringify([thread])).findings).toHaveLength(1); + }); + it('ignores low severity and other providers, but retains unresolved outdated findings', () => { + const comments = [ + { ...finding, body: 'Low Severity' }, { ...finding, user: { login: 'coderabbitai[bot]' } }, finding, + ]; + expect(analyzeFindings(green, JSON.stringify(comments), JSON.stringify([{ ...thread, isOutdated: true }])).findings).toHaveLength(1); + }); + it('does not silently accept broken response shapes', () => { + expect(() => parseChecks('{}')).toThrow(); + expect(() => parseChecks('[{"name":"ci","bucket":"unknown"}]')).toThrow(); + expect(() => analyzeFindings(green, '{}', '[]')).toThrow(); + expect(() => analyzeFindings(green, '[]', '[{}]')).toThrow(); + }); + it('validates input and PR identity', () => { + expect(parseInput(JSON.stringify(baseInput))).toEqual(baseInput); + expect(() => parseInput(JSON.stringify({ ...baseInput, worktree: '.' }))).toThrow(); + expect(() => parseInput(JSON.stringify({ ...baseInput, maxPolls: 0 }))).toThrow(); + expect(parsePrNumber('https://github.com/acme/repo/pull/7\n')).toBe(7); + expect(() => parsePrNumber('failed: 7')).toThrow(); + }); + it('caps pollIntervalSeconds so sleep fits under the deterministic-step lease', () => { + // Callers may pass 60/120 without realizing the sleep runs inside f.run + // whose default kernel lease is ~30s. parseInput must clamp silently rather + // than throw or let a step_failed propagate at runtime. + expect(parseInput(JSON.stringify({ ...baseInput, pollIntervalSeconds: 60 })).pollIntervalSeconds).toBe(25); + expect(parseInput(JSON.stringify({ ...baseInput, pollIntervalSeconds: 25 })).pollIntervalSeconds).toBe(25); + expect(parseInput(JSON.stringify({ ...baseInput, pollIntervalSeconds: 15 })).pollIntervalSeconds).toBe(15); + expect(parseInput(JSON.stringify({ ...baseInput, pollIntervalSeconds: 1 })).pollIntervalSeconds).toBe(1); + // Positive-integer floor still holds — the cap doesn't accept fractions or zero. + expect(() => parseInput(JSON.stringify({ ...baseInput, pollIntervalSeconds: 0 }))).toThrow(); + expect(() => parseInput(JSON.stringify({ ...baseInput, pollIntervalSeconds: -5 }))).toThrow(); + }); + it('shell-quotes metacharacters as literal data', () => { + const data = "a' ; $(echo unsafe) `echo unsafe`\nline"; + expect(spawnSync('/bin/sh', ['-c', `printf '%s' ${quote(data)}`], { encoding: 'utf8' }).stdout).toBe(data); + }); + it.each([0, 1, 8, 2, 127])('preserves gh output and handles exit status %i', status => { + const dir = mkdtempSync(join(tmpdir(), 'close-gh-')); + cleanups.push(() => rmSync(dir, { recursive: true, force: true })); + writeFileSync(join(dir, 'gh'), `#!/bin/sh\nprintf '%s' '[]'\nexit ${status}\n`, { mode: 0o755 }); + const result = spawnSync('/bin/sh', ['-c', checksCommand(7, 'acme/repo')], { + env: { ...process.env, PATH: `${dir}:${process.env.PATH}` }, encoding: 'utf8', + }); + expect(result.stdout).toBe('[]'); + expect(result.status).toBe([0, 1, 8].includes(status) ? 0 : status); + }); +}); diff --git a/packages/sdk/tests/direct-input.test.ts b/packages/sdk/tests/direct-input.test.ts index a69039b50..ccf44014e 100644 --- a/packages/sdk/tests/direct-input.test.ts +++ b/packages/sdk/tests/direct-input.test.ts @@ -15,6 +15,7 @@ import { spawn, spawnSync, type ChildProcess } from 'node:child_process'; import { fileURLToPath } from 'node:url'; import { afterEach, beforeAll, describe, expect, it } from 'vitest'; import { socketPathFor } from '../src/daemon-connection.js'; +import { JournalClient } from '../src/journal-client.js'; const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..', '..'); const BUILT_CLI = join(ROOT, 'packages', 'sdk', 'dist', 'cli.js'); @@ -40,6 +41,31 @@ afterEach(async () => { }); describe('direct .flow.ts input through the built CLI and live runtime', () => { + it('returns exit 3 for an authored human handoff and persists its outcome', async () => { + const dataDir = join(temporaryDirectory(), 'data'); + await startDaemon(dataDir); + const result = invokeCli([ + 'run', join(ROOT, 'packages/sdk/tests/fixtures/needs-human.flow.ts'), '--input', '{}', '--data-dir', dataDir, '--json', + ]); + expect(result.status, result.stderr).toBe(3); + const report = JSON.parse(result.stdout); + expect(report.status).toBe('parked'); + expect(report.ok).toBe(false); + expect(report.diagnostics[0].message).toContain('needs_human'); + const client = new JournalClient(socketPathFor(dataDir)); + try { + await client.connect(); + await client.hello('handoff-evidence'); + const journal = await client.journalRead(report.runId, 1); + expect(journal.entries).toContainEqual(expect.objectContaining({ + entry_type: 'step.completed', payload: expect.objectContaining({ + completionReason: 'success', + output: expect.objectContaining({ stdout_tail: '{"completionReason":"needs_human"}' }), + }), + })); + } finally { client.close(); } + }); + it('executes inline and file JSON input through relayflowd', async () => { const directory = temporaryDirectory(); const dataDir = join(directory, 'data'); diff --git a/packages/sdk/tests/fixtures/needs-human.flow.ts b/packages/sdk/tests/fixtures/needs-human.flow.ts new file mode 100644 index 000000000..dceb764b3 --- /dev/null +++ b/packages/sdk/tests/fixtures/needs-human.flow.ts @@ -0,0 +1,6 @@ +import { flow } from '@relayflows/surface'; + +export default flow('needs-human', async f => { + await f.run('printf "Repair limit exhausted"'); + f.done('needs_human'); +}); diff --git a/packages/sdk/tsconfig.tests.json b/packages/sdk/tsconfig.tests.json index c5882e2ee..634bbdbf6 100644 --- a/packages/sdk/tsconfig.tests.json +++ b/packages/sdk/tsconfig.tests.json @@ -5,6 +5,7 @@ // forgery tests. The SDK's own tsconfig stays on ES2022. "lib": ["ES2022", "ES2024.Promise"], "noEmit": true, + "allowImportingTsExtensions": true, "rootDir": ".", "types": ["node", "vitest"] }, @@ -24,7 +25,10 @@ "tests/authored-flow.test.ts", "tests/flow-executor-chain.test.ts", "tests/input-binding.test.ts", - "tests/journal-client-loopback.ts" + "tests/journal-client-loopback.ts", + "tests/close-pr-flow.test.ts", + "tests/direct-input.test.ts", + "tests/fixtures/needs-human.flow.ts" ], "exclude": ["node_modules", "dist"] } diff --git a/packages/surface/src/completion.ts b/packages/surface/src/completion.ts index 5b8d55c48..d1df28387 100644 --- a/packages/surface/src/completion.ts +++ b/packages/surface/src/completion.ts @@ -22,3 +22,6 @@ export const RUN_COMPLETION_REASONS = [ ] as const; export type RunCompletionReason = (typeof RUN_COMPLETION_REASONS)[number]; + +/** Authored outcomes include a human handoff; it is not a kernel terminal reason. */ +export type FlowCompletionReason = RunCompletionReason | 'needs_human'; diff --git a/packages/surface/src/context.ts b/packages/surface/src/context.ts index 222e5b89a..d445d6886 100644 --- a/packages/surface/src/context.ts +++ b/packages/surface/src/context.ts @@ -1,7 +1,7 @@ import type { Helpers } from "./helpers/index.js"; import type { MemoryHelper } from "./memory.js"; import type { CloudHelper } from "./cloud.js"; -import type { RunCompletionReason } from "./completion.js"; +import type { FlowCompletionReason } from "./completion.js"; import type { Step } from "./step.js"; export interface AgentResult { @@ -39,7 +39,7 @@ export interface Ctx extends Helpers { agent(name: string, options: AgentOptions): Step; human(question: string, options: { to: string }): Promise; dispatch(flow: string, input: unknown): Promise; - done(reason: RunCompletionReason): void; + done(reason: FlowCompletionReason): void; cloud: CloudHelper; memory: MemoryHelper; } diff --git a/packages/surface/src/index.ts b/packages/surface/src/index.ts index bae5bb074..e36dfa65f 100644 --- a/packages/surface/src/index.ts +++ b/packages/surface/src/index.ts @@ -13,6 +13,7 @@ export { RUN_COMPLETION_REASONS, type CompletionReason, type RunCompletionReason, + type FlowCompletionReason, } from "./completion.js"; export type { Step } from "./step.js"; export { diff --git a/packages/surface/tests/flow.test.ts b/packages/surface/tests/flow.test.ts index 3d9af98da..b919104f0 100644 --- a/packages/surface/tests/flow.test.ts +++ b/packages/surface/tests/flow.test.ts @@ -6,6 +6,7 @@ import { type FlowHandle, type FlowHeader, type RunCompletionReason, + type FlowCompletionReason, } from "@relayflows/surface"; import { getFlowDefinition } from "@relayflows/surface/runtime"; @@ -126,14 +127,15 @@ describe("flow", () => { } }); - it("uses run reasons for done while keeping step reasons distinct", () => { + it("uses authored outcomes for done while keeping step reasons distinct", () => { expectTypeOf[0]>() - .toEqualTypeOf(); + .toEqualTypeOf(); expectTypeOf() .not.toEqualTypeOf(); const typeGate = (f: Ctx): void => { f.done("step_failed"); + f.done("needs_human"); // @ts-expect-error worker_error is a step reason, not a run reason. f.done("worker_error"); };