Skip to content

Commit ecb5538

Browse files
authored
fix(engine): delegate opportunityFreshnessFactor to its pure mirror with an injected clock (#8011) (#8039)
opportunityFreshnessFactor hand-reimplemented pickIssueTimestamp/issueAgeDays/ isParseableIssueTimestamp instead of delegating to computeOpportunityFreshness (./opportunity-freshness.js), which already carries the identical round4/clamp/exp(-age/20) formula -- the exact duplicated-arithmetic setup whose drift #7529 had to fix for its sibling opportunityCompetitionFactor. Worse, the duplicated issueAgeDays read a bare Date.now(), so buildRepoRewardRisk's freshness output was not deterministic the way the rest of the module claims. Delegate to the mirror (same treatment #7529 gave the competition factor) and thread an injectable nowMs clock through buildRepoRewardRisk / buildContributorRewardRiskStrategy, defaulting to Date.now() only at the buildRepoRewardRisk call boundary -- never inside the pure calculators. Output is arithmetically identical for finite inputs. The now-redundant duplicated helpers are removed; their surviving copies stay covered through opportunity-freshness.ts's own internals tests. Tests: the existing parity suite now injects the same clock on both sides (exact equality, not wall-clock proximity) and a new determinism test pins same-inputs + same-clock to the same factor and to the concrete round4(exp(-2/20)) = 0.9048 value.
1 parent 42bd2cc commit ecb5538

3 files changed

Lines changed: 44 additions & 81 deletions

File tree

packages/loopover-engine/src/opportunity-freshness.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
export type FreshnessIssue = {
22
state: string;
3-
updatedAt?: string | null;
4-
createdAt?: string | null;
3+
updatedAt?: string | null | undefined;
4+
createdAt?: string | null | undefined;
55
};
66

77
function round4(value: number): number {

packages/loopover-engine/src/reward-risk.ts

Lines changed: 21 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import type { ScorePreviewResult } from "./scoring/preview.js";
1010
import { buildScorePreview } from "./scoring/preview.js";
1111
import { computeOpportunityCompetition } from "./opportunity-competition.js";
12+
import { computeOpportunityFreshness } from "./opportunity-freshness.js";
1213
import { isSuspiciousConfiguredLabel } from "./scoring/label-match.js";
1314
import { isFailingCheckSummary } from "./signals/check-summary.js";
1415
import { nowIso } from "./utils/json.js";
@@ -257,6 +258,9 @@ export function buildRepoRewardRisk(args: {
257258
/** Repo primary language (from sync metadata / ContributorFit.languageFit),
258259
* used for the personalFit language-match bonus. */
259260
repoLanguage?: string | null | undefined;
261+
/** Injected clock for the freshness factor (#8011); defaults to Date.now() at this call boundary only --
262+
* the pure calculators below never read the real clock themselves. */
263+
nowMs?: number | undefined;
260264
}, deps: RewardRiskEngineDeps): RepoRewardRisk {
261265
const roleContext = deps.buildRoleContext({
262266
login: args.login,
@@ -282,7 +286,7 @@ export function buildRepoRewardRisk(args: {
282286

283287
const labels = bestFitLabels(args.repo);
284288
const competitionFactor = opportunityCompetitionFactor(collisions.summary.highRiskCount, queueHealth.signals.openPullRequests);
285-
const freshnessFactor = opportunityFreshnessFactor(args.issues);
289+
const freshnessFactor = opportunityFreshnessFactor(args.issues, args.nowMs ?? Date.now());
286290
const currentOpenPrCount = nonNegative(args.outcomeHistory.totals.openPullRequests);
287291
const currentOpenIssueCount = nonNegative(repoOutcome?.openIssues ?? args.outcomeHistory.totals.openIssues);
288292
/* v8 ignore next -- Credibility fallback order protects sparse private snapshots; behavior is covered through scoring profile tests. */
@@ -427,6 +431,9 @@ export function buildContributorRewardRiskStrategy(args: {
427431
allIssues: IssueRecord[];
428432
allPullRequests: PullRequestRecord[];
429433
recentMergedPullRequests?: RecentMergedPullRequestRecord[] | undefined;
434+
/** Injected clock threaded through to every per-repo freshness factor (#8011); optional, resolved at
435+
* buildRepoRewardRisk's own boundary when absent. */
436+
nowMs?: number | undefined;
430437
}, deps: RewardRiskEngineDeps): ContributorRewardRiskStrategy {
431438
const registeredRepoNames = new Map(args.repositories.filter((repo) => repo.isRegistered).map((repo) => [repo.fullName.toLowerCase(), repo.fullName]));
432439
const candidateRepoNames = uniqueRegisteredRepoNames(
@@ -457,6 +464,7 @@ export function buildContributorRewardRiskStrategy(args: {
457464
pullRequests: pullRequestsByRepo.get(repoKey) ?? [],
458465
recentMergedPullRequests: recentMergedPullRequestsByRepo.get(repoKey) ?? [],
459466
repoLanguage: args.fit.languageFit.find((entry) => sameRepo(entry.repoFullName, repoFullName))?.language ?? null,
467+
nowMs: args.nowMs,
460468
}, deps);
461469
})
462470
/* v8 ignore next -- Locale tie ordering is deterministic presentation fallback after ranked analysis scores. */
@@ -908,38 +916,16 @@ function opportunityCompetitionFactor(highRiskDuplicateClusters: number, openPul
908916
return computeOpportunityCompetition(highRiskDuplicateClusters, openPullRequests);
909917
}
910918

911-
function opportunityFreshnessFactor(issues: IssueRecord[]): number {
912-
const openIssues = issues.filter((issue) => issue.state === "open");
913-
if (openIssues.length === 0) return 0;
914-
let mostRecentAgeDays = Number.POSITIVE_INFINITY;
915-
for (const issue of openIssues) {
916-
const ageDays = issueAgeDays(pickIssueTimestamp(issue));
917-
if (ageDays < mostRecentAgeDays) mostRecentAgeDays = ageDays;
918-
}
919-
// Freshness decays exponentially: ~1.0 at 0 days, ~0.6 at 7 days, ~0.2 at 30 days, ~0.05 at 90 days.
920-
return round(clamp(Math.exp(-mostRecentAgeDays / 20), 0.05, 1));
921-
}
922-
923-
function isParseableIssueTimestamp(value: string): boolean {
924-
return Number.isFinite(Date.parse(value));
925-
}
926-
927-
function pickIssueTimestamp(issue: IssueRecord): string | null {
928-
const updated = typeof issue.updatedAt === "string" ? issue.updatedAt.trim() : "";
929-
if (updated && isParseableIssueTimestamp(updated)) return updated;
930-
931-
const created = typeof issue.createdAt === "string" ? issue.createdAt.trim() : "";
932-
if (created && isParseableIssueTimestamp(created)) return created;
933-
934-
return null;
935-
}
936-
937-
/** Unknown/unparseable timestamps floor freshness (parity with loopover-engine opportunity-freshness.ts). */
938-
function issueAgeDays(value: string | null): number {
939-
if (!value) return Number.POSITIVE_INFINITY;
940-
const parsed = Date.parse(value);
941-
if (!Number.isFinite(parsed)) return Number.POSITIVE_INFINITY;
942-
return Math.floor((Date.now() - parsed) / 86_400_000);
919+
function opportunityFreshnessFactor(issues: IssueRecord[], nowMs: number): number {
920+
// Delegates to the pure mirror rather than repeating its arithmetic (#8011) -- the same treatment #7529
921+
// gave opportunityCompetitionFactor above. The hand-duplicated copy this replaced reimplemented
922+
// pickIssueTimestamp/issueAgeDays/isParseableIssueTimestamp with a bare Date.now() inside issueAgeDays,
923+
// so this path was neither deterministic nor guarded against drifting from the mirror's formula (the
924+
// exact drift #7529 had to fix for its sibling). `computeOpportunityFreshness` is arithmetically
925+
// identical for finite inputs -- same timestamp pick order, same floor-to-days, same
926+
// round4/clamp(exp(-age/20), 0.05, 1) -- with the clock injected; Date.now() now lives only at
927+
// buildRepoRewardRisk's call boundary.
928+
return computeOpportunityFreshness(issues, nowMs);
943929
}
944930

945931
function sameRepo(left: string, right: string): boolean {
@@ -990,8 +976,8 @@ function clamp(value: number, min: number, max: number): number {
990976

991977
/* v8 ignore start -- Test-only export surface for branch coverage. */
992978
export const rewardRiskFreshnessInternals = {
993-
pickIssueTimestamp,
994-
issueAgeDays,
979+
// pickIssueTimestamp/issueAgeDays left this surface with #8011: the freshness path now delegates to
980+
// opportunity-freshness.ts, whose own opportunityFreshnessInternals expose the surviving copies.
995981
bestFitLabels,
996982
};
997983

test/unit/reward-risk-freshness.test.ts

Lines changed: 21 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,10 @@ describe("reward-risk freshness parity with loopover-engine", () => {
9999
const staleIssues = [issue(collab.fullName, 2, "Stale", { updatedAt: "2020-01-01T00:00:00.000Z" })];
100100
const undatedIssues = [issue(collab.fullName, 3, "Undated", { updatedAt: null, createdAt: null })];
101101

102-
const fresh = buildRepoRewardRisk({ ...base, issues: freshIssues, pullRequests: [] });
103-
const stale = buildRepoRewardRisk({ ...base, issues: staleIssues, pullRequests: [] });
104-
const undated = buildRepoRewardRisk({ ...base, issues: undatedIssues, pullRequests: [] });
102+
// Inject the same clock both sides read (#8011) -- exact equality, not wall-clock-proximity equality.
103+
const fresh = buildRepoRewardRisk({ ...base, issues: freshIssues, pullRequests: [], nowMs });
104+
const stale = buildRepoRewardRisk({ ...base, issues: staleIssues, pullRequests: [], nowMs });
105+
const undated = buildRepoRewardRisk({ ...base, issues: undatedIssues, pullRequests: [], nowMs });
105106

106107
expect(fresh.rewardUpside.opportunityFactors.freshnessFactor).toBe(
107108
computeOpportunityFreshness(toFreshnessIssues(freshIssues), nowMs),
@@ -135,47 +136,23 @@ describe("reward-risk freshness parity with loopover-engine", () => {
135136
expect(result.rewardUpside.opportunityFactors.freshnessFactor).toBeGreaterThan(0.7);
136137
});
137138

138-
it("pickIssueTimestamp and issueAgeDays cover defensive timestamp branches", () => {
139-
const { pickIssueTimestamp, issueAgeDays } = rewardRiskFreshnessInternals;
140-
expect(
141-
pickIssueTimestamp({
142-
repoFullName: collab.fullName,
143-
number: 1,
144-
title: "t",
145-
state: "open",
146-
labels: [],
147-
linkedPrs: [],
148-
updatedAt: "2026-07-03T00:00:00.000Z",
149-
createdAt: "2020-01-01T00:00:00.000Z",
150-
}),
151-
).toBe("2026-07-03T00:00:00.000Z");
152-
expect(
153-
pickIssueTimestamp({
154-
repoFullName: collab.fullName,
155-
number: 2,
156-
title: "t",
157-
state: "open",
158-
labels: [],
159-
linkedPrs: [],
160-
updatedAt: " ",
161-
createdAt: "2026-07-03T00:00:00.000Z",
162-
}),
163-
).toBe("2026-07-03T00:00:00.000Z");
164-
expect(
165-
pickIssueTimestamp({
166-
repoFullName: collab.fullName,
167-
number: 3,
168-
title: "t",
169-
state: "open",
170-
labels: [],
171-
linkedPrs: [],
172-
updatedAt: null,
173-
createdAt: null,
174-
}),
175-
).toBeNull();
176-
expect(issueAgeDays(null)).toBe(Number.POSITIVE_INFINITY);
177-
expect(issueAgeDays("not-a-date")).toBe(Number.POSITIVE_INFINITY);
178-
expect(issueAgeDays("2026-07-03T00:00:00.000Z")).toBeGreaterThanOrEqual(0);
139+
it("is deterministic under an injected clock: same issues + same nowMs always yield the same factor (#8011)", () => {
140+
// A fixed epoch, no Date.now() anywhere: the factor must be a pure function of (issues, nowMs). The
141+
// pre-#8011 hand-duplicated issueAgeDays read the live clock, so this exact assertion was impossible.
142+
const fixedNowMs = Date.parse("2026-07-10T00:00:00.000Z");
143+
const fixedIssues = [
144+
issue(collab.fullName, 1, "Fixed", { updatedAt: "2026-07-08T00:00:00.000Z", createdAt: "2026-07-01T00:00:00.000Z" }),
145+
];
146+
147+
const first = buildRepoRewardRisk({ ...base, issues: fixedIssues, pullRequests: [], nowMs: fixedNowMs });
148+
const second = buildRepoRewardRisk({ ...base, issues: fixedIssues, pullRequests: [], nowMs: fixedNowMs });
149+
150+
expect(first.rewardUpside.opportunityFactors.freshnessFactor).toBe(second.rewardUpside.opportunityFactors.freshnessFactor);
151+
expect(first.rewardUpside.opportunityFactors.freshnessFactor).toBe(
152+
computeOpportunityFreshness(toFreshnessIssues(fixedIssues), fixedNowMs),
153+
);
154+
// 2 days old -> round4(exp(-2/20)) -- a concrete pin so a formula drift can't slip through as "still equal".
155+
expect(first.rewardUpside.opportunityFactors.freshnessFactor).toBe(0.9048);
179156
});
180157
});
181158

0 commit comments

Comments
 (0)