Skip to content

Commit 370fac2

Browse files
committed
fix(triage): clear the verdict label once on a new head (CL-12164)
1 parent 5a2068e commit 370fac2

12 files changed

Lines changed: 77 additions & 39 deletions

File tree

‎docs/Corbits Triage User Manual/Corbits Triage User Manual.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ When Triage may not post its verdict, the pull request's line says so, such as *
387387

388388
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.
389389

390-
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 the earlier verdict's label stays until the new verdict replaces it, so new commits change no label unless the verdict changes. 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.
390+
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.
391391

392392
An **Unsure** comment lists each rule Triage could not confirm, with checks still running or cancelled under **Waiting on**.
393393

@@ -400,7 +400,7 @@ Triage never merges a pull request while a person's review thread on it is open.
400400

401401
### Labels
402402

403-
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; while it waits, only its comment shows progress, and the earlier verdict's label stays. 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.
403+
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.
404404

405405
On Bitbucket Data Center labels have no effect; see [Bitbucket Data Center repositories](#bitbucket-data-center-repositories).
406406

@@ -547,7 +547,7 @@ The bar at the bottom of the pane always offers the next step first. For a passi
547547

548548
### What the author sees
549549

550-
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 verdict label stays through new commits until the new verdict replaces it, so it can stand on a commit Triage is still checking; the comment says when Triage is still checking. 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.
550+
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.
551551

552552
## Settings: Triage, code review and people
553553

‎docs/INVARIANTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ request that changes how Triage keeps a rule names the rule.
77

88
- **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.
99
- **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.
10-
- **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, and a verdict's label is added again only after another one.
10+
- **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.
1111
- **R4** Unreadable input never passes or approves. No automated check yet.
1212
- **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.
1313
- **R6** An older run never overwrites a newer head's comment, label, status or approval. No automated check yet.

‎packages/triage-manager/src/actions/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
export { execute, facts, mirror, rules } from "@corbits/triage-workflows/actions/index";
1+
export { execute, facts, mirror, rules, unready } from "@corbits/triage-workflows/actions/index";
22
export { executeApprovals } from "./approvals.js";
33
export { forward } from "./listener.js";
44
export { deployJudge, judgeMailed, judgeScope, verdict } from "./judge.js";

‎packages/triage-manager/src/actions/manager.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ export async function take(input: unknown, ctx: EffectContext, _signal: AbortSig
190190
// A pass of approvals alone, before any pull request mail was recorded, has nothing to triage.
191191
const triage = request !== undefined && (forHead !== undefined || report.review !== undefined || report.reviewFailed !== undefined || report.judge !== undefined || report.judgeFailed !== undefined || overdue || waitedOut);
192192
const starting = headSince === undefined || Date.now() - Date.parse(headSince) < CI_START_MS;
193-
const taken = request && { ...request, ...(waitedOut && { noWait: true }), ...(!waitedOut && starting && { ciStarting: true }) };
193+
const taken = request && { ...request, ...(waitedOut && { noWait: true }), ...(!waitedOut && starting && { ciStarting: true }), ...(headSince === undefined && { newHead: true }) };
194194
const verdict = report.judge ?? storedVerdict(reviewers, head);
195195
const waiting = waitingSince !== undefined && !waitedOut;
196196
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 }) };

‎packages/triage-manager/src/triage-manager.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ function judgeSteps(sources: Sources): Record<string, Primitive> {
107107
* reviewer first waits `POLL_MS`.
108108
*/
109109
function passOf(id: string, sources: Sources) {
110-
const { rules, mirrorRules, executeRules } = triageSteps(sources);
110+
const { unready, markUnready, rules, mirrorRules, executeRules } = triageSteps(sources);
111111
return defineWorkflow({
112112
id: `${id}-pass`,
113113
trigger: { type: "manual" },
@@ -123,8 +123,10 @@ function passOf(id: string, sources: Sources) {
123123
// Runs on every pass, approving nothing when there is nothing to run, so every branch's done reports its outcomes.
124124
approvals: { ...hostAction("execute", sources, stepsOf("take"), ["findingThreads"]), handler: "executeApprovals" },
125125
ran: gate({ when: { from: "steps.take.output.run" }, then: "triaged", else: "done", after: ["approvals"] }),
126-
triaged: gate({ when: { from: "steps.take.output.triage" }, then: "facts", else: "done", after: ["ran"] }),
127-
facts: hostAction("facts", sources, TAKEN, ["triaged"]),
126+
triaged: gate({ when: { from: "steps.take.output.triage" }, then: "unready", else: "done", after: ["ran"] }),
127+
unready: { ...unready, input: TAKEN, after: ["triaged"] },
128+
markUnready,
129+
facts: hostAction("facts", sources, TAKEN, ["markUnready"]),
128130
rules,
129131
route: gate({ when: { from: "steps.rules.output.needsJudgment" }, then: "judgeScope", else: "evaluateRules", after: ["rules"] }),
130132
...judgeSteps(sources),

‎packages/triage-workflows/src/actions/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ export { rules } from "./rules.js";
22
export { evaluate } from "./evaluate.js";
33
export { parts } from "./parts.js";
44
export { approval } from "./approval.js";
5+
export { unready } from "./waiting.js";
56
export { measure } from "./measure.js";
67
export { facts } from "./facts.js";
78
export { mirror } from "./mirror.js";

0 commit comments

Comments
 (0)