diff --git a/package.json b/package.json index 4d99fd7493..338b58f9af 100644 --- a/package.json +++ b/package.json @@ -89,6 +89,7 @@ "server-manifest:check": "node --experimental-strip-types scripts/check-server-manifest.ts", "dead-source-files:check": "node --experimental-strip-types scripts/check-dead-source-files.ts", "dead-exports:check": "node --experimental-strip-types scripts/check-dead-exports.ts", + "maintainer-associations:check": "node --experimental-strip-types scripts/check-maintainer-association-copies.ts", "regate-sort-key:check": "node --experimental-strip-types scripts/check-regate-sort-key.ts", "command-redelivery-guards:check": "node --experimental-strip-types scripts/check-command-redelivery-guards.ts", "dispatch-gate-reasons:check": "node --experimental-strip-types scripts/check-dispatch-gate-reasons.ts", @@ -140,7 +141,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run regate-sort-key:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/packages/loopover-engine/src/scoring/pending-pr-scenarios.ts b/packages/loopover-engine/src/scoring/pending-pr-scenarios.ts index d2c7e70fae..edfe0db387 100644 --- a/packages/loopover-engine/src/scoring/pending-pr-scenarios.ts +++ b/packages/loopover-engine/src/scoring/pending-pr-scenarios.ts @@ -1,5 +1,6 @@ import type { CheckSummaryRecord, PullRequestRecord, PullRequestReviewRecord, RoleContext } from "./types.js"; import type { ScorePreviewInput } from "./preview.js"; +import { isMaintainerAuthorAssociation } from "../settings/author-association.js"; // Deterministic subset of the backend's `src/scoring/pending-pr-scenarios.ts` (#2282): the pure // classification/detection logic that only needs already-fetched PR/review/check records. The two @@ -43,14 +44,6 @@ const STALE_DAYS = 14; // "draft-js" from matching. Trailing \s* lets the same pattern also strip the marker for dedup keys. export const DRAFT_TITLE_PATTERN = /^(?:\[\s*draft\s*\]|draft(?:\s*:|\s+-))\s*/i; -// Mirrors `src/github/commands.ts`'s `isMaintainerAssociation`/`MAINTAINER_ASSOCIATIONS` exactly. Duplicated -// here (rather than imported) because this package cannot reach into `src/`; keep the two in sync by hand. -const MAINTAINER_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); - -function isMaintainerAssociation(association: string | null | undefined): boolean { - return Boolean(association && MAINTAINER_ASSOCIATIONS.has(association)); -} - export function detectPendingPrScenario(args: { login: string; repoFullName: string; @@ -147,7 +140,7 @@ export function classifyOpenPullRequest(args: { reasons.push("Maintainer-lane context for this repo; not counted as outside-contributor pending reward work."); return { repoFullName: args.pr.repoFullName, number: args.pr.number, title: args.pr.title, classification: "maintainer_lane", reasons }; } - if (isMaintainerAssociation(args.pr.authorAssociation)) { + if (isMaintainerAuthorAssociation(args.pr.authorAssociation)) { reasons.push("Author association indicates maintainer-authored work."); return { repoFullName: args.pr.repoFullName, number: args.pr.number, title: args.pr.title, classification: "maintainer_lane", reasons }; } diff --git a/packages/loopover-engine/src/signals/engine.ts b/packages/loopover-engine/src/signals/engine.ts index fce9b8c0e3..e94beb9c92 100644 --- a/packages/loopover-engine/src/signals/engine.ts +++ b/packages/loopover-engine/src/signals/engine.ts @@ -45,6 +45,7 @@ import { diffFilePriority } from "../review/diff-file-priority.js"; import type { ImprovementBand, StructuralImprovementAssessment } from "../../../../src/signals/improvement.js"; import type { ImprovementMagnitude } from "../../../../src/services/ai-review.js"; import type { SlopBand } from "./slop.js"; +import { MAINTAINER_AUTHOR_ASSOCIATIONS, isMaintainerAuthorAssociation } from "../settings/author-association.js"; export type ParticipationLane = "direct_pr" | "issue_discovery" | "split" | "inactive" | "unknown"; export type SignalFinding = AdvisoryFinding; @@ -234,7 +235,7 @@ const MAINTAINER_WIP_LABELS = new Set([ /** True iff a maintainer-authored issue is labelled as the maintainer's own in-progress/internal work. */ function isMaintainerWipIssue(issue: IssueRecord): boolean { - return isMaintainerAssociation(issue.authorAssociation) && issue.labels.some((label) => MAINTAINER_WIP_LABELS.has(label.toLowerCase().trim())); + return isMaintainerAuthorAssociation(issue.authorAssociation) && issue.labels.some((label) => MAINTAINER_WIP_LABELS.has(label.toLowerCase().trim())); } /** @@ -243,7 +244,7 @@ function isMaintainerWipIssue(issue: IssueRecord): boolean { * exact condition the issue-watch monitor (#699 path B) notifies subscribers about. */ export function isGrabbableHighMultiplierIssue(issue: IssueRecord): boolean { - return issue.state === "open" && isMaintainerAssociation(issue.authorAssociation) && !isMaintainerWipIssue(issue); + return issue.state === "open" && isMaintainerAuthorAssociation(issue.authorAssociation) && !isMaintainerWipIssue(issue); } export type ContributorFit = { @@ -965,7 +966,7 @@ export function buildQueueHealth( const unlinkedPullRequests = openPullRequests.filter((pr) => pr.linkedIssues.length === 0); const stalePullRequests = openPullRequests.filter((pr) => daysSince(pr.updatedAt ?? pr.createdAt) >= 14); const draftPullRequests = openPullRequests.filter((pr) => pr.isDraft); - const maintainerAuthoredPullRequests = openPullRequests.filter((pr) => isMaintainerAssociation(pr.authorAssociation)); + const maintainerAuthoredPullRequests = openPullRequests.filter((pr) => isMaintainerAuthorAssociation(pr.authorAssociation)); const slopFlaggedPullRequests = openPullRequests.filter( (pr) => pr.slopBand === "elevated" || pr.slopBand === "high", ).length; @@ -1261,7 +1262,7 @@ export function buildContributorProfile( authoredPullRequests.filter((pr) => pr.state === "open" && pr.linkedIssues.length === 0).length, matchingStats.reduce((sum, stat) => sum + stat.unlinkedPullRequests, 0), ); - const maintainerAssociatedPullRequests = authoredPullRequests.filter((pr) => isMaintainerAssociation(pr.authorAssociation)).length; + const maintainerAssociatedPullRequests = authoredPullRequests.filter((pr) => isMaintainerAuthorAssociation(pr.authorAssociation)).length; const pullRequestCount = Math.max(authoredPullRequests.length, statPullRequests); const mergedPullRequestCount = Math.max(mergedPullRequests.length, statMergedPullRequests); const issueCount = Math.max(authoredIssues.length, statIssues); @@ -1297,7 +1298,7 @@ function buildGittensorContributorProfile( /* v8 ignore next -- Official Gittensor snapshots normally include the canonical GitHub login; request-login fallback protects legacy rows. */ const matchingStats = repoStats.filter((stat) => sameLogin(stat.login, snapshot.githubUsername) || sameLogin(stat.login, login)); const unlinkedOpenPullRequests = matchingStats.reduce((sum, stat) => sum + stat.unlinkedPullRequests, 0); - const maintainerAssociatedPullRequests = pullRequests.filter((pr) => sameLogin(pr.authorLogin, login) && isMaintainerAssociation(pr.authorAssociation)).length; + const maintainerAssociatedPullRequests = pullRequests.filter((pr) => sameLogin(pr.authorLogin, login) && isMaintainerAuthorAssociation(pr.authorAssociation)).length; const reposTouched = snapshot.repositories .filter((repo) => repo.pullRequests + repo.openIssues + repo.closedIssues > 0) .map((repo) => repo.repoFullName) @@ -1477,7 +1478,7 @@ export function buildContributorOpportunities( : quality?.status === "hold" ? -15 : 0; - const maintainerAuthored = isMaintainerAssociation(issue.authorAssociation); + const maintainerAuthored = isMaintainerAuthorAssociation(issue.authorAssociation); const maintainerWip = isMaintainerWipIssue(issue); const multiplierTier: ContributorOpportunity["multiplierTier"] = maintainerAuthored ? "maintainer_created" : "community"; const availability: ContributorOpportunity["availability"] = maintainerWip ? "maintainer_wip" : "ready"; @@ -1637,7 +1638,7 @@ export function buildRoleContext(args: { role = "collaborator"; source = "github_association"; /* v8 ignore next -- strongestAssociation resolves maintainer associations before this guard; it protects malformed mixed association rows. */ - } else if (authoredAssociations.some(isMaintainerAssociation)) { + } else if (authoredAssociations.some(isMaintainerAuthorAssociation)) { role = "repo_maintainer"; source = "github_association"; } else if (touchedByOfficial) { @@ -1887,8 +1888,8 @@ export function buildContributorReconciliationReport(args: { const maintainerLane = sameLogin(repo?.owner, args.login) || sameLogin(repoOwner, args.login) || - args.pullRequests.some((pr) => sameRepo(pr.repoFullName, repoFullName) && sameLogin(pr.authorLogin, args.login) && isMaintainerAssociation(pr.authorAssociation)) || - args.issues.some((issue) => sameRepo(issue.repoFullName, repoFullName) && sameLogin(issue.authorLogin, args.login) && isMaintainerAssociation(issue.authorAssociation)); + args.pullRequests.some((pr) => sameRepo(pr.repoFullName, repoFullName) && sameLogin(pr.authorLogin, args.login) && isMaintainerAuthorAssociation(pr.authorAssociation)) || + args.issues.some((issue) => sameRepo(issue.repoFullName, repoFullName) && sameLogin(issue.authorLogin, args.login) && isMaintainerAuthorAssociation(issue.authorAssociation)); return { repoFullName, maintainerLane, @@ -2081,7 +2082,7 @@ function normalizeRecentMergedOutcome( // lane so the record is excluded from outside-contributor statistics rather than silently // inflating the outside-contributor merge rate with unclassifiable data. const knownOutsider = association === "NONE" || association === "CONTRIBUTOR" || association === "FIRST_TIME_CONTRIBUTOR" || association === "FIRST_TIMER"; - const maintainerLane = isMaintainerAssociation(association) || !knownOutsider; + const maintainerLane = isMaintainerAuthorAssociation(association) || !knownOutsider; return { number: record.number, bucket: "merged", @@ -2153,7 +2154,7 @@ export function buildRepoOutcomePatterns(args: { bucket, decided: merged || closedUnmerged, merged, - maintainerLane: isMaintainerAssociation(pr.authorAssociation), + maintainerLane: isMaintainerAuthorAssociation(pr.authorAssociation), linked: pr.linkedIssues.length > 0 || (mergedDetail?.linkedIssues.length ?? 0) > 0, labels: [...new Set([...pr.labels, ...(mergedDetail?.labels ?? [])])].sort(), filePaths, @@ -2581,7 +2582,7 @@ export function buildPreflightResult( // snapshot; "inactive" (zero emission share) is unambiguous either way -- it is only reachable from real // synced data. const laneUnavailable = (lane.lane === "unknown" && registryEverSynced) || lane.lane === "inactive"; - const maintainerAuthored = isMaintainerAssociation(input.authorAssociation); + const maintainerAuthored = isMaintainerAuthorAssociation(input.authorAssociation); if (laneUnavailable) { findings.push({ code: "lane_not_recommended", @@ -2738,7 +2739,7 @@ export function buildMaintainerPacket( .map((pr) => { const reasons = [ ...(pr.linkedIssues.length === 0 ? ["Missing linked issue context."] : []), - ...(isMaintainerAssociation(pr.authorAssociation) ? ["Author has maintainer association."] : []), + ...(isMaintainerAuthorAssociation(pr.authorAssociation) ? ["Author has maintainer association."] : []), ...(collisions.clusters.some((cluster) => cluster.items.some((item) => item.type === "pull_request" && item.number === pr.number)) ? ["Potential overlap with other open work."] : []), @@ -3019,7 +3020,7 @@ export function buildIssueQualityReport( const linkage = buildIssueLinkageRecord(issue, lifecycleEntry, linkedPrs, linkedMergedPrs); // #186: maintainer-authored issues must not silently read as "ready" for outside contributors — // always warn to confirm intent, and downgrade ones labelled as the maintainer's own in-progress work. - const maintainerAuthored = isMaintainerAssociation(issue.authorAssociation); + const maintainerAuthored = isMaintainerAuthorAssociation(issue.authorAssociation); const maintainerWip = isMaintainerWipIssue(issue); const reasons = [ ...(bodyLength >= 200 ? ["Issue has enough body detail to evaluate."] : []), @@ -5238,16 +5239,15 @@ function outcomeFailurePatterns(history: ContributorOutcomeHistory): OutcomePatt } function strongestAssociation(values: string[]): string | undefined { - for (const association of ["OWNER", "MEMBER", "COLLABORATOR"]) { + // Iterates MAINTAINER_AUTHOR_ASSOCIATIONS in DECLARATION order, which is its precedence order + // (OWNER > MEMBER > COLLABORATOR). This is the one consumer that depends on that ordering rather than + // on membership alone, so it is stated here rather than left implicit in a re-typed literal. + for (const association of MAINTAINER_AUTHOR_ASSOCIATIONS) { if (values.includes(association)) return association; } return values[0]; } -function isMaintainerAssociation(value: string | null | undefined): boolean { - return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"; -} - function sameLogin(value: string | null | undefined, login: string): boolean { return value?.toLowerCase() === login.toLowerCase(); } diff --git a/packages/loopover-engine/src/signals/issue-quality-report.ts b/packages/loopover-engine/src/signals/issue-quality-report.ts index 7044b6ca15..077493f0d7 100644 --- a/packages/loopover-engine/src/signals/issue-quality-report.ts +++ b/packages/loopover-engine/src/signals/issue-quality-report.ts @@ -22,6 +22,7 @@ import type { RepositoryRecord, } from "../types/predicted-gate-types.js"; import { nowIso } from "../utils/json.js"; +import { isMaintainerAuthorAssociation } from "../settings/author-association.js"; import { bountyIssueKey, buildCollisionReport, @@ -77,16 +78,12 @@ function daysSince(value: string | null | undefined): number { return Math.floor((Date.now() - parsed) / 86_400_000); } -function isMaintainerAssociation(value: string | null | undefined): boolean { - return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"; -} - function sameLogin(value: string | null | undefined, login: string): boolean { return value?.toLowerCase() === login.toLowerCase(); } function isMaintainerWipIssue(issue: IssueRecord): boolean { - return isMaintainerAssociation(issue.authorAssociation) && issue.labels.some((label) => MAINTAINER_WIP_LABELS.has(label.toLowerCase().trim())); + return isMaintainerAuthorAssociation(issue.authorAssociation) && issue.labels.some((label) => MAINTAINER_WIP_LABELS.has(label.toLowerCase().trim())); } function indexPullRequestsByLinkedIssue(pullRequests: T[]): Map { @@ -253,7 +250,7 @@ export function buildIssueQualityReport( const bounty = bountyByIssue.get(bountyIssueKey(fullName, issue.number)) ?? null; const bountyLifecycle: BountyLifecycle | null = bounty ? classifyBountyLifecycle(bounty, issue) : null; const linkedWorkCount = linkedPrs.length + linkedMergedPrs.length + issue.linkedPrs.length; - const maintainerAuthored = isMaintainerAssociation(issue.authorAssociation); + const maintainerAuthored = isMaintainerAuthorAssociation(issue.authorAssociation); const maintainerWip = isMaintainerWipIssue(issue); const reasons = [ ...(bodyLength >= 200 ? ["Issue has enough body detail to evaluate."] : []), diff --git a/packages/loopover-engine/src/signals/predicted-gate-engine.ts b/packages/loopover-engine/src/signals/predicted-gate-engine.ts index 4a776ec664..de3e5f35ef 100644 --- a/packages/loopover-engine/src/signals/predicted-gate-engine.ts +++ b/packages/loopover-engine/src/signals/predicted-gate-engine.ts @@ -21,6 +21,7 @@ import { nowIso } from "../utils/json.js"; import { PREFLIGHT_LIMITS } from "./preflight-limits.js"; import { hasValidationNote, isCodeFile, isTestPath } from "./test-evidence.js"; import { diffFilePriority } from "../review/diff-file-priority.js"; +import { isMaintainerAuthorAssociation } from "../settings/author-association.js"; export type { IssueQualityReport, CollisionReport, CollisionCluster } from "../types/predicted-gate-types.js"; @@ -238,7 +239,7 @@ export function buildQueueHealth( const unlinkedPullRequests = openPullRequests.filter((pr) => pr.linkedIssues.length === 0); const stalePullRequests = openPullRequests.filter((pr) => daysSince(pr.updatedAt ?? pr.createdAt) >= 14); const draftPullRequests = openPullRequests.filter((pr) => pr.isDraft); - const maintainerAuthoredPullRequests = openPullRequests.filter((pr) => isMaintainerAssociation(pr.authorAssociation)); + const maintainerAuthoredPullRequests = openPullRequests.filter((pr) => isMaintainerAuthorAssociation(pr.authorAssociation)); const slopFlaggedPullRequests = openPullRequests.filter( (pr) => pr.slopBand === "elevated" || pr.slopBand === "high", ).length; @@ -398,7 +399,7 @@ export function buildPreflightResult( // snapshot; "inactive" (zero emission share) is unambiguous either way -- it is only reachable from real // synced data. const laneUnavailable = (lane.lane === "unknown" && registryEverSynced) || lane.lane === "inactive"; - const maintainerAuthored = isMaintainerAssociation(input.authorAssociation); + const maintainerAuthored = isMaintainerAuthorAssociation(input.authorAssociation); if (laneUnavailable) { findings.push({ code: "lane_not_recommended", @@ -984,10 +985,6 @@ function extractLinkedIssueNumbers(text: string, repoFullName: string): number[] return extractLinkedIssueNumbersWithOverflow(text, repoFullName).numbers; } -function isMaintainerAssociation(value: string | null | undefined): boolean { - return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"; -} - function sameLogin(value: string | null | undefined, login: string): boolean { return value?.toLowerCase() === login.toLowerCase(); } diff --git a/scripts/check-maintainer-association-copies.ts b/scripts/check-maintainer-association-copies.ts new file mode 100644 index 0000000000..bee39e19f6 --- /dev/null +++ b/scripts/check-maintainer-association-copies.ts @@ -0,0 +1,129 @@ +#!/usr/bin/env node +// Guards the maintainer-association vocabulary against re-duplication (#9860). +// +// "Does this author have standing in the repo?" is a security-relevant predicate, and it had been +// re-typed as a bare `["OWNER", "MEMBER", "COLLABORATOR"]` literal in SEVEN places: the mention-command +// gate, the settings preview, the advisory rules, local-branch's owner check, the gate-advisory engine +// TWIN, the engine's pending-PR scenarios (whose own comment admitted "keep the two in sync by hand"), +// and the contributor evidence graph. Two of them had already drifted -- one lower-cased its comparison, +// one was case-sensitive where the others were not -- which is exactly how a copy stops being a copy. +// +// They now all read `MAINTAINER_AUTHOR_ASSOCIATIONS` / `isMaintainerAuthorAssociation` from +// packages/loopover-engine/src/settings/author-association.ts. This check exists so the eighth copy fails +// CI instead of shipping: per #9860's own bar, compute the fact rather than remember it. A checker that +// merely listed the known copies would be the same hand-maintained list one level up. +// +// WHAT IS ALLOWED. The definition module itself, obviously. Also `z.enum([...])` declarations, which spell +// the FULL eight-value GitHub vocabulary rather than the three-value maintainer subset -- those are a wire +// schema, not this predicate, and conflating them would be its own mistake. +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join, relative } from "node:path"; +import { fileURLToPath, URL } from "node:url"; + +const REPO_ROOT = fileURLToPath(new URL("..", import.meta.url)); + +/** Where the predicate is allowed to be spelled out. */ +export const DEFINITION_FILE = "packages/loopover-engine/src/settings/author-association.ts"; + +const SCAN_ROOTS = ["src", "packages/loopover-engine/src", "packages/loopover-contract/src"]; +const SOURCE_PATTERN = /\.tsx?$/; +const EXCLUDED_SEGMENT = /(^|\/)(node_modules|dist|dist-test|coverage)(\/|$)/; + +/** Each member of the three-value maintainer subset, quoted, in any casing of quote. Detection is + * "all three appear close together", NOT "they appear comma-separated" -- the first version of this check + * only matched array literals, and review caught that it therefore missed + * `value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"` sitting in engine.ts with fourteen + * call sites. A guard that only recognises one spelling of the thing it is guarding is a list again. */ +const OWNER_LITERAL = /["']OWNER["']/; +const MEMBER_LITERAL = /["']MEMBER["']/; +const COLLABORATOR_LITERAL = /["']COLLABORATOR["']/; + +/** How many consecutive lines are considered together. A formatter will break a long `||` chain or a + * multi-entry array across lines, so a single-line scan would miss the same predicate purely on width. */ +const WINDOW_LINES = 4; + +function namesAllThree(text: string): boolean { + return OWNER_LITERAL.test(text) && MEMBER_LITERAL.test(text) && COLLABORATOR_LITERAL.test(text); +} + +/** A full eight-value GitHub association vocabulary -- a wire schema, not this predicate. */ +const FULL_VOCABULARY = /["']CONTRIBUTOR["']|["']FIRST_TIME(?:R|_CONTRIBUTOR)["']|["']MANNEQUIN["']/; + +/** + * Sites that name all three associations but are NOT this predicate, with the reason. + * + * This is an exception list, not a list of known copies -- the difference matters. It records places whose + * SEMANTICS genuinely differ, each of which would be wrong to "fix"; it does not record duplicates awaiting + * migration. Mirrors check-regate-sort-key.ts's ALLOWED_OMISSIONS, and an entry has to say why. + */ +export const ALLOWED_DISTINCT_SEMANTICS: ReadonlyMap = new Map([ + [ + "packages/loopover-engine/src/settings/command-authorization.ts", + "Not the maintainer predicate: it maps OWNER/MEMBER to the `maintainer` role and COLLABORATOR to a SEPARATE `collaborator` role. Collapsing the two would silently grant collaborators maintainer-only commands.", + ], +]); + +export type AssociationCopy = { file: string; line: number; snippet: string }; + +function listSourceFiles(root: string): string[] { + const absolute = join(REPO_ROOT, root); + try { + if (!statSync(absolute).isDirectory()) return []; + } catch { + return []; + } + return readdirSync(absolute, { recursive: true }) + .map(String) + .filter((entry) => SOURCE_PATTERN.test(entry) && !EXCLUDED_SEGMENT.test(entry)) + .map((entry) => `${root}/${entry}`); +} + +/** Pure over its inputs so the check is testable without touching the tree. */ +export function findMaintainerAssociationCopies( + options: { roots?: readonly string[]; readFile?: (file: string) => string; listFiles?: (root: string) => string[]; allowed?: ReadonlyMap } = {}, +): AssociationCopy[] { + const { roots = SCAN_ROOTS, listFiles = listSourceFiles, readFile = (file: string) => readFileSync(join(REPO_ROOT, file), "utf8"), allowed = ALLOWED_DISTINCT_SEMANTICS } = options; + const copies: AssociationCopy[] = []; + for (const root of roots) { + for (const file of listFiles(root)) { + if (relative(DEFINITION_FILE, file) === "") continue; + const lines = readFile(file).split("\n"); + let reportedThrough = -1; + for (const [index] of lines.entries()) { + if (index <= reportedThrough) continue; // one report per occurrence, not one per overlapping window + const window = lines.slice(index, index + WINDOW_LINES).join("\n"); + if (!namesAllThree(window)) continue; + // A window that also names the wider vocabulary is a schema enum, not the maintainer predicate. + if (FULL_VOCABULARY.test(window)) continue; + if (allowed.has(file)) continue; + // Report the line that actually names one of them, not the window's first line -- a window can open + // on a blank line or a closing brace, which tells a reader nothing about what was found. + const offset = lines.slice(index, index + WINDOW_LINES).findIndex((l) => OWNER_LITERAL.test(l) || MEMBER_LITERAL.test(l) || COLLABORATOR_LITERAL.test(l)); + const at = index + (offset === -1 ? 0 : offset); + copies.push({ file, line: at + 1, snippet: (lines[at] ?? "").trim().slice(0, 120) }); + reportedThrough = index + WINDOW_LINES - 1; + } + } + } + return copies.sort((a, b) => (a.file === b.file ? a.line - b.line : a.file.localeCompare(b.file))); +} + +function main(): void { + const copies = findMaintainerAssociationCopies(); + if (copies.length === 0) { + console.log("maintainer-association vocabulary: OK — one definition, no re-typed copies."); + return; + } + console.error(`Found ${copies.length} re-typed maintainer-association literal(s) (#9860):`); + for (const copy of copies) console.error(` ${copy.file}:${copy.line} — ${copy.snippet}`); + console.error(""); + console.error(`Import from ${DEFINITION_FILE} instead:`); + console.error(" isMaintainerAuthorAssociation(association) — membership, case-insensitive"); + console.error(" MAINTAINER_AUTHOR_ASSOCIATIONS — the list, in precedence order"); + console.error(" classifyAuthorAssociation(association) — maintainer | contributor | unknown"); + console.error(""); + console.error("src/ reaches it via src/github/author-association.ts, which re-exports the engine's copy."); + process.exitCode = 1; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/src/services/contributor-evidence-graph.ts b/src/services/contributor-evidence-graph.ts index af72760c39..a3329ca0ef 100644 --- a/src/services/contributor-evidence-graph.ts +++ b/src/services/contributor-evidence-graph.ts @@ -9,6 +9,7 @@ import type { RepoSyncStateRecord, } from "../types"; import { nowIso } from "../utils/json"; +import { isMaintainerAuthorAssociation } from "../github/author-association"; export const CONTRIBUTOR_EVIDENCE_GRAPH_SIGNAL = "contributor-evidence-graph"; const CONTRIBUTOR_EVIDENCE_GRAPH_VERSION = 1; @@ -481,7 +482,7 @@ function fallbackRoleContext( function maintainerAssociationVisible(pullRequests: PullRequestRecord[], issues: IssueRecord[]): boolean { return [...pullRequests.map((pr) => pr.authorAssociation), ...issues.map((issue) => issue.authorAssociation)].some((association) => - ["OWNER", "MEMBER", "COLLABORATOR"].includes((association ?? "").toUpperCase()), + isMaintainerAuthorAssociation(association), ); } diff --git a/src/services/control-panel-roles.ts b/src/services/control-panel-roles.ts index f906613fe4..be2aba5d34 100644 --- a/src/services/control-panel-roles.ts +++ b/src/services/control-panel-roles.ts @@ -3,6 +3,7 @@ import { getFreshOfficialMinerDetection, getRepository, listAllPullRequests, lis import type { ControlPanelRoleCard, ControlPanelRoleName, ControlPanelRoleSummary, InstallationRecord, PullRequestRecord, RepositoryRecord } from "../types"; import { nowIso } from "../utils/json"; import { PUBLIC_LOCAL_PATH_SCRUB_PATTERN, publicTokenPattern } from "../signals/redaction"; +import { isMaintainerAuthorAssociation } from "../github/author-association"; export type RoleSummaryInputs = { login: string; @@ -108,7 +109,7 @@ function resolveAccountRepoScope(args: RoleSummaryInputs): { (sameLogin(repo.owner, args.login) || (repo.installationId !== undefined && repo.installationId !== null && accountInstallationIds.has(repo.installationId))), ); const maintainerRepoNames = args.pullRequests - .filter((pull) => sameLogin(pull.authorLogin, args.login) && isMaintainerAssociation(pull.authorAssociation)) + .filter((pull) => sameLogin(pull.authorLogin, args.login) && isMaintainerAuthorAssociation(pull.authorAssociation)) .map((pull) => pull.repoFullName) .filter((repoFullName) => installedRepos.some((repo) => sameRepo(repo.fullName, repoFullName))); return { installedRepos, accountInstallations, ownedInstalledRepos, maintainerRepoNames }; @@ -294,10 +295,6 @@ function sameRepo(left: string | null | undefined, right: string | null | undefi return Boolean(left && right && left.toLowerCase() === right.toLowerCase()); } -function isMaintainerAssociation(value: string | null | undefined): boolean { - return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"; -} - export function sanitizeRoleText(value: string): string { const redacted = value .replace(PUBLIC_LOCAL_PATH_SCRUB_PATTERN, "") diff --git a/src/services/outcome-calibration.ts b/src/services/outcome-calibration.ts index e53793fbb5..5ce4164c87 100644 --- a/src/services/outcome-calibration.ts +++ b/src/services/outcome-calibration.ts @@ -12,6 +12,7 @@ import { listAgentRecommendationOutcomes, listPullRequests } from "../db/reposit import type { SlopBand } from "../signals/slop"; import type { AgentRecommendationOutcomeRecord, AgentRecommendationOutcomeState, PullRequestRecord } from "../types"; import { nowIso } from "../utils/json"; +import { isMaintainerAuthorAssociation } from "../github/author-association"; // The single place the recommendation-outcome states are grouped into positive/negative/pending. Both // aggregators over the agent_recommendation_outcomes ledger — this module and recommendation-quality-report.ts, @@ -74,12 +75,11 @@ function terminalOutcome(pr: PullRequestRecord): "merged" | "closed" | null { return null; } -// Same OWNER/MEMBER/COLLABORATOR classification settings-preview.ts's includeMaintainerAuthors check already -// uses to exclude this population from the public surface by default. -const MAINTAINER_AUTHOR_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); - +// Same classification settings-preview.ts's includeMaintainerAuthors check uses to exclude this +// population from the public surface by default -- now the shared predicate rather than a local copy +// of the list (#9860). function isMaintainerAuthoredPr(pr: PullRequestRecord): boolean { - return pr.authorAssociation != null && MAINTAINER_AUTHOR_ASSOCIATIONS.has(pr.authorAssociation); + return isMaintainerAuthorAssociation(pr.authorAssociation); } /** Per-slop-band merge/close calibration over the resolved PRs that carry a slop assessment. Pure. */ diff --git a/src/services/recommendation-outcomes.ts b/src/services/recommendation-outcomes.ts index dff7d9cc02..bc34736456 100644 --- a/src/services/recommendation-outcomes.ts +++ b/src/services/recommendation-outcomes.ts @@ -16,6 +16,7 @@ import type { PullRequestRecord, } from "../types"; import { nowIso } from "../utils/json"; +import { isMaintainerAuthorAssociation } from "../github/author-association"; const DEFAULT_RECOMMENDATION_OUTCOME_STALE_DAYS = 14; const DEFAULT_RECOMMENDATION_OUTCOME_IGNORED_DAYS = 7; @@ -367,7 +368,7 @@ function repoFromPayload(action: AgentActionRecord): string | null { function isMaintainerLane(login: string, repoFullName: string, association?: string | null | undefined): boolean { const owner = repoFullName.split("/")[0] ?? ""; - return sameLogin(owner, login) || association === "OWNER" || association === "MEMBER" || association === "COLLABORATOR"; + return sameLogin(owner, login) || isMaintainerAuthorAssociation(association); } function sameLogin(left: string | null | undefined, right: string | null | undefined): boolean { diff --git a/test/unit/check-maintainer-association-copies-script.test.ts b/test/unit/check-maintainer-association-copies-script.test.ts new file mode 100644 index 0000000000..e81185e02b --- /dev/null +++ b/test/unit/check-maintainer-association-copies-script.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vitest"; +import { DEFINITION_FILE, findMaintainerAssociationCopies } from "../../scripts/check-maintainer-association-copies"; + +// The guard for #9860's "eighth copy". Its own logic is tested against a synthetic tree so the cases that +// matter -- what it must catch, and what it must NOT -- are pinned independently of the real repo. +describe("check-maintainer-association-copies", () => { + const scan = (files: Record) => + findMaintainerAssociationCopies({ + roots: ["fake"], + listFiles: () => Object.keys(files), + readFile: (file) => files[file] ?? "", + }); + + it("catches a re-typed maintainer triple", () => { + const found = scan({ "fake/a.ts": 'const M = new Set(["OWNER", "MEMBER", "COLLABORATOR"]);' }); + expect(found).toHaveLength(1); + expect(found[0]).toMatchObject({ file: "fake/a.ts", line: 1 }); + }); + + it("catches it regardless of quoting or spacing", () => { + expect(scan({ "fake/a.ts": "if (['OWNER','MEMBER','COLLABORATOR'].includes(x)) {}" })).toHaveLength(1); + expect(scan({ "fake/a.ts": '[ "OWNER" , "MEMBER" , "COLLABORATOR" ]' })).toHaveLength(1); + }); + + it("catches an ===/|| chain, not just an array literal — the form review caught it missing", () => { + // The first version of this guard only matched comma-separated arrays, so it could never have found + // engine.ts's `value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"` and its 14 call + // sites. A guard that recognises one spelling of what it guards is a hand-maintained list again. + const chain = 'return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR";'; + expect(scan({ "fake/a.ts": chain })).toHaveLength(1); + }); + + it("catches a chain a formatter has broken across lines", () => { + const wrapped = 'return (\n value === "OWNER" ||\n value === "MEMBER" ||\n value === "COLLABORATOR"\n);'; + expect(scan({ "fake/a.ts": wrapped })).toHaveLength(1); + }); + + it("reports the line that NAMES an association, not the window's first line", () => { + const found = scan({ "fake/a.ts": '}\n\nfunction f() {\n return x === "OWNER" || x === "MEMBER" || x === "COLLABORATOR";' }); + expect(found[0]?.line, "a window opening on a brace must not be what gets reported").toBe(4); + expect(found[0]?.snippet).toContain("OWNER"); + }); + + it("does NOT flag a site with genuinely different semantics that is allowlisted with a reason", () => { + // command-authorization.ts maps OWNER/MEMBER to `maintainer` but COLLABORATOR to a SEPARATE role. + // "Fixing" it would grant collaborators maintainer-only commands. + const allowed = new Map([["fake/roles.ts", "different semantics: COLLABORATOR is its own role"]]); + const line = 'if (a === "OWNER" || a === "MEMBER") r.push("maintainer");\nif (a === "COLLABORATOR") r.push("collaborator");'; + expect(scan({ "fake/roles.ts": line })).toHaveLength(1); + expect(findMaintainerAssociationCopies({ roots: ["fake"], listFiles: () => ["fake/roles.ts"], readFile: () => line, allowed })).toEqual([]); + }); + + it("does NOT flag a full eight-value wire schema", () => { + // z.enum over GitHub's whole vocabulary is a schema, not this predicate; conflating them would be + // its own mistake, and the contract legitimately declares it four times. + const enumLine = 'authorAssociation: z.enum(["OWNER", "MEMBER", "COLLABORATOR", "CONTRIBUTOR", "FIRST_TIMER", "FIRST_TIME_CONTRIBUTOR", "MANNEQUIN", "NONE"]),'; + expect(scan({ "fake/schema.ts": enumLine })).toEqual([]); + }); + + it("does NOT flag the definition module itself", () => { + expect(scan({ [DEFINITION_FILE]: 'export const MAINTAINER_AUTHOR_ASSOCIATIONS = ["OWNER", "MEMBER", "COLLABORATOR"];' })).toEqual([]); + }); + + it("reports every occurrence, sorted by file then line", () => { + const spaced = ['["OWNER", "MEMBER", "COLLABORATOR"]', "", "", "", "", '["OWNER", "MEMBER", "COLLABORATOR"]'].join("\n"); + const found = scan({ "fake/b.ts": '["OWNER", "MEMBER", "COLLABORATOR"]', "fake/a.ts": spaced }); + expect(found.map((c) => `${c.file}:${c.line}`)).toEqual(["fake/a.ts:1", "fake/a.ts:6", "fake/b.ts:1"]); + }); + + it("collapses occurrences that fall inside ONE window into a single report", () => { + // Deliberate: the scan reads a 4-line window so a formatter-wrapped chain is still caught, and two + // hits inside one window are one finding, not two. The guard's job is to fail the build and name the + // file -- a developer fixes the file and re-runs, so duplicate noise costs more than it informs. + const adjacent = '["OWNER", "MEMBER", "COLLABORATOR"]\nx\n["OWNER", "MEMBER", "COLLABORATOR"]'; + expect(scan({ "fake/a.ts": adjacent })).toHaveLength(1); + }); + + it("passes on the REAL repository — the whole point of wiring it into test:ci", () => { + expect(findMaintainerAssociationCopies()).toEqual([]); + }); +});