From 066e5ec074dc8a5d8be7438d1d5e51a4b830af0e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 4 Nov 2026 19:01:55 -0800 Subject: [PATCH] refactor(triage): let a policy turn labels off (CL-12165) --- apps/hub/src/github/automation.ts | 4 +-- apps/hub/src/github/policy-grants.ts | 2 +- .../src/components/repo-panel/PolicyRules.tsx | 4 +-- packages/triage-contracts/src/check-pack.ts | 28 +++++++++++-------- packages/triage-contracts/src/do.ts | 10 +++---- .../triage-contracts/src/policy-export.ts | 5 ++-- .../src/policy-file-problems.ts | 2 +- packages/triage-contracts/src/policy-rules.ts | 2 +- packages/triage-contracts/src/presets.ts | 6 ++-- .../triage-contracts/src/resolve-policy.ts | 15 ++++++---- .../triage-workflows/src/actions/evaluate.ts | 24 +++++++++------- .../triage-workflows/src/actions/waiting.ts | 8 +++--- .../src/logic/duplicates.test.ts | 4 +-- .../triage-workflows/src/logic/render.test.ts | 9 +++--- packages/triage-workflows/src/logic/render.ts | 15 ++++++---- .../src/logic/request-changes.test.ts | 4 +-- tooling/facts.ts | 3 +- tooling/preview.ts | 2 +- 18 files changed, 83 insertions(+), 64 deletions(-) diff --git a/apps/hub/src/github/automation.ts b/apps/hub/src/github/automation.ts index d21064b63..43122ea17 100644 --- a/apps/hub/src/github/automation.ts +++ b/apps/hub/src/github/automation.ts @@ -5,7 +5,7 @@ import { evaluateGrants } from "@intx/authz"; import { schema, type DB } from "@intx/db"; import { generateId } from "@intx/hub-common"; import type { GrantRule } from "@intx/types/authz"; -import { ACTION_KINDS, DO_GRANT_ACTION, doResource, hostKinds, postingKinds, TRIAGE_ROLE, type ActionKind, type RepoSource } from "@corbits/triage-contracts"; +import { ACTION_KINDS, DO_GRANT_ACTION, doResource, hostKinds, postingKinds, TRIAGE_ROLE, VERDICT_LABELS, type ActionKind, type RepoSource } from "@corbits/triage-contracts"; import { lockHolder } from "../system-writes/grants.js"; import { allowRole, grantAdminHost, type Tx } from "./admin-github-grant.js"; @@ -62,7 +62,7 @@ export async function grantWanted(tx: Tx, tenantId: string, repo: string, source await lockHolder(tx, tenantId, roleId); // Checked under the role's lock, which every grant writer takes, so a read overtaken meanwhile writes nothing. if (!stillWanted()) return; - const kinds = hostKinds(source).filter((kind) => wanted.includes(kind) || postingKinds(source).includes(kind)); + const kinds = hostKinds(source).filter((kind) => wanted.includes(kind) || postingKinds(source, VERDICT_LABELS).includes(kind)); await deleteKinds(tx, tenantId, roleId, repo, source, ACTION_KINDS.filter((kind) => !kinds.includes(kind))); for (const kind of kinds) await allowRole(tx, tenantId, roleId, doResource(kind, source, repo), DO_GRANT_ACTION, stamp); } diff --git a/apps/hub/src/github/policy-grants.ts b/apps/hub/src/github/policy-grants.ts index 42d665240..79b5ee1ba 100644 --- a/apps/hub/src/github/policy-grants.ts +++ b/apps/hub/src/github/policy-grants.ts @@ -61,7 +61,7 @@ export function createPolicyGrants(deps: PolicyGrantsDeps) { const read = await deps.preview(tenantId, repo, source, text); if (read.status !== "ok") return []; // Enabling a repository grants posting, and triage runs on no other. - const waits = policyKinds(read.pack, read.policyWorkflow?.decide ?? [], source).filter((kind) => !postingKinds(source).includes(kind)); + const waits = policyKinds(read.pack, read.policyWorkflow?.decide ?? [], source).filter((kind) => !postingKinds(source, read.pack.labels).includes(kind)); return waits.filter((kind) => !read.automatic.includes(kind)); } diff --git a/apps/web/src/components/repo-panel/PolicyRules.tsx b/apps/web/src/components/repo-panel/PolicyRules.tsx index cebcffe41..e327a0c8f 100644 --- a/apps/web/src/components/repo-panel/PolicyRules.tsx +++ b/apps/web/src/components/repo-panel/PolicyRules.tsx @@ -47,7 +47,7 @@ const WAITS = " (waits for you on Triage)"; * labels, so a label waits there only beside a waiting comment. */ function postText(pack: CheckPack, verdict: VerdictWord, kind: ActionKind, allowed: (kind: ActionKind) => boolean): string { - if (kind === "labels") { + if (kind === "labels" && pack.labels) { const label = `Label “${pack.labels[verdict]}”`; if (allowed("labels")) return label; return allowed("comment") ? `${label} (not added: Triage may not add labels here)` : `${label}${WAITS}`; @@ -90,7 +90,7 @@ export default function PolicyRules({ pack, source, automatic }: { pack: CheckPa
{VERDICT_NAMES[verdict]}
- {postingKinds(source).map((kind) => {postText(pack, verdict, kind, allowed)})} + {postingKinds(source, pack.labels).map((kind) => {postText(pack, verdict, kind, allowed)})} {pack.actions[verdict].filter((step) => step.do !== "nothing").map((step, at) => {doText(step)}{forText(step)}{stepWaits(verdict, step) ? WAITS : ""})}
diff --git a/packages/triage-contracts/src/check-pack.ts b/packages/triage-contracts/src/check-pack.ts index 87c1c12bb..983270325 100644 --- a/packages/triage-contracts/src/check-pack.ts +++ b/packages/triage-contracts/src/check-pack.ts @@ -403,7 +403,9 @@ export type PolicyEvent = (typeof POLICY_EVENTS)[number]; export const VERDICTS = ["pass", "fail", "unsure"] as const; export type VerdictWord = (typeof VERDICTS)[number]; -export const VERDICT_LABELS: Record = { pass: "Triage: Ready", fail: "Triage: Not Ready", unsure: "Triage: Needs Human Review" }; +export type VerdictLabels = Record; + +export const VERDICT_LABELS: VerdictLabels = { pass: "Triage: Ready", fail: "Triage: Not Ready", unsure: "Triage: Needs Human Review" }; /** What a head's verdict still waits for. */ export type Awaits = "ci" | "judge" | "code-review"; @@ -442,8 +444,8 @@ export type CheckPack = { checks: Partial>; custom: CustomCheck[]; actions: PolicyActions; - /** So tools watching GitHub can react to it. */ - labels: Record; + /** So tools watching GitHub can react to it; absent, Triage writes no labels. */ + labels?: VerdictLabels; /** The groups the policy file defines, in its order, which steps name as `{ group }`. */ groups?: Record; /** What reading the stored policy left out because it no longer runs; each verdict says so. Never read back. */ @@ -583,7 +585,7 @@ export const checkPackSchema = type({ "checks?": CatalogChecksSchema, "custom?": CustomCheckSchema.array().atMostLength(CUSTOM_CHECK_CAP), "actions?": PolicyActionsSchema, - "labels?": { "+": "reject", "pass?": Label, "fail?": Label, "unsure?": Label }, + "labels?": type("true").or({ "+": "reject", "pass?": Label, "fail?": Label, "unsure?": Label }), "groups?": FileGroupsSchema, }); @@ -1158,8 +1160,8 @@ function parseGroups(raw: unknown): Record | undefined { return groups; } -function parseLabels(raw: unknown): Record { - if (raw === undefined) return { ...VERDICT_LABELS }; +function parseLabels(raw: unknown): VerdictLabels | undefined { + if (raw === undefined || 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(", ")}.`); @@ -1295,7 +1297,7 @@ export function readCheckPack(raw: unknown, expectedRepo?: string, refuse = fals const actions = parseActions(body.actions, dropped, refuse); const labels = parseLabels(body.labels); const groups = parseGroups(body.groups); - return withDropped(refuseTriageLabels({ kind: CHECK_PACK_KIND, schemaVersion: CHECK_PACK_SCHEMA_VERSION, repo, checks, custom, actions, labels, ...(groups && { groups }) }), dropped); + return withDropped(refuseTriageLabels({ kind: CHECK_PACK_KIND, schemaVersion: CHECK_PACK_SCHEMA_VERSION, repo, checks, custom, actions, ...(labels && { labels }), ...(groups && { groups }) }), dropped); } export type SharedPolicy = Omit; @@ -1304,7 +1306,8 @@ export type PolicyOverride = Pick & { extends: string; checks: Partial>>; actions: Partial; - labels: Partial>; + /** `true` turns the verdict labels on with their default names. */ + labels: Partial | true; }; export function extendsOf(raw: unknown): string | undefined { @@ -1323,7 +1326,7 @@ export function readSharedPolicy(raw: unknown): SharedPolicy { const custom = parseCustom(body.custom); const actions = parseActions(body.actions, dropped, false); const labels = parseLabels(body.labels); - return withDropped({ kind: CHECK_PACK_KIND, schemaVersion: CHECK_PACK_SCHEMA_VERSION, checks, custom, actions, labels }, dropped); + return withDropped({ kind: CHECK_PACK_KIND, schemaVersion: CHECK_PACK_SCHEMA_VERSION, checks, custom, actions, ...(labels && { labels }) }, dropped); } /** Built-in rule rows as a repository sets them: `enabled` only where the row names it, so a limit alone keeps the shared switch. */ @@ -1348,8 +1351,9 @@ function parseOverrideActions(row: Partial return actions; } -function parseOverrideLabels(row: Partial> | undefined): PolicyOverride["labels"] { - const labels: PolicyOverride["labels"] = {}; +function parseOverrideLabels(row: true | Partial> | undefined): PolicyOverride["labels"] { + if (row === true) return true; + const labels: Partial = {}; for (const verdict of VERDICTS) { if (row?.[verdict] !== undefined) labels[verdict] = text(row[verdict], `labels ${verdict}`, LABEL_MAX); } @@ -1436,7 +1440,7 @@ function levelLabelRule(label: string): LevelLabelRule | undefined { * step naming a level label would flip it on and off across runs. Sensitive paths and risk add their own level labels. */ export function refuseTriageLabels(pack: CheckPack): CheckPack { - const verdictLabels = Object.values(pack.labels).map((label) => label.toLowerCase()); + const verdictLabels = Object.values(pack.labels ?? {}).map((label) => label.toLowerCase()); for (const [id, label] of labelRules(pack)) { const lower = label.toLowerCase(); const level = levelLabelRule(lower); diff --git a/packages/triage-contracts/src/do.ts b/packages/triage-contracts/src/do.ts index c0ccd99ba..d74742188 100644 --- a/packages/triage-contracts/src/do.ts +++ b/packages/triage-contracts/src/do.ts @@ -1,5 +1,5 @@ import { type } from "arktype"; -import { ACTION_KINDS, VERDICTS, type ActionKind, type CheckPack, type VerdictWord } from "./check-pack.js"; +import { ACTION_KINDS, VERDICTS, type ActionKind, type CheckPack, type VerdictLabels, type VerdictWord } from "./check-pack.js"; import { kindsOf, type DecideRule } from "./policy-workflow.js"; import type { RepoSource } from "./repo-source.js"; @@ -41,9 +41,9 @@ export function alwaysHeld(list: VerdictWord, kind: ActionKind): boolean { return (list === "unsure" && kind === "close") || (list !== "pass" && kind === "merge"); } -/** What Triage posts with every verdict: a comment, and a label where the host has labels. */ -export function postingKinds(source: RepoSource): ActionKind[] { - return hostKinds(source).filter((kind) => kind === "comment" || kind === "labels"); +/** What Triage posts with every verdict: a comment, and a label where the host has labels and the policy turns them on. */ +export function postingKinds(source: RepoSource, labels: VerdictLabels | undefined): ActionKind[] { + return hostKinds(source).filter((kind) => kind === "comment" || (kind === "labels" && labels !== undefined)); } /** @@ -52,7 +52,7 @@ export function postingKinds(source: RepoSource): ActionKind[] { */ export function policyKinds(pack: CheckPack, decide: readonly DecideRule[], source: RepoSource): ActionKind[] { const steps = VERDICTS.flatMap((verdict) => pack.actions[verdict].filter((step) => step.do === "nothing" || !alwaysHeld(verdict, step.do)).map((step) => step.do)); - const used = new Set([...postingKinds(source), ...decide.flatMap((rule) => kindsOf(rule.do)), ...steps]); + const used = new Set([...postingKinds(source, pack.labels), ...decide.flatMap((rule) => kindsOf(rule.do)), ...steps]); return hostKinds(source).filter((kind) => used.has(kind)); } diff --git a/packages/triage-contracts/src/policy-export.ts b/packages/triage-contracts/src/policy-export.ts index 682ca7118..3c6f1d1d8 100644 --- a/packages/triage-contracts/src/policy-export.ts +++ b/packages/triage-contracts/src/policy-export.ts @@ -111,8 +111,9 @@ function fileActions(pack: CheckPack): Record | undefined { return Object.keys(actions).length ? actions : undefined; } -function fileLabels(pack: CheckPack): Record | undefined { - const changed = VERDICTS.flatMap((verdict) => (pack.labels[verdict] === VERDICT_LABELS[verdict] ? [] : [[verdict, pack.labels[verdict]]])); +function fileLabels({ labels }: CheckPack): Record | 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; } diff --git a/packages/triage-contracts/src/policy-file-problems.ts b/packages/triage-contracts/src/policy-file-problems.ts index fdeb28c6b..efec47c00 100644 --- a/packages/triage-contracts/src/policy-file-problems.ts +++ b/packages/triage-contracts/src/policy-file-problems.ts @@ -44,7 +44,7 @@ const EXPECTED: ReadonlyArray = { @@ -30,8 +30,8 @@ const 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[] { - return [...new Set([...TRIAGE_LABELS, ...Object.values(labels)])]; +export function ownedLabels(labels: VerdictLabels | undefined): string[] { + return [...new Set([...TRIAGE_LABELS, ...Object.values(labels ?? {})])]; } export interface Rendered { diff --git a/packages/triage-contracts/src/resolve-policy.ts b/packages/triage-contracts/src/resolve-policy.ts index 2cb249e69..a36b47136 100644 --- a/packages/triage-contracts/src/resolve-policy.ts +++ b/packages/triage-contracts/src/resolve-policy.ts @@ -5,11 +5,13 @@ import { readPolicyOverride, readSharedPolicy, refuseTriageLabels, + VERDICT_LABELS, type CatalogCheck, type CatalogId, type CheckPack, type PolicyOverride, type SharedPolicy, + type VerdictLabels, } from "./check-pack.js"; /** `sha` is the file's blob on the default branch. */ @@ -61,14 +63,17 @@ export function resolvePolicy(shared: SharedPolicy, repo: PolicyOverride): Check fail: repo.actions.fail ?? shared.actions.fail, unsure: repo.actions.unsure ?? shared.actions.unsure, }, - labels: { - pass: repo.labels.pass ?? shared.labels.pass, - fail: repo.labels.fail ?? shared.labels.fail, - unsure: repo.labels.unsure ?? shared.labels.unsure, - }, + ...resolvedLabels(repo.labels, shared.labels), }); } +/** The repository's own `labels` key wins, each name it leaves out taken from the shared policy or the default; without one the shared policy decides. */ +function resolvedLabels(repo: PolicyOverride["labels"], shared: VerdictLabels | undefined): { labels?: VerdictLabels } { + if (repo === true) return { labels: { ...VERDICT_LABELS } }; + if (Object.keys(repo).length === 0) return shared ? { labels: shared } : {}; + return { labels: { ...VERDICT_LABELS, ...shared, ...repo } }; +} + /** A policy file's pack over the policy file of the repository its `extends` names, which must extend nothing itself. */ export function extendedFilePack(repo: string, content: unknown, shared: { repo: string; policy: Record }): CheckPack { if (shared.policy.extends !== undefined) throw new Error(`${shared.repo}'s policy extends another, so no policy can extend it.`); diff --git a/packages/triage-workflows/src/actions/evaluate.ts b/packages/triage-workflows/src/actions/evaluate.ts index 017caff81..ea435196e 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 } from "@corbits/triage-contracts"; +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 { 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"; @@ -155,19 +155,25 @@ export function withoutDos(feedback: string, actions: SuggestedAction[]): string * verdict label on GitHub, an error status on Bitbucket Data Center. None without a known head, so a newer head is * never touched, nor where triage does not write the verdict. */ -function degradedMark({ repo, number, headSha }: PrFacts, source: RepoSource, automatic: ActionKind[], owned: readonly string[], appLogin: string | undefined): WaitingMark | undefined { +function degradedMark({ repo, number, headSha }: PrFacts, source: RepoSource, automatic: ActionKind[], labels: VerdictLabels | undefined, owned: readonly string[], appLogin: string | undefined): WaitingMark | undefined { if (headSha === "" || !marksWaiting(source, automatic)) return undefined; const pull = { repo, number, onHead: headSha, ...(appLogin && { appLogin }) }; const args = source === "bitbucket-datacenter" ? { ...pull, labels: [], owned: [], status: UNREAD_STATUS, statusHead: headSha } - : { ...pull, labels: [FAILED_LABEL], owned }; + : { ...pull, labels: labels ? [FAILED_LABEL] : [], owned }; 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; +} + function degradedOf(it: Item, reason: string): Verdict { - const verdict = { ...stamp(it, null), ...degradedVerdict(reason), automatic: it.automatic, actions: [] }; + const labels = verdictLabelsOf(it); + const verdict = { ...stamp(it, null), ...degradedVerdict(reason, labels), automatic: it.automatic, actions: [] }; const request = toMirrorRequest(verdict, it.pack && ownedLabels(it.pack.labels)); - const mark = degradedMark(it.facts, repoSourceOf(it.repoSource), verdict.automatic, request.owned, it.appLogin); + const mark = degradedMark(it.facts, repoSourceOf(it.repoSource), verdict.automatic, labels, request.owned, it.appLogin); return { ...verdict, request, ...(mark && { mark }) }; } @@ -205,14 +211,12 @@ function verdictFrom(it: Item, rendered: RenderOutput): Verdict { const shown = it.blocksOn ? rendered.feedback : withDos(rendered.feedback, actions); // What reading the policy left out is said on every verdict, so the policy can be fixed. const reasons = [...rendered.reasons, ...(it.pack?.dropped ?? [])]; - const labels = it.pack && !rendered.waiting ? [it.pack.labels[rendered.verdict]] : rendered.labels; const owned = it.pack ? ownedLabels(it.pack.labels) : undefined; const 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, it.appLogin); + const mark = closed ? closedMark(source, it.facts, automatic, verdictLabelsOf(it), 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, + ...stamp(it, it.facts.headSha), ...rendered, reasons, feedback: shown, automatic, actions, ...(it.pack && packApproves(it.pack) && { approvable: held === undefined }), ...(wanted !== undefined && { changesWanted: wanted }), ...(mark && { mark }), ...(closed && { closed: true as const }), }; @@ -261,7 +265,7 @@ function verdictOf(it: Item, replies: unknown[], partReplies: unknown[]): Verdic const answers = { decisions, malformed: false }; const answered = choiceOver(verdictAnswer, VERDICT_CHOICES); const rendered = renderVerdict({ - author: it.facts.author, repo: it.facts.repo, repoSource: repoSourceOf(it.repoSource), headSha: it.facts.headSha, det: it.det, answers, plan, focused, stack: it.facts.stack, judgeError, + author: it.facts.author, repo: it.facts.repo, repoSource: repoSourceOf(it.repoSource), headSha: it.facts.headSha, det: it.det, labels: verdictLabelsOf(it), answers, plan, focused, stack: it.facts.stack, judgeError, judgeLimitExceeded: it.judgeLimitExceeded, judgeSkipped: it.judgeSkipped, reviewers: it.facts.reviewers, choice: answered, ...(choice && "judgeError" in choice && { choiceRefused: choice.judgeError }), ...(hostAwaited(it) !== undefined && { hostAwaited: hostAwaited(it) }), }); diff --git a/packages/triage-workflows/src/actions/waiting.ts b/packages/triage-workflows/src/actions/waiting.ts index 2e8c1badf..be9b4a431 100644 --- a/packages/triage-workflows/src/actions/waiting.ts +++ b/packages/triage-workflows/src/actions/waiting.ts @@ -1,5 +1,5 @@ import type { EffectContext } from "@intx/workflow"; -import { ownedLabels, parseCheckPack, repoSourceOf, trustedRequestOf, WAITING_STATUS, type Awaits, type KeptVerdict, type RepoSource, type VerdictWord } from "@corbits/triage-contracts"; +import { ownedLabels, parseCheckPack, repoSourceOf, trustedRequestOf, WAITING_STATUS, type Awaits, type KeptVerdict, type RepoSource, type VerdictLabels } from "@corbits/triage-contracts"; import { progressComment, type WaitingMark } from "../logic/render.js"; type Head = { repo: string; number: number; headSha: string }; @@ -26,10 +26,10 @@ export function waitingMark(source: RepoSource, { repo, number, headSha }: Head, * 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 }: Head & { 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: VerdictLabels | undefined, appLogin: string | undefined): WaitingMark | undefined { const comment = automatic.includes("comment"); if (!comment) return undefined; - const owned = source === "github" && automatic.includes("labels") ? ownedLabels(labels) : []; + const owned = source === "github" && automatic.includes("labels") && 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 }; } @@ -43,7 +43,7 @@ export async function unready(input: unknown, _ctx: EffectContext, _signal: Abor 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) || !automatic.includes("labels") || !pack) return { unready: true }; + if (typeof repo !== "string" || typeof prNumber !== "number" || typeof headSha !== "string" || !Array.isArray(automatic) || !automatic.includes("labels") || !pack?.labels) return { unready: true }; const appLogin = typeof request["appLogin"] === "string" ? request["appLogin"] : undefined; 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/duplicates.test.ts b/packages/triage-workflows/src/logic/duplicates.test.ts index e0d94c02b..dba285fdb 100644 --- a/packages/triage-workflows/src/logic/duplicates.test.ts +++ b/packages/triage-workflows/src/logic/duplicates.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import type { ToolCall, ToolResult } from "@intx/types/runtime"; import type { EffectContext } from "@intx/workflow"; -import { readCheckPack, readPolicyFile } from "@corbits/triage-contracts"; +import { readCheckPack, readPolicyFile, VERDICT_LABELS } from "@corbits/triage-contracts"; import { readFacts } from "../actions/facts.js"; import { rules } from "../actions/index.js"; import { deriveState, type PrFacts } from "./checks.js"; @@ -111,7 +111,7 @@ describe("duplicates-open-pull-request", () => { return JSON.stringify({ id, type: "score", score: 1, legend: levels, probabilities: Object.fromEntries(probabilities.map((p, index) => [String(index), p])), confidence: 0.9 }); } const answers = parseAnswers([score("duplicates-open-pull-request-001", [0.2, 0.5, 0.3]), score("duplicates-open-pull-request-002", [0.8, 0.15, 0.05]), JSON.stringify({ id: "instructed", type: "noul", noul: 0.1 })].join("")); - const verdict = renderVerdict({ repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc", det, answers, plan }); + const verdict = renderVerdict({ labels: VERDICT_LABELS, repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc", det, answers, plan }); expect(verdict.checks.find((check) => check.check === "duplicates-open-pull-request")).toMatchObject({ result: "fail", reason: "looks like the same change as #13", onFail: "ask", output: { kind: "level", level: "same change" } }); expect(verdict.verdict).toBe("unsure"); expect(verdict.feedback).toContain("Asking a maintainer about:\n- looks like the same change as #13"); diff --git a/packages/triage-workflows/src/logic/render.test.ts b/packages/triage-workflows/src/logic/render.test.ts index 2a4793817..5bb59affa 100644 --- a/packages/triage-workflows/src/logic/render.test.ts +++ b/packages/triage-workflows/src/logic/render.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { classificationSources, emptyPack, type CheckPack } from "@corbits/triage-contracts"; +import { classificationSources, emptyPack, VERDICT_LABELS, type CheckPack } from "@corbits/triage-contracts"; import { MAX_MIRROR_COMMENT_BYTES, parseAnswers, renderVerdict } from "./render.js"; import type { DeterministicResult } from "./checks.js"; @@ -9,7 +9,7 @@ describe("cited evidence", () => { const failing = { check: "custom-1", kind: "machine", result: "fail", reason: "breaks the rule", evidence: ["src/a.ts:3"], output: { kind: "score", score: 0, passAt: 70, items: [citation] } } as const; const det: DeterministicResult = { state: "needs-author-update", reason: "a rule failed", findings: [], checks: [failing], needsJudgment: false, sources: classificationSources(emptyPack("acme/widgets")) }; function feedback(repoSource: "github" | "bitbucket-datacenter"): string { - return renderVerdict({ repoSource, author: "octocat", repo: "acme/widgets", headSha: "abc123", det }).feedback; + return renderVerdict({ labels: VERDICT_LABELS, repoSource, author: "octocat", repo: "acme/widgets", headSha: "abc123", det }).feedback; } expect(feedback("github")).toContain("[`src/a.ts:3`](https://github.com/acme/widgets/blob/abc123/src/a.ts?plain=1#L3)"); expect(feedback("bitbucket-datacenter")).toContain("breaks the rule: `src/a.ts:3`"); @@ -25,7 +25,7 @@ describe("a long comment", () => { test("an over-long comment is cut at line boundaries and still posts", () => { const custom = Array.from({ length: 200 }, (_, index) => ({ id: `custom-${index + 1}`, name: `${"N".repeat(500)}-${index}`, group: "code-vs-ci" as const, kind: "model" as const, shape: "is-true" as const, claim: "Keeps boundaries" })); const det: DeterministicResult = { state: "ready-monitoring", reason: "all configured checks pass", findings: [], checks: [], needsJudgment: true, sources: { quality: [], custom, rules: [] } }; - const verdict = renderVerdict({ repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc123", det, answers: noul(Object.fromEntries(custom.map((row) => [row.id, 0.1]))), choice: { pass: 0.1, fail: 0.85, cant_tell: 0.05 } }); + const verdict = renderVerdict({ labels: VERDICT_LABELS, repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc123", det, answers: noul(Object.fromEntries(custom.map((row) => [row.id, 0.1]))), choice: { pass: 0.1, fail: 0.85, cant_tell: 0.05 } }); const lines = verdict.feedback.split("\n").slice(3); expect(new TextEncoder().encode(verdict.feedback).byteLength).toBeLessThanOrEqual(MAX_MIRROR_COMMENT_BYTES); expect(lines[0]).toContain(custom[0]!.name); @@ -55,7 +55,7 @@ describe("score and choose rules", () => { test("a score rule records its level, never fails without failBelow, and fails when the level is likely below it", () => { const kind = JSON.stringify({ id: "kind", type: "choice", choice: "chore", probabilities: { fix: 0.1, feature: 0.1, chore: 0.8 }, confidence: 0.9 }); const answers = parseAnswers([score("style", [0.9, 0.1, 0, 0]), score("care", [0.5, 0.3, 0.2, 0]), kind].join("\n")); - const checks = renderVerdict({ repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc123", det: scored, answers }).checks; + const checks = renderVerdict({ labels: VERDICT_LABELS, repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc123", det: scored, answers }).checks; expect(checks.map((c) => [c.check, c.result, c.evidence])).toEqual([["style", "pass", ["poor"]], ["care", "fail", ["poor"]], ["kind", "fail", ["chore"]]]); expect(checks[1]!.output).toEqual({ kind: "level", level: "poor", levels, failBelow: "good" }); }); @@ -75,6 +75,7 @@ describe("author text in the comment", () => { { check: "focused", kind: "model" as const, result: "fail" as const, reason: "unrelated changes", evidence: hostile }, ]; const verdict = renderVerdict({ + labels: VERDICT_LABELS, repoSource: "github", author: "octocat", repo: "acme/widgets", diff --git a/packages/triage-workflows/src/logic/render.ts b/packages/triage-workflows/src/logic/render.ts index 5100284ff..f7fe05caf 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, 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, 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 VerdictLabels, 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"; @@ -112,6 +112,8 @@ export interface RenderInput { repoSource: RepoSource; headSha: string; det: DeterministicResult; + /** The verdict label names; absent, the verdict carries no label. */ + labels: VerdictLabels | undefined; answers?: ParsedAnswers | Record | null; plan?: QualityPlan; focused?: FocusedPlan; @@ -251,7 +253,7 @@ function band(noul: number, yesPasses: boolean): Decided { return { result: "unconfirmed", reason: UNDECIDED_REASON }; } -export function evaluateModel({ det, answers, plan = {}, focused, stack, judgeError, judgeLimitExceeded, judgeSkipped, choice }: Omit): ModelEvaluation { +export function evaluateModel({ det, answers, plan = {}, focused, stack, judgeError, judgeLimitExceeded, judgeSkipped, choice }: Omit): ModelEvaluation { if (!det.sources) return { checks: [], unanswered: false, instructed: false }; const sources = det.sources; // A failed chunk leaves only its own questions unanswered; the other chunks' answers still count. @@ -734,6 +736,7 @@ interface RenderContext { /** Set only on a stacked branch: the pull requests merged into it, and the open one it is based on. */ stack?: Stack; labelled: CheckResult[]; + labels: VerdictLabels | undefined; } function withChecks(rendered: VerdictBase, checks: CheckResult[], score: number | null, reasons: string[], ctx: RenderContext): RenderOutput { @@ -741,7 +744,7 @@ function withChecks(rendered: VerdictBase, checks: CheckResult[], score: number ...rendered, ...nextStep(checks, ctx.sources, ctx.reviewers, rendered.verdict), mirror: rendered.state !== "stale-unknown", - labels: [VERDICT_LABELS[rendered.verdict]], + labels: ctx.labels ? [ctx.labels[rendered.verdict]] : [], score, reasons, feedback: authorComment(ctx, checks, rendered.verdict), @@ -782,7 +785,7 @@ export function renderVerdict(input: RenderInput): RenderOutput { const awaited = input.hostAwaited === undefined ? undefined : HOST_AWAITED_TEXT[input.hostAwaited]; const evaluated = evaluateModel(input); const { deciding, labelled } = splitLabelled(det, evaluated.checks); - const ctx = { author, repo, repoSource, headSha, sources: det.sources, reviewers, purpose: input.focused?.purpose, stack: input.stack, labelled }; + const ctx = { author, repo, repoSource, headSha, sources: det.sources, reviewers, purpose: input.focused?.purpose, stack: input.stack, labelled, labels: input.labels }; const checks = [...det.checks, ...deciding]; const flaggedReason = checks.filter(isFlagged).map((check) => reasonOf(check, det.sources)).join("; "); const reasons = reasonsOf(det, deciding); @@ -825,11 +828,11 @@ export function renderVerdict(input: RenderInput): RenderOutput { return withChecks({ ...render(VERDICT_STATES[decided]), verdict: decided, degraded: null, reason: DECIDED_REASONS[decided] }, checks, choice.pass, reasons, ctx); } -export function degradedVerdict(reason: string): RenderOutput { +export function degradedVerdict(reason: string, labels: VerdictLabels | undefined): RenderOutput { return { ...render("stale-unknown"), mirror: false, - labels: [VERDICT_LABELS.unsure], + labels: labels ? [labels.unsure] : [], verdict: "unsure", score: null, reasons: ["Could not evaluate"], diff --git a/packages/triage-workflows/src/logic/request-changes.test.ts b/packages/triage-workflows/src/logic/request-changes.test.ts index 5aa610546..16008c6f1 100644 --- a/packages/triage-workflows/src/logic/request-changes.test.ts +++ b/packages/triage-workflows/src/logic/request-changes.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { getReviews, REQUEST_CHANGES_DO_MARKER, type GithubFetch } from "@corbits/github-reads"; -import { DEFAULT_REPO_POLICY, defaultActions, emptyPack, type CheckPack } from "@corbits/triage-contracts"; +import { DEFAULT_REPO_POLICY, defaultActions, emptyPack, VERDICT_LABELS, type CheckPack } from "@corbits/triage-contracts"; import { approvalHeldBy, evaluateActions, heldApprovals } from "./actions.js"; import { deriveState, type PrFacts } from "./checks.js"; import { buildFacts } from "./facts.js"; @@ -37,7 +37,7 @@ const pack: CheckPack = { function input(over: Partial, using = pack) { const pr = { ...facts, ...over }; const det = deriveState(pr, undefined, using); - const rendered = renderVerdict({ repoSource: "github", author: pr.author, repo: pr.repo, headSha: pr.headSha, det, answers: null, choice: { pass: 0.9, fail: 0.05, cant_tell: 0.05 } }); + const rendered = renderVerdict({ labels: VERDICT_LABELS, repoSource: "github", author: pr.author, repo: pr.repo, headSha: pr.headSha, det, answers: null, choice: { pass: 0.9, fail: 0.05, cant_tell: 0.05 } }); return { rendered, input: { facts: pr, checks: rendered.checks, verdict: rendered.verdict, reason: rendered.reason, pack: using } }; } diff --git a/tooling/facts.ts b/tooling/facts.ts index 2329f4978..78e89f50a 100644 --- a/tooling/facts.ts +++ b/tooling/facts.ts @@ -2,6 +2,7 @@ import { deriveState, type PrFacts } from "../packages/triage-workflows/src/logi import { prepareQualityEvaluation } from "../packages/triage-workflows/src/logic/quality.js"; import { renderVerdict, type RenderOutput } from "../packages/triage-workflows/src/logic/render.js"; import { NO_FACTS } from "../packages/triage-workflows/src/logic/item.js"; +import { VERDICT_LABELS } from "../packages/triage-contracts/src/check-pack.js"; import type { PRRecord } from "../packages/triage-contracts/src/types.js"; function toFacts(pr: PRRecord): PrFacts { @@ -25,6 +26,6 @@ export function triage(pr: PRRecord): RenderOutput { const facts = toFacts(pr); const det = deriveState(facts); const { focused } = prepareQualityEvaluation(facts, det.sources); - const v = renderVerdict({ author: pr.author, repo: facts.repo, repoSource: "github", headSha: facts.headSha, det, focused }); + const v = renderVerdict({ author: pr.author, repo: facts.repo, repoSource: "github", headSha: facts.headSha, det, labels: VERDICT_LABELS, focused }); return { ...v, humanGated: v.humanGated || pr.spam >= 0.9 || pr.confidence < 0.5 }; } diff --git a/tooling/preview.ts b/tooling/preview.ts index 69d926f6a..6fc7b90b5 100644 --- a/tooling/preview.ts +++ b/tooling/preview.ts @@ -86,7 +86,7 @@ if (import.meta.main) { const facts = factsFor(number); const det = deriveState(facts, undefined, pack); const evaluation = previewEvaluation(facts, det); - const v = renderVerdict({ author: facts.author, repo: facts.repo, repoSource: "github", headSha: facts.headSha, det, plan: evaluation.plan, focused: evaluation.focused, reviewers: facts.reviewers, ...(await previewJudge(det, evaluation)) }); + const v = renderVerdict({ author: facts.author, repo: facts.repo, repoSource: "github", headSha: facts.headSha, det, labels: pack.labels, plan: evaluation.plan, focused: evaluation.focused, reviewers: facts.reviewers, ...(await previewJudge(det, evaluation)) }); rows.push({ pr: `#${number}`, title: facts.title.slice(0, 40),