feat(flows-yaml): port PR-closed cleanup chain to YAML (pr-closed.yml) #1098

Merged

claude-desktop merged 1 commit from feat/flows-yaml-pr-closed into main 2026-05-11 01:13:44 +00:00

Conversation 1 Commits 1 Files changed 27 +1283 -213

claude-desktop commented 2026-05-11 00:52:03 +00:00

Collaborator

Copy link

Summary

Closes #1093.

Imperative handlePullRequestClosed in event-handlers.ts removed; the four side effects of pull_request.closed (branch cleanup, dep-row drop, post-merge rebase scan, pr.merged internal trigger publish) now live in flows/defaults/pr-closed.yml behind a new prClosed capability.

Capability + ops

• New PrClosedCapability in flows-yaml/types.ts; live + shadow bags wire it; LiveCapabilitiesDeps extended with prClosed
• 4 new ops wrapping the existing helpers:
  • cleanup_branch — wraps domain/workflow/cleanup.ts::cleanupBranch
  • drop_pr_dependencies — wraps db.ts::clearPrDependencies
  • handle_post_merge_rebase — wraps event-handlers.ts::handlePostMergeRebase (kept exported because the janitor unmergeable_pr_rebase rule also calls it)
  • publish_pr_merged — uses the live TriggerBus
• buildLiveCapabilitiesDeps now takes triggerBus so the live prClosed cap can publish without a module-level setter

Flow

• flows/defaults/pr-closed.yml on pull_request: [closed]: cleanup_branch → drop_pr_dependencies → (if merged) handle_post_merge_rebase → publish_pr_merged

Cleanup

• Delete handlePullRequestClosed from event-handlers.ts
• Delete the imperative call site at webhook.ts:333-335
• Delete tests for the removed handler (coverage moved to per-op tests + the new flow integration test in flows-yaml/defaults/__tests__/pr-closed.test.ts)
• Regenerated flows.schema.json (26 ops now)

Test plan

• 4 new op tests pass
• New pr-closed.yml flow test covers merged + unmerged paths
• Full server suite: 3252 pass / 0 fail
• CI green on this PR

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

## Summary Closes #1093. Imperative `handlePullRequestClosed` in `event-handlers.ts` removed; the four side effects of `pull_request.closed` (branch cleanup, dep-row drop, post-merge rebase scan, `pr.merged` internal trigger publish) now live in `flows/defaults/pr-closed.yml` behind a new `prClosed` capability. ### Capability + ops - New `PrClosedCapability` in `flows-yaml/types.ts`; live + shadow bags wire it; `LiveCapabilitiesDeps` extended with `prClosed` - 4 new ops wrapping the existing helpers: - `cleanup_branch` — wraps `domain/workflow/cleanup.ts::cleanupBranch` - `drop_pr_dependencies` — wraps `db.ts::clearPrDependencies` - `handle_post_merge_rebase` — wraps `event-handlers.ts::handlePostMergeRebase` (kept exported because the janitor `unmergeable_pr_rebase` rule also calls it) - `publish_pr_merged` — uses the live `TriggerBus` - `buildLiveCapabilitiesDeps` now takes `triggerBus` so the live `prClosed` cap can publish without a module-level setter ### Flow - `flows/defaults/pr-closed.yml` on `pull_request: [closed]`: cleanup_branch → drop_pr_dependencies → (if merged) handle_post_merge_rebase → publish_pr_merged ### Cleanup - Delete `handlePullRequestClosed` from `event-handlers.ts` - Delete the imperative call site at `webhook.ts:333-335` - Delete tests for the removed handler (coverage moved to per-op tests + the new flow integration test in `flows-yaml/defaults/__tests__/pr-closed.test.ts`) - Regenerated `flows.schema.json` (26 ops now) ## Test plan - [x] 4 new op tests pass - [x] New `pr-closed.yml` flow test covers merged + unmerged paths - [x] Full server suite: 3252 pass / 0 fail - [ ] CI green on this PR Co-Authored-By: Claude Opus 4.7 (1M context) &lt;noreply@anthropic.com&gt;

claude-desktop added 1 commit 2026-05-11 00:52:03 +00:00

feat(flows-yaml): port PR-closed cleanup chain to YAML (pr-closed.yml) ... a599679c6f

Closes #1093.

Imperative `handlePullRequestClosed` in `event-handlers.ts` removed; the
four side effects of `pull_request.closed` (branch cleanup, dep-row drop,
post-merge rebase scan, `pr.merged` internal trigger publish) now live in
`flows/defaults/pr-closed.yml` behind the new `prClosed` capability.

Capability + ops:
- new `PrClosedCapability` in `flows-yaml/types.ts`; live + shadow bags
  wire it; `LiveCapabilitiesDeps` extended with `prClosed`
- 4 new ops wrapping the existing helpers:
  `cleanup_branch` (wraps `domain/workflow/cleanup.ts::cleanupBranch`)
  `drop_pr_dependencies` (wraps `db.ts::clearPrDependencies`)
  `handle_post_merge_rebase` (wraps `event-handlers.ts::handlePostMergeRebase` —
   kept exported because the janitor `unmergeable_pr_rebase` rule also calls it)
  `publish_pr_merged` (uses the live `TriggerBus`)
- `buildLiveCapabilitiesDeps` now takes `triggerBus` so the live `prClosed`
  cap can publish without a module-level setter

Flow:
- `flows/defaults/pr-closed.yml` on `pull_request: [closed]`:
  cleanup_branch → drop_pr_dependencies →
  (if merged) handle_post_merge_rebase → publish_pr_merged

Cleanup:
- delete `handlePullRequestClosed` from `event-handlers.ts`
- delete the imperative call site at `webhook.ts:333-335`
- delete tests for the removed handler (coverage moved to per-op tests +
  the new flow integration test in
  `flows-yaml/defaults/__tests__/pr-closed.test.ts`)
- regenerated `flows.schema.json` (26 ops now)

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

claude-desktop requested review from reviewer 2026-05-11 00:52:19 +00:00

reviewer approved these changes 2026-05-11 00:57:00 +00:00

reviewer left a comment

Copy link

Review: APPROVE (CI pending — will merge when green)

Clean port. The four ops are correctly factored, the capability boundary is well-drawn, and the full cleanup of the imperative path is thorough. A few observations worth noting, none blocking.

---

AC verification

Item	Status
cleanup_branch op (repo, branch, pr_number)	✅
drop_pr_dependencies op (repo, pr_number)	✅
handle_post_merge_rebase op (repo, pr_number, base_ref)	✅ (not listed in AC Ops section but correctly identified as needed)
publish_pr_merged op (repo, pr_number); gated by if: event.pr.merged	✅
flows/defaults/pr-closed.yml on pull_request: [closed] with correct step order	✅
Unit tests for each op	✅ (4 × test + impl pairs)
Flow integration test: merged vs unmerged paths	✅
Verify pr-merged.yml cascade still fires	⚠️ see below
Delete handlePullRequestClosed from event-handlers.ts	✅
Delete call site webhook.ts:333-335	✅
Delete old tests	✅

---

Observations

1. pr-merged.yml cascade regression (AC item not explicitly covered)

The issue AC has:

[ ] Verify pr-merged.yml still fires after the switch (regression test for the cascade)

The pr-closed.yml flow test uses a recording stub for prClosed.publishPrMerged — it verifies the op is called, but doesn't wire a real trigger bus, so it can't assert pr-merged.yml fires in response. The AC item is implicitly covered if the existing pr-merged.yml tests pass as part of the 3252-test suite, but there's no dedicated end-to-end test for the chain pr-closed.yml → publishPrMerged → pr-merged.yml. Worth adding in a follow-up if the cascade ever misbehaves silently; acceptable as-is given the full suite pass.

2. Guard condition narrowing on handle_post_merge_rebase

Legacy code: if (pr.merged && pr.base?.ref) — both merged and base.ref had to be truthy before calling the rebase handler and publishing the trigger.

YAML flow: if: event.pr.merged — only merged is checked. The base_ref arg is required by the op schema, so if event.pr.baseRef ever evaluated to null/empty the step would fail validation rather than skip. In practice Forgejo always includes base.ref in pull_request.closed webhooks, so this is safe — just a subtle difference worth being aware of if the flow is reused in a context where baseRef might be absent.

3. Drop-count log removed

Old code logged pr-deps: cleared ${dropped} outgoing edge(s) when dropped > 0. The new dropPrDependencies adapter swallows the result silently on success (only warns on error). Low-stakes, but that log was occasionally useful for debugging stale dependency graphs. Could add a console.log in the success branch of the adapter for parity.

4. Error handling correctly moved to capability layer

All three mutating helpers (cleanupBranchHelper, handlePostMergeRebase, clearPrDependencies) are wrapped with try/catch in the main.ts adapter, matching the legacy "best-effort" semantics exactly. continue-on-error: true is correctly absent from the YAML steps — errors are already absorbed before they can propagate to the flow runner.

5. buildLiveCapabilitiesDeps refactor

Good call making triggerBus an explicit parameter instead of reading the module-level _triggerBus slot. This eliminates the only remaining reason for the slot to exist for the PR-closed path and makes the dependency graph explicit.

---

Merge plan

CI is pending — deferring merge to the post-CI gate. Will merge when green (squash) and close #1093.

## Review: APPROVE (CI pending — will merge when green) Clean port. The four ops are correctly factored, the capability boundary is well-drawn, and the full cleanup of the imperative path is thorough. A few observations worth noting, none blocking. --- ### AC verification | Item | Status | |---|---| | `cleanup_branch` op (repo, branch, pr_number) | ✅ | | `drop_pr_dependencies` op (repo, pr_number) | ✅ | | `handle_post_merge_rebase` op (repo, pr_number, base_ref) | ✅ (not listed in AC Ops section but correctly identified as needed) | | `publish_pr_merged` op (repo, pr_number); gated by `if: event.pr.merged` | ✅ | | `flows/defaults/pr-closed.yml` on `pull_request: [closed]` with correct step order | ✅ | | Unit tests for each op | ✅ (4 × test + impl pairs) | | Flow integration test: merged vs unmerged paths | ✅ | | Verify `pr-merged.yml` cascade still fires | ⚠️ see below | | Delete `handlePullRequestClosed` from `event-handlers.ts` | ✅ | | Delete call site `webhook.ts:333-335` | ✅ | | Delete old tests | ✅ | --- ### Observations **1. `pr-merged.yml` cascade regression (AC item not explicitly covered)** The issue AC has: > [ ] Verify `pr-merged.yml` still fires after the switch (regression test for the cascade) The `pr-closed.yml` flow test uses a recording stub for `prClosed.publishPrMerged` — it verifies the op is called, but doesn't wire a real trigger bus, so it can't assert `pr-merged.yml` fires in response. The AC item is implicitly covered if the existing `pr-merged.yml` tests pass as part of the 3252-test suite, but there's no dedicated end-to-end test for the chain `pr-closed.yml → publishPrMerged → pr-merged.yml`. Worth adding in a follow-up if the cascade ever misbehaves silently; acceptable as-is given the full suite pass. **2. Guard condition narrowing on `handle_post_merge_rebase`** Legacy code: `if (pr.merged && pr.base?.ref)` — both `merged` and `base.ref` had to be truthy before calling the rebase handler and publishing the trigger. YAML flow: `if: event.pr.merged` — only `merged` is checked. The `base_ref` arg is required by the op schema, so if `event.pr.baseRef` ever evaluated to null/empty the step would fail validation rather than skip. In practice Forgejo always includes `base.ref` in `pull_request.closed` webhooks, so this is safe — just a subtle difference worth being aware of if the flow is reused in a context where `baseRef` might be absent. **3. Drop-count log removed** Old code logged `pr-deps: cleared ${dropped} outgoing edge(s)` when `dropped > 0`. The new `dropPrDependencies` adapter swallows the result silently on success (only warns on error). Low-stakes, but that log was occasionally useful for debugging stale dependency graphs. Could add a `console.log` in the success branch of the adapter for parity. **4. Error handling correctly moved to capability layer** All three mutating helpers (`cleanupBranchHelper`, `handlePostMergeRebase`, `clearPrDependencies`) are wrapped with try/catch in the `main.ts` adapter, matching the legacy "best-effort" semantics exactly. `continue-on-error: true` is correctly absent from the YAML steps — errors are already absorbed before they can propagate to the flow runner. **5. `buildLiveCapabilitiesDeps` refactor** Good call making `triggerBus` an explicit parameter instead of reading the module-level `_triggerBus` slot. This eliminates the only remaining reason for the slot to exist for the PR-closed path and makes the dependency graph explicit. --- ### Merge plan CI is **pending** — deferring merge to the post-CI gate. Will merge when green (squash) and close #1093.

claude-desktop merged commit 605238a22a into main 2026-05-11 01:13:44 +00:00

claude-desktop referenced this pull request from a commit 2026-05-11 01:13:44 +00:00

feat(flows-yaml): port PR-closed cleanup chain to YAML (pr-closed.yml) (#1098)

claude-desktop deleted branch feat/flows-yaml-pr-closed 2026-05-11 01:13:44 +00:00

claude-desktop referenced this pull request 2026-05-11 03:04:12 +00:00

feat(nav-v2): drop top header, fold repo switcher + user card into side nav #1090

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!1098

No description provided.