fix(skills): designer + design-reviewer should close the mockup issue on completion #248

New issue

Closed

opened 2026-04-21 15:11:54 +00:00 by claude-desktop · 0 comments

claude-desktop commented

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

Collaborator

Bug

The Penpot mockup workflow leaves the mockup issue open forever because neither the designer nor the design-reviewer skill closes it. Four tickets on 2026-04-21 (#223, #233, #238, #240) all finished their full design-review cycle but stayed open with no assignee — classic zombie state. Each blocked an impl dependent from auto-assigning.

The propagator's Forgejo refuses to close an issue whose /dependencies still has an open blocker rule amplified this: #224 (steering impl) couldn't even be manually closed because it was native-wired blocked by #223, and #223 never closed itself. Five tickets had to be manually cleaned up to unstick the pool.

Current workflow (per CLAUDE.md)

Operator creates issue, adds area:design label.
Designer skill runs: builds Penpot frames, posts handoff comment, swaps the label (removes area:design, adds area:design-review).
Design-reviewer skill runs on the re-labeled issue: exports frames as PNG, inspects multimodally, posts findings, removes the area:design-review label.
…issue stays open forever.

The missing step: nobody closes the issue.

Acceptance criteria

Design-reviewer: close on approval

skills/design-review.md — when the review verdict is approval (no outstanding findings), the skill calls mcp__forgejo__issue_state_change with state="closed" at the end of its run, right after removing the area:design-review label.
When the verdict includes findings (changes requested), the skill does not close. Instead it posts the findings comment and re-adds area:design so the designer picks it back up. The label ping-pong mirrors the PR review loop.
Verdict is determined by a structured line in the review comment — e.g. **Verdict**: APPROVED or **Verdict**: CHANGES_REQUESTED. The skill emits one or the other explicitly so the logic is deterministic.

Designer: idempotent re-runs

skills/designer.md — on a re-run triggered by area:design being re-added, the skill reads the prior design-reviewer findings (most recent CHANGES_REQUESTED comment), addresses them in Penpot, and re-posts a shorter handoff comment ("addressed: X, Y, Z") before swapping the label back to area:design-review.

Operator override

If the operator wants to force-close a mockup issue regardless (e.g., scope changed, mockup abandoned), that stays a manual close via the Forgejo UI. No skill change needed — mcp__forgejo__issue_state_change already accepts that on the operator path.

Dashboard PR review path — unchanged

The other design-reviewer entry point (PRs labeled area:dashboard or authored by designer) already submits create_pull_review APPROVED / REQUEST_CHANGES. The PR merge closes the linked issue — no change needed there. Only the Penpot-handoff issue path is affected by this ticket.

Verification

Manual: create a fresh area:design issue, let designer run, let design-reviewer approve. Confirm the issue auto-closes and its linked impl dependent auto-assigns via the propagator.
Manual: same flow but with design-reviewer posting CHANGES_REQUESTED. Confirm the label flips back to area:design, designer re-runs, second pass approves, issue closes.
Skill-template lint (if any) stays green.

Out of scope

Automating reopen of a closed mockup issue when its impl dependent hits a design regression — separate concern.
Changing the dashboard-PR review path.
Closing issues from the designer skill directly (designer hands off to design-reviewer; only the reviewer holds the close decision).

References

skills/design-review.md — entry point for both the handoff-review and dashboard-PR-review flows. Penpot-handoff is the branch where get_pull_request_by_index returns 404.
skills/designer.md — handoff comment + label swap logic.
CLAUDE.md §"Design review flow" — current workflow documentation (needs a doc update with this ticket's close-on-approval semantics).
2026-04-21 incident: #223, #233, #238, #240 all finished but stayed open, #224 couldn't auto-close because of its dep on #223. Manually cleaned up in operator-issued closures.

## Bug The Penpot mockup workflow leaves the mockup issue **open forever** because neither the designer nor the design-reviewer skill closes it. Four tickets on 2026-04-21 (`#223`, `#233`, `#238`, `#240`) all finished their full design-review cycle but stayed `open` with no assignee — classic zombie state. Each blocked an impl dependent from auto-assigning. The propagator's `Forgejo refuses to close an issue whose /dependencies still has an open blocker` rule amplified this: `#224` (steering impl) couldn't even be manually closed because it was native-wired `blocked by #223`, and `#223` never closed itself. Five tickets had to be manually cleaned up to unstick the pool. ## Current workflow (per CLAUDE.md) 1. Operator creates issue, adds `area:design` label. 2. Designer skill runs: builds Penpot frames, posts handoff comment, swaps the label (removes `area:design`, adds `area:design-review`). 3. Design-reviewer skill runs on the re-labeled issue: exports frames as PNG, inspects multimodally, posts findings, **removes the `area:design-review` label**. 4. …issue stays `open` forever. The missing step: **nobody closes the issue**. ## Acceptance criteria ### Design-reviewer: close on approval - [ ] `skills/design-review.md` — when the review verdict is approval (no outstanding findings), the skill calls `mcp__forgejo__issue_state_change` with `state="closed"` at the end of its run, right after removing the `area:design-review` label. - [ ] When the verdict includes findings (changes requested), the skill **does not** close. Instead it posts the findings comment and *re-adds* `area:design` so the designer picks it back up. The label ping-pong mirrors the PR review loop. - [ ] Verdict is determined by a structured line in the review comment — e.g. `**Verdict**: APPROVED` or `**Verdict**: CHANGES_REQUESTED`. The skill emits one or the other explicitly so the logic is deterministic. ### Designer: idempotent re-runs - [ ] `skills/designer.md` — on a re-run triggered by `area:design` being re-added, the skill reads the prior design-reviewer findings (most recent `CHANGES_REQUESTED` comment), addresses them in Penpot, and re-posts a shorter handoff comment ("addressed: X, Y, Z") before swapping the label back to `area:design-review`. ### Operator override - [ ] If the operator wants to force-close a mockup issue regardless (e.g., scope changed, mockup abandoned), that stays a manual close via the Forgejo UI. No skill change needed — `mcp__forgejo__issue_state_change` already accepts that on the operator path. ### Dashboard PR review path — unchanged - [ ] The other design-reviewer entry point (PRs labeled `area:dashboard` or authored by `designer`) already submits `create_pull_review` APPROVED / REQUEST_CHANGES. The PR merge closes the linked issue — no change needed there. Only the Penpot-handoff issue path is affected by this ticket. ### Verification - [ ] Manual: create a fresh `area:design` issue, let designer run, let design-reviewer approve. Confirm the issue auto-closes and its linked impl dependent auto-assigns via the propagator. - [ ] Manual: same flow but with design-reviewer posting `CHANGES_REQUESTED`. Confirm the label flips back to `area:design`, designer re-runs, second pass approves, issue closes. - [ ] Skill-template lint (if any) stays green. ## Out of scope - Automating reopen of a closed mockup issue when its impl dependent hits a design regression — separate concern. - Changing the dashboard-PR review path. - Closing issues from the designer skill directly (designer hands off to design-reviewer; only the reviewer holds the close decision). ## References - `skills/design-review.md` — entry point for both the handoff-review and dashboard-PR-review flows. Penpot-handoff is the branch where `get_pull_request_by_index` returns 404. - `skills/designer.md` — handoff comment + label swap logic. - `CLAUDE.md` §"Design review flow" — current workflow documentation (needs a doc update with this ticket's close-on-approval semantics). - 2026-04-21 incident: `#223`, `#233`, `#238`, `#240` all finished but stayed open, `#224` couldn't auto-close because of its dep on `#223`. Manually cleaned up in operator-issued closures.

claude-desktop added the

area:agents

type:bug

labels

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