From 3cc22e32fe68c79f29c24983a9e2fcec8bfe0bf5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 26 Oct 2026 14:47:21 -0700 Subject: [PATCH] feat(rules): add sensitive-paths and risk-level rules --- .../src/github/policy-file.postgres.test.ts | 4 +- apps/hub/src/github/policy-file.ts | 2 +- docs/ARCHITECTURE.md | 4 +- .../Corbits Triage User Manual.md | 43 +++++-- packages/policy-check-action/src/cli.test.ts | 2 + packages/policy-check-action/src/local-git.ts | 4 +- packages/triage-contracts/src/check-pack.ts | 110 +++++++++++++++++- .../triage-contracts/src/code-patterns.ts | 3 + .../triage-contracts/src/policy-export.ts | 8 +- .../triage-contracts/src/policy-rules.test.ts | 16 +++ packages/triage-contracts/src/policy-rules.ts | 12 +- packages/triage-contracts/src/rule-prose.ts | 11 +- packages/triage-workflows/evals/risk.eval.ts | 4 +- .../triage-workflows/src/actions/evaluate.ts | 8 +- .../triage-workflows/src/actions/facts.ts | 2 +- .../triage-workflows/src/logic/actions.ts | 27 ++++- .../triage-workflows/src/logic/checks.test.ts | 70 +++++++++++ packages/triage-workflows/src/logic/checks.ts | 105 ++++++++++++++++- .../triage-workflows/src/logic/patterns.ts | 7 +- packages/triage-workflows/src/logic/render.ts | 28 ++++- packages/triage-workflows/src/logic/risk.ts | 61 +++++++++- .../triage-workflows/src/pr-triage.test.ts | 35 ++++++ .../triage-workflows/src/triage-steps.test.ts | 3 +- skills/corbits-triage-setup/SKILL.md | 2 +- skills/corbits-triage-setup/reference.md | 10 ++ 25 files changed, 539 insertions(+), 42 deletions(-) diff --git a/apps/hub/src/github/policy-file.postgres.test.ts b/apps/hub/src/github/policy-file.postgres.test.ts index 86ea0f671..9dad1f98c 100644 --- a/apps/hub/src/github/policy-file.postgres.test.ts +++ b/apps/hub/src/github/policy-file.postgres.test.ts @@ -350,13 +350,13 @@ test("PostgreSQL: a file already holding all a file can of the saved rules is no const saved: CheckPack = { ...emptyPack(FROM_FILE), checks: { "ci-green": { enabled: true } }, - custom: [{ id: "risk", name: "Risk", group: "pull-request", kind: "model", shape: "score", subject: "the risk", levels: ["high", "low"] }], + custom: [{ id: "risk-score", name: "Risk score", group: "pull-request", kind: "model", shape: "score", subject: "the risk", levels: ["high", "low"] }], }; await writePolicy(FROM_FILE, JSON.stringify(saved)); onMain(FROM_FILE, YML, exportPolicyFile(saved, false).text); await policyFile.onPush(TENANT, FROM_FILE, "github"); expect(await readPack(FROM_FILE)).toMatchObject({ status: "ok", source: { repo: { version: 1 } } }); - expect(await upgradeOf(FROM_FILE)).toMatchObject({ upgrade: { state: "differs", differences: [{ rule: "Risk", file: [] }], fixable: false } }); + expect(await upgradeOf(FROM_FILE)).toMatchObject({ upgrade: { state: "differs", differences: [{ rule: "Risk score", file: [] }], fixable: false } }); expect(await exportOf(FROM_FILE)).toMatchObject({ http: 409, error: { code: "policy_file_full" } }); }, 60_000); diff --git a/apps/hub/src/github/policy-file.ts b/apps/hub/src/github/policy-file.ts index 22272f06d..fc7db09d1 100644 --- a/apps/hub/src/github/policy-file.ts +++ b/apps/hub/src/github/policy-file.ts @@ -47,7 +47,7 @@ function addBody(lead: string): string { return [lead, "", "Edit it here before merging. Once merged, triage reads its rules from this file, and you change them by editing it."].join("\n"); } -const STARTER_BODY = addBody(`Adds \`${POLICY_FILE_PATH}\` with a few common rules: a size limit, no merge conflicts and passing CI.`); +const STARTER_BODY = addBody(`Adds \`${POLICY_FILE_PATH}\` with a few common rules: a size limit, no merge conflicts, passing CI, and a maintainer asked about sensitive or risky changes.`); const CHOSEN_BODY = addBody(`Adds \`${POLICY_FILE_PATH}\`.`); /** diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index cca04d47e..28ab3f6f4 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -359,13 +359,13 @@ label off that head, so it never stands on a head not yet triaged. details (`/api/integrations/github-pull`) find referenced GitHub issues in the same cleaned title, body and commit messages; a pasted URL of an issue in the same repository counts too, as GitHub renders it `#`. - With the regression-risk rule on, it also lists each changed file's + With the regression-risk or risk rule on, it also lists each changed file's default-branch commits from the last 30 days (`github_list_path_commits`, `bitbucket_datacenter_list_path_commits`, one page each, ten files at a time; tests, docs and lockfiles skipped; at most 300 files per pull request, GitHub's own diff limit) and keeps how many have a subject typed fix, bugfix, hotfix or revert (`fixes`); a history that fails, runs past - one page or is past the 300 is kept as unread, which leaves only that rule + one page or is past the 300 is kept as unread, which leaves only those rules unconfirmed. With the focused rule on, it also reads the stack (`get_stack`): the pull requests merged into the branch, and the open pull request whose branch it 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 3a40dafb8..17d7b6325 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -132,9 +132,9 @@ A few things work differently from GitHub, because Bitbucket works differently: A repository's policy is the file `.corbits-triage.yml` (`.corbits-triage.yaml` works too) on its default branch, so changes to it are reviewed like any other code. The **Policy** section reads it back in plain sentences: each rule that is on, then what Triage does on **Pass**, **Fail** and **Unsure**. Rules and actions change only in the file. **Change on GitHub**, or **Change on Bitbucket** for a Bitbucket repository, opens it there. -- With no file and no rules saved in Triage, the section shows a **Start a policy file** card with the steps **Review**, **Open pull request**, **Merge** (**Merge on Bitbucket** for a Bitbucket repository) and **Done**. It lists the rules the file starts with: at most 30 files and 3,000 lines changed, no merge conflicts and passing CI, with no actions. **Open the pull request** opens one adding `.corbits-triage.yml`, or shows the one already open; change the rules there before you merge. Triage runs nothing on the repository until it is merged. +- With no file and no rules saved in Triage, the section shows a **Start a policy file** card with the steps **Review**, **Open pull request**, **Merge** (**Merge on Bitbucket** for a Bitbucket repository) and **Done**. It lists the rules the file starts with: at most 30 files and 3,000 lines changed, no merge conflicts, passing CI, sensitive paths and risk, with no actions. **Open the pull request** opens one adding `.corbits-triage.yml`, or shows the one already open; change the rules there before you merge. Triage runs nothing on the repository until it is merged. - ![The Policy section for acme/widgets: Start a policy file, Triage runs nothing until .corbits-triage.yml is merged, the steps Review, Open pull request, Merge and Done, the four starter rules with Comment with the verdict and its label on Pass, Fail and Unsure, and Open the pull request; the Repositories list marks acme/widgets Add a policy file](images/39.png) + ![The Policy section for acme/widgets: Start a policy file, Triage runs nothing until .corbits-triage.yml is merged, the steps Review, Open pull request, Merge and Done, the six starter rules with Comment with the verdict and its label on Pass, Fail and Unsure, and Open the pull request; the Repositories list marks acme/widgets Add a policy file](images/39.png) - When a change to the file reaches the default branch, the next triage uses it. Where GitHub or Bitbucket cannot reach Triage to tell it about the change, Triage notices it within a minute. - If the file has a mistake, Triage runs nothing on the repository until it is fixed. The section says so and lists each problem, and the Inbox names the repository. A repository that still has rules saved in Triage keeps running on them instead, and its card below says **Triage can't read** the file and why. @@ -235,7 +235,7 @@ It also runs the rules that need no model, from the policy on the branch the pul #### Check locally -Before pushing, you can run the same rules on your own change. From a clone of this repository, run `packages/policy-check-action/src/cli.ts check` inside the repository you are changing (it needs Bun). It reads the policy on the default branch (`--base ` for another) and checks your change twice: **Committed**, your commits since your branch left it, which is what Triage sees once you push, then **Working tree**, which adds staged and unstaged edits and new files, or says there are no uncommitted changes. Each lists every rule with its result, then each failure. Rules a model answers show as skipped, and so do rules that need the pull request on GitHub or Bitbucket: CI, merge conflicts, linked issue, hidden content, conventional commits when they cover the title, and your own text rules that read the title or description. It never contacts the host and needs no account. It exits with 1 when a rule whose failure means Fail fails in **Committed** (a failure only in the working tree is a warning), 2 when the policy cannot be read or the change cannot be checked, and 0 otherwise. The rules are the same on both hosts, so it works for Bitbucket Data Center repositories too. +Before pushing, you can run the same rules on your own change. From a clone of this repository, run `packages/policy-check-action/src/cli.ts check` inside the repository you are changing (it needs Bun). It reads the policy on the default branch (`--base ` for another) and checks your change twice: **Committed**, your commits since your branch left it, which is what Triage sees once you push, then **Working tree**, which adds staged and unstaged edits and new files, or says there are no uncommitted changes. Each lists every rule with its result, then each failure. Rules a model answers show as skipped, and so do rules that need the pull request on GitHub or Bitbucket: CI, merge conflicts, linked issue, hidden content, regression risk, risk, conventional commits when they cover the title, and your own text rules that read the title or description. It never contacts the host and needs no account. It exits with 1 when a rule whose failure means Fail fails in **Committed** (a failure only in the working tree is a warning), 2 when the policy cannot be read or the change cannot be checked, and 0 otherwise. The rules are the same on both hosts, so it works for Bitbucket Data Center repositories too. #### Browser extension @@ -257,6 +257,8 @@ A rule asks one question about a pull request and answers pass, fail or unconfir - **Flag pull requests that hide text.** Fails when the title, description, a commit message or a file name hides something from a reviewer: two or more invisible characters together, Unicode tag characters other than those that draw the England, Scotland and Wales flags, variation selectors that change nothing a reader sees (one emoji-style selector on its own is allowed), characters that override or isolate text direction, or a link with nothing to click. Right-to-left text and colour codes in pasted terminal output are allowed. On GitHub it also fails when a comment or other hidden markup the author added to the description speaks to an AI, such as "ignore previous instructions", or holds those characters. A comment exactly as the repository's pull request template has it, or, with no template, its organisation's `.github` repository's, is skipped; one the author shortened or added to is checked. When it fails, Triage says what it found and where, such as `commit 2: bidi controls`, never the hidden text. On Bitbucket Data Center only the characters are checked. It knows only common wording: an instruction that is paraphrased, written with look-alike letters or HTML entities, or split across comments passes, and so does a single invisible character between letters. Give it `onFail: ask` so a maintainer decides rather than the author being failed. - **Flag changes to files with N+ fixes in the last 30 days, or to migrations.** Fails when a changed file has had at least N commits in the last 30 days whose title starts with a fix, bugfix, hotfix or revert type, such as `fix(api): …` (4 unless the file sets `hotFileFixes`), or a changed file is a migration (`**/migrations/**` or `**/*.sql` unless the file lists its own `migrations`). Tests, Markdown docs and lockfiles are left out. Its evidence names the hottest three files and how many more, such as `src/router.ts: 6 recent fixes` or `db/migrations/0004.sql: migration`. It only labels and notes the pull request unless the file gives it another `onFail`, so a `decide` rule or a step can hold back an approve or merge on its label. When Triage can't read a file's whole recent history, the rule is unconfirmed rather than passed, and the rest of the pull request is checked as usual. Triage reads the history of at most 300 changed files per pull request, so on a larger one the rule is unconfirmed with **could not read the recent history of some changed files**, unless a changed file is a migration or one it read is hot. The default of 4 was measured on one open-source repository; check it suits yours. - **Flag a pull request with the same changes as another open one.** Fails when an open pull request opened before it on the same base branch, drafts left out, has the same latest commit, the same diff or the same commits, and the comment names it, such as "has the same changes as #1612". The one opened first never fails it. Where a host gives no diff to compare, such as a very large pull request, the commits are compared. One whose diff or commits Triage can't read is compared on what it could read. Triage compares at most 100 earlier open pull requests; with more on the branch, when this pull request's own changes can't be read, or when another's list of changed files can't be read, the rule is unconfirmed rather than passed and says which. + - **Flag changes to sensitive paths.** Checks changed files against the file's named groups of globs, each `high` or `medium`, written as `groups:`, such as `infra: { level: high, paths: ["infra/**", "**/*.tf"] }`. Without `groups` there are four: `schema` (`**/migrations/**`, `**/*.sql`) and `auth` (`**/auth/**`, `**/secrets/**`), which are high, and `ci` (`.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`) and `locks` (`**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`), which are medium, since every dependency bump changes a lockfile. Globs read as in **Never accept changes to**. A change to a group at `failAt` or above, high unless the file says, fails the rule, which asks a maintainer unless the file gives it another `onFail`. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `sensitive: medium` and notes it under **Notes:** (on Bitbucket Data Center, only the note). Its evidence names each group with its files, at most five, such as `schema: db/migrations/0004.sql`, then how many more. Regression risk keeps its own migration globs. + - **Flag high-risk pull requests and label medium-risk ones.** Rates each pull request low, medium or high risk; see [Risk](#risk). - **Fail when …**, a text rule of your own, such as "Fail when any commit message contains "Co-Authored-By:"". It names where to look (the title, the description, any commit message, or several of them), whether the rule fails when the text **contains** or **does not contain** its phrases, and the phrases. Case does not matter. A **contains** rule on the description also looks at its hidden parts, such as HTML comments and the addresses behind links, while **does not contain** reads only what a reader sees. Only the first 100 commits are read. When a **contains** rule fails, Triage names each place and the phrases found there, asks for them to be removed, and quotes each matching line once. Text rules count toward the eight rules of your own. - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When a file can't be read whole or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. - **Judgement**, questions the decision model answers from the diff and the documentation: @@ -269,9 +271,9 @@ Two rules always run: a draft pull request waits until it is ready for review, a Each rule's `onFail` says what happens when it fails: -- `fail`, the default for every rule but the size rules, regression risk and the judged duplicate rule: the verdict is **Fail** and the comment asks the author to fix it. -- `ask`, the default for the judged duplicate rule: the verdict is **Unsure** and the comment lists the rule under **Asking a maintainer about:**, so the pull request waits for you in **Needs you** instead of on its author. -- `label`, the default for the size rules and regression risk: the verdict does not change. When the rule fails, or the decision model can't tell, Triage adds its label to the pull request and a line under **Notes:** in its comment. Triage's comment and the reading pane leave a `label` rule out of their rule list; it shows only as a note and a label (on Bitbucket Data Center, only the note). The browser extension, the **Corbits Triage policy / check** check and the local check still list it. Triage never takes the label off, since a person may have added it; remove it once it no longer applies. The label is `size: large` for the size rules, `docs: check` for the docs rule and the rule's id followed by `: check` for any other, such as `regression-risk: check`; `label:` names a different one, but not one starting `Triage:` or `rule: ` or a verdict label, which Triage takes back itself. `onFail: label` suits rules that are hygiene rather than a gate, such as size limits an atomic change has to cross. A top-level `onFail` sets it for every rule in the file that does not set its own. The Policy section shows a rule that does not fail the pull request with what it does instead, such as "(on fail, labels size: large)". +- `fail`, the default for every rule but the size rules, regression risk, the judged duplicate rule, sensitive paths and risk: the verdict is **Fail** and the comment asks the author to fix it. +- `ask`, the default for the judged duplicate rule, sensitive paths and risk: the verdict is **Unsure** and the comment lists the rule under **Asking a maintainer about:**, so the pull request waits for you in **Needs you** instead of on its author. +- `label`, the default for the size rules and regression risk: the verdict does not change. When the rule fails, or the decision model can't tell, Triage adds its label to the pull request and a line under **Notes:** in its comment. Triage's comment and the reading pane leave a `label` rule out of their rule list; it shows only as a note and a label (on Bitbucket Data Center, only the note). The browser extension, the **Corbits Triage policy / check** check and the local check still list it. Triage never takes the label off, since a person may have added it; remove it once it no longer applies. The exceptions are the level labels of sensitive paths and risk (`sensitive: medium`, `sensitive: high`, `risk: medium` and `risk: high`), which only Triage adds: when the level changes or drops to low, Triage takes the old one off. The label is `size: large` for the size rules, `docs: check` for the docs rule, `sensitive: high` and `risk: high` for sensitive paths and risk, and the rule's id followed by `: check` for any other, such as `regression-risk: check`; `label:` names a different one, but not one starting `Triage:` or `rule: ` or a verdict label, which Triage takes back itself. `onFail: label` suits rules that are hygiene rather than a gate, such as size limits an atomic change has to cross. A top-level `onFail` sets it for every rule in the file that does not set its own. The Policy section shows a rule that does not fail the pull request with what it does instead, such as "(on fail, labels size: large)". To make a size limit block, give it `onFail: fail` or `onFail: ask`: @@ -304,6 +306,33 @@ A code-pattern rule that labels instead of failing: Size rules and facts the host already knows cost nothing and always give the same answer for the same pull request. Judgement rules call the decision model, a few requests per pull request, each billed by your model provider. +#### Risk + +The risk rule counts five signals: + +- `sensitive`: a changed file in a medium sensitive-paths group, the rule's own groups or the four defaults, whether or not that rule is on. While sensitive paths is off, a changed file in a high group makes the pull request high risk on its own; while it is on, a high group is left to sensitive paths, so a migration is asked about once, with one reason. +- `size`: more files or lines changed than the size rules allow, 30 files and 3,000 lines unless the file sets `max-files-changed` or `max-lines-changed`. +- `no-tests`: source files changed and no test file did. Source is code in TypeScript, JavaScript, Go, Python, Rust, Java, Kotlin, Ruby, PHP, C# or Swift, outside vendored, built and installed code, `docs/` and `examples/` folders and type declarations (`*.d.ts`); configuration, data such as JSON, SQL, docs and assets are not. A test is a file under a test folder, `e2e/`, `cypress/` or `__snapshots__/`, or one named as a test, such as `*.test.ts`, `*.cy.js` or `*_test.go`. +- `hot-files`: a changed file with as many recent fixes as regression risk counts, 4 unless it sets `hotFileFixes`. +- `first-time`: an author contributing to the repository for the first time. Bitbucket Data Center does not say, so it never counts there. + +By default any one signal makes a pull request medium risk, and a size limit crossed with no test changed makes it high. At `failAt`, high unless the file says, the rule fails and asks a maintainer unless it has another `onFail`; the comment lists it under **Asking a maintainer about:**, such as **Risk: high (schema paths, 42 files, no tests)**. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `risk: medium` and notes it under **Notes:**, such as **Risk: medium (no tests)** (on Bitbucket Data Center, only the note). A `labelAt` at or above `failAt` never labels. + +`medium:` and `high:` take the number of signals that reach the level, or a list of signals any one of which does; a list inside the list needs all of its signals: + +```yaml + - check: risk + text: Ask about high-risk pull requests and label medium-risk ones. + medium: 2 + high: [hot-files, [size, no-tests]] + failAt: high + labelAt: medium + reason: A hot file or a large untested change needs a maintainer's eye. + evidence: [maintainer-stated] +``` + +When Triage can't read a changed file's recent history and a hot file would make the rule fail, the rule is unconfirmed, which leaves the verdict **Unsure** unless the rule only labels, and the comment notes **Risk could not be checked** with the reason under **Notes:**. A label it could only have added is left off. The defaults have not been measured against any repository's history; check they suit yours. + ### How the verdict is decided 1. If any rule fails, the verdict is **Fail** right away. That includes a Judgement rule the decision model answered no to. A failed rule set to `onFail: ask` makes it **Unsure** instead, unless another rule fails it. @@ -348,7 +377,7 @@ An **Unsure** comment lists each rule Triage could not confirm, with checks stil ### 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 for CI it shows **Triage: Waiting for CI**, and when it could not read the pull request **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` is added whatever the verdict, after the verdict's own steps, as an **Add labels** action: where Add labels is allowed, otherwise waiting for you in the Inbox. +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 for CI it shows **Triage: Waiting for CI**, and when it could not read the pull request **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: where Add labels is allowed, otherwise waiting for you in the Inbox. The same action takes off a level label of sensitive paths or risk that no longer applies. On Bitbucket Data Center labels have no effect; see [Bitbucket Data Center repositories](#bitbucket-data-center-repositories). diff --git a/packages/policy-check-action/src/cli.test.ts b/packages/policy-check-action/src/cli.test.ts index 8ef6f9807..f8c479c88 100644 --- a/packages/policy-check-action/src/cli.test.ts +++ b/packages/policy-check-action/src/cli.test.ts @@ -169,6 +169,8 @@ const EVERY_MACHINE_RULE = `rules: - { check: hidden-content, text: T., ${EVIDENCE} } - { check: regression-risk, text: T., ${EVIDENCE} } - { check: no-duplicate, text: T., ${EVIDENCE} } + - { check: sensitive-paths, text: T., ${EVIDENCE} } + - { check: risk, text: T., ${EVIDENCE} } - { id: conventions, check: code-patterns, text: T., presets: [convention], ${EVIDENCE} } `; diff --git a/packages/policy-check-action/src/local-git.ts b/packages/policy-check-action/src/local-git.ts index 24803613d..cc44b4512 100644 --- a/packages/policy-check-action/src/local-git.ts +++ b/packages/policy-check-action/src/local-git.ts @@ -186,9 +186,9 @@ export function localGithub(change: LocalChange): (url: URL) => Promise (row.kind === "text" && row.fields.some((field) => field !== "commits") ? [row.id] : [])); const titled = (pack.checks["conventional-commits"]?.fields ?? CONVENTIONAL_FIELDS).includes("title") ? ["conventional-commits"] : []; - return ["ci-green", "no-merge-conflicts", "linked-issue", "hidden-content", "regression-risk", "no-duplicate", NOT_A_DRAFT, NO_CHANGES_REQUESTED, ...titled, ...described].map((id) => ({ id, why: ON_HOST })); + return ["ci-green", "no-merge-conflicts", "linked-issue", "hidden-content", "regression-risk", "no-duplicate", "risk", NOT_A_DRAFT, NO_CHANGES_REQUESTED, ...titled, ...described].map((id) => ({ id, why: ON_HOST })); } diff --git a/packages/triage-contracts/src/check-pack.ts b/packages/triage-contracts/src/check-pack.ts index 17b3a388c..1486ff716 100644 --- a/packages/triage-contracts/src/check-pack.ts +++ b/packages/triage-contracts/src/check-pack.ts @@ -30,6 +30,8 @@ export const CATALOG_IDS = [ "regression-risk", "no-duplicate", "duplicates-open-pull-request", + "sensitive-paths", + "risk", ] as const; export type CatalogId = (typeof CATALOG_IDS)[number]; @@ -58,6 +60,8 @@ export const CHECK_CATALOG: Record> = { + schema: { level: "high", paths: ["**/migrations/**", "**/*.sql"] }, + auth: { level: "high", paths: ["**/auth/**", "**/secrets/**"] }, + ci: { level: "medium", paths: [".github/**", "**/Dockerfile", "**/railway.json", "**/railway.toml"] }, + locks: { level: "medium", paths: ["**/bun.lock", "**/package-lock.json", "**/yarn.lock", "**/pnpm-lock.yaml"] }, +}; +/** The label each of the sensitive-paths and risk rules adds at a level, which Triage takes back once the level changes. */ +export const LEVEL_LABELS = { "sensitive-paths": "sensitive", risk: "risk" } as const; +export type LevelLabelRule = keyof typeof LEVEL_LABELS; + +export function levelLabel(id: LevelLabelRule, level: RiskMark): string { + return `${LEVEL_LABELS[id]}: ${level}`; +} + +export function isLevelLabelRule(id: string): id is LevelLabelRule { + return Object.hasOwn(LEVEL_LABELS, id); +} + +/** Where the sensitive-paths and risk rules fail, and from where they label below that, when the policy does not say. */ +export const DEFAULT_MARKS: { failAt: RiskMark; labelAt: RiskMark } = { failAt: "high", labelAt: "medium" }; +/** `sensitive` is a change to a medium sensitive group; one to a high group makes the pull request high risk on its own. */ +export const RISK_SIGNALS = ["sensitive", "size", "no-tests", "hot-files", "first-time"] as const; +export type RiskSignal = (typeof RISK_SIGNALS)[number]; +export const RISK_LEVELS = ["low", "medium", "high"] as const; +export type RiskLevel = (typeof RISK_LEVELS)[number]; +/** How many signals reach a level, or the signals any one of which reaches it; a list among them reaches it only with all of its signals. */ +export type RiskThreshold = number | Array; +export type RiskLevels = { medium: RiskThreshold; high: RiskThreshold; failAt: RiskMark; labelAt: RiskMark }; +/** The risk rule's defaults: any one signal is medium; a size limit crossed with no test changed is high. */ +export const DEFAULT_RISK_LEVELS: RiskLevels = { medium: 1, high: [["size", "no-tests"]], ...DEFAULT_MARKS }; export const RECOMMENDED_ISSUE = { tracker: "either" } as const; export const CONVENTIONAL_TYPES = ["feat", "fix", "refactor", "perf", "docs", "test", "chore", "ci", "build", "style", "revert"] as const; export const CONVENTIONAL_FIELDS = ["title", "commits"] as const; @@ -168,6 +211,16 @@ export type CatalogCheck = RuleNote & OnFail & { /** Regression risk: fixes on one changed file in recent history that make it risky, and the paths that are migrations; absent is `DEFAULT_RISK`'s. */ hotFileFixes?: number; migrationGlobs?: string[]; + /** Sensitive paths: named groups of globs, each with its level; absent is `DEFAULT_SENSITIVE`. */ + sensitiveGroups?: Record; + /** + * Risk: what reaches medium and high; sensitive paths and risk: the level each fails at, and the level from which one + * below that is labelled. Absent is `DEFAULT_RISK_LEVELS`'. + */ + medium?: RiskThreshold; + high?: RiskThreshold; + failAt?: RiskMark; + labelAt?: RiskMark; }; /** Each size rule and the field its limit is stored in. */ @@ -184,10 +237,16 @@ export function isSizeRule(id: string): id is SizeRuleId { return Object.hasOwn(SIZE_RULES, id); } -/** What a policy file's rule does on failure when the file does not say: features and refactors cross size limits and touch risky code, so those only label. */ +const ASKING_RULES: ReadonlySet = new Set(["sensitive-paths", "risk"]); + +/** + * What a policy file's rule does on failure when the file does not say: features and refactors cross size limits and + * touch risky code, so those only label, and a sensitive or risky change is the author's to make but a maintainer's to accept. + */ export function defaultOnFail(id: string): (typeof ON_FAILS)[number] { // A judged duplicate may only overlap, so a maintainer decides. if (id === "duplicates-open-pull-request") return "ask"; + if (ASKING_RULES.has(id)) return "ask"; return isSizeRule(id) || id === "regression-risk" ? "label" : "fail"; } @@ -481,6 +540,11 @@ const CatalogCheckSchema = type({ "fields?": type.enumerated(...CONVENTIONAL_FIELDS).array(), "hotFileFixes?": "number", "migrationGlobs?": "string[]", + "sensitiveGroups?": type({ "[string]": { level: type.enumerated(...RISK_MARKS), paths: "string[]" } }), + "medium?": "number | (string | string[])[]", + "high?": "number | (string | string[])[]", + "failAt?": type.enumerated(...RISK_MARKS), + "labelAt?": type.enumerated(...RISK_MARKS), }); export const CustomId = type(CUSTOM_ID).atMostLength(CUSTOM_ID_MAX); @@ -610,6 +674,8 @@ export function recommendedPack(repo: string): CheckPack { "no-merge-conflicts": { enabled: true }, "ci-green": { enabled: true }, "no-forbidden-paths": { enabled: true, forbiddenGlobs: [...RECOMMENDED_PATHS.forbiddenGlobs] }, + "sensitive-paths": { enabled: true, onFail: "ask" }, + risk: { enabled: true, onFail: "ask" }, }, custom: [{ ...RECOMMENDED_TEXT, fields: [...RECOMMENDED_TEXT.fields], phrases: [...RECOMMENDED_TEXT.phrases] }], actions: { ...defaultActions(), pass: [{ do: "approve" }] }, @@ -621,6 +687,8 @@ const STARTER_REASONS = { "max-lines-changed": "Large pull requests are hard to review.", "no-merge-conflicts": "A pull request with conflicts can't be merged as it is.", "ci-green": "A change is not ready until CI passes.", + "sensitive-paths": "Schema, auth, CI and lockfile changes reach beyond what the diff shows.", + risk: "A risky change needs a maintainer's eye.", } as const satisfies Partial>; /** @@ -687,6 +755,11 @@ function parseCatalogCheck(id: CatalogId, raw: unknown): CatalogCheck | undefine const globs = parseGlobs(row.migrationGlobs, "Regression risk migration globs"); if (globs) check.migrationGlobs = globs; } + if (id === "sensitive-paths") { + if (row.sensitiveGroups !== undefined) check.sensitiveGroups = sensitiveGroups(row.sensitiveGroups); + Object.assign(check, marks(row, "Sensitive paths")); + } + if (id === "risk") Object.assign(check, riskLevels(row), marks(row, "Risk")); if (id === "conventional-commits") { if (row.types !== undefined) check.types = commitTypes(row.types); if (row.fields !== undefined) check.fields = conventionalFields(row.fields); @@ -694,6 +767,40 @@ function parseCatalogCheck(id: CatalogId, raw: unknown): CatalogCheck | undefine return check; } +function sensitiveGroups(value: unknown): Record { + const where = "Sensitive paths groups"; + const named = Object.entries(record(value, where)); + if (named.length === 0 || named.length > LIST_MAX) throw new Error(`${where} must name 1 to ${LIST_MAX} groups.`); + return Object.fromEntries(named.map(function group([name, raw]) { + if (!GroupName.allows(name)) throw new Error(`${where} names must be lowercase words joined by hyphens, not ${refusalNames([name])}.`); + const row = record(raw, `${where} ${name}`); + exactKeys(row, ["level", "paths"], `${where} ${name}`); + return [name, { level: oneOf(row.level, RISK_MARKS, `${where} ${name} level`), paths: globs(row.paths, `${where} ${name} paths`) }]; + })); +} + +function marks(row: Record, where: string): Pick { + return { + ...(row.failAt !== undefined && { failAt: oneOf(row.failAt, RISK_MARKS, `${where} failAt`) }), + ...(row.labelAt !== undefined && { labelAt: oneOf(row.labelAt, RISK_MARKS, `${where} labelAt`) }), + }; +} + +function riskThreshold(value: unknown, where: string): RiskThreshold { + if (typeof value === "number") { + if (!Number.isSafeInteger(value) || value < 1 || value > RISK_SIGNALS.length) throw new Error(`${where} must be a number of signals from 1 to ${RISK_SIGNALS.length}, or a list of signals.`); + return value; + } + return list(value, where).map((entry) => (Array.isArray(entry) ? [...new Set(list(entry, where).map((signal) => oneOf(signal, RISK_SIGNALS, where)))] : oneOf(entry, RISK_SIGNALS, where))); +} + +function riskLevels(row: Record): Pick { + return { + ...(row.medium !== undefined && { medium: riskThreshold(row.medium, "Risk medium") }), + ...(row.high !== undefined && { high: riskThreshold(row.high, "Risk high") }), + }; +} + function commitTypes(value: unknown): string[] { const types = texts(value, "Conventional commits types"); const bad = types.find((name) => !CONVENTIONAL_TYPE.test(name)); @@ -785,6 +892,7 @@ function parseNote(row: Record, where: string): RuleNote { function labelOf(id: string, row: OnFail): string { if (row.label !== undefined) return row.label; if (isSizeRule(id)) return "size: large"; + if (isLevelLabelRule(id)) return levelLabel(id, "high"); return id === "docs" ? "docs: check" : `${id}: check`; } diff --git a/packages/triage-contracts/src/code-patterns.ts b/packages/triage-contracts/src/code-patterns.ts index 4a0048c8a..4708e74bc 100644 --- a/packages/triage-contracts/src/code-patterns.ts +++ b/packages/triage-contracts/src/code-patterns.ts @@ -2,6 +2,9 @@ import picomatch from "picomatch"; export const PATTERN_LANGUAGES = ["typescript", "javascript"] as const; export type PatternLanguage = (typeof PATTERN_LANGUAGES)[number]; +/** The file extensions code-pattern rules read. */ +export const PATTERN_EXTENSIONS = ["ts", "mts", "cts", "tsx", "js", "mjs", "cjs", "jsx"] as const; +export type PatternExtension = (typeof PATTERN_EXTENSIONS)[number]; /** An ast-grep rule: `rule`, `files` and `ignores` are as an ast-grep rule config holds them, globs as strings only. */ export interface PatternRule { diff --git a/packages/triage-contracts/src/policy-export.ts b/packages/triage-contracts/src/policy-export.ts index 40868df5b..05479587c 100644 --- a/packages/triage-contracts/src/policy-export.ts +++ b/packages/triage-contracts/src/policy-export.ts @@ -54,6 +54,10 @@ function catalogParams(id: CatalogId, row: CatalogCheck): Record { @@ -239,6 +243,8 @@ function catalogLimits(pack: CheckPack, id: CatalogId): string[] { return row.forbiddenGlobs ?? []; case "conventional-commits": case "regression-risk": + case "sensitive-paths": + case "risk": return [catalogSentence(id, pack)]; default: return ["on"]; diff --git a/packages/triage-contracts/src/policy-rules.test.ts b/packages/triage-contracts/src/policy-rules.test.ts index e5734c421..c14c8d329 100644 --- a/packages/triage-contracts/src/policy-rules.test.ts +++ b/packages/triage-contracts/src/policy-rules.test.ts @@ -132,4 +132,20 @@ actions: expect(checkPolicyFile(`rules:\n${rule} label: docs\n`, REPO)).toEqual({ ok: false, problems: [{ message: "Rule docs label needs onFail: label.", line: 2 }] }); expect(checkPolicyFile(`rules:\n${rule} onFail: label\n label: "rule: docs"\n`, REPO)).toEqual({ ok: false, problems: [{ message: "Rule docs label rule: docs is one Triage takes back; name another." }] }); }); + + test("sensitive-paths reads its groups and risk its levels, both ask unless the file says, survive export, and a bad group name, signal or count is refused", () => { + const sensitive = " - check: sensitive-paths\n text: Sensitive paths need a maintainer.\n reason: Infra reaches production.\n evidence: [maintainer-stated]\n"; + const risk = " - check: risk\n text: Risky pull requests need a maintainer.\n reason: Risky changes regressed.\n evidence: [maintainer-stated]\n"; + const pack = readPolicyFile(`rules:\n${sensitive} groups: { infra: { level: high, paths: ["infra/**", "**/*.tf"] } }\n labelAt: high\n${risk} medium: 2\n high: [hot-files, [size, no-tests]]\n failAt: medium\n labelAt: high\n`, REPO) as CheckPack; + expect(pack.checks).toMatchObject({ + "sensitive-paths": { sensitiveGroups: { infra: { level: "high", paths: ["infra/**", "**/*.tf"] } }, labelAt: "high", onFail: "ask" }, + risk: { medium: 2, high: ["hot-files", ["size", "no-tests"]], failAt: "medium", labelAt: "high", onFail: "ask" }, + }); + expect(readPolicyFile(exportPolicyFile(pack, false).text, REPO)).toEqual(pack); + expect(askRules(readPolicyFile(`rules:\n${sensitive}${risk} onFail: fail\n`, REPO) as CheckPack)).toEqual(["sensitive-paths"]); + expect(() => readPolicyFile(`rules:\n${sensitive} groups: { Infra: { level: high, paths: ["infra/**"] } }\n`, REPO)).toThrow("lowercase words joined by hyphens"); + expect(checkPolicyFile(`rules:\n${sensitive} groups: { infra: ["infra/**"] }\n`, REPO)).toMatchObject({ ok: false }); + expect(checkPolicyFile(`rules:\n${risk} high: [hot-files, tired]\n`, REPO)).toMatchObject({ ok: false }); + expect(() => readPolicyFile(`rules:\n${risk} medium: 6\n`, REPO)).toThrow("Risk medium must be a number of signals from 1 to 5"); + }); }); diff --git a/packages/triage-contracts/src/policy-rules.ts b/packages/triage-contracts/src/policy-rules.ts index d8519a1ba..41a780f97 100644 --- a/packages/triage-contracts/src/policy-rules.ts +++ b/packages/triage-contracts/src/policy-rules.ts @@ -3,7 +3,7 @@ // mapping it onto the stored policy's shape, so the stored policy's reader stays the one authority on what a policy may hold. import { type } from "arktype"; import { asRecord } from "@corbits/common"; -import { CATALOG_IDS, CONVENTIONAL_FIELDS, CONVENTIONAL_TYPE, CustomId, defaultOnFail, Evidence, ISSUE_TRACKERS, Label, ON_FAILS, PatternRuleSchema, POLICY_EVENTS, SIZE_RULES, stepListSchema, Text, TEXT_FIELDS, TEXT_MATCHES, VERDICTS, type CatalogId, type SizeRuleId } from "./check-pack.js"; +import { CATALOG_IDS, CONVENTIONAL_FIELDS, CONVENTIONAL_TYPE, CustomId, defaultOnFail, Evidence, ISSUE_TRACKERS, Label, ON_FAILS, PatternRuleSchema, POLICY_EVENTS, RISK_MARKS, RISK_SIGNALS, SIZE_RULES, stepListSchema, Text, TEXT_FIELDS, TEXT_MATCHES, VERDICTS, type CatalogId, type SizeRuleId } from "./check-pack.js"; import { PATTERN_PRESETS } from "./code-patterns.js"; import { fileProblems } from "./policy-file-problems.js"; import { withAuthors, WORKFLOW_KEYS, WORKFLOW_PROPS, workflowProblems, type PathProblem, type PolicyWorkflow } from "./policy-workflow.js"; @@ -13,6 +13,10 @@ const Limit = type("number.integer").atLeast(1); const OnFailWord = type.enumerated(...ON_FAILS); +const RiskSignal = type.enumerated(...RISK_SIGNALS); +const RiskThreshold = Limit.or(RiskSignal.or(RiskSignal.array().atLeastLength(1)).array().atLeastLength(1)); +const RiskMark = type.enumerated(...RISK_MARKS); + /** A built-in rule's id is its check; a model or text rule names its own. */ const RULE_BASE = { "+": "reject", text: Text, reason: Text, evidence: Evidence, "enabled?": "boolean", "onFail?": OnFailWord, "label?": Label } as const; const OWN_RULE = { ...RULE_BASE, id: CustomId } as const; @@ -25,6 +29,8 @@ const RuleSchema = type({ ...RULE_BASE, check: type.enumerated(...SIZE_RULE_IDS) .or({ ...RULE_BASE, check: "'linked-issue'", "tracker?": type.enumerated(...ISSUE_TRACKERS.filter((tracker) => tracker !== "off")) }) .or({ ...RULE_BASE, check: "'no-forbidden-paths'", paths: Lines }) .or({ ...RULE_BASE, check: "'regression-risk'", "hotFileFixes?": Limit, "migrations?": Lines }) + .or({ ...RULE_BASE, check: "'sensitive-paths'", "groups?": type({ "[string]": { "+": "reject", level: RiskMark, paths: Lines } }), "failAt?": RiskMark, "labelAt?": RiskMark }) + .or({ ...RULE_BASE, check: "'risk'", "medium?": RiskThreshold, "high?": RiskThreshold, "failAt?": RiskMark, "labelAt?": RiskMark }) .or({ ...RULE_BASE, check: "'conventional-commits'", "types?": type(CONVENTIONAL_TYPE).describe("a single word").array().atLeastLength(1).atMostLength(50), "fields?": type.enumerated(...CONVENTIONAL_FIELDS).array().atLeastLength(1) }) .or({ ...RULE_BASE, check: type.enumerated(...SIMPLE_CHECKS) }) .or({ ...OWN_RULE, check: "'model'" }) @@ -88,6 +94,10 @@ function catalogRow(rule: Extract, onFail: Rec return { ...row, forbiddenGlobs: rule.paths }; case "regression-risk": return { ...row, ...(rule.hotFileFixes !== undefined && { hotFileFixes: rule.hotFileFixes }), ...(rule.migrations && { migrationGlobs: rule.migrations }) }; + case "sensitive-paths": + return { ...row, ...(rule.groups && { sensitiveGroups: rule.groups }), ...(rule.failAt && { failAt: rule.failAt }), ...(rule.labelAt && { labelAt: rule.labelAt }) }; + case "risk": + return { ...row, ...(rule.medium !== undefined && { medium: rule.medium }), ...(rule.high !== undefined && { high: rule.high }), ...(rule.failAt && { failAt: rule.failAt }), ...(rule.labelAt && { labelAt: rule.labelAt }) }; case "conventional-commits": return { ...row, ...(rule.types && { types: rule.types }), ...(rule.fields && { fields: rule.fields }) }; default: diff --git a/packages/triage-contracts/src/rule-prose.ts b/packages/triage-contracts/src/rule-prose.ts index 17f3115cc..a37698fde 100644 --- a/packages/triage-contracts/src/rule-prose.ts +++ b/packages/triage-contracts/src/rule-prose.ts @@ -1,5 +1,5 @@ // Plain-text words for a policy's rules. -import { CONVENTIONAL_FIELDS, DEFAULT_RISK, RECOMMENDED_ISSUE, type CatalogCheck, type CatalogId, type CheckPack, type CustomCheck, type IssueTracker, type TextCheck, type TextField, type TextMatch } from "./check-pack.js"; +import { CONVENTIONAL_FIELDS, DEFAULT_MARKS, DEFAULT_RISK, DEFAULT_SENSITIVE, RECOMMENDED_ISSUE, RISK_MARKS, type CatalogCheck, type CatalogId, type CheckPack, type CustomCheck, type IssueTracker, type TextCheck, type TextField, type TextMatch } from "./check-pack.js"; export const ISSUE_TRACKER_NAMES: Record = { github: "GitHub", linear: "Linear", either: "GitHub or Linear", off: "Not required" }; @@ -9,6 +9,13 @@ function count(value: number | undefined): string { const CONVENTIONAL_SUBJECTS = { title: "a conventional commit title", commits: "conventional commit messages", both: "a conventional commit title and messages" } as const; +function riskSentence(row: CatalogCheck | undefined): string { + const failAt = row?.failAt ?? DEFAULT_MARKS.failAt; + const labelAt = row?.labelAt ?? DEFAULT_MARKS.labelAt; + const labels = RISK_MARKS.indexOf(labelAt) < RISK_MARKS.indexOf(failAt) ? ` and label ${labelAt}-risk ones` : ""; + return `Flag ${failAt}-risk pull requests${labels}`; +} + function conventionalSentence(row: CatalogCheck | undefined): string { const fields = row?.fields ?? CONVENTIONAL_FIELDS; const subject = CONVENTIONAL_SUBJECTS[fields.length === 1 ? fields[0]! : "both"]; @@ -28,6 +35,8 @@ const CATALOG_SENTENCES: Record st "conventional-commits": conventionalSentence, "hidden-content": () => "Flag pull requests that hide text", "regression-risk": (row) => `Flag changes to files with ${row?.hotFileFixes ?? DEFAULT_RISK.hotFileFixes}+ fixes in the last ${DEFAULT_RISK.hotFileDays} days, or to migrations`, + "sensitive-paths": (row) => `Flag changes to sensitive paths: ${Object.keys(row?.sensitiveGroups ?? DEFAULT_SENSITIVE).join(", ")}`, + risk: riskSentence, focused: () => "Check the change is focused on one thing", docs: () => "Check docs are updated when behavior changes", "no-duplicate": () => "Flag a pull request with the same changes as another open one", diff --git a/packages/triage-workflows/evals/risk.eval.ts b/packages/triage-workflows/evals/risk.eval.ts index 2c60e6f37..2e0a726ca 100644 --- a/packages/triage-workflows/evals/risk.eval.ts +++ b/packages/triage-workflows/evals/risk.eval.ts @@ -7,7 +7,7 @@ // // bun run eval:risk (in packages/triage-workflows) import { existsSync } from "node:fs"; -import { fixCount, historyPaths, isFix, migrationPaths } from "../src/logic/risk.js"; +import { fixCount, historyPaths, isFix, matchingPaths } from "../src/logic/risk.js"; import { DEFAULT_RISK } from "../../triage-contracts/src/check-pack.js"; const REPOS: Record = { @@ -148,7 +148,7 @@ for (const [repo, head] of Object.entries(REPOS)) { return { hot: hottest(dir, change), fanIn: fanIn(dir, change), - migration: migrationPaths(change.paths, DEFAULT_RISK.migrationGlobs).length > 0, + migration: matchingPaths(change.paths, DEFAULT_RISK.migrationGlobs).length > 0, regressed: hit.has(change.sha), }; }); diff --git a/packages/triage-workflows/src/actions/evaluate.ts b/packages/triage-workflows/src/actions/evaluate.ts index fc408a01a..d6b5ccb39 100644 --- a/packages/triage-workflows/src/actions/evaluate.ts +++ b/packages/triage-workflows/src/actions/evaluate.ts @@ -166,10 +166,16 @@ function degradedOf(it: Item, reason: string): Verdict { type Rendered = Pick; +/** Each labelling rule's result, and each rule that did not fail but still labels. */ +function labelling(rendered: Rendered): CheckResult[] { + const marked = rendered.checks.filter((c) => c.label !== undefined); + return rendered.labelled ? [...rendered.labelled, ...marked] : marked; +} + function actionInputOf(it: Item, rendered: Rendered): ActionInput | undefined { if (!it.pack) return undefined; // Bitbucket Data Center has no labels, so there a labelling rule is only noted. - const labelled = hostKinds(it.repoSource ?? "github").includes("labels") ? rendered.labelled : undefined; + const labelled = hostKinds(it.repoSource ?? "github").includes("labels") ? labelling(rendered) : undefined; return { facts: it.facts, checks: rendered.checks, verdict: rendered.verdict, reason: rendered.reason, pack: it.pack, labelled }; } diff --git a/packages/triage-workflows/src/actions/facts.ts b/packages/triage-workflows/src/actions/facts.ts index 90e81d9ba..aafc6e714 100644 --- a/packages/triage-workflows/src/actions/facts.ts +++ b/packages/triage-workflows/src/actions/facts.ts @@ -237,7 +237,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number } const teams = namedTeams(pack, repo); const focused = catalogCheckEnabled(pack, "focused"); - const risk = catalogCheckEnabled(pack, "regression-risk"); + const risk = catalogCheckEnabled(pack, "regression-risk") || catalogCheckEnabled(pack, "risk"); const listed = new Map(); const filesErrors = new Map(); diff --git a/packages/triage-workflows/src/logic/actions.ts b/packages/triage-workflows/src/logic/actions.ts index 5aee3dc03..5c99455d7 100644 --- a/packages/triage-workflows/src/logic/actions.ts +++ b/packages/triage-workflows/src/logic/actions.ts @@ -1,7 +1,7 @@ import { sha256 } from "@noble/hashes/sha2.js"; import { bytesToHex, utf8ToBytes } from "@noble/hashes/utils.js"; import { ciRefusal, mergeableCi, missingFrom } from "@corbits/github-reads"; -import { alwaysHeld, authorGroupsOf, CATALOG_IDS, checkName, classificationSources, labelRules, ruleLabel, whenHolds, type ActionKind, type CheckPack, type ResolvedTarget, type CiState, type Step, type StepSubject, teamSlug, type VerdictWord, VERDICTS } from "@corbits/triage-contracts"; +import { alwaysHeld, authorGroupsOf, CATALOG_IDS, catalogCheckEnabled, checkName, classificationSources, labelRules, LEVEL_LABELS, levelLabel, RISK_MARKS, ruleLabel, whenHolds, type ActionKind, type CheckPack, type ResolvedTarget, type CiState, type Step, type StepSubject, teamSlug, type VerdictWord, VERDICTS } from "@corbits/triage-contracts"; import { issueReference, type CheckResult, type PrFacts } from "./checks.js"; import { authorFailures, isLabelledFinding } from "./render.js"; import { compare, errorText } from "@corbits/common"; @@ -42,7 +42,7 @@ export interface ActionInput { verdict: VerdictWord; reason: string; pack: CheckPack; - /** Each labelling rule's result, where the host has labels. */ + /** Each labelling rule's result, and each rule that did not fail but still labels, where the host has labels. */ labelled?: CheckResult[]; } @@ -179,14 +179,29 @@ function stepSubject(facts: PrFacts, pack: CheckPack): StepSubject { }; } +/** The label a result adds: its own, or else its labelling rule's when that rule did not pass. */ +function addedLabels(c: CheckResult, labels: ReadonlyMap): string[] { + if (c.label !== undefined) return [c.label]; + const label = labels.get(c.check); + return label !== undefined && isLabelledFinding(c) ? [label] : []; +} + +/** The level labels of the switched-on sensitive-paths and risk rules, which only Triage adds. */ +function levelLabels(pack: CheckPack): Set { + const ids = (Object.keys(LEVEL_LABELS) as Array).filter((id) => catalogCheckEnabled(pack, id)); + return new Set(ids.flatMap((id) => RISK_MARKS.map((level) => levelLabel(id, level).toLowerCase()))); +} + /** - * After the verdict's own steps, one labels Do adds the label of each labelling rule that did not pass. Nothing says who - * put a label on, so one is never taken back: it may be a person's. + * After the verdict's own steps, one labels Do adds the label of each labelling rule that did not pass, and each label a + * rule that did not fail still adds. Nothing says who put a label on, so one is never taken back, as it may be a + * person's, except a level label of sensitive paths or risk that no longer applies. */ function labelDo({ labelled, pack, facts, verdict: list }: ActionInput, reason: string): SuggestedAction[] { - if (!labelled?.length) return []; + if (labelled === undefined) return []; const labels = labelRules(pack); - const target = { labels: [...new Set(labelled.filter(isLabelledFinding).flatMap((c) => labels.get(c.check) ?? []))] }; + const owned = levelLabels(pack); + const target = derivedLabels([...new Set(labelled.flatMap((c) => addedLabels(c, labels)))], (label) => owned.has(label.toLowerCase()), facts); if (satisfied("labels", target, facts)) return []; const index = pack.actions[list].length; const effectId = doEffectId({ repo: facts.repo, number: facts.number, headSha: facts.headSha, list, index, kind: "labels", target }); diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index a835f7866..9d216ea9b 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -134,6 +134,76 @@ describe("deriveState check pack", () => { }); }); +describe("sensitive paths and risk", () => { + const open: PrFacts = { ...noisy, draft: false, mergeable: true, checks: "success", fixes: {} }; + const pack = readCheckPack({ ...emptyPack("acme/widgets"), checks: { "sensitive-paths": { enabled: true, onFail: "ask" }, risk: { enabled: true, onFail: "ask" } } }); + + function changed(paths: string[], facts: Partial = {}): PrFacts { + return { ...open, changedFiles: paths.length, files: paths.map((path) => ({ path, additions: 1, deletions: 0 })), paths, additions: paths.length, ...facts }; + } + + const riskOnly = readCheckPack({ ...emptyPack("acme/widgets"), checks: { risk: { enabled: true, onFail: "ask" } } }); + + function rule(facts: PrFacts, id: string, policy = pack) { + return deriveState(facts, undefined, policy).checks.find((check) => check.check === id); + } + + test("sensitive paths asks a maintainer about a high group, labels a medium one, names each group's files up to five, and reads the policy's own groups", () => { + expect(rule(changed(["db/migrations/0001.sql", "bun.lock", "src/a.ts"]), "sensitive-paths")).toMatchObject({ result: "fail", onFail: "ask", reason: "changes sensitive paths", evidence: ["schema: db/migrations/0001.sql", "locks: bun.lock"] }); + expect(rule(changed(["bun.lock", ".github/workflows/ci.yml"]), "sensitive-paths")).toMatchObject({ result: "pass", label: "sensitive: medium", evidence: ["ci: .github/workflows/ci.yml", "locks: bun.lock"] }); + expect(rule(changed(["src/a.ts", "README.md"]), "sensitive-paths")).toMatchObject({ result: "pass", evidence: [] }); + const own = readCheckPack({ ...pack, checks: { "sensitive-paths": { enabled: true, onFail: "ask", sensitiveGroups: { infra: { level: "high", paths: ["infra/**"] } } } } }); + const infra = Array.from({ length: 7 }, (_, at) => `infra/${at}.tf`); + expect(rule(changed([...infra, "db/migrations/0001.sql"]), "sensitive-paths", own)?.evidence).toEqual([...infra.slice(0, 5).map((path) => `infra: ${path}`), "and 2 more"]); + }); + + test.each([ + ["no signal is low", changed(["src/a.ts", "src/a.test.ts"]), "pass", "low", []], + ["source without tests is medium", changed(["src/a.ts"]), "pass", "medium", ["no tests"]], + ["a first-time contributor is medium", changed(["src/a.ts", "src/a.test.ts"], { authorKind: "FIRST_TIME_CONTRIBUTOR" }), "pass", "medium", ["first-time contributor"]], + ["a recently fixed file is medium", changed(["src/a.ts", "src/a.test.ts"], { fixes: { "src/a.ts": 4 } }), "pass", "medium", ["1 recently fixed file"]], + ["a size limit alone is medium", changed(["src/a.ts", "src/a.test.ts"], { additions: 3001 }), "pass", "medium", ["3001 lines"]], + ["a high sensitive group is high", changed(["db/schema.sql", "src/a.test.ts"]), "fail", "high", ["schema paths"]], + ["a medium sensitive group is medium", changed(["bun.lock", "src/a.ts", "src/a.test.ts"]), "pass", "medium", ["locks paths"]], + ["a medium sensitive group and another signal stay medium", changed(["bun.lock", "src/a.ts"]), "pass", "medium", ["locks paths", "no tests"]], + ["a size limit with no tests is high", changed(Array.from({ length: 42 }, (_, at) => `src/${at}.ts`)), "fail", "high", ["42 files", "no tests"]], + ["changed translations are not source", changed(["locales/en.json"]), "pass", "low", []], + ["changed reStructuredText docs are not source", changed(["docs/index.rst", "LICENSE"]), "pass", "low", []], + ["a CI-only change is its group's level and needs no tests", changed([".github/workflows/ci.yml"]), "pass", "medium", ["ci paths"]], + ["end-to-end tests count as tests", changed(["src/login.ts", "e2e/login.ts"]), "pass", "low", []], + ["Cypress tests and snapshots count as tests", changed(["app/login.py", "cypress/login.cy.js", "__snapshots__/login.snap"]), "pass", "low", []], + ["type declarations, docs and examples are not source", changed(["types/index.d.ts", "docs/guide.ts", "examples/demo.py"]), "pass", "low", []], + ["Go source without tests is medium", changed(["cmd/main.go", "config.yaml"]), "pass", "medium", ["no tests"]], + ])("%s", (_, facts, result, level, evidence) => { + expect(rule(facts, "risk", riskOnly)).toMatchObject({ result, evidence, output: { values: { level } } }); + }); + + test("with sensitive paths on, a high group is asked about once, by that rule", () => { + const migration = changed(["db/migrations/0001.sql", "src/a.test.ts"]); + expect(rule(migration, "sensitive-paths")).toMatchObject({ result: "fail", onFail: "ask" }); + expect(rule(migration, "risk")).toMatchObject({ result: "pass", evidence: [], output: { values: { level: "low" } } }); + expect(rule(changed(["bun.lock", "src/a.test.ts"]), "risk")).toMatchObject({ result: "pass", evidence: ["locks paths"], label: "risk: medium" }); + }); + + test("risk labels at its labelAt level below failAt, and the policy's thresholds move both", () => { + expect(rule(changed(["src/a.ts"]), "risk")?.label).toBe("risk: medium"); + expect(rule(changed(["src/a.ts", "src/a.test.ts"]), "risk")?.label).toBeUndefined(); + const strict = readCheckPack({ ...pack, checks: { risk: { enabled: true, onFail: "ask", medium: 2, high: ["first-time"], failAt: "medium" } } }); + expect(rule(changed(["src/a.ts"]), "risk", strict)).toMatchObject({ result: "pass", output: { values: { level: "low" } } }); + expect(rule(changed(["src/a.ts"], { fixes: { "src/a.ts": 9 } }), "risk", strict)).toMatchObject({ result: "fail", reason: "risk is medium" }); + expect(rule(changed(["src/a.ts", "src/a.test.ts"], { authorKind: "FIRST_TIME_CONTRIBUTOR" }), "risk", strict)).toMatchObject({ result: "fail", reason: "risk is high" }); + }); + + test("history it could not read leaves risk unconfirmed only when a hot file would make it fail", () => { + const unread = changed(["src/a.ts", "src/a.test.ts"], { fixes: { "src/a.ts": null } }); + expect(rule(unread, "risk")).toMatchObject({ result: "pass", output: { values: { level: "low" } } }); + expect(rule(unread, "risk")?.label).toBeUndefined(); + const failsAtMedium = readCheckPack({ ...pack, checks: { risk: { enabled: true, onFail: "ask", failAt: "medium" } } }); + expect(rule(unread, "risk", failsAtMedium)).toMatchObject({ result: "unconfirmed", reason: "could not read the recent history of some changed files" }); + expect(rule(changed(["src/a.ts"], { fixes: { "src/a.ts": null } }), "risk", failsAtMedium)).toMatchObject({ result: "fail", reason: "risk is medium" }); + }); +}); + describe("deriveState check output", () => { test("each fact is named for what it states, so pending CI never reads as passing", () => { const pack = recommendedPack("acme/widgets"); diff --git a/packages/triage-workflows/src/logic/checks.ts b/packages/triage-workflows/src/logic/checks.ts index e7f994d77..fd61e697c 100644 --- a/packages/triage-workflows/src/logic/checks.ts +++ b/packages/triage-workflows/src/logic/checks.ts @@ -1,11 +1,15 @@ import { askRules, + AUTHOR_GROUPS, catalogCheckEnabled, CLOSED_FINDING, classificationSources, CONVENTIONAL_FIELDS, CONVENTIONAL_TYPES, DEFAULT_RISK, + DEFAULT_MARKS, + DEFAULT_RISK_LEVELS, + DEFAULT_SENSITIVE, customCheckEnabled, hardFailures, ISSUE_LINKS, @@ -13,9 +17,11 @@ import { isPatternsCheck, isTextCheck, labelRules, + levelLabel, NO_CHANGES_REQUESTED, NOT_A_DRAFT, readCheckPack, + RECOMMENDED_SIZE, repoPolicy, SIZE_RULES, type AuthorKind, @@ -24,6 +30,9 @@ import { type CheckPack, type ConventionalField, type IssueTracker, + type RiskLevels, + type RiskSignal, + type SensitiveGroup, type SizeRuleId, type TextCustomCheck, type TriageEvent, @@ -35,7 +44,7 @@ import type { Stack } from "@corbits/github-reads"; import { globMatch } from "./glob.js"; import { duplicateResult, type OpenPulls } from "./duplicates.js"; import type { PatternsResult } from "./patterns.js"; -import { hotFiles, migrationPaths } from "./risk.js"; +import { hotFiles, matchingPaths, riskLevel, riskOutcome, sensitiveHits, sensitiveLevel, untested, type SensitiveHit } from "./risk.js"; export interface PrFileFacts { path: string; @@ -96,7 +105,7 @@ export interface PrFacts { stack?: Stack; /** What the title, description, commits and paths hid, as `where: what`, never the hidden text. */ hidden: string[]; - /** Set only when regression risk is on: the fix commits in each changed file's recent history, by path, or null when that history could not be read in full. */ + /** Set only when regression risk or risk is on: the fix commits in each changed file's recent history, by path, or null when that history could not be read in full. */ fixes?: Record; /** Each code-patterns rule's matches on the added lines, by rule id; matched where the files were read. */ patterns?: Record; @@ -124,6 +133,8 @@ export interface CheckResult { onFail?: "ask"; /** The evidence lines naming files the rule skipped rather than checked. */ skipped?: string[]; + /** The label a rule that did not fail still adds: sensitive paths or risk below its `failAt` and at or above its `labelAt`. */ + label?: string; } export interface DeterministicResult { @@ -419,6 +430,11 @@ interface CheckRules { tracker: IssueTracker; conventional?: ConventionalRule; risk: { hotFileFixes: number; migrationGlobs: readonly string[] }; + sensitiveGroups: Readonly>; + sensitiveMarks: Pick; + /** The risk rule's levels, and the size limits whose crossing is its size signal. */ + riskLevels: RiskLevels; + riskSizes: SizeLimits; } function resultOf(failed: boolean | null): CheckResult["result"] { @@ -492,7 +508,7 @@ function evaluate(facts: PrFacts, rules: CheckRules, pack?: CheckPack): Determin if (on("regression-risk")) { const read = Object.entries(facts.fixes ?? {}).filter((entry): entry is [string, number] => entry[1] !== null); const unread = facts.fixes === undefined ? ["every file"] : Object.keys(facts.fixes).filter((path) => facts.fixes![path] === null); - const evidence = [...hotFiles(new Map(read), rules.risk.hotFileFixes), ...migrationPaths(facts.paths, rules.risk.migrationGlobs).map((path) => `${path}: migration`)]; + const evidence = [...hotFiles(new Map(read), rules.risk.hotFileFixes), ...matchingPaths(facts.paths, rules.risk.migrationGlobs).map((path) => `${path}: migration`)]; if (evidence.length) { const reason = "changes code that regressed recently or a migration"; add("regression-risk", "needs-decision", reason); @@ -503,6 +519,17 @@ function evaluate(facts: PrFacts, rules: CheckRules, pack?: CheckPack): Determin result("regression-risk", null, reason, unread, { kind: "could-not-check", reason }); } else result("regression-risk", false, "no recently fixed files or migrations changed", [], { kind: "values", values: { risky: false } }); } + if (on("sensitive-paths")) { + const sensitive = sensitiveResult(facts, rules); + if (sensitive.result === "fail") add("sensitive-paths", "needs-decision", sensitive.reason); + checks.push(sensitive); + } + if (on("risk")) { + const risk = riskResult(facts, rules); + if (risk.result === "fail") add("risk", "needs-decision", risk.reason); + if (risk.result === "unconfirmed") add("risk", "stale-unknown", risk.reason); + checks.push(risk); + } if (on("linked-issue") && rules.tracker !== "off") { const ref = issueReference(facts, rules.tracker); if (!ref) add("linked-issue", "needs-author-update", "pull request does not reference an issue"); @@ -559,6 +586,62 @@ function evaluate(facts: PrFacts, rules: CheckRules, pack?: CheckPack): Determin return finish(findings, checks, facts, pack); } +const SENSITIVE_SHOWN = 5; + +/** Up to five changed files as `group: path`, then how many more. */ +function sensitiveEvidence(hits: readonly SensitiveHit[]): string[] { + const lines = hits.flatMap(({ name, paths }) => paths.map((path) => `${name}: ${path}`)); + return lines.length > SENSITIVE_SHOWN ? [...lines.slice(0, SENSITIVE_SHOWN), `and ${lines.length - SENSITIVE_SHOWN} more`] : lines; +} + +/** Fails at the highest changed group's level when that is `failAt` or above, and below it labels from `labelAt`. */ +function sensitiveResult(facts: PrFacts, rules: CheckRules): CheckResult { + const hits = sensitiveHits(facts.paths, rules.sensitiveGroups); + const level = sensitiveLevel(hits); + const outcome = riskOutcome(level, rules.sensitiveMarks); + const base = { check: "sensitive-paths", kind: "machine" as const, evidence: sensitiveEvidence(hits), output: { kind: "values" as const, values: { level } } }; + if (hits.length === 0) return { ...base, result: "pass", reason: "no sensitive paths changed" }; + if (outcome === "fail") return { ...base, result: "fail", reason: "changes sensitive paths" }; + return { ...base, result: "pass", reason: "changes sensitive paths", ...(outcome === "label" && level !== "low" && { label: levelLabel("sensitive-paths", level) }) }; +} + +/** Each risk signal the pull request shows, in words, beside whether its changed files' recent history was read in full. */ +function riskSignals(facts: PrFacts, rules: CheckRules): { signals: Map; sensitive: SensitiveHit[]; historyUnread: boolean } { + const signals = new Map(); + // With sensitive paths on, a high group is that rule's to ask about, so a migration is asked about once. + const sensitive = sensitiveHits(facts.paths, rules.sensitiveGroups).filter((hit) => hit.level !== "high" || !rules.enabled("sensitive-paths")); + if (sensitive.some((hit) => hit.level === "medium")) signals.set("sensitive", []); + const over = sizeResults(facts, rules.riskSizes).filter((size) => size.finding !== undefined); + if (over.length) signals.set("size", over.flatMap((size) => Object.entries(size.values).map(([key, count]) => `${count} ${key}`))); + if (untested(facts.paths)) signals.set("no-tests", ["no tests"]); + const fixes = Object.values(facts.fixes ?? {}); + const hot = fixes.filter((count) => count !== null && count >= rules.risk.hotFileFixes).length; + if (hot) signals.set("hot-files", [`${hot} recently fixed ${hot === 1 ? "file" : "files"}`]); + if (facts.authorKind && (AUTHOR_GROUPS["first-timers"] as readonly string[]).includes(facts.authorKind)) signals.set("first-time", ["first-time contributor"]); + return { signals, sensitive, historyUnread: hot === 0 && (facts.fixes === undefined || fixes.includes(null)) }; +} + +/** + * The pull request's risk level from its signals: it fails at `failAt`, and below that labels from `labelAt`. When a + * history left unread could have made a file hot and that would decide whether the rule fails, it is unconfirmed; a + * label it could only have added is left off. It never waits on the host, which will not read that history again. + */ +function riskResult(facts: PrFacts, rules: CheckRules): CheckResult { + const { signals, sensitive, historyUnread } = riskSignals(facts, rules); + const floor = sensitiveLevel(sensitive); + const level = riskLevel(new Set(signals.keys()), rules.riskLevels, floor); + const outcome = riskOutcome(level, rules.riskLevels); + const evidence = [...sensitive.map(({ name }) => `${name} paths`), ...[...signals.values()].flat()]; + const output: CheckOutput = { kind: "values", values: { level } }; + const unreadFails = riskOutcome(riskLevel(new Set([...signals.keys(), "hot-files"]), rules.riskLevels, floor), rules.riskLevels) === "fail"; + if (historyUnread && outcome !== "fail" && unreadFails) { + return { check: "risk", kind: "machine", result: "unconfirmed", reason: "could not read the recent history of some changed files", evidence, output }; + } + const base = { check: "risk", kind: "machine" as const, reason: `risk is ${level}`, evidence, output }; + if (outcome === "fail") return { ...base, result: "fail" }; + return { ...base, result: "pass", ...(outcome === "label" && level !== "low" && { label: levelLabel("risk", level) }) }; +} + function patternsReason(failed: boolean, found: { skipped: string[]; checked: number }): string { if (failed) return "added code matches a code pattern"; if (found.checked === 0 && found.skipped.length > 0) return "nothing checked: the only TypeScript or JavaScript changed is vendored, built or installed code"; @@ -574,9 +657,21 @@ function deriveFromPack(facts: PrFacts, pack: CheckPack): DeterministicResult { tracker: pack.checks["linked-issue"]?.tracker ?? "either", conventional: { types: pack.checks["conventional-commits"]?.types ?? CONVENTIONAL_TYPES, fields: pack.checks["conventional-commits"]?.fields ?? CONVENTIONAL_FIELDS }, risk: { hotFileFixes: pack.checks["regression-risk"]?.hotFileFixes ?? DEFAULT_RISK.hotFileFixes, migrationGlobs: pack.checks["regression-risk"]?.migrationGlobs ?? DEFAULT_RISK.migrationGlobs }, + ...riskRules(pack), }, pack); } +/** The sensitive groups and the size limits are the sensitive-paths and size rules' own, whether or not those rules are on. */ +function riskRules(pack: CheckPack): Pick { + const [sensitive, risk] = [pack.checks["sensitive-paths"], pack.checks.risk]; + return { + sensitiveGroups: sensitive?.sensitiveGroups ?? DEFAULT_SENSITIVE, + sensitiveMarks: { failAt: sensitive?.failAt ?? DEFAULT_MARKS.failAt, labelAt: sensitive?.labelAt ?? DEFAULT_MARKS.labelAt }, + riskLevels: { medium: risk?.medium ?? DEFAULT_RISK_LEVELS.medium, high: risk?.high ?? DEFAULT_RISK_LEVELS.high, failAt: risk?.failAt ?? DEFAULT_RISK_LEVELS.failAt, labelAt: risk?.labelAt ?? DEFAULT_RISK_LEVELS.labelAt }, + riskSizes: { "max-files-changed": pack.checks["max-files-changed"]?.maxFiles ?? RECOMMENDED_SIZE.maxFiles, "max-lines-changed": pack.checks["max-lines-changed"]?.maxLines ?? RECOMMENDED_SIZE.maxLines }, + }; +} + /** The hub read the stored policy, so what that read left out comes with the pack it mails. */ export function packFromInput(raw: unknown): CheckPack { const pack = readCheckPack(raw); @@ -594,5 +689,9 @@ export function deriveState(facts: PrFacts, policy?: unknown, pack?: CheckPack | forbiddenGlobs: [], tracker: "off", risk: DEFAULT_RISK, + sensitiveGroups: DEFAULT_SENSITIVE, + sensitiveMarks: DEFAULT_MARKS, + riskLevels: DEFAULT_RISK_LEVELS, + riskSizes: {}, }); } diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index 753a84b94..b0e10efaf 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -1,5 +1,5 @@ import { parse } from "@ast-grep/napi"; -import { isForeignPath, PATTERN_PRESETS, patternRuleApplies, type PatternLanguage, type PatternRule, type PatternsCheck } from "@corbits/triage-contracts"; +import { isForeignPath, PATTERN_EXTENSIONS, PATTERN_PRESETS, type PatternExtension, patternRuleApplies, type PatternLanguage, type PatternRule, type PatternsCheck } from "@corbits/triage-contracts"; import type { PrFileFacts } from "./checks.js"; import { addedLines, MAX_PATCH_INPUT_CHARS } from "./hunks.js"; @@ -7,7 +7,7 @@ type Grammar = { language: PatternLanguage; lang: "TypeScript" | "Tsx" | "JavaSc const TYPESCRIPT: Grammar = { language: "typescript", lang: "TypeScript" }; const JAVASCRIPT: Grammar = { language: "javascript", lang: "JavaScript" }; -const GRAMMARS: Record = { +const GRAMMARS: Record = { ts: TYPESCRIPT, mts: TYPESCRIPT, cts: TYPESCRIPT, @@ -33,7 +33,8 @@ export function patternRules(check: PatternsCheck): PatternRule[] { } function grammarOf(path: string): Grammar | undefined { - return GRAMMARS[path.slice(path.lastIndexOf(".") + 1)]; + const extension = PATTERN_EXTENSIONS.find((known) => path.endsWith(`.${known}`)); + return extension && GRAMMARS[extension]; } function checkable(file: PrFileFacts): boolean { diff --git a/packages/triage-workflows/src/logic/render.ts b/packages/triage-workflows/src/logic/render.ts index 8466a402a..04263347b 100644 --- a/packages/triage-workflows/src/logic/render.ts +++ b/packages/triage-workflows/src/logic/render.ts @@ -463,6 +463,8 @@ function failedStep(c: CheckResult, sources: Sources): Step { case "linked-issue": return { actor: "author", action: "Link an issue" }; case "no-duplicate": return { actor: "author", action: `Close it: it ${c.reason}` }; case "regression-risk": return { actor: "maintainer", action: `Review closely: ${c.evidence.join("; ")}` }; + case "sensitive-paths": return { actor: "maintainer", action: `Review closely: ${c.evidence.join("; ")}` }; + case "risk": return { actor: "maintainer", action: `Review closely: ${riskLine(c)}` }; case NO_CHANGES_REQUESTED: { const reviewers = c.evidence.map((e) => e.replace(/^changes requested by /, "")).join(", "); return c.reason === REVIEW_ANSWERED_REASON @@ -546,6 +548,22 @@ export function hostChecksPending(det: Pick, noWa return !noWait && det.checks.some(hostPending); } +/** "Risk: high (schema paths, 42 files, no tests)" */ +function riskLine(c: CheckResult): string { + const level = c.output.kind === "values" ? c.output.values.level : undefined; + return c.evidence.length ? `Risk: ${level} (${c.evidence.join(", ")})` : `Risk: ${level}`; +} + +/** Risk whose history Triage could not read: the host will not read it again, so it is noted rather than waited on. */ +function isUnchecked(c: CheckResult): boolean { + return c.check === "risk" && c.result === "unconfirmed"; +} + +/** A rule that did not fail but still labels, so it is noted beside the labelling rules. */ +function isMarked(c: CheckResult): boolean { + return c.label !== undefined; +} + /** A labelling rule's result that labels and notes the pull request. */ export function isLabelledFinding(c: CheckResult): boolean { return c.result === "fail" || unsure(c); @@ -627,7 +645,9 @@ const NOTHING_TO_CHANGE = "No changes needed from you. "; * nothing else is not posted. */ function noteOf(c: CheckResult, ctx: RenderContext): string { - return c.result === "fail" ? `- ${c.reason}${failureEvidence(c, ctx)}` : `- Not confirmed: ${passText(c.check, ctx.sources!)}`; + if (isUnchecked(c)) return `- Risk could not be checked: ${c.reason}`; + if (c.check === "risk") return `- ${riskLine(c)}`; + return c.result === "fail" || isMarked(c) ? `- ${c.reason}${failureEvidence(c, ctx)}` : `- Not confirmed: ${passText(c.check, ctx.sources!)}`; } function authorComment(ctx: RenderContext, checks: CheckResult[], verdict: VerdictWord): string { @@ -636,7 +656,7 @@ function authorComment(ctx: RenderContext, checks: CheckResult[], verdict: Verdi const mine = authorFailures(checks, sources); const flagged = checks.filter(isFlagged); const unconfirmed = sources ? checks.filter(unsure) : []; - const pending = checks.filter(unsettledMachine); + const pending = checks.filter((c) => unsettledMachine(c) && !isUnchecked(c)); const lines: string[] = []; if (mine.length) { lines.push(`@${author}, please address the following:`, ...mine.flatMap((c) => c.check === "focused" @@ -645,7 +665,7 @@ function authorComment(ctx: RenderContext, checks: CheckResult[], verdict: Verdi } if (flagged.length) { if (lines.length) lines.push(""); - lines.push("Asking a maintainer about:", ...flagged.map((c) => `- ${c.reason}${failureEvidence(c, ctx)}`)); + lines.push("Asking a maintainer about:", ...flagged.map((c) => (c.check === "risk" ? `- ${riskLine(c)}` : `- ${c.reason}${failureEvidence(c, ctx)}`))); } if (unconfirmed.length) { if (lines.length) lines.push(""); @@ -666,7 +686,7 @@ function authorComment(ctx: RenderContext, checks: CheckResult[], verdict: Verdi if (ctx.stack?.onto !== undefined && !holdsOthersChanges(ctx.stack)) lines.push("", `Stacked on #${ctx.stack.onto}: only this pull request's own changes were checked.`); const skipped = sources ? checks.flatMap((c) => skippedLine(c, sources)) : []; if (skipped.length) lines.push("", "Skipped:", ...skipped); - const notes = ctx.labelled.filter(isLabelledFinding); + const notes = [...ctx.labelled.filter((c) => isLabelledFinding(c) || isMarked(c) || isUnchecked(c)), ...checks.filter((c) => isMarked(c) || isUnchecked(c))]; if (notes.length) lines.push("", "Notes:", ...notes.map((c) => noteOf(c, ctx))); return bounded([commentTitle(verdict), "", ...lines]); } diff --git a/packages/triage-workflows/src/logic/risk.ts b/packages/triage-workflows/src/logic/risk.ts index 358cdeaf3..9132e0431 100644 --- a/packages/triage-workflows/src/logic/risk.ts +++ b/packages/triage-workflows/src/logic/risk.ts @@ -1,4 +1,5 @@ -import { isDocPath, isGeneratedPath, isTestPath } from "@corbits/triage-contracts/paths"; +import { PATTERN_EXTENSIONS, RISK_LEVELS, type RiskLevel, type RiskLevels, type RiskMark, type RiskSignal, type RiskThreshold, type SensitiveGroup } from "@corbits/triage-contracts"; +import { isDocPath, isForeignPath, isGeneratedPath, isTestPath } from "@corbits/triage-contracts/paths"; import { globMatch } from "./glob.js"; // Only the subject's type counts: branch commits like "fix lint" or a body mentioning a fix would make every busy file hot. @@ -27,6 +28,62 @@ export function hotFiles(fixes: ReadonlyMap, threshold: number): return hot.length > HOT_SHOWN ? [...shown, `and ${hot.length - HOT_SHOWN} more`] : shown; } -export function migrationPaths(paths: readonly string[], globs: readonly string[]): string[] { +/** The paths matching any of the globs. */ +export function matchingPaths(paths: readonly string[], globs: readonly string[]): string[] { return paths.filter((path) => globs.some((glob) => globMatch(path, glob))); } + +export type SensitiveHit = { name: string; level: RiskMark; paths: string[] }; + +/** Each group with a changed path matching any of its globs, and those paths. */ +export function sensitiveHits(paths: readonly string[], groups: Readonly>): SensitiveHit[] { + return Object.entries(groups).flatMap(function hits([name, group]): SensitiveHit[] { + const matched = matchingPaths(paths, group.paths); + return matched.length ? [{ name, level: group.level, paths: matched }] : []; + }); +} + +/** The highest level among the groups changed, or low with none. */ +export function sensitiveLevel(hits: readonly SensitiveHit[]): RiskLevel { + if (hits.some((hit) => hit.level === "high")) return "high"; + return hits.length ? "medium" : "low"; +} + +const SOURCE_EXTENSIONS = new Set([...PATTERN_EXTENSIONS, "go", "py", "rs", "java", "kt", "rb", "php", "cs", "swift"]); +// Browser and snapshot tests sit outside the unit-test layouts every other rule reads. +const MORE_TESTS = /(^|\/)(e2e|cypress|__snapshots__)\/|\.cy\.[^/]+$/; + +function isRiskTest(path: string): boolean { + return isTestPath(path) || MORE_TESTS.test(path); +} + +// Type declarations, docs and examples are written in a code language without being the code a change ships. +const NOT_SOURCE = /\.d\.[cm]?ts$|(^|\/)(docs|examples)\//; + +/** Code the repository writes in a common language; config, data, docs and assets are not source. */ +function isSource(path: string): boolean { + return SOURCE_EXTENSIONS.has(path.slice(path.lastIndexOf(".") + 1)) && !NOT_SOURCE.test(path) && !isRiskTest(path) && !isForeignPath(path); +} + +/** Source files changed without a test file changing. */ +export function untested(paths: readonly string[]): boolean { + return paths.some(isSource) && !paths.some(isRiskTest); +} + +function reaches(threshold: RiskThreshold, signals: ReadonlySet): boolean { + if (typeof threshold === "number") return signals.size >= threshold; + return threshold.some((entry) => (typeof entry === "string" ? signals.has(entry) : entry.every((signal) => signals.has(signal)))); +} + +/** A change to a high sensitive group is high whatever the other signals. */ +export function riskLevel(signals: ReadonlySet, levels: RiskLevels, sensitive: RiskLevel): RiskLevel { + if (sensitive === "high" || reaches(levels.high, signals)) return "high"; + return reaches(levels.medium, signals) ? "medium" : "low"; +} + +/** Whether a level fails the rule, or else labels the pull request; a `labelAt` at or above `failAt` never labels. */ +export function riskOutcome(level: RiskLevel, { failAt, labelAt }: Pick): "fail" | "label" | "pass" { + const rank = RISK_LEVELS.indexOf(level); + if (rank >= RISK_LEVELS.indexOf(failAt)) return "fail"; + return rank >= RISK_LEVELS.indexOf(labelAt) ? "label" : "pass"; +} diff --git a/packages/triage-workflows/src/pr-triage.test.ts b/packages/triage-workflows/src/pr-triage.test.ts index 4ce2fec54..2db3f26af 100644 --- a/packages/triage-workflows/src/pr-triage.test.ts +++ b/packages/triage-workflows/src/pr-triage.test.ts @@ -353,6 +353,41 @@ describe("pr-triage body", () => { expect((await triage(pack, { history: "unread" })).verdict).toMatchObject({ verdict: "pass", labelled: [{ check: "regression-risk", result: "unconfirmed", evidence: ["src/flag.ts"] }] }); }); + test("risk and sensitive paths label and note a medium pull request and ask a maintainer about a high one, naming what they found", async () => { + const pack: CheckPack = { ...emptyPack(REPO), checks: { "sensitive-paths": { enabled: true, onFail: "ask" }, risk: { enabled: true, onFail: "ask" } } }; + const medium = (await triage(pack)).verdict as Verdict; + expect(medium).toMatchObject({ verdict: "pass", actions: [{ list: "pass", kind: "labels", target: { labels: ["risk: medium"] } }] }); + expect(medium.request.comment).toBe("**Triage: Pass**\n\nNo changes needed from you. A maintainer will review this pull request.\n\nNotes:\n- Risk: medium (no tests)\n\nTriage will label."); + const migration: FileRow = { filename: "db/migrations/0002.sql", status: "added", additions: 1, deletions: 0 }; + const high = (await triage(pack, { files: [FLAG, migration] })).verdict as Verdict; + expect(high).toMatchObject({ verdict: "unsure", actor: "maintainer", actions: [{ list: "unsure", kind: "request-review" }, { list: "unsure", kind: "labels", target: { labels: ["risk: medium"] } }] }); + expect(high.request.comment).toBe("**Triage: Unsure**\n\nAsking a maintainer about:\n- changes sensitive paths: `schema: db/migrations/0002.sql`\n\nNotes:\n- Risk: medium (no tests)\n\nTriage will request review, label."); + expect((await triage(pack, { files: [FLAG, { ...FLAG, filename: "src/flag.test.ts" }] })).verdict).toMatchObject({ verdict: "pass", actions: [] }); + const bump = (await triage(pack, { files: [{ filename: "bun.lock", status: "modified", additions: 1, deletions: 1 }] })).verdict as Verdict; + expect(bump).toMatchObject({ verdict: "pass", actions: [{ list: "pass", kind: "labels", target: { labels: ["sensitive: medium", "risk: medium"] } }] }); + expect(bump.request.comment).toBe("**Triage: Pass**\n\nNo changes needed from you. A maintainer will review this pull request.\n\nNotes:\n- changes sensitive paths: `locks: bun.lock`\n- Risk: medium (locks paths)\n\nTriage will label."); + }); + + test("risk it could not check for unread history is noted, never waited on", async () => { + const pack: CheckPack = { ...emptyPack(REPO), checks: { risk: { enabled: true, onFail: "ask", failAt: "medium" } } }; + const unread = (await triage(pack, { files: [FLAG, { ...FLAG, filename: "src/flag.test.ts" }], history: "unread" })).verdict as Verdict; + expect(unread).toMatchObject({ verdict: "unsure", checks: expect.arrayContaining([expect.objectContaining({ check: "risk", result: "unconfirmed" })]) }); + expect(unread.waiting).toBeUndefined(); + expect(unread.request.comment).toBe("**Triage: Unsure**\n\nA maintainer will review this pull request.\n\nNotes:\n- Risk could not be checked: could not read the recent history of some changed files\n\nTriage will request review."); + }); + + test("Triage swaps the level labels of sensitive paths and risk as the level changes, and a risk rule that only labels adds risk: high", async () => { + const pack: CheckPack = { ...emptyPack(REPO), checks: { "sensitive-paths": { enabled: true, onFail: "ask" }, risk: { enabled: true, onFail: "ask" } } }; + const lowered = (await triage(pack, { labels: ["risk: high", "sensitive: medium", "risk: someday"] })).verdict as Verdict; + expect(lowered.actions).toEqual([expect.objectContaining({ kind: "labels", target: { labels: ["risk: medium"], unlabel: ["risk: high", "sensitive: medium"] } })]); + const tested = [FLAG, { ...FLAG, filename: "src/flag.test.ts" }]; + expect((await triage(pack, { files: tested, labels: ["risk: medium"] })).verdict).toMatchObject({ actions: [{ kind: "labels", target: { labels: [], unlabel: ["risk: medium"] } }] }); + expect((await triage(pack, { files: tested })).verdict).toMatchObject({ actions: [] }); + const labelOnly: CheckPack = { ...emptyPack(REPO), checks: { risk: { enabled: true, onFail: "label" } } }; + const migration: FileRow = { filename: "db/migrations/0002.sql", status: "added", additions: 1, deletions: 0 }; + expect((await triage(labelOnly, { files: [migration] })).verdict).toMatchObject({ verdict: "pass", actions: [{ kind: "labels", target: { labels: ["risk: high"] } }] }); + }); + test("a labelling judged rule is asked but never asks the verdict question or decides the verdict", async () => { const { path, judged, verdict } = await triage({ ...emptyPack(REPO), checks: { "ci-green": { enabled: true }, docs: { enabled: true, onFail: "label" } } }); expect(path).toEqual(VERDICT_PATH); diff --git a/packages/triage-workflows/src/triage-steps.test.ts b/packages/triage-workflows/src/triage-steps.test.ts index e868b4b21..20888572b 100644 --- a/packages/triage-workflows/src/triage-steps.test.ts +++ b/packages/triage-workflows/src/triage-steps.test.ts @@ -326,7 +326,7 @@ describe("judge on a blocked pull request", () => { test("a stored step for a repository role no longer runs, and every verdict says so", async () => { const checkPack = { ...recommendedPack("acme/widgets"), actions: { ...defaultActions(), fail: [{ do: "request-review", target: { to: "role", role: "leads" } }] } }; const { verdict } = await judgeAndEvaluate(await facts([], true, false, { checkPack }, code)); - expect(verdict.actions).toEqual([]); + expect(verdict.actions).toEqual([expect.objectContaining({ kind: "labels", target: { labels: ["risk: medium"], unlabel: [] } })]); expect(verdict.reasons).toContain("Fail step 1 (request-review role leads) no longer runs"); }); @@ -356,6 +356,7 @@ describe("judge on a blocked pull request", () => { expect(asked).toEqual(["docs-file-001", "focused", "instructed", "focused-file-001"]); expect(verdict.actions).toEqual([ { list: "fail", index: 0, effectId: expect.any(String), kind: "request-review", target: { users: ["dave"], teams: [] }, reason: "mixes unrelated changes: src/flag.ts", held: true }, + { list: "fail", index: 1, effectId: expect.any(String), kind: "labels", target: { labels: ["risk: medium"], unlabel: [] }, reason: "mixes unrelated changes: src/flag.ts", held: true }, ]); }); diff --git a/skills/corbits-triage-setup/SKILL.md b/skills/corbits-triage-setup/SKILL.md index 9f19345b1..b9cd72b43 100644 --- a/skills/corbits-triage-setup/SKILL.md +++ b/skills/corbits-triage-setup/SKILL.md @@ -36,7 +36,7 @@ If the user says to go ahead with defaults, use the suggested answers. ## 3. Propose the rules -Start from the four rules in reference.md's "Starter file", which hold for any repository and need no model. The size limits label large pull requests rather than fail them; keep 30 files and 3,000 lines unless the user names others. With no CI, `ci-green` passes until CI is added; keep it and say so. +Start from the six rules in reference.md's "Starter file", which hold for any repository and need no model. The size limits label large pull requests rather than fail them, and sensitive paths and risk ask a maintainer about high-level changes and label medium ones; keep 30 files and 3,000 lines unless the user names others. With no CI, `ci-green` passes until CI is added; keep it and say so. Then add only the extras that fit, from "When to add" in [reference.md](reference.md), each with a one-line reason. Show the rules and actions as one line each and ask for a yes or changes before writing. Once the user agrees, each rule is `maintainer-stated`. diff --git a/skills/corbits-triage-setup/reference.md b/skills/corbits-triage-setup/reference.md index 026b49297..e9275add9 100644 --- a/skills/corbits-triage-setup/reference.md +++ b/skills/corbits-triage-setup/reference.md @@ -33,6 +33,8 @@ A built-in rule is named only by `check`, never `id`, and each built-in may appe | `conventional-commits` | `types:` list of words (optional, default `feat`, `fix`, `refactor`, `perf`, `docs`, `test`, `chore`, `ci`, `build`, `style`, `revert`); `fields:` `title`, `commits` or both (optional, default both) | no | Fails unless the title and each commit's first line (first 100 commits) start `type: `, `type(scope): ` or `type!: ` with a listed type, in any case. Merges and `Revert "…"` commits are skipped. | | `hidden-content` | none | no | Fails when the title, description, a commit message or a changed path hides text: invisible, tag or bidi override characters, or a link with no visible content. On GitHub, also hidden markup in the description that speaks to a model, other than the repository's pull request template. | | `regression-risk` | `hotFileFixes:` whole number ≥ 1 (optional, default 4); `migrations:` list of 1–50 globs (optional, default `**/migrations/**`, `**/*.sql`) | no | Fails when a changed file (not a test, Markdown doc or lockfile) has at least `hotFileFixes` commits in the last 30 days whose subject starts with a `fix`, `bugfix`, `hotfix` or `revert` type (`fix:`, `fix(scope):`, `Revert "…"`); merges and body text don't count. Also fails when a changed file matches `migrations`. A history that can't be read in full, or a pull request changing more than 300 such files, leaves the rule unconfirmed. Default `onFail` is `label`. | +| `sensitive-paths` | `groups:` map of group name (lowercase words joined by hyphens) to `{ level: high or medium, paths: list of 1–50 globs }` (optional, default `schema`, `auth` high; `ci`, `locks` medium); `failAt:`, `labelAt:` `medium` or `high` (optional, default `high` and `medium`) | no | Fails when a changed file matches a group at `failAt` or above, globs as in `no-forbidden-paths`; below that, from `labelAt`, passes and labels `sensitive: `. Evidence names each group with up to five files. Defaults: `schema` `**/migrations/**`, `**/*.sql`; `auth` `**/auth/**`, `**/secrets/**`; `ci` `.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`; `locks` `**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`. Default `onFail` is `ask`. | +| `risk` | `medium:`, `high:` a number of signals from 1 to 5, or a list of signals where a nested list needs all of its own (optional, default `1` and `[[size, no-tests]]`); `failAt:`, `labelAt:` `medium` or `high` (optional, default `high` and `medium`) | no | Rates the pull request low, medium or high from five signals: `sensitive` (a medium `sensitive-paths` group, default groups if unset; a high group makes it high on its own unless `sensitive-paths` is on), `size` (over the `max-files-changed` or `max-lines-changed` limit, default 30 and 3,000), `no-tests` (source code in a common language changed, not `*.d.ts`, `docs/` or `examples/`, and no test file, `e2e/`, `cypress/` or `__snapshots__/` file), `hot-files` (a file `regression-risk` counts as hot) and `first-time` (a first-time contributor; never on Bitbucket Data Center). Fails at `failAt`; below it, from `labelAt`, passes and labels `risk: `. Unconfirmed when unread history could make a file hot and the rule fail. Default `onFail` is `ask`. | | `focused` | none | yes | The decision model checks the change is focused on one thing. | | `docs` | none | yes | The decision model checks docs are updated when behaviour changes. | @@ -174,6 +176,14 @@ rules: text: CI must pass before merge. reason: A change is not ready until CI passes. evidence: [maintainer-stated] + - check: sensitive-paths + text: "Flag changes to sensitive paths: schema, auth, ci, locks." + reason: Schema, auth, CI and lockfile changes reach beyond what the diff shows. + evidence: [maintainer-stated] + - check: risk + text: Flag high-risk pull requests and label medium-risk ones. + reason: A risky change needs a maintainer's eye. + evidence: [maintainer-stated] ``` ## Extras, written out