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 0f22989a..0786ab42 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -387,7 +387,7 @@ When Triage may not post its verdict, the pull request's line says so, such as * Which actions Triage takes on its own is set once for the whole workspace, under [Settings → Triage](#actions-triage-takes-on-its-own). In a new workspace Triage takes every action the policy file names, a few minutes after triage is turned on, and an action taken out of the file goes back to waiting for you. A workspace set to ask first posts only the verdict on its own, Comment and Add labels (Comment only on Bitbucket, which has no labels), so you can watch what Triage suggests before you let it act; **Always … for this repository** on a held action lets Triage take it on its own there from then on (see [Taking action](#taking-action)). Turning triage off, or removing the repository, takes back everything Triage did there on its own; turning it on again starts from Comment and Add labels. Two actions are always held for a person: Close on Unsure, and Merge on anything but Pass. -Triage checks a pull request as soon as it changes. When a rule is waiting on the host (CI still running, or the host has not yet worked out whether the pull request can merge), whatever else it could not confirm, or CI is still running and a step reads it (a CI condition, approve or merge), it gives no verdict yet and waits for the host, so you get one result instead of an early Unsure: no action runs, and Triage shows **In progress: Waiting on CI**, and the pull request waits under **Waiting** rather than **Needs you**. 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 the new commit's **Corbits Triage** build status shows **Triage: Waiting for CI**. The verdict replaces it once CI finishes, as soon as any check fails, or after an hour at most; on a host that does not tell Triage when a build finishes, such as Bitbucket, within about five minutes of it finishing. When the **CI must pass** rule is on, a commit still with no checks a few minutes after Triage first saw it is taken as having no CI. **Triage again**, or the host still not having worked out mergeability after Triage's last retry, gives the verdict at once. Where Triage comments on its own, its comment on the pull request reads **Triage: In progress** while it waits, and names what it is waiting for: CI, the decision model's answer or code review. Where Triage adds labels on its own, the waiting labels name the same: **Triage: Waiting for CI**, **Triage: Waiting for the decision model** or **Triage: Waiting for code review**. Triage keeps that one comment current, and the verdict replaces it in place once it decides. On a commit that already has a verdict the same rules still give, the comment keeps that verdict while Triage checks again, including while the decision model is asked again, until the new answer replaces it; CI running again on the commit, or **Review again**, shows **Triage: In progress** instead. If the decision model or code review keep Triage waiting three hours after it started on the pull request, it gives the verdict anyway, says what it stopped waiting for, and runs no action on its own, so you decide. Triage stops waiting for a decision model that has not answered 15 minutes after Triage asked it, and for a code review still running after 30 minutes. A code review that fails or whose run ends is not waited for: the verdict goes up at once without it, and approve and merge, and any step with a findings condition, wait for you with **Code review did not report on this head.** If Triage gives the verdict while CI is still running, on **Triage again** or its last retry, it holds every action that depends on CI, and its comment on the pull request lists them under **Waiting for checks to finish**. When Triage waited, for CI, the decision model or code review, the actions of the event that started the wait, such as an opened pull request, run once the verdict is made, as if it had not waited; the actions under **Waiting for checks to finish** are decided again once CI finishes. A commit pushed during the wait starts over: the earlier commit's events do not carry to it, so their actions run only if `on:` lists the new commit's event. +Triage checks a pull request as soon as it changes. When a rule is waiting on the host (CI still running, or the host has not yet worked out whether the pull request can merge), whatever else it could not confirm, or CI is still running and a step reads it (a CI condition, approve or merge), it gives no verdict yet and waits for the host, so you get one result instead of an early Unsure: no action runs, and Triage shows **In progress: Waiting on CI**, and the pull request waits under **Waiting** rather than **Needs you**. The pull request's page notes **New commits since Triage checked** above the earlier result, which stays until the new one lands. On GitHub a new commit takes the verdict label off once, and the new verdict puts its label on; other events on the same commit change no label. On Bitbucket the new commit's **Corbits Triage** build status shows **Triage: Waiting for CI**. The verdict replaces it once CI finishes, as soon as any check fails, or after an hour at most; on a host that does not tell Triage when a build finishes, such as Bitbucket, within about five minutes of it finishing. When the **CI must pass** rule is on, a commit still with no checks a few minutes after Triage first saw it is taken as having no CI. **Triage again**, or the host still not having worked out mergeability after Triage's last retry, gives the verdict at once. Where Triage comments on its own, its comment on the pull request reads **Triage: In progress** while it waits, and names what it is waiting for: CI, the decision model's answer or code review. Triage keeps that one comment current, and the verdict replaces it in place once it decides. On a commit that already has a verdict the same rules still give, the comment keeps that verdict while Triage checks again, including while the decision model is asked again, until the new answer replaces it; CI running again on the commit, or **Review again**, shows **Triage: In progress** instead. If the decision model or code review keep Triage waiting three hours after it started on the pull request, it gives the verdict anyway, says what it stopped waiting for, and runs no action on its own, so you decide. Triage stops waiting for a decision model that has not answered 15 minutes after Triage asked it, and for a code review still running after 30 minutes. A code review that fails or whose run ends is not waited for: the verdict goes up at once without it, and approve and merge, and any step with a findings condition, wait for you with **Code review did not report on this head.** If Triage gives the verdict while CI is still running, on **Triage again** or its last retry, it holds every action that depends on CI, and its comment on the pull request lists them under **Waiting for checks to finish**. When Triage waited, for CI, the decision model or code review, the actions of the event that started the wait, such as an opened pull request, run once the verdict is made, as if it had not waited; the actions under **Waiting for checks to finish** are decided again once CI finishes. A commit pushed during the wait starts over: the earlier commit's events do not carry to it, so their actions run only if `on:` lists the new commit's event. An **Unsure** comment lists each rule Triage could not confirm, with checks still running or cancelled under **Waiting on**. @@ -400,7 +400,7 @@ Triage never merges a pull request while a person's review thread on it is open. ### Labels -Triage keeps a verdict label on every pull request so that other tools, such as a coding agent, can react to it. It puts the verdict's label on the pull request and takes the other two off. The defaults are **Triage: Ready** for Pass, **Triage: Not Ready** for Fail and **Triage: Needs Human Review** for Unsure; the file can rename them. While Triage waits it shows what it waits for, the same as its comment: **Triage: Waiting for CI**, **Triage: Waiting for the decision model** or **Triage: Waiting for code review**, one for each, and each comes off once Triage stops waiting for it. When it could not read the pull request it shows **Triage: Failed to Run** (see [What the author sees](#what-the-author-sees)). A step that labels each failed rule, shown as **Label each failed rule**, adds a label such as `rule: linked-issue` for every rule that failed. The label of a rule set to `onFail: label`, and the `sensitive: medium` and `risk: medium` labels, is added whatever the verdict, after the verdict's own steps, as an **Add labels** action: on its own where Triage adds labels on its own, otherwise waiting for you on Triage. The same action takes off the label of a rule that now passes, and a level label of sensitive paths or risk that no longer applies. A label a `labels` step in the policy also names is left alone. +Triage keeps a verdict label on every pull request so that other tools, such as a coding agent, can react to it. It puts the verdict's label on the pull request and takes the other two off. The defaults are **Triage: Ready** for Pass, **Triage: Not Ready** for Fail and **Triage: Needs Human Review** for Unsure; the file can rename them. Triage writes the label once when it decides and changes it only when the verdict changes; a new commit takes it off until that commit's verdict, and while Triage waits only its comment shows progress. When it could not read the pull request it shows **Triage: Failed to Run** (see [What the author sees](#what-the-author-sees)). A step that labels each failed rule, shown as **Label each failed rule**, adds a label such as `rule: linked-issue` for every rule that failed. The label of a rule set to `onFail: label`, and the `sensitive: medium` and `risk: medium` labels, is added whatever the verdict, after the verdict's own steps, as an **Add labels** action: on its own where Triage adds labels on its own, otherwise waiting for you on Triage. The same action takes off the label of a rule that now passes, and a level label of sensitive paths or risk that no longer applies. A label a `labels` step in the policy also names is left alone. On Bitbucket Data Center labels have no effect; see [Bitbucket Data Center repositories](#bitbucket-data-center-repositories). @@ -547,7 +547,7 @@ The bar at the bottom of the pane always offers the next step first. For a passi ### 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. **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, they see what it waits for in its place, such as **Triage: Waiting for CI** or **Triage: Waiting for code review**, with no CI label when the commit has no checks; **Triage: Not Ready** and **Triage: Needs Human Review** stay until the new verdict. When Triage could not read the pull request, or the repository's policy, it shows **Triage: Failed to Run** in place of any verdict label, or on Bitbucket a failed **Corbits Triage** build status that says **Triage couldn't run**, and posts nothing else. On GitHub it does this only where Triage adds labels on its own; on Bitbucket it always does. It clears on the next run that can read the pull request, which shows the verdict again. 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, such as **Split out unrelated change: `src/b.ts`, not part of `feat: add search`**. 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. A new commit takes the verdict label off once, so it never stands on a commit Triage has not checked, and the new verdict puts its label on; while Triage waits, its comment says what it waits for. When Triage could not read the pull request, or the repository's policy, it shows **Triage: Failed to Run** in place of any verdict label, or on Bitbucket a failed **Corbits Triage** build status that says **Triage couldn't run**, and posts nothing else. On GitHub it does this only where Triage adds labels on its own; on Bitbucket it always does. It clears on the next run that can read the pull request, which shows the verdict again. 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, such as **Split out unrelated change: `src/b.ts`, not part of `feat: add search`**. 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. ## Settings: Triage, code review and people diff --git a/docs/INVARIANTS.md b/docs/INVARIANTS.md index f69bc84c..038f819d 100644 --- a/docs/INVARIANTS.md +++ b/docs/INVARIANTS.md @@ -5,13 +5,13 @@ request that changes how Triage keeps a rule names the rule. ## Rules -- **R1** One final verdict per pull request head, for the current head. A pull request that merges or closes before its verdict never stays in progress. Checked by `tests/model/rules.postgres.test.ts`: once the pull request settles it has exactly one current Triage comment, which is a verdict naming the head, and no waiting labels. One that merged or closed keeps a verdict it had on its last head, or else says it merged or closed before Triage finished and has none of Triage's labels, or has no Triage comment when it closed before any went up. +- **R1** One final verdict per pull request head, for the current head. A pull request that merges or closes before its verdict never stays in progress. Checked by `tests/model/rules.postgres.test.ts`: once the pull request settles it has exactly one current Triage comment, which is a verdict naming the head. One that merged or closed keeps a verdict it had on its last head, or else says it merged or closed before Triage finished and has none of Triage's labels, or has no Triage comment when it closed before any went up. - **R2** Triage approves or merges only on Pass with a complete code review that meets the policy's findings condition, all for the current head, and merges only while no person's review thread is unresolved or unread. No automated check yet; the merge write refuses while a thread is. -- **R3** Every host write happens once under its effect id. Checked by `tests/model/rules.postgres.test.ts` over every write the pull request received, whichever pass made it: no Triage comment is created while one is current, unless that one was superseded, and no label is added while it is on. +- **R3** Every host write happens once under its effect id. Checked by `tests/model/rules.postgres.test.ts` over every write the pull request received, whichever pass made it: no Triage comment is created while one is current, unless that one was superseded, and no label is added while it is on, no Triage label but a verdict's or Failed to Run is added, a new head holds no verdict label before its own verdict, and once a head has a verdict its labels change only with the verdict. - **R4** Unreadable input never passes or approves. No automated check yet. - **R5** Nothing is left running after the final post. Checked by `tests/model/rules.postgres.test.ts`: once the pull request settles, none of its managers is live, no live manager was deployed for no single pull request, and no mail for it was lost to a run that had ended. Settling delivers every queued mail, so no mail is left waiting by construction. - **R6** An older run never overwrites a newer head's comment, label, status or approval. No automated check yet. -- **R7** A pull request waiting on CI or a reviewer always gets its verdict. Checked by `tests/model/rules.postgres.test.ts`: R1's check covers a head waiting on CI, and with code review on, no manager waits on a code review run that has ended, before it was ready or after it took its mail. In every stack, no manager runs out of passes or takes its pull request more than four times on one head. Over every write it received, whenever a Triage comment is written and once it settles, the waiting labels on the pull request name what its in-progress comment waits for, and none stand beside a verdict, unless it is labelled Not Ready or Needs Human Review. +- **R7** A pull request waiting on CI or a reviewer always gets its verdict. Checked by `tests/model/rules.postgres.test.ts`: R1's check covers a head waiting on CI, and with code review on, no manager waits on a code review run that has ended, before it was ready or after it took its mail. In every stack, no manager runs out of passes or takes its pull request more than four times on one head. - **R8** GitHub and Bitbucket Data Center behave the same except where documented. No automated check yet. - **R9** Every reader of the policy agrees on what it says. No automated check yet. - **R10** The portal shows only what the hub says. No automated check yet. diff --git a/packages/github-tool/src/github.ts b/packages/github-tool/src/github.ts index a03cde74..f77782d4 100644 --- a/packages/github-tool/src/github.ts +++ b/packages/github-tool/src/github.ts @@ -17,8 +17,6 @@ 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, or the comment kept its verdict, 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; @@ -61,8 +59,7 @@ export async function mirror(gh: GithubFetch, i: MirrorInput, login: string) { if (i.comment === "") await deleteTriageComments(gh, repo, number, login); 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, ...(i.keepVerdict && { keepVerdictOn: { head: i.onHead!, ...i.keepVerdict } }) }, login) : null; const current: string[] = pr.labels.map((label: { name: string }) => label.name); - const kept = new Set((i.keep ?? []).map(lowercase)); - if (posted?.kept !== "verdict" && !current.some((name) => kept.has(lowercase(name)))) await syncLabels(gh, { repo, number, current, add: i.labels, remove: i.owned ?? [] }); + if (posted?.kept !== "verdict") 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 }), ...(posted?.kept && { kept: posted.kept }) }; } diff --git a/packages/github-tool/src/sidecar-bundle.ts b/packages/github-tool/src/sidecar-bundle.ts index d1ca13f7..7af886a8 100644 --- a/packages/github-tool/src/sidecar-bundle.ts +++ b/packages/github-tool/src/sidecar-bundle.ts @@ -164,7 +164,6 @@ const MIRROR_FIELDS = { 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({ @@ -344,7 +343,6 @@ async function dispatch(gh: GithubFetch, call: ToolCallInput, reads: Reads): Pro 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-contracts/src/check-pack.ts b/packages/triage-contracts/src/check-pack.ts index 884f15cd..87c1c12b 100644 --- a/packages/triage-contracts/src/check-pack.ts +++ b/packages/triage-contracts/src/check-pack.ts @@ -408,9 +408,6 @@ export const VERDICT_LABELS: Record = { pass: "Triage: Read /** What a head's verdict still waits for. */ export type Awaits = "ci" | "judge" | "code-review"; -/** On a head while its verdict waits for each, until the verdict replaces it. */ -export const WAITING_LABELS: Record = { ci: "Triage: Waiting for CI", judge: "Triage: Waiting for the decision model", "code-review": "Triage: Waiting for code review" }; - /** On a pull request Triage could not read, until its next verdict replaces it. */ export const FAILED_LABEL = "Triage: Failed to Run"; diff --git a/packages/triage-contracts/src/presets.ts b/packages/triage-contracts/src/presets.ts index 0a298a7e..1b96da7b 100644 --- a/packages/triage-contracts/src/presets.ts +++ b/packages/triage-contracts/src/presets.ts @@ -1,4 +1,4 @@ -import { FAILED_LABEL, VERDICT_LABELS, WAITING_LABELS, type VerdictWord } from "./check-pack.js"; +import { FAILED_LABEL, VERDICT_LABELS, type VerdictWord } from "./check-pack.js"; import type { Priority, WorkflowState } from "./types.js"; const OWNERS: Record = { @@ -21,10 +21,13 @@ const RETIRED_LABELS = [ "triage:blocked", "triage:ready", "triage:stale-unknown", + "Triage: Waiting for CI", + "Triage: Waiting for the decision model", + "Triage: Waiting for code review", ]; /** Includes every label triage once set. */ -export const TRIAGE_LABELS: readonly string[] = [...Object.values(VERDICT_LABELS), ...Object.values(WAITING_LABELS), FAILED_LABEL, ...RETIRED_LABELS]; +export const TRIAGE_LABELS: readonly string[] = [...Object.values(VERDICT_LABELS), FAILED_LABEL, ...RETIRED_LABELS]; /** The mirror takes back these when they no longer apply. */ export function ownedLabels(labels: Record): string[] { diff --git a/packages/triage-contracts/src/verdict-status.ts b/packages/triage-contracts/src/verdict-status.ts index 5e5364e8..57c2ffcc 100644 --- a/packages/triage-contracts/src/verdict-status.ts +++ b/packages/triage-contracts/src/verdict-status.ts @@ -1,4 +1,4 @@ -import { ruleLabel, WAITING_LABELS, type VerdictWord } from "./check-pack.js"; +import { ruleLabel, type VerdictWord } from "./check-pack.js"; /** The verdict as a status on the head, which a host shows beside it and can require before merge; each host maps the state to its own. */ type StatusState = "success" | "failure" | "pending" | "error"; @@ -14,7 +14,7 @@ export function verdictStatus(verdict: VerdictWord, failedRules: readonly string } /** On a head held for its CI until the verdict replaces it. */ -export const WAITING_STATUS: VerdictStatus = { state: "pending", description: WAITING_LABELS.ci }; +export const WAITING_STATUS: VerdictStatus = { state: "pending", description: "Triage: Waiting for CI" }; /** On a head Triage could not read. */ export const UNREAD_STATUS: VerdictStatus = { state: "error", description: "Triage couldn't run" }; diff --git a/packages/triage-manager/src/actions/manager.ts b/packages/triage-manager/src/actions/manager.ts index 3e293d0e..ad0c7040 100644 --- a/packages/triage-manager/src/actions/manager.ts +++ b/packages/triage-manager/src/actions/manager.ts @@ -190,7 +190,7 @@ export async function take(input: unknown, ctx: EffectContext, _signal: AbortSig // A pass of approvals alone, before any pull request mail was recorded, has nothing to triage. const triage = request !== undefined && (forHead !== undefined || report.review !== undefined || report.reviewFailed !== undefined || report.judge !== undefined || report.judgeFailed !== undefined || overdue || waitedOut); const starting = headSince === undefined || Date.now() - Date.parse(headSince) < CI_START_MS; - const taken = request && { ...request, ...(waitedOut && { noWait: true }), ...(!waitedOut && starting && { ciStarting: true }) }; + const taken = request && { ...request, ...(waitedOut && { noWait: true }), ...(!waitedOut && starting && { ciStarting: true }), ...(headSince === undefined && { newHead: true }) }; const verdict = report.judge ?? storedVerdict(reviewers, head); const waiting = waitingSince !== undefined && !waitedOut; return { run: answer.outcome === "pass" || overdue || waitedOut, approvals, triage, ...(taken && { taken }), head, ...report, ...(verdict && { verdict }), overdue, reviewers, ...(waiting && { waitingSince }), ...(stopped && { stopped: outstanding }), ...(threads.length > 0 && { threads }), ...(commentThreads !== undefined && commentThreads.length > 0 && { commentThreads }) }; diff --git a/packages/triage-manager/src/actions/progress.ts b/packages/triage-manager/src/actions/progress.ts index f7fe5101..a082f706 100644 --- a/packages/triage-manager/src/actions/progress.ts +++ b/packages/triage-manager/src/actions/progress.ts @@ -1,11 +1,10 @@ import type { EffectContext } from "@intx/workflow"; import { type } from "arktype"; import { asRecord } from "@corbits/common"; -import { repoSourceOf, VERDICT_LABELS, type Awaits, type KeptVerdict } from "@corbits/triage-contracts"; +import { repoSourceOf, type Awaits, type KeptVerdict } from "@corbits/triage-contracts"; import { withoutDos } from "@corbits/triage-workflows/actions/evaluate"; import { waitingMark } from "@corbits/triage-workflows/actions/waiting"; import { writeMark, type MirrorOutcome } from "@corbits/triage-workflows/actions/mirror"; -import type { Item } from "@corbits/triage-workflows/logic/item"; import { CODE_REVIEW_RUNNING, type Verdict } from "@corbits/triage-workflows/logic/render"; import { managerMailOf } from "./hub.js"; import { verdict } from "./judge.js"; @@ -64,9 +63,8 @@ export async function progress(input: unknown, ctx: EffectContext, signal: Abort const awaits: Awaits[] = [...(waitingOnHost(parsed.rules.output.items) ? ["ci" as const] : []), ...reviewing]; if (awaits.length === 0) return { skipped: true }; const { pullRequest } = managerMailOf(parsed.payload); - const item = parsed.rules.output.items[0] as Item; - const facts = { repo: pullRequest.repo, number: pullRequest.number, headSha: head, checks: item.facts.checks, events: item.facts.events }; + const facts = { repo: pullRequest.repo, number: pullRequest.number, headSha: head }; const keepVerdict = await keptBy(input, parsed, awaits, asRecord(granted.codeReview)?.["again"], ctx, signal); - const mark = waitingMark(repoSourceOf(pullRequest.source), facts, awaits, granted.automatic, item.pack?.labels ?? VERDICT_LABELS, granted.appLogin, keepVerdict, knownOf(parsed, awaits)); + const mark = waitingMark(repoSourceOf(pullRequest.source), facts, awaits, granted.automatic, granted.appLogin, keepVerdict, knownOf(parsed, awaits)); return mark ? writeMark(mark, ctx) : { skipped: true }; } diff --git a/packages/triage-manager/src/triage-manager.test.ts b/packages/triage-manager/src/triage-manager.test.ts index ce5bbb9e..b62f2dcb 100644 --- a/packages/triage-manager/src/triage-manager.test.ts +++ b/packages/triage-manager/src/triage-manager.test.ts @@ -4,7 +4,7 @@ import { createActionCredentialsFactory, createCredentialProviderRegistry, creat import type { GrantRule } from "@intx/types/authz"; import { HUB_CREDENTIAL_HANDLE } from "@intx/types/credential-delivery"; import { runLocal, type ActionHandler, type LoopFn, type OnTriggerPrimitive, type WorkflowAuthorizeFn, type WorkflowDefinition } from "@intx/workflow"; -import { ACTION_KINDS, APPROVE_DO_KIND, emptyPack, JUDGE_RULES_ATTACHMENT, VERDICT_LABELS, WAITING_LABELS, type ActionKind, type CheckPack } from "@corbits/triage-contracts"; +import { ACTION_KINDS, APPROVE_DO_KIND, emptyPack, JUDGE_RULES_ATTACHMENT, VERDICT_LABELS, type ActionKind, type CheckPack } from "@corbits/triage-contracts"; import type { Snapshot } from "@corbits/triage-workflows/evals/cases"; import { fixtureGithub, triggerMail } from "@corbits/triage-workflows/evals/serve"; import { evaluate } from "@corbits/triage-workflows/actions/evaluate"; @@ -226,13 +226,13 @@ async function heldApproval() { } describe("triage-manager", () => { - test("a head whose CI still runs, with no reviewer out, gets the Waiting label and the in-progress comment, and its manager ends", async () => { + test("a head whose CI still runs, with no reviewer out, gets the in-progress comment and no label, and its manager ends", async () => { const { writes, host } = githubHost(HEAD, { checkRuns: [{ name: "build", status: "in_progress", conclusion: null }] }); const hub = fakeHub({ takes: [PASS], dones: [ENDED] }); const { passes } = await runMail(github, wake(), host, hub.hub); expect(passes).toBe(1); expect(doneBody(hub.calls)).toMatchObject({ wait: false, report: { head: HEAD, verdict: expect.objectContaining({ waiting: true }) } }); - expect(writes).toContainEqual(expect.objectContaining({ method: "POST", path: `/repos/${PR.repo}/issues/${PR.number}/labels`, body: { labels: [WAITING_LABELS.ci] } })); + expect(writes.filter((write) => write.path.includes("/labels"))).toEqual([]); expect(writes.filter((write) => write.path.endsWith("/comments"))).toEqual([inProgress("CI to finish")]); }); @@ -272,12 +272,11 @@ describe("triage-manager", () => { }); test("a new head with no checks yet waits for its CI to start and its manager ends; one woken past the start grace decides it as having none", async () => { - const { writes, host } = githubHost(); + const { host } = githubHost(); const fresh = { ...PASS, headSince: undefined }; const first = fakeHub({ takes: [fresh], dones: [ENDED] }); await runMail(github, wake(), host, first.hub); expect(doneBody(first.calls)).toMatchObject({ wait: false, report: { head: HEAD, verdict: expect.objectContaining({ waiting: true, reason: expect.stringContaining("checks have not started yet") }) } }); - expect(writes).toContainEqual(expect.objectContaining({ method: "POST", path: `/repos/${PR.repo}/issues/${PR.number}/labels`, body: { labels: [WAITING_LABELS.ci] } })); const since = new Date(Date.now() - 2 * 60_000 - 10_000).toISOString(); const later = fakeHub({ takes: [{ ...PASS, waitingSince: since, headSince: since }], dones: [ENDED] }); await runMail(github, wake(), host, later.hub); diff --git a/packages/triage-workflows/src/actions/evaluate.ts b/packages/triage-workflows/src/actions/evaluate.ts index daf0c0e3..017caff8 100644 --- a/packages/triage-workflows/src/actions/evaluate.ts +++ b/packages/triage-workflows/src/actions/evaluate.ts @@ -210,7 +210,7 @@ function verdictFrom(it: Item, rendered: RenderOutput): Verdict { const source = repoSourceOf(it.repoSource); const labelNames = it.pack?.labels ?? VERDICT_LABELS; const closed = it.facts.state === "closed"; - const mark = closed ? closedMark(source, it.facts, automatic, labelNames, it.appLogin) : rendered.waiting && waitingMark(source, it.facts, ["ci"], automatic, labelNames, it.appLogin); + const mark = closed ? closedMark(source, it.facts, automatic, labelNames, it.appLogin) : rendered.waiting && waitingMark(source, it.facts, ["ci"], automatic, it.appLogin); const verdict = { ...stamp(it, it.facts.headSha), ...rendered, labels, reasons, feedback: shown, automatic, actions, ...(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 e7fe7c1c..2e8c1bad 100644 --- a/packages/triage-workflows/src/actions/waiting.ts +++ b/packages/triage-workflows/src/actions/waiting.ts @@ -1,83 +1,50 @@ import type { EffectContext } from "@intx/workflow"; -import { ACTION_KINDS, marksWaiting, ownedLabels, parseCheckPack, repoSourceOf, trustedRequestOf, WAITING_LABELS, WAITING_STATUS, type Awaits, type KeptVerdict, type RepoSource, type VerdictWord } from "@corbits/triage-contracts"; -import type { PrFacts } from "../logic/checks.js"; -import { progressComment, type MarkArgs, type WaitingMark } from "../logic/render.js"; +import { ownedLabels, parseCheckPack, repoSourceOf, trustedRequestOf, WAITING_STATUS, type Awaits, type KeptVerdict, type RepoSource, type VerdictWord } from "@corbits/triage-contracts"; +import { progressComment, type WaitingMark } from "../logic/render.js"; -/** `mark` is absent when there is nothing to take off, so nothing is written either. */ -export type UnreadyOutput = { unready: true; mark?: WaitingMark }; - -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, waiting: readonly string[]): MarkArgs { - if (source === "bitbucket-datacenter") return { ...pull, labels: [], owned: [], status: WAITING_STATUS, statusHead: pull.onHead }; - if (waiting.length === 0) return { ...pull, labels: [], owned: [labels.pass] }; - return { ...pull, labels: [...waiting], owned: ownedLabels(labels), keep: [labels.fail, labels.unsure] }; -} - -function marks(source: RepoSource, automatic: readonly unknown[]): boolean { - return marksWaiting(source, ACTION_KINDS.filter((kind) => automatic.includes(kind))); -} +type Head = { repo: string; number: number; headSha: string }; -/** Every triage event on the request is a person's approval, so labels stay as they are. */ -function approvalOnly(events: unknown): boolean { - return Array.isArray(events) && events.length > 0 && events.every((event) => event === "approved"); -} - -/** - * Whether the waiting marker writes labels, or Bitbucket Data Center's status, for `awaits`: where triage marks on this - * host, except on an approval-only GitHub pass. Bitbucket's status names only CI. - */ -function marked(source: RepoSource, awaits: readonly Awaits[], automatic: readonly unknown[], events: readonly unknown[]): boolean { - if (!marks(source, automatic)) return false; - return source === "github" ? !approvalOnly(events) : awaits.includes("ci"); -} +/** `mark` is absent where triage does not label on GitHub, so nothing is written. */ +export type UnreadyOutput = { unready: true; mark?: WaitingMark }; /** - * The waiting marker on `headSha` while its verdict waits for `awaits`: the label of each, CI's only where the head has - * checks, and the in-progress comment naming them where triage comments; none where triage writes neither. With - * `keepVerdict`, the host keeps that verdict, under that policy, where it is already posted on the head rather than - * writing this over it, labels included. `known` is what the comment already shows of the verdict. + * The waiting marker on `headSha` while its verdict waits for `awaits`: the in-progress comment naming them where triage + * comments, and on Bitbucket Data Center the head's status while it waits for CI; none where triage writes neither. + * Labels stay as the last verdict left them. With `keepVerdict`, the host keeps that verdict, under that policy, where + * it is already posted on the head rather than writing this over it. `known` is what the comment already shows of the verdict. */ -export function waitingMark(source: RepoSource, { repo, number, headSha, checks, events = [] }: { repo: string; number: number; headSha: string; checks: PrFacts["checks"]; events?: readonly unknown[] }, awaits: readonly Awaits[], automatic: readonly unknown[], labels: Record, appLogin: string | undefined, keepVerdict?: Omit, known?: string): WaitingMark | undefined { - const pull = { repo, number, onHead: headSha }; +export function waitingMark(source: RepoSource, { repo, number, headSha }: Head, awaits: readonly Awaits[], automatic: readonly unknown[], appLogin: string | undefined, keepVerdict?: Omit, known?: string): WaitingMark | undefined { const comment = automatic.includes("comment") ? { comment: progressComment(awaits, known), ...(keepVerdict && { keepVerdict }) } : {}; - const labelled = marked(source, awaits, automatic, events); - if (!labelled && !("comment" in comment)) return undefined; - const waiting = awaits.filter((awaited) => awaited !== "ci" || checks !== "none").map((awaited) => WAITING_LABELS[awaited]); - const args = labelled ? markArgs(source, pull, labels, waiting) : { ...pull, labels: [], owned: [] }; - return { effectId: `waiting:${repo.replace("/", "--")}:${number}:${headSha}:${awaits.join("+")}`, repoSource: source, args: { ...args, ...comment, ...(appLogin && { appLogin }) } }; + const status = source === "bitbucket-datacenter" && awaits.includes("ci") ? { status: WAITING_STATUS, statusHead: headSha } : {}; + if (!("comment" in comment) && !("status" in status)) return undefined; + const args = { repo, number, onHead: headSha, labels: [], owned: [], ...status, ...comment, ...(appLogin && { appLogin }) }; + return { effectId: `waiting:${repo.replace("/", "--")}:${number}:${headSha}:${awaits.join("+")}`, repoSource: source, args }; } /** - * What a pull request that merged or closed on `headSha` before its verdict shows: its comment says Triage did not - * finish where triage comments, and Triage's labels come off where it labels; none where triage writes neither. Without - * its comment a verdict on the head cannot be told from a stale one, so only the waiting label comes off. + * What a pull request that merged or closed on `headSha` before its verdict shows where triage comments: its comment + * says Triage did not finish, and Triage's labels come off where it labels. Without its comment a verdict on the head + * cannot be told from a stale one, so nothing is written. */ -export function closedMark(source: RepoSource, { repo, number, headSha, merged }: { repo: string; number: number; headSha: string; merged: boolean }, automatic: readonly unknown[], labels: Record, appLogin: string | undefined): WaitingMark | undefined { +export function closedMark(source: RepoSource, { repo, number, headSha, merged }: Head & { merged: boolean }, automatic: readonly unknown[], labels: Record, appLogin: string | undefined): WaitingMark | undefined { const comment = automatic.includes("comment"); - let owned: readonly string[] = []; - if (source === "github" && automatic.includes("labels")) owned = comment ? ownedLabels(labels) : Object.values(WAITING_LABELS); - if (!comment && owned.length === 0) return undefined; + if (!comment) return undefined; + const owned = source === "github" && automatic.includes("labels") ? ownedLabels(labels) : []; const args = { repo, number, onHead: headSha, labels: [], owned, closedOut: { merged, comment }, ...(appLogin && { appLogin }) }; return { effectId: `closed:${repo.replace("/", "--")}:${number}:${headSha}`, repoSource: source, args }; } /** - * A GitHub pull-request mail from the hub takes Ready off its head before facts, except when every triage event on the - * request is `"approved"`, which leaves labels as they are. A push, open, synchronize, check, comment, catch-up, or any - * mix still takes Ready off. Bitbucket writes nothing here. Any other mail goes on as it is. + * The first pass on a GitHub head takes the earlier verdict's label off before facts; a later event on the same head + * writes no label, and the head's own verdict puts its label on. */ export async function unready(input: unknown, _ctx: EffectContext, _signal: AbortSignal): Promise { const { request, trusted } = trustedRequestOf(input); - if (!request || !trusted || request["kind"] !== "pr") return { unready: true }; - const source = repoSourceOf(request["repoSource"]); + if (!request || !trusted || request["kind"] !== "pr" || request["newHead"] !== true || repoSourceOf(request["repoSource"]) !== "github") return { unready: true }; const { repo, prNumber, headSha, automatic } = request; const pack = parseCheckPack(request["pack"] ?? request["checkPack"]); - if (typeof repo !== "string" || typeof prNumber !== "number" || typeof headSha !== "string" || !Array.isArray(automatic) || !pack) return { unready: true }; - if (!marks(source, automatic) || source !== "github") return { unready: true }; - if (approvalOnly(request["events"])) return { unready: true }; + if (typeof repo !== "string" || typeof prNumber !== "number" || typeof headSha !== "string" || !Array.isArray(automatic) || !automatic.includes("labels") || !pack) return { unready: true }; const appLogin = typeof request["appLogin"] === "string" ? request["appLogin"] : undefined; - const args = { ...markArgs(source, { repo, number: prNumber, onHead: headSha }, pack.labels, []), ...(appLogin && { appLogin }) }; - return { unready: true, mark: { effectId: `unready:${repo.replace("/", "--")}:${prNumber}:${headSha}`, repoSource: source, args } }; + const args = { repo, number: prNumber, onHead: headSha, labels: [], owned: Object.values(pack.labels), ...(appLogin && { appLogin }) }; + return { unready: true, mark: { effectId: `unready:${repo.replace("/", "--")}:${prNumber}:${headSha}`, repoSource: "github", args } }; } diff --git a/packages/triage-workflows/src/logic/render.ts b/packages/triage-workflows/src/logic/render.ts index 39bed4e4..5100284f 100644 --- a/packages/triage-workflows/src/logic/render.ts +++ b/packages/triage-workflows/src/logic/render.ts @@ -1,4 +1,4 @@ -import { BUDGET_EXHAUSTED_NOTE, catalogCheckEnabled, INCOMPLETE_NOTE, citationOf, citationUrl, dependsOnCi, hardFailures, isSizeRule, itemText, NO_CHANGES_REQUESTED, NOT_A_DRAFT, render, TRIAGE_LABELS, VERDICT_LABELS, WAITING_LABELS, type ActionKind, type Awaits, type CheckPack, type CheckItem, type CheckOutput, type CodeReviewOutput, type KeptVerdict, type ModelCustomCheck, type PolicySource, type Rendered, type RepoSource, type TriageHeader, type VerdictStatus, type VerdictWord, type WorkflowState } from "@corbits/triage-contracts"; +import { BUDGET_EXHAUSTED_NOTE, catalogCheckEnabled, INCOMPLETE_NOTE, citationOf, citationUrl, dependsOnCi, hardFailures, isSizeRule, itemText, NO_CHANGES_REQUESTED, NOT_A_DRAFT, render, TRIAGE_LABELS, VERDICT_LABELS, type ActionKind, type Awaits, type CheckPack, type CheckItem, type CheckOutput, type CodeReviewOutput, type KeptVerdict, type ModelCustomCheck, type PolicySource, type Rendered, type RepoSource, type TriageHeader, type VerdictStatus, type VerdictWord, type WorkflowState } from "@corbits/triage-contracts"; import { isRecord } from "@corbits/common"; import type { Stack } from "@corbits/github-reads"; import { Decision } from "@corbits/system-one"; @@ -807,7 +807,7 @@ export function renderVerdict(input: RenderInput): RenderOutput { return withChecks({ ...render("awaiting-review"), verdict: "unsure", degraded, reason }, checks, score, reasons, ctx); } function waitingOf(reason: string, score: number | null): RenderOutput { - return { ...unsureOf(reason, null, score), waiting: true, labels: [WAITING_LABELS.ci], feedback: "" }; + return { ...unsureOf(reason, null, score), waiting: true, labels: [], feedback: "" }; } if (judgeSkipped === true) return unsureOf(NEEDS_JUDGE_REASON); if (judgeLimitExceeded === true) return unsureOf(judgeError!); @@ -843,7 +843,7 @@ export function degradedVerdict(reason: string): RenderOutput { } /** The mirror tool's arguments for a marker: the label on GitHub, the build status on Bitbucket Data Center. */ -export type MarkArgs = { repo: string; number: number; onHead: string; labels: string[]; owned: readonly string[]; keep?: readonly string[]; status?: VerdictStatus; statusHead?: string; comment?: string; keepVerdict?: Omit; closedOut?: { merged: boolean; comment: boolean }; appLogin?: string }; +export type MarkArgs = { repo: string; number: number; onHead: string; labels: string[]; owned: readonly string[]; status?: VerdictStatus; statusHead?: string; comment?: string; keepVerdict?: Omit; closedOut?: { merged: boolean; comment: boolean }; appLogin?: string }; const AWAITS_NAMES: Record = { ci: "CI to finish", judge: "the decision model's answer", "code-review": "code review" }; diff --git a/packages/triage-workflows/src/pr-triage.test.ts b/packages/triage-workflows/src/pr-triage.test.ts index 83246c54..c8a5535b 100644 --- a/packages/triage-workflows/src/pr-triage.test.ts +++ b/packages/triage-workflows/src/pr-triage.test.ts @@ -3,7 +3,7 @@ import { runLocal, type ActionHandler, type OnTriggerPrimitive, type StepInvoker import type { ReactorCapabilities, ReactorInboundEvent, ReactorState } from "@intx/types/runtime"; import { doMarker } from "@corbits/github-tool/do"; import { REPO_DOCS_MAX_BYTES, REPO_RULES_MAX_BYTES, type GithubFetch } from "@corbits/github-reads"; -import { ACTION_KINDS, defaultActions, emptyPack, readPolicyFile, recommendedPack, WAITING_LABELS, type ActionKind, type CheckPack, type CustomCheck, type Step, type PolicyActions, type PolicySource } from "@corbits/triage-contracts"; +import { ACTION_KINDS, defaultActions, emptyPack, readPolicyFile, recommendedPack, type ActionKind, type CheckPack, type CustomCheck, type Step, type PolicyActions, type PolicySource } from "@corbits/triage-contracts"; import { localActionCredentials } from "./local-credentials.js"; import * as actions from "./actions/index.js"; import type { ExecuteOutput } from "./actions/execute.js"; @@ -552,24 +552,12 @@ describe("pr-triage body", () => { expect(github.comments.filter((c) => c.body.includes("corbits-do:"))).toEqual([]); }); - test("a new head swaps Ready for Waiting for CI until it passes, and the verdict takes the waiting label back", async () => { - const pack: CheckPack = { ...emptyPack(REPO), checks: { "ci-green": { enabled: true } } }; - const github = fakeGithub({ labels: ["bug", "Triage: Ready"] }); - github.pr.head = "def0000000000000000000000000000000000000"; - github.pr.ci = "pending"; - await triage(pack, { github }); - expect(github.labels).toEqual(["bug", WAITING_LABELS.ci]); - github.pr.ci = "success"; - await triage(pack, { github }); - expect(github.labels).toEqual(["bug", "Triage: Ready"]); - }); - - test("CI still running holds the head under the waiting label with the in-progress comment and no Dos; once CI completes the verdict replaces the comment in place", async () => { + test("CI still running holds the head with the in-progress comment and no Dos; once CI completes the verdict replaces the comment in place", async () => { const github = fakeGithub(); github.pr.ci = "pending"; const pack: CheckPack = { ...emptyPack(REPO), checks: { "ci-green": { enabled: true } }, actions: HELLO }; const held = await triage(pack, { github }); - expect(held.verdict).toMatchObject({ waiting: true, labels: [WAITING_LABELS.ci], feedback: "", actions: [] }); + expect(held.verdict).toMatchObject({ waiting: true, labels: [], feedback: "", actions: [] }); expect(held.dos).toEqual([]); expect(github.comments.map((comment) => comment.body.split("\n")[1])).toEqual(["**Triage: In progress**"]); expect(github.comments[0]!.body).toContain("waiting for CI to finish"); @@ -845,7 +833,7 @@ describe("a run that could not read the pull request", () => { const { verdict } = await triage(pack, { github }); expect(verdict).toMatchObject({ degraded: "error", actions: [] }); expect(github.labels).toEqual(["bug", "Triage: Failed to Run"]); - expect(writes(github.requests)).toEqual([READY_OFF, FAILED]); + expect(writes(github.requests)).toEqual([FAILED, READY_OFF]); github.pr.unreadFiles = false; await triage(pack, { github }); expect(github.labels).toEqual(["bug", "Triage: Ready"]); diff --git a/packages/triage-workflows/src/triage-steps.ts b/packages/triage-workflows/src/triage-steps.ts index 99392398..58024be5 100644 --- a/packages/triage-workflows/src/triage-steps.ts +++ b/packages/triage-workflows/src/triage-steps.ts @@ -27,8 +27,7 @@ export function hostAction(handler: "facts" | "mirror" | "execute", sources: Sou // Facts, mirror and execute reach the pull request's host through its bound credential; each host call they make is // one call of the host's tool. 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. A held-Do approval mail skips -// triage and executes that one action. Any other mail triages the head. On GitHub, Ready comes off before facts unless -// every event on the request is a person's approval, which leaves labels in place. The mirror writes +// triage and executes that one action. Any other mail triages the head. On GitHub, a new head takes the verdict's label off once before facts. 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. diff --git a/tests/chain/github.ts b/tests/chain/github.ts index 1f53e13b..83138d9a 100644 --- a/tests/chain/github.ts +++ b/tests/chain/github.ts @@ -34,7 +34,8 @@ export type Pull = { /** Absent while the pull request is open. */ closed?: "merged" | "closed"; }; -export type Write = { method: string; path: string; body: unknown }; +/** `head` is the pull request's head when GitHub received the write, for a write to a pull request. */ +export type Write = { method: string; path: string; body: unknown; head?: string }; export function sha(seed: string): string { return new Bun.CryptoHasher("sha1").update(seed).digest("hex"); @@ -235,7 +236,9 @@ export function fakeGithub() { const body = text ? JSON.parse(text) as Record : {}; const answer = writeRoute(repo, named![2]!, req.method, body); if (answer) { - writes.push({ method: req.method, path: pathname, body }); + const pulled = /^\/(?:pulls|issues)\/(\d+)\//.exec(named![2]!); + const head = pulled ? repo.pulls.get(Number(pulled[1]))?.head : undefined; + writes.push({ method: req.method, path: pathname, body, ...(head && { head }) }); return answer; } } diff --git a/tests/chain/judged.test.ts b/tests/chain/judged.test.ts index 535c6608..5d568366 100644 --- a/tests/chain/judged.test.ts +++ b/tests/chain/judged.test.ts @@ -1,6 +1,6 @@ // A policy whose rules need the judge. import { expect, test } from "bun:test"; -import { VERDICT_LABELS, WAITING_LABELS } from "@corbits/triage-contracts"; +import { VERDICT_LABELS } from "@corbits/triage-contracts"; import { chainStack, JUDGED, once } from "./chain.ts"; import { sha, pullOf, comments, labelled } from "./github.ts"; @@ -20,7 +20,7 @@ test("a policy that needs the judge deploys it and posts the judge's verdict onc expect(asked).toContain("focused"); expect(comments(writes)).toEqual([expect.stringContaining("**Triage: In progress**\n\nTriage is waiting for the decision model's answer")]); expect(pull.comments.map((comment) => comment.body)).toEqual([expect.stringContaining("**Triage: Pass**")]); - expect(labelled(writes)).toEqual([WAITING_LABELS.judge, VERDICT_LABELS.pass]); + expect(labelled(writes)).toEqual([VERDICT_LABELS.pass]); // The pass that deployed the judge reported no verdict, so only the judge's is recorded. expect(await verdictsOf(JUDGED, 1)).toEqual([expect.objectContaining({ head_sha: head, state: "ready-monitoring" })]); }); diff --git a/tests/chain/waiting.test.ts b/tests/chain/waiting.test.ts index 80e5fed5..dcbcbe60 100644 --- a/tests/chain/waiting.test.ts +++ b/tests/chain/waiting.test.ts @@ -1,6 +1,6 @@ // A head whose CI runs after the first post: its manager ends while it waits, and the check event wakes the next. import { expect, test } from "bun:test"; -import { VERDICT_LABELS, WAITING_LABELS } from "@corbits/triage-contracts"; +import { VERDICT_LABELS } from "@corbits/triage-contracts"; import { deliver } from "./stack.ts"; import { chainStack, WAITING, isLiveDeployment, once } from "./chain.ts"; import { sha, pullOf, comments, labelled } from "./github.ts"; @@ -13,7 +13,7 @@ const waiting = once(async function waiting() { const [before, asked, missed] = [(await managers()).length, model.decisions.length, github.misses.length]; github.setChecks(WAITING, head, [{ name: "build", status: "in_progress", conclusion: null }]); await pushed(WAITING, pullOf(1, head)); - await until("the Waiting label", () => labelled(github.writesTo(WAITING, 1)).includes(WAITING_LABELS.ci) || undefined); + await until("the in-progress comment", () => comments(github.writesTo(WAITING, 1)).length > 0 || undefined); // The manager ends once it posted that the head waits; the check event is sent only after. const ended = await settled(WAITING, 1); const whileWaiting = { writes: [...github.writesTo(WAITING, 1)], claim: ended, managers: (await managers()).slice(before), asked: model.decisions.length - asked }; @@ -29,7 +29,7 @@ const waiting = once(async function waiting() { test("a head whose CI runs after the first post ends its manager while it waits, and the check event wakes a fresh one that judges it and posts the final verdict", async () => { const { head, whileWaiting, claim, deployed, asked, pull, misses } = await waiting(); expect(misses).toEqual([]); - expect(labelled(whileWaiting.writes)).toEqual([WAITING_LABELS.ci]); + expect(labelled(whileWaiting.writes)).toEqual([]); expect(comments(whileWaiting.writes)).toEqual([expect.stringContaining("**Triage: In progress**\n\nTriage is waiting for CI to finish")]); expect(whileWaiting.asked).toBe(0); expect(whileWaiting.claim.status).toBe("ended"); diff --git a/tests/model/rules.ts b/tests/model/rules.ts index 47672b1f..384f6d8a 100644 --- a/tests/model/rules.ts +++ b/tests/model/rules.ts @@ -1,8 +1,7 @@ // The rules in docs/INVARIANTS.md that a settled pull request is checked against: what GitHub ends up holding, the // writes it received, and what is left of its runs and mail. import { isTriageComment } from "@corbits/github-reads"; -import { closedText, HTML_MARKERS, TRIAGE_LABELS, VERDICT_LABELS, WAITING_LABELS, type Awaits } from "@corbits/triage-contracts"; -import { progressComment } from "@corbits/triage-workflows/logic/render"; +import { closedText, FAILED_LABEL, HTML_MARKERS, TRIAGE_LABELS, VERDICT_LABELS } from "@corbits/triage-contracts"; import type { Write } from "../chain/github.ts"; import type { Played } from "./events.ts"; import type { modelStack } from "./stack.ts"; @@ -12,13 +11,11 @@ type Stack = Awaited>; /** More takes than this by one manager on one head is a manager polling for what will not come; the model needs two. */ const TAKES_PER_HEAD = 4; -const PROGRESS_TITLE = "**Triage: In progress**"; - /** The title a verdict comment opens with under its marker, by the verdict its header names. */ const VERDICT_TITLES: Record = { pass: "**Triage: Pass**", fail: "**Triage: Fail**", unsure: "**Triage: Unsure**" }; /** - * R1: exactly one current Triage comment, a verdict on the head, and no waiting label left. A pull request that closed + * R1: exactly one current Triage comment, a verdict on the head. A pull request that closed * keeps a verdict it had on its last head, or else says it closed before Triage finished, with none of Triage's labels; * one closed before any comment went up has none. */ @@ -40,8 +37,6 @@ function oneVerdict(stack: Stack, pull: Played): string[] { const title = header && VERDICT_TITLES[header.verdict]; if (!title || line !== title) breaks.push(`R1: the Triage comment is no verdict: ${line}`); } - const waiting = labels.filter((label) => Object.values(WAITING_LABELS).includes(label)); - if (waiting.length > 0) breaks.push(`R1: the pull request still has ${waiting.join(", ")}`); return breaks; } @@ -91,42 +86,48 @@ function onceEach(writes: readonly Write[]): string[] { } /** - * R7, over every write the pull request received: whenever a Triage comment is written, and once it settles, the - * waiting labels the pull request holds name what its in-progress comment waits for, and none stand beside a verdict. - * Not Ready and Needs Human Review keep the labels as they are until the verdict. + * R3 for labels, over every write the pull request received: Triage adds no label but a verdict's or Failed to Run. A + * new head stands with no verdict label until its own verdict or verdict label, and once a head has a verdict its labels are written + * again only by a verdict that differs from the one before. */ -function waitingNamed(writes: readonly Write[]): string[] { +function labelPerVerdict(writes: readonly Write[]): string[] { const breaks: string[] = []; + const verdicts = [...Object.values(VERDICT_LABELS), FAILED_LABEL]; const on = new Set(); - let shown: string | undefined; - function check(): void { - if (shown === undefined || [VERDICT_LABELS.fail, VERDICT_LABELS.unsure].some((label) => on.has(label))) return; - const awaits = (Object.keys(WAITING_LABELS) as Awaits[]).filter((awaited) => on.has(WAITING_LABELS[awaited])); - const labels = awaits.map((awaited) => WAITING_LABELS[awaited]).join(", ") || "no waiting label"; - const progress = shown.includes(PROGRESS_TITLE); - if (!progress && awaits.length > 0) breaks.push(`R7: ${labels} beside a verdict`); - // What the comment already shows of the verdict sits between its title and what it waits for. - if (progress && (awaits.length === 0 || !shown.endsWith(progressComment(awaits).slice(PROGRESS_TITLE.length)))) breaks.push(`R7: ${labels} beside ${shown.slice(shown.indexOf(PROGRESS_TITLE))}`); - } - for (const { method, path, body } of writes) { + const decided = new Map(); + const labelledOn = new Set(); + let changed = false; + for (const { method, path, body, head } of writes) { const text = (body as { body?: unknown } | null)?.body; + const labelled = path.endsWith("/labels") || path.includes("/labels/"); if ((method === "POST" || method === "PATCH") && path.includes("/comments") && typeof text === "string" && isTriageComment(text) && !HTML_MARKERS.isSuperseded(text)) { - check(); - shown = text; - } else if (method === "POST" && path.endsWith("/labels")) { - for (const label of (body as { labels: string[] }).labels) on.add(label); + const header = HTML_MARKERS.headerOf(text); + if (header?.head && VERDICT_TITLES[header.verdict] === text.split("\n")[1]) { + changed = decided.get(header.head) !== header.verdict; + decided.set(header.head, header.verdict); + } else if (head && !decided.has(head) && !labelledOn.has(head) && verdicts.some((label) => on.has(label))) { + breaks.push(`R3: a verdict label stands on ${head} before its verdict`); + } + } else if (labelled && head && decided.has(head) && !changed) { + breaks.push(`R3: ${method} ${path} on ${head} with its verdict unchanged`); + } + if (method === "POST" && path.endsWith("/labels")) { + for (const label of (body as { labels: string[] }).labels) { + if (!verdicts.includes(label) && TRIAGE_LABELS.includes(label)) breaks.push(`R3: ${label} was added`); + if (verdicts.includes(label) && head) labelledOn.add(head); + on.add(label); + } } else if (method === "DELETE" && path.includes("/labels/")) { on.delete(decodeURIComponent(path.slice(path.lastIndexOf("/") + 1))); } } - check(); return breaks; } /** Each rule the settled pull request breaks, by its number, saying how; `missed` is how many misses the fake GitHub had before it. */ export async function broken(stack: Stack, pull: Played, missed: number): Promise { const { live, unattributed, lost, failed, waited, exhausted, mostTakes } = await stack.leftover(pull.number); - const breaks = [...oneVerdict(stack, pull), ...onceEach(stack.writes(pull.number)), ...waitingNamed(stack.writes(pull.number))]; + const breaks = [...oneVerdict(stack, pull), ...onceEach(stack.writes(pull.number)), ...labelPerVerdict(stack.writes(pull.number))]; if (live.length > 0) breaks.push(`R5: managers still live: ${live.join(", ")}`); if (unattributed.length > 0) breaks.push(`R5: live managers deployed for no single pull request: ${unattributed.join(", ")}`); if (lost.length > 0) breaks.push(`R5: mail no run took: ${lost.map((mail) => mail.to).join(", ")}`);