Skip to content

feat: docs/prds/ci-auto-retry.md#102

Open
jonit-dev wants to merge 1 commit into
masterfrom
night-watch/98-docs-prds-ci-auto-retry-md
Open

feat: docs/prds/ci-auto-retry.md#102
jonit-dev wants to merge 1 commit into
masterfrom
night-watch/98-docs-prds-ci-auto-retry-md

Conversation

@jonit-dev
Copy link
Copy Markdown
Owner

@jonit-dev jonit-dev commented May 4, 2026

Status

This PR is currently blocked by merge conflicts in scripts/night-watch-cron.sh against origin/master.

Conflict summary

  • PR branch night-watch/98-docs-prds-ci-auto-retry-md is 23 commits behind origin/master.
  • The conflict is in scripts/night-watch-cron.sh because master introduced the audit-triage workflow (is_audit_board_issue, is_tiny_audit_board_issue, build_audit_triage_prompt, etc.) in the same region where this PR adds the failed-CI retry prompt logic.
  • Auto-resolution is high-risk for an orchestration script; a bad merge could break future autonomous runs.

Next step

Manual rebase + conflict resolution required. The correct resolution should:

  1. Keep master’s skip_if_job_paused, audit-triage helpers, and lean-audit workflow.
  2. Integrate this PR’s CI-auto-retry feature as a separate, opt-in block (not replacing the audit workflow).
  3. Preserve RESUME_FAILED_CI / RESUME_FAILED_CI_SUMMARY variables and the failed-CI retry prompt, but place them after the audit-triage branch so both paths coexist.

Do not merge until

  • Conflicts resolved
  • yarn verify passes in a clean worktree
  • Fresh CI green on rebased head

@jonit-dev jonit-dev added nw:partial Executor started this PR and work is intentionally incomplete nw:resumable Executor should resume this unfinished PR before starting new work labels May 4, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 88/100

This PR implements a CI auto-retry feature for the Night Watch executor, enabling automatic resumption of ready-review PRs with failed CI checks. The implementation is well-structured with comprehensive test coverage and clear documentation.


✅ Key Strengths

  • Comprehensive Test Coverage: The test suite thoroughly validates the new selection logic, including positive cases (failed CI selection), negative cases (pending/passing/unknown CI ignored), and priority ordering (resumable label takes precedence).
  • Clear PRD Documentation: The PRD document clearly defines goals, non-goals, contract, and phased implementation, making the feature requirements transparent.
  • Defensive Implementation: The bash script properly initializes all new state variables (RESUME_REASON, RESUME_FAILED_CI, etc.) and handles edge cases like missing PRD files gracefully.

⚠️ Areas for Improvement

  • Complex jq Filter Logic: The find_executor_resume_pr function in night-watch-helpers.sh contains ~50 lines of complex jq with multiple custom definitions. Consider extracting the failed-CI detection into a separate, testable helper function.
  • Duplicated PR URL Fetch Logic: The success handling section fetches PR URLs with nearly identical gh pr list commands in three conditional branches. Consider refactoring into a shared helper function.
  • Potential Race Condition: The skip-claim path for missing PRD files (SKIP_PRD_CLAIM=1) allows concurrent cron runs to potentially pick up the same failed-CI PR since no lock file is created.

📋 Issues Found

Issue Type Issue Name Affected Components Description Impact/Severity
Maintainability Complex jq Filter scripts/night-watch-helpers.sh The failed-CI detection jq filter is complex and difficult to reason about, with nested definitions and transformations. Medium
Testing Missing Edge Case Tests Test file Tests don't cover edge cases like mixed CI states (some checks failed, some pending) or API rate limiting scenarios. Low
Security Potential Concurrent Runs scripts/night-watch-cron.sh The SKIP_PRD_CLAIM=1 path bypasses the claim mechanism, potentially allowing duplicate work on the same failed-CI PR. Low

🔚 Conclusion

This is a well-implemented feature with solid test coverage and clear documentation. The main follow-ups are maintainability concerns around complex jq logic and a potential race condition in the skip-claim path. Consider addressing the concurrency concern before merge, but other improvements can be follow-ups.


Analyzed using z-ai/glm-5

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 82/100

This PR implements CI auto-retry functionality for executor PRs with a well-structured PRD, comprehensive test coverage for the selection logic, and clean integration into the existing cron workflow. A few maintainability concerns and potential edge cases around environment variables and missing PRD files warrant attention.


✅ Key Strengths

  • Comprehensive Test Coverage: Tests thoroughly cover the three key scenarios: failed-CI selection, ignoring non-failed CI states, and priority ordering between resumable and failed-CI PRs.
  • Graceful Degradation: The implementation handles missing PRD files for failed-CI PRs by setting RESUME_WITHOUT_PRD_FILE=1 and providing a focused CI repair prompt instead of failing.
  • Clear Separation of Concerns: The helper function handles PR selection logic while the cron script orchestrates the workflow, maintaining clean architecture.

⚠️ Areas for Improvement

  • Environment Variable Documentation: The jq filters reference NW_EXECUTOR_READY_REVIEW_LABEL and NW_PR_RESOLVER_READY_LABEL via --arg, but these variables aren't shown as defined in the diff. Consider adding defaults or documenting the required environment configuration.

  • Test Data Structure Consistency: The failed-CI test uses nested statusCheckRollup:[{contexts:[...]}] while the ignored-states test uses flat statusCheckRollup:[{...}]. Both are valid GitHub API responses, but a comment noting this intentional coverage would help future maintainers.

  • Integration Test Gap: While unit tests cover the helper function well, there are no integration tests for the full cron workflow with failed-CI scenarios, making Phase 3 verification dependent on manual smoke tests.


📋 Issues Found

Issue Type Issue Name Affected Components Description Impact/Severity
Maintainability Undocumented Env Vars scripts/night-watch-helpers.sh NW_EXECUTOR_READY_REVIEW_LABEL and NW_PR_RESOLVER_READY_LABEL are used but not defined in the diff. Medium
Testing Missing Integration Coverage scripts/night-watch-cron.sh No tests for the full failed-CI repair workflow in the cron script, relying on manual verification per Phase 3. Low
Maintainability Synthetic PRD Name Usage scripts/night-watch-cron.sh When RESUME_WITHOUT_PRD_FILE=1, ELIGIBLE_PRD is set to a non-existent filename used in logging/tracking. Low

🔚 Conclusion

This is a solid implementation of the CI auto-retry feature with good test coverage and thoughtful handling of edge cases. The primary follow-up is ensuring the environment variables are properly documented or defaulted. The PR is merge-ready with minor documentation improvements recommended.


Analyzed using z-ai/glm-5

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 83/100

This PR implements CI auto-retry for Night Watch executor PRs with comprehensive test coverage for the helper function. The implementation follows existing patterns well, though the cron script logic has grown complex with multiple conditional branches for handling different resume scenarios.


✅ Key Strengths

  • Comprehensive Test Coverage: The new tests in night-watch-helpers.test.ts thoroughly cover the key scenarios: failed-CI selection, ignoring non-failed CI states, and priority ordering between resumable and failed-CI PRs.
  • Well-Documented PRD: The PRD clearly defines contracts, phases, and verification steps, making the feature scope explicit and traceable.
  • Robust CI State Detection: The failed_check jq function in night-watch-helpers.sh handles multiple CI state formats (conclusion, state, status, bucket) and various failure modes.

⚠️ Areas for Improvement

  • Cron Script Complexity: The night-watch-cron.sh now handles many resume scenarios with nested conditionals around RESUME_FAILED_CI. Consider extracting the CI repair workflow into a dedicated function or sourcing a separate script to improve maintainability.
  • Two API Calls for Resume Detection: find_executor_resume_pr makes two separate gh pr list calls (one for resumable-labeled PRs, one for failed-CI ready-review PRs). Consider consolidating into a single query with client-side filtering to reduce API rate usage.
  • Missing Cron Script Tests: While the helper function has good test coverage, the night-watch-cron.sh changes lack corresponding integration or smoke tests for the new CI repair workflow branches.

📋 Issues Found

Issue Type Issue Name Affected Components Description Impact/Severity
Performance Duplicate API Calls night-watch-helpers.sh Two sequential gh pr list calls increase API rate consumption and latency. Low
Maintainability Complex Conditional Branching night-watch-cron.sh Multiple interleaved conditionals for RESUME_FAILED_CI and ISSUE_NUMBER combinations make logic harder to follow. Medium
Testing No Cron Script Coverage night-watch-cron.sh The CI repair workflow paths (lines 695-740, 937-970, 1201-1231) lack automated test coverage. Medium

🔚 Conclusion

This is a solid implementation that follows existing patterns and includes good helper-level tests. The feature logic appears correct for the defined contracts. Consider adding integration tests for the cron workflow branches before or shortly after merge, and monitor for opportunities to refactor the conditional complexity.


Analyzed using z-ai/glm-5

@jonit-dev
Copy link
Copy Markdown
Owner Author

Overnight Night Watch sweep: merge conflicts in are too risky to auto-resolve (orchestration script). Left detailed resolution plan in PR body. Needs manual rebase.

@jonit-dev jonit-dev added nw:ready-review Executor finished implementation and the PR is ready for automated/human review and removed nw:partial Executor started this PR and work is intentionally incomplete nw:resumable Executor should resume this unfinished PR before starting new work labels May 12, 2026
@jonit-dev jonit-dev marked this pull request as ready for review May 12, 2026 19:37
@jonit-dev
Copy link
Copy Markdown
Owner Author

🤖 Implemented by Codex

@jonit-dev jonit-dev force-pushed the night-watch/98-docs-prds-ci-auto-retry-md branch from a1dddc7 to bcc67f7 Compare May 12, 2026 21:36
@jonit-dev jonit-dev added the ready-to-merge PR is ready to merge label May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nw:ready-review Executor finished implementation and the PR is ready for automated/human review ready-to-merge PR is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant