From e653fbc7d4cc4400793552d8f0367724a59c4c8d Mon Sep 17 00:00:00 2001 From: sean Date: Sun, 18 Oct 2026 17:58:53 +0100 Subject: [PATCH] fix(box): attempts on a repository GitHub doesn't host finish with the pick, and a pull request that doesn't open can be retried MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pr step ran gh pr create on any remote. On Gitea (#138) gh failed, the run failed at pr → open, cleanup was skipped and every worktree was left behind. - On a host gh can't open pull requests on (Gitea, Forgejo, GitLab, a folder; anything but github.com or a GitHub Enterprise host gh is signed in to), the step pushes the branch, says why it opened nothing, passes on the link the forge printed on push, and ends as skipped. A skipped leaf goes on like a success, so the run archives the other attempts and succeeds with the pick kept. - A pr step that fails is a publishing failure, not the agents' work: the run waits at a gate, "The pull request didn't open", with gh's words, the pick recorded and nothing archived. Retry publishing (approve) tries again; Finish without a pull request (reject) makes the step a skip and the run goes on. A restart asks the same question without running gh again. - Gates can name their two ways on (approve_label, reject_label); the app's run timeline and Review's Compare show them, Compare says what became of the pull request, and the CLI prints them. - URLs in push and gh output lose any user and password before they are kept. Co-Authored-By: Claude Opus 5.5 --- app/e2e/publish-failed.spec.ts | 48 +++++ app/src/lib/mock-runs.ts | 97 +++++++++- app/src/lib/orchestrate-core.ts | 3 + app/src/views/automations/flows/runs-tab.tsx | 10 +- app/src/views/review/compare-view.tsx | 73 +++++++- docs/changelog.mdx | 12 ++ docs/guides/runs.mdx | 12 ++ internal/box/prhost.go | 153 +++++++++++++++ internal/box/prhost_test.go | 185 +++++++++++++++++++ internal/box/runhost.go | 21 ++- internal/box/runs/engine.go | 4 +- internal/box/runs/exec.go | 71 ++++++- internal/box/runs/run.go | 8 + internal/box/runs/runs_test.go | 168 +++++++++++++++++ internal/box/runsapi.go | 24 ++- internal/boxcmd/runcmd.go | 14 +- packages/plugin-sdk/src/types.ts | 5 + 17 files changed, 884 insertions(+), 24 deletions(-) create mode 100644 app/e2e/publish-failed.spec.ts create mode 100644 internal/box/prhost.go create mode 100644 internal/box/prhost_test.go 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 783552b0..eb0c7efc 100644 --- a/docs/changelog.mdx +++ b/docs/changelog.mdx @@ -11,6 +11,18 @@ 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 helper that is back shows as back in a chat session.** In a chat (structured) Claude session, a helper Claude left working in the background stayed "working" in Crew, its time running on, after it had 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 {