warpdotdev/common-skills已通過檢查
SKILL DETAIL
write-pr-description
warpdotdev/common-skills/write-pr-description
This skill is used to draft or revise the body of a pull request (PR). It guides the process of collecting facts, following the repository's template, writing the body content, and adding reviewer guidance when warranted. The skill emphasizes that the description should explain why the change was made, what it affects, decisions made along the way, and where the author wants attention, rather than repeating what the diff shows. It provides advice on verifying information, handling templates, writing concise and effective prose, and trimming unnecessary content.
安裝量 · 152查看來源
Installation
npx skills add https://github.com/warpdotdev/common-skills --skill write-pr-description
技能檔案
SKILL.md
最近同步 · 2026年8月29日
evals/evals.json›
{
"skill_name": "write-pr-description",
"notes": "Each eval regenerates a PR description blind: the agent works from the diff, the commits, and the repository, and must not read the existing PR body, its review comments, or any linked conversation. Evals 1-4 are the tuning set. The real descriptions on these PRs are the reference points when judging output, not a target to reproduce.",
"evals": [
{
"id": 1,
"name": "small-iam-grant",
"prompt": "Write the PR description for warpdotdev/warp-terraform#1453. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A short description in the repository's fallback shape, since warp-terraform has no PR template. It explains what the new role permits that the previous one did not, and confirms the grant is scoped to staging. Despite the two-line diff it treats the permission change as the thing under review. It does not pad, and it does not claim validation that was not run.",
"files": [],
"assertions": [
"Uses a sensible section shape (such as Why / What changed / Validation) rather than inventing headings from another repository's template, because warp-terraform has no PR template.",
"States what the new IAM role allows that the previous role did not.",
"States that the change is scoped to staging and does not affect production.",
"Treats the permission grant as the review surface even though the diff is two lines.",
"Stays short: no padding, no restatement of the diff line by line.",
"Any validation described is presented as something to run or as a fact, and is not fabricated as a completed result."
]
},
{
"id": 2,
"name": "focused-bugfix",
"prompt": "Write the PR description for warpdotdev/warp#15396. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A description following the warp PR template, explaining why a fresh SDK process has no configured MCP execution path and why inheriting the process PATH is safe there. It names the per-execution-mode decision and the alternative, handles the machine-read CHANGELOG block correctly, and does not tick the manual-testing box falsely.",
"files": [],
"assertions": [
"Follows the warp template headings (Description, Linked Issue, Testing, Agent Mode) in the template's order.",
"Explains the root cause: the fresh SDK/CLI process has no mcp_execution_path because no terminal session has bootstrapped one.",
"Names the decision about which execution modes may fall back to inheriting the process PATH, and gives the reasoning.",
"Handles the CHANGELOG block deliberately: either supplies a CHANGELOG-BUG-FIX entry or explicitly opts out, rather than dropping the block.",
"Does not tick the manual-testing checkbox without a supporting statement.",
"Contains no narration of the branch's own review history or earlier revisions."
]
},
{
"id": 3,
"name": "ui-readonly-row",
"prompt": "Write the PR description for warpdotdev/warp-server#15736. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A description following the warp-server template, with the What and Why sections filled in and the gate checklists answered honestly (no new APIs, no new tables, no prompt changes, no breaking changes). It notes that the change is user-visible and that screenshots belong in the PR, without fabricating them or the test results.",
"files": [],
"assertions": [
"Follows the warp-server template headings (What, Why, the gate checklists, Testing, Agent Mode).",
"Answers each gate checklist rather than deleting the sections, and does not claim a new API, table, prompt change, or breaking change that the diff does not contain.",
"Explains the user-visible improvement, not just the components touched.",
"Recognizes that a UI change calls for a screenshot or recording, and does not invent one.",
"Describes only validation that was actually run, without inventing test counts or results.",
"Does not enumerate the changed files one by one as a substitute for explanation."
]
},
{
"id": 4,
"name": "authz-field-diff",
"prompt": "Write the PR description for warpdotdev/warp-server#15607. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A description following the warp-server template that explains the tag-driven field permission mechanism before its application to the factory patch endpoint, names the behavior changes, and includes reviewer guidance: a reading order that starts at the mechanism, and focus areas covering the permission policy encoded in the struct tags and the decisions the author is least certain about.",
"files": [],
"assertions": [
"Follows the warp-server template headings.",
"Explains the field-level permission mechanism before describing its use by the factory endpoint.",
"Includes a recommended reading order that starts with the new mechanism rather than with the handler or the tests.",
"Scopes the change honestly: notes which added surfaces have no production caller yet, so the reviewer does not judge them against the wrong standard.",
"Names at least one genuine open question, risky assumption, or one-way door, with enough specificity that a reviewer knows where to look.",
"Names the behavior changes that fall out of the change, including permission requirements that differ from before.",
"Assumes a competent reviewer: no generic instructions about checking tests or error handling.",
"Does not restate the diff file by file across 19 files."
]
},
{
"id": 5,
"name": "tiny-flake-fix",
"prompt": "Write the PR description for warpdotdev/warp#15394. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A short description following the warp template. It explains why the test is flaky rather than only that it is, and it stays proportionate to a two-line test-only change. No review guide, because there is nothing to guide.",
"files": [],
"assertions": [
"Follows the warp template headings.",
"Explains the cause of the flake, not just the symptom.",
"States that the change is test-only and touches no production behavior.",
"Stays proportionate to a two-line diff and does not include a reading order or focus areas.",
"Handles the CHANGELOG block deliberately, most likely by opting out."
]
},
{
"id": 6,
"name": "staging-network-rule",
"prompt": "Write the PR description for warpdotdev/warp-terraform#1449. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A description in the fallback shape that explains why a self-request from the server is rejected by the staging IP allowlist, what is now routed around it, and how the exposure is bounded. The security framing is the point: what is now reachable that was not, and what limits it.",
"files": [],
"assertions": [
"Uses a sensible section shape, since warp-terraform has no PR template.",
"Explains why the request fails today before describing the fix.",
"States precisely what path is now exempted and what remains behind the allowlist.",
"Describes the bound on the new exposure, such as the rate limit and the exact-path match.",
"Reports validation honestly: either the plan output if it was run, or a plain statement that it was not, without predicting plan contents as though they had been observed."
]
},
{
"id": 7,
"name": "cloud-interconnect",
"prompt": "Write the PR description for warpdotdev/warp-terraform#1409. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A description in the fallback shape explaining the AWS half of a cross-cloud bridge, the resources it creates, and the non-obvious dependency choice of a second Terraform provider, with the reason that choice was forced.",
"files": [],
"assertions": [
"Explains the purpose of the bridge and how this change relates to the half that already exists.",
"Describes the resources created in terms of their role, not as a list of Terraform addresses.",
"Calls out the use of a second Terraform provider and why the primary provider was not sufficient.",
"Notes that this is staging.",
"Keeps the prose in active voice with short sentences."
]
},
{
"id": 8,
"name": "wip-branch-no-pr",
"prompt": "I'm getting ready to put up the branch in /Users/ben/src/warp-server for review. Write the PR description for it. Treat that checkout as strictly read-only: do not check out, stash, commit, or modify anything there. There is no PR yet, so work from the branch itself.",
"expected_output": "A description following the warp-server template, drafted from the branch rather than from an existing PR. It accounts for the specs committed on the branch, notes the uncommitted work rather than silently describing it as landed, and includes reviewer guidance appropriate to a change of this size.",
"files": [],
"assertions": [
"Works from the local branch and its base without requiring an existing PR.",
"Leaves the checkout untouched: no checkout, stash, commit, or file modification in that repository.",
"Uses the specs committed on the branch as source material for the motivation.",
"Distinguishes committed work from uncommitted work rather than describing everything as done.",
"Includes a reading order and focus areas appropriate to a change of this size.",
"Follows the warp-server template headings."
]
},
{
"id": 9,
"name": "xl-foundation-slice",
"prompt": "Write the PR description for warpdotdev/warp-server#13839. Work from the diff and the commits only - do not read the current PR body or any of its comments. Give me the description text, ready to paste.",
"expected_output": "A description for a very large foundational change that states what is deliberately out of scope, gives a dependency-ordered reading order, and concentrates on the decisions a reviewer must ratify: the storage and deletion contracts, the authorization boundary, and the parts that are hard to change once data exists.",
"files": [],
"assertions": [
"States the scope boundary: what this change lands and what it deliberately defers.",
"Includes a dependency-ordered reading order that starts with the schema or the core contract.",
"Identifies the one-way doors, such as the persisted schema and the manifest format, as focus areas.",
"Calls out the authorization boundary and who can reach the data.",
"Names generated and mechanical files as skimmable.",
"Contains no narration of the branch's own review history, rework passes, or prior versions of the description.",
"Remains navigable: the reader can find the reading order and focus areas without scrolling through undifferentiated prose."
]
}
]
}
references/plain-language.md›
# Plain language for PR descriptions
ASD-STE100 (Simplified Technical English) is a controlled-language standard for technical
writing that is read fast, under load, by someone who did not write the thing being
described. That describes a PR description. Use it as a baseline, not a compliance
target: the goal is prose a reviewer parses correctly on the first pass.
The rules below are the whole of it. The [exemptions](#what-is-exempt) matter as much as
the rules, because a rule applied to an identifier does damage.
## The rules
**Use the active voice.** Name the actor. "The parser rejects unknown keys", not
"unknown keys are rejected". Passive voice hides who does the work, which is exactly
what a reviewer of a permissions or validation change needs to know.
**One topic per sentence.** If a sentence carries a fact and its consequence and a
caveat, it needs to be two or three sentences.
**Keep sentences under about 25 words.** Long sentences in a PR description almost
always turn out to be two facts joined by a comma.
**Use simple tenses.** Prefer "the gate rejected the request" over "the gate had been
rejecting requests". Simple past, simple present, simple future cover nearly everything.
**Keep the articles and the connecting words.** "Fix for case where config is nil" reads
as a telegram. "This fixes the case where the config is nil" costs three words and one
less reparse. Keep "that" and "which" rather than dropping them.
**Use one term per concept, for the whole description.** If you call it a tenant in the
first paragraph, it is a tenant in the last. Switching between tenant, organization, and
account makes a reviewer wonder whether you mean three things. The same applies in
reverse: do not use one word for two concepts.
**Do not stack more than three nouns.** "Consumer retry backoff configuration override"
forces the reader to guess the bracketing. Break it up with prepositions: "the override
for the retry backoff on the consumer".
**Use a vertical list when there is more than one condition or step.** Prose that
carries three conditions in one paragraph hides at least one of them.
**Prefer the specific word.** "Handles", "supports", "manages", and "processes" are
placeholders for a verb you have not chosen yet. "Validates", "retries", "rejects",
"caches" tell the reviewer what the code does.
**Keep paragraphs to about six sentences.** Past that, split them or make them a list.
## What is exempt
STE's approved-word list is for controlled documentation, not code review. These are
technical names and stay exactly as they are:
- Code identifiers, type names, field names, and file paths: `FactoryConfig`,
`spawn_server_impl`, `logic/factories.go`.
- Command lines and their flags, verbatim.
- Established repository and domain jargon: merge queue, feature flag, migration,
presubmit, one-way door.
- Product and service names.
Do not paraphrase an identifier into prose to satisfy a word rule. `nil` is `nil`, not
"an empty value".
## Two rewrites
**Passive and vague:**
> The cache layer was updated so that stale entries can be handled appropriately.
**Active and specific:**
> Each cache entry now records the version of the pricing table it was built from. The
> lookup compares that version against the current one, and rebuilds the entry when they
> differ.
---
**Intent stated as result:**
> Added tests to make sure the migration is safe to roll back.
**Result stated as result:**
> `migrate down` drops the index and leaves no other residue. The integration test
> asserts this against a local PostgreSQL instance.
## Words that add nothing
Cut these unless they carry weight: comprehensive, robust, cleanly, properly, simply,
just, basically, essentially, in order to, it should be noted that, a number of.
"Simply" and "just" are worth singling out. They tell a reviewer that the thing they are
about to read is easy, which is either redundant or wrong, and occasionally insulting.
references/review-guide.md›
# Writing guides for reviewers
A review guide says where to start and where to concentrate. The author knows which parts
were hard; the reviewer does not. That gap is the whole value.
## Which part of this file you need
- Deciding whether to write a guide at all: [When a guide is warranted](#when-a-guide-is-warranted).
- Small or medium PR, a few files: [Focus areas](#focus-areas). Skip the reading order.
- Large PR, or one where order matters: [Focus areas](#focus-areas) and
[Recommended reading order](#recommended-reading-order).
- Checking tone before you post: [Tone](#tone).
## When a guide is warranted
Three questions. Any one is enough.
**Is the reading order non-obvious?** If understanding file B requires having read file A
first, say so. This is about structure, not size.
**Do you have a specific place you want attention?** Something you are unsure of, a
decision that went one of two ways, a surface where a mistake is expensive. One real
pointer justifies a guide on a PR of any size.
**Can this break something well beyond the files it touches?** See below.
If none of the three holds, skip the guide. A large but mechanical PR usually needs one
sentence saying it is mechanical and naming the one file that is not.
### Blast radius, calibrated
Blast radius is how far a change can break things beyond the code it touches. It is not
the same as user visibility, and not the same as importance.
Wide, and worth a guide even when the diff is tiny:
- Permissions, authorization, IAM grants, tenancy boundaries
- Network and firewall rules, exposure of an endpoint
- Schema migrations, stored data formats, retention and deletion behavior
- Public API contracts and anything an external consumer parses
- Feature-flag defaults
- Foundational shared components, and architecture-level changes such as reworking
client-side routing or navigation
- Anything that changes a contract other code already depends on
Narrow, even when the change is user-visible and worth doing well:
- A small UI change, or a change to a lightly-shared component
- Adding or moving a few routes, as opposed to reworking how routing works
- Copy changes
- A new endpoint or a new component that nothing else depends on yet
- A test-only change
The test is what else can break, not how many people will see it. A visible tweak to one
row of one page has a narrow blast radius. An invisible change to how every page resolves
its navigation state does not.
## Focus areas
Point at specific places. A focus area that does not name a file, a symbol, or a decision
is not a focus area.
**Where you are unsure.** The single most valuable line an author can write. State the
call you made, why you think it is right, and what you are unsure about. This applies to
agent-authored changes too: if you inferred an intent that is written down nowhere, that
inference is exactly what needs a human.
**Discrepancies that look unintended.** A name, a value, or a comment that does not match
its surroundings. A resource named for one group but bound to another. A comment claiming
parity with a setting that is ten times different. A threshold that disagrees with the
constant it derives from.
These are visible in the diff, and that is exactly why they get missed: nothing marks
them as wrong, so a reviewer reads them as intended. You are usually the only person who
knows whether it was deliberate. Say which you think it is, and ask. Do not suppress one
because the reviewer could technically have spotted it, and do not quietly fix an
unrelated one either, since that buries it in the diff.
**Security-relevant surfaces.** Authentication, authorization, tenancy, secrets, network
exposure, user data. Say what the change permits that was not permitted before, and who
is now inside the boundary. Reviewers routinely miss a widened permission arriving as a
one-line role change.
**Decisions with a meaningful alternative.** Where a reasonable engineer would have gone
the other way, name the alternative and why it lost. The reviewer can then disagree with
the reasoning instead of reverse-engineering it. Skip non-decisions; a list of them
dilutes the real ones.
**One-way doors, where the ambiguity is genuine.** Persisted schema, stored formats,
public API shape, anything with a migration cost or an external consumer. The test is
whether reversing later would be expensive *and* you are not certain. A one-way door you
are confident about is worth a sentence of documentation, not a focus area.
Useful shape: the location, the decision, the reason, the question.
> `billing/invoice.go` treats a zero-amount line item as a deletion rather than a no-op,
> so it disappears from the rendered invoice. I think that is right, because the upstream
> system only emits zero for removed items. Worth a second opinion.
## Recommended reading order
Only for PRs large enough that the reviewer must choose an order. Below roughly ten
non-generated files, focus areas alone are usually better.
Order by dependency, not by directory and not by the order you wrote it. The reviewer
should meet each piece after the thing it depends on. The usual shape:
1. The new contract or mechanism: the type, the schema, the interface, the migration.
2. Its first real consumer, so the mechanism has a purpose before it has details.
3. The wiring: handlers, callers, registration.
4. The mechanical remainder, called out as such.
What makes an order worth reading:
- One clause of rationale per step. "Start with the store interface, because everything
else is an application of it." A bare list of paths is a table of contents.
- Three to six steps. Past that it becomes a file listing.
- Name what can be skimmed, and why. Generated code, mocks, snapshots, lockfiles:
"`types.gen.go` is generated from `openapi.yaml`; review the yaml." This buys real time.
- If the order needs a paragraph of preamble to make sense, the PR probably wants
splitting. Consider it, and say so if you cannot.
## Tone
Assume a competent engineer who reviews code regularly.
Do not explain the practice of code review. "Please check for edge cases", "make sure the
tests are meaningful", "verify error handling is correct" describe the reviewer's job back
to them, and displace the pointer only you could have given.
Do not explain the reviewer's own tools or your language's semantics. Explain this change.
Avoid:
> Please review carefully, especially the tests and error handling. Let me know if you
> have any questions!
Prefer:
> The retry path in `uploader.go` is the part I am least sure of: a retry reuses the same
> object key, so a partial write from the previous attempt is overwritten rather than
> appended. That is intentional, but it depends on the key being deterministic.
## Worked examples
**Small diff, wide blast radius** (one line added to a CORS origin allowlist):
> This puts a third-party domain inside the browser trust boundary for the authenticated
> API, so any XSS on that domain can read authenticated responses. The entry is
> exact-match rather than a wildcard subdomain, which is the part worth confirming.
**Medium PR, one real decision:**
> The decision worth checking is what happens to a poison message. I chose to dead-letter
> after three attempts rather than retry indefinitely, because an unparseable payload
> never becomes parseable and the retry loop blocks the partition. The cost is that a
> transient parse failure now lands in the dead-letter queue.
**Large PR:**
> Reading order:
>
> 1. `cache/entitlements` - the mechanism. Everything else applies it.
> 2. `model/plans` - the invalidation key each field declares. Most worth disagreeing
> with, since it encodes policy rather than mechanism.
> 3. `logic/checkout.go` and its tests - the first consumer, and the only behavior change
> users will notice.
> 4. `handlers/` - wiring, mechanical apart from the cold-start path.
>
> `model/mocks/` is generated by mockery.
>
> Focus areas:
>
> - A field with no declared invalidation key fails at construction, so every new plan
> field forces a decision. Deliberate, and it will stop the next person who adds a field
> without reading this. Worth agreeing on now.
> - The cache is keyed by tenant rather than by user, so a permission change takes effect
> for the whole tenant at once. I traded granularity for hit rate here.
SKILL.md›
---
name: write-pr-description
description: Writes the body of a pull request - the summary, the repository template sections, and reviewer guidance such as a recommended reading order and focus areas. Use whenever drafting or revising a PR description, filling in a repository's PR template, refreshing a description that no longer matches the branch, or preparing a branch for review. Use it even when the user only says "open a PR", "put this up for review", or "write this up" without naming the description.
---
# write-pr-description
A pull request description has one job: give the reviewer what the diff cannot.
The diff already states what changed. The description states why it changed, what it
affects, which decisions were made along the way, and where the author wants attention.
Everything below follows from that. When a rule here conflicts with what a specific
reviewer needs, serve the reviewer.
`create-pr` covers the mechanics of opening the PR. This skill covers what goes in the body.
## 1. Collect the facts before drafting
Usually you did the work and already know most of this. When you are describing a branch
you did not write, or a PR you have just been handed, start here.
```bash
# From a checkout of the branch
git --no-pager log <base>..HEAD # commit bodies first, they carry the why
git --no-pager diff <base>...HEAD --stat
# From a PR number, with no local branch
gh pr view <n> --repo <owner/repo> --json title,commits,files,baseRefName
gh pr diff <n> --repo <owner/repo>
```
Read the commit bodies before the diff. They are usually the richest source of
motivation. Then verify what they claim: a commit message saying "matches the existing
pattern in this file" is an assertion about code, and it is often wrong. Do not forward a
claim you have not checked.
Verify by looking, not by reasoning. The checks worth making are cheap and specific:
- Read what the change deletes. When a change exists to fix something, the defect is
usually visible in the removed code.
- Ask the system what a thing means, rather than inferring it: `gcloud iam roles
describe` for a role's permission set, the provider or API schema for a resource's
fields, the parser for what a marker does.
- Grep a flag across the environment files before claiming it is on or off.
- Open the file a comment or commit message points at, and confirm it says what the
pointer claims.
One check of this kind usually produces the best sentence in the description.
Gather what the diff cannot tell the reviewer:
- The motivation. What breaks, costs, or stays impossible without this change.
- The linked issue, spec, or design doc, and any spec files committed on the branch.
- Decisions with a real alternative, and why the alternative lost.
- The blast radius: what this can break beyond the files it touches.
- The validation you ran, and what it showed.
- Anything you are unsure of.
## 2. Follow the repository's template
Check the repository for a PR template. Where there are several, pick the one matching
the change. Templates differ per repository, so check every time rather than reusing the
shape from your last PR.
**The template's own instructions outrank this skill.** A template that says "remove this
section if it is not relevant" is telling you what this repository's reviewers want.
Follow it. The guidance below applies where the template is silent.
- Keep the headings, their wording, and their order. Reviewers and tooling both scan for
them.
- Answer every section, in your own words, under the heading. Do not reproduce the
template's question text, its explanatory links, or its examples. A template with four
gate sections can otherwise cost two hundred words to say "no" four times.
- Where a section does not apply, say so in a clause that shows you considered it ("No
new tables"), rather than deleting it or leaving it bare.
- Tick a box only when its statement is true. Otherwise leave it unticked with a short
reason on the same line. Do not delete the box, do not tick it with a caveat attached,
and do not restate the box's own text back to the reader.
- Preserve machine-read content: changelog markers, artifact or media markers, issue
linking keys. Check that the marker is live where the template puts it. Some templates
show a marker inside an HTML comment that the parser strips, which means a marker left
in place is silently ignored. Confirm against the parser or a merged PR.
- Instructional HTML comments can go once you have answered them.
When no template exists, use this shape, which is the same shape a template would give
you:
1. The opening paragraph from step 3, with no heading above it. It carries both what and
why, so there is no separate `Why` section.
2. `## Review guide`, when step 4 calls for one.
3. `## What changed`
4. `## Validation`
This shape collapses on a small change. A heading over a single line is the same ceremony
step 4 warns about, so drop any section that would hold one and let the opening carry it.
## 3. Write the body
Open with one to three sentences covering what the change does and why. A reviewer who
reads only the first paragraph should be able to tell whether they are the right
reviewer.
Where the repository has a template, this paragraph goes inside its first content
section, whatever that section is called. Do not add a lead above the template's first
heading, and do not repeat it once inside.
Then give the substance:
- Explain the mechanism only where it is not obvious from the code. A new abstraction, a
non-local invariant, or an unusual control flow needs a paragraph. A renamed field does
not.
- Name behavior changes explicitly, including ones that are side effects of the main
change. These are what break other people. For a permissions change, state the
direction: what is now allowed that was not, and what is now refused that was allowed.
- Record decisions with their rejected alternatives. This is the highest-value content in
most descriptions and it cannot be recovered from the diff. Where there was no real
alternative, state the decision plainly. Do not manufacture one to fill the slot.
- Include visual evidence for a user-visible change: a screenshot, or before and after
where the change is a modification. Reference only captures that exist.
- Link the issue, spec, or conversation once. Where there is none, say so only if the
repository asks for one, through a template section or a contributing guide.
Announcing "no linked issue" in a repository that does not use them is lint compliance
rather than information. Never invent a plausible ticket ID to fill the gap; a wrong
link costs more than no link.
### Validation
You normally ran the work, so report it: the command and what it showed.
Where you did not, or only partly did, these are the honest shapes. More than one can
apply at once: you may have verified a fact yourself and still be waiting on CI for the
rest.
- **Nothing ran.** State it plainly and name the command the reviewer or CI should run.
- **CI produces the result**, because the check needs credentials or an environment you
should not use. Name the job, and say what you expect it to show, marked as an
expectation rather than an observation. "Expect one destroy and one create, nothing
else" gives the reviewer something to check the run against, which is more useful than
silence. Phrase it so it cannot be misread as output you saw.
- **The branch adds tests but no run is recorded.** Say the tests are added and unrun, so
nobody reads the test list as evidence they passed.
- **You could not run it**, for want of a device, credentials, or an environment. Say so,
and name it as a check you are asking the reviewer to make. This does not cover
something you skipped; if you could have run it, run it.
- **Someone else recorded the result**, such as a verification noted in an earlier commit.
Attribute it or leave it out. Never restate it as your own observation.
Whichever apply, state each once. Repeating "this was not run" in three sections reads as
hedging, and buries the one line that says what to run instead.
Never write intent as though it were a result.
### Prose
Use ASD-STE100 as the baseline: active voice, one topic per sentence, sentences under
about 25 words, simple tenses, one term per concept for the whole description. Code
identifiers, command lines, and established repository jargon are technical names; leave
them alone. See [references/plain-language.md](references/plain-language.md) for the
rules and the exemptions.
## 4. Add reviewer guidance when it is warranted
Add a review guide when the reading order is not obvious, when you have a specific place
you want attention, or when the change can break things well beyond the files it touches.
Skip it when a competent reviewer will know where to look without being told.
Put it high, directly after the summary. A reviewer should not have to scroll past
compliance checklists to find where to start. `## Review guide` is a reasonable default
heading when the template does not supply one.
On a small PR, guidance is a sentence or two, not a section. Fold it into the opening
rather than raising a heading over it. A heading on two paragraphs is ceremony, and so is
a reading order for six files.
A review guide contains some or all of:
- A recommended reading order, for PRs large enough that the order matters.
- Focus areas: what you are unsure of, security-relevant surfaces, decisions with a
meaningful alternative, and one-way doors where the ambiguity is genuine.
Write it for a competent engineer. Point at the code and the open question; never explain
how to review code. See [references/review-guide.md](references/review-guide.md), which
opens with a short map of which of its sections you need.
## 5. Cut
Drafts run long, and the excess is almost never in the thinking. It collects in the parts
that feel obligatory: the checklist answers, the inventory of tests, the second statement
of something you already said. Those parts are also the easiest to delete, which makes
this pass cheap and worth doing every time.
Rough anchors for the whole body, before you start cutting:
- A test-only, config, or mechanical change with no behavior change: a short paragraph.
- A small change with a real risk surface: a few hundred words, nearly all of them about
the risk rather than the diff.
- A focused fix or feature: four or five hundred words.
- A new mechanism with consumers: under a thousand.
- A very large or foundational change: around a thousand, with most of the extra spent on
the reading order and the focus areas.
These are anchors, not limits. Being over one means look harder at the list below; it
never means cut a decision.
Then take each paragraph and name the decision it helps the reviewer make. If you cannot
name one, delete it. The usual finds:
- Anything the diff shows at a glance. "The hash-scroll logic was generalized", "the
field was renamed", one bullet per changed line, a restatement of the lockfile. The
exception is something that looks unintended, such as a name that disagrees with what
it binds to. The reviewer can see it but cannot tell it is wrong, so it stays.
- Paraphrases of comments this diff adds. The reviewer reads them in place, one scroll
away.
- Inventories of the tests you added. Name what is *not* covered, and roughly how much of
the change is tested. A list of test function names reads as padding, especially with
no result attached to it.
- Explanations of how standard tools behave. Terraform replacing a renamed resource, how
a test macro polls, when a React effect fires, how a library spawns a child process.
Explain this change, not the reviewer's tools.
- The second statement of a fact. Count them: "no linked issue" and "nothing was run"
each belong in exactly one place, and a fact worth stating twice is usually a fact
stated badly the first time.
- Sentences whose only job is to point at another part of the description. Cross
references inside one page mean the content is in the wrong place.
- Sentences about the description itself. "A review guide is included because this change
has a wide blast radius." Write the guide; do not justify it.
- Commit trailers. `Co-Authored-By` belongs in the commit, not in the PR body.
Keep, even when cutting hard: the motivation, the behavior changes, the decisions and
their rejected alternatives, the open questions, and the blast radius. These are the
reason the description exists. When something has to go, cut mechanism before you cut a
decision.
## 6. Self-check
- Does the first paragraph let a reviewer decide whether the PR is theirs?
- Does every claim about testing describe something that ran, or say plainly that it did
not?
- Have you verified every factual claim carried over from a commit message?
- Did the cut take a decision, a rejected alternative, or an open question with it? Those
are the first things to go when you compress, and the last things you should lose. Put
them back.
- Does the description match the branch as it stands now, rather than the path you took
to get there?
- If there is reviewer guidance, is it near the top?
## Anti-patterns
**Narrating the branch's own history.** The most common failure. Sentences like "this PR
previously included unit tests, which were removed after review feedback", or "the
description above overstated this and has been corrected", describe a transition that
does not exist in the diff the reviewer is reading. The reviewer sees one state against
the base. Describe that state.
Note what survives the rule: the reasoning usually still matters, only the transition
goes. "The tests were removed because they only reasserted the match arms" becomes "there
are no unit tests here, because a test at this layer would only reassert the match arms".
This holds however long the branch is. A branch with thirty commits still reaches the
reviewer as one state against the base.
**Restating the diff.** A file-by-file inventory is the standard way to write something
long that carries no information.
**Unverified claims, about anything.** Inflated test claims are the familiar case ("fully
tested", "no regressions"), but a confident wrong claim about mechanism is more
dangerous, because a reviewer is less likely to check it. Before asserting what a role
permits, what a flag gates, or what a function guarantees, verify it.
**Grading your own work.** "Comprehensive", "robust", "clean", "properly". Padding that
costs credibility.
**Lecturing the reviewer.** "Please check for edge cases and make sure the error handling
is correct." A competent reviewer already does this, and it displaces the specific
pointers only you can give.
**Leaving a stale description.** After a rework or a force-push, rewrite the body to
describe the current branch. Do not append a revision log to the bottom.