docs: Approve code review policy and CODEOWNERS layout#124
docs: Approve code review policy and CODEOWNERS layout#124jmccaffrey-nv wants to merge 5 commits into
Conversation
Adds an initial draft of the PLC +2 Code Review gate (MVSB-32942) along with a sectioned CODEOWNERS file mapping subsystems to placeholder @NVIDIA/flashdreams-* teams. The doc embeds the literal +2 Security Review Checklist from NVIDIA's PLC: Code Review Excellence training as an LLM-fillable table for reviewers. Both files are drafts pending team consensus on: - subsystem boundaries and per-recipe owner teams, - which paths require @NVIDIA/flashdreams-security sign-off, - branch protection settings to flip on (require Code Owner review, dismiss stale approvals, no admin bypass). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Signed-off-by: Jonathan McCaffrey <[email protected]>
Collapses the team layout down to: - @NVIDIA/flashdreams-maintainers (default for everything not below) - @NVIDIA/flashdreams-integrations (integrations/) - @NVIDIA/flashdreams-security (second approver on licensing, dependency manifests, and review-policy files) Removes per-recipe owner sub-teams (alpadreams-owners, cosmos-owners, wan-owners, lingbot-owners, causal-forcing-owners, ludus-renderer- owners) and per-subdir routing for core / infra / recipes / plugins / configs / tests / docs / CI — all fall through to maintainers. Individual names can be appended inline in CODEOWNERS later if a path warrants it. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Signed-off-by: Jonathan McCaffrey <[email protected]>
- Adds @NVIDIA/flashdreams-maintainers as an alternate owner on integrations/ so PRs are never blocked waiting on the integrations team alone. - Narrows LICENSE, reuse.toml, uv.lock, and flashdreams/pyproject.toml to @NVIDIA/flashdreams-security as sole approver. - Reframes docs/code_review.md to lead with CODEOWNERS-based community review (the GitHub Approve button + branch protection). The NVIDIA PLC "+2 Code Review" compliance requirement is now mentioned once, in a dedicated compliance-note section, rather than threading through the document. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Signed-off-by: Jonathan McCaffrey <[email protected]>
- "No bypass" exception now documents an issue and retains the audit trail, without the in-writing-approval phrasing. - "Becoming a code owner" explains the two CODEOWNERS reference forms (team handle vs individual @username) and notes that the @NVIDIA/* teams require NVIDIA org membership, so external contributors are recognised by being added as individuals. - Rephrases the "from a contributor's perspective" intro to describe the flow welcomingly rather than waving it off. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Signed-off-by: Jonathan McCaffrey <[email protected]>
Three cleanups from RFC feedback: - Section comment that read "remain maintainer-owned but require security sign-off too" misrepresented OR semantics; rewritten to describe the actual alternate-approver model. - pyproject.toml and uv.lock (root) now share security-only ownership, matching the reality that uv mutates them together. flashdreams/ and integrations/* workspace members resolve into the root uv.lock and have no per-package lockfile, so only their pyproject.toml files appear in CODEOWNERS. - LICENSE, LICENSES/, NOTICE, and reuse.toml are now grouped into a single "Licensing and attribution" section, all security-only. - Adds the nested integrations/alpadreams/ludus-renderer sub-workspace (its own pyproject.toml + uv.lock) to the dependency-manifest tier. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Signed-off-by: Jonathan McCaffrey <[email protected]>
| protection check. | ||
| 3. **CI and DCO must also be green** — formatting, type-checks, CI | ||
| tier markers per [`CONTRIBUTING.md` → Testing](../CONTRIBUTING.md#testing), | ||
| and a `Signed-off-by` trailer on every commit per |
There was a problem hiding this comment.
this is fragile and not secure anyway, could we request pgp-signed commits instead?
There was a problem hiding this comment.
OSRB Contribution requested this specific DCO language . We can add pgp-signed commits in addition, but I think need to keep Signed-off-by trailer , unless consult with OSRB to change to PGP signatures .
Use the following Developer Certificate of Origin (DCO) for 3rd party contributions to the NVIDIA project code: Link to DCO: https://developercertificate.org/ (Example NV project with DCO contribution process: https://github.com/NVIDIA/TensorRT/blob/main/CONTRIBUTING.md)
There was a problem hiding this comment.
Re-reading, https://developercertificate.org/ itself doesn't call for git commit --sign-off, possibly we can switch to PGP signing , change CONTRIBUTING.md, and keep the verbatim DCO text .
There was a problem hiding this comment.
There's another confluence version from OSRB which does have CONTRIBUTING.md template of --signoff .
But as that is outside of DCO exactly in CONTRIBUTING.md, probably we can substitute PGP-signed if that adds authentication .
https://nvidia.atlassian.net/wiki/spaces/OSS/pages/3293716536/DCO
#### Signing Your Work
* We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license.
* Any contribution which contains commits that are not Signed-Off will not be accepted.
* To sign off on a commit you simply use the `--signoff` (or `-s`) option when committing your changes:
```bash
$ git commit -s -m "Add cool feature."
This will append the following to your commit message:
Signed-off-by: Your Name <[email protected]>
- Full text of the DCO (https://developercertificate.org/):
...
| | **Default** (core, infra, recipes, plugins, configs, tests, docs, CI, container) | everything not listed below | `@NVIDIA/flashdreams-maintainers` | | ||
| | **Integrations** | `integrations/` (all recipes) | `@NVIDIA/flashdreams-integrations` *or* `@NVIDIA/flashdreams-maintainers` | | ||
| | **Licensing & attribution** | `LICENSE`, `LICENSES/`, `NOTICE`, `reuse.toml` | `@NVIDIA/flashdreams-security` | | ||
| | **Dependency manifests & lockfiles** | `pyproject.toml`, `uv.lock`, `flashdreams/pyproject.toml`, nested `ludus-renderer/{pyproject.toml,uv.lock}` | `@NVIDIA/flashdreams-security` | |
There was a problem hiding this comment.
only allowing the security team and not maintainers to update the lock / project files could be a bottleneck - can we make sure to have at least one (ideally more) security team members in each timezone (EU/US)?
There was a problem hiding this comment.
We can ensure we have timezone coverage in @NVIDIA/flashdreams-security.
My thinking with this CODEOWNERS setup is to control the dependencies more tightly than contributions in flashdreams code or integrations/ .
Greptile SummaryThis PR introduces the initial
Confidence Score: 3/5The CODEOWNERS file is safe to merge; the policy document needs its branch-protection claims aligned with what is actually enabled before or at merge time. The CODEOWNERS syntax, rule ordering, and glob scoping are all correct. The policy document's present-tense description of branch-protection settings that have not yet been enabled is the key concern — it could cause contributors or auditors to assume enforcement is in place before it actually is. docs/code_review.md — branch-protection tense/status needs to reflect what is actually enabled at merge time. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Contributor opens PR] --> B[GitHub reads CODEOWNERS\ncomputes owning teams]
B --> C{Paths touched}
C -->|integrations/*| D["@NVIDIA/flashdreams-integrations\nor @NVIDIA/flashdreams-maintainers"]
C -->|LICENSE, LICENSES/, NOTICE, reuse.toml\npyproject.toml, uv.lock| E["@NVIDIA/flashdreams-security"]
C -->|integrations/*/pyproject.toml| F["@NVIDIA/flashdreams-integrations\nor @NVIDIA/flashdreams-security"]
C -->|.github/CODEOWNERS, docs/code_review.md| G["@NVIDIA/flashdreams-maintainers\nor @NVIDIA/flashdreams-security"]
C -->|everything else| H["@NVIDIA/flashdreams-maintainers"]
D & E & F & G & H --> I[Code owner approves\nvia GitHub review UI]
I --> J{CI green?\nDCO signed?}
J -->|Yes| K[Maintainer squash-merges]
J -->|No| L[Fix and re-push\nstale approval dismissed]
L --> I
Reviews (1): Last reviewed commit: "docs: unify ownership tiers for licensin..." | Re-trigger Greptile |
| | Area | Paths | Code owners | | ||
| |------|-------|-------------| | ||
| | **Default** (core, infra, recipes, plugins, configs, tests, docs, CI, container) | everything not listed below | `@NVIDIA/flashdreams-maintainers` | | ||
| | **Integrations** | `integrations/` (all recipes) | `@NVIDIA/flashdreams-integrations` *or* `@NVIDIA/flashdreams-maintainers` | | ||
| | **Licensing & attribution** | `LICENSE`, `LICENSES/`, `NOTICE`, `reuse.toml` | `@NVIDIA/flashdreams-security` | | ||
| | **Dependency manifests & lockfiles** | `pyproject.toml`, `uv.lock`, `flashdreams/pyproject.toml`, nested `ludus-renderer/{pyproject.toml,uv.lock}` | `@NVIDIA/flashdreams-security` | | ||
| | **Per-integration packaging** | `integrations/*/pyproject.toml` | `@NVIDIA/flashdreams-integrations` *or* `@NVIDIA/flashdreams-security` | | ||
| | **Review-policy files** | `.github/CODEOWNERS`, `docs/code_review.md` | `@NVIDIA/flashdreams-maintainers` *or* `@NVIDIA/flashdreams-security` | | ||
|
|
||
| `@NVIDIA/flashdreams-maintainers` is the default owner for any path |
There was a problem hiding this comment.
Branch protection described as active before it is configured
The document uses present tense throughout the "How review and merge work on GitHub" section ("is configured so that … enabled"), and the "No bypass, no waiver" section repeats those settings as current fact. The PR description, however, calls these out as still pending: "branch protection settings to flip on (require Code Owner review, dismiss stale approvals, no admin bypass)". Once this document merges, any reader — including a new contributor or an auditor validating PLC compliance — will believe all of these controls are already enforced. If a direct push or admin bypass is exercised before the settings are actually enabled, the documentation will actively misrepresent the project's security posture. Consider using future tense ("will be configured") or adding a clearly-visible callout (e.g., a > **TODO:** block) until the branch-protection settings are confirmed live.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| CODEOWNERS rules are applied in order, with the **last matching | ||
| line winning**. A more specific path overrides a more general one. | ||
| If you are unsure who owns a file: | ||
|
|
||
| ```bash | ||
| # Validate the CODEOWNERS file itself | ||
| gh api repos/NVIDIA/flashdreams/codeowners/errors | ||
|
|
||
| # Inspect recent reviewers for a path |
There was a problem hiding this comment.
Doc table misrepresents
ludus-renderer/pyproject.toml ownership
The "Dependency manifests & lockfiles" row lists nested ludus-renderer/{pyproject.toml,uv.lock} with @NVIDIA/flashdreams-security as the sole owner. In the actual CODEOWNERS file, however, integrations/alpadreams/ludus-renderer/pyproject.toml is assigned to @NVIDIA/flashdreams-integrations @NVIDIA/flashdreams-security (i.e., either team can approve). Only the uv.lock for that path is security-only. A reviewer consulting the table to understand who must approve a ludus-renderer/pyproject.toml change will get incorrect information, potentially thinking security sign-off is mandatory when an integrations approval alone would satisfy the branch-protection check.
Adds an initial draft of the PLC +2 Code Review gate (MVSB-32942) along with a sectioned CODEOWNERS file mapping subsystems to placeholder @NVIDIA/flashdreams-* teams. The doc embeds the literal Security Review Checklist from NVIDIA's PLC: Code Review Excellence training as an LLM-fillable table for reviewers.
Both files are drafts pending team consensus on: