OSAC-2870: check-ep-naming grandfather against live main, not just stale base_sha - #147
Conversation
PRE_COMMIT_PR_BASE_SHA is a snapshot from the pull_request webhook payload, captured at the PR's last open/synchronize event. It doesn't advance as main gains new commits, and re-running an old CI job replays that same stale payload rather than refreshing it. This caused a false-positive class of failure: a long-lived PR that hasn't been pushed to since some other, unrelated PR merged a still-non-compliant enhancements/ directory into main would fail check-ep-naming on that unrelated directory, even though the PR never touches it and it's already correctly grandfathered on main itself. Observed concretely on PR osac-project#121, which failed on enhancements/storage-control-plane-osac-2872 (merged by PR osac-project#134) despite never touching that path. Fix: grandfathering now also checks the live tip of the base branch (PRE_COMMIT_LIVE_BASE_REF, e.g. origin/main, fetched fresh at the start of every CI run) in addition to the stale base SHA — a path is grandfathered if it exists at either reference. This keeps enforcement scoped to genuinely new paths, so contributors actively fixing their own directory's naming are never blocked by an unrelated pre-existing violation elsewhere in the repo. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
WalkthroughChangesNaming validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PreCommit as pre-commit/action
participant Main as main
participant Validator as validate_paths
participant Git as git refs
PreCommit->>Main: provide PRE_COMMIT_LIVE_BASE_REF
Main->>Validator: pass base SHA and live base ref
Validator->>Git: resolve live base ref
Validator->>Git: check path at base SHA or live ref
Git-->>Validator: path existence results
Validator-->>Main: naming violations
Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ 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 |
tchughesiv
changed the title
check-ep-naming grandfather against live main, not just stale base_sha
|
@tchughesiv: This pull request references OSAC-2870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/scripts/check_ep_naming.py:
- Line 128: Update the base-reference resolution flow around
resolve_live_base_ref so an unresolved base SHA preserves fail-closed behavior
and does not enable live-ref grandfathering; alternatively, if the fallback is
intentional, revise the warning to describe it accurately and add a regression
test covering the fallback.
In @.github/scripts/test_check_ep_naming.py:
- Around line 186-227: Add a test alongside
test_pre_existing_on_live_main_but_absent_at_stale_base_sha_is_not_flagged that
uses a DESIGN.md or PRD.md path whose filename exists only at live_base_ref,
with the stale base lacking it, and assert no violations are reported. Ensure
the case specifically exercises live-ref filename grandfathering rather than
only directory grandfathering.
In @.github/workflows/pre-commit.yaml:
- Line 19: Update the pre-commit action reference in the workflow to use the
full release commit SHA instead of the mutable v3.0.1 tag, while preserving the
existing action version and workflow behavior.
In `@CONTRIBUTING.md`:
- Around line 44-47: Update the documentation paragraph describing pre-existing
files so it refers to files currently on the PR’s base branch rather than files
currently on main, matching the checker’s use of
github.event.pull_request.base.ref.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 383e0908-af74-4eb1-b5ef-d28cc9e3e4a8
📒 Files selected for processing (4)
.github/scripts/check_ep_naming.py.github/scripts/test_check_ep_naming.py.github/workflows/pre-commit.yamlCONTRIBUTING.md
… wording - check_ep_naming.py: when the base SHA is unresolvable (CI misconfig, e.g. a shallow checkout), also disable live-ref grandfathering, not just the base-SHA one. The warning already claims grandfathering is fully disabled and every path is validated as new — the code now actually matches that, instead of silently falling back to a partial (live-ref-only) grandfather check. - test_check_ep_naming.py: add coverage for live-ref grandfathering of the filename-casing check specifically (a separate code path from directory grandfathering), and a regression test proving the fail-closed guarantee above holds even when the live ref would otherwise have grandfathered the path. 25 -> 27 tests, all passing. - CONTRIBUTING.md: say 'the PR's base branch' instead of 'main', since the checker keys off github.event.pull_request.base.ref rather than a hardcoded branch name. Addresses CodeRabbit review comments on PR osac-project#147. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: tchughesiv The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
… wording - check_ep_naming.py: when the base SHA is unresolvable (CI misconfig, e.g. a shallow checkout), also disable live-ref grandfathering, not just the base-SHA one. The warning already claims grandfathering is fully disabled and every path is validated as new — the code now actually matches that, instead of silently falling back to a partial (live-ref-only) grandfather check. - test_check_ep_naming.py: add coverage for live-ref grandfathering of the filename-casing check specifically (a separate code path from directory grandfathering), and a regression test proving the fail-closed guarantee above holds even when the live ref would otherwise have grandfathered the path. 25 -> 27 tests, all passing. - CONTRIBUTING.md: say 'the PR's base branch' instead of 'main', since the checker keys off github.event.pull_request.base.ref rather than a hardcoded branch name. Addresses CodeRabbit review comments on PR osac-project#147. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
… wording - check_ep_naming.py: when the base SHA is unresolvable (CI misconfig, e.g. a shallow checkout), also disable live-ref grandfathering, not just the base-SHA one. The warning already claims grandfathering is fully disabled and every path is validated as new — the code now actually matches that, instead of silently falling back to a partial (live-ref-only) grandfather check. - test_check_ep_naming.py: add coverage for live-ref grandfathering of the filename-casing check specifically (a separate code path from directory grandfathering), and a regression test proving the fail-closed guarantee above holds even when the live ref would otherwise have grandfathered the path. 25 -> 27 tests, all passing. - CONTRIBUTING.md: say 'the PR's base branch' instead of 'main', since the checker keys off github.event.pull_request.base.ref rather than a hardcoded branch name. Addresses CodeRabbit review comments on PR osac-project#147. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
2 participants
Summary
Fixes a false-positive class of
check-ep-namingCI failure: a long-lived PR that hasn't been pushed to since some other, unrelated PR merged a still-non-compliantenhancements/directory intomainfails on that unrelated directory — even though the PR never touches it, and it's already correctly grandfathered onmainitself.Observed concretely on #121, which fails
check-ep-namingonenhancements/storage-control-plane-osac-2872(merged non-compliant by #134) despite never touching that path — because #121'sbase.sha(captured at its last open/synchronize event, 2026-07-16) predates #134's merge.Root cause
PRE_COMMIT_PR_BASE_SHA(set fromgithub.event.pull_request.base.sha) is a snapshot taken at the PR's last open/synchronize event. It does not advance just becausemaingains new commits, and re-running an old CI job replays that same stale payload rather than refreshing it. The naming check's grandfather logic only checked this one, potentially stale, reference — so anything that landed onmainafter that snapshot looks "new" to the script, regardless of whether the current PR touches it.Fix
Grandfathering now checks two references instead of one — a path is grandfathered if it exists at either:
PRE_COMMIT_PR_BASE_SHA(existing behavior, unchanged), orPRE_COMMIT_LIVE_BASE_REF— the live tip of the base branch (origin/main), fetched fresh at the start of every CI run (new).This keeps enforcement scoped to genuinely new paths. It's purely additive/supplementary: if the live ref isn't available for any reason (not fetched, older workflow run, local
pre-commit installwithout network), it silently falls back to base-SHA-only behavior — no new failure mode introduced.Critically, this also protects contributors who are actively fixing their own directory's naming (as prompted by the review comments just posted on
#13,#46,#54,#61,#70,#81,#88,#91,#117,#118,#119,#126,#127): renaming their own non-compliant directory to a compliant one should never be blocked by an unrelated pre-existing violation elsewhere in the repo picked up by--all-files.Changes
.github/workflows/pre-commit.yaml— addPRE_COMMIT_LIVE_BASE_REF: origin/${{ github.event.pull_request.base.ref }}env var (relies on the existingfetch-depth: 0checkout, which already fetches all branches)..github/scripts/check_ep_naming.py— newis_grandfathered()helper checks both references;validate_paths()andmain()thread the newlive_base_refparameter through. Docstring expanded to explain the staleness mechanism and fix..github/scripts/test_check_ep_naming.py— 3 new tests: the exact OSAC-2766: Design - Type-Safe Resource References #121 false-positive scenario (grandfathered on live main, absent at stale base_sha → not flagged), a regression check that genuinely-new bad names are still flagged when absent from both refs, and a fallback check when the live ref isn't resolvable. 22 → 25 tests, all passing.CONTRIBUTING.md— one-sentence clarification that grandfathering checks both the PR's base commit and the current tip ofmain.Testing
python3 -m unittest test_check_ep_naming— 25/25 pass.pre-commit run --all-fileslocally — all hooks pass, includingcheck-ep-namingandyamllinton the workflow file.check_ep_naming.pydirectly against every currentenhancements/*file withPRE_COMMIT_LIVE_BASE_REF=origin/main— clean exit 0.Notes for reviewers
origin/${{ github.event.pull_request.base.ref }}resolves as expected post-checkout (should, peractions/checkout's documentedfetch-depth: 0→ full-history-all-branches behavior, and the existingfetch-depth: 0step is unchanged) — worth confirming thecheck-ep-namingstep doesn't log the "PR base SHA ... not available" warning-path when this PR's own CI runs. If for some reason the live ref doesn't resolve, the fallback is silent and behavior is unchanged from today (i.e., this PR is a strict improvement, never a regression).#121directly (that's still blocked until either it's rebased, or OSAC-2553: Design: Catalog Items — UI Management #128 — which already renames the offending directory — merges); this PR fixes the underlying mechanism so this class of false positive stops recurring for any future PR in the same situation.Summary by CodeRabbit
Bug Fixes
Documentation