fix(engine): delegate opportunityFreshnessFactor to its pure mirror with an injected clock - #8039
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…ith an injected clock (JSONbored#8011) 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 JSONbored#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 JSONbored#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.
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-22 17:24:09 UTC
Review summary Nits — 1 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8039 +/- ##
=======================================
Coverage ? 93.03%
=======================================
Files ? 647
Lines ? 54094
Branches ? 19283
=======================================
Hits ? 50325
Misses ? 2850
Partials ? 919
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
opportunityFreshnessFactor(packages/loopover-engine/src/reward-risk.ts) hand-reimplementedpickIssueTimestamp/issueAgeDays/isParseableIssueTimestampinstead of delegating tocomputeOpportunityFreshness(./opportunity-freshness.js), which already carries the identicalround4/clamp/exp(-age/20)formula — the exact duplicated-arithmetic setup whose drift fix(engine): opportunityCompetitionFactor doesn't fail closed on non-finite inputs like its own engine-mirror sibling does #7529 had to fix for its siblingopportunityCompetitionFactor. Worse, the duplicatedissueAgeDaysread a bareDate.now(), sobuildRepoRewardRisk's freshness output was not deterministic the way the rest of the module claims.opportunityCompetitionFactor's post-fix(engine): opportunityCompetitionFactor doesn't fail closed on non-finite inputs like its own engine-mirror sibling does #7529 shape in the same file): delegate to the pure mirror and thread an injectablenowMsclock throughbuildRepoRewardRisk/buildContributorRewardRiskStrategy, defaulting toDate.now()only at thebuildRepoRewardRiskcall boundary, never inside the pure calculators. Output is arithmetically identical for finite inputs (same timestamp pick order, same floor-to-days, sameround4(clamp(exp(-age/20), 0.05, 1))).opportunity-freshness.tsstay covered by its ownopportunityFreshnessInternalstests.FreshnessIssue's optional timestamps gain the codebase-standard| undefinedwidening soIssueRecord[]is directly assignable underexactOptionalPropertyTypes.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
vitest runoverreward-risk-freshness,reward-risk-engine-branch-coverage,opportunity-branch-internals,opportunity-competition,reward-risk-competition-fail-closed,reward-risk-reports,signals-coverage,signals-v2,decision-pack,maintainer-noise— 204 tests green — plusvitest run --coverageover the reward-risk suites: every changed line and branch (both sides of theargs.nowMs ?? Date.now()boundary default) is covered. Also rannpm run build --workspace @loopover/engine(clean).actionlint/workers/mcp/ui checks are untouched surfaces (no workflow, worker, MCP, or UI files changed); CI runs them all.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.UI Evidence
Not applicable — engine-only change (no UI, docs, or extension surface touched).
Notes
buildRepoRewardRisk({ ..., nowMs })vscomputeOpportunityFreshness(issues, nowMs)) — exact equality instead of wall-clock-proximity equality — and a new determinism test pins same-inputs + same-clock to the same factor and to the concreteround4(exp(-2/20)) = 0.9048value, an assertion that was impossible while the hand-duplicatedissueAgeDaysread the live clock.pickIssueTimestamp/issueAgeDaysentries leftrewardRiskFreshnessInternalswith the deleted duplicates; the surviving single implementations remain covered throughopportunityFreshnessInternals(test/unit/opportunity-branch-internals.test.ts). Tests that exercised the removed duplicates were replaced by the determinism test above;bestFitLabelscoverage is untouched.