Skip to content

feat(pr-gate): word the latest-dev box as the condition the gate enforces - #5471

Open
abhisheksharma2411 wants to merge 1 commit into
lidge-jun:devfrom
abhisheksharma2411:feat/readiness-latest-dev-wording-4443
Open

abhisheksharma2411 wants to merge 1 commit into
lidge-jun:devfrom
abhisheksharma2411:feat/readiness-latest-dev-wording-4443

Conversation

@abhisheksharma2411

@abhisheksharma2411 abhisheksharma2411 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Closes #4443.

The two requirements disagreed

The readiness box says:

I pushed my PR to the latest dev commit.

The gate says something else. readinessClaimViolations in pr-quality-state.cjs clears the claim while the head is at most READINESS_LATEST_DEV_BEHIND_MAX10 — commits behind the base:

if (behindUnknown || behindBase > behindMax) {
  violations.push("latest_dev");
}

So the box asks for the exact tip and the enforcement tolerates ten behind. On a fast-moving dev that gap is a treadmill, exactly as the issue describes: an author who reads the box literally resyncs for unrelated commits → the head moves → head-drift unticks all four boxes → the exact-head CI evidence from the previous round is invalidated → repeat. None of it reduces merge risk, because the gate was already satisfied.

The change

The sentence is now derived from the constant instead of written next to it:

function latestDevReadinessItem() {
  return (
    "I pushed my PR to a recent dev commit " +
    `(at most ${READINESS_LATEST_DEV_BEHIND_MAX} behind; ` +
    "a maintainer may still ask for the exact tip before merge)."
  );
}

rendering as:

- [ ] I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

Deriving it is the durable half: the wording and the threshold cannot drift apart again, and raising or lowering the tolerance rewords the box in the same commit. The trailing clause keeps exact-tip sync available as a merge boundary rather than a continuously maintained condition, which is what the issue asked for.

Backward compatibility — proved by the existing suites, not just asserted

Rewording a checklist could plausibly untick every open PR. It doesn't, and the repo's own machinery is why:

  • extractReviewReadiness matches /^\s*[-*]\s+\[([ xX])\]\s+/ — box count and checked state, never item text. Its doc comment already says so: "The author can reword an item, but the box count and the checked state are the contract."
  • appendReviewReadinessSection is idempotent: a body that already carries the marker pair is returned unchanged.

And this is exercised today, in both runners, against the old wording:

  • pr-quality.test.cjs builds a checked body with "- [x] I pushed my PR to the latest dev commit." and asserts uncheckReviewReadinessBoxes unticks by index while preserving the text.
  • tests/ci-workflows/ci-workflows.test.ts drives the real gate through the readinessChecklistBody fixture, which carries the same old line, and asserts on it after a behind_by: 11 run.

Both pass untouched on this branch. An open PR keeps its sentence and its ticks; only newly appended checklists use the new one.

Verification

node --test .github/scripts/pr-quality.test.cjs 81 pass / 0 fail (6 new)
all 22 .github/scripts/*.test.cjs suites every one 0 fail (pr-quality-state 43, issue-quality 118, enforce-pr-target 20, …)
bun test --isolate tests/ci-workflows/ 1114 pass / 3 fail, 8 skipped

The 3 failures are pre-existing: the same three fail on clean dev (a bare pass run removes its preload-owned TEMP root, a bare fail run …, release shell recovers only unverified reads after acknowledged publication).

One of the new tests is the one that makes the sentence honest — it checks the gate at the boundary the box now promises:

assert.deepEqual(readinessClaimViolations({ behindBase: READINESS_LATEST_DEV_BEHIND_MAX }), []);
assert.deepEqual(readinessClaimViolations({ behindBase: READINESS_LATEST_DEV_BEHIND_MAX + 1 }), ["latest_dev"]);

Mutation-tested 3/3:

mutation result
restore the hardcoded exact-tip sentence 3 fail
hardcode a number instead of deriving it 1 fail (the drift guard)
drop the maintainer escape clause 1 fail

Scope

Wording and its derivation only. READINESS_LATEST_DEV_BEHIND_MAX keeps its current value of 10, and no gate logic changes — whether ten is the right tolerance is a separate decision, and this PR deliberately does not make it.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Updated the PR readiness checklist to allow commits within the configured distance from the latest development commit.
    • The checklist now clearly displays the applicable threshold while preserving maintainer verification of the exact commit when needed.
  • Tests

    • Added coverage for threshold boundaries, checklist compatibility, and preservation of the four-item structure.

…rces

The readiness box read "I pushed my PR to the latest dev commit", which asks for
the exact tip. The gate does not: `readinessClaimViolations` clears the claim
while the head is at most `READINESS_LATEST_DEV_BEHIND_MAX` (10) commits behind
the base.

On a fast-moving `dev` that gap is a treadmill. An author who reads the box
literally resyncs for unrelated commits; every resync moves the head; head-drift
unticks all four boxes; and the exact-head CI evidence the previous round
produced is invalidated. None of it reduces merge risk, because the gate was
already satisfied — the two requirements simply pull in different directions.

The sentence is now derived from the constant rather than written beside it, so
the wording and the threshold cannot drift apart again: changing the tolerance
rewords the box in the same commit. It also states that a maintainer may still
ask for the exact tip, which keeps exact-tip sync available as a merge boundary
instead of a continuously maintained condition.

Rewording is safe for open pull requests, and the existing suites already prove
it: `extractReviewReadiness` matches box count and checked state, never item
text, and `appendReviewReadinessSection` returns a body that already carries the
marker pair unchanged. Both the Node fixtures in pr-quality.test.cjs and the Bun
`readinessChecklistBody` fixture build an old-worded checklist, and all of them
still pass untouched — an open PR keeps its sentence and its ticks.

Six new tests, including the boundary one that makes the sentence honest: the
gate clears exactly the number the box promises, and one more is a violation.
Mutation-tested 3/3 — restoring the hardcoded exact-tip sentence, hardcoding a
number instead of deriving it, and dropping the maintainer clause each fail.

All 22 .github/scripts suites pass. tests/ci-workflows/ is 1114 pass / 3 fail,
the same three that fail on clean dev.

Closes lidge-jun#4443
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the enhancement New feature or request label Sep 21, 2026
@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1bc4836e-e85e-43f8-beef-a2706d6dd3bc

📥 Commits

Reviewing files that changed from the base of the PR and between e4ceeb3 and a581063.

📒 Files selected for processing (2)
  • .github/scripts/pr-quality.cjs
  • .github/scripts/pr-quality.test.cjs

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The PR-quality checklist now derives its latest-dev wording from READINESS_LATEST_DEV_BEHIND_MAX. Regression tests cover threshold boundaries, maintainer exact-tip access, checklist structure, and compatibility with legacy wording.

Changes

Latest-dev readiness contract

Layer / File(s) Summary
Threshold-aware checklist wording and validation
.github/scripts/pr-quality.cjs:9-13, .github/scripts/pr-quality.cjs:27-55, .github/scripts/pr-quality.cjs:62-64, .github/scripts/pr-quality.test.cjs:1104-1174
REVIEW_READINESS_ITEMS now uses wording generated from READINESS_LATEST_DEV_BEHIND_MAX. Tests verify the configured threshold, boundary behavior, maintainer exact-tip access, four-item structure, and legacy checklist compatibility.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #4443 requires checklist wording to match the existing bounded latest-dev gate, while preserving CI, review, target-branch, and ancestry requirements. .github/scripts/pr-quality.cjs derives th…
Out of Scope Changes check ✅ Passed The reviewed changes are limited to .github/scripts/pr-quality.cjs and .github/scripts/pr-quality.test.cjs. They update the latest-dev checklist wording, derive it from the existing gate constant,…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: updating the latest-dev checklist wording to match the gate condition.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 18:53
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 71 / 80

체크리스트 한 칸이 “가장 최신 dev에 맞춰 밀어라”고 적혀 있었어요. 그런데 문이 실제로 보는 규칙은 “base보다 최대 10개만 뒤처지면 괜찮다”예요. 빠른 dev에서는 그 차이가 트레드밀이 돼요. 글자 그대로 맞추려 다시 동기화하면 헤드가 바뀌고, 헤드가 바뀌면 네 칸이 모두 풀리고, 직전에 맞춘 CI 증거가 다시 깨져요. 문은 이미 통과한 상태인데도요.

이 PR은 그 칸의 문장을 문 상수 READINESS_LATEST_DEV_BEHIND_MAX에서 바로 만들어요. 이제 “최근 dev, 최대 N개 뒤, 메인테이너가 합치기 전에 끝 커밋을 시킬 수 있다”고 적혀요. N을 바꾸면 같은 커밋에서 칸 글자도 같이 바뀌어요. 게이트 숫자나 로직은 건드리지 않아요. 이미 열려 있는 PR은 옛 문장과 체크를 그대로 두고, 새로 붙는 체크리스트만 새 문장을 써요. 그걸 지키는 테스트 여섯 개가 추가됐어요. base는 dev이고 #4443을 닫겠다고 해요. draft예요.

.github/scripts/pr-quality.cjs latestDevReadinessItem - 새 문장은 “몇 개 뒤까지 괜찮은지”만 말해요. 이슈 #4443은 “그사이에 들어온 dev 변경이 이 PR과 겹치는지 봤는지”도 적자고 했어요. 그 약속 문장은 없어요. 지금 그대로 Closes #4443이면 이슈가 원한 반쪽만 닫혀요.

메인테이너의 판단이 필요한 지점

#4443을 “문과 글자가 같아지면 충분”으로 닫을지, 겹침 확인 한 줄을 더 넣을지 정해 주세요. 10이 맞는 여유치인지는 이 PR이 일부러 안 건드렸어요. 같은 이슈를 노리는 다른 열린 PR은 없어요.

너의 추천

방향은 맞아요. 문과 칸이 서로 다른 말을 하던 구멍은 이걸로 막혀요. 테스트도 그 경계를 직접 고정해요. 겹침 확인까지 이슈에 넣을 거면 칸에 한 줄 더 쓰고, 아니면 #4443을 부분 해결로 두고 나머지를 새 이슈로 쪼개는 편이 덜 헷갈려요. 게이트 숫자를 바꾸는 커밋은 이 PR에 섞지 마세요.

이 댓글은 grok-bot이 작성했습니다

@github-actions
github-actions Bot marked this pull request as ready for review September 21, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants