mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-18 02:24:27 +02:00
chore: remove CODEOWNERS chassis and the code-owner review gate
Some checks are pending
CI / Classify Changes (push) Waiting to run
CI / Check AGENTS.md Links (push) Waiting to run
CI / Container Entrypoint (push) Waiting to run
CI / Test Workspace (push) Blocked by required conditions
CI / Test omnigraph-server --features aws (push) Blocked by required conditions
CI / RustFS S3 Integration (push) Blocked by required conditions
Release Edge / Prepare edge release (push) Waiting to run
Release Edge / Build edge omnigraph-linux-x86_64 (push) Blocked by required conditions
Release Edge / Build edge omnigraph-macos-arm64 (push) Blocked by required conditions
Release Edge / Build edge omnigraph-windows-x86_64 (push) Blocked by required conditions
Release Edge / Smoke Windows installer (push) Blocked by required conditions
Some checks are pending
CI / Classify Changes (push) Waiting to run
CI / Check AGENTS.md Links (push) Waiting to run
CI / Container Entrypoint (push) Waiting to run
CI / Test Workspace (push) Blocked by required conditions
CI / Test omnigraph-server --features aws (push) Blocked by required conditions
CI / RustFS S3 Integration (push) Blocked by required conditions
Release Edge / Prepare edge release (push) Waiting to run
Release Edge / Build edge omnigraph-linux-x86_64 (push) Blocked by required conditions
Release Edge / Build edge omnigraph-macos-arm64 (push) Blocked by required conditions
Release Edge / Build edge omnigraph-windows-x86_64 (push) Blocked by required conditions
Release Edge / Smoke Windows installer (push) Blocked by required conditions
The repo is a 2-person team where both maintainers own every path, so the CODEOWNERS machinery (generated CODEOWNERS, roles yml, render script, the two drift/hand-edit CI jobs) gated nothing real while adding friction: every PR showed "Review required" and own-PRs merged only via admin/bypass override. Remove the whole chassis and drop the review gate: - delete .github/CODEOWNERS, codeowners-roles.yml, render-codeowners.py, the CODEOWNERS workflow, and docs/dev/codeowners.md - branch-protection.json: drop the two CODEOWNERS required status checks, set require_code_owner_reviews=false and required_approving_review_count=0 (CI checks are the gate; maintainers merge their own PRs once green) - scrub CODEOWNERS references from AGENTS.md, docs indexes, branch-protection and ci docs, GOVERNANCE.md, and CONTRIBUTING.md The policy change is inert until an admin runs scripts/apply-branch-protection.sh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a9a5453520
commit
f2c512ae26
12 changed files with 26 additions and 485 deletions
|
|
@ -8,10 +8,9 @@ This page explains what the policy says and how to change it.
|
|||
|
||||
| Setting | Value | Why |
|
||||
|---|---|---|
|
||||
| **Required status checks (strict)** | `Classify Changes`, `Check AGENTS.md Links`, `Test omnigraph-server --features aws`, `CODEOWNERS matches source`, `CODEOWNERS not hand-edited` | Every PR must pass the AWS-feature build/test, AGENTS.md link integrity, and the CODEOWNERS hygiene checks. **`Test Workspace` is deliberately NOT required** — it runs only on push to `main` (post-merge), tags, and manual `workflow_dispatch`, to keep PR turnaround fast (it was the ~15min+ slow gate). It is therefore *not* listed here: a required check that never reports on PRs (the `test` job is `if: github.event_name != 'pull_request'`) would leave every PR permanently pending — the same job-never-reports trap the CODEOWNERS contexts call out below. The trade-off (a regression lands on `main` and is caught by the post-merge run, so `main` can briefly go red) and its mitigations are documented in [ci.md](ci.md). The two CODEOWNERS contexts must equal the job `name:` values in `.github/workflows/codeowners.yml` **verbatim** — a context naming a job that never reports (the old `CODEOWNERS / drift` used the job *id*, and the job was path-filtered) leaves every PR permanently pending and forces admin overrides. `strict: true` requires the branch to be up-to-date with `main` before merge. |
|
||||
| **Required approving reviews** | `1` | At least one reviewer. With a 2-person team, going higher would block all merges when one person is unavailable. |
|
||||
| **Require code-owner reviews** | `true` | The reviewer must be a code owner per `.github/CODEOWNERS`. This is what makes the codeowners chassis enforced. |
|
||||
| **Dismiss stale reviews on new commits** | `true` | A push after approval invalidates the prior review. Prevents the "approve, then sneak in unreviewed changes" pattern. |
|
||||
| **Required status checks (strict)** | `Classify Changes`, `Check AGENTS.md Links`, `Test omnigraph-server --features aws` | Every PR must pass the AWS-feature build/test and AGENTS.md link integrity. **`Test Workspace` is deliberately NOT required** — it runs only on push to `main` (post-merge), tags, and manual `workflow_dispatch`, to keep PR turnaround fast (it was the ~15min+ slow gate). It is therefore *not* listed here: a required check that never reports on PRs (the `test` job is `if: github.event_name != 'pull_request'`) would leave every PR permanently pending — the job-never-reports trap. The trade-off (a regression lands on `main` and is caught by the post-merge run, so `main` can briefly go red) and its mitigations are documented in [ci.md](ci.md). Each required context must equal a job `name:` that actually reports on PRs **verbatim** — a context naming a job that never reports leaves every PR permanently pending and forces admin overrides. `strict: true` requires the branch to be up-to-date with `main` before merge. |
|
||||
| **Required approving reviews** | `0` | No human-review gate. With a 2-person team where both maintainers own everything, requiring an approval meant every PR needed the *other* person (or an admin/bypass override) — friction with no real review value. CI checks are the gate; maintainers merge their own PRs once checks pass. Raise this to `1` if an outside-contributor flow ever needs a review gate. |
|
||||
| **Require code-owner reviews** | `false` | CODEOWNERS was removed entirely (see the git history of `.github/`); there is no code-owner review requirement. |
|
||||
| **Require linear history** | `true` | No merge commits — squash or rebase only. Matches recent practice. |
|
||||
| **Disallow force pushes** | `true` | No history rewrites on `main`. |
|
||||
| **Disallow branch deletions** | `true` | `main` cannot be deleted. |
|
||||
|
|
@ -57,7 +56,7 @@ Outputs the live policy. Compare against `.github/branch-protection.json` to det
|
|||
|
||||
- **Audit trail**: `git log .github/branch-protection.json` shows every change with a reviewable diff and a merge commit.
|
||||
- **Disaster recovery**: if branch protection is accidentally removed or weakened via the UI, the JSON is the canonical recovery point.
|
||||
- **Consistency**: pairs with `.github/codeowners-roles.yml` (the CODEOWNERS source of truth). Repository policy lives in the repository.
|
||||
- **Consistency**: repository policy lives in the repository, reviewed like code.
|
||||
|
||||
## What this gates
|
||||
|
||||
|
|
@ -65,11 +64,11 @@ After branch protection is applied, every PR targeting `main` must:
|
|||
|
||||
1. Pass all listed status checks.
|
||||
2. Be up-to-date with `main` (rebase or merge-from-main).
|
||||
3. Have at least one approving review from a code owner for the touched paths.
|
||||
4. Have all review conversations resolved.
|
||||
5. Be squash- or rebase-merged (no merge commits).
|
||||
3. Have all review conversations resolved.
|
||||
4. Be squash- or rebase-merged (no merge commits).
|
||||
|
||||
Even repository admins are subject to these rules.
|
||||
No human approval is required (`required_approving_review_count: 0`). Repository
|
||||
admins can override the gates (`enforce_admins: false`).
|
||||
|
||||
## Subsequent hardening (not in this PR)
|
||||
|
||||
|
|
@ -77,7 +76,7 @@ The branch-protection policy is the foundation. Future hardening adds:
|
|||
|
||||
- **Required signed commits** (`required_signatures: true`) — once maintainers enroll GPG/SSH signing.
|
||||
- **Tag protection** for `v*` tags via `repos/.../tags/protection`.
|
||||
- **Required reviewers from specific teams** for high-leverage paths (e.g., `docs/dev/invariants.md`) via CODEOWNERS tier expansion + the N-unique-approvers CI workaround.
|
||||
- **Required reviewers from specific teams** for high-leverage paths (e.g., `docs/dev/invariants.md`) via a GitHub ruleset's path-scoped required-review rule, if a review gate is ever reintroduced.
|
||||
- **More required CI checks**: `cargo deny`, `cargo audit`, `cargo fmt --check`, `cargo clippy -D warnings`, CodeQL, secret scanning, schema-lint (MR-946).
|
||||
|
||||
See the hardening playbook for the full plan.
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@
|
|||
`.github/workflows/`:
|
||||
|
||||
- **ci.yml**: text-only changes skip; otherwise `cargo test --workspace --locked` on ubuntu-latest with protobuf compiler. OpenAPI-drift check that auto-commits the regenerated `openapi.json` for same-repository PRs. Also runs the AGENTS.md cross-link integrity check (`scripts/check-agents-md.sh`).
|
||||
- **`Test Workspace` does not run on pull requests.** The job is gated `if: github.event_name != 'pull_request'`, so the full workspace + failpoints suite runs only on push to `main` (post-merge), on `v*` tags, and on manual `workflow_dispatch`. This was a deliberate PR-latency trade-off — it was the slowest gate (~15min warm, up to the 75min cold ceiling). `RustFS S3 Integration` `needs: test`, so it is push-/dispatch-only for the same reason. The fast PR gates remain: `Classify Changes`, `Check AGENTS.md Links`, `Test omnigraph-server --features aws`, and the two CODEOWNERS checks. `Test Workspace` is correspondingly **not** in the required-check list (`.github/branch-protection.json`); see [branch-protection.md](branch-protection.md).
|
||||
- **`Test Workspace` does not run on pull requests.** The job is gated `if: github.event_name != 'pull_request'`, so the full workspace + failpoints suite runs only on push to `main` (post-merge), on `v*` tags, and on manual `workflow_dispatch`. This was a deliberate PR-latency trade-off — it was the slowest gate (~15min warm, up to the 75min cold ceiling). `RustFS S3 Integration` `needs: test`, so it is push-/dispatch-only for the same reason. The fast PR gates remain: `Classify Changes`, `Check AGENTS.md Links`, and `Test omnigraph-server --features aws`. `Test Workspace` is correspondingly **not** in the required-check list (`.github/branch-protection.json`); see [branch-protection.md](branch-protection.md).
|
||||
- **Consequences to internalize:** (1) a regression that the suite would catch now lands on `main` and turns the post-merge run red, rather than being blocked pre-merge — `main` can briefly break, so run `cargo test --workspace --locked` locally before merging anything non-trivial, or trigger this workflow on your branch via the Actions "Run workflow" button. (2) `openapi.json` is no longer auto-regenerated on PRs (that step is inside the `test` job); for server/API changes, regenerate it locally with `OMNIGRAPH_UPDATE_OPENAPI=1 cargo test -p omnigraph-server --test openapi` and commit it, or the strict drift check fails the post-merge `main` run.
|
||||
- **Applying this policy:** removing `Test Workspace` from the JSON is inert until an admin runs `./scripts/apply-branch-protection.sh`. **Run it immediately after this change merges** — until then GitHub still requires a `Test Workspace` context that no longer reports on PRs, which leaves every open PR permanently pending (the job-never-reports trap).
|
||||
- **AWS feature build job**: `cargo build/test -p omnigraph-server --features aws` on ubuntu-latest.
|
||||
|
|
|
|||
|
|
@ -1,58 +0,0 @@
|
|||
# Code ownership
|
||||
|
||||
`.github/CODEOWNERS` is **generated** — not hand-edited. The source of truth is `.github/codeowners-roles.yml`, expanded by `.github/scripts/render-codeowners.py`. CI rejects drift between the two and rejects direct edits to `CODEOWNERS` that don't accompany a yml change.
|
||||
|
||||
This setup gives every role change a reviewable PR and a permanent in-repository audit trail (`git log .github/codeowners-roles.yml`).
|
||||
|
||||
## Who owns what
|
||||
|
||||
The tables below are **generated** from `.github/codeowners-roles.yml` by `.github/scripts/render-codeowners.py` (the same render that produces `.github/CODEOWNERS`). They are the always-current "who owns what at this commit" view — don't edit them by hand; edit the yml and re-render.
|
||||
|
||||
<!-- BEGIN GENERATED OWNERSHIP — edit codeowners-roles.yml + run render-codeowners.py -->
|
||||
|
||||
**Path → owners** (GitHub applies *last match wins*; the `*` catch-all is listed first and is overridden by the specific patterns below it):
|
||||
|
||||
| Path | Owners | Role(s) |
|
||||
|---|---|---|
|
||||
| `*` | @aaltshuler @ragnorc | engineering |
|
||||
| `crates/**` | @aaltshuler @ragnorc | engineering |
|
||||
| `docs/**` | @aaltshuler @ragnorc | docs |
|
||||
| `README.md` | @aaltshuler @ragnorc | docs |
|
||||
| `AGENTS.md` | @aaltshuler @ragnorc | docs |
|
||||
| `CLAUDE.md` | @aaltshuler @ragnorc | docs |
|
||||
| `SECURITY.md` | @aaltshuler @ragnorc | docs |
|
||||
|
||||
**Roles**:
|
||||
|
||||
| Role | Members | Description |
|
||||
|---|---|---|
|
||||
| `engineering` | @aaltshuler @ragnorc | All production code under crates/**. Engine, CLI, server, compiler. |
|
||||
| `docs` | @aaltshuler @ragnorc | Documentation under docs/**, plus repo-level docs (README.md, AGENTS.md, CLAUDE.md symlink, SECURITY.md). |
|
||||
|
||||
<!-- END GENERATED OWNERSHIP -->
|
||||
|
||||
GitHub treats multiple owners on a CODEOWNERS line as **"any one of them satisfies the review requirement"**. To require N distinct approvers on a specific path, layer a CI check on top (not currently configured).
|
||||
|
||||
## How to change role membership or path mappings
|
||||
|
||||
1. Edit `.github/codeowners-roles.yml`.
|
||||
2. Open a PR. **CI re-renders for you**: the `CODEOWNERS` workflow regenerates `.github/CODEOWNERS` and the ownership tables above and auto-commits them back to your PR branch on same-repository PRs — you don't have to run the script locally (though you can: `python3 .github/scripts/render-codeowners.py`, requires PyYAML).
|
||||
|
||||
On a fork (where CI can't push back), the workflow instead fails with the diff so you can run the script and commit it yourself.
|
||||
|
||||
CI fails the PR if:
|
||||
- a fork PR left a generated artifact out of sync, or
|
||||
- `CODEOWNERS` was edited without a corresponding yml change (the `CODEOWNERS not hand-edited` check).
|
||||
|
||||
## How to add a new role
|
||||
|
||||
1. Add a new entry to `roles:` in the yml with a `description` and `members` list.
|
||||
2. Reference the role from `paths:` (or `default:`).
|
||||
3. Regenerate + commit as above.
|
||||
|
||||
## Why a generator, not direct CODEOWNERS edits?
|
||||
|
||||
- **Audit trail**: `git log .github/codeowners-roles.yml` is the canonical record of every role change. The rendered `CODEOWNERS` is a derived artifact.
|
||||
- **Roles are first-class**: paths reference roles, not raw handles. Renaming a person or rotating a role updates one place, not every path.
|
||||
- **Future extension**: scheduled rotation (weekly on-call, quarterly leads) plugs into the same yml without changing the path mappings. Not enabled today.
|
||||
- **Consistency with the product**: omnigraph itself enforces auditable Cedar policy. The repository's code-owner policy follows the same "policy as reviewed code" pattern.
|
||||
|
|
@ -28,7 +28,6 @@ constraints. User-facing behavior should still be documented through
|
|||
| Three-way merge implementation and conflicts | [merge.md](merge.md) |
|
||||
| Diff/change-feed implementation | [changes.md](../user/branching/changes.md) |
|
||||
| Branch protection policy | [branch-protection.md](branch-protection.md) |
|
||||
| CODEOWNERS source of truth | [codeowners.md](codeowners.md) |
|
||||
|
||||
## Language, Runtime, And Boundaries
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue