Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
# FlashDreams CODEOWNERS
#
# Declares the GitHub teams responsible for approving changes in each
# part of the repository. Source of truth for code review
# responsibility, per CONTRIBUTING.md and docs/code_review.md.
#
# Format reference: https://docs.github.com/en/repositories/managing-your-repositories-settings-and-security/managing-repository-settings/about-code-owners
#
# Teams (kept intentionally small):
# @NVIDIA/flashdreams-maintainers — default owners; cover core,
# infra, recipes, plugins, configs,
# tests, CI, docs, container, and
# workspace metadata.
# @NVIDIA/flashdreams-integrations — per-recipe maintainers under
# integrations/. Maintainers can
# also review here.
# @NVIDIA/flashdreams-security — sole or alternate approver on
# licensing, attribution, and
# dependency-manifest files.
#
# Notes:
# * GitHub honours one CODEOWNERS file per repository; this is it.
# "Delegation" between subsystems is expressed by path-prefix rules
# below. The last matching rule wins, so more specific paths appear
# below more general ones.
# * When multiple owners are listed on a single rule, GitHub treats
# them as alternatives (OR) — any *one* of them satisfies the
# required-review-from-Code-Owners check.
# * Owners listed here are the project's qualified code-review
# reviewers. They apply the security review checklist in
# docs/code_review.md on every review. Branch protection on `main`
# requires at least one approving review from a CODEOWNER for the
# touched paths.
# * External contributors are welcome on every path. CODEOWNERS does
# not gate who may *open* a PR — only who must *approve* it.
# * The review requirement cannot be bypassed or waived; any
# exception requires a documented audit trail.

# ---------------------------------------------------------------------
# Default owners
# ---------------------------------------------------------------------
* @NVIDIA/flashdreams-maintainers

# ---------------------------------------------------------------------
# Integrations
#
# Maintainers can also review here so an integration PR is never
# blocked waiting on the integrations team alone.
# ---------------------------------------------------------------------
/integrations/ @NVIDIA/flashdreams-integrations @NVIDIA/flashdreams-maintainers

# ---------------------------------------------------------------------
# Licensing and attribution
#
# Apache-2.0 obligations: the project license, per-file SPDX licenses,
# the NOTICE file, and the REUSE manifest are all security-owned so a
# trained reviewer signs off on attribution changes.
# ---------------------------------------------------------------------
/LICENSE @NVIDIA/flashdreams-security
/LICENSES/ @NVIDIA/flashdreams-security
/NOTICE @NVIDIA/flashdreams-security
/reuse.toml @NVIDIA/flashdreams-security

# ---------------------------------------------------------------------
# Dependency manifests and lockfiles
#
# `pyproject.toml` and `uv.lock` change together (any `uv add`,
# `uv lock`, or override edit touches both), so they share a single
# owner. `flashdreams/` and `integrations/*/` are workspace members
# whose dependencies resolve into the root `uv.lock` — they have no
# per-package lockfile.
#
# Per-integration `pyproject.toml` files keep the integrations team as
# an alternate owner so recipe-local dep changes don't always need to
# wait on security.
# ---------------------------------------------------------------------
/pyproject.toml @NVIDIA/flashdreams-security
/uv.lock @NVIDIA/flashdreams-security
/flashdreams/pyproject.toml @NVIDIA/flashdreams-security
/integrations/*/pyproject.toml @NVIDIA/flashdreams-integrations @NVIDIA/flashdreams-security
/integrations/alpadreams/ludus-renderer/pyproject.toml @NVIDIA/flashdreams-integrations @NVIDIA/flashdreams-security
/integrations/alpadreams/ludus-renderer/uv.lock @NVIDIA/flashdreams-security

# ---------------------------------------------------------------------
# Review-policy files themselves
# ---------------------------------------------------------------------
/.github/CODEOWNERS @NVIDIA/flashdreams-maintainers @NVIDIA/flashdreams-security
/docs/code_review.md @NVIDIA/flashdreams-maintainers @NVIDIA/flashdreams-security
292 changes: 292 additions & 0 deletions docs/code_review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,292 @@
<!--
SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
SPDX-License-Identifier: Apache-2.0

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->

# Code review and ownership

Welcome — and thank you for sending a pull request to FlashDreams.
This document explains *who* approves changes in each part of the
repository, *how* code review works in practice, and *what* a
contributor or reviewer can expect day-to-day.

It is a companion to [`CONTRIBUTING.md`](../CONTRIBUTING.md), not a
replacement. If you have not read CONTRIBUTING yet, start there for
the fork-and-PR mechanics, DCO sign-off, testing markers, and the
SPDX header convention. This document picks up where CONTRIBUTING's
[*Code review and merge*](../CONTRIBUTING.md#code-review-and-merge)
section leaves off.

## At a glance

- Every PR to `main` requires approval from a **code owner** for the
touched paths. The code owners for each area of the repository are
listed in [`/.github/CODEOWNERS`](../.github/CODEOWNERS).
- A single approval from a code owner is the bar. GitHub treats the
teams listed on a CODEOWNERS line as alternatives — *any one* of
them can approve.
- Code owners run the [security review checklist](#security-review-checklist)
at the bottom of this document on every review.
- Outside contributors are welcome everywhere. CODEOWNERS controls
who must *approve* a change, not who may *open* one.
- The review requirement cannot be bypassed; any operational
exception requires a documented audit trail.

## How review and merge work on GitHub

There is no special button for code-owner approval — it's the
standard GitHub PR flow:

1. **A contributor opens a pull request.** GitHub reads
[`.github/CODEOWNERS`](../.github/CODEOWNERS), computes which
teams own the paths touched, and adds them to the *Reviewers*
sidebar under "Reviewers requested by Code owners".
2. **A code owner clicks "Review changes → Approve"** in the GitHub
UI (or runs `gh pr review --approve` from the CLI). That single
approval, from a member of a CODEOWNERS team for the touched
paths, satisfies the "Require review from Code Owners" branch
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is fragile and not secure anyway, could we request pgp-signed commits instead?

@jmccaffrey-nv jmccaffrey-nv May 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 .

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 <your@email.com>

[`CONTRIBUTING.md` → DCO](../CONTRIBUTING.md#developer-certificate-of-origin-dco).
4. **A maintainer squash-merges.** The PR title and description
become the squash commit message.

A non-CODEOWNER approval is welcome — review feedback from anyone
helps — but on its own it does not satisfy the protected-branch
check.

Branch protection on `main` is configured so that:

- **Require pull request reviews before merging** — enabled.
- **Require review from Code Owners** — enabled.
- **Dismiss stale approvals when new commits are pushed** — enabled.
- **Restrict who can push to matching branches** — enabled; direct
pushes are not allowed.
- **Do not allow bypassing the above settings** — enabled, including
for administrators.

## How ownership is organised

GitHub honours exactly one CODEOWNERS file per repository, so all of
the rules live in [`/.github/CODEOWNERS`](../.github/CODEOWNERS). The
file is divided into sections by subsystem, with each section
behaving like its own delegated CODEOWNERS for the subtree it
covers. The table below mirrors that structure so you can find the
right reviewer without reading the whole file.

### Subsystems

We keep ownership intentionally lean — three teams cover the whole
repository.

| 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` |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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/ .

| **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
Comment on lines +96 to +105

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.

P1 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!

not explicitly matched. We can split out additional teams later if
any subsystem grows enough to deserve its own ownership boundary,
but the current shape keeps the review surface small and easy to
staff.

**A note on "or" in the table above.** When CODEOWNERS lists
multiple owners on the same line, GitHub treats them as
alternatives — approval from *any one* of the listed teams
satisfies the required-review-from-Code-Owners check. If we ever
need a path to require sign-off from *both* the subsystem team
*and* the security team (rather than either), we will raise the
branch-protection "required approvals" count and split the rule
across two lines.

### Reading the file

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
Comment on lines +121 to +130

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.

P2 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.

git log -1 --format=%H -- <path>
```

When in doubt, open the PR and request a review from
`@NVIDIA/flashdreams-maintainers`; they will route it to the right
person.

## Security-sensitive paths

A small set of paths is owned by `@NVIDIA/flashdreams-security`,
either as sole owner or as an additional approver:

- `LICENSE`, `LICENSES/`, `NOTICE`, `reuse.toml` — Apache-2.0
attribution and third-party license bookkeeping.
- `pyproject.toml`, `flashdreams/pyproject.toml`,
`integrations/*/pyproject.toml`, `uv.lock` — dependency manifests
and lock files.
- `.github/CODEOWNERS` and `docs/code_review.md` — the review
policy itself.

Mistakes in these files are unusually expensive to roll back, so the
security team signs off on them as a matter of routine.

For **vulnerability reports**, do *not* open a public issue or PR.
Follow NVIDIA's coordinated disclosure process at
<https://www.nvidia.com/en-us/security/>, as described in
[`CONTRIBUTING.md` → Filing issues and security reports](../CONTRIBUTING.md#filing-issues-and-security-reports).

## What this looks like from a contributor's perspective

Here's what to expect when you open a pull request — the
mechanics are straightforward, and you'll see each step on the
PR page as it happens:

1. Fork, branch, code, sign off, push, open a PR — same flow as in
[`CONTRIBUTING.md` → Submitting a pull request](../CONTRIBUTING.md#submitting-a-pull-request).
2. GitHub automatically tags the right code owners based on the
paths you touched.
3. A code owner works through the checklist below and approves.
4. The squash-merge button lights up, and a maintainer presses it.

If your PR spans multiple subsystems (say, a core change that also
updates an integration), you will see review requests from each
affected CODEOWNERS team. That is intentional: cross-subsystem
changes benefit from a reviewer in each area.

We aim for first review within two business days; if a PR is
quieter than that, please leave a friendly ping comment.

## Becoming a code owner

CODEOWNERS membership is how the project formalises long-term
ownership of a subsystem, and it's the main mechanism by which
governance opens up over time (see
[`CONTRIBUTING.md` → Project governance](../CONTRIBUTING.md#project-governance)).
Contributors — NVIDIA employee or not — who consistently land
high-quality work in an area, participate in reviews, and engage
with the issue tracker can be invited to become code owners.
CODEOWNERS supports two ways to be listed: team references such as
`@NVIDIA/flashdreams-integrations`, and individual GitHub handles
such as `@username`. The `@NVIDIA/*` teams require NVIDIA
organization membership, so contributors outside NVIDIA are
recognised by adding their individual handle to the relevant
CODEOWNERS rule — they still count as required reviewers for the
paths they own.

There is no fixed time bar; we look for sustained good judgment
about when to ship, when to push back, and when to ask for help. If
you'd like to grow into that role, please say so in a Discussion or
to any maintainer — we'd rather hear it than not.

## No bypass, no waiver

Branch protection on `main` is configured so that:

- Administrators are *not* allowed to bypass review or status checks.
- Direct pushes to `main` are blocked.
- Force-pushes to `main` are blocked.

If an operational situation genuinely requires an exception — for
example, an emergency security patch where the usual reviewer is
unavailable — the exception must be documented as an issue and the
audit trail of the incident retained. We have not needed to use
this path, and we would prefer to keep it that way.

## Security review checklist

Code owners — human or LLM agent — run this checklist on every PR
before approving. It covers the most common ways a change can
introduce a security regression.

**How to use the table.** For each item, set **Status** to one of:

- `Pass` — the change clearly satisfies the item.
- `Fail` — the change clearly violates the item; block until fixed.
- `N/A` — the item does not apply to this PR (e.g. crypto items on
a pure-docs change). Explain why in the *Evidence / finding*
column.
- `Human review` — the item requires judgment beyond what an LLM
agent should make alone; flag it for a human code owner.

**Evidence / finding** should cite the relevant `path/to/file.py:LN`
or quote the specific lines or commit hash that justify the status.
For `Pass` on a non-trivial item, briefly say *why* (one short
sentence) rather than just `Pass`. LLM agents should err toward
`Human review` whenever they are uncertain, and should never mark
an item `Pass` without concrete evidence.

| # | Category | Item | Question | Status | Evidence / finding |
|---|----------|------|----------|--------|--------------------|
| 1 | Process & Static Analysis | SCA Clean | Have all critical and high-severity issues identified by Static Code Analysis (SCA) tools been resolved? | | |
| 2 | Process & Static Analysis | Risk Assessed | Has the code been evaluated against the product's Threat and Vulnerability Analysis (TAVA)? | | |
| 3 | Input Validation | Trust Boundaries | Is all input crossing the trust boundary strictly validated? | | |
| 4 | Input Validation | Length & Range | Are the lengths and ranges of the data explicitly checked? | | |
| 5 | Input Validation | Failures | Does any input validation failure result in an error? | | |
| 6 | Safe Function Usage | C/C++ APIs | Have unsafe functions been replaced with secure alternatives (e.g. `snprintf` vs `sprintf`, `strlcpy` vs `strcpy`, `fgets` vs `gets`)? | | |
| 7 | Safe Function Usage | Python APIs | Are unsafe executions avoided (e.g. `literal_eval` vs `eval`, `shell=False` in `subprocess`)? | | |
| 8 | Variable Management | Initialization | Are all variables initialized before use with a deny-by-default approach? | | |
| 9 | Variable Management | Typing | Are unsigned types used unless negative values are required? | | |
| 10 | Variable Management | Scope | Are variables scoped minimally and not reused improperly? | | |
| 11 | Compilation & Resilience | Compiler Flags | Are strict security compiler flags enabled (e.g. `-Werror`, `-Wall`, `-fstack-protector-strong`)? | | |
| 12 | Compilation & Resilience | Fault Injection (if applicable) | Are mitigations in place for critical low-level code (e.g. redundancy, glitch resistance)? | | |
| 13 | Error Handling | Action Taken | Are errors properly handled and propagated (logging alone is insufficient)? | | |
| 14 | Error Handling | No Leakage | Do error flows prevent exposure of sensitive data (stack traces, memory, secrets)? | | |
| 15 | Error Handling | Resource Cleanup | Are resources properly freed during error handling? | | |
| 16 | Cryptography & Secrets | No Hardcoded Secrets | Is the code free of plaintext secrets (API keys, passwords, tokens, internal IPs)? | | |
| 17 | Cryptography & Secrets | Strong Crypto | Are only modern, vetted cryptographic algorithms used (e.g. CNSA Suite 2.0)? | | |
| 18 | Cryptography & Secrets | Authentication First | Is data authenticated before decryption? | | |
| 19 | Cryptography & Secrets | Safe Randomness | Are cryptographically secure RNGs used (e.g. `/dev/urandom` vs `rand()`)? | | |
| 20 | Access Control & Concurrency | Least Privilege | Are access controls enforced using allowlists vs blocklists? | | |
| 21 | Access Control & Concurrency | Shared Resources | Are shared resources protected against DoS or privilege abuse? | | |
| 22 | Access Control & Concurrency | Race Conditions | Is the code protected against TOCTOU vulnerabilities? | | |
| 23 | Access Control & Concurrency | No Backdoors | Are there no intentional or accidental bypass mechanisms? | | |

## NVIDIA PLC compliance note

FlashDreams is released alongside an NVIDIA Product Lifecycle (PLC)
"+2 Code Review" compliance requirement. The CODEOWNERS-based
review described above — one approval from a qualified code owner,
checklist applied, security team on sensitive paths, no bypass —
is what satisfies that requirement. Code owners are trained as
required by NVIDIA PLC before being added to a CODEOWNERS team, and
the checklist questions above are reused from NVIDIA's *PLC: Code
Review Excellence* training. No separate, parallel process is
needed.

## Changing this document or the CODEOWNERS file

Both this file and `.github/CODEOWNERS` are themselves owned by
`@NVIDIA/flashdreams-maintainers` and `@NVIDIA/flashdreams-security`.
If you want to propose a change — new subsystem owners, a different
ownership boundary, a refinement to how review works — please open
an issue or Discussion first so we can talk through it before the
PR. The goal is for the review policy to be predictable; we change
it deliberately rather than incrementally.

---

Thanks for reading this far. The project is healthier when
reviewers and contributors share a clear picture of how decisions
get made; if anything above is unclear, that's a bug in the
documentation and we'd appreciate the issue.
Loading