From f2c512ae2616e02dddeb9c2ab6a57b842bed2f26 Mon Sep 17 00:00:00 2001 From: aaltshuler Date: Thu, 18 Jun 2026 02:38:02 +0300 Subject: [PATCH] chore: remove CODEOWNERS chassis and the code-owner review gate 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) --- .github/CODEOWNERS | 18 --- .github/branch-protection.json | 20 +-- .github/codeowners-roles.yml | 56 -------- .github/scripts/render-codeowners.py | 205 --------------------------- .github/workflows/codeowners.yml | 110 -------------- AGENTS.md | 1 - CONTRIBUTING.md | 4 +- GOVERNANCE.md | 17 ++- docs/dev/branch-protection.md | 19 ++- docs/dev/ci.md | 2 +- docs/dev/codeowners.md | 58 -------- docs/dev/index.md | 1 - 12 files changed, 26 insertions(+), 485 deletions(-) delete mode 100644 .github/CODEOWNERS delete mode 100644 .github/codeowners-roles.yml delete mode 100755 .github/scripts/render-codeowners.py delete mode 100644 .github/workflows/codeowners.yml delete mode 100644 docs/dev/codeowners.md diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS deleted file mode 100644 index ce8510c..0000000 --- a/.github/CODEOWNERS +++ /dev/null @@ -1,18 +0,0 @@ -# AUTOGENERATED from .github/codeowners-roles.yml. Do not edit by hand. -# -# To change role membership or path assignments: -# 1. Edit .github/codeowners-roles.yml -# 2. Run `python3 .github/scripts/render-codeowners.py` -# 3. Commit both files together -# -# CI fails if this file drifts from its source, and rejects PRs that -# edit this file directly without also editing the yml. - -* @aaltshuler @ragnorc - -crates/** @aaltshuler @ragnorc -docs/** @aaltshuler @ragnorc -README.md @aaltshuler @ragnorc -AGENTS.md @aaltshuler @ragnorc -CLAUDE.md @aaltshuler @ragnorc -SECURITY.md @aaltshuler @ragnorc diff --git a/.github/branch-protection.json b/.github/branch-protection.json index aa1ab19..fa1d57f 100644 --- a/.github/branch-protection.json +++ b/.github/branch-protection.json @@ -1,27 +1,19 @@ { - "_comment": "Branch protection policy for main. Applied via scripts/apply-branch-protection.sh. See docs/branch-protection.md for rationale. NOTE: bypass_pull_request_allowances.users must mirror the engineering owners in .github/codeowners-roles.yml — code owners merge their own PRs without a second review; non-owners still need a code-owner approval. (render-codeowners.py does NOT generate this list; keep it in sync by hand.)", + "_comment": "Branch protection policy for main. Applied via scripts/apply-branch-protection.sh. See docs/dev/branch-protection.md for rationale. CODEOWNERS was removed (2-person team where both maintainers own everything, so code-owner review added friction without value). Review is no longer code-owner-scoped and no approvals are required; the required CI status checks are the gate. Maintainers merge their own PRs once checks pass.", "required_status_checks": { "strict": true, "contexts": [ "Classify Changes", "Check AGENTS.md Links", - "Test omnigraph-server --features aws", - "CODEOWNERS matches source", - "CODEOWNERS not hand-edited" + "Test omnigraph-server --features aws" ] }, "enforce_admins": false, "required_pull_request_reviews": { - "dismissal_restrictions": {}, - "dismiss_stale_reviews": true, - "require_code_owner_reviews": true, - "required_approving_review_count": 1, - "require_last_push_approval": false, - "bypass_pull_request_allowances": { - "users": ["ragnorc", "aaltshuler"], - "teams": [], - "apps": [] - } + "dismiss_stale_reviews": false, + "require_code_owner_reviews": false, + "required_approving_review_count": 0, + "require_last_push_approval": false }, "restrictions": null, "required_linear_history": true, diff --git a/.github/codeowners-roles.yml b/.github/codeowners-roles.yml deleted file mode 100644 index 65f2400..0000000 --- a/.github/codeowners-roles.yml +++ /dev/null @@ -1,56 +0,0 @@ -# Source of truth for .github/CODEOWNERS. -# -# How to change role membership or path assignments: -# 1. Edit this file. -# 2. Run `python3 .github/scripts/render-codeowners.py` to regenerate -# .github/CODEOWNERS. -# 3. Commit both files in the same PR. -# -# CI fails on drift between this source and the generated CODEOWNERS -# (see .github/workflows/codeowners.yml). CI also rejects direct edits -# to .github/CODEOWNERS that don't accompany a change here. -# -# Why a generator instead of editing CODEOWNERS directly? -# The yml is the audit trail: `git log .github/codeowners-roles.yml` -# shows every role change with a reviewable diff and a merge commit. -# The rendered CODEOWNERS is what GitHub reads at PR time. - -roles: - engineering: - description: > - All production code under crates/**. Engine, CLI, server, - compiler. - members: - - aaltshuler - - ragnorc - - docs: - description: > - Documentation under docs/**, plus repo-level docs (README.md, - AGENTS.md, CLAUDE.md symlink, SECURITY.md). - members: - - aaltshuler - - ragnorc - -# Path → role mapping. GitHub CODEOWNERS uses "last match wins" -# semantics — when multiple patterns match a file, only the last -# matching pattern's owners apply. The generator handles this by -# emitting `default` as the first `*` line and the specific patterns -# below afterward, so specific paths override the catch-all. -# -# Within this list, order matters only between overlapping specific -# patterns (the later one wins). Today nothing overlaps; future -# additions should keep more-specific patterns later. -paths: - "crates/**": [engineering] - "docs/**": [docs] - "README.md": [docs] - "AGENTS.md": [docs] - "CLAUDE.md": [docs] - "SECURITY.md": [docs] - -# Catch-all for paths not explicitly mapped (.github/, scripts/, -# Cargo.toml, Cargo.lock, openapi.json, LICENSE, etc.). Defaults to -# engineering — every change to repo infrastructure needs the -# engineering owner's review. -default: [engineering] diff --git a/.github/scripts/render-codeowners.py b/.github/scripts/render-codeowners.py deleted file mode 100755 index 5e96545..0000000 --- a/.github/scripts/render-codeowners.py +++ /dev/null @@ -1,205 +0,0 @@ -#!/usr/bin/env python3 -"""Render .github/CODEOWNERS and the ownership tables in -docs/dev/codeowners.md from .github/codeowners-roles.yml. - -The yml is the source of truth. This script expands the role-based yml -into (1) the flat path→owners format GitHub expects in -`.github/CODEOWNERS`, and (2) the "who owns what" markdown tables spliced -between the generated-region markers in `docs/dev/codeowners.md`. Both are -derived artifacts; CI re-renders them on every PR (see -.github/workflows/codeowners.yml) and auto-commits the result on same-repo -PRs, so the source of truth and the human-readable view never drift. - -Usage: - python3 .github/scripts/render-codeowners.py - -Exits non-zero on: - - Missing PyYAML. - - Unknown role referenced in `paths` or `default`. - - Role with no members (a role must always resolve to at least - one owner; otherwise CODEOWNERS would assign nobody and GitHub - would silently fall back to "no required reviewer", which - defeats the purpose). - - Missing generated-region markers in docs/dev/codeowners.md. -""" - -from __future__ import annotations - -import sys -from pathlib import Path - -try: - import yaml -except ImportError: - sys.exit( - "error: PyYAML is required. Install with `pip install pyyaml` " - "or `python3 -m pip install pyyaml`." - ) - -REPO_ROOT = Path(__file__).resolve().parents[2] -SOURCE = REPO_ROOT / ".github" / "codeowners-roles.yml" -OUTPUT = REPO_ROOT / ".github" / "CODEOWNERS" -DOCS = REPO_ROOT / "docs" / "dev" / "codeowners.md" - -# The "who owns what" tables in docs/dev/codeowners.md are spliced between -# these markers so the human-readable view never drifts from the source of -# truth. Edit codeowners-roles.yml and re-render — never the table by hand. -DOCS_BEGIN = "" -DOCS_END = "" - -BANNER = """\ -# AUTOGENERATED from .github/codeowners-roles.yml. Do not edit by hand. -# -# To change role membership or path assignments: -# 1. Edit .github/codeowners-roles.yml -# 2. Run `python3 .github/scripts/render-codeowners.py` -# 3. Commit both files together -# -# CI fails if this file drifts from its source, and rejects PRs that -# edit this file directly without also editing the yml. -""" - - -def resolve(role_name: str, roles: dict) -> list[str]: - role = roles.get(role_name) - if role is None: - sys.exit( - f"error: unknown role '{role_name}'. " - f"Known roles: {sorted(roles.keys())}" - ) - members = role.get("members") or [] - if not members: - sys.exit( - f"error: role '{role_name}' has no members. " - f"A role must resolve to at least one owner." - ) - return members - - -def owners_for(role_names: list[str], roles: dict) -> list[str]: - """Return @-prefixed GitHub handles, deduped, preserving order.""" - seen: list[str] = [] - for role_name in role_names: - for member in resolve(role_name, roles): - handle = f"@{member}" - if handle not in seen: - seen.append(handle) - return seen - - -def _oneline(text: str) -> str: - """Collapse a folded/multi-line YAML description into one cell of text.""" - return " ".join((text or "").split()) - - -def ownership_tables(spec: dict, roles: dict) -> str: - """Render the human-readable "who owns what" markdown — a path→owners - table (the operative view at PR time, in last-match-wins order with the - catch-all first) plus a role→members table. Spliced into the docs between - the markers so it is always current with the source of truth.""" - out: list[str] = [] - - out.append("**Path → owners** (GitHub applies *last match wins*; the `*` " - "catch-all is listed first and is overridden by the specific " - "patterns below it):") - out.append("") - out.append("| Path | Owners | Role(s) |") - out.append("|---|---|---|") - if "default" in spec: - owners = " ".join(owners_for(spec["default"], roles)) - out.append(f"| `*` | {owners} | {', '.join(spec['default'])} |") - for pattern, role_names in (spec.get("paths") or {}).items(): - owners = " ".join(owners_for(role_names, roles)) - out.append(f"| `{pattern}` | {owners} | {', '.join(role_names)} |") - out.append("") - - out.append("**Roles**:") - out.append("") - out.append("| Role | Members | Description |") - out.append("|---|---|---|") - for name, role in roles.items(): - members = " ".join(f"@{m}" for m in (role.get("members") or [])) - out.append(f"| `{name}` | {members} | {_oneline(role.get('description', ''))} |") - out.append("") - - return "\n".join(out) - - -def splice_docs(table_md: str) -> None: - """Replace the region between DOCS_BEGIN/DOCS_END in the docs file with the - freshly generated tables, leaving surrounding prose untouched.""" - if not DOCS.exists(): - sys.exit(f"error: docs file not found: {DOCS}") - text = DOCS.read_text() - if DOCS_BEGIN not in text or DOCS_END not in text: - sys.exit( - f"error: ownership markers not found in {DOCS.relative_to(REPO_ROOT)}. " - f"Add the lines:\n {DOCS_BEGIN}\n {DOCS_END}\n" - f"around the generated table region." - ) - head, rest = text.split(DOCS_BEGIN, 1) - _, tail = rest.split(DOCS_END, 1) - new = f"{head}{DOCS_BEGIN}\n\n{table_md}\n{DOCS_END}{tail}" - DOCS.write_text(new) - - -def main() -> int: - if not SOURCE.exists(): - sys.exit(f"error: source file not found: {SOURCE}") - spec = yaml.safe_load(SOURCE.read_text()) - - roles = spec.get("roles") or {} - if not roles: - sys.exit("error: codeowners-roles.yml declares no roles") - - paths = spec.get("paths") or {} - if not paths: - sys.exit("error: codeowners-roles.yml declares no paths") - - lines: list[str] = [BANNER] - - # Pad the path column for alignment. Width is the longest pattern - # plus a small margin. - width = max(len(p) for p in paths) + 2 - - # GitHub CODEOWNERS uses "last match wins" semantics. Emit the - # default catch-all `*` FIRST so specific patterns below override - # it for the paths they cover. If we emitted `*` last, every file - # would resolve to the default owners regardless of more-specific - # rules — which would silently nullify any role distinction. - if "default" in spec: - default_owners = owners_for(spec["default"], roles) - lines.append(f"{'*':<{width}} {' '.join(default_owners)}") - lines.append("") - - for pattern, role_names in paths.items(): - owners = owners_for(role_names, roles) - lines.append(f"{pattern:<{width}} {' '.join(owners)}") - - lines.append("") # trailing newline so the file ends cleanly - - rendered = "\n".join(lines) - - # Regression check: the catch-all `*` line (if any) must precede - # every specific-path line. Failure here means the generator is - # silently nullifying specific rules. - if "default" in spec: - non_comment = [ln for ln in rendered.splitlines() if ln and not ln.startswith("#")] - first_pattern = non_comment[0].split()[0] if non_comment else None - if first_pattern != "*": - sys.exit( - f"error: generator invariant violated — first emitted pattern is " - f"{first_pattern!r}, expected '*'. CODEOWNERS uses last-match-wins; " - f"the catch-all must come first." - ) - - OUTPUT.write_text(rendered) - print(f"wrote {OUTPUT.relative_to(REPO_ROOT)}") - - splice_docs(ownership_tables(spec, roles)) - print(f"updated {DOCS.relative_to(REPO_ROOT)}") - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml deleted file mode 100644 index 75b3515..0000000 --- a/.github/workflows/codeowners.yml +++ /dev/null @@ -1,110 +0,0 @@ -name: CODEOWNERS - -# Runs on EVERY pull request (no paths filter). The two jobs below are -# required status checks on `main`; a path-filtered required check never -# reports for PRs outside the filter and leaves them permanently "pending" -# (the trap that forced admin-override merges). Always-run + cheap -# short-circuit is what keeps them honest. -on: - pull_request: - workflow_dispatch: - -# `drift` auto-commits the regenerated artifacts back to same-repo PR -# branches, so it needs write access. -permissions: - contents: write - -jobs: - # NOTE: the job `name:` values below ("CODEOWNERS matches source" / - # "CODEOWNERS not hand-edited") ARE the status-check contexts that - # .github/branch-protection.json must list verbatim. Renaming a job here - # is a branch-protection change — update the JSON and re-apply. - drift: - name: CODEOWNERS matches source - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v5.0.1 - - - name: Set up Python - uses: actions/setup-python@v5.4.0 - with: - python-version: '3.13' - - - name: Install PyYAML - run: pip install pyyaml - - - name: Re-render CODEOWNERS + ownership docs - run: python3 .github/scripts/render-codeowners.py - - # Same-repo PR: push the regenerated artifacts back so contributors - # never have to run the script locally. Mirrors the openapi.json - # auto-commit in ci.yml (separate shallow clone of the head branch so - # the pushed commit carries only the regenerated files). - - name: Commit regenerated artifacts to PR branch - if: | - github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - if git diff --quiet -- .github/CODEOWNERS docs/dev/codeowners.md; then - echo "CODEOWNERS and ownership docs already in sync." - exit 0 - fi - tmp=$(mktemp -d) - git clone --depth 1 --branch "${{ github.head_ref }}" \ - "https://x-access-token:${GITHUB_TOKEN}@github.com/${{ github.repository }}.git" \ - "$tmp" - cp .github/CODEOWNERS "$tmp/.github/CODEOWNERS" - cp docs/dev/codeowners.md "$tmp/docs/dev/codeowners.md" - cd "$tmp" - if git diff --quiet -- .github/CODEOWNERS docs/dev/codeowners.md; then - echo "Head branch already matches; nothing to push." - exit 0 - fi - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - git add .github/CODEOWNERS docs/dev/codeowners.md - git commit -m "chore: regenerate CODEOWNERS + ownership docs" - git push - - # Fork PR / workflow_dispatch: cannot push back, so enforce drift - # strictly. The contributor runs the script and commits the result. - - name: Verify in sync (forks / manual runs) - if: | - !(github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository) - run: | - if ! git diff --quiet -- .github/CODEOWNERS docs/dev/codeowners.md; then - echo "::error::Generated CODEOWNERS / ownership docs are out of sync with .github/codeowners-roles.yml." - echo "::error::Run \`python3 .github/scripts/render-codeowners.py\` and commit the result." - echo "--- diff ---" - git --no-pager diff -- .github/CODEOWNERS docs/dev/codeowners.md - exit 1 - fi - echo "Generated artifacts are in sync with their source." - - noedit: - name: CODEOWNERS not hand-edited - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v5.0.1 - with: - # Need history so we can diff against the PR base. - fetch-depth: 0 - - - name: Reject hand-edits to generated file - # Only meaningful for PRs (needs a base to diff against). - if: github.event_name == 'pull_request' - run: | - base="origin/${{ github.base_ref }}" - git fetch origin "${{ github.base_ref }}" --quiet - changed=$(git diff --name-only "$base" HEAD) - edited_generated=$(echo "$changed" | grep -E '^\.github/CODEOWNERS$' || true) - edited_source=$(echo "$changed" | grep -E '^\.github/codeowners-roles\.yml$' || true) - if [ -n "$edited_generated" ] && [ -z "$edited_source" ]; then - echo "::error::This PR edits .github/CODEOWNERS but not its source .github/codeowners-roles.yml." - echo "::error::Edit the yml and regenerate via \`python3 .github/scripts/render-codeowners.py\`." - exit 1 - fi - echo "CODEOWNERS edits accompany source edits (or no CODEOWNERS edits in this PR)." diff --git a/AGENTS.md b/AGENTS.md index e8cd035..1772f77 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,7 +102,6 @@ Full diagram and concurrency model: [docs/dev/architecture.md](docs/dev/architec | Install (binary / Homebrew / source / channels) | [docs/user/install.md](docs/user/install.md) | | Deployment (binary / container / S3-local testing / auth / build variants) | [docs/user/deployment.md](docs/user/deployment.md) | | CI / release workflows | [docs/dev/ci.md](docs/dev/ci.md) | -| Code ownership (CODEOWNERS source of truth, roles, regeneration) | [docs/dev/codeowners.md](docs/dev/codeowners.md) | | Branch protection policy (declarative, applied via `scripts/apply-branch-protection.sh`) | [docs/dev/branch-protection.md](docs/dev/branch-protection.md) | | Constants & tunables cheat sheet | [docs/user/reference/constants.md](docs/user/reference/constants.md) | | Per-version release notes | [docs/releases/](docs/releases/) | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2d77ef0..1029b2f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,8 +22,8 @@ that turns out to be non-trivial will be redirected — that's about process, no the merit of the change. > **Maintainers (ModernRelay team)** follow a separate internal process and are -> not bound by the intake rules above. Everyone is bound by review, CODEOWNERS, -> branch protection, and CI. +> not bound by the intake rules above. Everyone is bound by review, branch +> protection, and CI. ## Development diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 5878f1f..2768e5b 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -9,19 +9,18 @@ to happen before code lands?* > Discussions, RFCs, and pull requests from people outside the ModernRelay > team. **Maintainers operate under a separate internal process** and are not > bound by the intake gates below. Everyone, maintainer or not, is still bound -> by the universal gates: branch protection on `main` and CODEOWNERS review -> (see [docs/dev/branch-protection.md](docs/dev/branch-protection.md) and -> [docs/dev/codeowners.md](docs/dev/codeowners.md)). +> by the universal gates: branch protection on `main` and CI +> (see [docs/dev/branch-protection.md](docs/dev/branch-protection.md)). ## Roles | Role | Who | Authority | |---|---|---| -| **Maintainer** | The code owners in [`.github/CODEOWNERS`](.github/CODEOWNERS) (generated from [`.github/codeowners-roles.yml`](.github/codeowners-roles.yml)) | Validate issues, accept/reject RFCs, review and merge PRs, set direction. Final decision authority. | +| **Maintainer** | The ModernRelay team (repository admins) | Validate issues, accept/reject RFCs, review and merge PRs, set direction. Final decision authority. | | **Contributor** | Anyone else | Report problems (Issues), propose ideas (Discussions), author RFCs, and open pull requests. | -Decision authority rests with the maintainers. CODEOWNERS is the single source -of truth for who that is; this document does not duplicate the list. +Decision authority rests with the maintainers (the ModernRelay team holding +repository-admin access). ## The three channels @@ -51,7 +50,7 @@ contribution. Pull request ◀──────────┴──────────│── merged == accepted │ (links the issue or the accepted RFC) ◀───────┘ (implementation PRs reference it) │ │ - review + CODEOWNERS + branch protection + review + branch protection + CI ▼ merged ``` @@ -91,8 +90,8 @@ where it's reviewable. ## What maintainers do *not* gate Maintainers' own changes do not pass through the intake gates above — the team -runs a separate internal process. The universal gates (review, CODEOWNERS, -branch protection, CI) apply to everyone. Enforcement of the intake rules is, to +runs a separate internal process. The universal gates (review, branch +protection, CI) apply to everyone. Enforcement of the intake rules is, to start, **by convention and review** (PR template + labels); an automated check keyed to author association may be added later if volume warrants. diff --git a/docs/dev/branch-protection.md b/docs/dev/branch-protection.md index 1d1c094..d3a9f6b 100644 --- a/docs/dev/branch-protection.md +++ b/docs/dev/branch-protection.md @@ -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. diff --git a/docs/dev/ci.md b/docs/dev/ci.md index 2e80f40..6cc4e1f 100644 --- a/docs/dev/ci.md +++ b/docs/dev/ci.md @@ -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. diff --git a/docs/dev/codeowners.md b/docs/dev/codeowners.md deleted file mode 100644 index 707f4f4..0000000 --- a/docs/dev/codeowners.md +++ /dev/null @@ -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. - - - -**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). | - - - -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. diff --git a/docs/dev/index.md b/docs/dev/index.md index a0a6afb..1fc0b77 100644 --- a/docs/dev/index.md +++ b/docs/dev/index.md @@ -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