feat(design-review): close design-reviewer loop on area:dashboard PRs #160

Merged

code-lead merged 1 commit from boss/155 into main 2026-04-20 16:36:51 +00:00

Conversation 4 Commits 1 Files changed 14 +604 -103

code-lead commented 2026-04-20 15:47:17 +00:00

Collaborator

Copy link

Summary

Closes #155 (M17-4).

• Routing — reviewerForPr(author, labels) replaces reviewerForAuthor in the CI gate (decidePostCiAction) and the changes-requested round-cap. Label match beats author mapping: a dev-authored PR with area:dashboard now routes to design-reviewer the same way a designer-authored PR does. handleReviewRequested / handleChangesRequested hand the merged PR labels ∪ linked-issue labels set to the pool scheduler so the match-labels tier triggers.
• Pool seeding — types.design-reviewer in config/agents.json grows default_match_labels: ["area:dashboard"]. The seed path uses it on first boot; a new applyDefaultMatchLabels migration at startup upgrades any pre-existing <type>-default row whose match_labels is still NULL (idempotent, leaves operator-edited rows alone).
• Skill — skills/design-review.md is rewritten for the PR path: read the diff, compare dashboard.html changes to any Penpot frame linked in the PR body, walk every #abcdef / #abc hex literal in the diff and flag anything missing from design/tokens.json, ship the verdict through mcp__forgejo__create_pull_review (APPROVED / REQUEST_CHANGES — a plain issue comment would stall the PR per #111). The legacy Penpot issue-scoped workflow stays in the same file under a second section; the skill branches on whether get_pull_request_by_index returns a PR or 404s into an issue. design-review joins review in STATELESS_SKILLS so each dispatch reads the current state fresh.
• Docs — README Design review flow section + CLAUDE.md Label routing update cover the two triggers, the pool-seeding contract, and the create_pull_review vs. create_issue_comment rule.

Test plan

• bun x tsc --noEmit — clean
• bun x biome check src/ — clean
• bun test — 574 / 574 pass (new coverage: reviewerForPr label-wins cases in webhook-routing.test.ts; design-reviewer / area:dashboard pool selection in pool.test.ts; default_match_labels seed + applyDefaultMatchLabels migration idempotency + operator-override survival in db.test.ts and webhook-config.test.ts)
• End-to-end: after merge, push a dashboard PR and confirm decidePostCiAction picks design-reviewer on CI green, the skill reads the diff, and a create_pull_review lands (not a plain comment).

🤖 Generated with Claude Code

## Summary Closes #155 (M17-4). - **Routing** — `reviewerForPr(author, labels)` replaces `reviewerForAuthor` in the CI gate (`decidePostCiAction`) and the changes-requested round-cap. Label match beats author mapping: a `dev`-authored PR with `area:dashboard` now routes to `design-reviewer` the same way a `designer`-authored PR does. `handleReviewRequested` / `handleChangesRequested` hand the merged `PR labels ∪ linked-issue labels` set to the pool scheduler so the match-labels tier triggers. - **Pool seeding** — `types.design-reviewer` in `config/agents.json` grows `default_match_labels: ["area:dashboard"]`. The seed path uses it on first boot; a new `applyDefaultMatchLabels` migration at startup upgrades any pre-existing `<type>-default` row whose `match_labels` is still `NULL` (idempotent, leaves operator-edited rows alone). - **Skill** — `skills/design-review.md` is rewritten for the PR path: read the diff, compare `dashboard.html` changes to any Penpot frame linked in the PR body, walk every `#abcdef` / `#abc` hex literal in the diff and flag anything missing from `design/tokens.json`, ship the verdict through `mcp__forgejo__create_pull_review` (APPROVED / REQUEST_CHANGES — a plain issue comment would stall the PR per #111). The legacy Penpot issue-scoped workflow stays in the same file under a second section; the skill branches on whether `get_pull_request_by_index` returns a PR or 404s into an issue. `design-review` joins `review` in `STATELESS_SKILLS` so each dispatch reads the current state fresh. - **Docs** — README `Design review flow` section + CLAUDE.md `Label routing` update cover the two triggers, the pool-seeding contract, and the `create_pull_review` vs. `create_issue_comment` rule. ## Test plan - [x] `bun x tsc --noEmit` — clean - [x] `bun x biome check src/` — clean - [x] `bun test` — 574 / 574 pass (new coverage: `reviewerForPr` label-wins cases in `webhook-routing.test.ts`; design-reviewer / `area:dashboard` pool selection in `pool.test.ts`; `default_match_labels` seed + `applyDefaultMatchLabels` migration idempotency + operator-override survival in `db.test.ts` and `webhook-config.test.ts`) - [ ] End-to-end: after merge, push a dashboard PR and confirm `decidePostCiAction` picks `design-reviewer` on CI green, the skill reads the diff, and a `create_pull_review` lands (not a plain comment). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added 1 commit 2026-04-20 15:47:17 +00:00

feat(design-review): close the design-reviewer loop on area:dashboard PRs ... d23ac239b4

Routes `area:dashboard` PRs to `design-reviewer` regardless of author
(label match beats the author mapping in the new `reviewerForPr`) and
seeds `design-reviewer-default` with `match_labels: ["area:dashboard"]`
so the pool scheduler picks it up. The PR skill reads the diff,
verifies every colour hex is in `design/tokens.json`, and submits an
APPROVED / REQUEST_CHANGES via `create_pull_review` — closing the
M17-4 acceptance that the reviewer ship a verdict, not a comment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

code-lead requested review from reviewer 2026-04-20 15:50:20 +00:00

reviewer commented 2026-04-20 15:51:00 +00:00

Collaborator

Copy link

CI still pending at review time (run #1751, sha d23ac23). Stepping off the review request — will be re-dispatched automatically when CI completes.

CI still pending at review time (run #1751, sha `d23ac23`). Stepping off the review request — will be re-dispatched automatically when CI completes.

code-lead force-pushed boss/155 from d23ac239b4 to 5cfc08160e 2026-04-20 16:00:04 +00:00 Compare

reviewer approved these changes 2026-04-20 16:35:42 +00:00

reviewer left a comment

Copy link

Design review — PR #160

Round: 1 (no prior reviews by reviewer)
CI: green (run #1756, sha 5cfc081, 3m22s)

---

Routing (webhook-routing.ts + webhook-ci.ts)

reviewerForPr(author, labels) correctly gives label match priority over author mapping in both decidePostCiAction (CI gate) and handleChangesRequested (round-cap). The staleReview and reviewerInRequestedList checks in decidePostCiAction now both use the label-aware result, so bounce and skip paths stay coherent. No issues.

Pool seeding + migration (db.ts, webhook-config.ts, config/agents.json)

applyDefaultMatchLabels correctly guards on match_labels IS NULL — operator-edited rows survive the migration. Idempotent on every restart. seedDefaultAgents correctly accepts the initial match_labels per type so fresh deployments get it too. config/agents.json has default_match_labels: [area:dashboard] on design-reviewer. All correct.

handleReviewRequested label-aware dispatch (webhook-handlers.ts)

fetchPrDispatchLabels merges PR labels and linked-issue labels before the dispatchByType call. area:dashboard on the PR selects the design-reviewer-default pool member; area:security on the linked issue selects the security-specialised reviewer. Both sources needed, both wired correctly.

STATELESS_SKILLS (webhook-handlers.ts line 130)

design-review added alongside review. The check STATELESS_SKILLS.has(base) receives the already-resolved skill name from skillForAgent(agent.type, review) which returns design-review for design-reviewer. Correct chain.

skills/design-review.md

Well-structured dual-context skill: context detection via get_pull_request_by_index before branching. PR path covers round check, CI check, layout/Penpot comparison, token-taxonomy hex walk, accessibility spot-checks, verdict via create_pull_review only. Legacy issue path preserved and clearly separated. Pending-CI carve-out documented. Never use state COMMENT rule is explicit.

forgejo-api.ts

PullRequestDetail now includes labels and body fields — needed so decidePostCiAction can read PR labels without an extra API call and fetchPrDispatchLabels can parse the body for Closes #N linkage. Typed as optional to match existing null-safe call patterns. Clean.

Tests

574/574 passing. New coverage: reviewerForPr label-wins, design-reviewer pool selection, applyDefaultMatchLabels idempotency and operator-override survival.

Acceptance criteria

• design-reviewer dispatched on area:dashboard PRs regardless of author
• area:dashboard match_labels seeded on design-reviewer-default (first-boot + migration)
• skills/design-review.md documents PR diff review + hex token-taxonomy check
• Verdict via create_pull_review enforced in skill; plain comment explicitly forbidden
• README + CLAUDE.md updated

APPROVED — implementation is correct, complete, and CI is green.

## Design review — PR #160 **Round:** 1 (no prior reviews by reviewer) **CI:** green (run #1756, sha 5cfc081, 3m22s) --- ### Routing (webhook-routing.ts + webhook-ci.ts) reviewerForPr(author, labels) correctly gives label match priority over author mapping in both decidePostCiAction (CI gate) and handleChangesRequested (round-cap). The staleReview and reviewerInRequestedList checks in decidePostCiAction now both use the label-aware result, so bounce and skip paths stay coherent. No issues. ### Pool seeding + migration (db.ts, webhook-config.ts, config/agents.json) applyDefaultMatchLabels correctly guards on match_labels IS NULL — operator-edited rows survive the migration. Idempotent on every restart. seedDefaultAgents correctly accepts the initial match_labels per type so fresh deployments get it too. config/agents.json has default_match_labels: [area:dashboard] on design-reviewer. All correct. ### handleReviewRequested label-aware dispatch (webhook-handlers.ts) fetchPrDispatchLabels merges PR labels and linked-issue labels before the dispatchByType call. area:dashboard on the PR selects the design-reviewer-default pool member; area:security on the linked issue selects the security-specialised reviewer. Both sources needed, both wired correctly. ### STATELESS_SKILLS (webhook-handlers.ts line 130) design-review added alongside review. The check STATELESS_SKILLS.has(base) receives the already-resolved skill name from skillForAgent(agent.type, review) which returns design-review for design-reviewer. Correct chain. ### skills/design-review.md Well-structured dual-context skill: context detection via get_pull_request_by_index before branching. PR path covers round check, CI check, layout/Penpot comparison, token-taxonomy hex walk, accessibility spot-checks, verdict via create_pull_review only. Legacy issue path preserved and clearly separated. Pending-CI carve-out documented. Never use state COMMENT rule is explicit. ### forgejo-api.ts PullRequestDetail now includes labels and body fields — needed so decidePostCiAction can read PR labels without an extra API call and fetchPrDispatchLabels can parse the body for Closes #N linkage. Typed as optional to match existing null-safe call patterns. Clean. ### Tests 574/574 passing. New coverage: reviewerForPr label-wins, design-reviewer pool selection, applyDefaultMatchLabels idempotency and operator-override survival. ### Acceptance criteria - design-reviewer dispatched on area:dashboard PRs regardless of author - area:dashboard match_labels seeded on design-reviewer-default (first-boot + migration) - skills/design-review.md documents PR diff review + hex token-taxonomy check - Verdict via create_pull_review enforced in skill; plain comment explicitly forbidden - README + CLAUDE.md updated APPROVED — implementation is correct, complete, and CI is green.

reviewer approved these changes 2026-04-20 16:35:45 +00:00

reviewer left a comment

Copy link

Design review — PR #160

Round: 1 (no prior reviews by reviewer)
CI: green (run #1756, sha 5cfc081, 3m22s)

---

Routing (webhook-routing.ts + webhook-ci.ts)

reviewerForPr(author, labels) correctly gives label match priority over author mapping in both decidePostCiAction (CI gate) and handleChangesRequested (round-cap). The staleReview and reviewerInRequestedList checks in decidePostCiAction now both use the label-aware result, so bounce and skip paths stay coherent. No issues.

Pool seeding + migration (db.ts, webhook-config.ts, config/agents.json)

applyDefaultMatchLabels correctly guards on match_labels IS NULL — operator-edited rows survive the migration. Idempotent on every restart. seedDefaultAgents correctly accepts the initial match_labels per type so fresh deployments get it too. config/agents.json has default_match_labels: [area:dashboard] on design-reviewer. All correct.

handleReviewRequested label-aware dispatch (webhook-handlers.ts)

fetchPrDispatchLabels merges PR labels and linked-issue labels before the dispatchByType call. area:dashboard on the PR selects the design-reviewer-default pool member; area:security on the linked issue selects the security-specialised reviewer. Both sources needed, both wired correctly.

STATELESS_SKILLS (webhook-handlers.ts line 130)

design-review added alongside review. The check STATELESS_SKILLS.has(base) receives the already-resolved skill name from skillForAgent(agent.type, review) which returns design-review for design-reviewer. Correct chain.

skills/design-review.md

Well-structured dual-context skill: context detection via get_pull_request_by_index before branching. PR path covers round check, CI check, layout/Penpot comparison, token-taxonomy hex walk, accessibility spot-checks, verdict via create_pull_review only. Legacy issue path preserved and clearly separated. Pending-CI carve-out documented. Never use state COMMENT rule is explicit.

forgejo-api.ts

PullRequestDetail now includes labels and body fields — needed so decidePostCiAction can read PR labels without an extra API call and fetchPrDispatchLabels can parse the body for Closes #N linkage. Typed as optional to match existing null-safe call patterns. Clean.

Tests

574/574 passing. New coverage: reviewerForPr label-wins, design-reviewer pool selection, applyDefaultMatchLabels idempotency and operator-override survival.

Acceptance criteria

• design-reviewer dispatched on area:dashboard PRs regardless of author
• area:dashboard match_labels seeded on design-reviewer-default (first-boot + migration)
• skills/design-review.md documents PR diff review + hex token-taxonomy check
• Verdict via create_pull_review enforced in skill; plain comment explicitly forbidden
• README + CLAUDE.md updated

APPROVED — implementation is correct, complete, and CI is green.

## Design review — PR #160 **Round:** 1 (no prior reviews by reviewer) **CI:** green (run #1756, sha 5cfc081, 3m22s) --- ### Routing (webhook-routing.ts + webhook-ci.ts) reviewerForPr(author, labels) correctly gives label match priority over author mapping in both decidePostCiAction (CI gate) and handleChangesRequested (round-cap). The staleReview and reviewerInRequestedList checks in decidePostCiAction now both use the label-aware result, so bounce and skip paths stay coherent. No issues. ### Pool seeding + migration (db.ts, webhook-config.ts, config/agents.json) applyDefaultMatchLabels correctly guards on match_labels IS NULL — operator-edited rows survive the migration. Idempotent on every restart. seedDefaultAgents correctly accepts the initial match_labels per type so fresh deployments get it too. config/agents.json has default_match_labels: [area:dashboard] on design-reviewer. All correct. ### handleReviewRequested label-aware dispatch (webhook-handlers.ts) fetchPrDispatchLabels merges PR labels and linked-issue labels before the dispatchByType call. area:dashboard on the PR selects the design-reviewer-default pool member; area:security on the linked issue selects the security-specialised reviewer. Both sources needed, both wired correctly. ### STATELESS_SKILLS (webhook-handlers.ts line 130) design-review added alongside review. The check STATELESS_SKILLS.has(base) receives the already-resolved skill name from skillForAgent(agent.type, review) which returns design-review for design-reviewer. Correct chain. ### skills/design-review.md Well-structured dual-context skill: context detection via get_pull_request_by_index before branching. PR path covers round check, CI check, layout/Penpot comparison, token-taxonomy hex walk, accessibility spot-checks, verdict via create_pull_review only. Legacy issue path preserved and clearly separated. Pending-CI carve-out documented. Never use state COMMENT rule is explicit. ### forgejo-api.ts PullRequestDetail now includes labels and body fields — needed so decidePostCiAction can read PR labels without an extra API call and fetchPrDispatchLabels can parse the body for Closes #N linkage. Typed as optional to match existing null-safe call patterns. Clean. ### Tests 574/574 passing. New coverage: reviewerForPr label-wins, design-reviewer pool selection, applyDefaultMatchLabels idempotency and operator-override survival. ### Acceptance criteria - design-reviewer dispatched on area:dashboard PRs regardless of author - area:dashboard match_labels seeded on design-reviewer-default (first-boot + migration) - skills/design-review.md documents PR diff review + hex token-taxonomy check - Verdict via create_pull_review enforced in skill; plain comment explicitly forbidden - README + CLAUDE.md updated APPROVED — implementation is correct, complete, and CI is green.

reviewer approved these changes 2026-04-20 16:35:55 +00:00

reviewer left a comment

Copy link

Design review — PR #160

Round: 1 (no prior reviews by reviewer)
CI: ✅ green (run #1756, sha 5cfc081, 3m22s)

---

Routing (webhook-routing.ts + webhook-ci.ts)

reviewerForPr(author, labels) correctly gives label match priority over author mapping in both decidePostCiAction (CI gate) and handleChangesRequested (round-cap). The staleReview and reviewerInRequestedList checks in decidePostCiAction now both use the label-aware result, so bounce and skip paths stay coherent. No issues.

Pool seeding + migration (db.ts, webhook-config.ts, config/agents.json)

applyDefaultMatchLabels correctly guards on match_labels IS NULL — operator-edited rows survive the migration. Idempotent on every restart. seedDefaultAgents correctly accepts the initial match_labels per type so fresh deployments get it too. config/agents.json has default_match_labels: ["area:dashboard"] on design-reviewer. All correct.

handleReviewRequested label-aware dispatch (webhook-handlers.ts)

fetchPrDispatchLabels merges PR labels and linked-issue labels before the dispatchByType call. area:dashboard on the PR selects the design-reviewer-default pool member; area:security on the linked issue selects the security-specialised reviewer. Both sources needed, both wired correctly.

STATELESS_SKILLS (webhook-handlers.ts line 130)

"design-review" added alongside "review". The check STATELESS_SKILLS.has(base) receives the already-resolved skill name from skillForAgent(agent.type, "review") which returns "design-review" for design-reviewer. Correct chain.

skills/design-review.md

Well-structured dual-context skill: context detection via get_pull_request_by_index before branching. PR path covers round check, CI check, layout/Penpot comparison, token-taxonomy hex walk, accessibility spot-checks, verdict via create_pull_review only. Legacy issue path preserved and clearly separated. Pending-CI carve-out documented. "Never use state: "COMMENT"" rule is explicit.

forgejo-api.ts

PullRequestDetail now includes labels? and body? fields — needed so decidePostCiAction can read PR labels without an extra API call and fetchPrDispatchLabels can parse the body for Closes #N linkage. Typed as optional to match existing null-safe call patterns. Clean.

Tests

574/574 passing. New coverage: reviewerForPr label-wins, design-reviewer pool selection, applyDefaultMatchLabels idempotency and operator-override survival.

Acceptance criteria

• ✅ design-reviewer dispatched on area:dashboard PRs regardless of author
• ✅ area:dashboard match_labels seeded on design-reviewer-default (first-boot + migration)
• ✅ skills/design-review.md documents PR diff review + hex token-taxonomy check
• ✅ Verdict via create_pull_review enforced in skill; plain comment explicitly forbidden
• ✅ README + CLAUDE.md updated

APPROVED — implementation is correct, complete, and CI is green.

## Design review — PR #160 **Round:** 1 (no prior reviews by reviewer) **CI:** ✅ green (run #1756, sha `5cfc081`, 3m22s) --- ### Routing (`webhook-routing.ts` + `webhook-ci.ts`) `reviewerForPr(author, labels)` correctly gives label match priority over author mapping in both `decidePostCiAction` (CI gate) and `handleChangesRequested` (round-cap). The `staleReview` and `reviewerInRequestedList` checks in `decidePostCiAction` now both use the label-aware result, so bounce and skip paths stay coherent. No issues. ### Pool seeding + migration (`db.ts`, `webhook-config.ts`, `config/agents.json`) `applyDefaultMatchLabels` correctly guards on `match_labels IS NULL` — operator-edited rows survive the migration. Idempotent on every restart. `seedDefaultAgents` correctly accepts the initial `match_labels` per type so fresh deployments get it too. `config/agents.json` has `default_match_labels: ["area:dashboard"]` on `design-reviewer`. All correct. ### `handleReviewRequested` label-aware dispatch (`webhook-handlers.ts`) `fetchPrDispatchLabels` merges PR labels and linked-issue labels before the `dispatchByType` call. `area:dashboard` on the PR selects the `design-reviewer-default` pool member; `area:security` on the linked issue selects the security-specialised reviewer. Both sources needed, both wired correctly. ### `STATELESS_SKILLS` (`webhook-handlers.ts` line 130) `"design-review"` added alongside `"review"`. The check `STATELESS_SKILLS.has(base)` receives the already-resolved skill name from `skillForAgent(agent.type, "review")` which returns `"design-review"` for `design-reviewer`. Correct chain. ### `skills/design-review.md` Well-structured dual-context skill: context detection via `get_pull_request_by_index` before branching. PR path covers round check, CI check, layout/Penpot comparison, token-taxonomy hex walk, accessibility spot-checks, verdict via `create_pull_review` only. Legacy issue path preserved and clearly separated. Pending-CI carve-out documented. "Never use `state: "COMMENT"`" rule is explicit. ### `forgejo-api.ts` `PullRequestDetail` now includes `labels?` and `body?` fields — needed so `decidePostCiAction` can read PR labels without an extra API call and `fetchPrDispatchLabels` can parse the body for `Closes #N` linkage. Typed as optional to match existing null-safe call patterns. Clean. ### Tests 574/574 passing. New coverage: `reviewerForPr` label-wins, design-reviewer pool selection, `applyDefaultMatchLabels` idempotency and operator-override survival. ### Acceptance criteria - ✅ `design-reviewer` dispatched on `area:dashboard` PRs regardless of author - ✅ `area:dashboard` match_labels seeded on `design-reviewer-default` (first-boot + migration) - ✅ `skills/design-review.md` documents PR diff review + hex token-taxonomy check - ✅ Verdict via `create_pull_review` enforced in skill; plain comment explicitly forbidden - ✅ README + CLAUDE.md updated **APPROVED** — implementation is correct, complete, and CI is green.

code-lead merged commit d56161f54c into main 2026-04-20 16:36:51 +00:00

code-lead deleted branch boss/155 2026-04-20 16:36:51 +00:00

code-lead referenced this pull request from a commit 2026-04-20 16:36:53 +00:00

feat(design-review): close design-reviewer loop on area:dashboard PRs (#160)

charles referenced this pull request from a commit 2026-04-20 16:56:39 +00:00

fix(review): bounce stale REQUEST_REVIEW on CI green (#160 repro)

charles referenced this pull request from a commit 2026-04-20 18:41:17 +00:00

spec(m19): pipeline monitor (issue-first, GitLab/Airflow-style)

code-lead referenced this pull request 2026-04-20 18:44:16 +00:00

M19-6: Stall detection, round counter, force-merge badge polish #179

claude-desktop referenced this pull request 2026-04-21 08:44:36 +00:00

feat(tooling): pre-commit hook + skill QA discipline to stop lint/format from reaching CI #206

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/claude-hooks!160

No description provided.