Skip to content

fix: stop CODEOWNERS edits from requesting every owner - #15244

Merged
dagil-nvidia merged 3 commits into
mainfrom
dagil-nvidia/codeowners-request-scope
Sep 29, 2026
Merged

dagil-nvidia merged 3 commits into
mainfrom
dagil-nvidia/codeowners-request-scope

Conversation

@dagil-nvidia

@dagil-nvidia dagil-nvidia commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Overview:

Editing CODEOWNERS requested every codeowner group. GitHub requests every team on the last matching line, and that line named all of them.

Details:

  • Drop the shared rule that listed every area on CODEOWNERS.
  • The process area remains the only owner of the generated file.
  • areas.yaml stays shared by ops and process, so one process review still covers the source and the artifact.
  • Add a test that the CODEOWNERS path resolves to the process team only.

Where should the reviewer start?

.github/codeowners/areas.yaml

Related Issues

🚫 This PR is NOT linked to an issue:

  • Confirmed — no related issue

Summary by CodeRabbit

  • Chores
    • Updated repository ownership rules so the CODEOWNERS file is assigned to the process area, while area rules remain shared by ops and process.
  • Tests
    • Added a check to verify the CODEOWNERS file resolves to the intended owner.

GitHub requests every team on the last matching line. The generated
file listed every codeowner group, so any edit paged all of them.

Signed-off-by: Dan Gil <dagil@nvidia.com>
@dagil-nvidia
dagil-nvidia requested review from a team as code owners September 24, 2026 01:26
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d16e10f0-bf8c-4cda-bcf8-c3483fa79b59

📥 Commits

Reviewing files that changed from the base of the PR and between a6a2001 and 12f6031.

📒 Files selected for processing (3)
  • .github/codeowners/areas.yaml
  • .github/codeowners/test_codeowners.py
  • CODEOWNERS
💤 Files with no reviewable changes (1)
  • CODEOWNERS

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


Walkthrough

The ownership configuration removes the shared CODEOWNERS rule. Comments specify process-area ownership, and a regression test checks that CODEOWNERS resolves only to the process team.

Changes

CODEOWNERS ownership

Layer / File(s) Summary
Ownership rule and regression test
.github/codeowners/areas.yaml, CODEOWNERS, .github/codeowners/test_codeowners.py
The ownership source and generated CODEOWNERS file remove the shared ownership rule. A regression test checks that CODEOWNERS resolves only to @ai-dynamo/dynamo-process-codeowners.

Priority: ⬇️ Low

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

Merge Risk: ⚪ Minimal · up to 12f60

CODEOWNERS edits now request only the process team. No issue requiring a change before merge was identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preventing CODEOWNERS edits from requesting every owner.
Description check ✅ Passed The description includes all required sections. It explains the cause, lists the configuration and test changes, identifies the reviewer starting point, and confirms that no related issue exists.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI

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

Comment thread .github/codeowners/test_codeowners.py

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not approving yet. The one finding is a P2, but it asks you to choose who approves routing changes. My approval alone meets every code-owner rule on this PR, so I will wait for that choice. If the next head has either fix from the inline comment and no new defect, I will approve it.

Read at 12f60317d5, and merged locally with main at a6a2001f06.

  • [P2] .github/codeowners/areas.yaml:563-565: the comment says that ops or process can unblock a routing change. After this PR, only a process approval does.
  • No path other than CODEOWNERS changes owners. The codeowners gates pass on this head and on the merge. The new test fails on a copy that restores the removed rule.
  • The change that the bot asks for at test_codeowners.py:1140 is optional. Its reference to pull request 15170 is accurate, and .ai/linear-ticket-refs.md allows GitHub numbers in code comments.

Comment thread .github/codeowners/areas.yaml Outdated

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 12f60317d5. I accept process as the only approver of changes to the generated CODEOWNERS. That settles the choice that my earlier review asked for.

The evidence is in that review. Only CODEOWNERS changes owners. The gates pass on the head and on a merge with main. The new test fails on the base. Since then, main gained 4 commits, and none of them touches CODEOWNERS or .github/codeowners/.

One item stays open. The comment at .github/codeowners/areas.yaml:563-565 still says that ops or process can unblock a routing change. Please reword it to say that a process approval is required. That thread stays open for it.

@dagil-nvidia

Copy link
Copy Markdown
Collaborator Author

/ok to test 12f6031

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve again at 106868b038, the merge of main into this branch. The merge changed none of the three files in this PR, and one P2 is still open.

The merge also changed nothing under .github/codeowners/ or in the codeowners workflow. The codeowners job passes on this head. The new test still fails on the old CODEOWNERS from main, so it still guards this fix.

[P2], still open: the comment at .github/codeowners/areas.yaml:563-565 says that ops or process can unblock a routing change. After this PR, only a process approval can. The inline thread stays open for it.

A routing change regenerates CODEOWNERS, which process owns alone, so an
ops approval no longer unblocks one.

Signed-off-by: Dan Gil <dagil@nvidia.com>

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this PR at d309d6fd28. The new commit fixes my last open finding, and no finding of mine is open.

What I checked at d309d6fd28.

The commit changes only comment lines in .github/codeowners/areas.yaml. The parsed file is the same as at 106868b038, and CODEOWNERS and CONTRIBUTORS.md did not change.

The new comment is correct. The matcher of the repository gives process as the only owner of CODEOWNERS, and ops and process as the owners of areas.yaml. So only a process approval covers a routing change.

The unit tests (158 pass), build_codeowners.py --strict, and the drift check pass on this head. They also pass on its merge with main at 86d68a4856. The codeowners job passes on this head.

@dagil-nvidia

Copy link
Copy Markdown
Collaborator Author

Admin merge at d309d6fd28. All seven required checks pass on this head, approvals are in, and every review thread is resolved. The last commit only rewords a comment in .github/codeowners/areas.yaml, as the reviewer asked; the admin override skips waiting on the non-required checks that are still pending.

@dagil-nvidia
dagil-nvidia merged commit 4dc45f7 into main Sep 29, 2026
107 checks passed
@dagil-nvidia
dagil-nvidia deleted the dagil-nvidia/codeowners-request-scope branch September 29, 2026 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants