Skip to content

fix: wait for fresh CI after merger rebase#106

Merged
jonit-dev merged 1 commit into
masterfrom
codex/fix-merger-fresh-ci
May 7, 2026
Merged

fix: wait for fresh CI after merger rebase#106
jonit-dev merged 1 commit into
masterfrom
codex/fix-merger-fresh-ci

Conversation

@jonit-dev
Copy link
Copy Markdown
Owner

Summary

  • require merger CI checks to be attached to the current PR head before merging
  • after rebase, compare headRefOid before/after and wait for checks on the new head only
  • add smoke regressions for absent fresh checks and delayed passing fresh checks

Verification

  • bash -n scripts/night-watch-merger-cron.sh
  • yarn vitest run packages/cli/src/tests/scripts/core-flow-smoke.test.ts
  • yarn vitest run packages/cli/src/tests/scripts/core-flow-smoke.test.ts -t "merger should"
  • yarn verify

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 88/100

This PR adds robust head-tracking and CI verification logic to the merger script, ensuring fresh CI runs on rebased commits before merging. The implementation is thorough with excellent test coverage.


✅ Key Strengths

  • Correctness Improvement: The head OID tracking ensures CI checks are verified against the correct post-rebase commit, preventing a race condition where stale CI results could be incorrectly accepted.
  • Comprehensive Test Coverage: Three new smoke tests cover the key scenarios: unchanged head after rebase, changed head without passing CI, and changed head with eventual CI success.
  • Configurable Parameters: Adding NW_MERGER_CI_MAX_WAIT and NW_MERGER_CI_POLL_INTERVAL environment variables provides operational flexibility without code changes.
  • Robust Status Parsing: The ci_status_for_head() jq query correctly handles both CheckRun and StatusContext types with comprehensive failure/pending state detection.

⚠️ Areas for Improvement

  • Error Visibility: The ci_status_for_head() function returns "unknown" on errors, which suppresses the actual error. Consider logging the underlying gh/jq error before returning "unknown" for easier debugging.
  • Variable Scope: LAST_CI_STATUS is set as a global variable for communication between wait_for_ci_passing_on_head() and its caller. Consider returning this via echo instead for cleaner function interfaces.
  • Edge Case Documentation: Setting CI_MAX_WAIT=0 skips the polling loop entirely but still performs one CI check. Document this "check once" behavior if intentional.

📋 Issues Found

Issue Type Issue Name Affected Components Description Impact/Severity
Maintainability Global Variable Pattern night-watch-merger-cron.sh LAST_CI_STATUS uses global variable pattern for inter-function communication, making flow less explicit. Low
Maintainability Error Masking ci_status_for_head() jq errors are suppressed with `

🔚 Conclusion

This is a strong improvement to the merger's CI verification logic. The changes address a real correctness issue where rebased commits could be merged with stale CI results. The implementation is well-tested and ready for merge with only minor maintainability suggestions.


Analyzed using z-ai/glm-5

@jonit-dev jonit-dev merged commit 61ca5be into master May 7, 2026
5 checks passed
@jonit-dev jonit-dev deleted the codex/fix-merger-fresh-ci branch May 7, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant