diff --git a/.env.example b/.env.example index 5e7b7c7fa1..24836986fa 100644 --- a/.env.example +++ b/.env.example @@ -67,7 +67,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild # history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals # undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety -# looseRange,terminology,todoMarker,magicNumber,conflictMarker +# looseRange,terminology,todoMarker,magicNumber,conflictMarker,commitLint # # Profile defaults: # fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol @@ -78,12 +78,12 @@ GITTENSORY_REVIEW_ENRICHMENT=false # iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink # approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber -# conflictMarker +# conflictMarker,commitLint # deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig # nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity # ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio -# migrationSafety,looseRange,terminology,todoMarker,magicNumber,conflictMarker +# migrationSafety,looseRange,terminology,todoMarker,magicNumber,conflictMarker,commitLint # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index 53ce189ba7..e2f514346d 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -886,6 +886,29 @@ export const REES_ANALYZERS = [ "Structural: an exactly-seven-character marker run at column 0. The ambiguous `=======` separator is not flagged in Markdown/AsciiDoc files, where it is a legitimate section rule.", }, }, + { + name: "commitLint", + title: "Conventional-commit subjects", + category: "quality", + cost: "github-light", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["github-token"], + limits: { + maxCommits: 100, + maxFindings: 25, + }, + docs: { + summary: + "Lints the PR's commit subjects against the Conventional Commits spec and flags non-conforming subjects (bad/absent type, over-long, or empty).", + looksAt: "The PR's commits (one bounded page) — each commit's message subject line.", + reports: + "A short commit-SHA prefix, the subject line, and the failing reason — never full diff/file content.", + network: "Calls the GitHub PR-commits API once, bounded to one page.", + notes: + "Structured-fields-only: reads commit.message subjects, linted independently, never cross-line state. Fail-safe on missing token/fetch error.", + }, + }, ] as const satisfies readonly ReesAnalyzerDoc[]; export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index a5384b9e1c..df8b580a6c 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -998,6 +998,31 @@ "network": "Pure local analyzer. No external network call.", "notes": "Structural: an exactly-seven-character marker run at column 0. The ambiguous `=======` separator is not flagged in Markdown/AsciiDoc files, where it is a legitimate section rule." } + }, + { + "name": "commitLint", + "title": "Conventional-commit subjects", + "category": "quality", + "cost": "github-light", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "github-token" + ], + "limits": { + "maxCommits": 100, + "maxFindings": 25 + }, + "docs": { + "summary": "Lints the PR's commit subjects against the Conventional Commits spec and flags non-conforming subjects (bad/absent type, over-long, or empty).", + "looksAt": "The PR's commits (one bounded page) — each commit's message subject line.", + "reports": "A short commit-SHA prefix, the subject line, and the failing reason — never full diff/file content.", + "network": "Calls the GitHub PR-commits API once, bounded to one page.", + "notes": "Structured-fields-only: reads commit.message subjects, linted independently, never cross-line state. Fail-safe on missing token/fetch error." + } } ] } diff --git a/review-enrichment/src/analyzers/commit-lint.ts b/review-enrichment/src/analyzers/commit-lint.ts new file mode 100644 index 0000000000..e07b4662f8 --- /dev/null +++ b/review-enrichment/src/analyzers/commit-lint.ts @@ -0,0 +1,142 @@ +// Conventional-Commits subject linter (#2021). Reads a PR's commit subjects from the structured GitHub PR-commits +// API and flags each subject that does not conform to the Conventional Commits spec — a wrong/absent type, a +// missing `type: ` structure, an over-long subject, or an empty subject. A house-rule the gate cares about, +// surfaced early. Reads only documented fields (sha, commit.message) and lints each subject independently — no +// cross-commit or cross-line state, and the subject strings come from the API already clean (no diff/comment/ +// string parsing). Bounded to one page of commits (MAX_COMMITS). Fail-safe: no token, a bad repo slug, or a +// fetch error all yield no finding rather than an error. Follows commit-hygiene.ts / commit-signature.ts. +import type { + AnalyzerDiagnostics, + CommitLintFinding, + EnrichRequest, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; + +const GITHUB_API = "https://api.github.com"; +const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const MAX_COMMITS = 100; +const MAX_FINDINGS = 25; +const SHA_PREFIX_LEN = 12; +const MAX_SUBJECT_LEN = 72; // Conventional Commits / git convention soft cap for the subject line. + +// The Conventional Commits type set (the spec's two + the Angular/commitlint conventional preset). A finite, +// documented enum — not free-form text. +const ALLOWED_TYPES = new Set([ + "feat", + "fix", + "docs", + "style", + "refactor", + "perf", + "test", + "build", + "ci", + "chore", + "revert", +]); + +// `type(scope)?!?: summary` — captures the type and asserts the `: ` structure. `!` (breaking-change marker) and a +// parenthesized scope are both optional, per the spec. +const CONVENTIONAL_RE = /^([a-zA-Z]+)(?:\([^)]*\))?!?:\s+\S/; + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + +/** The slice of a GitHub PR-commit list item this analyzer reads. */ +interface CommitListItem { + sha?: string; + commit?: { message?: string }; +} + +function githubHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; +} + +async function fetchPrCommits( + owner: string, + repo: string, + prNumber: number, + headers: Record, + fetchFn: typeof fetch, + signal: AbortSignal | undefined, + options: Pick, +): Promise { + const url = + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/pulls/` + + `${encodeURIComponent(String(prNumber))}/commits?per_page=${MAX_COMMITS}`; + const fetchOptions = { + endpointCategory: "github-pr-commits", + headers, + signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "commit-lint", + subcall: "github-pr-commits", + maxBytes: 512 * 1024, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + return response.ok && Array.isArray(response.data) ? response.data : null; +} + +/** Lint one commit subject against the Conventional Commits spec. Returns the failing reason, or null when the + * subject conforms. `empty` and `too-long` take priority over structural checks. Pure. */ +export function lintSubject(subject: string): CommitLintFinding["reason"] | null { + const trimmed = subject.trim(); + if (!trimmed) return "empty"; + if (trimmed.length > MAX_SUBJECT_LEN) return "too-long"; + const match = CONVENTIONAL_RE.exec(trimmed); + if (!match) return "missing-colon"; + if (!ALLOWED_TYPES.has(match[1]!.toLowerCase())) return "bad-type"; + return null; +} + +/** Pure reduction: a PR's commit list → conventional-commit lint findings, in list order, each independent. + * Bounded by maxFindings. */ +export function analyzeCommitSubjects( + commits: CommitListItem[], + maxFindings = MAX_FINDINGS, +): CommitLintFinding[] { + const findings: CommitLintFinding[] = []; + for (const item of commits) { + if (findings.length >= maxFindings) break; + const sha = item.sha; + if (!sha) continue; + const subject = (item.commit?.message ?? "").split("\n")[0] ?? ""; + const reason = lintSubject(subject); + if (reason) { + findings.push({ sha: sha.slice(0, SHA_PREFIX_LEN), subject: subject.trim().slice(0, 120), reason }); + } + } + return findings; +} + +/** Analyzer entrypoint: a PR's commit subjects → conventional-commit lint findings. Fail-safe — no token, a bad + * repo slug, or a fetch error all yield no finding rather than an error. */ +export async function scanCommitLint( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const { repoFullName, githubToken, prNumber } = req; + if (!githubToken) return []; + const parts = repoFullName.split("/"); + const owner = parts[0]; + const repo = parts[1]; + if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + + const headers = githubHeaders(githubToken); + const commits = await fetchPrCommits(owner, repo, prNumber, headers, fetchFn, options.signal, options); + if (!commits) return []; + + return analyzeCommitSubjects(commits); +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index d83b3b7e3b..76da147c3f 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -29,6 +29,7 @@ import { scanMigrationSafety } from "./migration-safety.js"; import { scanLooseRanges } from "./loose-range.js"; import { scanMagicNumbers } from "./magic-number.js"; import { scanConflictMarkers } from "./conflict-marker.js"; +import { scanCommitLint } from "./commit-lint.js"; import { scanTerminology } from "./terminology.js"; import { scanTodoMarker } from "./todo-marker.js"; import { scanTyposquat } from "./typosquat.js"; @@ -916,6 +917,48 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req) => scanConflictMarkers(req), }), + descriptor({ + name: "commitLint", + title: "Conventional-commit subjects", + category: "quality", + cost: "github-light", + defaultEnabled: true, + requires: ["github-token"], + limits: { maxCommits: 100, maxFindings: 25 }, + docs: { + summary: + "Lints the PR's commit subjects against the Conventional Commits spec and flags non-conforming subjects (bad/absent type, over-long, or empty).", + looksAt: "The PR's commits (one bounded page) — each commit's message subject line.", + reports: "A short commit-SHA prefix, the subject line, and the failing reason — never full diff/file content.", + network: "Calls the GitHub PR-commits API once, bounded to one page.", + notes: + "Structured-fields-only: reads commit.message subjects, linted independently, never cross-line state. Fail-safe on missing token/fetch error.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const explain = (reason: (typeof findings)[number]["reason"]): string => { + switch (reason) { + case "empty": + return "empty subject"; + case "too-long": + return "subject exceeds 72 characters"; + case "missing-colon": + return "not `type: summary` — missing the Conventional-Commit structure"; + case "bad-type": + return "unrecognized commit type"; + } + }; + const lines = ["### Non-conforming commit subjects (Conventional Commits)"]; + for (const item of findings) { + lines.push( + `- ${helpers.safeCodeSpan(item.sha)} — ${explain(item.reason)}: ${helpers.promptText(item.subject)}`, + ); + } + return lines; + }, + run: (req, { signal, analysis, diagnostics }) => + scanCommitLint(req, fetch, { signal, analysis, diagnostics }), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index d483299f89..38ed8483aa 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -460,6 +460,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("todoMarker", findings.todoMarker)); lines.push(...renderDescriptorSection("magicNumber", findings.magicNumber)); lines.push(...renderDescriptorSection("conflictMarker", findings.conflictMarker)); + lines.push(...renderDescriptorSection("commitLint", findings.commitLint)); if (!lines.length) return { promptSection: "", systemSuffix: "" }; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 53f803ac82..aa2b7da70d 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -455,6 +455,14 @@ export interface ConflictMarkerFinding { marker: "<<<<<<<" | "|||||||" | "=======" | ">>>>>>>"; } +/** A PR commit subject that does not conform to the Conventional Commits spec (#2021, part of #1499). Reports a + * short SHA prefix, the subject, and the failing reason — never author/email. */ +export interface CommitLintFinding { + sha: string; + subject: string; + reason: "bad-type" | "missing-colon" | "too-long" | "empty"; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -492,6 +500,7 @@ export interface BriefFindings { todoMarker?: TodoMarkerFinding[]; magicNumber?: MagicNumberFinding[]; conflictMarker?: ConflictMarkerFinding[]; + commitLint?: CommitLintFinding[]; } /** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 0a5d1cac94..edd5354d4d 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -45,6 +45,7 @@ const EXPECTED_ANALYZERS = [ "todoMarker", "magicNumber", "conflictMarker", + "commitLint", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/commit-lint.test.ts b/review-enrichment/test/commit-lint.test.ts new file mode 100644 index 0000000000..de8c096783 --- /dev/null +++ b/review-enrichment/test/commit-lint.test.ts @@ -0,0 +1,85 @@ +// Units for the conventional-commit subject linter (#2021). Own file (not enrichment.test.ts) so concurrent +// analyzer PRs don't collide. Network is mocked via an injected fetch. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + lintSubject, + analyzeCommitSubjects, + scanCommitLint, +} from "../dist/analyzers/commit-lint.js"; + +const commitsResponse = (subjects) => + async () => + new Response( + JSON.stringify(subjects.map((s, i) => ({ sha: `${i}`.padStart(40, "0"), commit: { message: s } }))), + { status: 200 }, + ); +const req = (overrides = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_test", + ...overrides, +}); + +test("lintSubject: a conforming Conventional-Commit subject passes (with and without scope / breaking marker)", () => { + assert.equal(lintSubject("feat: add retries"), null); + assert.equal(lintSubject("fix(parser): handle nulls"), null); + assert.equal(lintSubject("refactor(core)!: drop legacy path"), null); + assert.equal(lintSubject("chore: bump deps"), null); +}); + +test("lintSubject: flags each non-conforming reason", () => { + assert.equal(lintSubject(""), "empty"); + assert.equal(lintSubject(" "), "empty"); + assert.equal(lintSubject("feat: " + "x".repeat(80)), "too-long"); + assert.equal(lintSubject("just a plain sentence"), "missing-colon"); + assert.equal(lintSubject("Add a feature"), "missing-colon"); + assert.equal(lintSubject("feature: not an allowed type"), "bad-type"); + assert.equal(lintSubject("wip: work in progress"), "bad-type"); +}); + +test("lintSubject: empty and too-long take priority over structural checks", () => { + // An over-long subject is reported as too-long even if it would also be a bad type. + assert.equal(lintSubject("nope: " + "y".repeat(80)), "too-long"); +}); + +test("analyzeCommitSubjects: reports only non-conforming subjects with a short sha and truncated subject", () => { + const findings = analyzeCommitSubjects([ + { sha: "a".repeat(40), commit: { message: "feat: good\n\nbody ignored" } }, + { sha: "b".repeat(40), commit: { message: "bad subject here" } }, + { sha: "c".repeat(40), commit: { message: "wip: nope" } }, + ]); + assert.deepEqual(findings, [ + { sha: "bbbbbbbbbbbb", subject: "bad subject here", reason: "missing-colon" }, + { sha: "cccccccccccc", subject: "wip: nope", reason: "bad-type" }, + ]); +}); + +test("analyzeCommitSubjects: lints the SUBJECT line only, ignoring the commit body", () => { + const findings = analyzeCommitSubjects([ + { sha: "d".repeat(40), commit: { message: "feat: ok\n\nnot: a subject line" } }, + ]); + assert.deepEqual(findings, []); +}); + +test("analyzeCommitSubjects: enforces the maxFindings cap and skips items with no sha", () => { + const items = Array.from({ length: 30 }, (_, i) => ({ sha: `${i}`.padStart(40, "0"), commit: { message: "nope" } })); + assert.equal(analyzeCommitSubjects(items, 5).length, 5); + assert.deepEqual(analyzeCommitSubjects([{ commit: { message: "nope" } }]), []); +}); + +test("scanCommitLint: end-to-end flags non-conforming subjects from the fetched commit list", async () => { + const findings = await scanCommitLint(req(), commitsResponse(["feat: fine", "broken subject"])); + assert.equal(findings.length, 1); + assert.equal(findings[0].reason, "missing-colon"); + assert.equal(findings[0].subject, "broken subject"); +}); + +test("scanCommitLint: fail-safe — no token, a bad repo slug, or a fetch error yields no finding", async () => { + const good = commitsResponse(["broken"]); + assert.deepEqual(await scanCommitLint(req({ githubToken: undefined }), good), []); + assert.deepEqual(await scanCommitLint(req({ repoFullName: "octo/repo/extra" }), good), []); + assert.deepEqual(await scanCommitLint(req({ repoFullName: "bad slug!/x" }), good), []); + const err = async () => new Response("nope", { status: 500 }); + assert.deepEqual(await scanCommitLint(req(), err), []); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 4191c2d16b..f1f7d44de3 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -39,6 +39,7 @@ export const REES_ANALYZER_NAMES = [ "todoMarker", "magicNumber", "conflictMarker", + "commitLint", ] as const; export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number]; diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index 606be32c8e..9f4f57c51a 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -626,7 +626,7 @@ describe("resolveReesAnalyzers", () => { resolveReesAnalyzers( env({ REES_ANALYZERS: - "dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,conflictMarker", + "dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,conflictMarker,commitLint", }), ), ).toEqual([ @@ -664,6 +664,7 @@ describe("resolveReesAnalyzers", () => { "terminology", "todoMarker", "conflictMarker", + "commitLint", ]); });