feat(webhook): handle issues.closed and pull_request.closed for cleanup #12

Closed

claude-desktop wants to merge 0 commits from dev/4 into main

claude-desktop commented

2026-04-16 23:22:03 +00:00

Collaborator

Summary

Add cleanup.ts stub module with cleanupIssue and cleanupBranch functions
Wire issues event closed action to call cleanupIssue per registered agent
Wire pull_request event closed action to call cleanupBranch (both merged and unmerged)
issues.reopened is intentionally a no-op
Add webhook.test.ts with synthetic payload tests covering all three scenarios

Closes #4

## Summary - Add `cleanup.ts` stub module with `cleanupIssue` and `cleanupBranch` functions - Wire `issues` event `closed` action to call `cleanupIssue` per registered agent - Wire `pull_request` event `closed` action to call `cleanupBranch` (both merged and unmerged) - `issues.reopened` is intentionally a no-op - Add `webhook.test.ts` with synthetic payload tests covering all three scenarios Closes #4

dev was assigned by claude-desktop

2026-04-16 23:22:03 +00:00

claude-desktop requested review from reviewer

2026-04-16 23:22:28 +00:00

claude-desktop commented

2026-04-16 23:27:12 +00:00

Author

Collaborator

Code Review (REQUEST_CHANGES)

2 issues need fixing before merge.

The webhook routing and cleanup stub are correctly structured, and the test coverage for the main scenarios (issues.closed, pull_request.closed merged/unmerged) is solid. But there is a forward-compatibility bug in the cleanup dispatch loop, and the cleanup-stub tests are silently testing the mock instead of the real module.

🔴 `src/webhook.ts` line 237 — Bug: `agentName` in scope but never passed to `cleanupIssue`

The loop iterates over agents and agentName is bound, yet every call is identical:

cleanupIssue(repo, issueNumber);  // agentName dropped on the floor

When the workdir module lands, cleanup will need to know which agent's worktree to release (each agent has its own branch/worktree). The stub needs the agent parameter now so the signature is already correct when the TODO is filled in.

Fix: add agentName to the signature of both cleanupIssue and cleanupBranch, then pass it at the call sites:

// cleanup.ts
export function cleanupIssue(repo: string, issueNumber: number, agentName: string, _opts?: CleanupOpts): void

// webhook.ts – handleIssueClosed
for (const agentName of Object.keys(config.agents)) {
    cleanupIssue(repo, issueNumber, agentName);
}

handlePullRequestClosed should similarly loop per agent and pass agentName.

🔴 `src/webhook.test.ts` lines 132–142 — Bug: "cleanup stubs" tests exercise the mock, not `cleanup.ts`

mock.module("./cleanup", ...) replaces the module in Bun's registry at line 14. When lines 134 and 139 call await import("./cleanup") they receive the spy — not the real implementation. The assertions (not.toThrow()) are trivially true because the spy is just () => calls.push(...).

Fix: move these two tests to a new file src/cleanup.test.ts that imports directly from ./cleanup without any mocking:

// src/cleanup.test.ts
import { describe, expect, test } from "bun:test";
import { cleanupBranch, cleanupIssue } from "./cleanup";

describe("cleanup stubs", () => {
    test("cleanupIssue logs and does not throw", () => {
        expect(() => cleanupIssue("charles/repo", 42, "forge-veteran")).not.toThrow();
    });
    test("cleanupBranch logs and does not throw", () => {
        expect(() => cleanupBranch("charles/repo", "dev/42", "forge-veteran")).not.toThrow();
    });
});

(Adjust signature once the agentName parameter is added per the comment above.)

🟡 `src/webhook.test.ts` line 75 — Minor: use `toBe(1)` not `toBeGreaterThanOrEqual(1)`

TEST_CONFIG has exactly one agent, so the assertion should be exact:

expect(cleanupIssueCalls.length).toBe(1);

toBeGreaterThanOrEqual(1) would silently pass even if a bug caused the loop to fire twice per event.

## Code Review (REQUEST_CHANGES) **2 issues need fixing before merge.** The webhook routing and cleanup stub are correctly structured, and the test coverage for the main scenarios (issues.closed, pull_request.closed merged/unmerged) is solid. But there is a forward-compatibility bug in the cleanup dispatch loop, and the cleanup-stub tests are silently testing the mock instead of the real module. --- ### 🔴 `src/webhook.ts` line 237 — Bug: `agentName` in scope but never passed to `cleanupIssue` The loop iterates over agents and `agentName` is bound, yet every call is identical: ```ts cleanupIssue(repo, issueNumber); // agentName dropped on the floor ``` When the workdir module lands, cleanup will need to know *which* agent's worktree to release (each agent has its own branch/worktree). The stub needs the agent parameter now so the signature is already correct when the TODO is filled in. **Fix:** add `agentName` to the signature of both `cleanupIssue` and `cleanupBranch`, then pass it at the call sites: ```ts // cleanup.ts export function cleanupIssue(repo: string, issueNumber: number, agentName: string, _opts?: CleanupOpts): void // webhook.ts – handleIssueClosed for (const agentName of Object.keys(config.agents)) { cleanupIssue(repo, issueNumber, agentName); } ``` `handlePullRequestClosed` should similarly loop per agent and pass `agentName`. --- ### 🔴 `src/webhook.test.ts` lines 132–142 — Bug: "cleanup stubs" tests exercise the mock, not `cleanup.ts` `mock.module("./cleanup", ...)` replaces the module in Bun's registry at line 14. When lines 134 and 139 call `await import("./cleanup")` they receive the spy — not the real implementation. The assertions (`not.toThrow()`) are trivially true because the spy is just `() => calls.push(...)`. **Fix:** move these two tests to a new file `src/cleanup.test.ts` that imports directly from `./cleanup` without any mocking: ```ts // src/cleanup.test.ts import { describe, expect, test } from "bun:test"; import { cleanupBranch, cleanupIssue } from "./cleanup"; describe("cleanup stubs", () => { test("cleanupIssue logs and does not throw", () => { expect(() => cleanupIssue("charles/repo", 42, "forge-veteran")).not.toThrow(); }); test("cleanupBranch logs and does not throw", () => { expect(() => cleanupBranch("charles/repo", "dev/42", "forge-veteran")).not.toThrow(); }); }); ``` (Adjust signature once the `agentName` parameter is added per the comment above.) --- ### 🟡 `src/webhook.test.ts` line 75 — Minor: use `toBe(1)` not `toBeGreaterThanOrEqual(1)` `TEST_CONFIG` has exactly one agent, so the assertion should be exact: ```ts expect(cleanupIssueCalls.length).toBe(1); ``` `toBeGreaterThanOrEqual(1)` would silently pass even if a bug caused the loop to fire twice per event.

claude-desktop referenced this pull request

2026-04-16 23:35:23 +00:00

feat(worker): swap mkdtemp+clone for persistent worktree #14

charles force-pushed dev/4 from 980717b602

qa / qa (pull_request) Successful in 42s

Details

to ef7e198281

qa / qa (pull_request) Successful in 43s

Details

2026-04-17 09:07:34 +00:00

Compare