fix(skills): designer + design-reviewer close mockup issue on approval #252

Merged

code-lead merged 1 commit from dev/248 into main

2026-04-21 15:41:11 +00:00

dev commented

2026-04-21 15:27:18 +00:00

Collaborator

Summary

skills/design-review.md: Penpot handoff path now emits a structured **Verdict**: APPROVED / **Verdict**: CHANGES_REQUESTED line. On APPROVED: removes area:design-review and closes the issue. On CHANGES_REQUESTED: removes area:design-review and re-adds area:design to re-dispatch the designer for another pass.
skills/design-implement.md: added re-run detection at the top — if the most recent design-reviewer comment contains CHANGES_REQUESTED, the skill reads findings, applies/disagrees/defers each, posts a shorter reply, then does the normal label swap back to area:design-review.
CLAUDE.md: documents the close-on-approval semantics and ping-pong loop in the Design review flow section.

Test plan

Create a fresh area:design issue, let designer run, let design-reviewer approve → issue auto-closes and linked impl dependent auto-assigns via the propagator.
Same flow with design-reviewer posting CHANGES_REQUESTED → label flips back to area:design, designer re-runs addressing findings, second pass approves, issue closes.
Biome lint stays green (bun x biome check apps/ packages/).

Closes #248

🤖 Generated with Claude Code

## Summary - `skills/design-review.md`: Penpot handoff path now emits a structured `**Verdict**: APPROVED` / `**Verdict**: CHANGES_REQUESTED` line. On APPROVED: removes `area:design-review` and closes the issue. On CHANGES_REQUESTED: removes `area:design-review` and re-adds `area:design` to re-dispatch the designer for another pass. - `skills/design-implement.md`: added re-run detection at the top — if the most recent design-reviewer comment contains `CHANGES_REQUESTED`, the skill reads findings, applies/disagrees/defers each, posts a shorter reply, then does the normal label swap back to `area:design-review`. - `CLAUDE.md`: documents the close-on-approval semantics and ping-pong loop in the Design review flow section. ## Test plan - [ ] Create a fresh `area:design` issue, let designer run, let design-reviewer approve → issue auto-closes and linked impl dependent auto-assigns via the propagator. - [ ] Same flow with design-reviewer posting `CHANGES_REQUESTED` → label flips back to `area:design`, designer re-runs addressing findings, second pass approves, issue closes. - [ ] Biome lint stays green (`bun x biome check apps/ packages/`). Closes #248 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dev added 1 commit

2026-04-21 15:27:18 +00:00

fix(skills): designer + design-reviewer close mockup issue on approval

qa / qa (pull_request) Successful in 3m18s

Details

qa / dockerfile (pull_request) Successful in 8s

Details

344bd67934

The Penpot handoff review loop left mockup issues open forever after a
completed design-review cycle, blocking impl dependents from
auto-assigning via the propagator.

- design-review.md (Penpot handoff path): emit a structured
  `**Verdict**: APPROVED` or `**Verdict**: CHANGES_REQUESTED` verdict
  line in the findings comment; on APPROVED remove `area:design-review`
  and close the issue; on CHANGES_REQUESTED remove `area:design-review`
  and re-add `area:design` to dispatch another designer pass.
- design-implement.md: detect a re-run (prior CHANGES_REQUESTED comment
  from design-reviewer) before starting; on re-run read findings, apply/
  disagree/defer each, post a shorter reply comment, then perform the
  normal label swap back to `area:design-review`.
- CLAUDE.md: document close-on-approval semantics and the
  ping-pong loop in the Design review flow section.

Closes #248

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev requested review from reviewer

2026-04-21 15:38:01 +00:00

reviewer approved these changes

2026-04-21 15:40:37 +00:00

reviewer left a comment

Review — PR #252

Head: 344bd67 | CI: ✅ green (run #1916, 3m26s)

Acceptance criteria (issue #248)

✅ skills/design-review.md Penpot-handoff path now emits a structured **Verdict**: APPROVED / **Verdict**: CHANGES_REQUESTED line at the end of the review comment (step 4), making the verdict deterministic and machine-readable.
✅ APPROVED path (step 5a/b): removes area:design-review first, then calls mcp__forgejo__update_issue with state: "closed" — unblocks impl dependents via the propagator. Correct tool, correct order.
✅ CHANGES_REQUESTED path (step 5a/b): removes area:design-review, re-adds area:design via mcp__forgejo__add_issue_labels (resolving the label id with list_repo_labels first). Does not close the issue. Label ping-pong mirrors the PR review loop.
✅ skills/design-implement.md re-run detection: scans list_issue_comments for the most recent comment containing **Verdict**: CHANGES_REQUESTED (posted by design-reviewer). If found, enters re-run workflow: reads findings, re-opens the Penpot file from the original handoff deep-link, applies/disagrees/defers each finding, posts a shorter re-handoff comment, then performs the label swap (area:design-review in, area:design out). Completion gate requires reply comment posted + touched frames re-exported + labels swapped. Clean.
✅ CLAUDE.md documents close-on-approval semantics and the designer re-run ("addressed: X, Y, Z" reply) correctly.

Correctness notes

The re-run detection uses "most recent comment containing the verdict string" — correctly handles multiple review rounds (each new CHANGES_REQUESTED from the reviewer becomes the target for the next designer pass).
design-review stays in STATELESS_SKILLS; the re-run detection calls list_issue_comments fresh each time — no session dependency.
Dashboard PR review path (PR context / create_pull_review) is untouched as required.
Operator force-close via Forgejo UI is unaffected — no skill handles that path.

No issues. Fixes the zombie-issue root cause cleanly.

## Review — PR #252 **Head:** `344bd67` | **CI:** ✅ green (run #1916, 3m26s) ### Acceptance criteria (issue #248) - ✅ `skills/design-review.md` Penpot-handoff path now emits a structured `**Verdict**: APPROVED` / `**Verdict**: CHANGES_REQUESTED` line at the end of the review comment (step 4), making the verdict deterministic and machine-readable. - ✅ **APPROVED path** (step 5a/b): removes `area:design-review` first, then calls `mcp__forgejo__update_issue` with `state: "closed"` — unblocks impl dependents via the propagator. Correct tool, correct order. - ✅ **CHANGES_REQUESTED path** (step 5a/b): removes `area:design-review`, re-adds `area:design` via `mcp__forgejo__add_issue_labels` (resolving the label id with `list_repo_labels` first). Does **not** close the issue. Label ping-pong mirrors the PR review loop. - ✅ `skills/design-implement.md` re-run detection: scans `list_issue_comments` for the most recent comment containing `**Verdict**: CHANGES_REQUESTED` (posted by `design-reviewer`). If found, enters re-run workflow: reads findings, re-opens the Penpot file from the original handoff deep-link, applies/disagrees/defers each finding, posts a shorter re-handoff comment, then performs the label swap (`area:design-review` in, `area:design` out). Completion gate requires reply comment posted + touched frames re-exported + labels swapped. Clean. - ✅ `CLAUDE.md` documents close-on-approval semantics and the designer re-run ("addressed: X, Y, Z" reply) correctly. ### Correctness notes - The re-run detection uses "most recent comment containing the verdict string" — correctly handles multiple review rounds (each new `CHANGES_REQUESTED` from the reviewer becomes the target for the next designer pass). - `design-review` stays in `STATELESS_SKILLS`; the re-run detection calls `list_issue_comments` fresh each time — no session dependency. - Dashboard PR review path (PR context / `create_pull_review`) is untouched as required. - Operator force-close via Forgejo UI is unaffected — no skill handles that path. No issues. Fixes the zombie-issue root cause cleanly.