refactor(architect): remove /breakdown webhook, architect creates issues directly #1224

Merged

charles merged 2 commits from fix/render-skill-address-review into main 2026-05-16 09:19:45 +00:00

Conversation 1 Commits 2 Files changed 33 +51 -1561

claude-desktop commented 2026-05-15 21:52:50 +00:00

Collaborator

Copy link

Summary

• Retire the /breakdown slash-command webhook path (skill file, flow YAML, dispatch_breakdown op, POST /api/breakdown route, all capability wiring, ~1500 lines deleted)
• Fix stale ARCHITECT_INTRO system prompt — was claiming host-process / process.cwd() access; architect runs in a Docker container since #1133
• Add write_file, list_dir, create_label, list_repo_milestones, create_milestone to FORGE_TOOLS_ALLOWLIST so architect can write spec files and create issues autonomously
• Rewrite ARCHITECT_INTRO with accurate container context + inline issue-authoring rules (dedup, label validation, native dependency registration via mcp__forge__add_blocker)

Test plan

• just qa — 3368 server tests pass, forge-mcp pass
• Architect chat: ask to write a spec → should use mcp__forge__write_file, not Write tool
• Architect chat: ask to create issues from spec → should call mcp__forge__create_issue directly, register blockers via mcp__forge__add_blocker

🤖 Generated with Claude Code

## Summary - Retire the `/breakdown` slash-command webhook path (skill file, flow YAML, `dispatch_breakdown` op, `POST /api/breakdown` route, all capability wiring, ~1500 lines deleted) - Fix stale `ARCHITECT_INTRO` system prompt — was claiming host-process / `process.cwd()` access; architect runs in a Docker container since #1133 - Add `write_file`, `list_dir`, `create_label`, `list_repo_milestones`, `create_milestone` to `FORGE_TOOLS_ALLOWLIST` so architect can write spec files and create issues autonomously - Rewrite `ARCHITECT_INTRO` with accurate container context + inline issue-authoring rules (dedup, label validation, native dependency registration via `mcp__forge__add_blocker`) ## Test plan - [ ] `just qa` — 3368 server tests pass, forge-mcp pass - [ ] Architect chat: ask to write a spec → should use `mcp__forge__write_file`, not `Write` tool - [ ] Architect chat: ask to create issues from spec → should call `mcp__forge__create_issue` directly, register blockers via `mcp__forge__add_blocker` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-desktop added the

area:agents

label 2026-05-15 21:52:50 +00:00

claude-desktop added 3 commits 2026-05-15 21:52:50 +00:00

fix(flows): render address-review skill before dispatching author ... 20eb55dc4e

The review-changes-requested.yml flow was dispatching with
task: event.pr.title (bare PR title) — the agent received no
instruction to read review comments or invoke the address-review
skill. It would re-implement from scratch instead of fixing feedback.

Add render_skill op (RenderSkillCapability) that calls renderPrompt
against the author's agent type + skill name. The flow now renders
the address-review template ({{pr_number}}, {{title}} interpolated)
before dispatch. Falls back gracefully when skill is missing (found:
false gate skips dispatch).

Wire render_skill into live, shadow, and test capability bags.
Regenerate flows.schema.json (33 ops → 32+1).

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

fix(web): sessionStorage-based back navigation + update stale test ... bc0c462363

Replace window.history.back() in BackToMainNav with sessionStorage
lookup: remember the last non-settings path via useEffect and navigate
back to it on click (falling back to /board). Sidebar test updated
to assert history.back is NOT called (new behaviour), not that it is.

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

refactor(architect): remove /breakdown webhook, architect creates issues directly ... b163cc2439

- Delete skills/breakdown.md, flows/defaults/breakdown-comment.yml,
  dispatch_breakdown op, POST /api/breakdown route, and all related
  capability wiring — the /breakdown slash-command path is retired.
- Add write_file, list_dir, create_label, list_repo_milestones,
  create_milestone to FORGE_TOOLS_ALLOWLIST so architect can write
  spec files and create issues autonomously.
- Rewrite ARCHITECT_INTRO: runs in container (not host process), no
  local FS access, use forge MCP for all file and issue operations,
  inline dedup + label-validation + dependency-registration rules.

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

charles requested review from reviewer 2026-05-15 21:53:26 +00:00

reviewer requested changes 2026-05-15 21:57:49 +00:00

reviewer left a comment

Copy link

The overall direction is correct — the ARCHITECT_INTRO was materially wrong and the breakdown infrastructure was a rube goldberg machine. The cleanup is thorough and the new render_skill op is well-structured. Two issues need fixing before this lands.

---

🔴 Blocker 1 — architectProof fails for issue-creation-only sessions

File: apps/server/src/domain/dispatch/completion-proof.ts

The proof was updated from:

(body) => body.includes("Dispatched") || body.includes("Broken down") || body.includes("Skill:")

to:

(body) => body.includes("Dispatched") || body.includes("Skill:")

Removing "Broken down" is correct — that marker was posted by the breakdown flow, which is gone. But the new workflow creates a new gap: when the operator asks architect to create issues from a spec, it will call mcp__forge__create_issue N times, post a summary comment like "Created issues #100–#110", then stop. That summary comment contains neither "Dispatched" nor "Skill:", so architectProof returns passed: false and the task is logged as a misroute.

The old /breakdown path avoided this gap because the breakdown flow (not architect directly) posted the "Broken down" proof marker. The new direct-creation path has no equivalent.

Fix options:

1. Add a third marker: body.includes("Created") — and update the new ARCHITECT_INTRO to instruct architect to post a comment like "Created N issues: …" (which it would do naturally as a summary). The proof comment content: "Created \d+ issue" or simply "Created issue".
2. Add a new check alongside the existing ones: body.includes("Created issue").
3. Update the ARCHITECT_INTRO to explicitly require architect to post a comment starting with "Dispatched" after any work session (spec-write, issue-create, or task-dispatch), making the proof requirement explicit in the system prompt.

Option 1 is the least invasive. The proof failure is a real operational regression — tasks that succeed functionally will be logged as incomplete, polluting the task dashboard and potentially triggering unwanted re-dispatches.

---

🟡 Blocker 2 — Verify add_blocker (and create_issue / list_issues) are in FORGE_TOOLS_ALLOWLIST

File: apps/server/src/domain/agent/mcp-config.ts

The new ARCHITECT_INTRO explicitly instructs:

"call mcp__forge__add_blocker for each pair — this is what drives auto-assign automation"
"deduplicate against existing issues via mcp__forge__list_issues"
"create issues directly via mcp__forge__create_issue"

The diff adds write_file, list_dir, create_label, list_repo_milestones, create_milestone — all correct. But create_issue, list_issues, and add_blocker are not in the additions. Since the previous architect workflow never called these directly (breakdown dispatched code-lead to do it), they may not have been needed in the allowlist before.

create_issue and list_issues are almost certainly in the existing allowlist (dev/reviewer use them). add_blocker is less certain — it may have been used only inside the breakdown skill, not by agents calling the allowlisted tools directly.

Action required: Confirm add_blocker is in FORGE_TOOLS_ALLOWLIST and add it if not. A quick grep -n "add_blocker" apps/server/src/domain/agent/mcp-config.ts will confirm.

---

🔵 Minor — render_skill + review-changes-requested.yml: silent skip on missing skill

File: flows/defaults/review-changes-requested.yml

The dispatch step now gates on steps.skill.outputs.found. If the address-review skill template isn't seeded for an agent, the dispatch is silently skipped — no task created, no error surfaced. The old behavior used event.pr.title as a fallback, which was weak context but at least guaranteed a dispatch.

This is probably intentional (dispatching with just a PR title as task is bad for the agent), but it's a silent behavior change. If a dev agent is misconfigured and missing the skill template, changes-requested reviews will stop triggering address-review tasks with no observable error.

Recommend adding a comment step after the dispatch step that fires when !steps.skill.outputs.found to surface the failure visibly (even just posting "⚠️ address-review skill not found for agent X — dispatch skipped" on the PR).

---

🔵 Minor — unrelated changes bundled

Files: apps/web/src/components/nav-sections.tsx, apps/web/src/components/sidebar-nav.test.tsx

The sessionStorage back-navigation fix is completely orthogonal to the architect refactor. It should ship in its own PR so blame history stays clean and it can be reverted independently if needed. Not a blocker, but worth separating.

---

✅ What's solid

• ARCHITECT_INTRO: Major improvement. The old prompt was factually wrong (claimed host filesystem via process.cwd(), mentioned POST /breakdown). The new one correctly describes the container context and gives precise MCP tool instructions.
• Cleanup coverage: All breakdown references removed consistently — types, capabilities, ops, flows, skills, tests, docs. No stale references spotted.
• render_skill op: Well-designed, correct interface split between RenderSkillCapability (object input) and RenderSkillAdapterDeps (positional). Tests cover found/not-found/empty-vars paths.
• forgejo-api.ts POST vs PUT fix: Real correctness fix — new files need POST (no SHA), existing files need PUT. The error logging addition in the adapter is also welcome.
• shadow-capabilities.ts: Correctly falls through to live deps for renderSkill (read-only) rather than stubbing it.
• CI is currently pending — hold merge until green regardless.

Fix the proof marker and confirm the allowlist, then this is good to go.

The overall direction is correct — the ARCHITECT_INTRO was materially wrong and the breakdown infrastructure was a rube goldberg machine. The cleanup is thorough and the new `render_skill` op is well-structured. Two issues need fixing before this lands. --- ## 🔴 Blocker 1 — `architectProof` fails for issue-creation-only sessions **File:** `apps/server/src/domain/dispatch/completion-proof.ts` The proof was updated from: ```ts (body) => body.includes("Dispatched") || body.includes("Broken down") || body.includes("Skill:") ``` to: ```ts (body) => body.includes("Dispatched") || body.includes("Skill:") ``` Removing "Broken down" is correct — that marker was posted by the breakdown flow, which is gone. But the new workflow creates a new gap: when the operator asks architect to *create issues from a spec*, it will call `mcp__forge__create_issue` N times, post a summary comment like "Created issues #100–#110", then stop. That summary comment contains neither "Dispatched" nor "Skill:", so `architectProof` returns `passed: false` and the task is logged as a misroute. The old `/breakdown` path avoided this gap because the breakdown *flow* (not architect directly) posted the "Broken down" proof marker. The new direct-creation path has no equivalent. **Fix options:** 1. Add a third marker: `body.includes("Created")` — and update the new ARCHITECT_INTRO to instruct architect to post a comment like "Created N issues: …" (which it would do naturally as a summary). The proof comment content: `"Created \d+ issue"` or simply `"Created issue"`. 2. Add a new check alongside the existing ones: `body.includes("Created issue")`. 3. Update the ARCHITECT_INTRO to explicitly require architect to post a comment starting with "Dispatched" after any work session (spec-write, issue-create, or task-dispatch), making the proof requirement explicit in the system prompt. Option 1 is the least invasive. The proof failure is a real operational regression — tasks that succeed functionally will be logged as incomplete, polluting the task dashboard and potentially triggering unwanted re-dispatches. --- ## 🟡 Blocker 2 — Verify `add_blocker` (and `create_issue` / `list_issues`) are in `FORGE_TOOLS_ALLOWLIST` **File:** `apps/server/src/domain/agent/mcp-config.ts` The new ARCHITECT_INTRO explicitly instructs: > "call `mcp__forge__add_blocker` for each pair — this is what drives auto-assign automation" > "deduplicate against existing issues via `mcp__forge__list_issues`" > "create issues directly via `mcp__forge__create_issue`" The diff adds `write_file`, `list_dir`, `create_label`, `list_repo_milestones`, `create_milestone` — all correct. But `create_issue`, `list_issues`, and `add_blocker` are not in the additions. Since the previous architect workflow never called these directly (breakdown dispatched code-lead to do it), they may not have been needed in the allowlist before. `create_issue` and `list_issues` are almost certainly in the existing allowlist (dev/reviewer use them). `add_blocker` is less certain — it may have been used only inside the breakdown skill, not by agents calling the allowlisted tools directly. **Action required:** Confirm `add_blocker` is in `FORGE_TOOLS_ALLOWLIST` and add it if not. A quick `grep -n "add_blocker" apps/server/src/domain/agent/mcp-config.ts` will confirm. --- ## 🔵 Minor — `render_skill` + `review-changes-requested.yml`: silent skip on missing skill **File:** `flows/defaults/review-changes-requested.yml` The dispatch step now gates on `steps.skill.outputs.found`. If the `address-review` skill template isn't seeded for an agent, the dispatch is silently skipped — no task created, no error surfaced. The old behavior used `event.pr.title` as a fallback, which was weak context but at least guaranteed a dispatch. This is probably intentional (dispatching with just a PR title as task is bad for the agent), but it's a silent behavior change. If a dev agent is misconfigured and missing the skill template, changes-requested reviews will stop triggering address-review tasks with no observable error. Recommend adding a `comment` step after the `dispatch` step that fires when `!steps.skill.outputs.found` to surface the failure visibly (even just posting "⚠️ address-review skill not found for agent X — dispatch skipped" on the PR). --- ## 🔵 Minor — unrelated changes bundled **Files:** `apps/web/src/components/nav-sections.tsx`, `apps/web/src/components/sidebar-nav.test.tsx` The sessionStorage back-navigation fix is completely orthogonal to the architect refactor. It should ship in its own PR so blame history stays clean and it can be reverted independently if needed. Not a blocker, but worth separating. --- ## ✅ What's solid - **ARCHITECT_INTRO**: Major improvement. The old prompt was factually wrong (claimed host filesystem via `process.cwd()`, mentioned `POST /breakdown`). The new one correctly describes the container context and gives precise MCP tool instructions. - **Cleanup coverage**: All breakdown references removed consistently — types, capabilities, ops, flows, skills, tests, docs. No stale references spotted. - **`render_skill` op**: Well-designed, correct interface split between `RenderSkillCapability` (object input) and `RenderSkillAdapterDeps` (positional). Tests cover found/not-found/empty-vars paths. - **`forgejo-api.ts` POST vs PUT fix**: Real correctness fix — new files need `POST` (no SHA), existing files need `PUT`. The error logging addition in the adapter is also welcome. - **`shadow-capabilities.ts`**: Correctly falls through to live deps for `renderSkill` (read-only) rather than stubbing it. - CI is currently pending — hold merge until green regardless. Fix the proof marker and confirm the allowlist, then this is good to go.

charles force-pushed fix/render-skill-address-review from b163cc2439 to addfa787f7 2026-05-15 22:02:53 +00:00 Compare

charles merged commit 8e41f4ec30 into main 2026-05-16 09:19:45 +00:00

charles deleted branch fix/render-skill-address-review 2026-05-16 09:19:46 +00:00

charles referenced this pull request from a commit 2026-05-16 09:19:47 +00:00

refactor(architect): remove /breakdown webhook, architect creates issues directly (#1224)

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

3 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/agent-hooks!1224

No description provided.