From 32fc7fdf37e94e43a86c58f03f5984cd36f4c796 Mon Sep 17 00:00:00 2001 From: Jason Date: Thu, 22 Oct 2026 07:30:09 +1100 Subject: [PATCH] chore: enforce concise PR descriptions Signed-off-by: Jason --- .github/pull_request_template.md | 113 +++++++-------------- .github/workflows/pr-description-tests.yml | 22 ++++ .github/workflows/pr-description.yml | 47 +++++++++ AGENTS.md | 25 +++++ CONTRIBUTING.md | 87 +++++++++++++++- docs/development/change-governance.md | 17 ++++ docs/development/code_contract.md | 14 +++ docs/development/pr_review_checklist.md | 28 +++++ scripts/ci/pr_description.py | 90 ++++++++++++++++ scripts/ci/pr_description_test.py | 108 ++++++++++++++++++++ 10 files changed, 473 insertions(+), 78 deletions(-) create mode 100644 .github/workflows/pr-description-tests.yml create mode 100644 .github/workflows/pr-description.yml create mode 100644 scripts/ci/pr_description.py create mode 100644 scripts/ci/pr_description_test.py diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 9a7313049..39b74b650 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,91 +1,50 @@ -## Related Issue +## TLDR - -Use one of: -- Closes #issue_number -- Fixes #issue_number -- Relates to #issue_number +## Problem -Small exceptions are allowed for typo-only docs edits, CI/dependency chores, -release chores, and maintainer-approved emergency fixes. If this PR has no -issue, explain the exception here. ---> + -Closes | Fixes | Relates to # - -## Summary - -- - -## Scope - -- [ ] This PR changes one logical boundary only. -- [ ] No unrelated refactor, benchmark, metadata model, Holt layout, - object-store, agent interface, or docs change is mixed in. -- [ ] The linked issue describes the user-visible problem, design decision, or - maintenance task this PR resolves. -- [ ] Any breaking change is intentional and documented. -- [ ] No compatibility shim, deprecated alias, or forwarding wrapper was added without a removal condition. - -## Change Size And Review - -- [ ] I checked GitHub's additions plus deletions for this pull request. -- [ ] If the total is more than 5,000 lines, one core maintainer other than the - current-head pusher approved the exact current head commit. The author, - bots, non-core reviewers, duplicate reviewers, dismissed reviews, and - stale approvals do not count. -- [ ] This change is still small enough for a focused review; approval count is - not being used to justify mixed package or lifecycle boundaries. - -## Code Contract (Code Changes Only) - -- [ ] Not applicable; this is a docs/config-only change. -- [ ] Package boundaries follow `docs/development/code_contract.md`. -- [ ] Shared helpers reuse the standard library or existing repository helpers. - New generic helper modules are domain-neutral and tested. -- [ ] File names and file placement follow the code contract. -- [ ] New types, interfaces, structs, fields, and functions use domain-specific names. -- [ ] New errors are in the owning package's `errors.rs` and carry stable error kinds when crossing package boundaries. -- [ ] New metrics/stats are owned by the package that reports or serves them. -- [ ] Metadata changes document durability, atomicity, revision-reference - lifetime, snapshot/commit/event retention, GC epochs, and fail-closed - recovery boundaries. -- [ ] Placement or routing changes preserve persisted `RootId -> LogicalShardId` - affinity, one active writer per shard, owner-epoch fencing, and explicit - shard-local atomicity; no split root or cross-shard transaction is implied. -- [ ] Agent-interface changes keep the transport-free facade and schemas in - `nokv-agent`, routing and workflows in `nokv-client`, and the concrete - backend plus native full CLI wiring in `nokv`. -- [ ] User-facing integration guidance orders the native full CLI first, the - direct Python SDK second, and the Rust SDK third, and does not present the - deprecated `nokv mcp` sidecar as a supported NoKV surface. - -## Claims and Evidence - -- [ ] User-facing documentation distinguishes current, experimental, and - planned capabilities. -- [ ] Security claims do not treat a Workbench root or Agent scope as tenant - authentication or authorization. -- [ ] Performance claims state the topology, comparison boundary, cache state, - run count, and raw-evidence location. -- [ ] Benchmark-only behavior does not alter product semantics. +## Consequences -## Validation + + +## Example + + - +## Solution -- Command and result: -- Not run (with reason): + -## Contributor Sign-off +## Gain + + + +## Tradeoffs + + + +## Validation -- [ ] Every commit in this PR includes a DCO `Signed-off-by` trailer. + diff --git a/.github/workflows/pr-description-tests.yml b/.github/workflows/pr-description-tests.yml new file mode 100644 index 000000000..7384bbf32 --- /dev/null +++ b/.github/workflows/pr-description-tests.yml @@ -0,0 +1,22 @@ +# Copyright 2024-2026 The NoKV Authors. +# SPDX-License-Identifier: Apache-2.0 + +name: PR Description Tests + +on: + pull_request: + branches: [main] + push: + branches: [main] + +permissions: + contents: read + +jobs: + pr-description-tests: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + - name: Test PR description policy + run: python3 scripts/ci/pr_description_test.py diff --git a/.github/workflows/pr-description.yml b/.github/workflows/pr-description.yml new file mode 100644 index 000000000..cb7c0ade1 --- /dev/null +++ b/.github/workflows/pr-description.yml @@ -0,0 +1,47 @@ +# Copyright 2024-2026 The NoKV Authors. +# SPDX-License-Identifier: Apache-2.0 + +name: PR Description + +on: + pull_request_target: + branches: [main] + types: [opened, reopened, synchronize, edited, ready_for_review] + workflow_dispatch: + inputs: + pull_request: + description: PR number to recheck against the current protected policy + required: true + type: string + +permissions: + contents: read + pull-requests: read + +concurrency: + group: pr-description-${{ github.event_name }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + pr-description: + runs-on: ubuntu-latest + timeout-minutes: 5 + env: + GH_TOKEN: ${{ github.token }} + POLICY_SHA: ${{ github.event.pull_request.base.sha || github.sha }} + PR_NUMBER: ${{ github.event.pull_request.number || inputs.pull_request }} + steps: + # Never check out PR code or interpolate PR titles/bodies into shell code. + - name: Validate live PR metadata with the protected base policy + shell: bash + run: | + set -euo pipefail + [[ "$PR_NUMBER" =~ ^[1-9][0-9]*$ ]] + policy="$RUNNER_TEMP/pr_description.py" + metadata="$RUNNER_TEMP/pr-description.json" + gh api \ + "repos/$GITHUB_REPOSITORY/contents/scripts/ci/pr_description.py?ref=$POLICY_SHA" \ + --jq .content | base64 --decode > "$policy" + gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER" \ + --jq '{title, body}' > "$metadata" + python3 "$policy" --pull-request-json "$metadata" diff --git a/AGENTS.md b/AGENTS.md index d008a5b1c..95766d5e5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,6 +9,31 @@ This repository uses `docs/development/code_contract.md` as the source of truth for Rust package boundaries, naming, errors, metrics, tests, DCO, and storage safety review. +## PR Writing + +Follow the authoritative +[PR description format](CONTRIBUTING.md#pr-description-format) and +[pull request template](.github/pull_request_template.md) when creating or +updating a PR. This is the repository's required format, including for agents. + +- Write a concise English title as `: `. +- Use these body headings in order: `TLDR`, `Problem`, `Consequences`, + `Example`, `Solution`, `Gain`, `Tradeoffs`, then `Validation`. +- Use short, plain English paragraphs. Explain the concrete problem, user + impact, and before/after behavior with minimal code details. Avoid file-by-file + summaries, pasted diffs, repeated explanations, and conversational history. +- Link the issue under `Problem`; explain any permitted exception there. +- Report exact validation commands and results, relevant checks not run, and + evidence limits. Do not claim unmeasured gains or unverified recovery. +- Keep every heading; briefly explain when a section does not apply. Refresh + the title and body when the final scope changes. +- Keep DCO sign-offs and all existing engineering and merge requirements. +- Before opening or editing a PR, run `scripts/ci/pr_description.py` against a + JSON object containing its final `title` and `body`, using + `--pull-request-json `. The same policy is the required remote + `pr-description` check; its regression tests are in + `scripts/ci/pr_description_test.py`. + ## Local Collaboration Direction LingTai is no longer an active partner or a default integration target. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 97429dcf1..a0f6b4df5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -111,7 +111,8 @@ and run `git diff --check`. ## Pull Request Rules - Rebase on latest `upstream/main` before opening or updating a PR. -- PR description must include: what changed, why it changed, and how you validated it (commands + key results). +- Follow the [PR description format](#pr-description-format) and the + [pull request template](.github/pull_request_template.md). - Link related issue(s). - Include docs updates when behavior/config/CLI changes. - Keep PRs small enough for focused review. @@ -131,6 +132,90 @@ and run `git diff --check`. - Every non-merge commit must include a `Signed-off-by` trailer matching the Developer Certificate of Origin in [`DCO`](./DCO). - If you use Codex or another agent to review a PR, point it at [`docs/development/code_contract.md`](./docs/development/code_contract.md) and [`docs/development/pr_review_checklist.md`](./docs/development/pr_review_checklist.md). +### PR Description Format + +Use a concise English title in the form `: `. Describe the final +change for a reviewer who has not read the discussion. Write short, plain +English paragraphs focused on the problem and observable behavior. Mention +code, symbols, and file paths only when needed to explain a decision or risk; +do not turn the description into a file-by-file change log or paste the diff. + +Every PR description uses these headings in this order: + +| Heading | What to write | +| --- | --- | +| `TLDR` | One or two sentences stating the problem and resulting behavior. | +| `Problem` | The concrete trigger and current behavior. Link the related issue here. | +| `Consequences` | Why the problem matters to users or maintainers. | +| `Example` | A short, concrete before/after scenario. Use a code snippet only if it makes the behavior clearer. | +| `Solution` | How the change solves the problem, in plain English. | +| `Gain` | The resulting benefit. Quantified claims require linked evidence. | +| `Tradeoffs` | Relevant costs, limitations, breaking changes, or remaining risks. | +| `Validation` | Exact commands and key results; explain relevant checks not run and link raw evidence for storage or performance claims. | + +Keep all headings. Usually one or two sentences per narrative section are +enough; validation may use a short list. Do not repeat the same explanation +across sections or invent benefits, risks, or examples to fill space. If a +section does not apply, say so briefly with a reason. Update the title and +description when the final scope changes, and omit abandoned approaches unless +they explain a remaining tradeoff. + +Every non-trivial PR needs an existing issue linked with `Closes`, `Fixes`, or +`Relates to`. Typo-only docs edits, CI/dependency chores, release chores, and +maintainer-approved emergency fixes may omit an issue; explain the exception +under `Problem`. + +For example, a documentation-only PR could read: + +```markdown +## TLDR +Make every PR explain its user impact with the same short structure. + +## Problem +The template and contributor guide ask for different descriptions. Relates to #123. + +## Consequences +Reviewers have to ask follow-up questions to understand the change. + +## Example +Before: a list of edited files. After: a concrete problem and before/after behavior. + +## Solution +Use one description format across the template, contributor guide, and agent instructions. + +## Gain +Authors and reviewers have one clear writing standard. + +## Tradeoffs +Small PRs still keep each heading, with brief answers where appropriate. + +## Validation + +- git diff --check: passed. +- Local Markdown links: checked. +- Rust checks: not applicable; documentation only. +``` + +This format does not replace DCO, scope, review, or evidence requirements. +Use the [PR Review Checklist](docs/development/pr_review_checklist.md) for the +engineering review; keep the description focused on this PR's behavior and +validation. + +The required `pr-description` check enforces the title structure, exact section +order, and filled-in content. It ignores HTML comments and does not treat +headings inside fenced code examples as sections. Bare placeholders such as +`TODO` or `N/A` fail; explain why a section does not apply. The check reruns on +PR creation, reopening, pushes, title/body edits, and ready-for-review events. +Plain English, brevity, factual claims, and relevance still require review. + +Run the same policy locally before opening or editing a PR: + +```bash +gh pr view PR_NUMBER --json title,body > /tmp/nokv-pr-description.json +python3 scripts/ci/pr_description.py --pull-request-json /tmp/nokv-pr-description.json +python3 scripts/ci/pr_description_test.py +``` + ## Code Guidelines - Use `rustfmt` formatting and pass `clippy` with warnings denied. diff --git a/docs/development/change-governance.md b/docs/development/change-governance.md index 372751365..65345a76e 100644 --- a/docs/development/change-governance.md +++ b/docs/development/change-governance.md @@ -21,6 +21,9 @@ protection: - the current-head pusher cannot supply that approval; - unresolved review conversations block merge; - administrators are subject to the same restrictions; +- PR titles and descriptions follow the + [required format](../../CONTRIBUTING.md#pr-description-format), enforced by + the required `pr-description` check; - the branch must be current with `main` and every required status must pass. GitHub-reported additions plus deletions are the review-size measure. The @@ -49,6 +52,15 @@ the workflow does not publish a second custom commit status. This distinction is required: a normal `pull_request` workflow is part of the proposed diff and can otherwise weaken the check that evaluates itself. +[`pr-description.yml`](../../.github/workflows/pr-description.yml) applies the +same protected-base boundary to PR metadata. It reads the live title and body +through `gh`, treats them only as data, and fetches the validator from the exact +base SHA. It reruns on `edited` as well as creation, reopening, pushes, and +ready-for-review events. A manual dispatch can recheck a PR with the current +protected policy. Candidate policy regression tests run separately in +[`pr-description-tests.yml`](../../.github/workflows/pr-description-tests.yml), +so a skipped candidate job cannot satisfy the metadata check. + The required status set is: - `nokv-workspace`; @@ -57,6 +69,11 @@ The required status set is: - `signoff`; - `large-change-review`. +The PR writing policy additionally requires `pr-description` and +`pr-description-tests`. Enable the new required checks after the introducing +PR has merged and the protected workflow is available; then verify the live +branch-protection settings without removing existing checks. + Every validation job that runs for each pull-request head is required. The Docker `image` check is the only CI exception: it continues in the background and reports failures, but its runtime does not delay a merge. `sync-project` is diff --git a/docs/development/code_contract.md b/docs/development/code_contract.md index 6030bc4eb..c63c0aabf 100644 --- a/docs/development/code_contract.md +++ b/docs/development/code_contract.md @@ -151,6 +151,20 @@ that owns them. Do not move domain-specific or single-use logic into a generic S3-compatible boundary. - Prefer explicit invariants and local reasoning over manager-style wrappers. +## PR Communication And DCO + +The [contribution guide](../../CONTRIBUTING.md#pr-description-format) owns the +PR writing format. Use `TLDR`, `Problem`, `Consequences`, `Example`, `Solution`, +`Gain`, `Tradeoffs`, and `Validation`, in that order, following the +[template](../../.github/pull_request_template.md). Keep descriptions concise +and in plain English, with minimal code details and a concrete before/after +example. Include exact validation results and evidence for storage or +performance claims; state relevant limits and checks not run. + +Every non-merge commit must carry a DCO `Signed-off-by` trailer as required by +the [contribution guide](../../CONTRIBUTING.md#branch-and-commit-conventions) +and [DCO](../../DCO). + ## Validation Before pushing substantial code changes: diff --git a/docs/development/pr_review_checklist.md b/docs/development/pr_review_checklist.md index 01bc9d356..1f67d36a8 100644 --- a/docs/development/pr_review_checklist.md +++ b/docs/development/pr_review_checklist.md @@ -9,6 +9,25 @@ Findings come first, ordered by severity. Passing tests does not excuse weaker metadata atomicity, object publication, retention, ownership fencing, package boundaries, or Workbench behavior. +## PR Description + +- Does the title use a concise English `: `? +- Does the body follow the + [required format](../../CONTRIBUTING.md#pr-description-format): `TLDR`, + `Problem`, `Consequences`, `Example`, `Solution`, `Gain`, `Tradeoffs`, then + `Validation`? +- Does it explain the concrete problem, user impact, and before/after behavior + in short, plain English paragraphs with minimal code details? +- Are repeated explanations, file-by-file summaries, pasted diffs, and + conversational history omitted? +- Is the related issue linked under `Problem`, or is a permitted exception + explained there? +- Do the title and body describe the final scope, including relevant breaking + changes, limitations, and risks, with evidence for any quantified gains? +- Are exact validation commands and key results included, with reasons for + relevant checks not run? +- Do the required `pr-description` and `pr-description-tests` checks pass? + ## Merge Governance - If additions plus deletions exceed 5,000 lines, does one core maintainer @@ -165,6 +184,15 @@ boundaries, or Workbench behavior. - Are `inode`, `source_root`, `destination_root`, and `checkpoints.jsonl` absent from Workbench responses and contract state? +## Claims And Evidence + +- Do public docs distinguish current, experimental, and planned capabilities? +- Do security claims avoid treating a Workbench root or Agent scope as tenant + authentication or authorization? +- Do performance claims state the topology, comparison boundary, cache state, + run count, and raw-evidence location? +- Does benchmark-only behavior leave product semantics unchanged? + ## Tests And Evidence - Is there a package test for each local invariant? diff --git a/scripts/ci/pr_description.py b/scripts/ci/pr_description.py new file mode 100644 index 000000000..7f622f43e --- /dev/null +++ b/scripts/ci/pr_description.py @@ -0,0 +1,90 @@ +#!/usr/bin/env python3 +# Copyright 2024-2026 The NoKV Authors. +# SPDX-License-Identifier: Apache-2.0 +"""Validate the repository's PR title and description structure.""" + +from __future__ import annotations + +import argparse +import json +import re +import sys +from pathlib import Path + + +SECTIONS = ( + "TLDR", "Problem", "Consequences", "Example", "Solution", "Gain", + "Tradeoffs", "Validation", +) +TITLE = re.compile(r"[a-z][a-z0-9-]*: \S.*") +PLACEHOLDERS = {"todo", "tbd", "na", "none", "placeholder"} + + +def validate_description(title: object, body: object) -> list[str]: + """Return actionable errors without echoing untrusted PR content.""" + errors = [] + if not isinstance(title, str) or not TITLE.fullmatch(title.strip()): + errors.append("Title must use : , for example chore: enforce PR descriptions.") + if not isinstance(body, str) or not body.strip(): + return errors + ["PR body must contain all required sections with content."] + + body = re.sub(r"|\Z)", "", body, flags=re.S) + sections: list[tuple[str, list[str]]] = [] + preamble = [] + fence_character = "" + fence_length = 0 + for line in body.splitlines(): + fence = re.match(r"^ {0,3}(`{3,}|~{3,})(.*)$", line) + if fence: + marker, suffix = fence.groups() + if not fence_character: + fence_character, fence_length = marker[0], len(marker) + elif marker[0] == fence_character and len(marker) >= fence_length and not suffix.strip(): + fence_character = "" + (sections[-1][1] if sections else preamble).append(line) + continue + heading = re.match(r"^ {0,3}##\s+(.+?)\s*#*\s*$", line) if not fence_character else None + if heading: + sections.append((heading.group(1), [])) + else: + (sections[-1][1] if sections else preamble).append(line) + + if [name for name, _ in sections] != list(SECTIONS): + errors.append("Use exactly these ## headings in order: " + ", ".join(SECTIONS) + ".") + if any(line.strip() for line in preamble): + errors.append("Start the PR body with ## TLDR; put issue links under Problem.") + if fence_character: + errors.append("Close every Markdown code fence.") + for name, lines in sections: + if name not in SECTIONS: + continue + content = "\n".join(line for line in lines if not re.match(r"^ {0,3}(`{3,}|~{3,})", line)) + words = re.sub(r"[\W_]+", "", content.casefold()) + if not words or words in PLACEHOLDERS: + errors.append(f"{name} needs content; explain briefly when it does not apply.") + return errors + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--pull-request-json", required=True, type=Path, + help="JSON object containing title and body, obtained with gh pr view or gh api") + args = parser.parse_args() + try: + payload = json.loads(args.pull_request_json.read_text(encoding="utf-8")) + if not isinstance(payload, dict): + raise ValueError("expected a JSON object") + except (OSError, ValueError) as error: + print(f"PR description input could not be read: {error}", file=sys.stderr) + return 1 + errors = validate_description(payload.get("title"), payload.get("body")) + for error in errors: + print(error, file=sys.stderr) + if errors: + return 1 + print("PR title and all eight description sections are valid.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/ci/pr_description_test.py b/scripts/ci/pr_description_test.py new file mode 100644 index 000000000..068b608a6 --- /dev/null +++ b/scripts/ci/pr_description_test.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +# Copyright 2024-2026 The NoKV Authors. +# SPDX-License-Identifier: Apache-2.0 +"""Regression tests for PR description enforcement.""" + +from __future__ import annotations + +import json +from pathlib import Path +import re +import subprocess +import sys +import tempfile +import unittest + +from pr_description import SECTIONS, validate_description + + +def description() -> str: + return "\n\n".join(f"## {section}\n\nConcrete explanation for {section}." for section in SECTIONS) + + +class DescriptionTest(unittest.TestCase): + def test_complete_description_passes(self) -> None: + self.assertEqual(validate_description("chore: enforce PR descriptions", description()), []) + + def test_title_requires_type_and_outcome(self) -> None: + for title in (None, "", "update docs", "docs:", "docs: ", "docs(scope): update docs"): + with self.subTest(title=title): + self.assertTrue(validate_description(title, description())) + + def test_missing_duplicate_reordered_and_extra_sections_fail(self) -> None: + for body in ( + description().replace("## Example", "### Example"), + description() + "\n\n## Gain\nDuplicate.", + description().replace("## Problem", "## Temporary").replace("## Gain", "## Problem").replace("## Temporary", "## Gain"), + description() + "\n\n## Files Changed\nFile list.", + ): + with self.subTest(body=body): + self.assertTrue(validate_description("docs: update guidance", body)) + + def test_empty_and_comment_only_sections_fail(self) -> None: + for content in ("", "", "- [ ]", "___", "TODO", "_TODO_", "N/A", "None.", "```markdown\n```", "~~~~\n~~~~"): + body = description().replace("Concrete explanation for Tradeoffs.", content) + with self.subTest(content=content): + self.assertTrue(validate_description("docs: update guidance", body)) + + def test_not_applicable_with_reason_passes(self) -> None: + body = description().replace("Concrete explanation for Tradeoffs.", "Not applicable: no runtime behavior changes.") + self.assertEqual(validate_description("docs: update guidance", body), []) + + def test_comments_cannot_supply_headings_or_content(self) -> None: + self.assertTrue(validate_description("docs: update guidance", "")) + body = description().replace("Concrete explanation for Tradeoffs.", "