marcel/familienarchiv
1
0
Fork 0
Code Issues 109 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

feat(sdd): adopt review-issue, review-pr, implement skills to the SDD workflow

Browse Source 
review-issue becomes the SDD spec-review gate (adds the requirements engineer,
pairs each .claude persona identity with its .specify checklist, EARS/REQ-NNN
aware). review-pr verifies the diff against the constitution and the spec's
REQ-NNN traceability. implement reads the spec artifacts, plans from tasks.md,
ties tasks to REQ-NNN, and keeps the RTM and generated API types in sync.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-06-13 11:56:49 +02:00
committed by marcel
parent a401e595d7
commit fa6677a7c5
3 changed files with 185 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

102
.claude/skills/review-pr/SKILL.md
View File

@@ -1,74 +1,96 @@
---
name: review-pr
description: Multi-persona PR review. Each persona from .claude/personas/ reviews the PR and posts their findings as a separate Gitea comment.
description: Multi-persona SDD code review of a Gitea PR. Each persona pairs its .claude/personas/ identity with its .specify/personas/ checklist, verifies the diff against the constitution and the feature spec's REQ-NNN (every requirement implemented and tested), and posts findings as a separate Gitea comment.
---
# Multi-Persona PR Review
# Multi-Persona PR Review (SDD)
You will perform a thorough multi-persona code review of the given PR URL and post each persona's findings as a **separate comment** on the PR.
You will perform a thorough multi-persona code review of the given PR and post each persona's
findings as a **separate comment**. Under SDD, the review verifies the diff against two
contracts: the project [constitution](../../../.specify/constitution.md) and the feature's
spec (`spec.md` — every `REQ-NNN` must be implemented **and** covered by a test).
## Argument
The user provides a Gitea PR URL, e.g.:
`http://heim-nas:3005/marcel/familienarchiv/pulls/160`
Parse it to extract:
- `owner` — e.g. `marcel`
- `repo` — e.g. `familienarchiv`
- `pull_number` — e.g. `160`
Parse it to extract `owner`, `repo`, and `pull_number`.
## Step 1 — Gather PR Context
## Step 0 — Load the SDD ground truth
Read before reviewing:
- [`.specify/constitution.md`](../../../.specify/constitution.md) — rules the code must obey (esp. §4 Do-Not-Touch)
- [`.specify/AGENTS.md`](../../../.specify/AGENTS.md) — constraints
- The feature's spec, if the PR references one: `.specify/features/<name>/spec.md`, plus its
`api-contract.yaml`, `threat-model.md`, and `tasks.md`. Find the `REQ-NNN` ids it claims to
satisfy (from the PR description, `Closes #n`, or the spec's Traceability table).
- [`.specify/rtm.md`](../../../.specify/rtm.md) — the requirement→test→status matrix
## Step 1 — Gather PR context
Use the Gitea MCP tools to collect:
1. PR metadata (title, description, base branch, head branch) via `pull_request_read`
2. The list of changed files via `get_dir_contents` or the PR files endpoint
3. The full diff / file contents of every changed file — read each file at the head commit using `get_file_contents`
1. PR metadata (title, description, base/head branch) via `pull_request_read`
2. The list of changed files
3. The full content of every changed file at the head commit via `get_file_contents`
Read ALL changed files completely before starting any review. Do not skip files.
Read ALL changed files completely before starting. Do not skip files.
## Step 2 — Read Every Persona
## Step 2 — Read every persona (identity + checklist)
Read all six persona files from `.claude/personas/`:
- `developer.md` → Felix Brandt
- `architect.md` → architect persona
- `tester.md` → tester persona
- `security_expert.md` → security persona
- `ui_expert.md` → UI/UX persona
- `devops.md` → DevOps persona
Adopt each persona's voice from `.claude/personas/`; apply its review lens. For the SDD
personas, also re-read the matching `.specify/personas/` checklist — at PR time the same
checklist items are verified against the **code** rather than the spec.
## Step 3 — Write Each Review
| Persona | Identity (`.claude/personas/`) | Checklist (`.specify/personas/`) | PR-time focus |
|---|---|---|---|
| Requirements Engineer | `req_engineer.md` | `requirements-engineer.md` | Traceability: every `REQ-NNN` implemented; RTM updated |
| Developer (Felix Brandt) | `developer.md` | `developer.md` | Clean code, layering, generate:api run, ErrorCode four-site |
| Tester | `tester.md` | — (uses identity) | Test quality: each REQ has a real failing-first test; edge cases; levels right |
| Security (Nora "NullX") | `security_expert.md` | `security.md` | authn/authz, IDOR, mass-assignment, `{@html}`, secrets/PII |
| DevOps | `devops.md` | `devops.md` | migration rollback, env vars, CI guards intact, artifact pin |
| UI/UX | `ui_expert.md` | `ui-ux.md` | states, i18n, a11y, design tokens |
| Architect | `architect.md` | `architect.md` | boundaries, ADR present for irreversible choices, no superseded-ADR violation |
For each persona, fully adopt their identity, priorities, and review lens as described in their persona file. Write a review that:
## Step 3 — Write each review
For each persona, write a review that:
- Opens with a one-line verdict: **✅ Approved**, **⚠️ Approved with concerns**, or **🚫 Changes requested**
- Lists concrete findings with file paths and line references where relevant
- Distinguishes blockers (must fix) from suggestions (nice to have)
- Uses the persona's voice and priorities (e.g. Felix cares about TDD and clean code; the security expert checks for injection, auth, and data exposure; the architect checks layer boundaries and coupling)
- Stays focused — only comment on what the persona would actually care about
Format each comment in Markdown with a persona header, e.g.:
- Lists concrete findings with file paths and line references; cite the constitution rule
(e.g. "violates §2.4 — `updatedBy` bound from request body") or the `REQ-NNN` at issue
- Distinguishes **blockers** (must fix) from **suggestions** (nice to have)
- **Requirements Engineer specifically** produces a traceability table — for each `REQ-NNN`:
is it implemented? is there a test? is `rtm.md` updated to `Done`? Any unimplemented or
untested REQ is a blocker. Any code behavior with no backing requirement is flagged
(scope creep — should it be a new REQ, or removed?).
- A constitution **Do-Not-Touch** violation (edited generated file, edited shipped migration,
edited an Accepted ADR, bumped the artifact action past v3, weakened a CI guard) is always
a blocker.
```
## 👨‍💻 Felix Brandt — Senior Fullstack Developer
### 🔐 Security — PR Review
**Verdict: ⚠️ Approved with concerns**
### Blockers
...
- `UserAvatarController.java:42` — REQ-009's 403 path has no test (constitution §2.8)
### Suggestions
...
- ...
```
## Step 4 — Post Comments
## Step 4 — Post comments
Post each persona's review as a **separate comment** on the PR using the Gitea MCP `issue_write` tool (issues and PRs share the comment API in Gitea).
Post each persona's review as a **separate comment** via the Gitea MCP `issue_write` tool
(issues and PRs share the comment API). Post all personas; if one has nothing to flag, post a
brief "LGTM" naming what they checked.
Post all six comments. Do not skip any persona even if their domain has nothing to flag — in that case write a brief "LGTM" with a short explanation of what they checked.
## Step 5 — Report back
## Step 5 — Report Back
After all comments are posted, summarize to the user:
- Which personas posted comments
- The overall verdict across all personas (worst-case wins: if any said "Changes requested", the overall is "Changes requested")
- A bullet list of the top blockers found (if any)
Summarize to the user:
- Each persona's verdict and the overall verdict (worst-case wins: any "Changes requested" → overall "Changes requested")
- The full list of blockers, grouped by persona
- **Traceability status:** which `REQ-NNN` are implemented+tested vs. missing, and whether
`rtm.md` is in sync
- Any constitution Do-Not-Touch violations (called out explicitly)