Detect stale ready PR head drift#693
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
워크스루이 PR은 ready_to_merge 상태 이후 PR HEAD가 진전될 때를 감지하고 감사 가능한 상태 복구를 제공하며, 스택된 PR 베이스 병합 순서 해저드를 경고하는 기능을 구현합니다. recover-state.js에 객관적 드리프트 증거가 있을 때만 review_pending으로 되돌리는 새로운 전이 경로를 추가하고, run-preflight.js는 라이브 PR HEAD를 조회해 ready_status를 계산하며, finalize-run.js는 스택된 베이스 위험을 탐지하고 선택적으로 차단합니다. 변경 사항Ready-to-Merge Head Drift Detection and Recovery
🎯 4 (복합도) | ⏱️ ~45분
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relay Review Round 1Verdict: CHANGES_REQUESTED
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe039e73d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| prNumber: live.prNumber || prNumber, | ||
| headRefName: live.headRefName, | ||
| oldSha: differsFromReviewed ? lastReviewedSha : manifestHeadSha, | ||
| newSha: live.headSha, |
There was a problem hiding this comment.
When a ready_to_merge PR was advanced from another checkout, this recovery accepts the live GitHub headRefOid and stamps it into the manifest, but it never fetches or verifies that the retained worktree/branch is actually at that SHA. The next review-runner still reviews git rev-parse HEAD from the local checkout, so it can review the old code (or overwrite the manifest back to the old SHA), leaving the stale-ready recovery unable to produce a fresh review of the new PR head. Please fetch/update or refuse unless the local review checkout matches live.headSha.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
skills/relay-merge/scripts/finalize-run.js (1)
683-700: ⚖️ Poor tradeoff중복 로직 추출 고려 (선택 사항)
세 경로(skip-review, READY_TO_MERGE, force-finalize)에서 거의 동일한 패턴이 반복됩니다:
buildStackedBaseGuard호출- blocked 상태면
MERGE_BLOCKED이벤트 기록assertStackedBaseGuard호출현재 코드는 각 경로의 컨텍스트(head_sha 소스 등)가 명시적으로 드러나 디버깅에 유리합니다. 향후 가드 로직이 복잡해지면 헬퍼 함수로 추출을 검토해 볼 수 있습니다.
Also applies to: 744-760, 764-780
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/relay-merge/scripts/finalize-run.js` around lines 683 - 700, The repeated pattern calling buildStackedBaseGuard(...), conditionally appending a MERGE_BLOCKED event via appendRunEvent(repoPath, safeData.run_id, { event: EVENTS.MERGE_BLOCKED, ... }), and then calling assertStackedBaseGuard(...) should be extracted into a small helper to reduce duplication; implement a function (e.g., ensureStackedBaseGuard(repoPath, prNumber, headSha, stackedBaseOverrideReason, safeData)) that calls buildStackedBaseGuard with the provided args, appends the MERGE_BLOCKED event with the correct context (use safeData.run_id, safeData.state, currentHeadSha / passed headSha, safeData.review?.rounds), and finally calls assertStackedBaseGuard, then replace the three duplicated blocks (including the one using preMerge, and the blocks around lines cited) with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/relay-dispatch/scripts/recover-state.test.js`:
- Around line 126-146: writeGhPrHeadScript currently only overrides PATH so an
external RELAY_GH_BIN in the parent environment can bypass the stub and make
recover-state.js tests flaky; update writeGhPrHeadScript to return an env
mapping that also sets RELAY_GH_BIN to the stub gh path (or explicitly unsets
it) so the test process always uses the created stub; reference the
writeGhPrHeadScript helper and the recover-state.js usage and ensure the
returned object includes RELAY_GH_BIN: ghPath (and still composes PATH) to
prevent inheritance from parent env.
In `@tests/relay/scripts/run-preflight.test.js`:
- Around line 76-77: The test fixture returns an env that only mutates PATH,
leaving RELAY_GH_BIN to inherit from the host and potentially bypass test stubs;
update the returned env object (the env property alongside repoRoot, runId,
manifestPath) to explicitly set RELAY_GH_BIN to the test stub inside binDir
(e.g., RELAY_GH_BIN: `${binDir}/gh`) or to an empty/controlled value so the test
cannot pick up an external binary, ensuring binDir is used instead of
process.env.RELAY_GH_BIN.
---
Nitpick comments:
In `@skills/relay-merge/scripts/finalize-run.js`:
- Around line 683-700: The repeated pattern calling buildStackedBaseGuard(...),
conditionally appending a MERGE_BLOCKED event via appendRunEvent(repoPath,
safeData.run_id, { event: EVENTS.MERGE_BLOCKED, ... }), and then calling
assertStackedBaseGuard(...) should be extracted into a small helper to reduce
duplication; implement a function (e.g., ensureStackedBaseGuard(repoPath,
prNumber, headSha, stackedBaseOverrideReason, safeData)) that calls
buildStackedBaseGuard with the provided args, appends the MERGE_BLOCKED event
with the correct context (use safeData.run_id, safeData.state, currentHeadSha /
passed headSha, safeData.review?.rounds), and finally calls
assertStackedBaseGuard, then replace the three duplicated blocks (including the
one using preMerge, and the blocks around lines cited) with calls to this
helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2bce686-23f4-47bf-bcd0-8d542cc054d2
📒 Files selected for processing (10)
skills/relay-dispatch/references/recovery-playbook.mdskills/relay-dispatch/scripts/cli-schema.jsskills/relay-dispatch/scripts/recover-state.jsskills/relay-merge/scripts/finalize-run.jsskills/relay/references/preflight-guards.mdskills/relay/scripts/run-preflight.jstests/relay-dispatch/scripts/recover-state.test.jstests/relay-merge/scripts/finalize-run.test.jstests/relay-review/scripts/review-runner.test.jstests/relay/scripts/run-preflight.test.js
| function writeGhPrHeadScript(headRefOid, { number = 334, headRefName = "issue-211" } = {}) { | ||
| const binDir = fs.mkdtempSync(path.join(os.tmpdir(), "relay-gh-head-")); | ||
| const ghPath = path.join(binDir, "gh"); | ||
| fs.writeFileSync(ghPath, `#!/usr/bin/env node | ||
| const args = process.argv.slice(2); | ||
| if (args[0] === "pr" && args[1] === "view") { | ||
| process.stdout.write(JSON.stringify({ | ||
| number: ${JSON.stringify(number)}, | ||
| headRefName: ${JSON.stringify(headRefName)}, | ||
| headRefOid: ${JSON.stringify(headRefOid)} | ||
| })); | ||
| process.exit(0); | ||
| } | ||
| console.error("unexpected gh args: " + args.join(" ")); | ||
| process.exit(2); | ||
| `, "utf-8"); | ||
| fs.chmodSync(ghPath, 0o755); | ||
| return { | ||
| PATH: `${binDir}${path.delimiter}${process.env.PATH || ""}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
GH 스텁이 RELAY_GH_BIN 상속값에 의해 우회될 수 있습니다.
현재 helper는 PATH만 덮어써서, 상위 환경에 RELAY_GH_BIN이 있으면 recover-state.js가 해당 바이너리를 우선 사용해 테스트가 비결정적으로 깨질 수 있습니다. 테스트 env에 RELAY_GH_BIN을 명시해 고정해 주세요.
패치 제안
function writeGhPrHeadScript(headRefOid, { number = 334, headRefName = "issue-211" } = {}) {
@@
return {
PATH: `${binDir}${path.delimiter}${process.env.PATH || ""}`,
+ RELAY_GH_BIN: ghPath,
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/relay-dispatch/scripts/recover-state.test.js` around lines 126 - 146,
writeGhPrHeadScript currently only overrides PATH so an external RELAY_GH_BIN in
the parent environment can bypass the stub and make recover-state.js tests
flaky; update writeGhPrHeadScript to return an env mapping that also sets
RELAY_GH_BIN to the stub gh path (or explicitly unsets it) so the test process
always uses the created stub; reference the writeGhPrHeadScript helper and the
recover-state.js usage and ensure the returned object includes RELAY_GH_BIN:
ghPath (and still composes PATH) to prevent inheritance from parent env.
| return { repoRoot, runId, manifestPath, env: { ...process.env, PATH: `${binDir}:${process.env.PATH}` } }; | ||
| } |
There was a problem hiding this comment.
테스트 환경에서 RELAY_GH_BIN을 고정하지 않아 외부 설정에 취약합니다.
run-preflight.js는 RELAY_GH_BIN을 PATH보다 우선 사용합니다. 현재 fixture env는 PATH만 바꿔서, CI/로컬의 기존 RELAY_GH_BIN 값에 따라 테스트가 스텁을 우회할 수 있습니다.
패치 제안
- return { repoRoot, runId, manifestPath, env: { ...process.env, PATH: `${binDir}:${process.env.PATH}` } };
+ return {
+ repoRoot,
+ runId,
+ manifestPath,
+ env: {
+ ...process.env,
+ PATH: `${binDir}:${process.env.PATH}`,
+ RELAY_GH_BIN: ghPath,
+ },
+ };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/relay/scripts/run-preflight.test.js` around lines 76 - 77, The test
fixture returns an env that only mutates PATH, leaving RELAY_GH_BIN to inherit
from the host and potentially bypass test stubs; update the returned env object
(the env property alongside repoRoot, runId, manifestPath) to explicitly set
RELAY_GH_BIN to the test stub inside binDir (e.g., RELAY_GH_BIN: `${binDir}/gh`)
or to an empty/controlled value so the test cannot pick up an external binary,
ensuring binDir is used instead of process.env.RELAY_GH_BIN.
Relay Review Round 2Verdict: CHANGES_REQUESTED
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d36a217a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const merged = candidates.find((entry) => normalizePrState(entry.state) === "MERGED" || Boolean(entry.mergedAt)); | ||
| if (merged) { |
There was a problem hiding this comment.
When a stacked base branch name has been reused, gh pr list --head <base> --state all can return both an older merged PR and the current open/unmerged base PR. Because this clears the guard as soon as it finds any merged candidate, the later OPEN candidate is ignored and the dependent PR can be merged even though its current non-default base branch is still unmerged. Prefer blocking when any matching open PR exists, or otherwise ensure the merged PR corresponds to the current base branch head.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1e044dd0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| status: overrideReason ? "overridden" : "blocked", | ||
| reason, |
There was a problem hiding this comment.
When a relay run is created from a long-lived non-default branch such as release/1.2 or develop, the PR base is intentionally not the repository default, but there may be no PR whose head branch is that base. In that case fetchPrsForHeadBranch() returns no candidates and this path marks the guard as blocked with base_pr_missing, so finalize-run refuses to merge an otherwise valid PR unless the operator uses an override. The guard should distinguish actual stacked bases from normal non-default target branches, for example by consulting the manifest/base policy or only blocking when an unmerged base PR is found.
Useful? React with 👍 / 👎.
Relay ReviewVerdict: LGTM |
1 participant
Closes #691.
Summary
ready_statusforready_to_mergeruns by comparing GitHub's live PRheadRefOidwithreview.last_reviewed_sha/git.head_sha.recover-statesupport forready_to_merge -> review_pendingonly when live PR HEAD drift is objective evidence; the recovery updatesgit.head_shaand emitsstate_recoverywithpr_number,previous_head_sha,new_head_sha,last_reviewed_sha, and the operator reason.--allow-stacked-base-hazard <reason>as the explicit override.Validation
node --test tests/relay/scripts/run-preflight.test.js tests/relay-dispatch/scripts/recover-state.test.js tests/relay-dispatch/scripts/relay-events.test.js tests/relay-dispatch/scripts/cli-schema.test.js tests/relay-dispatch/scripts/cli-unknown-flag-wiring.test.jsnode --test tests/relay-review/scripts/review-runner*.test.jsnode --test tests/relay-merge/scripts/gate-check.test.js tests/relay-merge/scripts/finalize-run.test.jsgit diff --checkgit diff --cached --checkDeferred
Summary by CodeRabbit
릴리스 노트
새로운 기능
문서