fix: exclude skipped reviewer workers from notifications

[jonit-dev]

Summary

• require targeted reviewer workers to confirm the PR is still OPEN, not merely viewable
• aggregate reviewer notification metadata from workers that actually emitted success_reviewed
• add smoke coverage for a PR merging between parent selection and worker start

Verification

• yarn vitest run packages/cli/src/tests/scripts/core-flow-smoke.test.ts -t "reviewer should not include a PR that merged|reviewer target mode should treat|reviewer parallel mode should aggregate" --reporter=dot
• yarn verify

[jonit-dev]

Self-review

I reviewed this change for the reported reviewer notification inconsistency.

Findings:

• Root cause was confirmed in pdf-generation-api logs: the parent reviewer selected PR refactor: code cleanup Q1 2026 #61 and PR feat: migrate bash cron utilities to TypeScript #62, but the worker for PR refactor: code cleanup Q1 2026 #61 emitted NIGHT_WATCH_RESULT:skip_all_passing after the PR was already merged. The parent still emitted completion metadata using the original PRS_NEEDING_WORK_CSV, so the CLI enriched and notified PR refactor: code cleanup Q1 2026 #61 even though no review was actually performed by that worker.
• The fix is scoped to reviewer orchestration only:
  • targeted worker mode now checks gh pr view --json state and only treats OPEN as actionable
  • parallel aggregation now builds notification prs= from worker success_reviewed results, not from the parent’s pre-worker candidate list
• Added smoke tests for both the merged-target guard and the parallel orphan-notification case.

Verification:

• yarn vitest run packages/cli/src/__tests__/scripts/core-flow-smoke.test.ts -t "reviewer should not include a PR that merged|reviewer target mode should treat|reviewer parallel mode should aggregate" --reporter=dot
• yarn verify

Risk: low. This narrows notification metadata and target PR handling; it does not broaden reviewer selection.

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 88/100

This PR adds proper handling for edge cases where PRs merge between being listed and worker execution, ensuring notification metadata accurately reflects only successfully reviewed PRs. The implementation is solid with comprehensive test coverage.

---

✅ Key Strengths

• Well-Designed Edge Case Handling: The is_pr_open() function cleanly encapsulates PR state checking, and tracking ACTUAL_REVIEWED_PRS separately from assigned PRs correctly filters out merged PRs from notification metadata.
• Comprehensive Test Coverage: Two focused smoke tests validate both the worker metadata scenario and the TARGET_PR mode scenario, with properly mocked gh CLI responses.
• Minimal, Targeted Changes: The fix is surgical—adding a helper function and tracking variable rather than restructuring the entire script.

---

⚠️ Areas for Improvement

• Unused Variable Cleanup: PRS_NEEDING_WORK_CSV appears to still be computed in the script but is no longer passed to emit_final_status. If it's unused elsewhere, it should be removed to avoid confusion.
• Function Documentation: The is_pr_open() function would benefit from a brief comment explaining its purpose and return semantics for future maintainers.

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Maintainability	Potentially Unused Variable	night-watch-pr-reviewer-cron.sh	PRS_NEEDING_WORK_CSV is replaced by ACTUAL_REVIEWED_PRS in emit_final_status call but may still be computed in loop logic.	Low

---

🔚 Conclusion

Strong implementation with good test coverage for a subtle race condition. The change correctly ensures only successfully reviewed PRs appear in notification metadata. Consider a follow-up cleanup pass for PRS_NEEDING_WORK_CSV if it's no longer needed. Ready to merge with minor optional improvements.

---

Analyzed using z-ai/glm-5

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 85/100

This PR adds logic to exclude already-merged PRs from the reviewer's final notification metadata, with a new is_pr_open() helper function and comprehensive smoke tests covering both parallel worker and target mode scenarios.

---

✅ Key Strengths

• Robust PR State Detection: The is_pr_open() function in night-watch-pr-reviewer-cron.sh handles OPEN/MERGED/CLOSED states explicitly with a backward-compatible fallback for older gh versions.
• Comprehensive Test Coverage: Two new smoke tests cover the critical edge cases—parallel workers with pre-merged PRs and target mode with a merged target PR.
• Clean Integration: The tracking of ACTUAL_REVIEWED_PRS from worker results and conditional aggregation only on success_reviewed status is well-structured.

---

⚠️ Areas for Improvement

• Semantic Clarity on Parameter Change: The emit_final_status call changes from passing PRS_NEEDING_WORK_CSV to ACTUAL_REVIEWED_PRS. Verify that emit_final_status semantics align with this change, as the variable names suggest different purposes (PRs needing work vs. successfully reviewed PRs).
• Test Mock Maintainability: The inline fake gh scripts in tests are 50+ lines of bash with complex branching. Consider extracting to a shared test utility or using a more structured mock approach for long-term maintainability.

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Maintainability	Complex Inline Mock Scripts	core-flow-smoke.test.ts (new tests)	The fake gh script in the first test is complex and modifies behavior based on argument patterns, making it hard to debug if tests fail.	Low
Testing	Partial Mock Coverage	core-flow-smoke.test.ts (second test)	The second test's mock only handles pr view --json state, returning exit 1 for all other gh commands, which could mask failures if the script makes unexpected calls.	Low

---

🔚 Conclusion

This is a solid implementation with good test coverage for the edge case of merged PRs. The main consideration is confirming the emit_final_status parameter semantics, but the passing tests suggest this is intentional. Ready to merge after verifying the parameter change is correct.

---

Analyzed using z-ai/glm-5