mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-14 01:50:39 +09:00
[codex] Add skills CLI and catalog management (#6782)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies through company-scoped control-plane workflows. > - Agents need reusable, inspectable skills that can be installed, reset, audited, exported, and assigned without bespoke local setup. > - The existing skill truth model needed cleanup so bundled skills, optional catalog skills, runtime skills, and adapter-provided skills have clear provenance. > - Operators also need a practical CLI and board UI for discovering and managing company skills. > - This pull request adds the skills CLI, packaged skills catalog, company skills APIs, and catalog-aware board UI. > - The benefit is a more reusable Paperclip company setup where skills are portable, auditable, and easier for operators and agents to manage. ## What Changed - Added `paperclipai skills` CLI commands and coverage for catalog listing, installing, resetting, and inspecting company skills. - Added a packaged `@paperclipai/skills-catalog` workspace with bundled and optional skill content plus validation/build tests. - Added shared company-skill types and validators used across CLI, server, and UI contracts. - Added server catalog APIs/services for company skill catalog operations, reset semantics, audit behavior, and portability provenance. - Updated adapter skill handling so runtime/catalog provenance remains explicit across local adapters. - Added board UI support for browsing and managing catalog-backed company skills. - Updated docs for the skills CLI/catalog flow and the company skills Paperclip skill reference. - Rebased the branch onto current `paperclipai/paperclip:master`; no `pnpm-lock.yaml`, `.github/workflows`, or migration files are included in the final PR diff. ## Verification - Passed: `pnpm run preflight:workspace-links && pnpm exec vitest run cli/src/__tests__/skills.test.ts packages/skills-catalog/src/catalog-builder.test.ts packages/skills-catalog/src/shipped-catalog.test.ts packages/shared/src/validators/company-skill.test.ts packages/adapter-utils/src/server-utils.test.ts packages/plugins/create-paperclip-plugin/src/entrypoints.test.ts server/src/__tests__/company-skills-catalog-service.test.ts server/src/__tests__/company-skills-routes.test.ts server/src/__tests__/company-portability.test.ts`. - Passed: `pnpm exec vitest run server/src/__tests__/workspace-runtime.test.ts -t "default branch|origin/master|symbolic-ref"`. - Attempted: full `server/src/__tests__/workspace-runtime.test.ts`. Four provisioning tests failed while seeding an isolated worktree database from the local Paperclip instance because the local plugin schema dump contains a duplicate-column foreign key (`plugin_content_machine_18a7bc327b.content_case_signals`). The default-branch tests touched by the rebase conflict passed in the focused run above. - Checked final diff: no `pnpm-lock.yaml`, no `.github/workflows`, and no migration-file changes relative to `master`. ## Risks - Medium: this is a broad skills/catalog change touching CLI, server APIs, shared contracts, adapter skill sync, and UI. - Catalog validation and reset semantics need careful reviewer attention because they affect reusable company setup and portability. - No database migrations are included in this PR, so there is no migration ordering/idempotency risk in the final diff. - No lockfile is included by design; dependency resolution will be handled by the repository lockfile workflow. ## Model Used - OpenAI Codex coding agent based on GPT-5, running in Paperclip via the `codex_local` adapter with shell, git, GitHub CLI, and code-editing tool access. Exact hosted model build/context-window metadata is not exposed in this runtime. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have run targeted tests locally and documented the local workspace-runtime seed failure above - [x] I have added or updated tests where applicable - [x] If this change affects the UI, screenshots were intentionally omitted per PAP-10124 instructions; UI behavior is covered by tests and reviewer inspection - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Dotta
GitHub
parent
8da50dbcf8
commit
9eac727cf1
77 changed files with 9704 additions and 530 deletions
|
|
@ -0,0 +1,75 @@
|
|||
---
|
||||
name: doc-maintenance
|
||||
description: Keep project docs aligned with recent code and feature changes — detect drift, update affected pages, and add release-relevant notes without rewriting unchanged sections.
|
||||
key: paperclipai/bundled/docs/doc-maintenance
|
||||
recommendedForRoles:
|
||||
- engineer
|
||||
- product
|
||||
- devrel
|
||||
tags:
|
||||
- docs
|
||||
- documentation
|
||||
- release-notes
|
||||
---
|
||||
|
||||
# Doc Maintenance
|
||||
|
||||
Keep the documentation honest with minimum churn. The goal is alignment between docs and behavior, not stylistic rewrites or cosmetic re-organization. Reviewers should be able to read a diff and see "this updates docs to match recent behavior changes".
|
||||
|
||||
## When to use
|
||||
|
||||
- A PR or recent set of merges changed user-visible behavior: CLI flags, API shapes, default values, configuration keys, endpoints, environment variables, supported versions.
|
||||
- A user-reported bug traced back to outdated documentation.
|
||||
- A release is being cut and the docs need a pass against the merged commits.
|
||||
- A new feature shipped but only the engineer's PR description describes how to use it.
|
||||
|
||||
## When not to use
|
||||
|
||||
- The change is internal-only (private helper rename, refactor) with no user-visible impact.
|
||||
- You want to "improve the docs" without a behavior anchor. That is a separate scoped project, not maintenance — make a plan first.
|
||||
|
||||
## The pass
|
||||
|
||||
1. **Establish the baseline.** Get the commit range you are documenting against (since last release tag, since last merged-doc commit, or since a specific PR).
|
||||
2. **Enumerate user-visible changes.** Read commits and PR descriptions. List, for each change, what a user can now do differently.
|
||||
3. **Map changes to docs.** For each change, find every page that mentions the affected concept. Common targets: README, CLI reference, API reference, configuration reference, migration guide, FAQ, examples.
|
||||
4. **Update precisely.** Edit only the lines that need to change. Do not rewrap paragraphs you did not modify — it pollutes the diff.
|
||||
5. **Add new entries where needed.** New CLI flag → CLI reference entry. New env var → configuration reference entry. New endpoint → API reference entry. Don't only add it to the changelog.
|
||||
6. **Update examples and snippets.** Code blocks in docs are wrong faster than prose. Re-run any example that touches new behavior.
|
||||
7. **Write the release note.** One sentence per user-visible change. Group by Added / Changed / Fixed / Deprecated / Removed. Link to the relevant PRs and docs section.
|
||||
8. **Cross-check.** Search the docs for the old behavior wording and remove or update stragglers.
|
||||
|
||||
## Style baseline
|
||||
|
||||
- Voice: second person ("you can pass `--json` to ..."). Avoid "we" except in narrative pages.
|
||||
- Tense: present, not future. The behavior exists once shipped.
|
||||
- Headings: imperative ("Configure the cache") or noun-phrase ("Cache configuration"), match the surrounding page.
|
||||
- Code blocks: include the language tag so syntax highlighting works.
|
||||
- Cross-links: link the first mention of a concept on each page; do not link every occurrence.
|
||||
- Avoid promising future behavior. If something is unreleased, mark it `experimental` or omit it.
|
||||
|
||||
## Drift detection
|
||||
|
||||
A doc page is drifting if any of these are true:
|
||||
|
||||
- It documents a flag, key, or endpoint that no longer exists.
|
||||
- An example does not run as written.
|
||||
- A default value in the docs does not match the code.
|
||||
- A supported-versions list excludes a version the project actually supports, or includes one it dropped.
|
||||
- A "Coming soon" section references a feature that shipped or was cancelled.
|
||||
|
||||
When you find drift, fix it in the same pass and note it in the release note's `Fixed` group.
|
||||
|
||||
## Release-note rules
|
||||
|
||||
- One sentence per item. If two sentences are needed, the item is likely two items.
|
||||
- User impact first, internal cause second. `Faster cold start (avoid full bundle download on first run)` beats `Refactor bootstrap loader`.
|
||||
- Link the PR for engineering readers and the docs page for users.
|
||||
- Mark breaking changes explicitly: `**Breaking:**` prefix. Include migration steps inline or via link.
|
||||
|
||||
## Anti-patterns
|
||||
|
||||
- Massive doc PRs that bundle stylistic rewrites with real updates. Reviewers cannot tell which lines reflect actual behavior changes.
|
||||
- "Updated docs" commit messages with no detail. Make the commit say what changed and why.
|
||||
- Adding to the changelog without updating the reference docs the changelog points to.
|
||||
- Marking a feature as available before its code lands. Documentation must follow behavior, not promise it.
|
||||
|
|
@ -0,0 +1,74 @@
|
|||
---
|
||||
name: issue-triage
|
||||
description: Triage Paperclip inbox issues that are stale, blocked, in-review, or assigned-but-not-progressing, and decide a single next action per issue (resume, reassign, unblock, escalate, or close).
|
||||
key: paperclipai/bundled/paperclip-operations/issue-triage
|
||||
recommendedForRoles:
|
||||
- manager
|
||||
- ceo
|
||||
- engineer
|
||||
tags:
|
||||
- paperclip
|
||||
- triage
|
||||
- inbox
|
||||
- workflow
|
||||
---
|
||||
|
||||
# Issue Triage
|
||||
|
||||
Convert a noisy inbox into a small set of clear next actions. Each pass through this skill should leave every touched issue with a defined owner, status, and the single concrete action that will move it forward.
|
||||
|
||||
## When to use
|
||||
|
||||
- Daily or shift-start review of `in_progress`, `in_review`, and `blocked` assignments.
|
||||
- An inbox has many open assignments and no clear priority.
|
||||
- A manager wants a status read on their reports without asking each agent.
|
||||
- You are woken by a comment that suggests an old issue stalled.
|
||||
|
||||
## When not to use
|
||||
|
||||
- You are checked out on one specific issue and the wake context names it. Work that issue, do not triage the whole inbox.
|
||||
- An issue thread already has an open `request_confirmation` or `ask_user_questions`. Wait for the response — re-triage is noise.
|
||||
|
||||
## Inputs
|
||||
|
||||
- `GET /api/agents/me/inbox-lite` for the compact assignment list.
|
||||
- For each candidate issue, `GET /api/issues/{issueId}/heartbeat-context` for compact state including `blockerAttention`, `executionState`, ancestors, and `commentCursor`.
|
||||
- Only fall back to the full thread when the heartbeat context is not enough.
|
||||
|
||||
## Per-issue triage decision
|
||||
|
||||
For each issue, classify into exactly one of:
|
||||
|
||||
1. **Resume** — execution path is alive. Confirm the assignee is set and let the heartbeat continue. Do not comment.
|
||||
2. **Wake-needed** — assignee is stalled with no live continuation. Post one comment that names the blocker resolution or the exact next action, then leave `in_progress` or move to `todo` so the assignee picks it up.
|
||||
3. **Reassign** — the assignee is not the right specialty. Reassign and set `in_review` only if the new assignee is human, otherwise leave `in_progress`.
|
||||
4. **Unblock** — a first-class `blockedByIssueIds` entry is now `done` or `cancelled`. If `cancelled`, replace or remove it from `blockedByIssueIds`. The blockers-resolved wake will fire automatically when all are `done`.
|
||||
5. **Escalate** — the issue needs board, CTO, or user input. Create a `request_confirmation`, `ask_user_questions`, or `request_board_approval` and set the issue to `in_review`.
|
||||
6. **Close** — work is complete, duplicate, or no longer relevant. Set `done` or `cancelled` with a one-line reason.
|
||||
|
||||
If you cannot classify in under a minute of reading, escalate rather than guess.
|
||||
|
||||
## Stuck-state heuristics
|
||||
|
||||
- `in_progress` with no comments or document updates in the last 24h and no monitor or queued continuation → wake-needed.
|
||||
- `in_review` with no reviewer participant, no pending interaction, no approval — invalid review path → reassign to a real reviewer or move to `todo`.
|
||||
- `blocked` with no `blockedByIssueIds`, only free-text "blocked by X" → convert to first-class blockers or move to `todo` with a named action.
|
||||
- `blocked` with all blockers `done` → unblock the issue by setting status back; the assignee will wake.
|
||||
- Child issues all complete but parent still `in_progress` → confirm parent acceptance, then close.
|
||||
|
||||
## Don't-do list
|
||||
|
||||
- Do not @-mention agents during triage; mentions cost budget. Use direct reassignment instead.
|
||||
- Do not re-comment on a `blocked` issue if your most recent comment was also a blocked update with no reply since.
|
||||
- Do not cancel cross-team issues. Reassign to the responsible manager with a comment.
|
||||
- Do not change status without a comment that explains the change.
|
||||
|
||||
## Output of a triage pass
|
||||
|
||||
A short comment chain or summary message that lists, per issue touched:
|
||||
|
||||
- Issue id and title.
|
||||
- Verdict (resume / wake-needed / reassign / unblock / escalate / close).
|
||||
- The one action you took or asked for.
|
||||
|
||||
This is the bar for "the triage is done."
|
||||
|
|
@ -0,0 +1,84 @@
|
|||
---
|
||||
name: task-planning
|
||||
description: Turn a Paperclip issue or request into a structured implementation plan with child task graph, blockers, owners, and acceptance criteria, then save it as the issue `plan` document.
|
||||
key: paperclipai/bundled/paperclip-operations/task-planning
|
||||
recommendedForRoles:
|
||||
- manager
|
||||
- engineer
|
||||
- product
|
||||
tags:
|
||||
- paperclip
|
||||
- planning
|
||||
- issues
|
||||
- delegation
|
||||
---
|
||||
|
||||
# Task Planning
|
||||
|
||||
Produce implementation plans that the Paperclip executor can actually run: explicit child issues, real blockers, named owners, and a defined acceptance bar. Avoid plans that read well but cannot be split into work.
|
||||
|
||||
## When to use
|
||||
|
||||
- An issue asks you to "plan", "scope", "break down", "design the rollout", "propose the work", or similar.
|
||||
- A user wants a written plan before approving implementation.
|
||||
- A manager needs to delegate non-trivial work and the shape of the work is not obvious yet.
|
||||
- You inherited an issue too large to deliver in one heartbeat and need to split it.
|
||||
|
||||
## When not to use
|
||||
|
||||
- The issue is a single small change you can ship in the same heartbeat. Just ship it.
|
||||
- The issue is forensic ("why did this break"). Use a diagnosis skill first; plan only after the root cause is named.
|
||||
- A current `plan` document already exists and the change is minor. Update that document; do not start fresh.
|
||||
|
||||
## Outputs
|
||||
|
||||
1. An updated issue document with key `plan` (markdown).
|
||||
2. A short comment on the issue that links to the plan document and names the next action.
|
||||
3. Where the plan requires approval, an issue-thread interaction of kind `request_confirmation` bound to the latest plan revision.
|
||||
|
||||
Do not create implementation subtasks until the plan is accepted.
|
||||
|
||||
## Plan structure
|
||||
|
||||
Required sections, in order:
|
||||
|
||||
1. **Goal** — one paragraph. What changes for the user, the operator, or the system once this work lands.
|
||||
2. **Context reviewed** — bullet list of documents, files, and prior issues you read. Lets reviewers spot missing inputs.
|
||||
3. **Constraints and non-goals** — what must hold (compatibility, security, performance) and what this plan deliberately will not do.
|
||||
4. **Approach** — the chosen path, with a short rationale. If you considered alternatives, name them and why you rejected them.
|
||||
5. **Work breakdown** — ordered list of child issues. Each child has:
|
||||
- Title in imperative form.
|
||||
- Owner specialty (Engineer, QA, Designer, Security, DevRel, Manager, etc.).
|
||||
- Scope and deliverables.
|
||||
- Acceptance criteria.
|
||||
- Blocks/blocked-by relationships expressed by phase letter or child title.
|
||||
6. **Acceptance** — the bar for the parent issue. How the user knows the whole thing is done.
|
||||
7. **Risks and mitigations** — short list. Skip if there are none.
|
||||
8. **Deferrals** — what is intentionally pushed to follow-up issues, with why.
|
||||
|
||||
## Rules of thumb for splitting
|
||||
|
||||
- One child issue, one specialty. If two specialties have to coordinate inside the same issue, split it.
|
||||
- One child issue, one acceptance verdict. If a reviewer would say "this is half done", split it.
|
||||
- A child must be checkout-able by the owner from its title and description alone. Reviewers should not have to re-read the parent plan to understand a child.
|
||||
- Order children by real blocker chains, not by author preference. Parallel children should explicitly say `blockers: none`.
|
||||
- Avoid `polish` or `cleanup` child issues without acceptance criteria — they never close.
|
||||
|
||||
## Filing the plan
|
||||
|
||||
Use the Paperclip API to write the plan document, then comment:
|
||||
|
||||
- `PUT /api/issues/{issueId}/documents/plan` with the markdown body. If `plan` already exists, include the latest `baseRevisionId`.
|
||||
- `POST /api/issues/{issueId}/comments` with a short summary that links the plan: `/<prefix>/issues/<issue-id>#document-plan`.
|
||||
- If approval is required: `POST /api/issues/{issueId}/interactions` with `kind: request_confirmation`, `targetRevisionId` set to the new plan revision, `continuationPolicy: wake_assignee`, and `idempotencyKey: "confirmation:{issueId}:plan:{revisionId}"`.
|
||||
- Set the issue to `in_review` after creating the confirmation. Stay assigned so the acceptance wakes the planner.
|
||||
|
||||
When the plan is accepted, see the companion skill for converting accepted plans into Paperclip executable tasks.
|
||||
|
||||
## Anti-patterns
|
||||
|
||||
- Plan disguised as a description edit. Use the `plan` document.
|
||||
- "Phases A–Z" with no work breakdown inside the phases.
|
||||
- Children with descriptions that say "see parent" — they fail at delegation time.
|
||||
- Acceptance written as "code review approval". Reviewers need a behavior bar, not a process bar.
|
||||
- Plans that bury blocker chains in prose. Use explicit blocked-by lines.
|
||||
|
|
@ -0,0 +1,93 @@
|
|||
---
|
||||
name: qa-acceptance
|
||||
description: Produce QA acceptance criteria and a manual validation plan for a feature change — golden path, edge cases, error states, performance limits, and explicit pass/fail evidence.
|
||||
key: paperclipai/bundled/quality/qa-acceptance
|
||||
recommendedForRoles:
|
||||
- qa
|
||||
- engineer
|
||||
- product
|
||||
tags:
|
||||
- qa
|
||||
- acceptance
|
||||
- validation
|
||||
- testing
|
||||
---
|
||||
|
||||
# QA Acceptance
|
||||
|
||||
Write acceptance criteria that a reviewer can run against the running app and decide pass or fail without asking the author. The criteria are the contract — automated tests cover correctness, QA covers feature-level behavior.
|
||||
|
||||
## When to use
|
||||
|
||||
- A feature change is heading to QA and needs a written validation plan.
|
||||
- A reviewer is asked to verify a PR that touches user-visible behavior.
|
||||
- An incident postmortem requires a regression check before reopen-prevention.
|
||||
- A release candidate needs a pre-cut smoke pass.
|
||||
|
||||
## When not to use
|
||||
|
||||
- The change is unit-test-only (utility refactor, internal naming). Acceptance criteria are unnecessary churn.
|
||||
- You are asked to write tests against API contracts. Use contract testing, not feature QA.
|
||||
|
||||
## Acceptance criteria format
|
||||
|
||||
Each criterion is a single, independently-verifiable statement:
|
||||
|
||||
```md
|
||||
- **Given** <starting state>, **when** <action>, **then** <observable outcome>.
|
||||
```
|
||||
|
||||
Example:
|
||||
|
||||
```md
|
||||
- **Given** a CSV export with 0 rows, **when** the user clicks Export, **then** the file downloads with only the header row and the UI shows "Exported 0 rows".
|
||||
```
|
||||
|
||||
Avoid criteria that combine multiple `when`s or `then`s. Split them.
|
||||
|
||||
## What every plan must cover
|
||||
|
||||
1. **Golden path.** The most common successful flow, end to end.
|
||||
2. **Empty and minimum states.** Zero items, one item, missing optional inputs.
|
||||
3. **Boundary inputs.** Max length strings, max numeric values, unicode, RTL text where applicable.
|
||||
4. **Error states.** Network failure, permission denied, validation failures, conflict (409), not found (404).
|
||||
5. **Concurrency and ordering.** Two users acting at once, race against background jobs, refresh during mutation.
|
||||
6. **Performance envelope.** The largest realistic input the change must handle without UI hangs or timeouts.
|
||||
7. **Backward compatibility.** Existing data, existing URLs, persisted user preferences continue to work.
|
||||
8. **Telemetry and audit.** Events, logs, or activity entries the change is supposed to emit.
|
||||
|
||||
If a section is genuinely not applicable, write "N/A: <why>" — do not silently omit.
|
||||
|
||||
## Evidence
|
||||
|
||||
Each criterion needs evidence on the verification pass:
|
||||
|
||||
- Screenshot or short clip for UI behavior.
|
||||
- Copied console / network output for API behavior.
|
||||
- Log snippet or activity row for telemetry.
|
||||
- Timing measurement for performance criteria.
|
||||
|
||||
"Looks good to me" without evidence is not a pass.
|
||||
|
||||
## Quarantine and follow-up
|
||||
|
||||
- A failing criterion blocks acceptance unless explicitly waived by the owner with a tracked follow-up issue.
|
||||
- "Known issue" without a linked follow-up is not a waiver.
|
||||
- If you add a new criterion mid-pass, restart the pass — partial coverage hides regressions.
|
||||
|
||||
## Handoff back to the author
|
||||
|
||||
Return the validation plan with three sections:
|
||||
|
||||
- **Pass.** Criteria that passed, with one-line evidence summaries.
|
||||
- **Fail.** Criteria that failed, with the exact reproduction.
|
||||
- **Blocked.** Criteria you could not run, with why.
|
||||
|
||||
The author owns turning failures into either fixes or accepted deferrals.
|
||||
|
||||
## Anti-patterns
|
||||
|
||||
- Acceptance phrased as test plan ("write a Cypress test for X"). Acceptance is what is true after the change ships; tests are how you check.
|
||||
- Criteria that depend on inspecting implementation details (selectors, query plans). Stay observable.
|
||||
- Long checklists with no priority. Mark must-pass criteria distinctly from nice-to-have.
|
||||
- Validation reports that say "passed" with no evidence. Reviewers cannot audit those.
|
||||
|
|
@ -0,0 +1,93 @@
|
|||
---
|
||||
name: github-pr-workflow
|
||||
description: Prepare a GitHub pull request from a feature branch — branch hygiene, commit shape, title/body, verification notes, screenshots for UI work, and replies to review comments.
|
||||
key: paperclipai/bundled/software-development/github-pr-workflow
|
||||
recommendedForRoles:
|
||||
- engineer
|
||||
tags:
|
||||
- github
|
||||
- pull-requests
|
||||
- code-review
|
||||
- release
|
||||
---
|
||||
|
||||
# GitHub Pull Request Workflow
|
||||
|
||||
Ship a PR a reviewer can land without follow-up clarifying questions. The aim is high signal in the title and body, evidence the change works, and clean replies when feedback comes in.
|
||||
|
||||
## When to use
|
||||
|
||||
- You are about to open a PR for a change that is functionally complete.
|
||||
- A reviewer left comments and you need to respond and push fixes.
|
||||
- A PR has been open more than a day and needs to be brought back into shape (stale conflicts, missing description, missing verification).
|
||||
|
||||
## When not to use
|
||||
|
||||
- The change is not yet functionally complete. Finish the work first; draft PRs that bounce on review are noise.
|
||||
- The repository uses a non-GitHub forge. Adjust to that forge's conventions; do not force GitHub-isms.
|
||||
|
||||
## Branch hygiene before opening
|
||||
|
||||
- Rebase or merge from the target base so the diff is current.
|
||||
- Squash WIP commits into reviewable units. Prefer one commit per logical change; do not force one-commit-per-PR if the work is genuinely multi-step.
|
||||
- Confirm tests, typecheck, and lint pass locally. Note any deliberate skips in the PR body.
|
||||
- Remove debug prints, commented-out code, and `TODO` markers that are not tracked.
|
||||
|
||||
## PR title
|
||||
|
||||
- Imperative mood, under 70 characters.
|
||||
- Lead with the user-visible change, not the file touched. `Allow CSV export from reports table` beats `Update reports.tsx`.
|
||||
- If the repo uses an issue prefix convention (`PAP-1234:`, `[security]`), follow it.
|
||||
- No trailing period.
|
||||
|
||||
## PR body
|
||||
|
||||
Use this structure:
|
||||
|
||||
```md
|
||||
## Summary
|
||||
- 1–3 bullets describing what changed and why.
|
||||
|
||||
## Implementation notes
|
||||
- Anything non-obvious in the diff: trade-offs, dropped alternatives, gotchas.
|
||||
- Migration or config implications.
|
||||
|
||||
## Verification
|
||||
- The exact commands or steps you ran.
|
||||
- Screenshots or short clips for UI changes (required if pixels moved).
|
||||
- Edge cases you exercised by hand.
|
||||
|
||||
## Risk and rollback
|
||||
- What breaks if this is reverted, and how to revert cleanly.
|
||||
```
|
||||
|
||||
Skip the `Risk and rollback` section only for clearly trivial PRs (typos, docs).
|
||||
|
||||
## Verification evidence
|
||||
|
||||
- Tests passing in CI is necessary, not sufficient. Reviewers also need to know the change behaves correctly end to end.
|
||||
- For UI work, include screenshots of the golden path and one edge case. Tag dark and light mode if the project supports both.
|
||||
- For migrations, include a dry-run plan and reversal steps.
|
||||
- For performance changes, include a before/after measurement, not adjectives.
|
||||
|
||||
## Replying to review comments
|
||||
|
||||
- Reply on every comment, even with just "fixed in <commit-sha>" — silent fixes leave the reviewer guessing.
|
||||
- Push fixes as new commits while review is active; do not amend during review unless the reviewer agrees.
|
||||
- If you disagree with feedback, say so with one sentence of rationale and let the reviewer decide. Don't escalate over comments.
|
||||
- Re-request review explicitly after pushing changes.
|
||||
|
||||
## Merge checklist
|
||||
|
||||
- All required checks green.
|
||||
- All review comments resolved.
|
||||
- PR title/body still accurate (update if scope changed mid-review).
|
||||
- Linked issue moves to `in_review` or `done` per project convention.
|
||||
- Delete the branch after merge unless it is a long-lived integration branch.
|
||||
|
||||
## Anti-patterns
|
||||
|
||||
- PR description that says "see commits". Reviewers should not need to read the log.
|
||||
- Mixing refactor and behavior change in the same PR with no separation in the body.
|
||||
- "Address feedback" commits that bundle unrelated edits. One commit per round of feedback is fine; one commit for everything in flight is not.
|
||||
- Force-pushing during active review without telling the reviewer.
|
||||
Loading…
Add table
Add a link
Reference in a new issue