[Bug]: Recommendation-outcome classifier credits a pre-action **merged** PR as `accepted` (a positive outcome) when it gets any post-action update

[galuis116]

Summary

pullRequestOutcomeState attributes a recommendation outcome based on what
happened to the matched PR after the action. The terminal "merged" branch
correctly requires the merge to post-date the action (mergedAt >= actionAt),
but the "accepted" fallback only requires a later updatedAt. So a PR that was
merged before the recommendation — which fails the merged guard, and whose
!pr.mergedAt short-circuit is false because mergedAt is set — falls through
to "accepted" the moment it receives any post-action update (e.g. a single
comment). "accepted" is a positive state (recommendation-quality-report.ts:74
POSITIVE_STATES = ["accepted", "merged", "improved"]), so the recommendation is
credited with a merge that causally predates it, inflating the positive rate.

Evidence

// src/services/recommendation-outcomes.ts:271
function pullRequestOutcomeState(pr, action, actionAt, actionAgeMs, staleAfterMs) {
  const updatedAt = timestamp(pr.updatedAt ?? pr.createdAt);
  const createdAt = timestamp(pr.createdAt ?? pr.updatedAt);
  const mergedAt = timestamp(pr.mergedAt);
  if ((Number.isFinite(mergedAt) && mergedAt >= actionAt) || (!pr.mergedAt && pr.state === "merged" && updatedAt >= actionAt)) return "merged";
  if (pr.state === "closed" && updatedAt >= actionAt) return "closed";
  if (action.targetPullNumber && updatedAt >= actionAt) {
    if (hasChangesRequestedReview(pr)) return "rejected";
    if (hasPositiveOpenPullRequestSignal(pr)) return "improved";
  }
  if (createdAt >= actionAt || updatedAt > actionAt) return "accepted";   // <-- pre-action merge + later comment lands here
  if (actionAgeMs >= staleAfterMs) return "stale";
  return "ignored";
}

For a PR merged before the action: line 281's first clause is false
(mergedAt < actionAt); the second clause is false (!pr.mergedAt is false). If
pr.state is "merged" (not "closed"), line 282 is skipped. So the PR reaches
line 287, and a post-action comment (updatedAt > actionAt) yields "accepted".

The intended semantics are already in the tests

test/unit/recommendation-outcomes.test.ts:233-247 asserts that exactly this
pre-action-merged PR — merged 2026-04-01, no post-action activity
(updatedAt == mergedAt) — is classified "stale" (a non-positive state):

prRecord(60, "owner/old-merged", { state: "closed", mergedAt: "2026-04-01...", createdAt: "2026-03-01...", updatedAt: "2026-04-01..." })
// => outcomeState: "stale"

So the design intent is that a merge predating the recommendation is not a
positive outcome. The bug is that a single post-action updatedAt bump (a
comment, a label, a base-branch update) flips that same PR from "stale" to the
positive "accepted".

Concrete trace

• Action A recommends work on owner/old-merged at actionAt = 2026-05-01,
  with targetPullNumber = 60 (an exact-PR match via
  recommendation-outcomes.ts:79).
• PR fix(ci): isolate GitHub write token to release job #60 was created 2026-03-01 and merged 2026-04-01 (before A).
• On 2026-05-10 someone comments → updatedAt = 2026-05-10.
• pullRequestOutcomeState: mergedAt (Apr 1) >= actionAt (May 1) → false;
  !pr.mergedAt → false; state === "closed"? if "merged", skipped → line
  287: updatedAt (May 10) > actionAt (May 1) → "accepted".
• Without the comment, the same PR is "stale" (per the test above). The
  recommendation is now falsely scored as a positive ("accepted") outcome.

Impact

evaluateRecommendationOutcomes feeds the operator recommendation-quality
report; "accepted" is a positive state. Pre-existing merges that merely receive
incidental post-action activity are counted as recommendation wins, biasing the
quality/positive-rate metrics upward.

Test status

Not locked in. The merged-before-action case is only tested with
updatedAt == mergedAt (no post-action activity → "stale"); no test exercises
a pre-action-merged PR that receives a later update.

Suggested fix

Guard the "accepted" fallback against a terminal state that predates the
action (mirroring the "merged" branch's own >= actionAt discipline):

const mergedBeforeAction = Number.isFinite(mergedAt) && mergedAt < actionAt;
if (!mergedBeforeAction && (createdAt >= actionAt || updatedAt > actionAt)) return "accepted";

Add a test: PR merged before actionAt with updatedAt > actionAt →
"stale"/"ignored", not "accepted".

Distinct from prior reports

Same function as the closed #475 (which fixed a changes-requested PR being scored
improved while merge-clean), but a different defect: the "accepted" fallback's
failure to exclude pre-action merges. #475 establishes that mis-classifications in
pullRequestOutcomeState are treated as real bugs.

Confidence note

Reachability requires the exact-PR-target path (action.targetPullNumber) to
point at a PR that merged before the action and then received a later update — a
real but not the most common data shape. The internal inconsistency (terminal
branches guard the terminal event time; the accepted fallback does not) and the
test-revealed intent make it a genuine correctness defect rather than a modeling
choice.