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 0786ab42f..5443b7b07 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -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. 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. +Triage adds no labels unless the policy file sets `labels: true`. With labels off, Triage takes its own verdict labels and **Triage: Failed to Run** off a pull request the first time it writes to it. With `labels: true`, or a map renaming them, Triage 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. 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. 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. 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. +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 the policy file turns labels on and 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/packages/triage-contracts/src/check-pack.ts b/packages/triage-contracts/src/check-pack.ts index 983270325..3a45ae353 100644 --- a/packages/triage-contracts/src/check-pack.ts +++ b/packages/triage-contracts/src/check-pack.ts @@ -686,7 +686,7 @@ const STARTER_REASONS = { } as const satisfies Partial>; /** - * The first policy file for a repository with none: a chosen subset of the recommended rules, with no steps. It keeps + * The first policy file for a repository with none: a chosen subset of the recommended rules, with no steps or labels. It keeps * the rules that need no model and hold for any repository; a linked issue and forbidden paths are left out, since a * repository without an issue tracker, or one that edits `.github/**`, would fail every pull request. */ @@ -696,7 +696,8 @@ export function starterPack(repo: string): CheckPack { for (const [id, reason] of Object.entries(STARTER_REASONS) as Array<[keyof typeof STARTER_REASONS, string]>) { checks[id] = { ...recommended[id]!, reason, evidence: [MAINTAINER_STATED] }; } - return { ...emptyPack(repo), checks, actions: { when: [...POLICY_EVENTS], pass: [], fail: [], unsure: [] } }; + const { kind, schemaVersion, repo: clean, custom } = emptyPack(repo); + return { kind, schemaVersion, repo: clean, checks, custom, actions: { when: [...POLICY_EVENTS], pass: [], fail: [], unsure: [] } }; } export function catalogCheckEnabled(pack: CheckPack, id: CatalogId): boolean { @@ -1161,7 +1162,8 @@ function parseGroups(raw: unknown): Record | undefined { } function parseLabels(raw: unknown): VerdictLabels | undefined { - if (raw === undefined || raw === true) return { ...VERDICT_LABELS }; + if (raw === undefined) return undefined; + if (raw === true) return { ...VERDICT_LABELS }; const row = record(raw, "labels"); const extra = Object.keys(row).filter((key) => !(VERDICTS as readonly string[]).includes(key)); if (extra.length) throw new Error(`labels must set only pass, fail or unsure, not ${extra.join(", ")}.`); diff --git a/packages/triage-contracts/src/do.test.ts b/packages/triage-contracts/src/do.test.ts index 4d13c4a34..941cb8c8d 100644 --- a/packages/triage-contracts/src/do.test.ts +++ b/packages/triage-contracts/src/do.test.ts @@ -10,6 +10,7 @@ const FILE = `rules: text: CI passes. reason: Required. evidence: [maintainer-stated] +labels: true actions: pass: [{ do: assign, target: { to: author } }] unsure: [{ do: close }, { do: nothing }] diff --git a/packages/triage-contracts/src/policy-export.ts b/packages/triage-contracts/src/policy-export.ts index 3c6f1d1d8..faf4e3496 100644 --- a/packages/triage-contracts/src/policy-export.ts +++ b/packages/triage-contracts/src/policy-export.ts @@ -111,10 +111,10 @@ function fileActions(pack: CheckPack): Record | undefined { return Object.keys(actions).length ? actions : undefined; } -function fileLabels({ labels }: CheckPack): Record | undefined { +function fileLabels({ labels }: CheckPack): Record | true | undefined { if (!labels) return undefined; const changed = VERDICTS.flatMap((verdict) => (labels[verdict] === VERDICT_LABELS[verdict] ? [] : [[verdict, labels[verdict]]])); - return changed.length ? Object.fromEntries(changed) : undefined; + return changed.length ? Object.fromEntries(changed) : true; } /** The schema 3 file for `pack`, and each thing it holds that the file cannot. */ diff --git a/packages/triage-workflows/src/actions/evaluate.ts b/packages/triage-workflows/src/actions/evaluate.ts index ea435196e..57a9a792b 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, FAILED_LABEL, hostKinds, marksWaiting, ownedLabels, repoSourceOf, UNREAD_STATUS, VERDICT_LABELS, type ActionKind, type CheckPack, type RepoSource, type Step, type VerdictLabels } from "@corbits/triage-contracts"; +import { checkName, FAILED_LABEL, hostKinds, marksWaiting, ownedLabels, repoSourceOf, UNREAD_STATUS, type ActionKind, type CheckPack, type RepoSource, type Step, type VerdictLabels } from "@corbits/triage-contracts"; import { type } from "arktype"; import { approvalHeldBy, changesWanted, heldApprovals, holdDos, packApproves, stepActions, type ActionInput, type SuggestedAction } from "../logic/actions.js"; import type { CheckResult, DeterministicResult, PrFacts } from "../logic/checks.js"; @@ -164,9 +164,8 @@ function degradedMark({ repo, number, headSha }: PrFacts, source: RepoSource, au return { effectId: `degraded:${repo.replace("/", "--")}:${number}:${headSha}`, repoSource: source, args }; } -/** Without a policy Triage labels with the default names. */ function verdictLabelsOf(it: Item): VerdictLabels | undefined { - return it.pack ? it.pack.labels : VERDICT_LABELS; + return it.pack?.labels; } function degradedOf(it: Item, reason: string): Verdict { diff --git a/packages/triage-workflows/src/pr-triage.test.ts b/packages/triage-workflows/src/pr-triage.test.ts index c8a5535b5..0983f4a19 100644 --- a/packages/triage-workflows/src/pr-triage.test.ts +++ b/packages/triage-workflows/src/pr-triage.test.ts @@ -839,7 +839,7 @@ describe("a run that could not read the pull request", () => { expect(github.labels).toEqual(["bug", "Triage: Ready"]); }); - test("a policy the hub can't read marks each head the mail names failed with why on both hosts, once, on GitHub only with the grant", async () => { + test("a policy the hub can't read marks each head the mail names failed with why, on GitHub with no label and the verdict label taken off, on Bitbucket once", async () => { const reason = "The policy file has unknown checks: max-size."; const mail = { kind: "pr", prNumber: 8, policy: { enabled: true }, policyUnreadable: reason, automatic: ACTION_KINDS, events: ["opened"] }; const github = fakeGithub({ labels: ["bug", "Triage: Ready"] }); @@ -847,7 +847,7 @@ describe("a run that could not read the pull request", () => { const [marked] = (outputs["evaluateRules"] as { items: Verdict[] }).items; expect(marked).toMatchObject({ repo: REPO, number: 8, degraded: "error", reason: `Policy can't be read: ${reason}`, actions: [] }); expect(unsettledBy(marked!, github.pr.head)).toEqual({}); - expect(github.labels).toEqual(["bug", "Triage: Failed to Run"]); + expect(github.labels).toEqual(["bug"]); const ungranted = fakeGithub({ labels: ["bug"] }); await runTriage(ungranted.gh, [], false, hubMail({ ...mail, repo: REPO, headSha: ungranted.pr.head, automatic: [] }, "github@acme.test")); expect(writes(ungranted.requests)).toEqual([]); diff --git a/skills/corbits-triage-setup/reference.md b/skills/corbits-triage-setup/reference.md index 9708ee69d..60e3de310 100644 --- a/skills/corbits-triage-setup/reference.md +++ b/skills/corbits-triage-setup/reference.md @@ -97,7 +97,7 @@ To add a group to `sensitive-paths`, change the starter's rule. Writing `groups` ## Actions -Optional. Without `actions`, Triage only posts its comment and verdict label (a build status on Bitbucket Data Center). Each verdict has an ordered list of at most 20 steps; a verdict left out has none. +Optional. Without `actions`, Triage only posts its comment, and its verdict label where `labels` turns labels on (a build status on Bitbucket Data Center). Each verdict has an ordered list of at most 20 steps; a verdict left out has none. ```yaml actions: @@ -150,7 +150,7 @@ Triage carries out an action on its own only where the workspace allows that act ## Labels -Optional. Renames the verdict labels (at most 50 characters each). Defaults: +Optional. Without `labels`, Triage adds no labels. `labels: true` turns the verdict labels on with their default names; a map renames them (at most 50 characters each), a verdict left out keeping its default: ```yaml labels: diff --git a/tests/chain/chain.ts b/tests/chain/chain.ts index 6682cd60a..36c50c5ca 100644 --- a/tests/chain/chain.ts +++ b/tests/chain/chain.ts @@ -42,7 +42,7 @@ const ASSIGN_ON_PASS = "actions:\n pass:\n - do: assign\n target:\n const REVIEW_ON_PASS = "actions:\n pass:\n - do: request-review\n target:\n to: users\n users:\n - hubot\n"; export function policy(rules: string[], extra = ""): string { - return `rules:\n${rules.map((rule) => `${rule} reason: The maintainers set it.\n evidence:\n - maintainer-stated\n`).join("")}${extra}`; + return `rules:\n${rules.map((rule) => `${rule} reason: The maintainers set it.\n evidence:\n - maintainer-stated\n`).join("")}labels: true\n${extra}`; } /** The judge finds every change focused, by the first of a choice's options, and no pull request steering it. */ diff --git a/tests/model/rules.postgres.test.ts b/tests/model/rules.postgres.test.ts index 709af2fd4..73f344aea 100644 --- a/tests/model/rules.postgres.test.ts +++ b/tests/model/rules.postgres.test.ts @@ -5,8 +5,8 @@ import { errorText } from "@corbits/common"; import { isTriageComment } from "@corbits/github-reads"; import { HTML_MARKERS, VERDICT_LABELS, emptyPack } from "@corbits/triage-contracts"; import { CODE_REVIEW_RUNNING } from "@corbits/triage-workflows/logic/render"; -import { labelled, OWNER, type Write } from "../chain/github.ts"; -import { LETTERS, open, play, settle, walk, type Letter } from "./events.ts"; +import { labelled, OWNER, pullOf, sha, type Write } from "../chain/github.ts"; +import { LETTERS, open, play, settle, walk, type Letter, type Played } from "./events.ts"; import { broken, verdictKept } from "./rules.ts"; import { MANAGER_RELEASE, modelStack, REVIEW_RELEASE } from "./stack.ts"; @@ -30,6 +30,9 @@ const diesAfterMail = await modelStack(REPO, { pack: PACK, automatic: ["labels", const LABELLING: typeof PACK = { ...PACK, actions: { ...PACK.actions, when: ["opened"], pass: [{ do: "labels", target: { from: "list", labels: ["shipit"] } }] } }; const labelling = await modelStack(`${OWNER}/labelling`, { pack: LABELLING, automatic: ["labels", "comment"] }); const labellingReviewed = await modelStack(`${OWNER}/labelling-reviewed`, { pack: LABELLING, automatic: ["labels", "comment"], review: "dies-after-mail" }); +const UNLABELLED: typeof PACK = { ...PACK, repo: `${OWNER}/unlabelled` }; +delete UNLABELLED.labels; +const unlabelled = await modelStack(UNLABELLED.repo, { pack: UNLABELLED, automatic: ["labels", "comment"] }); setDefaultTimeout(5 * 60_000); afterAll(async () => { await stack.stop(); @@ -37,6 +40,7 @@ afterAll(async () => { await diesAfterMail.stop(); await labelling.stop(); await labellingReviewed.stop(); + await unlabelled.stop(); }); let pulls = 0; @@ -164,3 +168,16 @@ test("with code review on, an action on opened runs once when the pass after CI await labellingReviewed.drain(); expect(shipits(labellingReviewed, pull.number)).toEqual(["shipit"]); }); + +test("with labels off, Triage adds no label and takes an earlier verdict label off once", async () => { + const number = ++pulls; + const pull: Played = { number, heads: [sha(`${number}-0`)], draft: false }; + await unlabelled.open({ ...pullOf(number, pull.heads[0]!), labels: [VERDICT_LABELS.pass] }); + await settle(unlabelled, pull); + await play(unlabelled, pull, "push"); + await settle(unlabelled, pull); + const writes = unlabelled.writes(number); + const removed = writes.filter((write) => write.method === "DELETE" && write.path.includes("/labels/")).map((write) => decodeURIComponent(write.path.split("/labels/")[1]!)); + expect(labelled(writes)).toEqual([]); + expect(removed).toEqual([VERDICT_LABELS.pass]); +}); diff --git a/tests/policy-corpus/corpus.test.ts b/tests/policy-corpus/corpus.test.ts index 5a451b4d6..28e90881d 100644 --- a/tests/policy-corpus/corpus.test.ts +++ b/tests/policy-corpus/corpus.test.ts @@ -5,7 +5,7 @@ import { join } from "node:path"; import { type } from "arktype"; import { parse } from "yaml"; import { asRecord } from "@corbits/common"; -import { checkPolicyFile, POLICY_FILE_PATH, policyFileReason, RESERVED_BLOCKS, type PolicyWorkflow, type RepoSource } from "@corbits/triage-contracts"; +import { checkPolicyFile, POLICY_FILE_PATH, policyFileReason, RESERVED_BLOCKS, type PolicyWorkflow, type RepoSource, type VerdictLabels } from "@corbits/triage-contracts"; import { stateOf, textRefusal, type PolicyFileState } from "../../apps/hub/src/github/policy-file.ts"; import { fileRefusal, readRepoPolicy, type RepoPolicyDeps } from "../../apps/hub/src/github/repo-policy.ts"; import { Failure } from "../../apps/hub/src/route-schema.ts"; @@ -37,7 +37,7 @@ const REPORTS: Record = { }; /** `seams` is where a reader answers otherwise by design; every other reader answers `accept`, `reason` and `pending`. */ -const Expected = type({ "+": "reject", ...outcome, "seams?": type({ "+": "reject", "action?": Outcome, "checkPolicyFile?": Outcome, "hubRead?": Outcome, "hubWrite?": Outcome }) }); +const Expected = type({ "+": "reject", ...outcome, "labels?": type({ pass: "string", fail: "string", unsure: "string" }).or("null"), "seams?": type({ "+": "reject", "action?": Outcome, "checkPolicyFile?": Outcome, "hubRead?": Outcome, "hubWrite?": Outcome }) }); function hubDeps(state: PolicyFileState): RepoPolicyDeps { return { @@ -86,24 +86,26 @@ function action(text: string): Observed { } /** The file as the hub reads it from the default branch, resolved as triage runs it. */ -async function hubRead(state: PolicyFileState): Promise { +async function hubRead(state: PolicyFileState): Promise { const read = await readRepoPolicy(hubDeps(state), TENANT, REPO, "github"); if (read.status === "invalid") return { accept: false, reason: read.reason }; if (read.status !== "ok" || state.status !== "ok") throw new Error(`The hub read ${state.status} but resolved ${read.status}.`); - return { accept: true, pending: state.file.pending, reserved: reservedIn(read.policyWorkflow ?? state.workflow) }; + return { accept: true, pending: state.file.pending, reserved: reservedIn(read.policyWorkflow ?? state.workflow), labels: read.pack.labels ?? null }; } -async function readers(text: string): Promise> { +async function readers(text: string): Promise & { labels?: VerdictLabels | null }> { const checked = checkPolicyFile(text, REPO); const state = stateOf({ path: POLICY_FILE_PATH, read: { kind: "file", sha: "0".repeat(40), text, url: `https://github.com/${REPO}/blob/main/${POLICY_FILE_PATH}` } }, REPO); async function refusal(tenantId: string, repo: string, source: RepoSource, policy: Record): Promise { return fileRefusal(hubDeps(state), tenantId, repo, source, policy); } const refused = await textRefusal(refusal, TENANT, { name: REPO, connected: true }, text); + const { labels, ...read } = await hubRead(state); return { + labels, checkPolicyFile: checked.ok ? { accept: true, pending: checked.pending && policyFileReason(checked.pending), reserved: reservedIn(checked.workflow) } : { accept: false, reason: policyFileReason(checked.problems) }, action: action(text), - hubRead: await hubRead(state), + hubRead: read, hubWrite: refused ? { accept: false, reason: Failure.assert(await refused.json()).error.message } : { accept: true }, }; } @@ -115,8 +117,9 @@ function wanted({ accept, reason, pending }: Outcome, reader: ReaderName): Obser const policies = readdirSync(FILES).filter((name) => !name.endsWith(".expect.yml")); test.each(policies)("every reader answers %s as expected", async (name) => { - const { seams, ...expected } = Expected.assert(parse(readFileSync(join(FILES, name.replace(/\.yml$/, ".expect.yml")), "utf8"))); + const { seams, labels, ...expected } = Expected.assert(parse(readFileSync(join(FILES, name.replace(/\.yml$/, ".expect.yml")), "utf8"))); const observed = await readers(readFileSync(join(FILES, name), "utf8")); + if (labels !== undefined) expect(observed.labels).toEqual(labels); expect(READERS.map((reader) => [reader, observed[reader]])).toEqual(READERS.map((reader) => [reader, wanted(seams?.[reader] ?? expected, reader)])); }, 15_000); diff --git a/tests/policy-corpus/files/labels-off.expect.yml b/tests/policy-corpus/files/labels-off.expect.yml new file mode 100644 index 000000000..7fd837414 --- /dev/null +++ b/tests/policy-corpus/files/labels-off.expect.yml @@ -0,0 +1,3 @@ +# Without a labels key Triage adds no labels. +accept: true +labels: null diff --git a/tests/policy-corpus/files/labels-off.yml b/tests/policy-corpus/files/labels-off.yml new file mode 100644 index 000000000..577d93564 --- /dev/null +++ b/tests/policy-corpus/files/labels-off.yml @@ -0,0 +1,5 @@ +rules: + - check: ci-green + text: CI passes. + reason: Required. + evidence: [maintainer-stated] diff --git a/tests/policy-corpus/files/labels-on.expect.yml b/tests/policy-corpus/files/labels-on.expect.yml new file mode 100644 index 000000000..8e4cb0596 --- /dev/null +++ b/tests/policy-corpus/files/labels-on.expect.yml @@ -0,0 +1,3 @@ +# labels: true turns the verdict labels on with their default names. +accept: true +labels: { pass: "Triage: Ready", fail: "Triage: Not Ready", unsure: "Triage: Needs Human Review" } diff --git a/tests/policy-corpus/files/labels-on.yml b/tests/policy-corpus/files/labels-on.yml new file mode 100644 index 000000000..65e4d60c0 --- /dev/null +++ b/tests/policy-corpus/files/labels-on.yml @@ -0,0 +1,6 @@ +rules: + - check: ci-green + text: CI passes. + reason: Required. + evidence: [maintainer-stated] +labels: true diff --git a/tests/policy-corpus/files/labels-renamed.expect.yml b/tests/policy-corpus/files/labels-renamed.expect.yml new file mode 100644 index 000000000..79ac6cda4 --- /dev/null +++ b/tests/policy-corpus/files/labels-renamed.expect.yml @@ -0,0 +1,3 @@ +# A rename map turns the verdict labels on; a verdict it leaves out keeps its default name. +accept: true +labels: { pass: "Ready to ship", fail: "Triage: Not Ready", unsure: "Triage: Needs Human Review" } diff --git a/tests/policy-corpus/files/labels-renamed.yml b/tests/policy-corpus/files/labels-renamed.yml new file mode 100644 index 000000000..e2b18ddc8 --- /dev/null +++ b/tests/policy-corpus/files/labels-renamed.yml @@ -0,0 +1,7 @@ +rules: + - check: ci-green + text: CI passes. + reason: Required. + evidence: [maintainer-stated] +labels: + pass: "Ready to ship"