From 8cfbed627ecdd9da7125b31371c122675e110ddd Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 18 Oct 2026 23:23:38 -0700 Subject: [PATCH] fix(triage): keep a pull request's verdict label while a new commit is checked --- apps/hub/src/github/bridge.ts | 6 +- apps/hub/src/github/coalescer.test.ts | 8 ++ apps/hub/src/github/coalescer.ts | 6 +- apps/hub/src/github/triage-reconciler.ts | 2 +- docs/ARCHITECTURE.md | 12 ++- .../Corbits Triage User Manual.md | 6 +- docs/PRODUCT.md | 13 +-- packages/github-tool/src/github.ts | 5 +- packages/github-tool/src/sidecar-bundle.ts | 6 +- .../triage-workflows/src/actions/evaluate.ts | 4 +- .../triage-workflows/src/actions/waiting.ts | 50 ++++++++---- packages/triage-workflows/src/directors.ts | 4 +- .../triage-workflows/src/pr-triage.test.ts | 79 ++++++++++++++++--- packages/triage-workflows/src/triage-steps.ts | 7 +- 14 files changed, 153 insertions(+), 55 deletions(-) diff --git a/apps/hub/src/github/bridge.ts b/apps/hub/src/github/bridge.ts index 87e7d2e62..da397a254 100644 --- a/apps/hub/src/github/bridge.ts +++ b/apps/hub/src/github/bridge.ts @@ -184,9 +184,9 @@ export function mailPayload(record: Pick, policy: }; } -/** The mail for a head held for its CI: triage marks it waiting where it writes the verdict, and triages nothing. */ -export function waitingPayload(record: Pick, policy: RepoPolicy, pack: StoredPack, head: { number: number; headSha: string }) { - return { ...mailPayload(record, policy, pack, { prNumber: head.number, headSha: head.headSha }), kind: WAITING_KIND }; +/** The mail for a head held for its CI: triage marks it waiting where it writes the verdict, and triages nothing. `noCi` names a head with no checks yet. */ +export function waitingPayload(record: Pick, policy: RepoPolicy, pack: StoredPack, head: { number: number; headSha: string }, noCi: boolean) { + return { ...mailPayload(record, policy, pack, { prNumber: head.number, headSha: head.headSha, ...(noCi && { noCi }) }), kind: WAITING_KIND }; } export async function resolvedPack( diff --git a/apps/hub/src/github/coalescer.test.ts b/apps/hub/src/github/coalescer.test.ts index 1f40c0659..4a2ca3810 100644 --- a/apps/hub/src/github/coalescer.test.ts +++ b/apps/hub/src/github/coalescer.test.ts @@ -343,6 +343,14 @@ test("a head held for CI where triage writes labels is mailed to be marked waiti expect(mails.map((mail) => mail.kind)).toEqual(["waiting", "pr"]); }); +test("a head with no checks yet is mailed to be marked waiting as having no CI", async () => { + const { clock, mails, coalescer } = harness([], { headChecks: async () => [] }); + const pack = { ...emptyPack(REPO), checks: { "ci-green": { enabled: true } } }; + await coalescer.accept({ ...delivery("updated", "sha2"), pack, automatic: ["labels" as const] }); + await clock.advance(WINDOW.burstMs); + expect(mails).toEqual([expect.objectContaining({ kind: "waiting", headSha: "sha2", noCi: true })]); +}); + function suiteCompleted(headSha: string): CoalescedEvent { const input = delivery("checks", headSha); return { ...input, mail: { ...input.mail, event: "check_suite" } }; diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index 8e57f860f..e0a1cb62d 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -173,10 +173,10 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { } /** The waiting marker is only shown, so a mail that did not go out is logged and the head still waits. */ - async function markWaiting({ latest, head }: Open): Promise { + async function markWaiting({ latest, head }: Open, noCi: boolean): Promise { if (!marksWaiting(sourceOf(latest.record), latest.automatic)) return; try { - await deps.sendMail({ ...head, workflow: latest.workflow, source: "event", payload: waitingPayload(latest.record, latest.policy, latest, head) }); + await deps.sendMail({ ...head, workflow: latest.workflow, source: "event", payload: waitingPayload(latest.record, latest.policy, latest, head, noCi) }); } catch (err) { deps.log({ level: "warn", msg: "waiting_mail_failed", tenantId: head.tenantId, repo: latest.record.name, pr: head.number, headSha: head.headSha, error: String(err) }); } @@ -207,7 +207,7 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { if (readsCi && waitsForCi(ci, firstSeenAt, deps.now(), deps.ciWait)) { await waitHead(deps.store, head, events, deps.now()); deps.log({ level: "info", msg: "triage_waiting_ci", ...entry, events, ci }); - if (prior?.status !== "waiting") await markWaiting(open); + if (prior?.status !== "waiting") await markWaiting(open, ci === "none"); return; } try { diff --git a/apps/hub/src/github/triage-reconciler.ts b/apps/hub/src/github/triage-reconciler.ts index 61f9ef66b..02151a127 100644 --- a/apps/hub/src/github/triage-reconciler.ts +++ b/apps/hub/src/github/triage-reconciler.ts @@ -226,7 +226,7 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { if (!marksWaiting(sourceOf(repo.record), repo.automatic)) return; const waited = new Set(repo.stored.rows.filter((row) => row.status === "waiting").map(headKey)); for (const row of rows.filter((candidate) => candidate.status === "waiting" && !waited.has(headKey(candidate)))) { - await deps.send({ tenantId: pass.tenantId, workflow: triageWorkflowFor(deps.source).id, repo: triageStateKey(repo.record), number: row.number, headSha: row.headSha, source: "reconcile", payload: waitingPayload(repo.record, repo.policy, repo, row) }); + await deps.send({ tenantId: pass.tenantId, workflow: triageWorkflowFor(deps.source).id, repo: triageStateKey(repo.record), number: row.number, headSha: row.headSha, source: "reconcile", payload: waitingPayload(repo.record, repo.policy, repo, row, false) }); } } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0067aac85..bdd86de29 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -151,7 +151,9 @@ name per tenant; no deployment ids are configured anywhere. ## Triage pipeline Every run is the same steps; the repository's policy decides what each one -does. +does. Before facts, a pull request mail from the hub naming a GitHub head +mirrors `unready:--::`, which takes the pass +label off that head, so it never stands on a head not yet triaged. 1. **facts**: reads the pull request, its files, reviews, CI, commits (the first 100 messages in full), the repository's docs and its contribution @@ -188,7 +190,13 @@ does. waiting mail skips every other step: `markWaiting` writes only the waiting label or `INPROGRESS` build status under the effect id `waiting:--::`, and nothing once the pull - request moved past that head. + request moved past that head. On GitHub the marker passes the fail and + unsure verdict labels as `keep`, so a pull request carrying one keeps its + labels until the verdict, which swaps them only when it differs. A head + with no checks (the coalescer's `noCi` on the waiting mail, or facts' + `checks: "none"` in a run) gets no waiting label: the marker only takes the + pass label off. The portal reads the wait from the head's triage-state + row, not the label. 8. **execute**: runs the actions whose kind the `pr-triage` role holds on the repository. Every other action is held in the verdict (`held: true`). diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 78701bf99..44daedb4f 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -257,9 +257,9 @@ Under Code review, **Actions Triage takes on its own** lists each action with it Enabling a repository switches on Comment and Add labels (Comment only on Bitbucket, which has no labels); every other action waits for a person to click until you switch it on, so you can watch what Triage suggests before you let it act. Turning triage off and on again switches them back on. Two actions are always held for a person: Close on Unsure, and Merge on anything but Pass. -When the policy depends on CI (the **CI must pass** rule is on, a step has a CI condition, or a step approves or merges), Triage waits for a pull request's CI to finish before it checks it, so it gives one result instead of an early Unsure. Meanwhile the Inbox shows **Waiting for CI** (**Waiting for CI · new commits since Triage checked** when an earlier result is still on show). A pull request that already needed you stays in Needs you meanwhile; one Triage has not checked yet waits under **Waiting**. The pull request's page notes **New commits since Triage checked** above the earlier result, which stays until the new one lands. The pull request itself shows **Triage: Waiting for CI**, as a label on GitHub where Triage adds labels on its own and as the **Corbits Triage** build status on Bitbucket where Triage comments on its own. The verdict replaces it once Triage has checked the pull request. Triage goes ahead as soon as any check fails, after two minutes when the repository has no CI, and after an hour at most. **Triage again** does not wait. +When the policy depends on CI (the **CI must pass** rule is on, a step has a CI condition, or a step approves or merges), Triage waits for a pull request's CI to finish before it checks it, so it gives one result instead of an early Unsure. Meanwhile the Inbox shows **Waiting for CI** (**Waiting for CI · new commits since Triage checked** when an earlier result is still on show). A pull request that already needed you stays in Needs you meanwhile; one Triage has not checked yet waits under **Waiting**. The pull request's page notes **New commits since Triage checked** above the earlier result, which stays until the new one lands. On GitHub, where Triage adds labels on its own, a pull request labelled **Triage: Not Ready** or **Triage: Needs Human Review** keeps it, so new commits change no label unless the verdict changes; any other, **Triage: Ready** included, shows **Triage: Waiting for CI**, or no Triage label while the commit has no checks. On Bitbucket, where Triage comments on its own, the new commit's **Corbits Triage** build status shows **Triage: Waiting for CI**. The verdict replaces it once Triage has checked the pull request. Triage goes ahead as soon as any check fails, after two minutes when the repository has no CI, and after an hour at most. **Triage again** does not wait. -If Triage checks a pull request anyway while the only rules it cannot confirm yet are waiting on the host (CI still running, or the host has not yet worked out whether the pull request can merge), it gives no verdict yet: the pull request keeps **Triage: Waiting for CI**, nothing is posted and no action runs, and Triage checks it again once they are done. After an hour, on **Triage again**, or when the host still has not worked out mergeability after Triage's last retry, it gives the verdict anyway. If a step depends on CI that is still running, Triage holds every action, and its comment on the pull request lists them under **Waiting for checks to finish**. They go ahead once the checks are done. +If Triage checks a pull request anyway while the only rules it cannot confirm yet are waiting on the host (CI still running, or the host has not yet worked out whether the pull request can merge), it gives no verdict yet: the pull request keeps its label or build status as above, nothing is posted and no action runs, and Triage checks it again once they are done. After an hour, on **Triage again**, or when the host still has not worked out mergeability after Triage's last retry, it gives the verdict anyway. If a step depends on CI that is still running, Triage holds every action, and its comment on the pull request lists them under **Waiting for checks to finish**. They go ahead once the checks are done. An **Unsure** comment lists each rule Triage could not confirm, with checks still running under **Waiting on**. @@ -369,7 +369,7 @@ Anything you do here is also written to the audit log. ### What the author sees -Authors do not use the portal. Depending on the repository's actions, they see a comment from Triage on their pull request with the verdict and reasons, review requests, and on GitHub labels such as **Triage: Ready**, **Triage: Not Ready** or **Triage: Needs Human Review** and assignments. On Bitbucket they see the verdict as the **Corbits Triage** build status instead of a label. While Triage waits for CI, they see **Triage: Waiting for CI** in its place. Triage keeps one comment and edits it in place as the pull request changes, so an author who pushes a fix sees the fresh verdict in the same comment. Each change Triage asks to split out names what it is not part of: the pull request's title. When new commits change the verdict, the comment also says what the verdict was, how many commits arrived since and which pull requests were merged into the branch. Only when someone changes the repository's policy and that changes the verdict on the same commit does Triage post a new comment, which says it supersedes the one above. +Authors do not use the portal. Depending on the repository's actions, they see a comment from Triage on their pull request with the verdict and reasons, review requests, and on GitHub labels such as **Triage: Ready**, **Triage: Not Ready** or **Triage: Needs Human Review** and assignments. On Bitbucket they see the verdict as the **Corbits Triage** build status instead of a label. **Triage: Ready** comes off as soon as Triage sees a new commit and returns once that commit passes, so it never stands on a commit Triage has not checked. While Triage waits for CI, they see **Triage: Waiting for CI** in its place, or no Triage label when the commit has no checks; **Triage: Not Ready** and **Triage: Needs Human Review** stay until the new verdict. Triage keeps one comment and edits it in place as the pull request changes, so an author who pushes a fix sees the fresh verdict in the same comment. Each change Triage asks to split out names what it is not part of: the pull request's title. When new commits change the verdict, the comment also says what the verdict was, how many commits arrived since and which pull requests were merged into the branch. Only when someone changes the repository's policy and that changes the verdict on the same commit does Triage post a new comment, which says it supersedes the one above. ### Pulse diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index 1ec27e0d5..ced2a8621 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -194,9 +194,12 @@ When the policy depends on CI (CI green is on, a step has a `ci` condition, or a step approves or merges), Triage waits for CI to finish on the head, or for its first check to fail, before it triages it. A head with no checks after 2 minutes is taken as having no CI, and none waits longer than an hour; a run by -hand does not wait. Meanwhile the Inbox shows **Waiting for CI**, and the pull -request shows the `Triage: Waiting for CI` label on GitHub (or the `Corbits -Triage` build status in progress on Bitbucket) until the verdict replaces it. +hand does not wait. Meanwhile the Inbox shows **Waiting for CI**. On GitHub a +pull request labelled `Triage: Not Ready` or `Triage: Needs Human Review` +keeps it, so a push that leaves the verdict as it was changes no label; any +other shows `Triage: Waiting for CI` until the verdict replaces it, or no +Triage label while the head has no checks. On Bitbucket the head's `Corbits +Triage` build status is in progress until its verdict. A head triaged anyway while the only rules left to confirm are the host's own (CI still running, mergeability not computed yet) gets no verdict yet: it shows the same marker, nothing is posted or run, and it is triaged again once @@ -211,10 +214,10 @@ Their names are stable: | Label | When | | --- | --- | -| `Triage: Ready` | The head passes. | +| `Triage: Ready` | The head passes. Taken off as soon as Triage sees a new head, until that head passes. | | `Triage: Not Ready` | The head fails. | | `Triage: Needs Human Review` | The head is unsure. | -| `Triage: Waiting for CI` | Triage is waiting for the head's CI before it decides. | +| `Triage: Waiting for CI` | Triage is waiting for the head's CI before it decides, unless the pull request is Not Ready or Needs Human Review or the head has no checks. | | `rule: ` | A rule that failed on the head, such as `rule: docs`, from a `{ do: labels, target: { from: rules } }` step. | A head carries one verdict label at a time, and Triage takes back the ones diff --git a/packages/github-tool/src/github.ts b/packages/github-tool/src/github.ts index e88a210de..a59aa1f58 100644 --- a/packages/github-tool/src/github.ts +++ b/packages/github-tool/src/github.ts @@ -16,6 +16,8 @@ export interface MirrorInput { labels: string[]; /** Labels the caller owns; the mirror removes only these. Optional so requests built before it existed remove nothing. */ owned?: readonly string[]; + /** While the pull request carries any of these, its labels are left as they are. */ + keep?: readonly string[]; comment?: string; /** What the comment is written for; without it the comment is written whatever it said before. */ header?: TriageHeader; @@ -54,7 +56,8 @@ export async function mirror(gh: GithubFetch, i: MirrorInput, login: AppLogin) { if (i.comment === "") await deleteTriageComments(gh, repo, number); const posted = i.comment ? await writeTriageComment(gh, { repo, number, body: i.comment, header: i.header, branch: pr.head.repo?.id === pr.base.repo.id ? pr.head.ref : undefined }, login) : null; const current: string[] = pr.labels.map((label: { name: string }) => label.name); - await syncLabels(gh, { repo, number, current, add: i.labels, remove: i.owned ?? [] }); + const kept = new Set((i.keep ?? []).map(lowercase)); + if (!current.some((name) => kept.has(lowercase(name)))) await syncLabels(gh, { repo, number, current, add: i.labels, remove: i.owned ?? [] }); if (i.close) await json(gh, `/repos/${repo}/pulls/${number}`, send("PATCH", { state: "closed" })); return { commentId: posted?.commentId ?? null, updated: posted?.updated ?? false, closed: i.close, sha: pr.head.sha, ...(posted?.unchanged && { unchanged: posted.unchanged }) }; } diff --git a/packages/github-tool/src/sidecar-bundle.ts b/packages/github-tool/src/sidecar-bundle.ts index 9dedf33ff..68cb7f901 100644 --- a/packages/github-tool/src/sidecar-bundle.ts +++ b/packages/github-tool/src/sidecar-bundle.ts @@ -132,6 +132,7 @@ const MIRROR_INPUT = obj({ number: { type: "integer" }, labels: { type: "array", items: str }, owned: { type: "array", items: str }, + keep: { type: "array", items: str, description: "While the pull request carries any of these labels, its labels are left as they are" }, comment: str, header: { ...obj({ @@ -150,13 +151,13 @@ const WRITE_DEFINITIONS: ToolDefinition[] = [ { name: "github_mirror", description: - "Post (or update) the triage comment on a pull request when comment is given, add labels and remove any owned label not in labels. While notReadyHead is the head, which may not be approved, dismisses the approve Do's standing approvals; while fixedHead is the head, which leaves the author nothing to fix, dismisses the request-changes Do's standing requests. Writes nothing to a closed pull request. Never merges.", + "Post (or update) the triage comment on a pull request when comment is given, add labels and remove any owned label not in labels, unless it carries a keep label. While notReadyHead is the head, which may not be approved, dismisses the approve Do's standing approvals; while fixedHead is the head, which leaves the author nothing to fix, dismisses the request-changes Do's standing requests. Writes nothing to a closed pull request. Never merges.", inputSchema: MIRROR_INPUT, }, { name: "github_mirror_auto", description: - "Post (or update) the triage comment on a pull request when comment is given, without waiting for a human approval, add labels and remove any owned label not in labels. While notReadyHead is the head, which may not be approved, dismisses the approve Do's standing approvals; while fixedHead is the head, which leaves the author nothing to fix, dismisses the request-changes Do's standing requests. Writes nothing to a closed pull request. Never merges.", + "Post (or update) the triage comment on a pull request when comment is given, without waiting for a human approval, add labels and remove any owned label not in labels, unless it carries a keep label. While notReadyHead is the head, which may not be approved, dismisses the approve Do's standing approvals; while fixedHead is the head, which leaves the author nothing to fix, dismisses the request-changes Do's standing requests. Writes nothing to a closed pull request. Never merges.", inputSchema: MIRROR_INPUT, }, { @@ -272,6 +273,7 @@ async function dispatch(gh: GithubFetch, call: ToolCallInput, login: AppLogin, t number: a.number, labels: a.labels, owned: a.owned, + keep: a.keep, comment: a.comment, header: a.header, onHead: a.onHead, diff --git a/packages/triage-workflows/src/actions/evaluate.ts b/packages/triage-workflows/src/actions/evaluate.ts index 067671202..17bfbb757 100644 --- a/packages/triage-workflows/src/actions/evaluate.ts +++ b/packages/triage-workflows/src/actions/evaluate.ts @@ -1,5 +1,5 @@ import type { EffectContext } from "@intx/workflow"; -import { checkName, ownedLabels, repoSourceOf, TRIAGE_LABELS, type ActionKind, type CheckPack } from "@corbits/triage-contracts"; +import { checkName, ownedLabels, repoSourceOf, VERDICT_LABELS, type ActionKind, type CheckPack } from "@corbits/triage-contracts"; import { type } from "arktype"; import { ciRefusal } from "@corbits/github-reads"; import { approvalHeldBy, changesWanted, evaluateActions, heldApprovals, holdDos, packApproves, type SuggestedAction } from "../logic/actions.js"; @@ -191,7 +191,7 @@ function verdictOf(it: Item, replies: unknown[], partReplies: unknown[]): Verdic const reasons = [...rendered.reasons, ...(it.pack?.dropped ?? [])]; const labels = it.pack && !rendered.waiting ? [it.pack.labels[rendered.verdict]] : rendered.labels; const owned = it.pack ? ownedLabels(it.pack.labels) : undefined; - const mark = rendered.waiting && waitingMark(repoSourceOf(it.repoSource), it.facts, automatic, owned ?? TRIAGE_LABELS); + const mark = rendered.waiting && waitingMark(repoSourceOf(it.repoSource), it.facts, automatic, it.pack?.labels ?? VERDICT_LABELS, it.facts.checks !== "none"); const verdict = { ...stamp(it.facts, it), ...rendered, labels, reasons, feedback, automatic, actions, ...(Object.keys(judged).length > 0 && { judged }), ...(it.pack && packApproves(it.pack) && { approvable: held === undefined }), ...(wanted !== undefined && { changesWanted: wanted }), diff --git a/packages/triage-workflows/src/actions/waiting.ts b/packages/triage-workflows/src/actions/waiting.ts index 9b3213900..a79c8b80b 100644 --- a/packages/triage-workflows/src/actions/waiting.ts +++ b/packages/triage-workflows/src/actions/waiting.ts @@ -1,35 +1,53 @@ import type { EffectContext } from "@intx/workflow"; -import { ACTION_KINDS, isFromTriageSender, marksWaiting, ownedLabels, repoSourceOf, triggerRequestOf, WAITING_KIND, WAITING_LABEL, type RepoSource } from "@corbits/triage-contracts"; +import { ACTION_KINDS, isFromTriageSender, marksWaiting, ownedLabels, repoSourceOf, triggerRequestOf, WAITING_KIND, WAITING_LABEL, type RepoSource, type VerdictWord } from "@corbits/triage-contracts"; import { packFromInput } from "../logic/checks.js"; /** The mirror tool's arguments for the waiting marker: the label on GitHub, the build status on Bitbucket Data Center. */ -type MarkArgs = { repo: string; number: number; onHead: string; labels: string[]; owned: readonly string[]; build?: { state: "INPROGRESS"; description: string } }; +type MarkArgs = { repo: string; number: number; onHead: string; labels: string[]; owned: readonly string[]; keep?: readonly string[]; build?: { state: "INPROGRESS"; description: string } }; /** `effectId` names the marker on its head; the write itself is a no-op where the head already shows it, and skipped once the head moved. */ export type WaitingMark = { effectId: string; repoSource: RepoSource; args: MarkArgs }; /** `mark` is absent when triage does not write the verdict on this host, so nothing is written either. */ -export type WaitingOutput = { waiting: false } | { waiting: true; mark?: WaitingMark }; +export type WaitingOutput = { waiting: boolean; mark?: WaitingMark }; -function markArgs(source: RepoSource, pull: { repo: string; number: number; onHead: string }, owned: readonly string[]): MarkArgs { +type Pull = { repo: string; number: number; onHead: string }; + +/** Ready never stands on a head triage has not checked; Not Ready and Needs Human Review stay until its verdict, so a push changes no label. */ +function markArgs(source: RepoSource, pull: Pull, labels: Record, ci: boolean): MarkArgs { if (source === "bitbucket-datacenter") return { ...pull, labels: [], owned: [], build: { state: "INPROGRESS", description: WAITING_LABEL } }; - return { ...pull, labels: [WAITING_LABEL], owned }; + if (!ci) return { ...pull, labels: [], owned: [labels.pass] }; + return { ...pull, labels: [WAITING_LABEL], owned: ownedLabels(labels), keep: [labels.fail, labels.unsure] }; +} + +function mark(kind: "waiting" | "unready", source: RepoSource, pull: Pull, labels: Record, ci: boolean): WaitingMark { + return { effectId: `${kind}:${pull.repo.replace("/", "--")}:${pull.number}:${pull.onHead}`, repoSource: source, args: markArgs(source, pull, labels, ci) }; +} + +function marks(source: RepoSource, automatic: readonly unknown[]): boolean { + return marksWaiting(source, ACTION_KINDS.filter((kind) => automatic.includes(kind))); } -/** The waiting marker on `headSha`; none where triage does not write the verdict on this host. */ -export function waitingMark(source: RepoSource, { repo, number, headSha }: { repo: string; number: number; headSha: string }, automatic: readonly unknown[], owned: readonly string[]): WaitingMark | undefined { - if (!marksWaiting(source, ACTION_KINDS.filter((kind) => automatic.includes(kind)))) return undefined; - const effectId = `waiting:${repo.replace("/", "--")}:${number}:${headSha}`; - return { effectId, repoSource: source, args: markArgs(source, { repo, number, onHead: headSha }, owned) }; +/** The waiting marker on `headSha`, `Triage: Waiting for CI` only where the head has CI; none where triage does not write the verdict on this host. */ +export function waitingMark(source: RepoSource, { repo, number, headSha }: { repo: string; number: number; headSha: string }, automatic: readonly unknown[], labels: Record, ci: boolean): WaitingMark | undefined { + if (!marks(source, automatic)) return undefined; + return mark("waiting", source, { repo, number, onHead: headSha }, labels, ci); } -/** A waiting mail from the hub's sender marks its head as held for CI and triages nothing; any other mail goes on to triage. */ +/** + * A waiting mail from the hub's sender marks its head as held for CI and triages nothing. A pull request mail from it goes on + * to triage once Ready is off its head on GitHub; Bitbucket's build status is per commit already. Any other mail goes on to triage. + */ export async function waiting(input: unknown, _ctx: EffectContext, _signal: AbortSignal): Promise { const request = triggerRequestOf(input); - if (request?.["kind"] !== WAITING_KIND || !isFromTriageSender(input)) return { waiting: false }; + const held = request?.["kind"] === WAITING_KIND; + if (!request || !isFromTriageSender(input) || (!held && request["kind"] !== "pr")) return { waiting: false }; + const source = repoSourceOf(request["repoSource"]); const { repo, prNumber, headSha, automatic } = request; - const pack = packFromInput(request["pack"]); - if (typeof repo !== "string" || typeof prNumber !== "number" || typeof headSha !== "string" || !Array.isArray(automatic) || !pack) return { waiting: true }; - const mark = waitingMark(repoSourceOf(request["repoSource"]), { repo, number: prNumber, headSha }, automatic, ownedLabels(pack.labels)); - return mark ? { waiting: true, mark } : { waiting: true }; + const pack = packFromInput(request["pack"] ?? request["checkPack"]); + if (typeof repo !== "string" || typeof prNumber !== "number" || typeof headSha !== "string" || !Array.isArray(automatic) || !pack) return { waiting: held }; + if (!marks(source, automatic) || (!held && source !== "github")) return { waiting: held }; + const pull = { repo, number: prNumber, onHead: headSha }; + if (held) return { waiting: true, mark: mark("waiting", source, pull, pack.labels, request["noCi"] !== true) }; + return { waiting: false, mark: mark("unready", source, pull, pack.labels, false) }; } diff --git a/packages/triage-workflows/src/directors.ts b/packages/triage-workflows/src/directors.ts index 5b7d422bf..ef410e900 100644 --- a/packages/triage-workflows/src/directors.ts +++ b/packages/triage-workflows/src/directors.ts @@ -507,7 +507,7 @@ function mirrorDirector(caps: ReactorCapabilities): ReactorDirector { return call(toolName(source, "mirror_auto"), { repo: r.repo, number: r.number, ...labels, ...comment, ...build, ...dismissals(v) }, `mirror:${r.repo.replace("/", "--")}:${r.number}`); } - function markWaiting({ mark }: WaitingOutput & { waiting: true }) { + function markWaiting({ mark }: WaitingOutput) { if (!mark) return caps.reply(JSON.stringify({ skipped: true })); return b.run([markCall(mark)], finish); } @@ -518,7 +518,7 @@ function mirrorDirector(caps: ReactorCapabilities): ReactorDirector { case "message.received": { const input = parseInput(event.message.content); if (!input) return caps.reply(JSON.stringify({ skipped: true, reason: "mirror: input is not JSON" })); - if (input.waiting === true) return markWaiting(input as WaitingOutput & { waiting: true }); + if (typeof input.waiting === "boolean") return markWaiting(input as WaitingOutput); const parsed = verdictsOf(input); batch = parsed.batch; const mirrored = parsed.verdicts.filter(mirrors); diff --git a/packages/triage-workflows/src/pr-triage.test.ts b/packages/triage-workflows/src/pr-triage.test.ts index da6b45b6c..a09111f61 100644 --- a/packages/triage-workflows/src/pr-triage.test.ts +++ b/packages/triage-workflows/src/pr-triage.test.ts @@ -95,10 +95,12 @@ function fakeGithub({ missing = false, files = [FLAG], labels = [], prior, failC if (review) review.state = "DISMISSED"; return Response.json(review); } + if (method === "POST" && path === `/repos/${REPO}/issues/8/labels`) labels.push(...JSON.parse(String(init?.body)).labels); + if (method === "DELETE" && path.startsWith(`/repos/${REPO}/issues/8/labels/`)) labels.splice(labels.indexOf(decodeURIComponent(path.slice(path.lastIndexOf("/") + 1))), 1); if (method === "GET") return Response.json([]); return Response.json({}); } - return { gh: gh satisfies GithubFetch, requests, comments, reviews, pr }; + return { gh: gh satisfies GithubFetch, requests, comments, reviews, pr, labels }; } const allow: WorkflowAuthorizeFn = async () => ({ effect: "allow", matchingGrants: [], resolvedBy: null }); @@ -156,16 +158,16 @@ function stepInvoker(gh: GithubFetch, invoked: string[], judged: SystemOne[], ju }; } -const RULES_PATH = { invoked: ["triage-facts", "triage-mirror", "triage-execute"], mirrors: ["mirrorRules"] }; +const RULES_PATH = { invoked: ["triage-mirror", "triage-facts", "triage-mirror", "triage-execute"], mirrors: ["mirrorRules"] }; // Without judged rules to ask first, the verdict question is the judge's only call. -const VERDICT_PATH = { invoked: ["triage-facts", "triage-judge", "triage-mirror", "triage-execute"], mirrors: ["mirror"] }; +const VERDICT_PATH = { invoked: ["triage-mirror", "triage-facts", "triage-judge", "triage-mirror", "triage-execute"], mirrors: ["mirror"] }; // `judgeText` fails focused, so the judge is called again for its file questions; a failed judged rule decides Fail without the verdict question. -const JUDGE_PATH = { invoked: ["triage-facts", "triage-judge", "triage-judge", "triage-mirror", "triage-execute"], mirrors: ["mirror"] }; +const JUDGE_PATH = { invoked: ["triage-mirror", "triage-facts", "triage-judge", "triage-judge", "triage-mirror", "triage-execute"], mirrors: ["mirror"] }; type TriageOptions = GithubSeed & { backlog?: boolean; judgeFails?: boolean; noWait?: true; automatic?: readonly ActionKind[]; from?: string; github?: ReturnType; source?: PolicySource }; async function triage(checkPack: CheckPack, { backlog = false, judgeFails = false, noWait, automatic = ACTION_KINDS, from = "github@acme.test", github, source, ...seed }: TriageOptions = {}) { - const { gh, requests } = github ?? fakeGithub(seed); + const { gh, requests, pr } = github ?? fakeGithub(seed); const policy = { enabled: true, ...(source && { source }) }; const invoked: string[] = []; const judged: SystemOne[] = []; @@ -176,7 +178,7 @@ async function triage(checkPack: CheckPack, { backlog = false, judgeFails = fals hasUpstreamSignalResolver: true, invokeStep: stepInvoker(gh, invoked, judged, judgeFails), actionResolver: (ref) => (actions as Record)[ref]!, - triggerPayload: hubMail(backlog ? { kind: "backlog", repo: REPO, policy, checkPack, automatic } : { kind: "pr", repo: REPO, prNumber: 8, policy, checkPack, automatic, events: ["opened"], ...(noWait && { noWait }) }, from), + triggerPayload: hubMail(backlog ? { kind: "backlog", repo: REPO, policy, checkPack, automatic } : { kind: "pr", repo: REPO, prNumber: 8, headSha: pr.head, policy, checkPack, automatic, events: ["opened"], ...(noWait && { noWait }) }, from), }).complete; expect(terminalStatus).toBe("completed"); const mirrors = Object.keys(outputs).filter((id) => id.startsWith("mirror")); @@ -429,15 +431,13 @@ describe("pr-triage body", () => { expect((await approve(github, mail)).dos).toEqual([{ effectId: held.effectId, status: "skipped", error: expect.stringContaining("has new commits") }]); }); - test("a waiting mail labels the head Waiting for CI in place of its verdict label without triaging it; the verdict takes it back", async () => { + test("a waiting mail labels an unlabelled head Waiting for CI without triaging it; the verdict takes it back", async () => { const pack: CheckPack = { ...emptyPack(REPO), checks: { "ci-green": { enabled: true } } }; const waiting = { kind: "waiting", repo: REPO, prNumber: 8, headSha: "abc", pack, automatic: ["labels"] }; - const held = fakeGithub({ labels: ["Triage: Ready"] }); + const held = fakeGithub(); expect(await approve(held, hubMail(waiting, "github@acme.test"))).toMatchObject({ terminalStatus: "completed", invoked: ["triage-mirror"] }); - expect(held.requests.filter((r) => r.method !== "GET")).toEqual([ - { method: "POST", path: `/repos/${REPO}/issues/8/labels` }, - { method: "DELETE", path: `/repos/${REPO}/issues/8/labels/Triage%3A%20Ready` }, - ]); + expect(held.requests.filter((r) => r.method !== "GET")).toEqual([{ method: "POST", path: `/repos/${REPO}/issues/8/labels` }]); + expect(held.labels).toEqual([WAITING_LABEL]); const ungranted = fakeGithub(); expect(await approve(ungranted, hubMail({ ...waiting, automatic: ["comment"] }, "github@acme.test"))).toMatchObject({ terminalStatus: "completed", invoked: ["triage-mirror"] }); expect(ungranted.requests).toEqual([]); @@ -448,6 +448,61 @@ describe("pr-triage body", () => { expect(requests).toContainEqual({ method: "DELETE", path: `/repos/${REPO}/issues/8/labels/Triage%3A%20Waiting%20for%20CI` }); }); + test("a new head swaps Ready for Waiting for CI until it passes", async () => { + const pack: CheckPack = { ...emptyPack(REPO), checks: { "ci-green": { enabled: true } } }; + const github = fakeGithub({ labels: ["bug", "Triage: Ready"] }); + github.pr.head = "def"; + github.pr.ci = "pending"; + await approve(github, hubMail({ kind: "waiting", repo: REPO, prNumber: 8, headSha: "def", pack, automatic: ["labels"] }, "github@acme.test")); + expect(github.labels).toEqual(["bug", WAITING_LABEL]); + github.pr.ci = "success"; + await triage(pack, { github }); + expect(github.labels).toEqual(["bug", "Triage: Ready"]); + }); + + test("a new head with no CI takes Ready off as soon as it is seen and puts no Triage label on until it passes", async () => { + const pack: CheckPack = { ...emptyPack(REPO), checks: { "ci-green": { enabled: true } } }; + const held = fakeGithub({ labels: ["bug", "Triage: Ready"] }); + held.pr.head = "def"; + await approve(held, hubMail({ kind: "waiting", repo: REPO, prNumber: 8, headSha: "def", noCi: true, pack, automatic: ["labels"] }, "github@acme.test")); + expect(held.labels).toEqual(["bug"]); + await triage(pack, { github: held }); + expect(held.labels).toEqual(["bug", "Triage: Ready"]); + const triaged = fakeGithub({ labels: ["bug", "Triage: Ready"] }); + triaged.pr.head = "def"; + await triage(pack, { github: triaged }); + expect(triaged.requests.filter((r) => r.method !== "GET" && r.path.includes("/labels"))).toEqual([ + { method: "DELETE", path: `/repos/${REPO}/issues/8/labels/Triage%3A%20Ready` }, + { method: "POST", path: `/repos/${REPO}/issues/8/labels` }, + ]); + expect(triaged.labels).toEqual(["bug", "Triage: Ready"]); + }); + + test("pushes that keep Not Ready write no label; a changed verdict swaps it once", async () => { + const pack: CheckPack = { ...emptyPack(REPO), checks: { "ci-green": { enabled: true } } }; + const github = fakeGithub({ labels: ["bug", "Triage: Not Ready"] }); + function labelWrites() { + return github.requests.filter((r) => r.method !== "GET" && r.path.includes("/labels")); + } + for (const head of ["def", "ghi", "jkl"]) { + github.pr.head = head; + github.pr.ci = "pending"; + await approve(github, hubMail({ kind: "waiting", repo: REPO, prNumber: 8, headSha: head, pack, automatic: ["labels"] }, "github@acme.test")); + await triage(pack, { github }); + github.pr.ci = "failure"; + await triage(pack, { github }); + } + expect(labelWrites()).toEqual([]); + github.pr.head = "mno"; + github.pr.ci = "success"; + await triage(pack, { github }); + expect(labelWrites()).toEqual([ + { method: "POST", path: `/repos/${REPO}/issues/8/labels` }, + { method: "DELETE", path: `/repos/${REPO}/issues/8/labels/Triage%3A%20Not%20Ready` }, + ]); + expect(github.labels).toEqual(["bug", "Triage: Ready"]); + }); + test("CI still running holds the head under the waiting label and posts nothing; once CI completes the head passes", async () => { const github = fakeGithub(); github.pr.ci = "pending"; diff --git a/packages/triage-workflows/src/triage-steps.ts b/packages/triage-workflows/src/triage-steps.ts index 471ab227c..72413304b 100644 --- a/packages/triage-workflows/src/triage-steps.ts +++ b/packages/triage-workflows/src/triage-steps.ts @@ -24,7 +24,7 @@ const RULED = { project: outputOf("rules"), fields: ["items", "batch"] } as cons // as directors, reaching GitHub through the tool's credential, until upstream exposes credentials to actions. The judge maps over the // rules output's chunks, one System One call each; a second map asks focused's file questions only for the items it failed, // and makes no call otherwise. An approval mail skips triage and executes the one held action it names; a waiting mail -// skips triage and mirrors only the waiting marker. The mirror writes the verdict's comment and label (or build status) before execute, +// skips triage and mirrors only the waiting marker; any other takes Ready off its head before triaging it. The mirror writes the verdict's comment and label (or build status) before execute, // so a merge or close finds them on the head it closes; a mirror on a closed pull request would write nothing. Each gate // branch evaluates, mirrors and executes on its own: a live run never hands a skipped step's output to a selector, so a join // after the gate could not read the judge when it was skipped. @@ -34,9 +34,10 @@ export function triageSteps({ facts: factsAgent, judge: judgeAgent, execute: exe approved: gate({ when: { from: "steps.approval.output.approved" }, then: "executeApproved", else: "waiting", after: ["approval"] }), executeApproved: step({ agent: executeAgent, input: outputOf("approval"), after: ["approved"], ...STEP }), waiting: action({ handler: "waiting", input: { from: "trigger.payload" }, after: ["approved"] }), - held: gate({ when: { from: "steps.waiting.output.waiting" }, then: "markWaiting", else: "facts", after: ["waiting"] }), + held: gate({ when: { from: "steps.waiting.output.waiting" }, then: "markWaiting", else: "unready", after: ["waiting"] }), markWaiting: step({ agent: mirrorAgent, input: outputOf("waiting"), after: ["held"], ...STEP }), - facts: step({ agent: factsAgent, input: { from: "trigger.payload" }, after: ["held"], ...STEP }), + unready: step({ agent: mirrorAgent, input: outputOf("waiting"), after: ["held"], ...STEP }), + facts: step({ agent: factsAgent, input: { from: "trigger.payload" }, after: ["unready"], ...STEP }), rules: action({ handler: "rules", input: replyOf("facts"), after: ["facts"] }), route: gate({ when: { from: "steps.rules.output.needsJudgment" }, then: "judge", else: "evaluateRules", after: ["rules"] }), judge: map({ over: { from: "steps.rules.output.chunks" }, step: step({ agent: judgeAgent, input: { from: "trigger.payload" }, ...STEP }), after: ["route"] }),