From b08b1718091fe4a5e34d86e21e9447e2afd8d544 Mon Sep 17 00:00:00 2001 From: karunkop Date: Wed, 28 Oct 2026 11:55:54 +0545 Subject: [PATCH] Add write-pr skill for pull request descriptions The rules for a pull request description were spread across end-session, create-issue, plan, the Copilot instructions and docs/testing.md, and none of them said what a description should contain. write-pr gives the shape, what to cut, and a worked example, and the instructions that cover opening a pull request now point to it. Claude-Session: https://claude.ai/code/session_01Fd4vvqGUWV42TDr5CEEzGf Co-Authored-By: Claude claude-opus-5-5 (xhigh) --- .agents/skills/write-pr | 1 + .github/agents/worker-bee.agent.md | 2 +- .github/copilot-instructions.md | 2 +- .github/skills/end-session/SKILL.md | 2 +- .github/skills/write-pr/SKILL.md | 162 ++++++++++++++++++++++++++++ AGENTS.md | 2 + docs/agents/external-agents.md | 4 +- docs/agents/skills.md | 1 + 8 files changed, 171 insertions(+), 5 deletions(-) create mode 120000 .agents/skills/write-pr create mode 100644 .github/skills/write-pr/SKILL.md diff --git a/.agents/skills/write-pr b/.agents/skills/write-pr new file mode 120000 index 00000000000..77c84d98249 --- /dev/null +++ b/.agents/skills/write-pr @@ -0,0 +1 @@ +../../.github/skills/write-pr \ No newline at end of file diff --git a/.github/agents/worker-bee.agent.md b/.github/agents/worker-bee.agent.md index 0e642fd21d2..6a3b7e1f213 100644 --- a/.github/agents/worker-bee.agent.md +++ b/.github/agents/worker-bee.agent.md @@ -48,7 +48,7 @@ Once both gates are satisfied (or determined not to apply), continue with the li - Begin by creating a new branch for the work. If a previous agent working on the same task already created a branch and a PR, use that one. - When opening a PR, start the description with a closing keyword and the issue number (e.g. "Fixes #1234") when merging the PR resolves the issue — GitHub then links the issue in the sidebar's Development section and closes it automatically on merge. Use the bare issue number (e.g. "#1234") only when the PR relates to the issue without resolving it. -- Keep the PR description free of redundancy. Say each thing once: no reviewer-notes section that restates the summary, and no consequence the reader has already drawn from it. Never state that tests pass — GitHub renders the CI status on the PR itself. +- Keep the PR description free of redundancy. Say each thing once: no reviewer-notes section that restates the summary, and no consequence the reader has already drawn from it. Never state that tests pass — GitHub renders the CI status on the PR itself. Write the title and description with the `write-pr` skill. - Make all commits in this branch. Push after each meaningful change. Never commit directly to main or protected branches. - Run `yarn prettier --write .` before committing any changes to ensure proper code formatting. There is no pre-commit hook, and formatting violations in source files fail `yarn lint` in CI. - After completing the initial implementation, open a draft pull request with a clear, descriptive title and summary to merge your feature branch into `main`. Create the PR with the `runtime-tools-create_pull_request` tool — do not shell out to `git` or `gh` to open it. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index bf2ce569790..625897667ab 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -45,7 +45,7 @@ Once both gates are satisfied (or determined not to apply), continue with the li - Begin by creating a new branch for the work. If a previous agent working on the same task already created a branch and a PR, use that one. - When opening a PR, start the description with a closing keyword and the issue number (e.g. "Fixes #1234") when merging the PR resolves the issue — GitHub then links the issue in the sidebar's Development section and closes it automatically on merge. Use the bare issue number (e.g. "#1234") only when the PR relates to the issue without resolving it. -- Keep the PR description free of redundancy. Say each thing once: no reviewer-notes section that restates the summary, and no consequence the reader has already drawn from it. Never state that tests pass — GitHub renders the CI status on the PR itself. +- Keep the PR description free of redundancy. Say each thing once: no reviewer-notes section that restates the summary, and no consequence the reader has already drawn from it. Never state that tests pass — GitHub renders the CI status on the PR itself. Write the title and description with the `write-pr` skill. - Make all commits in this branch. Push after each meaningful change. Never commit directly to main or protected branches. - Run `yarn prettier --write .` before committing any changes to ensure proper code formatting. There is no pre-commit hook, and formatting violations in source files fail `yarn lint` in CI. - After completing the initial implementation, open a draft pull request with a clear, descriptive title and summary to merge your feature branch into `main`. Create the PR with the `runtime-tools-create_pull_request` tool — do not shell out to `git` or `gh` to open it. diff --git a/.github/skills/end-session/SKILL.md b/.github/skills/end-session/SKILL.md index db2bc3dcb01..4670cd79552 100644 --- a/.github/skills/end-session/SKILL.md +++ b/.github/skills/end-session/SKILL.md @@ -114,7 +114,7 @@ Use `ci-monitor`. Wait for every run on the branch to complete; do not report on - A draft PR exists for the branch. On Copilot, create it with the `runtime-tools-create_pull_request` tool — do not shell out to `git` or `gh` to open one. In a local harness, where that tool does not exist, `gh pr create --draft` is the equivalent. - Its description starts with the issue reference — `Fixes #1234` when merging the PR resolves the issue (GitHub links it in the sidebar's Development section and closes it on merge), or the bare `#1234` when the issue stays open — with the architectural plan from the `plan` skill below it. -- Its title and summary describe what actually landed — not what you set out to do three fixes ago. +- Its title and summary describe what actually landed — not what you set out to do three fixes ago. The `write-pr` skill says how to write and revise them. - **Every section earns its place.** Say each thing once: a reviewer-notes section that restates the summary is noise, and so is a consequence the reader already drew — "no behaviour change" after you explained the code was unreachable, "no references in `docs/`" after you called it dead code. Cut the section rather than pad it. - **Never state that tests pass.** GitHub renders the CI status on the PR itself; asserting it in prose adds nothing and goes stale the next time you push. diff --git a/.github/skills/write-pr/SKILL.md b/.github/skills/write-pr/SKILL.md new file mode 100644 index 00000000000..4e31a673149 --- /dev/null +++ b/.github/skills/write-pr/SKILL.md @@ -0,0 +1,162 @@ +--- +name: write-pr +description: >- + ALWAYS USE THIS SKILL when writing or editing the title or description of a pull request in this repo: opening one with `gh pr create`, filling in a draft, or rewriting the description after the change moved on. Triggers on "open a PR", "create a PR", "write the PR description", "update the PR body", and any request to draft pull request text. Review comments on someone else's pull request belong to `review-pr` instead. +allowed-tools: + - bash +--- + +A pull request description is read once, by a reviewer, just before they read the diff. Its job is to tell them what the diff cannot: why the change was needed, what it does to behaviour, and the decisions they would otherwise have to ask about. Everything else is already in the diff, the linked issue, or the CI checks, and repeating it is a cost the reviewer pays before they can start. + +So keep it minimal, and make every sentence one the reviewer needs. + +## The shape + +```markdown +Fixes #1234 + + + +- +- <...> +``` + +Most descriptions are that and nothing more. A one-line fix is the reference and a sentence. A larger change gets more bullets, not more sections. + +### The first line + +The issue reference, on its own line. + +- `Fixes #1234` when merging resolves the issue. GitHub links it in the Development sidebar and closes it on merge. +- The bare `#1234` when the issue stays open, with the relationship in words: `Part of #5481`, or `Refs #5762. This makes the test press what it claims to, but does not fix the flake.` Only close an issue whose Expected Behavior this pull request fully meets. +- `Stacked on #6153. Merge that first.` goes above the reference when another pull request has to land first. + +### The opening paragraph + +One to three short sentences, usually the cause in one and the change in another. With a linked issue, do not restate its steps or what the user saw, since the reviewer can open it. Say what the issue could not: the cause, named in the code (`useLongPress.stop` defers `onLongPressEnd` by 10 ms), and what this pull request does about it. + +Minimal means fewer ideas, not denser sentences. A sentence that chains three clauses with "so" and "rather than" to fit the whole mechanism in is harder to read than the two sentences it replaced. Cut the clause the reviewer does not need. + +With no linked issue: + +- **A bug a user can see** carries Steps to Reproduce, Current Behavior and Expected Behavior in the description itself, written as [`create-issue`](../create-issue/SKILL.md) says. They go first, and the changes follow them. +- **Anything else** (a refactor, a test, CI, tooling, a skill) opens with the paragraph alone, saying what was wrong or missing and why it matters. + +### The bullets + +One per change the reviewer should know about, stated as behaviour, with where it lives. They are not a file list, since the Files tab already is one. + +- Group by concern, not by file. +- A change that reaches past the issue gets its own bullet. When the fix is in a shared function, name the other callers that now behave differently (a fix in `moveThought` for Indent changes Outdent and drag-and-drop too). That is what the reviewer most needs to check, and the diff does not show it. +- A doc update is one bullet naming the doc and what it now says. +- A test is one bullet naming the behaviour it covers, never a count. + +### Sections that earn their place + +Add a heading only when the content is a different kind from the bullets and the reviewer would otherwise have to ask. These have each been worth one here: + +- **Decisions.** A choice between reasonable options, each with its one-line reason. The maintainer singled out such a section on #5596 because it "anticipated my questions". +- **For review.** A behaviour change you are not sure the maintainer wants, put as a question (#6093). +- **Not changed** or **Not fixed.** What the reviewer would expect this to touch and it does not, and why (#6118, #6083). +- **Verification.** Only what CI cannot show: a reproduction on a real device, a flake's failure rate before and after, a measurement the fix rests on. +- **Before and after screenshots or video** for a visual change. Copy attachment markup across verbatim. + +No `Summary` heading over the opening paragraph, no `Changes` heading over the only list, and no `Testing` heading that says tests were added. + +### The plan + +The [`plan`](../plan/SKILL.md) skill puts its plan in the description, below the issue reference. Its conclusions belong in the body: the approach taken, and the alternative rejected with its reason, as Decisions. Its evidence, the quoted surface area and the adjacent behaviour checklist, goes in one collapsed block at the end, so it is there for a reviewer who wants it and costs nothing for one who does not. + +```markdown +
+Plan + +... + +
+``` + +The blank line after `` is required, or a list or fence inside will not render. + +## What to cut + +Everything that is about your session rather than about the change. + +- **Results CI already shows.** "The unit suite passes", "eslint, prettier and tsc are clean", test counts. GitHub renders the checks on the pull request, and prose about them goes stale on the next push. +- **The issue, restated.** +- **The story of the investigation.** What you tried first, how you found the cause. Present findings, not a chronology. A number stays only when it is the evidence for the change ("11 of 40 runs before, 0 of 20 after"). +- **Mechanism the diff makes obvious.** "The observer is torn down in a `finally`." +- **Non-events.** "No test changes are needed." "No behaviour change" after you already said the code was unreachable. +- **A section that repeats another.** A reviewer notes section restating the summary, a Docs section restating the docs bullet. +- **Hedges and preambles.** "This PR aims to", "Note that", "It's worth mentioning". + +## A before and after + +The first description of #6163 ran to about 440 words, under Cause, Design, Docs and Testing headings with a results table. Rewritten: + +```markdown +Fixes #5879 + +`dragAndDropThought` waited only for the drag alert. When a held drag moves on to a second drop target (`skipMouseDown`), that alert is already showing. The helper then returned before the second drop hover rendered, in 11 of 40 local runs before this change and 0 of 20 after. + +- Each drop target renders `data-drop-over`, from react-dnd's shallow `isOver`. It is true on a target that rejects the drop too, so a drop that shows no bar still passes. +- The helper records that attribute turning true under the pointer, waits for it once the drag is in progress, and throws if it never does. It records rather than polls because a still pointer fires no `dragover`. A target that a hover moves under the pointer is never hovered, so polling for it would hang. +- It does not wait on `state.hoveringPath`, which is only written when the pointer moves and can still name the previous target. +- The own-position test no longer waits on `.drop-hover` itself, since the helper now does. +- `docs/drag-and-drop.md` describes the attribute and corrects `DropUncle`'s zone to `SubthoughtsDrop`. `docs/testing.md` lists `window.__dropTargetHoverWatch` among the sanctioned exceptions. + +Replaces #5882. +``` + +About 190 words, and nothing the reviewer needs was lost. The before rate stayed because it is the evidence the bug was real, and the after rate because no CI run can show it. The lint and unit-suite results went because CI shows them, the Docs section became one bullet, and the `finally` teardown went because the diff already shows it. + +## Title + +The behaviour that changes, in sentence case, in the imperative: `Indent: Keep an empty thought at the end of a sorted context`. Lead with the area when the change belongs to one, as an issue title does. For test or tooling work, say what changes for whoever runs it: `Wait for the long-press release to be processed in the longPressThought helper`. + +Not a commit-style prefix (`fix(dnd):`), and not the mechanism alone (`Add data-drop-over attribute`). + +## TDD + +- When the pre-fix control is a commit other than the base, put `/tdd ` at the start of its own line. The TDD workflow reads it from the description, so it has to survive every later edit. See [TDD regression validation](../../../docs/testing.md#tdd-regression-validation). +- Skipping TDD is a label (`skip-tdd`, `skip-tdd-unit`, `skip-tdd-puppeteer`, `skip-tdd-ios`), not text. A `/skip-tdd` line in the description does nothing. When you add one, say in a sentence why a red pre-fix run is not expected. + +## Words + +- **No em dashes in prose.** +- **No semicolons in prose.** Two sentences, or a comma and a conjunction. +- **Leave commit hashes bare**, with no backticks, so GitHub links them. +- **Use the project's vocabulary.** Resolve an unfamiliar term in [`docs/glossary.md`](../../../docs/glossary.md), and do not invent one. +- **Format the way an issue does.** Backticks for identifiers and paths, `` for keys, and arrows for gestures, as in [`create-issue`](../create-issue/SKILL.md). + +## Editing a description + +The description says what the pull request does now, the way the docs say how the project works now. When the change moves on, rewrite the sentences it made false, and keep the title in step. Do not append an `Update:` section, since the history is already in the commits and the review thread. + +Keep the issue reference, any `/tdd` line, and the attribution footer in place through every edit. + +On Copilot, the description goes through the `runtime-tools-create_pull_request` tool, as `end-session` says. In a local harness, write the body to a file and pass it with `--body-file`, so the shell does not eat backticks or `$`: + +```bash +gh pr create --draft --title "..." --body-file body.md +gh pr edit --body-file body.md +``` + +## Attribution + +End with the attribution line your harness supplies, once. If one is already there, keep it rather than adding a second. + +## Before you post + +Read it as the reviewer, sentence by sentence, and delete each one they would not miss. Then check: + +- The first line is the issue reference, and `Fixes` only where the issue is fully resolved. +- Nothing claims what CI shows. +- Every heading has something under it that the bullets do not already say. +- The title names the behaviour, not the mechanism. + +[`end-session`](../end-session/SKILL.md) checks the rest: that the pull request is a draft, carries the plan, and describes what actually landed. + +## Escalation + +Default to acting on your own. Where the user or a maintainer has asked for particular content in the description, include it, and keep everything else to this skill. diff --git a/AGENTS.md b/AGENTS.md index a089d9eebcd..01a7e1aea78 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,8 @@ The repo's `commit-msg` hook adds this automatically for Claude Code, Codex, Ope **5. Leave commit hashes bare on GitHub.** In issues, comments, and pull request descriptions, write a commit hash as plain text — no backticks and no code block. GitHub autolinks a bare hash to the commit; wrapping it in code formatting suppresses the link. +**6. Use `write-pr` for a pull request's title and description.** A description tells the reviewer what the diff, the linked issue, and CI do not: the cause, the changes as behaviour, and the decisions they would otherwise ask about. The skill has the shape and what to leave out, and applies when you revise a description as much as when you open one. + ## Accessing documentation - `docs/` contains comprehensive documentation on the codebase. Start from [`docs/readme.md`](docs/readme.md), which indexes every subsystem doc. diff --git a/docs/agents/external-agents.md b/docs/agents/external-agents.md index bd32773c26d..478bbc00c7f 100644 --- a/docs/agents/external-agents.md +++ b/docs/agents/external-agents.md @@ -83,9 +83,9 @@ The skill states *what* must be true and lets a single clause carry *how* per en ## What is shared, and what is not -**Shared** — [`bisect`](skills.md#bisect), [`create-issue`](skills.md#create-issue), [`plan`](skills.md#plan), [`tdd-write-failing-test`](skills.md#tdd-write-failing-test), [`compare-debug-log`](skills.md#compare-debug-log), [`test-diagnosis`](skills.md#test-diagnosis), [`puppeteer-update-snapshots`](skills.md#puppeteer-update-snapshots), [`ci-monitor`](skills.md#ci-monitor), [`docs-sync`](skills.md#docs-sync), [`end-session`](skills.md#end-session), [`liminal`](skills.md#liminal), [`report-review-gaps`](skills.md#report-review-gaps). +**Shared** — [`bisect`](skills.md#bisect), [`create-issue`](skills.md#create-issue), [`plan`](skills.md#plan), [`tdd-write-failing-test`](skills.md#tdd-write-failing-test), [`compare-debug-log`](skills.md#compare-debug-log), [`test-diagnosis`](skills.md#test-diagnosis), [`puppeteer-update-snapshots`](skills.md#puppeteer-update-snapshots), [`ci-monitor`](skills.md#ci-monitor), [`docs-sync`](skills.md#docs-sync), [`end-session`](skills.md#end-session), [`liminal`](skills.md#liminal), [`report-review-gaps`](skills.md#report-review-gaps), [`write-pr`](../../.github/skills/write-pr/SKILL.md). -Three of those needed a per-harness clause. `end-session` and `ci-monitor` name a Copilot tool that has no local equivalent — opening a pull request, listing workflow runs — and now name the `gh` command alongside it. `test-diagnosis` was written as though a failure could only arrive from CI; its trigger now covers a suite run locally, where the output is already on screen rather than in a log to be fetched. +Four of those needed a per-harness clause. `end-session`, `ci-monitor` and `write-pr` name a Copilot tool that has no local equivalent — opening a pull request, listing workflow runs — and now name the `gh` command alongside it. `test-diagnosis` was written as though a failure could only arrive from CI; its trigger now covers a suite run locally, where the output is already on screen rather than in a log to be fetched. `create-issue` needed nothing — it shells out to `gh` and names no provisioned resource. Neither did `liminal`, which is CSS and a pointer to an image in the repo, nor `report-review-gaps`, whose script only calls `gh`. diff --git a/docs/agents/skills.md b/docs/agents/skills.md index 0f926d4218a..e55054c896c 100644 --- a/docs/agents/skills.md +++ b/docs/agents/skills.md @@ -77,6 +77,7 @@ The two green boxes are the gates — the agent must run them before it is allow | [`docs-sync`](#docs-sync) | Repair the documentation your change made untrue, in the same commit | [SKILL.md](../../.github/skills/docs-sync/SKILL.md) | | [`bisect`](#bisect) | Find whether a bug is a recent regression, and the commit and pull request that introduced it | [SKILL.md](../../.github/skills/bisect/SKILL.md) | | `create-issue` | Write a GitHub issue, or the reproduction sections of a pull request with no linked issue, in the format this repo uses | [SKILL.md](../../.github/skills/create-issue/SKILL.md) | +| `write-pr` | Write or edit a pull request's title and description, keeping only what the diff, the issue and CI do not already say | [SKILL.md](../../.github/skills/write-pr/SKILL.md) | | [`liminal`](#liminal) | Style a static artifact, such as a report page, like em's Liminal UI | [SKILL.md](../../.github/skills/liminal/SKILL.md) | | [`report-review-gaps`](#report-review-gaps) | Report the feedback a final reviewer gave after the first review, as review principles | [SKILL.md](../../.github/skills/report-review-gaps/SKILL.md) |