diff --git a/src/signals/engine.ts b/src/signals/engine.ts index 15082ec677..014e0702b0 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -1808,12 +1808,17 @@ function normalizeRecentMergedOutcome( const association = typeof record.payload.author_association === "string" ? (record.payload.author_association as string) : undefined; const fileRecords = filesByNumber.get(record.number) ?? []; const reviewRecords = reviewsByNumber.get(record.number) ?? []; + // Conservative fallback: when author_association is absent or unrecognised, treat as maintainer + // 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; return { number: record.number, bucket: "merged", decided: true, merged: true, - maintainerLane: isMaintainerAssociation(association), + maintainerLane, linked: record.linkedIssues.length > 0, labels: [...new Set(record.labels)].sort(), filePaths: [...new Set([...fileRecords.map((file) => file.path), ...record.changedFiles])].sort(), @@ -2027,18 +2032,22 @@ export function buildRepoOutcomePatterns(args: { for (const state of args.detailSyncStates ?? []) { if (state.repoFullName.toLowerCase() === repoKey) detailByNumber.set(state.pullNumber, state); } - const withFileDetail = analyzed.filter((pr) => Boolean(detailByNumber.get(pr.number)?.filesSyncedAt)).length; - const withReviewDetail = analyzed.filter((pr) => Boolean(detailByNumber.get(pr.number)?.reviewsSyncedAt)).length; - const withCheckDetail = analyzed.filter((pr) => Boolean(detailByNumber.get(pr.number)?.checksSyncedAt)).length; + // evidenceCompleteness tracks detail-sync progress for pull_requests records only — merged-only records from + // recent_merged_pull_requests are never eligible for detail sync and must not dilute the denominator. + const syncEligible = analyzed.filter((pr) => seenNumbers.has(pr.number)); + const withFileDetail = syncEligible.filter((pr) => Boolean(detailByNumber.get(pr.number)?.filesSyncedAt)).length; + const withReviewDetail = syncEligible.filter((pr) => Boolean(detailByNumber.get(pr.number)?.reviewsSyncedAt)).length; + const withCheckDetail = syncEligible.filter((pr) => Boolean(detailByNumber.get(pr.number)?.checksSyncedAt)).length; const fullyDecidedWithDetail = decided.filter((pr) => { + if (!seenNumbers.has(pr.number)) return false; const state = detailByNumber.get(pr.number); return Boolean(state?.filesSyncedAt && state?.reviewsSyncedAt && state?.checksSyncedAt); }).length; - const filesCompletenessRatio = rate(withFileDetail, analyzed.length); - const reviewsCompletenessRatio = rate(withReviewDetail, analyzed.length); - const checksCompletenessRatio = rate(withCheckDetail, analyzed.length); + const filesCompletenessRatio = rate(withFileDetail, syncEligible.length); + const reviewsCompletenessRatio = rate(withReviewDetail, syncEligible.length); + const checksCompletenessRatio = rate(withCheckDetail, syncEligible.length); const completenessStatus: RepoOutcomeEvidenceCompleteness["status"] = - analyzed.length === 0 || (withFileDetail === 0 && withReviewDetail === 0 && withCheckDetail === 0) + syncEligible.length === 0 || (withFileDetail === 0 && withReviewDetail === 0 && withCheckDetail === 0) ? "missing" : filesCompletenessRatio >= 0.85 && reviewsCompletenessRatio >= 0.85 && checksCompletenessRatio >= 0.85 ? "complete" diff --git a/test/unit/repo-outcome-patterns.test.ts b/test/unit/repo-outcome-patterns.test.ts index e4aa1b95d8..ffdf52f1db 100644 --- a/test/unit/repo-outcome-patterns.test.ts +++ b/test/unit/repo-outcome-patterns.test.ts @@ -429,6 +429,171 @@ describe("buildRepoOutcomePatterns", () => { expect(details.join("\n")).not.toMatch(/duplicate\n@octo-team|@octo-team|[^\\]\[click\]\(https:\/\/example\.test\)/); }); + it("includes merged-only PRs absent from pull_requests in the analysis", () => { + // Repo has 5 open/closed PRs in pull_requests and 200 merged PRs only in recent_merged_pull_requests. + // The merged-only records carry author_association: "NONE" so they are outside-contributor lane. + // Without the fix: merge rate = 0/3, triggering false "high closure risk". + // With the fix: merge rate = ~0.97 from the full unified set. + const pullRequests: PullRequestRecord[] = [ + closedPr(1), + closedPr(2), + closedPr(3), + pr(4, { state: "open" }), + pr(5, { state: "open" }), + ]; + const recentMergedPullRequests: RecentMergedPullRequestRecord[] = Array.from({ length: 200 }, (_, i) => ({ + repoFullName: REPO, + number: 100 + i, + title: `Merged PR ${100 + i}`, + authorLogin: "dev", + mergedAt: "2026-05-01T00:00:00.000Z", + labels: ["bug"], + linkedIssues: [200 + i], + changedFiles: ["src/feature.ts"], + payload: { author_association: "NONE" }, + })); + const result = buildRepoOutcomePatterns({ repo: repo(), repoFullName: REPO, pullRequests, recentMergedPullRequests }); + + expect(result.totals.analyzed).toBe(205); + expect(result.totals.merged).toBe(200); + expect(result.totals.closedUnmerged).toBe(3); + expect(result.outsideContributorMergeRate).toBeCloseTo(200 / 203, 4); + expect(result.riskPatterns.some((p) => p.title === "Outside contributor PRs rarely merge here")).toBe(false); + expect(result.successPatterns.some((p) => p.title === "Outside contributors merge well here")).toBe(true); + }); + + it("conservatively excludes merged-only PRs with unknown author_association from outside-contributor statistics", () => { + // Merged-only records with payload: {} (no author_association) cannot be safely classified. + // Conservative fallback: treat as maintainer lane so they do not inflate outside-contributor merge rate. + const pullRequests: PullRequestRecord[] = [ + closedPr(1), + closedPr(2), + mergedPr(3, { authorAssociation: "NONE" }), + ]; + const recentMergedPullRequests: RecentMergedPullRequestRecord[] = Array.from({ length: 10 }, (_, i) => ({ + repoFullName: REPO, + number: 100 + i, + title: `Unknown-assoc PR ${100 + i}`, + authorLogin: "dev", + mergedAt: "2026-05-01T00:00:00.000Z", + labels: [], + linkedIssues: [], + changedFiles: [], + payload: {}, + })); + const result = buildRepoOutcomePatterns({ repo: repo(), repoFullName: REPO, pullRequests, recentMergedPullRequests }); + + expect(result.totals.analyzed).toBe(13); + expect(result.totals.maintainerLanePullRequests).toBe(10); + expect(result.totals.outsideContributorPullRequests).toBe(3); + // Outside-contributor rate uses only the 3 classifiable outside PRs (1 merged, 2 closed). + expect(result.outsideContributorMergeRate).toBeCloseTo(1 / 3, 4); + // Maintainer lane rate includes the 10 unknown-association merged PRs. + expect(result.maintainerLaneMergeRate).toBeCloseTo(1, 4); + }); + + it("does not double-count PRs present in both pull_requests and recent_merged_pull_requests", () => { + const pullRequests = [mergedPr(1), mergedPr(2), closedPr(3)]; + const recentMergedPullRequests: RecentMergedPullRequestRecord[] = [ + { repoFullName: REPO, number: 1, title: "PR 1", authorLogin: "dev", mergedAt: "2026-05-01T00:00:00.000Z", labels: [], linkedIssues: [], changedFiles: [], payload: {} }, + { repoFullName: REPO, number: 2, title: "PR 2", authorLogin: "dev", mergedAt: "2026-05-01T00:00:00.000Z", labels: [], linkedIssues: [], changedFiles: [], payload: {} }, + { repoFullName: REPO, number: 99, title: "Merged only", authorLogin: "dev", mergedAt: "2026-05-01T00:00:00.000Z", labels: [], linkedIssues: [], changedFiles: [], payload: {} }, + ]; + const result = buildRepoOutcomePatterns({ repo: repo(), repoFullName: REPO, pullRequests, recentMergedPullRequests }); + expect(result.totals.analyzed).toBe(4); + expect(result.totals.merged).toBe(3); + expect(result.totals.closedUnmerged).toBe(1); + }); + + it("reconciles a closed PR in pull_requests that has a mergedAt in its recent-merged record", () => { + // A PR recorded as "closed" in the pull_requests table was actually merged; the merged table has the timestamp. + const pullRequests = [ + closedPr(1), + closedPr(2), + closedPr(3), + mergedPr(4), + mergedPr(5), + mergedPr(6), + ]; + const recentMergedPullRequests: RecentMergedPullRequestRecord[] = [ + { repoFullName: REPO, number: 1, title: "PR 1", authorLogin: "dev", mergedAt: "2026-05-01T00:00:00.000Z", labels: [], linkedIssues: [], changedFiles: ["src/a.ts"], payload: {} }, + { repoFullName: REPO, number: 2, title: "PR 2", authorLogin: "dev", mergedAt: "2026-05-01T00:00:00.000Z", labels: [], linkedIssues: [], changedFiles: ["src/b.ts"], payload: {} }, + ]; + const result = buildRepoOutcomePatterns({ repo: repo(), repoFullName: REPO, pullRequests, recentMergedPullRequests }); + expect(result.totals.merged).toBe(5); + expect(result.totals.closedUnmerged).toBe(1); + expect(result.outsideContributorMergeRate).toBeCloseTo(5 / 6, 4); + }); + + it("does not reconcile a closed PR whose recent-merged record carries no mergedAt timestamp", () => { + const pullRequests = [closedPr(1), mergedPr(2), mergedPr(3), mergedPr(4)]; + const recentMergedPullRequests: RecentMergedPullRequestRecord[] = [ + { repoFullName: REPO, number: 1, title: "PR 1", authorLogin: "dev", mergedAt: null, labels: [], linkedIssues: [], changedFiles: [], payload: {} }, + ]; + const result = buildRepoOutcomePatterns({ repo: repo(), repoFullName: REPO, pullRequests, recentMergedPullRequests }); + expect(result.totals.closedUnmerged).toBe(1); + expect(result.totals.merged).toBe(3); + }); + + it("does not count merged-only OWNER/MEMBER/COLLABORATOR PRs in the outside-contributor merge rate", () => { + // 3 outside-contributor PRs from pull_requests: 1 merged, 2 closed. + // 10 merged-only maintainer PRs in recent_merged_pull_requests with OWNER/MEMBER/COLLABORATOR associations. + // Without the fix: outside merge rate = 11/13 ≈ 0.85 (falsely inflated by owner work). + // With the fix: outside merge rate = 1/3 ≈ 0.33 (only outside-contributor decided PRs counted). + const pullRequests: PullRequestRecord[] = [ + mergedPr(1, { authorAssociation: "NONE" }), + closedPr(2, { authorAssociation: "NONE" }), + closedPr(3, { authorAssociation: "NONE" }), + ]; + const recentMergedPullRequests: RecentMergedPullRequestRecord[] = [ + ...["OWNER", "OWNER", "OWNER", "MEMBER", "MEMBER", "COLLABORATOR", "COLLABORATOR", "COLLABORATOR", "OWNER", "MEMBER"].map( + (association, i): RecentMergedPullRequestRecord => ({ + repoFullName: REPO, + number: 100 + i, + title: `Maintainer PR ${100 + i}`, + authorLogin: "repo-owner", + mergedAt: "2026-05-01T00:00:00.000Z", + labels: [], + linkedIssues: [], + changedFiles: ["src/internal.ts"], + payload: { author_association: association }, + }), + ), + ]; + const result = buildRepoOutcomePatterns({ repo: repo(), repoFullName: REPO, pullRequests, recentMergedPullRequests }); + + expect(result.totals.analyzed).toBe(13); + expect(result.totals.maintainerLanePullRequests).toBe(10); + expect(result.totals.outsideContributorPullRequests).toBe(3); + // Only the 3 outside-contributor PRs (1 merged, 2 closed) form the denominator. + expect(result.outsideContributorMergeRate).toBeCloseTo(1 / 3, 4); + // Maintainer merge rate covers the 10 merged-only maintainer PRs. + expect(result.maintainerLaneMergeRate).toBeCloseTo(1, 4); + // The repo should NOT be flagged as "merges well" since outside rate is low. + expect(result.successPatterns.some((p) => p.title === "Outside contributors merge well here")).toBe(false); + }); + + it("derives returning_contributor authorRole from payload.author_association CONTRIBUTOR on merged-only rows", () => { + const pullRequests: PullRequestRecord[] = [closedPr(1)]; + const recentMergedPullRequests: RecentMergedPullRequestRecord[] = [ + { + repoFullName: REPO, + number: 99, + title: "Returning contributor PR", + authorLogin: "returning-dev", + mergedAt: "2026-05-01T00:00:00.000Z", + labels: [], + linkedIssues: [], + changedFiles: [], + payload: { author_association: "CONTRIBUTOR" }, + }, + ]; + const result = buildRepoOutcomePatterns({ repo: repo(), repoFullName: REPO, pullRequests, recentMergedPullRequests }); + // CONTRIBUTOR is outside-contributor lane but returning — must not be maintainerLane. + expect(result.totals.maintainerLanePullRequests).toBe(0); + expect(result.outsideContributorMergeRate).toBeCloseTo(1 / 2, 4); + }); + it("never emits forbidden public-surface language", () => { const fixtures = [ buildRepoOutcomePatterns(primaryFixture()),