From 830659b56f8c93606170d8ed611fd2981492cf67 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Sun, 27 Sep 2026 20:08:03 +0300 Subject: [PATCH] v2.9.0: review inbox without going to GitHub A PR waiting on you now shows its diff, gate evidence, verify verdict, CI status and per-stage cost right in the dashboard, across every repo the factory watches, with an operator merge button sharing the same merge primitive as the automated policy. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 47 +++++++ dashboard/public/app.js | 106 ++++++++++++--- dashboard/public/styles.css | 3 + dashboard/server.ts | 139 ++++++++++++++++---- package.json | 2 +- src/github.ts | 52 +++++++- src/merge-policy.ts | 36 ++++- src/paths.ts | 22 ++++ src/watch.ts | 4 +- template-ci/factory.yml.example | 2 +- tests/dashboard-routes.test.ts | 117 +++++++++++++++- tests/merge-policy-operator.test.ts | 93 +++++++++++++ tests/ported/machinist/merge-policy.test.ts | 1 + tests/scenarios.test.ts | 1 + 14 files changed, 569 insertions(+), 56 deletions(-) create mode 100644 tests/merge-policy-operator.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 37d6c02..b7275ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,52 @@ # Changelog +## v2.9.0 + +The review inbox stops sending you to GitHub. A PR waiting on you now shows its diff, gate +evidence, verify verdict, CI status and per-stage cost right in the dashboard, across every repo +the factory watches, with a merge button for the operator's own authority. + +- **`src/merge-policy.ts`: an operator merge path.** `decideOperatorMerge(readiness, ci)` reuses + `checkReadiness` with the PR's own base branch, skipping `autoEligible`'s risk/path/size gates + entirely, since a human click is a different authority source than the `auto` policy. It shares + one merge primitive, `attemptMerge`, with the automated policy (a spy test proves the same + function serves both). `renderOperatorAuditComment` posts its own `**operator**` wording, kept + distinct from `**auto**`/`**dry-run**` so the audit trail always shows who approved. +- **`src/github.ts`.** `prDiff` (`gh pr diff`) and `prForIssue`, the one open PR that closes an + issue, shared by the triage check and the new review route. `mergeReadiness`'s + `changesRequestedStale` flag: a CHANGES_REQUESTED review against a commit the head has since + moved past no longer blocks the item, idea only, from assembler's unmerged fix branch, no code + copied. +- **`src/paths.ts`: `discoverRepos(FACTORY_HOME)`.** Every `//factory.db` under the + factory's home directory becomes one inbox source, so a machine watching several repos gets one + inbox with a repo filter, not one dashboard per repo. +- **`dashboard/server.ts`.** `GET /api/inbox` aggregates across every discovered repo (or filters + to one with `?repo=`); `GET /api/issues/:n/review` returns the diff, CI, gate evidence, verify + verdict and per-stage cost for the PR that closes an issue; `POST /api/issues/:n/merge` runs + `decideOperatorMerge` and, on `outcome: "merge"`, calls the same `attemptMerge` the policy uses. +- **`dashboard/public/app.js`.** A repo filter on the inbox list; a review panel for `review-pr` + and `merge-dry-run` items (diff, CI status, gate line, verify findings, a per-stage cost table); + an "Approve and merge" button wired to the new merge route, separate from the generic + approve/revise/cancel actions, since neither of those inbox kinds carries a server-side + "approve" action. + +Structural tests: every factory state or parked label yields one inbox item with at least one +action, or none for in-flight labels (`tests/inbox.test.ts`); one merge function serves both the +dashboard button and the automated policy (`tests/merge-policy-operator.test.ts`, a spy across +both call sites). Verified live against a real in-review PR +(`learnwithparam/splitbill-demo` issue #10 / PR #16): the review route returned the actual diff, +CI status, gate evidence and verify verdict from GitHub, unchanged from the fixture shape. +Playwright screenshots at 1440, 768 and 375 px in light and dark themes cover the inbox list, the +review panel and the repo filter, with no console or page errors at any size. + +Not in v2.9.0: +- "Request changes" as its own review action: the existing `revise` action (with your note) fills + this role; a separate action would duplicate it. +- A CHANGES_REQUESTED-superseded check on the operator merge path beyond what `mergeReadiness` + already reports: `changesRequestedStale` is read, not re-derived, by `decideOperatorMerge`. +- Any change to the automated `merge.policy` behavior from v2.8.0: this release only adds a + second, human-triggered caller of the same merge primitive. + ## v2.8.0 The merge-policy half of the lwp-website pilot: the factory can now tell whether an in-review PR diff --git a/dashboard/public/app.js b/dashboard/public/app.js index c05fcf9..68c9a53 100644 --- a/dashboard/public/app.js +++ b/dashboard/public/app.js @@ -9,6 +9,8 @@ const STAGES = ["triage", "plan", "build", "verify", "pr"]; const WAITING = new Set(["needs-info", "awaiting-approval", "needs-human", "failed"]); const ACTIVE = new Set(["running", "verifying"]); const ACTION_LABEL = { approve: "Approve plan", revise: "Request changes", answer: "Send answer", retry: "Retry", cancel: "Cancel run" }; +const REVIEW_KINDS = new Set(["review-pr", "merge-dry-run"]); +const KIND_LABEL = { "approve-plan": "Plan to approve", "answer-question": "Question for you", "review-pr": "Pull request to review", "merge-dry-run": "Ready to merge", parked: "Needs a human", failed: "Failed", budget: "Over budget" }; const ICONS = { inbox: "M3 13l3-8h12l3 8v6H3zM3 13h5l1 3h6l1-3h5", line: "M4 6h10M4 12h16M4 18h7", @@ -19,7 +21,7 @@ const ICONS = { }; const NAV = [["inbox", "Inbox"], ["line", "Line"], ["runs", "Runs"], ["analytics", "Analytics"], ["agents", "Agents"]]; -const state = { route: routeFromHash(location.hash), inbox: [], repo: "", selected: null, thread: null, filter: "all", data: {}, error: {} }; +const state = { route: routeFromHash(location.hash), inbox: [], repo: "", repos: [], repoFilter: "", selected: null, thread: null, review: null, filter: "all", data: {}, error: {} }; function h(tag, attrs, ...kids) { const el = document.createElement(tag); @@ -68,7 +70,7 @@ const post = (path, body) => api(path, { method: "POST", headers: { "content-typ /* ---- states shared by every view ---- */ const quiet = (title, hint, ...extra) => h("div", { class: "quiet-state" }, h("div", { class: "quiet-state-mark", "aria-hidden": "true" }, h("i"), h("i"), h("i")), h("strong", null, title), h("span", null, hint), ...extra); -const heading = (title, lede) => h("div", { class: "page-heading" }, h("div", null, h("h1", null, title), lede && h("p", { class: "lede" }, lede))); +const heading = (title, lede, ...extra) => h("div", { class: "page-heading" }, h("div", null, h("h1", null, title), lede && h("p", { class: "lede" }, lede)), ...extra); const stateOr = (name, ready) => { if (state.error[name]) return quiet("Could not load this view", state.error[name]); if (!state.data[name]) return quiet("Loading", "Reading the factory's state."); @@ -107,24 +109,65 @@ function lineView() { } /* ---- Inbox ---- */ -async function selectItem(issue) { - state.selected = issue; +function isSelected(i) { return state.selected != null && state.selected.repo === i.repo && state.selected.issue === i.issue; } + +async function selectItem(item) { + state.selected = { repo: item.repo, issue: item.issue }; state.thread = null; + state.review = null; render(); - try { state.thread = (await api(`/api/issues/${issue}/thread`)).issue; } catch (e) { state.thread = { error: e.message }; } + const q = `?repo=${encodeURIComponent(item.repo)}`; + const wantsReview = REVIEW_KINDS.has(item.kind); + const [thread, review] = await Promise.allSettled([api(`/api/issues/${item.issue}/thread${q}`), wantsReview ? api(`/api/issues/${item.issue}/review${q}`) : Promise.resolve(null)]); + state.thread = thread.status === "fulfilled" ? thread.value.issue : { error: thread.reason.message }; + if (wantsReview) state.review = review.status === "fulfilled" ? review.value : { error: review.reason.message }; render(); } async function actOn(item, action, text) { if (action === "cancel" && !confirm(`Cancel the run for #${item.issue}? This closes the issue.`)) return; try { - await post(`/api/inbox/${item.issue}/act`, { action, text }); + await post(`/api/inbox/${item.issue}/act`, { action, text, repo: item.repo }); + state.selected = null; + state.review = null; + await Promise.all([loadInbox(), loadView()]); + } catch (e) { alert(e.message); } + render(); +} + +async function mergeApprove(item) { + try { + const r = await post(`/api/issues/${item.issue}/merge`, { repo: item.repo }); + if (r.decision.outcome !== "merge") { alert(`Not merged:\n${r.decision.refusals.map((x) => `- ${x.detail}`).join("\n")}`); return; } state.selected = null; + state.review = null; await Promise.all([loadInbox(), loadView()]); } catch (e) { alert(e.message); } render(); } +function actionLabel(item, a) { return REVIEW_KINDS.has(item.kind) && a === "cancel" ? "Close" : ACTION_LABEL[a]; } + +function reviewPanel() { + const r = state.review; + if (!r) return quiet("Loading", "Reading the pull request."); + if (r.error) return quiet("Could not load the review", r.error); + const ciTone = r.ci.status === "passed" ? "ok" : r.ci.status === "failed" ? "bad" : "warn"; + return h("div", { class: "review" }, + h("dl", { class: "kv" }, + h("dt", null, "Pull request"), h("dd", null, h("a", { href: r.pr.url, target: "_blank", rel: "noreferrer" }, `#${r.pr.number}`)), + h("dt", null, "CI"), h("dd", null, h("span", { class: "status", "data-tone": ciTone }, statusText(r.ci.status))), + r.gate ? [h("dt", null, "Gate"), h("dd", null, `${r.gate.status} · ${r.gate.line}`)] : null, + r.verify?.result ? [h("dt", null, "Verify"), h("dd", null, r.verify.result)] : null), + r.verify?.findings?.length ? h("div", null, h("h3", null, "Verify findings"), + h("ul", { class: "plain-list" }, r.verify.findings.map((f) => h("li", null, typeof f === "string" ? f : `${f.severity}: ${f.what}`)))) : null, + r.stages.length ? h("div", { class: "table-wrap" }, h("table", null, + h("thead", null, h("tr", null, ["Stage", "Agent", "Model", "Cost"].map((c, i) => h("th", { class: i === 3 ? "num" : "" }, c)))), + h("tbody", null, r.stages.map((s) => h("tr", null, h("td", null, s.stage), h("td", null, s.agent), h("td", null, s.model || "default"), h("td", { class: "num" }, s.usage_complete === 0 ? "Not reported" : money(s.cost_usd))))))) : null, + h("h3", { style: "margin-top:1rem" }, "Diff"), + h("pre", { class: "log" }, r.diff || "No diff available.")); +} + function conversation(item) { const t = state.thread; const body = !t ? quiet("Loading", "Reading the issue thread.") @@ -134,25 +177,38 @@ function conversation(item) { .map((m) => h("div", { class: `msg${m.bot ? " bot" : ""}` }, h("header", null, m.bot ? "factory" : m.author), cleanBody(m.body)))); const needsText = item.actions.filter((a) => a === "revise" || a === "answer"); const box = needsText.length ? h("textarea", { class: "field-control", id: "composer", rows: "3", placeholder: item.kind === "answer-question" ? "Your answer" : "What should change?", "aria-label": "Your reply" }) : null; + const isReview = REVIEW_KINDS.has(item.kind); return h("div", { class: "panel" }, - h("div", { class: "panel-pad" }, h("h2", null, `#${item.issue} ${item.title}`), h("span", { class: "muted" }, `${item.label.replace("factory:", "").replace(/-/g, " ")} · waiting ${age(item.waitingSince)}`)), + h("div", { class: "panel-pad" }, h("h2", null, `#${item.issue} ${item.title}`), h("span", { class: "muted" }, `${item.repo} · ${item.label.replace("factory:", "").replace(/-/g, " ")} · waiting ${age(item.waitingSince)}`)), + isReview ? h("div", { class: "panel-pad" }, reviewPanel()) : null, body, - h("div", { class: "composer" }, box, h("div", { class: "actions" }, item.actions.map((a) => - h("button", { class: `btn${a === "approve" || a === "answer" ? " btn-primary" : a === "cancel" ? " btn-danger" : ""}`, type: "button", - onclick: () => { const text = box ? box.value : ""; if ((a === "revise" || a === "answer") && !text.trim()) { box.focus(); return; } actOn(item, a, text); } }, ACTION_LABEL[a]))))); + h("div", { class: "composer" }, box, h("div", { class: "actions" }, + isReview ? h("button", { class: "btn btn-primary", type: "button", onclick: () => mergeApprove(item) }, "Approve and merge") : null, + item.actions.map((a) => + h("button", { class: `btn${a === "approve" || a === "answer" ? " btn-primary" : a === "cancel" ? " btn-danger" : ""}`, type: "button", + onclick: () => { const text = box ? box.value : ""; if ((a === "revise" || a === "answer") && !text.trim()) { box.focus(); return; } actOn(item, a, text); } }, actionLabel(item, a)))))); +} + +function repoFilterControl() { + if (state.repos.length <= 1) return null; + return h("select", { class: "field-control repo-filter", "aria-label": "Filter by repo", + onchange: (e) => { state.repoFilter = e.target.value; loadInbox().then(render); } }, + h("option", { value: "", selected: state.repoFilter === "" }, "All repos"), + ...state.repos.map((r) => h("option", { value: r, selected: state.repoFilter === r }, r))); } function inboxView() { const items = state.inbox; - const item = items.find((i) => i.issue === state.selected); - return h("section", null, heading("Inbox", "Everything the factory is waiting on you for."), + const multiRepo = state.repos.length > 1; + const item = items.find(isSelected); + return h("section", null, heading("Inbox", "Everything the factory is waiting on you for.", repoFilterControl()), !items.length ? quiet("Nothing is waiting on you", "The factory will list plans to approve and questions to answer here.") : h("div", { class: "split", "data-open": String(Boolean(item)) }, h("div", { class: "list-col" }, h("div", { class: "list" }, items.map((i) => - h("button", { class: "list-item", type: "button", "aria-current": String(i.issue === state.selected), onclick: () => selectItem(i.issue) }, - h("span", { class: "kind" }, { "approve-plan": "Plan to approve", "answer-question": "Question for you", "review-pr": "Pull request to review", parked: "Needs a human", failed: "Failed" }[i.kind]), + h("button", { class: "list-item", type: "button", "aria-current": String(isSelected(i)), onclick: () => selectItem(i) }, + h("span", { class: "kind" }, KIND_LABEL[i.kind], multiRepo ? h("span", { class: "muted repo-tag" }, ` · ${i.repo}`) : null), h("strong", null, `#${i.issue} ${i.title}`), h("span", { class: "muted" }, `waiting ${age(i.waitingSince)}`), h("span", { class: "muted" }, i.ask.slice(0, 110)))))), - h("div", { class: "detail-col" }, item ? [h("button", { class: "btn back", type: "button", onclick: () => { state.selected = null; render(); } }, "Back to inbox"), conversation(item)] : quiet("Pick an item", "Its conversation and actions show up here.")))); + h("div", { class: "detail-col" }, item ? [h("button", { class: "btn back", type: "button", onclick: () => { state.selected = null; state.review = null; render(); } }, "Back to inbox"), conversation(item)] : quiet("Pick an item", "Its conversation and actions show up here.")))); } /* ---- Runs ---- */ @@ -285,11 +341,14 @@ function toggleTheme() { try { localStorage.setItem("factory-theme", next); } catch {} } -function go(view, issue) { - state.selected = issue ?? null; +function go(view, issue, repo = state.repo) { + state.selected = issue ? { repo, issue } : null; if (location.hash !== `#/${view}`) location.hash = `#/${view}`; else render(); - if (issue) selectItem(issue); + if (issue) { + const item = state.inbox.find((i) => i.issue === issue && i.repo === repo); + if (item) selectItem(item); + } } function showLogin() { @@ -300,8 +359,15 @@ function showLogin() { } async function loadInbox() { - try { const r = await api("/api/inbox"); state.inbox = r.items; state.repo = r.repo; state.error.inbox = null; } catch (e) { state.error.inbox = e.message; } - document.getElementById("repo-label").textContent = state.repo || ""; + try { + const q = state.repoFilter ? `?repo=${encodeURIComponent(state.repoFilter)}` : ""; + const r = await api(`/api/inbox${q}`); + state.inbox = r.items; + state.repo = r.repo; + state.repos = r.repos || []; + state.error.inbox = null; + } catch (e) { state.error.inbox = e.message; } + document.getElementById("repo-label").textContent = state.repo || (state.repos.length > 1 ? `${state.repos.length} repos` : ""); renderNav(); } diff --git a/dashboard/public/styles.css b/dashboard/public/styles.css index cee5a35..d7c8844 100644 --- a/dashboard/public/styles.css +++ b/dashboard/public/styles.css @@ -125,6 +125,7 @@ h2 { margin: 0 0 .75rem; font-size: 1rem; font-weight: 600; letter-spacing: -.01 .field-control { width: 100%; min-height: 2.5rem; border: 1px solid var(--border); border-radius: 8px; outline: none; background: var(--background); padding: .625rem .75rem; box-shadow: inset 0 1px 0 color-mix(in srgb, var(--foreground) 3%, transparent); transition: border-color .15s ease, box-shadow .15s ease; } .field-control:focus { border-color: color-mix(in srgb, var(--primary) 60%, transparent); box-shadow: 0 0 0 3px color-mix(in srgb, var(--primary) 14%, transparent); } textarea.field-control { resize: vertical; min-height: 5rem; } +.repo-filter { width: auto; min-width: 12rem; } .tabs { display: flex; gap: .25rem; flex-wrap: wrap; } .tab { padding: .3rem .7rem; border: 1px solid transparent; border-radius: 999px; background: none; color: var(--muted-foreground); } .tab[aria-pressed="true"] { border-color: var(--border); background: var(--surface); color: var(--foreground); } @@ -198,6 +199,8 @@ tr[data-href]:hover { background: color-mix(in srgb, var(--muted) 50%, transpare .kv dd { margin: 0; overflow-wrap: anywhere; } pre.log { margin: 0; max-height: 22rem; overflow: auto; padding: .75rem; background: var(--background); border: 1px solid var(--border); border-radius: 8px; font: .75rem/1.1rem ui-monospace, SFMono-Regular, Menlo, monospace; white-space: pre-wrap; overflow-wrap: anywhere; } @keyframes slide-in { from { transform: translateX(1.5rem); opacity: 0; } to { transform: none; opacity: 1; } } +.review { padding-bottom: 1.25rem; margin-bottom: 1.25rem; border-bottom: 1px solid var(--border); } +.review .kv { margin: .5rem 0 1rem; } /* Empty, loading and error states */ .quiet-state { display: grid; justify-items: start; gap: .5rem; min-height: 11rem; align-content: center; color: var(--muted-foreground); } diff --git a/dashboard/server.ts b/dashboard/server.ts index b7d4dfa..06976ad 100644 --- a/dashboard/server.ts +++ b/dashboard/server.ts @@ -21,8 +21,10 @@ import { agentCatalog } from "../src/agents/docs"; import { agentChecks } from "../src/doctor"; import { versionOf, which } from "../src/probes"; import { DEFAULT_CONFIG, type FactoryConfig } from "../src/config"; -import { workspacesDir } from "../src/paths"; -import { runDir } from "../src/artifacts"; +import { discoverRepos, workspacesDir } from "../src/paths"; +import { runDir, readGateEvidence, readStageArtifacts } from "../src/artifacts"; +import { ciStatusNow } from "../src/ci"; +import { attemptMerge, decideOperatorMerge, renderOperatorAuditComment } from "../src/merge-policy"; import { analytics } from "./analytics"; const here = dirname(fileURLToPath(import.meta.url)); @@ -84,8 +86,8 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin const indexHtml = readFileSync(join(here, "public", "index.html"), "utf8"); const sessions = new Map(); - let boardCache: { at: number; issues: Awaited> } | null = null; - let boardInflight: Promise>> | null = null; + const issuesCache = new Map> }>(); + const issuesInflight = new Map>>>(); // The Agents page shows each configured agent's installed version and doctor rows. // Probing spawns `--version`, so it is cached for a minute and shared by concurrent requests. @@ -116,22 +118,51 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin // Single-flight + 10s cache in front of `gh issue list` (plan: "cached gh // listing, single-flight, 10s") so a browser polling every few seconds, // times any number of open tabs, doesn't turn into one `gh` call per poll. - async function cachedIssues() { + // Keyed per repo so the multi-repo inbox (v2.9.0 item 2) never lets one + // repo's cache serve another's issues. + async function cachedIssuesFor(r: string) { const now = Date.now(); - if (boardCache && now - boardCache.at < BOARD_CACHE_MS) return boardCache.issues; - if (boardInflight) return boardInflight; - boardInflight = github - .listOpenIssues(repo) + const cached = issuesCache.get(r); + if (cached && now - cached.at < BOARD_CACHE_MS) return cached.issues; + const inflight = issuesInflight.get(r); + if (inflight) return inflight; + const promise = github + .listOpenIssues(r) .then((issues) => { - boardCache = { at: Date.now(), issues }; - boardInflight = null; + issuesCache.set(r, { at: Date.now(), issues }); + issuesInflight.delete(r); return issues; }) .catch((err) => { - boardInflight = null; + issuesInflight.delete(r); throw err; }); - return boardInflight; + issuesInflight.set(r, promise); + return promise; + } + + function invalidateIssuesCache(r: string): void { + issuesCache.delete(r); + } + + async function cachedIssues() { + return cachedIssuesFor(repo); + } + + // Every repo the multi-repo inbox reads from (v2.9.0 item 2): an explicit + // FACTORY_REPOS list, else the single configured repo, else whatever repos + // have ever run under FACTORY_HOME. + function inboxRepos(): string[] { + const fromEnv = (process.env.FACTORY_REPOS ?? "").split(",").map((s) => s.trim()).filter(Boolean); + if (fromEnv.length > 0) return fromEnv; + return repo ? [repo] : discoverRepos(); + } + + // The primary configured repo's workspaces dir is the injectable + // `workspaces` param (tests point it at a scratch dir); any other repo in a + // multi-repo inbox resolves its own workspaces dir under FACTORY_HOME. + function workspacesFor(r: string): string { + return r === repo ? workspaces : workspacesDir(process.env, r); } function json(data: unknown, init?: ResponseInit): Response { @@ -154,8 +185,8 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin const needRepo = (): Response | null => (repo ? null : json({ error: "FACTORY_REPO not set" }, { status: 500 })); - async function inboxItem(number: number) { - return buildInbox(await cachedIssues()).find((i) => i.issue === number); + async function inboxItem(r: string, number: number) { + return buildInbox(await cachedIssuesFor(r)).find((i) => i.issue === number); } // Files a stage left in .factory/runs/issue-N/ of the issue's worktree. The @@ -267,7 +298,7 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin if (missing) return missing; if (!Number.isInteger(body.issue)) return json({ error: "issue must be an integer" }, { status: 400 }); await github.addLabels(repo, body.issue!, [LABEL.ready]); - boardCache = null; // next /api/board reflects the label immediately + invalidateIssuesCache(repo); // next /api/board reflects the label immediately return json({ ok: true }); }, }, @@ -275,7 +306,11 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin method: "GET", pattern: /^\/api\/issues\/(\d+)\/thread$/, label: "GET /api/issues/:n/thread", - handler: async (_req, _url, m) => needRepo() ?? json({ issue: plainIssue(await github.getIssue(repo, Number(m[1]))) }), + handler: async (_req, url, m) => { + const r = url.searchParams.get("repo") || repo; + if (!r) return json({ error: "no repo configured or specified" }, { status: 400 }); + return json({ issue: plainIssue(await github.getIssue(r, Number(m[1]))) }); + }, }, { method: "GET", @@ -283,6 +318,52 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin label: "GET /api/issues/:n/artifacts", handler: (_req, url, m) => artifacts(Number(m[1]), url), }, + { + // The PR review view (v2.9.0 item 1): diff, gate evidence, verify + // findings, CI status and per-stage cost/model, all in one call so the + // dashboard never has to go to GitHub itself. + method: "GET", + pattern: /^\/api\/issues\/(\d+)\/review$/, + label: "GET /api/issues/:n/review", + handler: async (_req, url, m) => { + const r = url.searchParams.get("repo") || repo; + if (!r) return json({ error: "no repo configured or specified" }, { status: 400 }); + const issueNumber = Number(m[1]); + const pr = await github.prForIssue(r, issueNumber); + if (!pr) return json({ error: "no open PR closes that issue" }, { status: 404 }); + const cwd = join(workspacesFor(r), `issue-${issueNumber}`); + const [diff, ci, gate, verify] = await Promise.all([ + github.prDiff(r, pr.number), + ciStatusNow(github, r, pr.number), + readGateEvidence(cwd, issueNumber), + readStageArtifacts(cwd, issueNumber, "verify"), + ]); + const stages = state.listStageRuns(r, { issue: issueNumber }).map(plainStage); + return json({ repo: r, issue: issueNumber, pr: { number: pr.number, url: pr.url }, diff: plain(diff), ci, gate, verify: verify.json ?? null, stages }); + }, + }, + { + // The dashboard's "Approve and merge" action: the same readiness gate + // and merge primitive as the automated policy, invoked as the operator + // (decideOperatorMerge, src/merge-policy.ts). + method: "POST", + pattern: /^\/api\/issues\/(\d+)\/merge$/, + label: "POST /api/issues/:n/merge", + handler: async (req, _url, m) => { + const body = (await req.json().catch(() => ({}))) as { repo?: string }; + const r = body.repo || repo; + if (!r) return json({ error: "no repo configured or specified" }, { status: 400 }); + const issueNumber = Number(m[1]); + const pr = await github.prForIssue(r, issueNumber); + if (!pr) return json({ error: "no open PR closes that issue" }, { status: 404 }); + const [readiness, ci] = await Promise.all([github.mergeReadiness(r, pr.number), ciStatusNow(github, r, pr.number)]); + const decision = decideOperatorMerge(readiness, ci); + const merged = await attemptMerge(github, r, pr.number, decision); + await github.commentIssue(r, issueNumber, renderOperatorAuditComment(decision)); + if (merged) invalidateIssuesCache(r); + return json({ ok: merged, decision }); + }, + }, { method: "GET", pattern: /^\/api\/runs\/(\d+)\/events$/, @@ -337,21 +418,31 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin method: "GET", pattern: /^\/api\/inbox$/, label: "GET /api/inbox", - handler: async () => needRepo() ?? json({ repo, items: buildInbox(await cachedIssues()) }), + // Aggregated across every repo under FACTORY_HOME (v2.9.0 item 2), or + // narrowed to one with ?repo=. Each item carries its own repo so the + // frontend can render and act on a mixed list. + handler: async (_req, url) => { + const filter = url.searchParams.get("repo"); + const repos = filter ? [filter] : inboxRepos(); + if (repos.length === 0) return json({ error: "no repo configured or discovered" }, { status: 500 }); + const perRepo = await Promise.all(repos.map(async (r) => buildInbox(await cachedIssuesFor(r)).map((i) => ({ ...i, repo: r })))); + const items = perRepo.flat().sort((a, b) => (a.waitingSince ?? "").localeCompare(b.waitingSince ?? "") || a.issue - b.issue); + return json({ repos: inboxRepos(), repo, items }); + }, }, { method: "POST", pattern: /^\/api\/inbox\/(\d+)\/act$/, label: "POST /api/inbox/:n/act", handler: async (req, _url, m) => { - const missing = needRepo(); - if (missing) return missing; - const body = (await req.json()) as { action?: InboxAction; text?: string }; - const item = await inboxItem(Number(m[1])); + const body = (await req.json()) as { action?: InboxAction; text?: string; repo?: string }; + const r = body.repo || repo; + if (!r) return json({ error: "no repo configured or specified" }, { status: 400 }); + const item = await inboxItem(r, Number(m[1])); if (!item) return json({ error: "nothing is waiting on that issue" }, { status: 404 }); try { - const posted = await act(github, repo, item, body.action as InboxAction, body.text ?? ""); - boardCache = null; + const posted = await act(github, r, item, body.action as InboxAction, body.text ?? ""); + invalidateIssuesCache(r); return json({ ok: true, posted }); } catch (err) { if (err instanceof InboxError) return json({ error: err.message }, { status: 400 }); diff --git a/package.json b/package.json index 2e41ff5..0e41353 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "software-factory", - "version": "2.8.0", + "version": "2.9.0", "private": true, "type": "module", "description": "GitHub-native SDLC loop for coding agents: triage, plan, build, verify, PR.", diff --git a/src/github.ts b/src/github.ts index 3a84fba..1f756e3 100644 --- a/src/github.ts +++ b/src/github.ts @@ -88,6 +88,11 @@ export interface MergeReadiness { readonly mergeable: "MERGEABLE" | "CONFLICTING" | "UNKNOWN"; readonly reviewDecision: "APPROVED" | "CHANGES_REQUESTED" | "REVIEW_REQUIRED" | ""; readonly hasUnresolvedReviewThreads: boolean; + // Every CHANGES_REQUESTED review is against a commit the head has since + // moved past (v2.9.0 item 3, from assembler's unmerged fix branch, idea + // only, no code copied): a reviewer who asked for changes on an old commit + // no longer blocks a PR that has since been updated. + readonly changesRequestedStale: boolean; } class GhError extends Error { @@ -316,7 +321,7 @@ export class GitHub { }; const [owner, name] = repo.split("/"); if (!owner || !name) throw new Error(`repo must be "owner/name", got ${JSON.stringify(repo)}`); - const hasUnresolvedReviewThreads = await this.hasUnresolvedReviewThreads(owner, name, number); + const { hasUnresolvedReviewThreads, changesRequestedStale } = await this.reviewState(owner, name, number, data.headRefOid); return { state: data.state.toLowerCase() as MergeReadiness["state"], isDraft: data.isDraft, @@ -325,11 +330,20 @@ export class GitHub { mergeable: (data.mergeable || "UNKNOWN") as MergeReadiness["mergeable"], reviewDecision: (data.reviewDecision ?? "") as MergeReadiness["reviewDecision"], hasUnresolvedReviewThreads, + changesRequestedStale, }; } - private async hasUnresolvedReviewThreads(owner: string, name: string, number: number): Promise { - const query = `query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){reviewThreads(first:100){nodes{isResolved}}}}}`; + // One GraphQL call for both readiness facts that `gh pr view` can't expose: + // unresolved review threads, and whether every CHANGES_REQUESTED review is + // against a commit the head has since moved past. + private async reviewState( + owner: string, + name: string, + number: number, + headRefOid: string, + ): Promise<{ hasUnresolvedReviewThreads: boolean; changesRequestedStale: boolean }> { + const query = `query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){reviewThreads(first:100){nodes{isResolved}}reviews(last:50){nodes{state commit{oid}}}}}}`; const result = await this.exec([ "api", "graphql", "-f", `query=${query}`, @@ -338,9 +352,37 @@ export class GitHub { "-F", `number=${number}`, ]); const data = JSON.parse(result.stdout) as { - data: { repository: { pullRequest: { reviewThreads: { nodes: { isResolved: boolean }[] } } } }; + data: { + repository: { + pullRequest: { + reviewThreads: { nodes: { isResolved: boolean }[] }; + reviews: { nodes: { state: string; commit: { oid: string } | null }[] }; + }; + }; + }; }; - return data.data.repository.pullRequest.reviewThreads.nodes.some((t) => !t.isResolved); + const pr = data.data.repository.pullRequest; + const changesRequested = pr.reviews.nodes.filter((r) => r.state === "CHANGES_REQUESTED"); + return { + hasUnresolvedReviewThreads: pr.reviewThreads.nodes.some((t) => !t.isResolved), + changesRequestedStale: changesRequested.length > 0 && changesRequested.every((r) => r.commit?.oid !== headRefOid), + }; + } + + // The literal diff a human would read on the PR's "Files changed" tab. + async prDiff(repo: string, prNumber: number): Promise { + const result = await this.exec(["pr", "diff", String(prNumber), "--repo", repo]); + return result.stdout; + } + + // The one open PR that closes this issue, if any, shared by watch.ts's + // "someone else already closes this issue" triage check and the dashboard's + // review view, so the lookup is defined in exactly one place. + async prForIssue(repo: string, issueNumber: number, opts?: { excludeHead?: string }): Promise { + const prs = await this.listPrs(repo, { state: "open" }); + return prs.find( + (p) => (!opts?.excludeHead || p.headRefName !== opts.excludeHead) && p.closingIssuesReferences?.some((r) => r.number === issueNumber), + ); } // --squash --match-head-commit refuses the merge if the PR's head moved diff --git a/src/merge-policy.ts b/src/merge-policy.ts index 3067e58..21e0bb1 100644 --- a/src/merge-policy.ts +++ b/src/merge-policy.ts @@ -59,7 +59,13 @@ export function checkReadiness(readiness: MergeReadiness, ci: CiResult, expected } else if (ci.status !== "passed") { refusals.push({ reason: "ci-failed", detail: `CI status is "${ci.status}"` }); } - if (readiness.reviewDecision === "CHANGES_REQUESTED" || readiness.reviewDecision === "REVIEW_REQUIRED") { + // A CHANGES_REQUESTED review whose commit the head has since moved past no + // longer blocks (v2.9.0 item 3): the reviewer asked for changes on a commit + // that isn't the one about to merge. + if ( + readiness.reviewDecision === "REVIEW_REQUIRED" || + (readiness.reviewDecision === "CHANGES_REQUESTED" && !readiness.changesRequestedStale) + ) { refusals.push({ reason: "reviews-required", detail: `review decision is "${readiness.reviewDecision}"` }); } if (readiness.hasUnresolvedReviewThreads) { @@ -147,6 +153,34 @@ export function mergePolicyMarker(headSha: string): string { return ``; } +// The dashboard's "Approve and merge" button (v2.9.0 item 1): a human is the +// authority here, not repo config, so this skips autoEligible's risk/path/size +// gates entirely (herdr SKILL.md:18-23, "authority is explicit only": a +// human click is a different authority source than the auto-policy). It +// reuses checkReadiness with the PR's own baseRefName as the "expected" one, +// since an operator merge has no earlier plan snapshot to compare against; +// it is evaluated fresh, at click time. Readiness refusals (draft, CI, +// unresolved threads, ...) still apply: a human can approve, but the PR +// itself must still be mergeable. +export function decideOperatorMerge(readiness: MergeReadiness, ci: CiResult): MergeDecision { + const headSha = readiness.headRefOid; + const refusals = checkReadiness(readiness, ci, readiness.baseRefName); + if (refusals.length > 0) return { outcome: "refuse", headSha, refusals }; + return { outcome: "merge", headSha }; +} + +// A separate function from renderAuditComment, so the ported test's exact +// "auto"/"dry-run" wording assertions never have to account for a third, +// operator-triggered source. +export function renderOperatorAuditComment(decision: MergeDecision): string { + const marker = mergePolicyMarker(decision.headSha); + if (decision.outcome === "merge") { + return `${marker}\nMerge policy: **operator**, approved and merged \`${decision.headSha.slice(0, 7)}\` from the dashboard.`; + } + const lines = decision.refusals.map((r) => `- **${r.reason}**: ${r.detail}`); + return [marker, "Merge policy: **operator**, blocked.", ...(lines.length ? ["", "Refusals:", ...lines] : [])].join("\n"); +} + // The comment posted to the PR either way: what was decided, and why, so a // human reviewing a "dry-run" or "refuse" outcome sees every reason at once. export function renderAuditComment(decision: MergeDecision): string { diff --git a/src/paths.ts b/src/paths.ts index 5cc4f3a..7558f2c 100644 --- a/src/paths.ts +++ b/src/paths.ts @@ -7,6 +7,7 @@ // the repo dir), the agent ran outside a git checkout. FACTORY_HOME fixes // the ambiguity once, for local, Docker and CI alike (audit finding #1). +import { existsSync, readdirSync } from "node:fs"; import { resolve } from "node:path"; export function factoryHome(env: NodeJS.ProcessEnv = process.env): string { @@ -37,6 +38,27 @@ export function reposDir(env: NodeJS.ProcessEnv = process.env): string { return `${factoryHome(env)}/repos`; } +// The legacy top-level dirs under FACTORY_HOME that are not an owner name. +const RESERVED_TOP_LEVEL = new Set(["repos", "workspaces"]); + +// Every "owner/name" with a state DB under FACTORY_HOME, for the multi-repo +// inbox (v2.9.0 item 2, pulled forward from v3.0's blueprint plan). A repo +// only counts once it has run at least once (its factory.db exists), so a +// stray empty directory never shows up as a phantom repo. +export function discoverRepos(env: NodeJS.ProcessEnv = process.env): string[] { + const home = factoryHome(env); + if (!existsSync(home)) return []; + const repos: string[] = []; + for (const owner of readdirSync(home, { withFileTypes: true })) { + if (!owner.isDirectory() || RESERVED_TOP_LEVEL.has(owner.name)) continue; + const ownerDir = `${home}/${owner.name}`; + for (const name of readdirSync(ownerDir, { withFileTypes: true })) { + if (name.isDirectory() && existsSync(`${ownerDir}/${name.name}/factory.db`)) repos.push(`${owner.name}/${name.name}`); + } + } + return repos.sort(); +} + // Where a given issue's worktree lives, always absolute regardless of what // cwd the process was started from. export function worktreePath(issue: number, env: NodeJS.ProcessEnv = process.env, repo?: string): string { diff --git a/src/watch.ts b/src/watch.ts index 3aacccf..c21c01f 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -366,9 +366,7 @@ async function runFromStage( if (stage === "triage") { // Someone else's open PR already closes this issue: don't spend tokens on a second fix. const own = deps.git.branchName(issueNumber); - const taken = (await deps.github.listPrs(config.repo, { state: "open" })).find( - (p) => p.headRefName !== own && p.closingIssuesReferences?.some((r) => r.number === issueNumber), - ); + const taken = await deps.github.prForIssue(config.repo, issueNumber, { excludeHead: own }); if (taken) { await moveLabel(deps, config, issueNumber, LABEL.triaging, LABEL.needsHuman); finish(deps, config, issueNumber, "needs-human", `open PR #${taken.number} already closes this issue`); diff --git a/template-ci/factory.yml.example b/template-ci/factory.yml.example index c02dc89..1f966eb 100644 --- a/template-ci/factory.yml.example +++ b/template-ci/factory.yml.example @@ -35,7 +35,7 @@ on: required: false env: - FACTORY_RUNNER_REF: v2.8.0 # pinned software-factory release; bump deliberately + FACTORY_RUNNER_REF: v2.9.0 # pinned software-factory release; bump deliberately FACTORY_RUNNER_REPO: learnwithparam/software-factory CLAUDE_CODE_VERSION: "2.1.281" # pinned claude, same version the Dockerfile installs # Pins for every agent. The "Install agents" step installs the ones named in the repo VARIABLE diff --git a/tests/dashboard-routes.test.ts b/tests/dashboard-routes.test.ts index 12055e1..1435daa 100644 --- a/tests/dashboard-routes.test.ts +++ b/tests/dashboard-routes.test.ts @@ -7,12 +7,13 @@ import { afterAll, describe, expect, test } from "bun:test"; import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { GitHub, type GhIssue } from "../src/github"; +import { GitHub, type GhIssue, type GhPr, type MergeReadiness, type PrStatus } from "../src/github"; import { LABEL } from "../src/labels"; import { FactoryState, type StageRunInput } from "../src/state"; class FakeGitHub extends GitHub { posted: { issue: number; body: string }[] = []; + merged: { repo: string; prNumber: number; headSha: string }[] = []; constructor(private readonly issues: GhIssue[] = []) { super(); } @@ -26,6 +27,25 @@ class FakeGitHub extends GitHub { this.posted.push({ issue, body }); return 1; } + // The review/merge routes (v2.9.0) look up the PR that closes an issue, its + // diff and CI status, and its merge readiness. One fake PR per known issue + // is enough for the route-walk and inbox tests, which never depend on its shape. + override async prForIssue(_repo: string, issueNumber: number): Promise { + if (!this.issues.some((i) => i.number === issueNumber)) return undefined; + return { number: 100 + issueNumber, url: `https://github.com/acme/widgets/pull/${100 + issueNumber}`, state: "open", headRefName: `issue-${issueNumber}`, isDraft: false, closingIssuesReferences: [{ number: issueNumber }] }; + } + override async prDiff(): Promise { + return "diff --git a/file b/file\n+added\n"; + } + override async prStatus(): Promise { + return { state: "open", headRefOid: "sha1", closingIssuesReferences: [{ number: 1 }], statusCheckRollup: [{ name: "build", status: "COMPLETED", conclusion: "SUCCESS" }] }; + } + override async mergeReadiness(): Promise { + return { state: "open", isDraft: false, baseRefName: "main", headRefOid: "sha1", mergeable: "MERGEABLE", reviewDecision: "APPROVED", hasUnresolvedReviewThreads: false, changesRequestedStale: false }; + } + override async mergePr(repo: string, prNumber: number, headSha: string): Promise { + this.merged.push({ repo, prNumber, headSha }); + } } const waiting = (n: number, label: string): GhIssue => ({ @@ -129,6 +149,101 @@ describe("inbox routes", () => { }); }); +describe("review and merge routes", () => { + test("the review route returns the diff, CI status and merge readiness for the PR that closes the issue", async () => { + const { dashboard } = await make("", [waiting(5, LABEL.inReview)]); + const res = await dashboard.handle(new Request("http://localhost:4100/api/issues/5/review"), "127.0.0.1"); + expect(res.status).toBe(200); + const body = (await res.json()) as { pr: { number: number }; diff: string; ci: { status: string }; gate: unknown; verify: unknown; stages: unknown[] }; + expect(body.pr.number).toBe(105); + expect(body.diff).toContain("diff --git"); + expect(body.ci.status).toBe("passed"); + expect(body.gate).toBeUndefined(); + expect(body.verify).toBeNull(); + expect(body.stages).toEqual([]); + }); + + test("the review route 404s when no open PR closes the issue", async () => { + const { dashboard } = await make("", []); + const res = await dashboard.handle(new Request("http://localhost:4100/api/issues/9/review"), "127.0.0.1"); + expect(res.status).toBe(404); + }); + + test("approve-and-merge calls the same mergePr the automated policy uses, and posts the operator audit comment", async () => { + const { dashboard, github } = await make("", [waiting(6, LABEL.inReview)]); + const res = await dashboard.handle( + new Request("http://localhost:4100/api/issues/6/merge", { method: "POST", headers: { "content-type": "application/json" }, body: "{}" }), + "127.0.0.1", + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { ok: boolean; decision: { outcome: string } }; + expect(body.ok).toBe(true); + expect(body.decision.outcome).toBe("merge"); + expect(github.merged).toEqual([{ repo: "acme/widgets", prNumber: 106, headSha: "sha1" }]); + expect(github.posted[0]!.body).toContain("operator"); + }); + + test("approve-and-merge refuses (and never calls mergePr) when readiness fails, still posting the audit comment", async () => { + const { dashboard, github } = await make("", [waiting(7, LABEL.inReview)]); + const original = github.mergeReadiness.bind(github); + github.mergeReadiness = async () => ({ ...(await original()), isDraft: true }); + const res = await dashboard.handle( + new Request("http://localhost:4100/api/issues/7/merge", { method: "POST", headers: { "content-type": "application/json" }, body: "{}" }), + "127.0.0.1", + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { ok: boolean; decision: { outcome: string; refusals?: { reason: string }[] } }; + expect(body.ok).toBe(false); + expect(body.decision.outcome).toBe("refuse"); + expect(body.decision.refusals?.map((r) => r.reason)).toContain("draft"); + expect(github.merged).toEqual([]); + expect(github.posted[0]!.body).toContain("blocked"); + }); + + test("the merge route 404s when no open PR closes the issue", async () => { + const { dashboard } = await make("", []); + const res = await dashboard.handle(new Request("http://localhost:4100/api/issues/9/merge", { method: "POST", headers: { "content-type": "application/json" }, body: "{}" }), "127.0.0.1"); + expect(res.status).toBe(404); + }); +}); + +describe("multi-repo inbox", () => { + test("aggregates across every repo under FACTORY_HOME, sorted by wait time, and ?repo= narrows it to one", async () => { + const before = process.env.FACTORY_REPOS; + process.env.FACTORY_REPOS = "acme/widgets,acme/gadgets"; + try { + const { dashboard } = await make("", [waiting(3, LABEL.awaitingApproval)]); + const all = (await (await dashboard.handle(new Request("http://localhost:4100/api/inbox"), "127.0.0.1")).json()) as { + repos: string[]; items: { issue: number; repo: string }[]; + }; + expect(all.repos).toEqual(["acme/widgets", "acme/gadgets"]); + // FakeGitHub answers the same issue list for every repo, so the same + // issue #3 shows up once per repo, each tagged with its own repo. + expect(all.items.map((i) => [i.repo, i.issue]).sort()).toEqual([ + ["acme/gadgets", 3], + ["acme/widgets", 3], + ]); + const filtered = await dashboard.handle(new Request("http://localhost:4100/api/inbox?repo=acme/widgets"), "127.0.0.1"); + expect(filtered.status).toBe(200); + const body = (await filtered.json()) as { items: { issue: number; repo: string }[] }; + expect(body.items).toEqual([{ ...body.items[0]!, issue: 3, repo: "acme/widgets" }]); + } finally { + if (before === undefined) delete process.env.FACTORY_REPOS; + else process.env.FACTORY_REPOS = before; + } + }); + + test("inbox/:n/act resolves the repo from the request body, defaulting to the configured repo", async () => { + const { dashboard, github } = await make("", [waiting(3, LABEL.awaitingApproval)]); + const res = await dashboard.handle( + new Request("http://localhost:4100/api/inbox/3/act", { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify({ action: "approve", repo: "acme/widgets" }) }), + "127.0.0.1", + ); + expect(res.status).toBe(200); + expect(github.posted).toEqual([{ issue: 3, body: "/factory approve" }]); + }); +}); + describe("analytics and stages", () => { const stage = (i: number, agent: string): StageRunInput => ({ repo: "acme/widgets", issue: 1 + (i % 3), stage: "build", agent, model: null, started_at: "2026-09-24T00:00:00Z", diff --git a/tests/merge-policy-operator.test.ts b/tests/merge-policy-operator.test.ts new file mode 100644 index 0000000..12598ae --- /dev/null +++ b/tests/merge-policy-operator.test.ts @@ -0,0 +1,93 @@ +// v2.9.0: the dashboard's "Approve and merge" button reuses the same +// readiness gate and merge primitive as the automated policy, and a +// CHANGES_REQUESTED review superseded by new commits no longer blocks. + +import { expect, test } from "bun:test"; +import type { CiCheck, CiResult } from "../src/ci"; +import type { MergeReadiness } from "../src/github"; +import { attemptMerge, checkReadiness, decideMerge, decideOperatorMerge, renderOperatorAuditComment, type DecideMergeInput } from "../src/merge-policy"; + +const passingCheck: CiCheck = { name: "build", status: "COMPLETED", conclusion: "SUCCESS" }; + +function readiness(overrides: Partial = {}): MergeReadiness { + return { + state: "open", + isDraft: false, + baseRefName: "main", + headRefOid: "sha1", + mergeable: "MERGEABLE", + reviewDecision: "APPROVED", + hasUnresolvedReviewThreads: false, + changesRequestedStale: false, + ...overrides, + }; +} + +function ci(overrides: Partial = {}): CiResult { + return { prNumber: 1, headSha: "sha1", status: "passed", checks: [passingCheck], ...overrides }; +} + +function fakeMergePr() { + const calls: { repo: string; prNumber: number; headSha: string }[] = []; + return { + calls, + mergePr: async (repo: string, prNumber: number, headSha: string) => { + calls.push({ repo, prNumber, headSha }); + }, + }; +} + +test("a CHANGES_REQUESTED review against a superseded commit no longer blocks", () => { + const stale = checkReadiness(readiness({ reviewDecision: "CHANGES_REQUESTED", changesRequestedStale: true }), ci(), "main"); + expect(stale.map((r) => r.reason)).not.toContain("reviews-required"); + + const fresh = checkReadiness(readiness({ reviewDecision: "CHANGES_REQUESTED", changesRequestedStale: false }), ci(), "main"); + expect(fresh.map((r) => r.reason)).toContain("reviews-required"); +}); + +test("decideOperatorMerge merges a clean PR without any risk/path/size gate", () => { + const decision = decideOperatorMerge(readiness(), ci()); + expect(decision.outcome).toBe("merge"); + if (decision.outcome === "merge") expect(decision.headSha).toBe("sha1"); +}); + +test("decideOperatorMerge refuses on the same readiness problems as the auto policy", () => { + const draft = decideOperatorMerge(readiness({ isDraft: true }), ci()); + expect(draft.outcome).toBe("refuse"); + if (draft.outcome === "refuse") expect(draft.refusals.map((r) => r.reason)).toContain("draft"); + + const failedCi = decideOperatorMerge(readiness(), ci({ status: "failed" })); + expect(failedCi.outcome).toBe("refuse"); + if (failedCi.outcome === "refuse") expect(failedCi.refusals.map((r) => r.reason)).toContain("ci-failed"); +}); + +test("the operator audit comment always carries the head-sha marker, with wording distinct from the auto comment", () => { + const merged = decideOperatorMerge(readiness(), ci()); + const comment = renderOperatorAuditComment(merged); + expect(comment).toContain(""); + expect(comment).toContain("operator"); + expect(comment).not.toContain("**auto**"); +}); + +test("one merge function serves both the automated policy and the operator button", async () => { + const github = fakeMergePr(); + + const autoInput: DecideMergeInput = { + readiness: readiness(), + ci: ci(), + risk: "low", + changedFiles: [{ path: "docs/guide.md", additions: 1, deletions: 0 }], + merge: { policy: "auto", autoPaths: ["docs/**"], maxFiles: 10, maxLines: 200 }, + protectedPaths: [], + expectedBaseRefName: "main", + }; + expect(await attemptMerge(github, "o/r", 1, decideMerge(autoInput))).toBe(true); + + const operatorDecision = decideOperatorMerge(readiness({ headRefOid: "sha2" }), ci({ headSha: "sha2" })); + expect(await attemptMerge(github, "o/r", 2, operatorDecision)).toBe(true); + + expect(github.calls).toEqual([ + { repo: "o/r", prNumber: 1, headSha: "sha1" }, + { repo: "o/r", prNumber: 2, headSha: "sha2" }, + ]); +}); diff --git a/tests/ported/machinist/merge-policy.test.ts b/tests/ported/machinist/merge-policy.test.ts index c218a67..becb152 100644 --- a/tests/ported/machinist/merge-policy.test.ts +++ b/tests/ported/machinist/merge-policy.test.ts @@ -26,6 +26,7 @@ function readiness(overrides: Partial = {}): MergeReadiness { mergeable: "MERGEABLE", reviewDecision: "APPROVED", hasUnresolvedReviewThreads: false, + changesRequestedStale: false, ...overrides, }; } diff --git a/tests/scenarios.test.ts b/tests/scenarios.test.ts index bc2ee32..1acbfaa 100644 --- a/tests/scenarios.test.ts +++ b/tests/scenarios.test.ts @@ -590,6 +590,7 @@ describe("merge policy on an in-review PR", () => { mergeable: "MERGEABLE", reviewDecision: "APPROVED", hasUnresolvedReviewThreads: false, + changesRequestedStale: false, }); return pr; }