RFC: Centralized community files and automated OWNERS sync#5
Conversation
fishman
requested review from
Nimbus318,
archlitchi,
wawa0210 and
william-wang
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fishman The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds workflow automation for syncing community files and generating per-repository OWNERS files from maintainer data, along with new governance documents and a maintained repository roster. ChangesCommunity file and OWNERS sync automation
Community governance documents
Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
fishman
force-pushed
the
community-file-sync
branch
from
04fba1e to
ba69edb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
.github/workflows/sync-community.yml (3)
20-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThird-party action pinned to a mutable tag.
BetaHuhn/repo-file-sync-action@v1is pinned to a floating tag rather than a commit SHA. Since it runs withsecrets.ORG_PAT(broad write access across many repos), pinning to a full commit SHA reduces supply-chain risk if the tag is ever repointed.🤖 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 @.github/workflows/sync-community.yml at line 20, The workflow step using BetaHuhn/repo-file-sync-action@v1 is pinned to a mutable tag; update that action reference to a full commit SHA in the sync-community workflow so the third-party action is locked to an immutable version, especially since it runs with secrets.ORG_PAT.
3-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrigger paths miss the sync config itself.
Path filters list
maintainers.yaml,sync-owners.py, and the community docs, but not.github/repo-file-sync.yml. Editing the sync config alone (e.g., adding/removing a target repo) won't trigger this workflow on push; contributors would need to rememberworkflow_dispatch.♻️ Suggested addition
paths: + - '.github/repo-file-sync.yml' - 'maintainers.yaml' - 'sync-owners.py'🤖 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 @.github/workflows/sync-community.yml around lines 3 - 13, The push trigger in the sync workflow is missing the sync configuration file itself, so edits to the repo-file sync setup will not automatically run the job. Update the path filter in the workflow’s on.push.paths list to include the sync config used by the repository sync process, alongside maintainers.yaml and sync-owners.py, so changes to that configuration also trigger the workflow on push.
19-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCheckout doesn't disable credential persistence.
Neither
actions/checkout@v4step setspersist-credentials: false. Since the checked-out git credentials aren't needed after checkout (writes happen viaGH_TOKEN/GH_PAT), disable persistence to reduce blast radius per zizmor'sartipackedfinding.🔒 Suggested fix (apply to both steps)
- uses: actions/checkout@v4 + with: + persist-credentials: falseAlso applies to: 28-28
🤖 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 @.github/workflows/sync-community.yml at line 19, The workflow’s two actions/checkout uses still persist git credentials unnecessarily. Update both checkout steps in the sync-community workflow to set persist-credentials to false so the temporary checkout token is not retained after cloning, and keep using GH_TOKEN/GH_PAT for any later write operations.Source: Linters/SAST tools
sync-owners.py (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog summary uses index-based team access instead of name-based lookup.
gen_ownerscorrectly resolves teams byname(project-maintainers/reviewers) regardless of order. This log line instead assumesteams[0]is maintainers andteams[1]is reviewers — it'll misreport counts (or raiseIndexErrorif teams are ever missing/reordered) even though the generatedOWNERSfile itself stays correct.♻️ Suggested fix
- print(f" {repo}: maintainers={len(entry['teams'][0].get('members',[]))}, reviewers={len(entry['teams'][1].get('members',[])) if len(entry['teams'])>1 else 0}") + teams = {t["name"]: t.get("members", []) for t in entry.get("teams", [])} + print(f" {repo}: maintainers={len(teams.get('project-maintainers', []))}, reviewers={len(teams.get('reviewers', []))}")🤖 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 `@sync-owners.py` at line 41, The summary print in gen_owners is still using positional access on entry['teams'], which can misreport maintainers/reviewers or fail if the order changes. Update that log line to resolve the project-maintainers and project-reviewers teams by name, matching the lookup logic already used in gen_owners, and then count members from those named team objects instead of relying on teams[0] and teams[1].
🤖 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/repo-file-sync.yml:
- Around line 6-14: The repo-file sync mapping is using the wrong governance
filename for the code of conduct, so the sync action will not find the source
file. Update the file mapping in the repo-file sync configuration to use the
same underscore-based name as the workflow trigger and generated doc, and verify
the affected entries in the sync config stay consistent with the actual source
filenames used by BetaHuhn/repo-file-sync-action.
In @.github/workflows/sync-community.yml:
- Around line 44-47: The push/update command in the sync loop is swallowing
failures because the fallback echo after the gh api call does not fail the step,
so broken updates can still report success. Update the logic around the OWNERS
push in the workflow so failures from the repository update path are surfaced as
a non-zero exit for the step/job, and keep the error handling tied to the
existing sync loop and gh api/branch=main command so any auth, 404, or sha
conflict stops the workflow instead of only printing a message.
- Around line 16-23: Add an explicit least-privilege permissions block to the
workflow jobs so the default GITHUB_TOKEN is not granted broad access. Update
the sync jobs in sync-community.yml, including the sync-static job and the other
affected job(s) in the workflow, to declare only the permissions needed for
actions/checkout and the repo-file-sync action; keep using secrets.ORG_PAT for
actual write operations. Use the job definitions and
BetaHuhn/repo-file-sync-action configuration as the anchor points when updating
the workflow.
- Around line 38-46: The OWNERS sync step always passes a sha to gh api in the
update/create request, which breaks file creation when OWNERS does not exist.
Update the generated/* loop in sync-community.yml so the gh api call only
includes the sha field when the existing file lookup for
repos/Project-HAMi/$repo/contents/OWNERS succeeds; keep the PUT path and
branch/message/content handling in the same block, and use the existing repo/dir
flow to detect whether to omit sha for new files.
In `@CONTRIBUTING.md`:
- Around line 124-129: The contributor workflow example still references the old
base branch name, so update the branch example in the workflow section to use
the repository’s current main branch instead of master. Adjust the wording in
the contributor guidance so it aligns with the main-based workflow and keeps new
contributors from following the wrong starting branch.
- Around line 22-24: The Code of Conduct link in the CONTRIBUTING content points
to a repo-local path that may break in mirrored copies if the synced filename
differs. Update the markdown link in the Code of Conduct section to use the
canonical community URL, or otherwise align the synced filename and link target
so the reference remains valid across replicas; use the existing Code of Conduct
section anchor in CONTRIBUTING.md as the place to make this change.
---
Nitpick comments:
In @.github/workflows/sync-community.yml:
- Line 20: The workflow step using BetaHuhn/repo-file-sync-action@v1 is pinned
to a mutable tag; update that action reference to a full commit SHA in the
sync-community workflow so the third-party action is locked to an immutable
version, especially since it runs with secrets.ORG_PAT.
- Around line 3-13: The push trigger in the sync workflow is missing the sync
configuration file itself, so edits to the repo-file sync setup will not
automatically run the job. Update the path filter in the workflow’s
on.push.paths list to include the sync config used by the repository sync
process, alongside maintainers.yaml and sync-owners.py, so changes to that
configuration also trigger the workflow on push.
- Line 19: The workflow’s two actions/checkout uses still persist git
credentials unnecessarily. Update both checkout steps in the sync-community
workflow to set persist-credentials to false so the temporary checkout token is
not retained after cloning, and keep using GH_TOKEN/GH_PAT for any later write
operations.
In `@sync-owners.py`:
- Line 41: The summary print in gen_owners is still using positional access on
entry['teams'], which can misreport maintainers/reviewers or fail if the order
changes. Update that log line to resolve the project-maintainers and
project-reviewers teams by name, matching the lookup logic already used in
gen_owners, and then count members from those named team objects instead of
relying on teams[0] and teams[1].
🪄 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 Plus
Run ID: 3060a656-5b0a-40a3-bd7b-5c8b0fda3ff3
📒 Files selected for processing (9)
.github/repo-file-sync.yml.github/workflows/sync-community.yml.gitignoreCODE_OF_CONDUCT.mdCONTRIBUTING.mdLICENSESECURITY.mdmaintainers.yamlsync-owners.py
fishman
force-pushed
the
community-file-sync
branch
2 times, most recently
from
5e1e109 to
a126f51
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sync-owners.py (1)
29-31: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
project_idused unvalidated as a path component.Static analysis flags
open(MAINTAINERS_FILE)for path traversal; the more relevant instance isrepo_dir = GENERATED_DIR / repowhererepo = entry["project_id"]comes straight frommaintainers.yaml. Since the workflow only triggers on pushes tomain(already reviewed/merged content), risk is low, but a stray..inproject_idwould still let it write outsidegenerated/.Also applies to: 36-39
🤖 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 `@sync-owners.py` around lines 29 - 31, `main` and the `repo_dir = GENERATED_DIR / repo` path construction use `entry["project_id"]` directly as a filesystem component, so sanitize or validate `project_id` before using it to build paths. Update the logic around `main` and the per-entry processing near the `repo_dir` assignment to reject absolute paths, `..`, and path separators, or normalize to a safe slug, then only allow writes inside `GENERATED_DIR`.Source: Linters/SAST tools
.github/workflows/sync-community.yml (1)
19-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout steps.Neither job needs the checked-out credential to persist (pushes go through
gh api/GH_PAT), so leaving the default risks credential leakage via workspace/artifacts.🔒 Suggested fix
- uses: actions/checkout@v4 + with: + persist-credentials: falseAlso applies to: 28-28
🤖 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 @.github/workflows/sync-community.yml at line 19, The checkout steps in the sync-community workflow are leaving Git credentials persisted by default, which should be disabled. Update the `actions/checkout` usages in the workflow jobs to set `persist-credentials: false`, since the pushes are handled separately through `gh api`/`GH_PAT`; make the change on each checkout step referenced by the workflow.Source: Linters/SAST tools
🤖 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 `@sync-owners.py`:
- Around line 35-41: The logging in sync-owners.py is using fragile positional
access on entry['teams'], which can crash or report wrong counts when team order
or length differs. Update the print logic to use the same name-based lookup
approach as gen_owners, retrieving project-maintainers and reviewers from a dict
(or equivalent safe lookup) before counting members, and fall back to 0 when a
team is missing.
---
Nitpick comments:
In @.github/workflows/sync-community.yml:
- Line 19: The checkout steps in the sync-community workflow are leaving Git
credentials persisted by default, which should be disabled. Update the
`actions/checkout` usages in the workflow jobs to set `persist-credentials:
false`, since the pushes are handled separately through `gh api`/`GH_PAT`; make
the change on each checkout step referenced by the workflow.
In `@sync-owners.py`:
- Around line 29-31: `main` and the `repo_dir = GENERATED_DIR / repo` path
construction use `entry["project_id"]` directly as a filesystem component, so
sanitize or validate `project_id` before using it to build paths. Update the
logic around `main` and the per-entry processing near the `repo_dir` assignment
to reject absolute paths, `..`, and path separators, or normalize to a safe
slug, then only allow writes inside `GENERATED_DIR`.
🪄 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 Plus
Run ID: 1ca8d5b4-84dd-4743-bbda-163c5d45cccb
📒 Files selected for processing (10)
.github/repo-file-sync.yml.github/workflows/sync-community.yml.gitignoreCODE_OF_CONDUCT.mdCONTRIBUTING.mdLICENSESECURITY.mdmaintainers.yamlproject.yamlsync-owners.py
✅ Files skipped from review due to trivial changes (6)
- SECURITY.md
- LICENSE
- .github/repo-file-sync.yml
- CODE_OF_CONDUCT.md
- .gitignore
- maintainers.yaml
fishman
force-pushed
the
community-file-sync
branch
from
a126f51 to
6c4a73e
Compare
fishman
force-pushed
the
community-file-sync
branch
from
09770fe to
af34111
Compare
|
I removed the fix for the validate-metadata from this PR. It's handled by #6 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sync-owners.py (2)
14-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMinor: empty OWNERS content when no members present.
If both
approversandreviewerslists are empty for a project,gen_ownersreturns just"\n", writing a near-emptyOWNERSfile that GitHub may reject or treat as invalid. Consider skipping the write (or logging a warning) when no roles are populated.🤖 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 `@sync-owners.py` around lines 14 - 22, The gen_owners function currently returns a near-empty OWNERS body when both approvers and reviewers are absent, so update the sync flow to detect that case before writing the file. Use gen_owners and the teams/ROLE_MAP data to check whether any role entries were added, and if none were, skip creating/updating OWNERS (or emit a warning) instead of writing just a newline.
31-37: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueNo validation of
project_idbefore use as a path/repo name.
entry["project_id"]is used directly to buildrepo_dirand (per the workflow) the target repo path for the GitHub API push. A malformed or unexpectedproject_id(e.g., containing/or..) would silently write into an unintended nested/parent directory. Sincemaintainers.yamlis repo-controlled, this is low risk today, but a simple sanity check would harden against future misconfiguration.🤖 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 `@sync-owners.py` around lines 31 - 37, The sync-owners flow uses entry["project_id"] directly as both the repository identifier and the directory name, so add a sanity check in the loop in sync-owners.py before creating repo_dir or writing OWNERS. Validate that project_id is a safe single path/repo segment (for example, reject values containing separators or traversal-like components) and skip or fail fast on invalid entries, keeping the existing logic in the maintainer processing block and gen_owners path unchanged for valid repos.
🤖 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.
Nitpick comments:
In `@sync-owners.py`:
- Around line 14-22: The gen_owners function currently returns a near-empty
OWNERS body when both approvers and reviewers are absent, so update the sync
flow to detect that case before writing the file. Use gen_owners and the
teams/ROLE_MAP data to check whether any role entries were added, and if none
were, skip creating/updating OWNERS (or emit a warning) instead of writing just
a newline.
- Around line 31-37: The sync-owners flow uses entry["project_id"] directly as
both the repository identifier and the directory name, so add a sanity check in
the loop in sync-owners.py before creating repo_dir or writing OWNERS. Validate
that project_id is a safe single path/repo segment (for example, reject values
containing separators or traversal-like components) and skip or fail fast on
invalid entries, keeping the existing logic in the maintainer processing block
and gen_owners path unchanged for valid repos.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bf88443-03b3-42a2-b1df-cc796672cac8
📒 Files selected for processing (1)
sync-owners.py
fishman
force-pushed
the
community-file-sync
branch
from
81cd016 to
6068b83
Compare
| - project_id: "HAMi" | ||
| org: "Project-HAMi" | ||
| teams: | ||
| - name: "project-maintainers" |
There was a problem hiding this comment.
please change that to 'approvers'
There was a problem hiding this comment.
The current way this works currently is that project-maintainers are translated to approvers in the OWNERS file. I wonder if the incorrect project_id is why the landscape is not updated. But should we separate project-maintainers from approvers and reviewers across the board? I'll check the .project CNCF spec again
…ration Signed-off-by: Reza Jelveh <fishmangit@dynamia.ai>
Signed-off-by: Reza Jelveh <fishmangit@dynamia.ai>
- Set `contents: read` at workflow and job levels for security - Refactor `sync-owners` to onditionally create or update OWNERS files Signed-off-by: Reza Jelveh <fishmangit@dynamia.ai>
…t URLs Replace the static sync of `CONTRIBUTING.md` via repo-file-sync with per-repo generation from a template using `sync-owners.py`. The template references `Project-HAMi/.project` and gets injected with the correct org and repo for each project. Update the workflow to push both `OWNERS` and `CONTRIBUTING.md` files to each target repository. Signed-off-by: Reza Jelveh <fishmangit@dynamia.ai>
…files - Add `persist-credentials: false` to checkout steps in workflows. - Skip writing OWNERS file when no approvers or reviewers are present. Signed-off-by: Reza Jelveh <fishmangit@dynamia.ai>
Replace the version tag `@v1` with its resolved commit hash to prevent the action from silently updating and reduce blast radius per zizmor's unpinned-action finding. Signed-off-by: Reza Jelveh <fishmangit@dynamia.ai>
fishman
force-pushed
the
community-file-sync
branch
from
f4223fc to
4d8b062
Compare
2 participants
Title: RFC: Centralized community files and automated OWNERS, and CONTRIBUTING sync
Summary:
This PR introduces a centralized workflow to manage community files (LICENSE, CODE_OF_CONDUCT, CONTRIBUTING, SECURITY) and OWNERS files across all active Project-HAMi repositories. Instead of maintaining copies in each repo and relying on manual PRs to update them, we define them once in
.projectand sync them automatically. Given that we currently are adjusting the CONTRIBUTING.md and will probably be tuning it this makes sense.Changes:
.github/repo-file-sync.yml– Configuration for repo-file-sync-action that mirrorsLICENSE,CODE_OF_CONDUCT.md,CONTRIBUTING.md, andSECURITY.mdto 15 repositories..github/workflows/sync-community.yml– GitHub Actions workflow triggered on pushes tomain(when any of the source files change) or manually. Contains two jobs:sync-static– runs the file sync action to push the four community files.sync-owners– runs a Python script (sync-owners.py) that readsmaintainers.yaml, generates per-repoOWNERSfiles, and pushes them via the GitHub API.sync-owners.py– GeneratesOWNERSfiles undergenerated/<project_id>/OWNERSwithapproversandreviewerslists extracted frommaintainers.yaml. The workflow then pushes each one to the corresponding repo.maintainers.yaml– Expanded to cover all repos (HAMi, HAMi-core, volcano-vgpu-device-plugin, ascend-device-plugin, k8s-dra-driver, etc.) with per-repo maintainer and reviewer teams.Standard community files –
LICENSE(Apache 2.0),CODE_OF_CONDUCT.md,CONTRIBUTING.md,SECURITY.mdadded to this repository as the single source of truth..gitignore– Addedgenerated/to ignore locally generated OWNERS files.What this accomplishes:
CONTRIBUTING.md). Update it here, and it propagates.maintainers.yaml. Add/remove a maintainer or reviewer here, and the workflow updates the corresponding repo’s OWNERS automatically.maintainers.yamlwill trigger a sync within minutes.Next stage: making files read-only:
Once the sync is proven, we can flag these files as read-only in each repository’s settings (branch protection rules or
.github/CODEOWNERS). This ensures no one can bypass the centralised workflow by editing a community file directly in a downstream repo. The sync action will overwrite any local changes, effectively enforcing the singleton source of truth.Usage:
To add a new repo to the sync:
repos:in.github/repo-file-sync.yml.project_identry inmaintainers.yaml.Open questions:
Summary by CodeRabbit
OWNERSandCONTRIBUTING.mdbased on the expanded maintainer roster.LICENSE,CODE_OF_CONDUCT.md,CONTRIBUTING.md, andSECURITY.mdwith clearer community, security, and contribution guidance (including AI assistance disclosure requirements).