Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/hub/src/github/policy-file.postgres.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion apps/hub/src/github/policy-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}\`.`);

/**
Expand Down
4 changes: 2 additions & 2 deletions docs/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `#<number>`.
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
Expand Down
43 changes: 36 additions & 7 deletions docs/Corbits Triage User Manual/Corbits Triage User Manual.md

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions packages/policy-check-action/src/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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} }
`;

Expand Down
4 changes: 2 additions & 2 deletions packages/policy-check-action/src/local-git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,9 @@ export function localGithub(change: LocalChange): (url: URL) => Promise<Response

const ON_HOST = "needs the pull request on its host";

/** Rules that read what only the host knows: its CI, mergeability, reviews, default-branch fix history, its other open pull requests and the pull request's own title and description. */
/** Rules that read what only the host knows: its CI, mergeability, reviews, default-branch fix history, its other open pull requests, the author's standing and the pull request's own title and description. */
export function hostOnly(pack: CheckPack): Skipped[] {
const described = pack.custom.flatMap((row) => (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 }));
}
110 changes: 109 additions & 1 deletion packages/triage-contracts/src/check-pack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];

Expand Down Expand Up @@ -58,6 +60,8 @@ export const CHECK_CATALOG: Record<CatalogId, { name: string; group: CheckPackGr
"regression-risk": { name: "Regression risk", group: "pull-request", kind: "machine" },
"no-duplicate": { name: "No duplicate", group: "around", kind: "machine" },
"duplicates-open-pull-request": { name: "Duplicates an open pull request", group: "around", kind: "quality" },
"sensitive-paths": { name: "Sensitive paths", group: "pull-request", kind: "machine" },
risk: { name: "Risk", group: "pull-request", kind: "machine" },
};

export const NOT_A_DRAFT = "not-a-draft";
Expand Down Expand Up @@ -120,6 +124,45 @@ export const RECOMMENDED_PATHS = { forbiddenGlobs: ["vendor/**", "node_modules/*
* with at least `hotFileFixes` fix or revert commits in the last `hotFileDays`, or a changed path matching a migration glob.
*/
export const DEFAULT_RISK = { hotFileFixes: 4, hotFileDays: 30, migrationGlobs: ["**/migrations/**", "**/*.sql"] } as const;
export const RISK_MARKS = ["medium", "high"] as const;
export type RiskMark = (typeof RISK_MARKS)[number];
/** A named group of sensitive paths, and how sensitive a change to it is. */
export type SensitiveGroup = { level: RiskMark; paths: string[] };
/**
* The sensitive-paths rule's groups when the policy names none: a schema change is hard to undo and auth and secrets
* decide who gets in, so those are high; CI and deploy files decide what runs, and a lockfile pulls in code the diff does
* not show, but every dependency bump changes one, so those are medium.
*/
export const DEFAULT_SENSITIVE: Readonly<Record<string, SensitiveGroup>> = {
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<RiskSignal | RiskSignal[]>;
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;
Expand Down Expand Up @@ -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<string, SensitiveGroup>;
/**
* 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. */
Expand All @@ -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<string> = 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";
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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" }] },
Expand All @@ -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<Record<CatalogId, string>>;

/**
Expand Down Expand Up @@ -687,13 +755,52 @@ 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);
}
return check;
}

function sensitiveGroups(value: unknown): Record<string, SensitiveGroup> {
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<string, unknown>, where: string): Pick<CatalogCheck, "failAt" | "labelAt"> {
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<string, unknown>): Pick<CatalogCheck, "medium" | "high"> {
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));
Expand Down Expand Up @@ -785,6 +892,7 @@ function parseNote(row: Record<string, unknown>, 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`;
}

Expand Down
3 changes: 3 additions & 0 deletions packages/triage-contracts/src/code-patterns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 7 additions & 1 deletion packages/triage-contracts/src/policy-export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ function catalogParams(id: CatalogId, row: CatalogCheck): Record<string, unknown
return row.forbiddenGlobs?.length ? { paths: row.forbiddenGlobs } : undefined;
case "regression-risk":
return { ...(row.hotFileFixes !== undefined && { hotFileFixes: row.hotFileFixes }), ...(row.migrationGlobs && { migrations: row.migrationGlobs }) };
case "sensitive-paths":
return { ...(row.sensitiveGroups && { groups: row.sensitiveGroups }), ...(row.failAt && { failAt: row.failAt }), ...(row.labelAt && { labelAt: row.labelAt }) };
case "risk":
return { ...(row.medium !== undefined && { medium: row.medium }), ...(row.high !== undefined && { high: row.high }), ...(row.failAt && { failAt: row.failAt }), ...(row.labelAt && { labelAt: row.labelAt }) };
case "conventional-commits":
return { ...(row.types && { types: row.types }), ...(row.fields && { fields: row.fields }) };
default:
Expand Down Expand Up @@ -155,7 +159,7 @@ function catalogChecks(pack: CheckPack, id: CatalogId): string | undefined {
if (id === "ci-green" && row?.enabled === false) return CI_SKIPPED;
if (!row?.enabled) return undefined;
const conventional = id === "conventional-commits" && { types: sorted(row.types ?? CONVENTIONAL_TYPES), fields: row.fields ?? CONVENTIONAL_FIELDS };
return canonical({ max: isSizeRule(id) ? row[SIZE_RULES[id]] : undefined, tracker: row.tracker ?? "either", paths: sorted(row.forbiddenGlobs), hotFileFixes: row.hotFileFixes, migrations: sorted(row.migrationGlobs), onFail: onFailText(pack, id), ...conventional });
return canonical({ max: isSizeRule(id) ? row[SIZE_RULES[id]] : undefined, tracker: row.tracker ?? "either", paths: sorted(row.forbiddenGlobs), hotFileFixes: row.hotFileFixes, migrations: sorted(row.migrationGlobs), groups: row.sensitiveGroups, medium: row.medium, high: row.high, failAt: row.failAt, labelAt: row.labelAt, onFail: onFailText(pack, id), ...conventional });
}

function modelAsks(row: ModelCustomCheck): Record<string, unknown> {
Expand Down Expand Up @@ -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"];
Expand Down
Loading
Loading