1 Commits

Include renames

Author	SHA1	Message	Date
PerishFireandGitHub	819c34fd8f	fix(tools-pr): fall back on reviewDecision for unresolved-changes-requested (#1287) ... * fix(tools-pr): fall back on reviewDecision for unresolved-changes-requested Patrol classify on the live 102-PR queue missed three PRs (#1101, #1127, #1163) where GitHub's reviewDecision is CHANGES_REQUESTED but the classify tag did not fire. Root cause is a divergence between two notions of "latest review state per reviewer": - GitHub's reviewDecision keeps a reviewer's CHANGES_REQUESTED in effect until that same reviewer submits APPROVED or DISMISSED. A subsequent COMMENTED review by the same reviewer does NOT supersede it. - Our `reduceLatestReviewsByAuthor` collapses every reviewer to their latest review with no special-casing of state, so a CHANGES_REQUESTED followed by COMMENTED disappears from the reduced view. `tagUnresolvedChangesRequested` filtered the reduced view for `state === "CHANGES_REQUESTED"`, so the three PRs above (each had a reviewer write CHANGES_REQUESTED → COMMENTED) escaped the rule even though the PR-level reviewDecision was still CHANGES_REQUESTED. Add a narrow fallback: when the first path returns no per-reviewer reviewers, trust `facts.reviewDecision === "CHANGES_REQUESTED"` as the source of truth. The fallback reason and source token differ from the first path so report consumers can tell which signal fired. Reducer semantics left alone on purpose — flipping COMMENTED handling there would cascade to `bot-only-approval`, `stale-approval`, and `humanReviewerSignalAt`, each of which has its own correctness story. * fix(tools-pr): keep fallback reason strictly factual Codex flagged that the fallback path's reason text asserted a specific review sequence ("CHANGES_REQUESTED then COMMENTED") that the condition alone does not prove. The condition only observes: - `facts.reviewDecision === "CHANGES_REQUESTED"`, and - after `reduceLatestReviewsByAuthor`, no review carries `state === "CHANGES_REQUESTED"`. Multiple GitHub configurations satisfy that pair — a reviewer's CR followed by COMMENTED, a CR that sits outside the `reviews(last: 30)` fetch window, etc. Per `tools/pr/AGENTS.md`'s strictly-factual rule, the reason must report only what is directly observed, not the most likely upstream cause. Drop the inferred-cause clause from `reason`; move the explanation of possible upstream causes into the code comment above the branch where it does not show up in classify output. * docs(tools-pr): document fallback data source for unresolved-changes-requested Siri-Ray and lefarcen both flagged that the tag dictionary row for `unresolved-changes-requested` only describes the primary per-reviewer path. The fallback added earlier in this PR emits the same tag with a different `source` token (`gh.reviewDecision` vs the original `gh.latestReviews[].state`), so report consumers need the dictionary to list both paths to interpret which one fired. Update the row to call out both: the primary per-reviewer rule, and the PR-level reviewDecision fallback that fires when no per-reviewer CR survives the latest-per-author reduction. The two-token source column mirrors the actual `Tag.source` strings emitted at runtime. * test(tools-pr): pin both emission paths of unresolved-changes-requested lefarcen flagged the fallback was validated only by live-PR examples in the PR body, so a refactor could silently regress the coverage. Add a deterministic test file `tests/tags-unresolved-cr.test.ts` that exercises `classifyPr` against crafted `PrFacts` fixtures: - primary path (per-reviewer CR after reduction) fires with source=gh.latestReviews[].state and surfaces the reviewer login - fallback path fires with source=gh.reviewDecision when no per-reviewer CR survives reduction (covers both the COMMENTED-follow-up shape and the empty-reviews shape — the latter pins the `reviews(last: 30)` out-of-window concern from the factual-reason fix) - primary wins over fallback when both signals are present (single tag emitted, source=gh.latestReviews[].state) - two negative cases: empty reviewDecision and APPROVED — neither emits Also extend the fallback's code comment with the observed scale (3 of 102 open PRs hit this gap: #1101, #1127, #1163) so future maintainers can tell this is a recurring queue pattern, not a theoretical edge case. This is the first test under `tools/pr/tests/`; the package test script already ran `node --import tsx --test tests/*.test.ts` against an empty glob, so no scaffolding changes are needed.	2026-05-12 09:40:50 +08:00