diff --git a/app/e2e/publish-failed.spec.ts b/app/e2e/publish-failed.spec.ts new file mode 100644 index 00000000..94f057b1 --- /dev/null +++ b/app/e2e/publish-failed.spec.ts @@ -0,0 +1,48 @@ +import { expect, mockOnly, test } from "./fixtures"; + +// An attempts run whose pick's pull request didn't open (#138): Review's +// Compare says publishing failed, not the agents' work, and offers to retry +// or finish without one. A run on a repository GitHub doesn't host +// finishes with its branch pushed and says so. + +test.beforeEach(async ({ app }) => { + mockOnly("the mock's runs"); + await app.open(); + await app.page.getByTestId("nav-review").click(); +}); + +test("a pull request that didn't open can be retried from Compare, with the pick kept", async ({ app }) => { + const page = app.page; + await page.getByRole("button", { name: "search: 2 attempts · publishing failed" }).click(); + const note = page.getByRole("region", { name: "The pull request didn't open" }); + await expect(note).toContainText("The agents' work is done and your pick is kept; only publishing failed: gh pr create failed."); + await expect(note).toContainText("HTTP 401: Bad credentials"); + // The pick is still the pick, and there is nothing to pick again. + await expect(page.getByText("picked", { exact: true })).toBeVisible(); + await expect(page.getByRole("button", { name: "Pick this one" })).toHaveCount(0); + await note.getByRole("button", { name: "Retry publishing" }).click(); + await expect(page.getByText("Trying the pull request again")).toBeVisible(); + const pr = page.getByRole("region", { name: "Pull request" }); + await expect(pr).toContainText("Pull request: opened"); + await expect(pr.getByRole("button", { name: "Open pull request" })).toBeVisible(); + await expect(note).toHaveCount(0); +}); + +test("a pull request that didn't open can be skipped: the run finishes without one", async ({ app }) => { + const page = app.page; + await page.getByRole("button", { name: "search: 2 attempts · publishing failed" }).click(); + const note = page.getByRole("region", { name: "The pull request didn't open" }); + await note.getByRole("button", { name: "Finish without a pull request" }).click(); + await expect(page.getByText("The pick is kept, and the other attempts are archived.")).toBeVisible(); + await expect(page.getByRole("region", { name: "Pull request" })).toContainText("Finished without a pull request (gh pr create failed)."); + await expect(page.getByText(/succeeded · started/)).toBeVisible(); +}); + +test("an attempts run on a Gitea repository says its branch is pushed and links to the forge", async ({ app }) => { + const page = app.page; + await page.getByRole("button", { name: "invoices: 2 attempts" }).click(); + const pr = page.getByRole("region", { name: "Pull request" }); + await expect(pr).toContainText("This repository is on git.acme.dev, not GitHub, so Shipyard didn't open a pull request."); + await expect(pr.getByRole("button", { name: "Open git.acme.dev" })).toBeVisible(); + await expect(page.getByText(/succeeded · started/)).toBeVisible(); +}); diff --git a/app/src/lib/mock-runs.ts b/app/src/lib/mock-runs.ts index 4d1cbedb..778227da 100644 --- a/app/src/lib/mock-runs.ts +++ b/app/src/lib/mock-runs.ts @@ -1,8 +1,10 @@ import { ApiError, type BerthEvent, type Run, type RunCandidate, type RunStep, type RunSummary } from "@/lib/api"; // Runs in the demo: the shop world on devl has a loop going, an attempts run -// of three agents waiting for you to pick one in Review's Compare, a flow's -// run, a review and a finished broadcast. Deciding, cancelling and starting +// of three agents waiting for you to pick one in Review's Compare, one whose +// pick's pull request didn't open (retry, or finish without one), one on a +// Gitea repository that finished with its branch pushed and no pull +// request, a flow's run, a review and a finished broadcast. Deciding, cancelling and starting // runs work, so the loops panel, Automations → Runs, Compare and the phone // all have something to show. @@ -155,12 +157,81 @@ const gpuAttempts = (): Run => ({ steps: [step("1", "judge", "succeeded", 10, 8, { id: "judge", output: "Winner: attempt 1. passes; adds a migration it doesn't need" })], }); +// An attempts run's candidate in another task's worktrees. +const renamed = (c: RunCandidate, name: string, picked?: boolean): RunCandidate => { + const wt = `${name}-a${c.index + 1}`; + return { ...c, worktree: wt, path: `/home/me/work/shop-${wt}`, branch: wt, session: `${wt}-${c.agent}-x7k2-0i${c.index}0`, picked }; +}; + +// The steps of an attempts run of two that got as far as its pull request. +const pickedSteps = (name: string, start: number, pr: RunStep, tidy?: RunStep): RunStep[] => [ + step("0", "map", "succeeded", start, start - 30, { id: "attempts", output: "1. attempt 1 (claude): check passed\n2. attempt 2 (codex): check passed" }), + step("1", "judge", "succeeded", start - 30, start - 31, { id: "judge", output: "Winner: attempt 1. passes with the smaller diff" }), + step("2", "if", "succeeded", start - 31, start - 33, { id: "choose", children: [step("2.t", "then", "", start - 31, start - 33, { children: [step("2.t.0", "gate", "succeeded", start - 31, start - 33, { id: "pick", output: `Pick the best attempt at ${name}` })] })] }), + step("3", "if", pr.status === "failed" ? "running" : "succeeded", start - 33, pr.status === "failed" ? undefined : start - 34, { id: "pr", children: [step("3.t", "then", "", start - 33, undefined, { children: [pr] })] }), + ...(tidy ? [tidy] : []), +]; + +// Picked, but its pull request didn't open (GitHub said no): publishing +// failed, not the agents' work, and it waits to retry or finish without one. +const publishRun = (): Run => ({ + id: "r_mshp9pb4x", + template: "attempts", + title: "search: 2 attempts", + status: "waiting_gate", + origin: "laptop:mac", + cursor: "3.t.0", + created: at(60), + updated: at(4), + params: { location: "shop", name: "search", prompt: "Make catalog search fast for 100k products.", then: { pr: { draft: true } } }, + gate: { + path: "3.t.0.publish1", + title: "The pull request didn't open", + text: "The agents' work is done and your pick is kept; only publishing failed: gh pr create failed.\n\nHTTP 401: Bad credentials (https://api.github.com/graphql)\nTry authenticating with: gh auth login\n\nRetry once it's fixed, or finish without a pull request: the run goes on and archives the other attempts.", + approve_label: "Retry publishing", + reject_label: "Finish without a pull request", + }, + candidates: [ + renamed(candidate(0, "claude", true, 64, 12, 1, "passes with the smaller diff", "perf: index the catalog for search"), "search", true), + renamed(candidate(1, "codex", true, 118, 40, 2, "passes, but caches more than it needs to", "perf: cache search results"), "search"), + ], + steps: pickedSteps("search", 60, step("3.t.0", "pr", "failed", 26, 25.9, { id: "open", attempt: 1, exit_code: 1, error: "gh pr create failed", output: "HTTP 401: Bad credentials (https://api.github.com/graphql)\nTry authenticating with: gh auth login" })), +}); + +// Picked on a repository hosted on Gitea: gh can't open a pull request +// there, so the branch is pushed, the step says so, and the run finished. +const giteaRun = (): Run => ({ + id: "r_mshp9gt3a", + template: "attempts", + title: "invoices: 2 attempts", + status: "succeeded", + origin: "laptop:mac", + created: at(150), + updated: at(110), + finished: at(110), + params: { location: "shop", name: "invoices", prompt: "Add PDF invoices.", then: { pr: { draft: true } } }, + candidates: [ + renamed(candidate(0, "claude", true, 88, 6, 1, "passes with a test for the PDF", "feat: PDF invoices"), "invoices", true), + renamed(candidate(1, "codex", false, 140, 20, 2, "its check fails on the totals", "feat: invoices as PDF"), "invoices"), + ], + steps: pickedSteps( + "invoices", + 150, + step("3.t.0", "pr", "skipped", 116, 115.9, { + id: "open", + output: + "This repository is on git.acme.dev, not GitHub, so Shipyard didn't open a pull request. The branch invoices-a1 is pushed to origin; open one there: https://git.acme.dev/acme/shop/compare/main...invoices-a1. (If git.acme.dev is GitHub Enterprise, run \"gh auth login --hostname git.acme.dev\" on this box.)", + }), + step("4", "if", "succeeded", 115.9, 110, { id: "tidy", children: [step("4.t", "then", "", 115.9, 110, { children: [step("4.t.0", "cleanup", "succeeded", 115.9, 110, { id: "archive", output: "archived invoices-a2" })] })] }), + ), +}); + let gpuRuns: Run[] | undefined; let runs: Run[] | undefined; let round = 2; function world(): Run[] { - if (!runs) runs = [attemptsRun(), loopRun(round), flowRun(), reviewRun()]; + if (!runs) runs = [attemptsRun(), publishRun(), loopRun(round), flowRun(), reviewRun(), giteaRun()]; return runs; } @@ -202,6 +273,23 @@ export function runsCall(box: string, method: string, path: string, body: unknow const r = all.find((x) => x.id === m![1]); const d = body as { approve: boolean; pick?: number }; if (!r?.gate) return Promise.reject(new ApiError("that run is not waiting at a gate", 409)); + if (r.gate.approve_label) { + // The publish gate: a retry opens it this time; finishing without + // one is a skip. Either way the run archives the rest and is done. + const pr = r.steps.find((s) => s.kind === "if" && s.id === "pr")?.children?.[0]?.children?.[0]; + if (pr) + Object.assign( + pr, + d.approve + ? { status: "succeeded", attempt: 2, error: undefined, output: "https://github.com/acme/shop/pull/318", ended: new Date().toISOString() } + : { status: "skipped", error: undefined, output: "Finished without a pull request (gh pr create failed).", ended: new Date().toISOString() }, + ); + const loser = r.candidates?.find((c) => !c.picked)?.worktree; + r.steps.push(step("4", "if", "succeeded", 0, 0, { id: "tidy", children: [step("4.t", "then", "", 0, 0, { children: [step("4.t.0", "cleanup", "succeeded", 0, 0, { id: "archive", output: `archived ${loser}` })] })] })); + Object.assign(r, { status: "succeeded", finished: new Date().toISOString(), updated: new Date().toISOString(), gate: undefined }); + emit({ type: "run.finished", box, data: { run: r.id, status: r.status } }); + return delay({ decided: r.id, approve: d.approve }); + } const pick = d.pick ?? r.gate.default ?? 0; for (const c of r.candidates ?? []) c.picked = d.approve && c.index === pick; Object.assign(r, { status: d.approve ? "succeeded" : "failed", finished: new Date().toISOString(), gate: undefined, error: d.approve ? undefined : "rejected" }); @@ -248,7 +336,8 @@ export function runsCall(box: string, method: string, path: string, body: unknow { path: "src/ledger/entry.ts", code: "M", added: Math.round(c.diff.added * 0.2), removed: Math.round(c.diff.removed * 0.6) }, { path: "test/refunds.test.ts", code: "M", added: Math.round(c.diff.added * 0.25), removed: Math.round(c.diff.removed * 0.4) }, ]; - return delay({ run: summary(r), gate: r.gate, judge: r.steps.find((s) => s.kind === "judge")?.output, candidates: (r.candidates ?? []).map((c) => ({ ...c, review: { files: files(c) } })) }); + const pr = r.steps.find((s) => s.kind === "if" && s.id === "pr")?.children?.[0]?.children?.[0]; + return delay({ run: summary(r), gate: r.gate, judge: r.steps.find((s) => s.kind === "judge")?.output, pr, candidates: (r.candidates ?? []).map((c) => ({ ...c, review: { files: files(c) } })) }); } if (method === "GET" && path === "autofix") return delay([{ path: "/home/me/work/shop-checkout-fix", ci: true, review: false, max: 3 }]); if (method === "PUT" && path === "autofix") return delay([body]); diff --git a/app/src/lib/orchestrate-core.ts b/app/src/lib/orchestrate-core.ts index c15d6dcf..0ac44a48 100644 --- a/app/src/lib/orchestrate-core.ts +++ b/app/src/lib/orchestrate-core.ts @@ -355,6 +355,9 @@ export interface RunCompare { run: RunSummary; gate?: Run["gate"]; judge?: string; + // The pull request step: opened, skipped (a host without them, or + // finished without one) or failed, with what it said. + pr?: Run["steps"][number]; candidates: (NonNullable[number] & { review?: { files?: { path: string; added?: number; removed?: number; status?: string }[]; commits?: { sha: string; subject: string }[] } })[]; } diff --git a/app/src/views/automations/flows/runs-tab.tsx b/app/src/views/automations/flows/runs-tab.tsx index ad098cfc..ce88ed1e 100644 --- a/app/src/views/automations/flows/runs-tab.tsx +++ b/app/src/views/automations/flows/runs-tab.tsx @@ -320,11 +320,11 @@ export function RunTimeline({ box, id, summary }: { box: string; id: string; sum ) : ( )} - @@ -358,7 +358,9 @@ function Steps({ steps, t0, span, depth }: { steps: RunStep[]; t0: number; span: return (
0 && "border-l pl-3")}> {steps - .filter((s) => s.status !== "skipped") + // A step its when rule skipped says nothing; one that chose not to + // act (a pull request on Gitea) says why. + .filter((s) => s.status !== "skipped" || s.output) .map((s) => ( ))} diff --git a/app/src/views/review/compare-view.tsx b/app/src/views/review/compare-view.tsx index 34cdcf82..8cc92bca 100644 --- a/app/src/views/review/compare-view.tsx +++ b/app/src/views/review/compare-view.tsx @@ -1,4 +1,4 @@ -import { ArrowLeftIcon, CheckIcon, Columns2Icon, CrownIcon, GitCompareArrowsIcon, SquareTerminalIcon, XIcon } from "lucide-react"; +import { ArrowLeftIcon, CheckIcon, Columns2Icon, CrownIcon, ExternalLinkIcon, GitCompareArrowsIcon, GitPullRequestIcon, RotateCcwIcon, SquareTerminalIcon, XIcon } from "lucide-react"; import { useEffect, useState } from "react"; import { AgentIcon } from "@/components/agent-glyph"; @@ -16,6 +16,7 @@ import { openCompare } from "@/lib/compare-actions"; import { usePrefs } from "@/lib/prefs"; import { tokens } from "@/views/automations/flows/runs-tab"; import { ErrorText } from "@/components/error-note"; +import { openUrl } from "@/lib/open-url"; // CompareStrip lists attempts runs with candidates to compare, at the top of // Review: the ones waiting for a pick first. @@ -28,13 +29,60 @@ export function CompareStrip({ runs, onOpen }: { runs: BoxRun[]; onOpen(r: BoxRu {runs.map((r) => ( ))}
); } +// PullRequestNote says what became of the pick's pull request: that it +// didn't open (publishing failed, not the agents' work), with retry and +// finish without one; or, once the run is past it, what the step said (on +// Gitea, say, the branch is pushed and the link is the forge's own). +function PullRequestNote({ data, where, busy, onPublish }: { data: RunCompare; where?: string; busy: boolean; onPublish(retry: boolean): void }) { + const gate = data.gate; + if (gate && !gate.pick && gate.approve_label) { + return ( +
+

+ + {gate.title} + {where && on {where}} +

+ {gate.text &&

{gate.text}

} +
+ + +
+
+ ); + } + const pr = data.pr; + if (gate || !pr?.output || (pr.status !== "succeeded" && pr.status !== "skipped")) return null; + const link = /https?:\/\/[^\s'"<>)]+/.exec(pr.output)?.[0]?.replace(/[.,;]+$/, ""); + return ( +
+ + + Pull request{where ? ` on ${where}` : ""}: + {pr.status === "succeeded" ? "opened" : pr.output} + + {link && ( + + )} +
+ ); +} + // CompareView is Review's Compare mode for an attempts run: a column per // attempt, with its check, diffstat, commits, the judge's ranking and // reasons, and the files it changed. Picking one decides the run's gate; @@ -69,7 +117,7 @@ export function CompareView({ box, id, onClose }: { box: string; id: string; onC setBusy(true); try { for (const r of all) { - if (!r.data.gate) continue; + if (!r.data.gate?.pick) continue; const mine = at && r.box === at.box && r.id === at.id; await runsApi.decide(r.box, r.id, approve ? { approve: true, pick: mine ? at.index : -1 } : { approve: false }); scheduleRuns(r.box, 0); @@ -83,6 +131,22 @@ export function CompareView({ box, id, onClose }: { box: string; id: string; onC } }; + // A pull request that didn't open waits at its own gate: the pick is in, + // so this only publishes again or lets the run finish without one. + const publish = async (r: (typeof all)[number], retry: boolean) => { + setBusy(true); + try { + await runsApi.decide(r.box, r.id, { approve: retry }); + scheduleRuns(r.box, 0); + toastManager.add({ type: "success", title: retry ? "Trying the pull request again" : "Finished without a pull request", description: retry ? undefined : "The pick is kept, and the other attempts are archived." }); + await load(); + } catch (err) { + toastManager.add({ type: "error", title: "Couldn't decide", description: errorMessage(err) }); + } finally { + setBusy(false); + } + }; + const data = all.find((r) => r.box === box && r.id === id)?.data ?? all[0]?.data; const canPick = all.some((r) => r.data.gate?.pick); const multi = all.length > 1; @@ -147,6 +211,9 @@ export function CompareView({ box, id, onClose }: { box: string; id: string; onC )} {data && (
+ {all.map((r) => ( + void publish(r, retry)} /> + ))} {all .filter((r) => r.data.judge) .map((r) => ( diff --git a/docs/changelog.mdx b/docs/changelog.mdx index 6540e22f..bbc93f91 100644 --- a/docs/changelog.mdx +++ b/docs/changelog.mdx @@ -10,7 +10,18 @@ upgrades with `berth upgrade BOX`, or by running the again. ## Unreleased - +- **Attempts on a repository GitHub doesn't host finish with your pick.** + On Gitea, Forgejo, GitLab or a folder, picking an attempt with a draft PR + turned on failed the run at `gh pr create` and left every worktree + behind. Shipyard now pushes the picked branch, says plainly that it + didn't open a pull request there (with the forge's own link to open one + when it prints it), archives the other attempts and finishes the run. +- **A pull request that doesn't open no longer fails the run.** When `gh` + can't open the pick's pull request (a sign-in that ran out, say), the + run waits with your pick kept and nothing archived, and says it is + publishing that failed, not the agents' work. Review's Compare and + Automations → Runs offer **Retry publishing** or **Finish without a pull + request**; `berth run approve` and `berth run reject` do the same. - **A saved Compare tab no longer leaves the window empty.** Quitting with a Compare tab open left the next launch, and every one after, on a dark grey window with no sidebar, until the saved tab was removed by hand. It diff --git a/docs/guides/runs.mdx b/docs/guides/runs.mdx index fcf65e73..4b8cd8d3 100644 --- a/docs/guides/runs.mdx +++ b/docs/guides/runs.mdx @@ -110,6 +110,17 @@ POST /v1/runs 6. The pick gets a draft PR (`then.pr`), and the other worktrees go through the project's archive script (`cleanup`). +**The pull request.** The `pr` step pushes the pick's branch and opens the +pull request with `gh`, on github.com or a GitHub Enterprise host `gh` is +signed in to. On any other host (Gitea, Forgejo, GitLab, a folder) it pushes +the branch and stops there: the step is `skipped`, says why, and passes on +the link the forge printed on push for opening one by hand. The run goes on +and archives the other attempts. If `gh` fails, that is publishing failing, +not the agents' work: the run waits at a gate, "The pull request didn't +open", with `gh`'s words, the pick kept and nothing archived. **Retry +publishing** (`berth run approve`) tries again; **Finish without a pull +request** (`berth run reject`) skips it, and the run goes on and succeeds. + In the app, **Try N ways…** in ⌘K or a session's Orchestrate menu starts one. @@ -139,6 +150,7 @@ were, and the step that was in flight follows its kind's rule: | `headless` | Adopts the answer if the agent finished, or keeps waiting for it; never asks again | | `notify`, `webhook` | Sent again with the same `idem_key` (an `Idempotency-Key` header on webhooks) | | `gate`, `sleep` | Their deadlines are absolute, kept in the journal | +| `pr` | Pushes again and adopts the pull request if it opened. One that waits at "The pull request didn't open" waits there again, without trying `gh` by itself | ## Admission, concurrency and budgets diff --git a/internal/box/prhost.go b/internal/box/prhost.go new file mode 100644 index 00000000..8f21d8d0 --- /dev/null +++ b/internal/box/prhost.go @@ -0,0 +1,153 @@ +package box + +import ( + "bufio" + "context" + "net/url" + "os" + "os/exec" + "regexp" + "strings" + "time" +) + +// forge is where a repository's origin lives, as a pr step sees it. +type forge struct { + // Host is origin's host ("gitea.acme.dev"), or "" when origin is a + // folder or there is no origin. + Host string + // Local is set when origin is a folder on this box. + Local bool + // GitHub is set when gh can open a pull request there: github.com, or + // a host gh is signed in to (GitHub Enterprise). + GitHub bool +} + +// originForge reads where the checkout at dir pushes to and whether gh +// can open a pull request there. Nothing is fetched or pushed. +func originForge(ctx context.Context, dir string) forge { + remote := remoteURL(ctx, dir) + if remote == "" { + // No origin: the push says so. + return forge{GitHub: true} + } + host, ssh := remoteHost(remote) + if host == "" { + return forge{Local: true} + } + if ssh { + // An SSH alias (Host work in ~/.ssh/config) names its real host, + // as gh reads it. + host = sshHostname(ctx, host) + } + f := forge{Host: host} + f.GitHub = isGitHubHost(host) || ghSignedIn(ctx, dir, host) + return f +} + +// remoteHost is a remote URL's host, lower-cased, and whether it is +// reached over SSH; "" for a path. +func remoteHost(remote string) (string, bool) { + if m := scpLike.FindStringSubmatch(remote); m != nil { + return strings.ToLower(m[1]), true + } + u, err := url.Parse(remote) + if err != nil || u.Host == "" || u.Scheme == "file" { + return "", false + } + return strings.ToLower(u.Hostname()), u.Scheme == "ssh" || strings.HasPrefix(u.Scheme, "git+ssh") +} + +func isGitHubHost(host string) bool { + return host == "github.com" || host == "www.github.com" || host == "ssh.github.com" || strings.HasPrefix(host, "github.com-") +} + +// sshHostname is what ssh would connect to for alias, from ssh -G (which +// reads the config and connects to nothing). +func sshHostname(ctx context.Context, alias string) string { + bin, err := exec.LookPath("ssh") + if err != nil { + return alias + } + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + out, err := exec.CommandContext(ctx, bin, "-G", "--", alias).Output() + if err != nil { + return alias + } + sc := bufio.NewScanner(strings.NewReader(string(out))) + for sc.Scan() { + if k, v, ok := strings.Cut(sc.Text(), " "); ok && k == "hostname" && v != "" { + return strings.ToLower(strings.TrimSpace(v)) + } + } + return alias +} + +// ghSignedIn says whether gh has an account on host, the way gh itself +// would find a GitHub Enterprise remote. +func ghSignedIn(ctx context.Context, dir, host string) bool { + bin, err := toolPath("gh") + if err != nil { + return false + } + ctx, cancel := context.WithTimeout(ctx, 15*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, "auth", "status", "--hostname", host) + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GH_PROMPT_DISABLED=1", "GIT_TERMINAL_PROMPT=0", "NO_COLOR=1") + return cmd.Run() == nil +} + +var ( + remoteLink = regexp.MustCompile(`https?://[^\s'"<>]+`) + userinfo = regexp.MustCompile(`(\b[a-z][a-z0-9+.-]*://)[^/\s@'"]+@`) +) + +// noUserinfo takes any user and password out of the URLs in a tool's +// output, so a token in a remote URL is never kept in a run. +func noUserinfo(s string) string { return userinfo.ReplaceAllString(s, "$1") } + +// pushLink is the link a forge prints on push to open a pull or merge +// request for the branch (Gitea, Forgejo, GitLab and Bitbucket all do), +// without any credentials in it; "" when it printed none. +func pushLink(out string) string { + for _, l := range strings.Split(out, "\n") { + l = strings.TrimSpace(l) + if !strings.HasPrefix(l, "remote:") { + continue + } + m := remoteLink.FindString(l) + if m == "" { + continue + } + u, err := url.Parse(m) + if err != nil || u.Host == "" { + continue + } + u.User = nil + return u.String() + } + return "" +} + +// noPRText says why a pr step opened nothing, and where the branch is. +func noPRText(f forge, branch, link string) string { + var b strings.Builder + if f.Local { + b.WriteString("This repository's origin is a folder on this box, not GitHub, so Shipyard didn't open a pull request. ") + } else { + b.WriteString("This repository is on " + f.Host + ", not GitHub, so Shipyard didn't open a pull request. ") + } + b.WriteString("The branch " + branch + " is pushed to origin") + if link != "" { + b.WriteString("; open one there: " + link) + } else if !f.Local { + b.WriteString(", ready for a pull request on " + f.Host) + } + b.WriteString(".") + if !f.Local { + b.WriteString(" (If " + f.Host + " is GitHub Enterprise, run \"gh auth login --hostname " + f.Host + "\" on this box.)") + } + return b.String() +} diff --git a/internal/box/prhost_test.go b/internal/box/prhost_test.go new file mode 100644 index 00000000..f433afa6 --- /dev/null +++ b/internal/box/prhost_test.go @@ -0,0 +1,185 @@ +package box + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/cosscom/shipyard/internal/box/runs" + "github.com/cosscom/shipyard/internal/events" +) + +// prCheckout is a clone whose origin reads as remote, while its pushes go +// to a bare repository here (pushInsteadOf), with the branch committed. +// hook, when set, is the bare repository's post-receive: what a forge +// says back on a push. +func prCheckout(t *testing.T, remote, branch, hook string) (dir, bare string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + root, _ := filepath.EvalSymlinks(t.TempDir()) + bare, dir = filepath.Join(root, "shop.git"), filepath.Join(root, "shop") + run := func(in string, args ...string) { + cmd := exec.Command("git", args...) + cmd.Dir = in + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.com", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.com") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %s", args, out) + } + } + run(root, "init", "-q", "--bare", "-b", "main", bare) + if hook != "" { + os.WriteFile(filepath.Join(bare, "hooks", "post-receive"), []byte("#!/bin/sh\n"+hook), 0o755) + } + os.MkdirAll(dir, 0o755) + run(dir, "init", "-q", "-b", "main") + run(dir, "remote", "add", "origin", remote) + run(dir, "config", "url."+bare+".pushInsteadOf", remote) + os.WriteFile(filepath.Join(dir, "refunds.go"), []byte("package shop\n"), 0o644) + run(dir, "add", ".") + run(dir, "commit", "-q", "-m", "feat: partial refunds") + run(dir, "checkout", "-q", "-b", branch) + return dir, bare +} + +// fakeGHCalls puts a gh on PATH that logs its arguments and answers +// `auth status` with authExit and `pr create` with prOut and prExit; +// `pr view` finds no pull request. +func fakeGHCalls(t *testing.T, authExit, prExit int, prOut string) func() []string { + t.Helper() + dir := t.TempDir() + log := filepath.Join(dir, "calls") + script := `#!/bin/sh +echo "$*" >> "` + log + `" +case "$1 $2" in + "auth status") exit ` + strconv.Itoa(authExit) + ` ;; + "pr view") echo 'no pull requests found' >&2; exit 1 ;; + "pr create") cat <<'OUT' +` + prOut + ` +OUT + exit ` + strconv.Itoa(prExit) + ` ;; +esac +exit 1 +` + os.WriteFile(filepath.Join(dir, "gh"), []byte(script), 0o755) + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + return func() []string { + b, _ := os.ReadFile(log) + return strings.Split(strings.TrimSpace(string(b)), "\n") + } +} + +func prHostBox(t *testing.T) *runHost { + b := &Box{Name: "devbox", Locations: NewLocations(filepath.Join(t.TempDir(), "locations.json")), Events: &events.Bus{}} + return &runHost{b: b, env: map[string]*runEnv{}} +} + +func prStep(dir, branch string) *runs.StepCtx { + return &runs.StepCtx{Run: runs.Summary{ID: "r_pr", Template: "attempts"}, Path: "3.t.0", Attempt: 1, + Step: runs.Step{ID: "open", Kind: "pr", Draft: true, Title: "refunds", Text: "Picked from 2 attempts."}, + Vars: map[string]string{"pick.path": dir, "pick.branch": branch}} +} + +// #138: on a Gitea remote the pr step never runs gh pr create. It pushes +// the branch, passes on the link Gitea printed, and is a skip that says +// why, which the run goes on from. +func TestThePRStepOnGiteaPushesAndSkips(t *testing.T) { + calls := fakeGHCalls(t, 1, 1, "should not be called") + dir, bare := prCheckout(t, "https://gitea.acme.test/acme/shop.git", "refunds-a2", + `echo +echo "Create a new pull request for 'refunds-a2':" +echo " https://gitea.acme.test/acme/shop/compare/main...refunds-a2" +echo`) + res := prHostBox(t).pullRequest(context.Background(), prStep(dir, "refunds-a2")) + t.Logf("status %s: %s", res.Status, res.Out) + if res.Status != runs.Skipped || res.Err != "" { + t.Fatalf("status %s, err %q", res.Status, res.Err) + } + for _, want := range []string{"This repository is on gitea.acme.test, not GitHub, so Shipyard didn't open a pull request.", + "The branch refunds-a2 is pushed to origin; open one there: https://gitea.acme.test/acme/shop/compare/main...refunds-a2"} { + if !strings.Contains(res.Out, want) { + t.Fatalf("out %q lacks %q", res.Out, want) + } + } + if res.Set["pr.skipped"] != "1" || res.Set["pr.link"] != "https://gitea.acme.test/acme/shop/compare/main...refunds-a2" { + t.Fatalf("set %v", res.Set) + } + if out, err := exec.Command("git", "-C", bare, "rev-parse", "--verify", "refs/heads/refunds-a2").CombinedOutput(); err != nil { + t.Fatalf("the branch was not pushed: %s", out) + } + got := calls() + if len(got) != 1 || got[0] != "auth status --hostname gitea.acme.test" { + t.Fatalf("gh calls %q", got) + } +} + +// On GitHub, a gh pr create that fails is the step failing (the engine +// makes it a publishing failure a person retries or finishes past), with +// gh's words kept and no credentials in them. +func TestThePRStepOnGitHubFailsWithGhsWords(t *testing.T) { + calls := fakeGHCalls(t, 1, 1, "HTTP 401: Bad credentials (https://x-access-token:ghp_SECRET@github.com/acme/shop)") + dir, _ := prCheckout(t, "https://github.com/acme/shop.git", "refunds-a2", "") + res := prHostBox(t).pullRequest(context.Background(), prStep(dir, "refunds-a2")) + t.Logf("status %s, err %q: %s", res.Status, res.Err, res.Out) + if res.Status != runs.Failed || res.Err != "gh pr create failed" || !strings.Contains(res.Out, "HTTP 401: Bad credentials") { + t.Fatalf("%+v", res) + } + if strings.Contains(res.Out, "ghp_SECRET") { + t.Fatalf("a credential was kept: %q", res.Out) + } + var created bool + for _, c := range calls() { + if strings.HasPrefix(c, "pr create --head refunds-a2") && strings.HasSuffix(c, "--draft") { + created = true + } + if strings.HasPrefix(c, "auth") { + t.Fatalf("asked gh about github.com: %q", c) + } + } + if !created { + t.Fatalf("gh calls %q", calls()) + } +} + +// A host gh is signed in to is GitHub Enterprise: the pull request opens +// there as on github.com. +func TestThePRStepOpensOnASignedInEnterpriseHost(t *testing.T) { + fakeGHCalls(t, 0, 0, "https://ghe.acme.test/acme/shop/pull/12") + dir, _ := prCheckout(t, "git@ghe.acme.test:acme/shop.git", "refunds-a2", "") + res := prHostBox(t).pullRequest(context.Background(), prStep(dir, "refunds-a2")) + if res.Status != runs.Succeeded || res.Set["pr.url"] != "https://ghe.acme.test/acme/shop/pull/12" { + t.Fatalf("%+v", res) + } +} + +func TestRemoteHostsAndPushLinks(t *testing.T) { + for in, want := range map[string]string{ + "https://gitea.acme.test/acme/shop.git": "gitea.acme.test", + "git@Gitea.Acme.test:acme/shop.git": "gitea.acme.test", + "ssh://git@forge.acme.test:2222/acme/shop": "forge.acme.test", + "/srv/git/shop.git": "", + "file:///srv/git/shop.git": "", + } { + if got, _ := remoteHost(in); got != want { + t.Errorf("remoteHost(%q) = %q, want %q", in, got, want) + } + } + gitlab := "remote: \nremote: To create a merge request for refunds, visit:\nremote: https://gitlab.acme.test/acme/shop/-/merge_requests/new?merge_request%5Bsource_branch%5D=refunds\nremote: \nTo gitlab.acme.test:acme/shop.git\n * [new branch] refunds -> refunds\n" + if got := pushLink(gitlab); got != "https://gitlab.acme.test/acme/shop/-/merge_requests/new?merge_request%5Bsource_branch%5D=refunds" { + t.Errorf("gitlab link %q", got) + } + if got := pushLink("remote: https://me:tok@forge.acme.test/acme/shop/compare/main...x\n"); got != "https://forge.acme.test/acme/shop/compare/main...x" { + t.Errorf("link with a credential %q", got) + } + if got := pushLink("To https://forge.acme.test/acme/shop\n * [new branch] x -> x\n"); got != "" { + t.Errorf("a link from git itself %q", got) + } + if got := noPRText(forge{Local: true}, "x", ""); !strings.Contains(got, "origin is a folder on this box") { + t.Errorf("local text %q", got) + } +} diff --git a/internal/box/runhost.go b/internal/box/runhost.go index 62a5f5b6..abd4a084 100644 --- a/internal/box/runhost.go +++ b/internal/box/runhost.go @@ -673,8 +673,21 @@ func (h *runHost) pullRequest(ctx context.Context, x *runs.StepCtx) runs.Result if err := h.b.beforeAs(ctx, origOf(x), "run.pr", map[string]any{"path": dir, "branch": branch, "run": x.Run.ID}); err != nil { return fail(err) } - if out, err := git(ctx, "-C", dir, "push", "-u", "origin", branch); err != nil { - return runs.Result{Status: runs.Failed, Code: 1, Out: string(out), Err: "git push failed"} + // gh opens pull requests on GitHub only: elsewhere (Gitea, Forgejo, + // GitLab, a folder) the branch is pushed and the step says so, a skip + // the run goes on from, rather than a gh error that fails it. + f := originForge(ctx, dir) + out, err := git(ctx, "-C", dir, "push", "-u", "origin", branch) + if err != nil { + return runs.Result{Status: runs.Failed, Code: 1, Out: noUserinfo(string(out)), Err: "git push failed"} + } + if !f.GitHub { + link := pushLink(string(out)) + set := map[string]string{"pr.skipped": "1"} + if link != "" { + set["pr.link"] = link + } + return runs.Result{Status: runs.Skipped, Out: noPRText(f, branch, link), Set: set} } var existing struct { URL string `json:"url"` @@ -695,9 +708,9 @@ func (h *runHost) pullRequest(ctx context.Context, x *runs.StepCtx) runs.Result cmd := exec.CommandContext(cctx, bin, args...) cmd.Dir = dir cmd.Env = append(os.Environ(), "GH_PROMPT_DISABLED=1", "GIT_TERMINAL_PROMPT=0", "NO_COLOR=1") - out, err := cmd.CombinedOutput() + out, err = cmd.CombinedOutput() if err != nil { - return runs.Result{Status: runs.Failed, Code: 1, Out: string(out), Err: "gh pr create failed"} + return runs.Result{Status: runs.Failed, Code: 1, Out: noUserinfo(string(out)), Err: "gh pr create failed"} } u := strings.TrimSpace(string(out)) if i := strings.LastIndex(u, "\n"); i >= 0 { diff --git a/internal/box/runs/engine.go b/internal/box/runs/engine.go index a4ea7e29..407a46db 100644 --- a/internal/box/runs/engine.go +++ b/internal/box/runs/engine.go @@ -46,7 +46,7 @@ type StepCtx struct { // Result is what a leaf step did. type Result struct { - Status string // succeeded, failed, unknown, cancelled + Status string // succeeded, failed, skipped, unknown, cancelled Code int Out string Err string @@ -56,7 +56,7 @@ type Result struct { Candidate *Candidate } -func (r Result) ok() bool { return r.Status == Succeeded } +func (r Result) ok() bool { return r.Status == Succeeded || r.Status == Skipped } // Request starts a run. type Request struct { diff --git a/internal/box/runs/exec.go b/internal/box/runs/exec.go index 37e60c25..6b0d644b 100644 --- a/internal/box/runs/exec.go +++ b/internal/box/runs/exec.go @@ -369,7 +369,7 @@ func (x *execution) seq(ctx context.Context, sc *scope, steps []Step, prefix str if err != nil { return false, err } - if ctx.Err() != nil && res.Status != Succeeded && res.Status != Failed { + if ctx.Err() != nil && res.Status != Succeeded && res.Status != Failed && res.Status != Skipped { return false, ctx.Err() } prevOK = res.ok() @@ -771,7 +771,13 @@ func (x *execution) await(ctx context.Context, g Gate, timeout, onTimeout, id st if op, ok := x.opened[path]; ok && op.Step != nil { st.Started = op.Step.Started } - if !d.Approve { + if g.RejectLabel != "" { + // A choice between two ways on, not a yes or no: neither failed. + st.Output = g.RejectLabel + if d.Approve { + st.Output = g.ApproveLabel + } + } else if !d.Approve { st.Status = Failed } x.record(Record{T: recGateDone, Path: path, Decided: &d, Step: &st}, true) @@ -850,6 +856,11 @@ func (x *execution) leaf(ctx context.Context, sc *scope, s Step, id, path string sctx.Attempt = rec.Attempt } sctx.IdemKey = idemKey(x.run.ID, path, sctx.Attempt) + if s.Kind == "pr" && rec.Step != nil && rec.Step.Status == Failed { + // berthd stopped while a person was asked about a pull request + // that didn't open: the same question, not another try. + return x.publishFailed(ctx, sc, sctx, id, Result{Status: Failed, Code: rec.Step.ExitCode, Out: rec.Step.Output, Err: rec.Step.Error}, rec.Step) + } if res, ok := x.e.Host.Reenter(ctx, sctx); ok { if res.Status == "unknown" { arrived, err := x.await(ctx, Gate{Path: path + ".arrived", Title: "Did the prompt arrive?", @@ -918,10 +929,66 @@ func (x *execution) execute(ctx context.Context, sc *scope, sctx *StepCtx, id st res.Err = "cancelled" } } + if s.Kind == "pr" && res.Status == Failed { + return x.publishFailed(ctx, sc, sctx, id, res, started) + } x.finish(sc, sctx, id, res, started) return res, nil } +// publishFailed is a pull request that didn't open. That is publishing +// failing, not the agents' work: the pick stays recorded, nothing is +// archived, and the run waits for a person to retry, or to finish without +// a pull request, which the steps after see as a skip and go on. +func (x *execution) publishFailed(ctx context.Context, sc *scope, sctx *StepCtx, id string, res Result, started *StepRun) (Result, error) { + path := sctx.Path + g := Gate{Path: fmt.Sprintf("%s.publish%d", path, sctx.Attempt), Title: "The pull request didn't open", Text: publishText(res, sctx.Vars["pick.path"] != ""), + ApproveLabel: "Retry publishing", RejectLabel: "Finish without a pull request"} + if _, ok := x.opened[g.Path]; !ok { + // The step shows its failure while the run waits. + failed := *started + failed.Status, failed.Error, failed.Output, failed.ExitCode = Failed, res.Err, tail(res.Out, 4000), res.Code + failed.Ended = x.e.now().UTC() + x.record(Record{T: recStarted, Path: path, Attempt: sctx.Attempt, Step: &failed}, true) + } + d, err := x.await(ctx, g, "", "", id) + if err != nil { + return Result{Status: Failed}, err + } + if d.Approve { + sctx.Attempt++ + sctx.IdemKey = idemKey(x.run.ID, path, sctx.Attempt) + return x.execute(ctx, sc, sctx, id) + } + out := "Finished without a pull request" + if res.Err != "" { + out += " (" + res.Err + ")" + } + skip := Result{Status: Skipped, Out: out + ".", Set: map[string]string{"pr.skipped": "1"}} + x.finish(sc, sctx, id, skip, started) + return skip, nil +} + +// publishText is the publish gate's text: what failed, in the tool's own +// words, and what each way on does. +func publishText(res Result, picked bool) string { + what := res.Err + if what == "" { + what = "it failed" + } + t := "The agents' work is done and kept; only publishing failed: " + what + "." + if picked { + t = "The agents' work is done and your pick is kept; only publishing failed: " + what + "." + } + if o := strings.TrimSpace(res.Out); o != "" { + t += "\n\n" + tail(o, 1200) + } + if picked { + return t + "\n\nRetry once it's fixed, or finish without a pull request: the run goes on and archives the other attempts." + } + return t + "\n\nRetry once it's fixed, or finish without a pull request: the run goes on with its next steps." +} + // finish journals a leaf's result. func (x *execution) finish(sc *scope, sctx *StepCtx, id string, res Result, started *StepRun) { if res.Status == "" { diff --git a/internal/box/runs/run.go b/internal/box/runs/run.go index a2b015a8..921566cc 100644 --- a/internal/box/runs/run.go +++ b/internal/box/runs/run.go @@ -177,6 +177,11 @@ type Gate struct { Pick bool `json:"pick,omitempty"` // Default is the candidate a pick approves without naming one. Default int `json:"default,omitempty"` + // ApproveLabel and RejectLabel name the two ways on when they are not + // approve and reject: "Retry publishing" and "Finish without a pull + // request" at a pull request that didn't open. + ApproveLabel string `json:"approve_label,omitempty"` + RejectLabel string `json:"reject_label,omitempty"` } // Decision decides a gate. @@ -253,6 +258,9 @@ const ( Succeeded = "succeeded" Failed = "failed" Cancelled = "cancelled" + // Skipped, for a leaf step, is one that chose not to act (a pull + // request on a host without them): the run goes on as on success. + Skipped = "skipped" Interrupted = "interrupted" ) diff --git a/internal/box/runs/runs_test.go b/internal/box/runs/runs_test.go index 43927ce3..632571fb 100644 --- a/internal/box/runs/runs_test.go +++ b/internal/box/runs/runs_test.go @@ -778,3 +778,171 @@ func TestAttemptsPickNoneArchivesAll(t *testing.T) { } } } + +// stepAt is the step at path in a run's step tree. +func stepAt(steps []StepRun, path string) *StepRun { + for i := range steps { + if steps[i].Path == path { + return &steps[i] + } + if s := stepAt(steps[i].Children, path); s != nil { + return s + } + } + return nil +} + +// prAttempts starts an attempts run of two whose pr step does what pr +// says, and picks attempt 2 at its gate. +func prAttempts(t *testing.T, dir string, pr func(n int) Result) (*Engine, func(), *fakeHost, string) { + t.Helper() + h := attemptsHost(`{"winner": 2, "ranking": [2, 1]}`) + inner := h.leaf + var mu sync.Mutex + n := 0 + h.leaf = func(ctx context.Context, x *StepCtx) Result { + if x.Step.Kind == "pr" { + mu.Lock() + n++ + k := n + mu.Unlock() + return pr(k) + } + return inner(ctx, x) + } + e, stop := newEngine(t, dir, h) + s, _, err := e.Start(Request{Template: "attempts", Params: map[string]any{"location": "shop", "name": "refunds", "prompt": "p", + "attempts": []any{"claude", "codex"}, "then": map[string]any{"pr": map[string]any{"draft": true}}}}) + if err != nil { + t.Fatal(err) + } + waitStatus(t, e, s.ID, WaitingGate) + one := 1 + if err := e.Decide(s.ID, "", Decision{Approve: true, Pick: &one}); err != nil { + t.Fatal(err) + } + return e, stop, h, s.ID +} + +const prPath = "3.t.0" // the attempts template's pr → open + +// waitGate waits for the run to wait at the gate at path. +func waitGate(t *testing.T, e *Engine, id, path string) Run { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + var r Run + for time.Now().Before(deadline) { + r, _ = e.Get(id) + if r.Status == WaitingGate && r.Gate != nil && r.Gate.Path == path { + return r + } + if Terminal(r.Status) { + break + } + time.Sleep(5 * time.Millisecond) + } + t.Fatalf("run %s is %s (%s), gate %+v; want the gate at %s", id, r.Status, r.Error, r.Gate, path) + return r +} + +// On a host gh can't open pull requests on (Gitea), the pr step is a skip +// with its reason: the run still archives the other attempts and succeeds, +// with the pick kept. +func TestAttemptsPRSkippedOnAnotherForgeStillCleansUp(t *testing.T) { + why := "This repository is on gitea.acme.test, not GitHub, so Shipyard didn't open a pull request. The branch refunds-a2 is pushed to origin." + e, stop, h, id := prAttempts(t, t.TempDir(), func(int) Result { + return Result{Status: Skipped, Out: why, Set: map[string]string{"pr.skipped": "1"}} + }) + defer stop() + r := waitStatus(t, e, id, Succeeded, Failed) + if r.Status != Succeeded || r.Error != "" { + t.Fatalf("%s: %s", r.Status, r.Error) + } + if h.count("pr:") != 1 || h.count("cleanup:") != 1 { + t.Fatalf("pr/cleanup: %v", h.calls) + } + if !r.Candidates[1].Picked || r.Candidates[0].Picked { + t.Fatalf("picked %+v", r.Candidates) + } + st := stepAt(r.Steps, prPath) + if st == nil || st.Status != Skipped || st.Output != why { + t.Fatalf("pr step %+v", st) + } + t.Logf("pr step: %s: %s", st.Status, st.Output) +} + +// A pull request that doesn't open on GitHub is a publishing failure, not +// the agents' work failing: the run waits with the pick kept and nothing +// archived, survives a restart without trying again by itself, and +// "Retry publishing" tries again and goes on. +func TestAttemptsPRFailureWaitsThenRetries(t *testing.T) { + dir := t.TempDir() + ghSays := "none of the git remotes configured for this repository point to a known GitHub host." + pr := func(n int) Result { + if n == 1 { + return Result{Status: Failed, Code: 1, Out: ghSays, Err: "gh pr create failed"} + } + return Result{Status: Succeeded, Out: "https://github.com/acme/shop/pull/7", Set: map[string]string{"pr.url": "https://github.com/acme/shop/pull/7"}} + } + e, stop, h, id := prAttempts(t, dir, pr) + r := waitGate(t, e, id, prPath+".publish1") + if r.Status != WaitingGate || r.Gate.Path != prPath+".publish1" || r.Gate.ApproveLabel != "Retry publishing" || r.Gate.RejectLabel != "Finish without a pull request" { + t.Fatalf("%s, gate %+v", r.Status, r.Gate) + } + if !strings.Contains(r.Gate.Text, "only publishing failed: gh pr create failed") || !strings.Contains(r.Gate.Text, ghSays) { + t.Fatalf("gate text %q", r.Gate.Text) + } + if !r.Candidates[1].Picked || h.count("cleanup:") != 0 { + t.Fatalf("pick lost or archived early: %+v %v", r.Candidates, h.calls) + } + if st := stepAt(r.Steps, prPath); st == nil || st.Status != Failed || st.Error != "gh pr create failed" { + t.Fatalf("pr step %+v", st) + } + t.Logf("waiting at %q: %s / %s", r.Gate.Title, r.Gate.ApproveLabel, r.Gate.RejectLabel) + + // berthd restarts: the same question, and gh is not run again. + stop() + e, stop = newEngine(t, dir, h) + defer stop() + r = waitStatus(t, e, id, WaitingGate) + if r.Gate.Path != prPath+".publish1" || h.count("pr:") != 1 { + t.Fatalf("after a restart: gate %+v, calls %v", r.Gate, h.calls) + } + if err := e.Decide(id, "", Decision{Approve: true, By: "cli"}); err != nil { + t.Fatal(err) + } + r = waitStatus(t, e, id, Succeeded, Failed) + if r.Status != Succeeded || h.count("pr:") != 2 || h.count("cleanup:") != 1 { + t.Fatalf("%s %s: %v", r.Status, r.Error, h.calls) + } + st := stepAt(r.Steps, prPath) + if st == nil || st.Status != Succeeded || st.Attempt != 2 || st.Output != "https://github.com/acme/shop/pull/7" { + t.Fatalf("pr step %+v", st) + } + t.Logf("retried: pr step %s (attempt %d): %s", st.Status, st.Attempt, st.Output) +} + +// "Finish without a pull request" ends the run as a success: the pr step +// is a skip that says so, and the other attempts are archived. +func TestAttemptsPRFailureFinishesWithoutOne(t *testing.T) { + e, stop, h, id := prAttempts(t, t.TempDir(), func(int) Result { + return Result{Status: Failed, Code: 1, Out: "HTTP 401: Bad credentials", Err: "gh pr create failed"} + }) + defer stop() + waitGate(t, e, id, prPath+".publish1") + if err := e.Decide(id, "", Decision{Approve: false, By: "app"}); err != nil { + t.Fatal(err) + } + r := waitStatus(t, e, id, Succeeded, Failed) + if r.Status != Succeeded || r.Error != "" { + t.Fatalf("%s: %s", r.Status, r.Error) + } + if h.count("pr:") != 1 || h.count("cleanup:") != 1 || !r.Candidates[1].Picked { + t.Fatalf("calls %v, candidates %+v", h.calls, r.Candidates) + } + st := stepAt(r.Steps, prPath) + if st == nil || st.Status != Skipped || st.Output != "Finished without a pull request (gh pr create failed)." { + t.Fatalf("pr step %+v", st) + } + t.Logf("finished: pr step %s: %s", st.Status, st.Output) +} diff --git a/internal/box/runsapi.go b/internal/box/runsapi.go index 70e07c70..6d62d31d 100644 --- a/internal/box/runsapi.go +++ b/internal/box/runsapi.go @@ -309,10 +309,13 @@ func (b *Box) reviewRun(w http.ResponseWriter, r *http.Request, id string) error Review *ReviewItem `json:"review,omitempty"` } out := struct { - Run runs.Summary `json:"run"` - Gate *runs.Gate `json:"gate,omitempty"` - Judge string `json:"judge,omitempty"` - Candidates []compared `json:"candidates"` + Run runs.Summary `json:"run"` + Gate *runs.Gate `json:"gate,omitempty"` + Judge string `json:"judge,omitempty"` + // PR is the pull request step: opened, skipped (a host without + // them) or failed, with what it said. + PR *runs.StepRun `json:"pr,omitempty"` + Candidates []compared `json:"candidates"` }{Gate: run.Gate, Candidates: []compared{}} out.Run = runs.Summary{ID: run.ID, Template: run.Template, Title: run.Title, Status: run.Status, Created: run.Created, Updated: run.Updated, Usage: run.Usage} for _, st := range run.Steps { @@ -320,6 +323,19 @@ func (b *Box) reviewRun(w http.ResponseWriter, r *http.Request, id string) error out.Judge = st.Output } } + var findPR func([]runs.StepRun) + findPR = func(steps []runs.StepRun) { + for i := range steps { + // A pr step skipped by its when rule says nothing: not that one. + if steps[i].Kind == "pr" && (steps[i].Status != runs.Skipped || steps[i].Output != "") { + st := steps[i] + st.Children = nil + out.PR = &st + } + findPR(steps[i].Children) + } + } + findPR(run.Steps) for _, c := range run.Candidates { item := compared{Candidate: c} if loc, wt, ok := b.worktreeAt(r.Context(), c.Path); ok { diff --git a/internal/boxcmd/runcmd.go b/internal/boxcmd/runcmd.go index 438e3910..57565b32 100644 --- a/internal/boxcmd/runcmd.go +++ b/internal/boxcmd/runcmd.go @@ -267,7 +267,7 @@ func recordLine(r runs.Record) string { return line case "gate.opened": if r.Gate != nil { - return fmt.Sprintf("%s waiting at gate %q (%s): run approve|reject RUN", at, r.Gate.Title, r.Path) + return fmt.Sprintf("%s waiting at gate %q (%s): %s", at, r.Gate.Title, r.Path, gateWays(r.Gate)) } case "gate.decided": if r.Decided != nil { @@ -283,6 +283,15 @@ func recordLine(r runs.Record) string { return "" } +// gateWays is how to decide a gate: approve or reject, or the two ways on +// it names (retry publishing, or finish without a pull request). +func gateWays(g *runs.Gate) string { + if g.ApproveLabel == "" || g.RejectLabel == "" { + return "run approve|reject RUN" + } + return "run approve RUN to " + strings.ToLower(g.ApproveLabel) + ", run reject RUN to " + strings.ToLower(g.RejectLabel) +} + func firstLineOf(s string) string { s = strings.TrimSpace(s) if i := strings.IndexByte(s, '\n'); i >= 0 { @@ -304,6 +313,9 @@ func printRun(out io.Writer, r runs.Run) { } if r.Gate != nil { fmt.Fprintf(out, " waiting at gate %q (%s)\n", r.Gate.Title, r.Gate.Path) + if r.Gate.ApproveLabel != "" { + fmt.Fprintf(out, " %s\n", gateWays(r.Gate)) + } } if r.Usage != nil { fmt.Fprintf(out, " agents used %d tokens", r.Usage.Tokens()) diff --git a/packages/plugin-sdk/src/types.ts b/packages/plugin-sdk/src/types.ts index 0b484c25..068ba9af 100644 --- a/packages/plugin-sdk/src/types.ts +++ b/packages/plugin-sdk/src/types.ts @@ -720,6 +720,11 @@ export interface RunGate { pick?: boolean; // The candidate a pick approves without naming one (0-based). default?: number; + // The two ways on when they aren't approve and reject: "Retry + // publishing" and "Finish without a pull request" at a pull request + // that didn't open. + approve_label?: string; + reject_label?: string; } export interface RunStep {