diff --git a/.gittensory.yml.example b/.gittensory.yml.example index fc491b0b7f..3b3ba1ad94 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -326,6 +326,20 @@ review: - dependabot - renovate + # Deterministic label suggestions (#2045). Each rule SUGGESTS a non-scoring label when a PR matches ALL of the + # `when` criteria it sets (at least one is required): when_paths (any changed path matches a glob), title_contains, + # description_contains (both case-insensitive). Suggestions are advisory; they are auto-applied only when the repo's + # autoLabelEnabled is on. Reserved `gittensor:` labels (scoring/type) are refused. Empty/unset ⇒ no suggestions. + labeling_rules: + - label: area:docs + when_paths: + - "docs/**" + - "**/*.md" + - label: needs:migration + when_paths: + - "migrations/**" + description_contains: schema + settings: # Who receives the public PR comment. # off | detected_contributors_only | all_prs. Default: detected_contributors_only. diff --git a/src/review/labeling-rules.ts b/src/review/labeling-rules.ts new file mode 100644 index 0000000000..69de2678f8 --- /dev/null +++ b/src/review/labeling-rules.ts @@ -0,0 +1,43 @@ +import { matchesManifestPath, type LabelingRule } from "../signals/focus-manifest"; + +// Deterministic evaluation of `review.labeling_rules` (#2045, part of #1959). Given a repo's parsed rules and a PR's +// facts, decide which non-scoring labels the rules SUGGEST — and, when the repo's `autoLabelEnabled` is set, which to +// auto-apply. Pure (no IO), mirroring the pure `resolvePrTypeLabel` decider in src/settings/pr-type-label.ts: the +// actual GitHub apply is a separate processor concern, kept out of here so this stays deterministic and unit-testable. + +/** The PR facts a labeling rule matches against. */ +export type LabelingRuleFacts = { + changedPaths: readonly string[]; + title: string; + description: string; +}; + +/** `suggest`: every firing rule's label (advisory, deduped, in rule order). `apply`: the subset to actually write — + * the same list when `autoLabelEnabled`, otherwise empty (suggestions only). */ +export type LabelingDecision = { suggest: string[]; apply: string[] }; + +/** A rule fires when ALL of its specified criteria match: at least one changed path matches a `whenPaths` glob (when + * any is set), the title contains `titleContains` (case-insensitive), and the description contains + * `descriptionContains`. Unset criteria don't constrain. A rule always has ≥1 criterion (enforced at parse). Pure. */ +function ruleMatches(rule: LabelingRule, facts: LabelingRuleFacts): boolean { + if (rule.whenPaths.length > 0 && !facts.changedPaths.some((path) => rule.whenPaths.some((glob) => matchesManifestPath(path, glob)))) { + return false; + } + if (rule.titleContains !== null && !facts.title.toLowerCase().includes(rule.titleContains.toLowerCase())) return false; + if (rule.descriptionContains !== null && !facts.description.toLowerCase().includes(rule.descriptionContains.toLowerCase())) return false; + return true; +} + +/** Resolve the labels suggested (and, when auto-labeling is on, to apply) for a PR. Deterministic: preserves rule + * order, dedupes, and never mutates its inputs. Pure. */ +export function resolveLabelingRules(input: { + rules: readonly LabelingRule[]; + facts: LabelingRuleFacts; + autoLabelEnabled: boolean; +}): LabelingDecision { + const suggest: string[] = []; + for (const rule of input.rules) { + if (ruleMatches(rule, input.facts) && !suggest.includes(rule.label)) suggest.push(rule.label); + } + return { suggest, apply: input.autoLabelEnabled ? [...suggest] : [] }; +} diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 8f1c979b58..e3e069454c 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -340,6 +340,20 @@ export type FocusManifestReviewConfig = { /** `review.auto_review`: deterministic eligibility filters that skip the AI review (never a gate failure). * Empty/default ⇒ every PR is reviewed (byte-identical). (#1954 / #2038–#2041) */ autoReview: AutoReviewConfig; + /** `review.labeling_rules`: deterministic `{label, when}` rules that SUGGEST a non-scoring label when a PR's + * changed paths / title / description match. Surfaced as advisory suggestions, and auto-applied only when the + * repo's `autoLabelEnabled` is set. Reserved `gittensor:` labels are refused at parse. Empty (default) ⇒ no + * suggestion (byte-identical). (#2045, part of #1959) */ + labelingRules: LabelingRule[]; +}; + +/** One `review.labeling_rules[]` entry: a non-reserved `label` plus the deterministic `when` criteria that must ALL + * match for it to fire. A rule always has at least one criterion (enforced at parse). */ +export type LabelingRule = { + label: string; + whenPaths: string[]; + titleContains: string | null; + descriptionContains: string | null; }; /** Per-repo AI review eligibility knobs under `review.auto_review`. Unset fields are byte-identical defaults. */ @@ -524,7 +538,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [] }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -554,7 +568,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [] }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1469,7 +1483,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [] }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -1512,6 +1526,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const pathFilters = parseReviewPathFilters(r.path_filters, warnings); const preMergeChecks = parseReviewPreMergeChecks(r.pre_merge_checks, warnings); const autoReview = parseAutoReviewConfig(r.auto_review, warnings); + const labelingRules = parseReviewLabelingRules(r.labeling_rules, warnings); return { present: footerText !== null || @@ -1526,6 +1541,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo pathFilters.length > 0 || preMergeChecks.length > 0 || autoReviewPresent(autoReview) || + labelingRules.length > 0 || Object.keys(fields).length > 0 || Object.keys(enrichmentAnalyzers).length > 0, footerText, @@ -1542,9 +1558,57 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo excludePaths, pathFilters, preMergeChecks, + labelingRules, }; } +/** The reserved label namespace Gittensor uses for scoring/type/priority (`gittensor:bug`, `gittensor:feature`, + * `gittensor:priority`, …). A maintainer's `labeling_rules` must not drive these — they're managed by the scorer + * and the type-labeler, never by ad-hoc manifest rules — so any `gittensor:`-prefixed label is refused at parse. */ +const RESERVED_LABEL_PREFIX = "gittensor:"; + +/** Parse `review.labeling_rules` into deterministic {@link LabelingRule}s (mirrors {@link parseReviewPreMergeChecks}). + * Non-list warns + ignores; each entry needs a public-safe, NON-reserved `label` and at least one `when` criterion + * (when_paths / title_contains / description_contains). Invalid entries are dropped with a warning; capped at + * MAX_PATH_INSTRUCTIONS so a hostile manifest can't bloat the matcher. Pure. */ +function parseReviewLabelingRules(value: JsonValue | undefined, warnings: string[]): LabelingRule[] { + if (value === undefined || value === null) return []; + if (!Array.isArray(value)) { + warnings.push(`Manifest "review.labeling_rules" must be a list of rules; ignoring it.`); + return []; + } + const out: LabelingRule[] = []; + for (const [index, entry] of value.entries()) { + if (out.length >= MAX_PATH_INSTRUCTIONS) { + warnings.push(`Manifest "review.labeling_rules" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`); + break; + } + if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { + warnings.push(`Manifest "review.labeling_rules[${index}]" must be a mapping; ignoring it.`); + continue; + } + const e = entry as Record; + const label = e.label === undefined || e.label === null ? null : parsePublicSafeText(e.label, `review.labeling_rules[${index}].label`, warnings); + if (label === null) { + if (e.label === undefined || e.label === null) warnings.push(`Manifest "review.labeling_rules[${index}].label" is required; ignoring the entry.`); + continue; // non-string / empty / not-public-safe already warned by parsePublicSafeText + } + if (label.toLowerCase().startsWith(RESERVED_LABEL_PREFIX)) { + warnings.push(`Manifest "review.labeling_rules[${index}].label" ("${label}") uses the reserved "${RESERVED_LABEL_PREFIX}" namespace; ignoring the entry.`); + continue; + } + const titleContains = e.title_contains === undefined || e.title_contains === null ? null : parsePublicSafeText(e.title_contains, `review.labeling_rules[${index}].title_contains`, warnings); + const descriptionContains = e.description_contains === undefined || e.description_contains === null ? null : parsePublicSafeText(e.description_contains, `review.labeling_rules[${index}].description_contains`, warnings); + const whenPaths = parseManifestGlobList(e.when_paths, `review.labeling_rules[${index}].when_paths`, warnings); + if (whenPaths.length === 0 && titleContains === null && descriptionContains === null) { + warnings.push(`Manifest "review.labeling_rules[${index}]" needs at least one of when_paths / title_contains / description_contains; ignoring it.`); + continue; + } + out.push({ label, whenPaths, titleContains, descriptionContains }); + } + return out; +} + function autoReviewPresent(config: AutoReviewConfig): boolean { return ( config.skipDrafts !== null || @@ -1805,6 +1869,15 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue } if (Object.keys(review.fields).length > 0) out.fields = { ...review.fields } as Record; if (Object.keys(review.enrichmentAnalyzers).length > 0) out.enrichment = { ...review.enrichmentAnalyzers } as Record; + if (review.labelingRules.length > 0) { + out.labeling_rules = review.labelingRules.map((rule) => { + const entry: Record = { label: rule.label }; + if (rule.whenPaths.length > 0) entry.when_paths = [...rule.whenPaths]; + if (rule.titleContains !== null) entry.title_contains = rule.titleContains; + if (rule.descriptionContains !== null) entry.description_contains = rule.descriptionContains; + return entry; + }); + } return out; } diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 726ae7ed1b..ae1b73c9ef 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -548,7 +548,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [] }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, diff --git a/test/unit/review-labeling-rules.test.ts b/test/unit/review-labeling-rules.test.ts new file mode 100644 index 0000000000..622ce2673e --- /dev/null +++ b/test/unit/review-labeling-rules.test.ts @@ -0,0 +1,117 @@ +import { describe, expect, it } from "vitest"; +import { parseFocusManifest, reviewConfigToJson } from "../../src/signals/focus-manifest"; +import { resolveLabelingRules } from "../../src/review/labeling-rules"; + +const rulesOf = (labeling_rules: unknown) => parseFocusManifest({ review: { labeling_rules } }); +const facts = (over: Partial<{ changedPaths: string[]; title: string; description: string }> = {}) => ({ + changedPaths: [], + title: "", + description: "", + ...over, +}); + +describe("review.labeling_rules parse + round-trip (#2045)", () => { + it("absent ⇒ empty and OMITTED on serialize (byte-identical)", () => { + const review = parseFocusManifest({ review: { note: "x" } }).review; + expect(review.labelingRules).toEqual([]); + expect("labeling_rules" in (reviewConfigToJson(review) as Record)).toBe(false); + }); + + it("a full rule round-trips parse → serialize → parse identically", () => { + const review = rulesOf([ + { label: "area:docs", when_paths: ["docs/**"], title_contains: "doc", description_contains: "readme" }, + ]).review; + expect(review.labelingRules).toEqual([ + { label: "area:docs", whenPaths: ["docs/**"], titleContains: "doc", descriptionContains: "readme" }, + ]); + const json = reviewConfigToJson(review) as Record; + expect(json.labeling_rules).toEqual([ + { label: "area:docs", when_paths: ["docs/**"], title_contains: "doc", description_contains: "readme" }, + ]); + expect(parseFocusManifest({ review: json }).review.labelingRules).toEqual(review.labelingRules); + }); + + it("serializes only the criteria that are set (a path-only rule omits title/description keys)", () => { + const review = rulesOf([{ label: "area:ci", when_paths: [".github/**"] }]).review; + expect((reviewConfigToJson(review) as Record).labeling_rules).toEqual([ + { label: "area:ci", when_paths: [".github/**"] }, + ]); + }); + + it("refuses a reserved gittensor: label and warns", () => { + const m = rulesOf([{ label: "gittensor:feature", when_paths: ["src/**"] }]); + expect(m.review.labelingRules).toEqual([]); + expect(m.warnings.some((w) => /reserved "gittensor:" namespace/.test(w))).toBe(true); + }); + + it("drops a rule with no when-criterion, a rule with no label, and a non-mapping entry (each warns)", () => { + const m = rulesOf([{ label: "area:x" }, { when_paths: ["a/**"] }, "nope"]); + expect(m.review.labelingRules).toEqual([]); + expect(m.warnings.some((w) => /needs at least one of when_paths/.test(w))).toBe(true); + expect(m.warnings.some((w) => /\.label" is required/.test(w))).toBe(true); + expect(m.warnings.some((w) => /\[2\]" must be a mapping/.test(w))).toBe(true); + }); + + it("a present-but-invalid (non-string) label is dropped and warned by the text validator (not 'required')", () => { + const m = rulesOf([{ label: 123, when_paths: ["src/**"] }]); + expect(m.review.labelingRules).toEqual([]); + expect(m.warnings.some((w) => /labeling_rules\[0\]\.label/.test(w))).toBe(true); + }); + + it("a title-only rule round-trips with when_paths omitted", () => { + const review = rulesOf([{ label: "type:wip", title_contains: "WIP" }]).review; + expect(review.labelingRules).toEqual([ + { label: "type:wip", whenPaths: [], titleContains: "WIP", descriptionContains: null }, + ]); + expect((reviewConfigToJson(review) as Record).labeling_rules).toEqual([ + { label: "type:wip", title_contains: "WIP" }, + ]); + }); + + it("a non-list labeling_rules warns and is ignored", () => { + const m = rulesOf("nope"); + expect(m.review.labelingRules).toEqual([]); + expect(m.warnings.some((w) => /"review\.labeling_rules" must be a list/.test(w))).toBe(true); + }); + + it("caps at 50 rules", () => { + const many = Array.from({ length: 60 }, (_, i) => ({ label: `area:${i}`, when_paths: ["src/**"] })); + const m = rulesOf(many); + expect(m.review.labelingRules.length).toBe(50); + expect(m.warnings.some((w) => /capped at 50/.test(w))).toBe(true); + }); +}); + +describe("resolveLabelingRules deterministic evaluation (#2045)", () => { + const rules = [ + { label: "area:docs", whenPaths: ["docs/**"], titleContains: null, descriptionContains: null }, + { label: "type:wip", whenPaths: [], titleContains: "WIP", descriptionContains: null }, + { label: "needs:migration", whenPaths: ["migrations/**"], titleContains: null, descriptionContains: "schema" }, + ]; + + it("fires a path rule only when a changed path matches", () => { + expect(resolveLabelingRules({ rules, facts: facts({ changedPaths: ["docs/readme.md"] }), autoLabelEnabled: false }).suggest).toEqual(["area:docs"]); + expect(resolveLabelingRules({ rules, facts: facts({ changedPaths: ["src/a.ts"] }), autoLabelEnabled: false }).suggest).toEqual([]); + }); + + it("title match is case-insensitive; a multi-criterion rule needs ALL criteria", () => { + expect(resolveLabelingRules({ rules, facts: facts({ title: "wip: draft" }), autoLabelEnabled: false }).suggest).toEqual(["type:wip"]); + // needs:migration requires BOTH a migrations/** path AND "schema" in the description + expect(resolveLabelingRules({ rules, facts: facts({ changedPaths: ["migrations/001.sql"] }), autoLabelEnabled: false }).suggest).toEqual([]); + expect(resolveLabelingRules({ rules, facts: facts({ changedPaths: ["migrations/001.sql"], description: "adds a schema column" }), autoLabelEnabled: false }).suggest).toEqual(["needs:migration"]); + }); + + it("apply is empty unless autoLabelEnabled, then mirrors suggest", () => { + const f = facts({ changedPaths: ["docs/x.md"], title: "WIP" }); + expect(resolveLabelingRules({ rules, facts: f, autoLabelEnabled: false })).toEqual({ suggest: ["area:docs", "type:wip"], apply: [] }); + expect(resolveLabelingRules({ rules, facts: f, autoLabelEnabled: true })).toEqual({ suggest: ["area:docs", "type:wip"], apply: ["area:docs", "type:wip"] }); + }); + + it("dedupes a label shared by two firing rules, preserving first-seen order", () => { + const dup = [ + { label: "area:x", whenPaths: ["a/**"], titleContains: null, descriptionContains: null }, + { label: "area:x", whenPaths: ["b/**"], titleContains: null, descriptionContains: null }, + ]; + expect(resolveLabelingRules({ rules: dup, facts: facts({ changedPaths: ["a/1", "b/2"] }), autoLabelEnabled: true }).suggest).toEqual(["area:x"]); + }); +}); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 50d84c7557..fc1ebbbb2a 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null } }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [] }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead