Skip to content

docs(codereview): require design docs for deep, high-risk PRs #441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smolpaws wants to merge 2 commits into
base: main
Choose a base branch
from
Open

docs(codereview): require design docs for deep, high-risk PRs #441

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6e5732
docs(codereview): require design docs for deep, high-risk PRs
enyst Sep 10, 2026
2529f6f
docs(codereview): add ~500+ line diff as a deep-PR signal
enyst Sep 10, 2026
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
36 changes: 36 additions & 0 deletions .agents/skills/custom-codereview-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,42 @@ A review that says "Worth merging" but is submitted as COMMENTED does not
satisfy the repository's required review gate and leaves the PR blocked
indefinitely. Always match your submission action to your verdict.

## Design Docs for Deep PRs

A diff shows what changed line by line, not the design: the shape of the
change, the API before and after, and why this approach. For a *deep* PR,
expect a short design doc. The `pr-design-doc` skill in
`.agents/skills/pr-design-doc/` produces a self-contained `.pr/` HTML page

@all-hands-bot all-hands-bot Sep 23, 2026

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.

Blocking - the doc this points at does not survive approval. pr-design-doc writes to .pr/ (see .agents/skills/pr-design-doc/SKILL.md), and .github/workflows/pr-artifacts.yml's cleanup-on-approval job runs git rm -rf .pr/ as soon as any same-repo approval is submitted - which this bot's approval is, since the guide designates it as satisfying the required gate. Following this guidance can therefore delete the only design artifact before a human maintainer reads it. Issue #443 asks the policy to require durable equivalent context in the PR description before approval, or otherwise reconcile the cleanup timing; this section does neither.

(big picture plus before/after, grounded to real code) linked from the PR
description.

A PR is "deep" when a reviewer cannot fully judge it from the diff in a couple
of minutes, for example:

- a new or changed automation contract, webhook/event payload, or dispatch API;
- a new module or subsystem, or a cross-cutting refactor or migration;
- a behavior change in core logic (scheduling, run history, dispatch flow); or
- a large diff (roughly 500+ lines changed) whose intent a reviewer cannot hold
in their head at once, even if no single hunk is complex.

Skip it for trivial PRs — a typo, a one-line guard, a config or dependency
bump, a docs tweak, a small localized bug fix. If the diff is its own
explanation, do not ask for a page.

When a deep PR ships without a design doc, weigh the omission against the
change's risk assessment:

- **🔴 HIGH risk and deep, no design doc:** withhold approval. Submit the

@enyst enyst Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔴 Critical — approval removes the evidence too early: The repository's PR Artifacts workflow removes .pr/ immediately after any same-repository approval. Because this bot's approval satisfies the required review gate, approving on the strength of a branch-linked design doc can delete that page before a human maintainer reads it. Require either prior human-maintainer review while the doc is present or durable equivalent context in the PR description with a minimum rubric: intent, important before/after behavior or API shape, compatibility/risk, and grounded code references.

@all-hands-bot all-hands-bot Sep 23, 2026

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.

Blocking - "equivalent write-up" has no minimum content. Issue #443's acceptance criteria require the PR-description alternative to state intent, the important before/after behavior or API shape, the compatibility/risk, and grounded code references. As written, "an equivalent write-up in the PR description" lets a thin or non-durable write-up satisfy the policy while giving a maintainer nothing to judge, so the HIGH-risk withhold-approval branch can still be cleared without durable design context. Please define the minimum content the write-up must carry.

review as COMMENTED and ask for a design doc (or an equivalent write-up in
the PR description) so a human can judge the proposal before merge.
- **🟡 MEDIUM risk and deep, no design doc:** use judgment. Prefer to withhold
approval and request one when the change is hard to reconstruct from the
diff; a MEDIUM change that is small and self-evident does not need a page.
- **🟢 LOW risk:** never block on a missing design doc.

A design doc is a review aid, not a merge gate by itself. A well-written doc
does not excuse real correctness, security, or architecture problems.

## Repository Context

This repository uses GitHub branch protection rules that require at least one
Expand Down
Loading