fix(analytics): don't credit a pre-action merged PR as accepted

[philluiz2323]

Closes #605.

pullRequestOutcomeState's "merged" branch correctly requires mergedAt >= actionAt, but the "accepted" fallback only required a later updatedAt. So a PR merged before the recommendation (which fails the merged guard, and whose !pr.mergedAt short-circuit is false) fell through to "accepted" the moment it received any post-action update (a comment, label, base bump). "accepted" is a positive state (POSITIVE_STATES), so the recommendation was credited with a merge that causally predates it — inflating the operator recommendation-quality positives.

The intended semantics were already pinned: recommendation-outcomes.test.ts asserts the same pre-action-merged PR with no post-action activity is "stale". A single updatedAt bump should not flip that to the positive "accepted".

Change

• Guard the accepted fallback against a merge that predates actionAt, mirroring the merged branch's own >= actionAt discipline.
• Add a regression case: PR merged before the action with updatedAt after it -> "stale", not "accepted".

Verification

• recommendation-outcomes.test.ts 12/12 (the new case fails on the old code, which returns "accepted"); tsc --noEmit clean; full suite green; branch coverage holds (>= 97% gate).

Same function as the closed #475 (which fixed a changes-requested PR being scored improved); a different defect in the accepted fallback.

[ghost]

reviewbot · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action: ✅ Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR adds a guard to prevent pre-action merged pull requests from being classified as "accepted" and adds a corresponding unit test. The change is well‑scoped, includes documentation, and the new test validates the intended behavior.

Suggestions

• Consider adding a comment clarifying the expected type of mergedAt (numeric timestamp) to avoid future confusion.
• Add a test case where a PR is merged after the action to ensure it still yields the "accepted" outcome.

Worth double-checking

• If mergedAt can be a string or undefined in production, Number.isFinite may incorrectly treat it as not merged before the action.
• Verify that this guard does not unintentionally affect other outcome branches (e.g., when mergedAt is missing).

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR adds a guard to prevent crediting a pull request that was merged before the recommendation action as an 'accepted' outcome, and includes a test covering the scenario where a pre‑action merged PR later receives an updatedAt bump. The change is small, focused, and aligns the logic with the existing 'merged' branch discipline.

Suggestions

• Consider clarifying the comment to note that the guard also handles the edge case where mergedAt equals actionAt (currently treated as not merged before action).
• Optionally extract the mergedBeforeAction calculation into a named helper function for improved readability.

Worth double-checking

• Verify that the new condition does not unintentionally affect other outcome classifications (e.g., 'improved' or 'rejected') for edge cases where mergedAt is exactly equal to actionAt.
• Ensure the test suite passes for all existing scenarios after the change.

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[ghost]

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal	Result	Evidence	Action
Gate result	⚠️ Skipped	#606 is no longer open.	No action.

---

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

[ghost]

gittensory · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action: ✅ Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR adds a guard to prevent PRs merged before the recommendation action from being counted as an "accepted" outcome, and includes a corresponding unit test. The change is small, well‑documented, and appears correct.

Suggestions

• Consider adding a test for the edge case where mergedAt == actionAt to confirm the intended behavior.
• If mergedAt can be a string timestamp elsewhere, ensure it is normalized to a number before this guard or adjust the check accordingly.

Worth double-checking

• If mergedAt is not always a numeric timestamp, Number.isFinite may incorrectly skip the guard, leading to regressions.
• The new guard changes outcome classification; verify that downstream analytics correctly handle the new "stale" result.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR adds a guard in pullRequestOutcomeState to prevent a pull request that merged before the recommendation action from being incorrectly credited as an 'accepted' outcome when it later receives an updatedAt bump. A corresponding unit test verifies this behavior. The change is small, focused, and appears correct.

Suggestions

• Consider adding a brief comment or type assertion clarifying that mergedAt is expected to be a numeric timestamp (e.g., epoch ms) to avoid future misunderstandings if the field ever becomes a string.
• Optionally extract the merged‑before‑action check into a well‑named variable or helper for readability.

Worth double-checking

• If mergedAt is ever stored as an ISO string elsewhere, Number.isFinite would return false and the guard would not work, potentially restoring the original bug. Ensure the codebase consistently treats mergedAt as a numeric timestamp.