diff --git a/src/__tests__/loop.test.ts b/src/__tests__/loop.test.ts index 6355c99..37b0a35 100644 --- a/src/__tests__/loop.test.ts +++ b/src/__tests__/loop.test.ts @@ -85,6 +85,55 @@ const config: StormConfig = { github: { repo: "owner/repo", baseBranch: "main", label: "storm" }, }; +describe("processIssue — AbortSignal stops the loop", () => { + beforeEach(() => { + mockSpawnAgent.mockReset(); + mockRunChecks.mockReset(); + mockCheckoutBase.mockImplementation(async () => true); + mockCreateBranch.mockImplementation(async () => true); + mockCommitAndPush.mockImplementation(async () => true); + mockOpenPR.mockImplementation(async () => "https://github.com/owner/repo/pull/1"); + mockLoadContexts.mockImplementation(async () => new Map()); + mockLoadInstructions.mockImplementation(async () => new Map()); + mockDiscoverWorkflow.mockImplementation(async () => ({ body: "workflow", name: "workflow" })); + mockResolveTemplate.mockImplementation(() => "prompt"); + mockSpawnAgent.mockImplementation(async () => ({ done: true, exitCode: 0, usage: null, sessionId: null })); + mockRunChecks.mockImplementation(async () => ({ allPassed: true, failureSummary: "", results: [] })); + }); + + it("skips the agent loop when signal is already aborted", async () => { + const controller = new AbortController(); + controller.abort(); + + const result = await processIssue(issue, config, "/tmp", controller.signal); + expect(mockSpawnAgent).not.toHaveBeenCalled(); + // commit/push still runs after the loop + expect(mockCommitAndPush).toHaveBeenCalledTimes(1); + expect(result.success).toBe(true); + }); + + it("each call is independent — an aborted signal from one call does not affect the next", async () => { + const controller = new AbortController(); + controller.abort(); + + // First call with aborted signal — agent should not run + await processIssue(issue, config, "/tmp", controller.signal); + expect(mockSpawnAgent).not.toHaveBeenCalled(); + + mockSpawnAgent.mockReset(); + mockSpawnAgent.mockImplementation(async () => ({ done: true, exitCode: 0, usage: null, sessionId: null })); + mockCommitAndPush.mockReset(); + mockCommitAndPush.mockImplementation(async () => true); + mockOpenPR.mockReset(); + mockOpenPR.mockImplementation(async () => "https://github.com/owner/repo/pull/1"); + + // Second call with no signal — agent SHOULD run + const result = await processIssue(issue, config, "/tmp"); + expect(result.success).toBe(true); + expect(mockSpawnAgent).toHaveBeenCalledTimes(1); + }); +}); + describe("processIssue — final checks gate", () => { beforeEach(() => { mockSpawnAgent.mockReset(); diff --git a/src/commands/continue.ts b/src/commands/continue.ts index fae267d..67b3057 100644 --- a/src/commands/continue.ts +++ b/src/commands/continue.ts @@ -13,7 +13,6 @@ import { loadInstructions } from "../primitives/instructions.js"; import { runCommandArgs } from "../primitives/runner.js"; import { log } from "../core/output.js"; import { CONFIG_DIR } from "../core/constants.js"; -import { requestStop } from "../core/loop.js"; import type { PRReviewContext } from "../core/types.js"; import { existsSync } from "fs"; import { join } from "path"; @@ -39,9 +38,10 @@ export async function continueCommand( const { prNumber, dryRun = false } = options; // Handle SIGINT + const controller = new AbortController(); process.on("SIGINT", () => { log.warn("SIGINT received, finishing current work..."); - requestStop(); + controller.abort(); }); // Fetch PR details @@ -154,7 +154,7 @@ When done, output %%STORM_DONE%% on its own line. // Run the continue loop log.info(`Processing PR #${prNumber}...`); - const result = await processContinue(prContext, config, cwd); + const result = await processContinue(prContext, config, cwd, controller.signal); if (result.success) { log.success(`Successfully addressed review feedback on PR #${prNumber}`); diff --git a/src/commands/run.ts b/src/commands/run.ts index 4260e0f..74a9965 100644 --- a/src/commands/run.ts +++ b/src/commands/run.ts @@ -7,7 +7,7 @@ import { fetchPRReviews, fetchPRSessionId, } from "../core/github.js"; -import { processIssue, processIssueInWorktree, processContinue, requestStop } from "../core/loop.js"; +import { processIssue, processIssueInWorktree, processContinue } from "../core/loop.js"; import { log } from "../core/output.js"; import { CONFIG_DIR } from "../core/constants.js"; import { loadContexts } from "../primitives/context.js"; @@ -40,9 +40,10 @@ export async function runCommand( const { dryRun = false } = options; // Handle SIGINT + const controller = new AbortController(); process.on("SIGINT", () => { log.warn("SIGINT received, finishing current work..."); - requestStop(); + controller.abort(); }); let issues; @@ -125,9 +126,9 @@ export async function runCommand( issues.map(async (issue) => { const linked = await findLinkedPR(config.github.repo, issue.number); if (linked) { - return runContinueForLinkedPR(linked.number, issue, config, cwd); + return runContinueForLinkedPR(linked.number, issue, config, cwd, controller.signal); } - return processIssueInWorktree(issue, config, cwd); + return processIssueInWorktree(issue, config, cwd, controller.signal); }) ); @@ -150,9 +151,9 @@ export async function runCommand( const linked = await findLinkedPR(config.github.repo, issue.number); let result: { success: boolean }; if (linked) { - result = await runContinueForLinkedPR(linked.number, issue, config, cwd); + result = await runContinueForLinkedPR(linked.number, issue, config, cwd, controller.signal); } else { - result = await processIssue(issue, config, cwd); + result = await processIssue(issue, config, cwd, controller.signal); } if (result.success) { succeeded++; @@ -169,7 +170,8 @@ async function runContinueForLinkedPR( prNumber: number, issue: import("../core/types.js").GitHubIssue, config: import("../core/types.js").StormConfig, - cwd: string + cwd: string, + signal?: AbortSignal ): Promise<{ success: boolean }> { log.info(`Issue #${issue.number} has linked PR #${prNumber}, switching to continue flow`); @@ -196,5 +198,5 @@ async function runContinueForLinkedPR( sessionId, }; - return processContinue(prContext, config, cwd); + return processContinue(prContext, config, cwd, signal); } diff --git a/src/core/loop.test.ts b/src/core/loop.test.ts new file mode 100644 index 0000000..92952ff --- /dev/null +++ b/src/core/loop.test.ts @@ -0,0 +1,200 @@ +import { describe, it, expect, mock, beforeEach } from "bun:test"; + +// Mock all external dependencies before importing the module under test +const mockCheckoutBase = mock(() => Promise.resolve(true)); +const mockCreateBranch = mock(() => Promise.resolve(true)); +const mockCommitAndPush = mock(() => Promise.resolve(true)); +const mockOpenPR = mock(() => Promise.resolve("https://github.com/owner/repo/pull/1")); +const mockCheckoutExistingBranch = mock(() => Promise.resolve(true)); +const mockBranchName = mock((issue: { number: number }) => `storm/issue-${issue.number}`); +const mockSpawnAgent = mock(() => + Promise.resolve({ done: true, exitCode: 0, sessionId: "sess-1", usage: null, timedOut: false }) +); +const mockRunChecks = mock(() => + Promise.resolve({ allPassed: true, results: [], failureSummary: "" }) +); +const mockLoadContexts = mock(() => Promise.resolve(new Map())); +const mockLoadInstructions = mock(() => Promise.resolve(new Map())); +const mockDiscoverWorkflow = mock(() => + Promise.resolve({ body: "do the thing\n%%STORM_DONE%%\n", frontmatter: { completable: true } }) +); +const mockDiscoverContinueWorkflow = mock(() => Promise.resolve(null)); +const mockResolveTemplate = mock((tpl: string) => tpl); +const mockResolveContinueTemplate = mock((tpl: string) => tpl); +const mockCommentOnIssue = mock(() => Promise.resolve()); + +mock.module("./pr.js", () => ({ + checkoutBase: mockCheckoutBase, + createBranch: mockCreateBranch, + commitAndPush: mockCommitAndPush, + openPR: mockOpenPR, + checkoutExistingBranch: mockCheckoutExistingBranch, + mergeBaseBranch: mock(() => Promise.resolve({ merged: true })), + branchName: mockBranchName, +})); + +mock.module("./agent.js", () => ({ + spawnAgent: mockSpawnAgent, +})); + +mock.module("./checks.js", () => ({ + runChecks: mockRunChecks, +})); + +mock.module("../primitives/context.js", () => ({ + loadContexts: mockLoadContexts, +})); + +mock.module("../primitives/instructions.js", () => ({ + loadInstructions: mockLoadInstructions, +})); + +mock.module("../primitives/discovery.js", () => ({ + discoverWorkflow: mockDiscoverWorkflow, + discoverContinueWorkflow: mockDiscoverContinueWorkflow, +})); + +mock.module("./resolver.js", () => ({ + resolveTemplate: mockResolveTemplate, + resolveContinueTemplate: mockResolveContinueTemplate, +})); + +mock.module("./github.js", () => ({ + commentOnIssue: mockCommentOnIssue, +})); + +mock.module("./output.js", () => ({ + log: { + issue: mock(() => {}), + step: mock(() => {}), + warn: mock(() => {}), + error: mock(() => {}), + success: mock(() => {}), + info: mock(() => {}), + dim: mock(() => {}), + }, + formatDuration: mock(() => "1s"), +})); + +const { processIssue, processContinue } = await import("./loop.js"); + +import type { GitHubIssue, StormConfig, PRReviewContext } from "./types.js"; + +const baseIssue: GitHubIssue = { + number: 42, + title: "Test issue", + body: "Issue body", + labels: ["storm"], + url: "https://github.com/owner/repo/issues/42", +}; + +const baseConfig: StormConfig = { + github: { repo: "owner/repo", label: "storm", baseBranch: "main" }, + agent: { command: "claude", args: [], model: "sonnet" }, + defaults: { maxIterations: 5, delay: 0, stopOnError: false, parallel: false }, +}; + +const basePRContext: PRReviewContext = { + prNumber: 1, + prTitle: "Test PR", + prBody: "Closes #42", + prBranch: "storm/issue-42", + baseBranch: "main", + diffSummary: "+ some changes", + reviews: [], + linkedIssue: baseIssue, +}; + +beforeEach(() => { + mockSpawnAgent.mockReset(); + mockSpawnAgent.mockImplementation(() => + Promise.resolve({ done: true, exitCode: 0, sessionId: "sess-1", usage: null, timedOut: false }) + ); + mockCheckoutBase.mockReset(); + mockCheckoutBase.mockImplementation(() => Promise.resolve(true)); + mockCreateBranch.mockReset(); + mockCreateBranch.mockImplementation(() => Promise.resolve(true)); + mockCommitAndPush.mockReset(); + mockCommitAndPush.mockImplementation(() => Promise.resolve(true)); + mockOpenPR.mockReset(); + mockOpenPR.mockImplementation(() => Promise.resolve("https://github.com/owner/repo/pull/1")); + mockCheckoutExistingBranch.mockReset(); + mockCheckoutExistingBranch.mockImplementation(() => Promise.resolve(true)); +}); + +describe("processIssue", () => { + it("runs the agent when no signal is provided", async () => { + const result = await processIssue(baseIssue, baseConfig, "/tmp/test"); + expect(result.success).toBe(true); + expect(mockSpawnAgent).toHaveBeenCalledTimes(1); + }); + + it("skips the agent loop when signal is already aborted", async () => { + const controller = new AbortController(); + controller.abort(); + + const result = await processIssue(baseIssue, baseConfig, "/tmp/test", controller.signal); + // Loop body never executes, but commit/push still runs + expect(mockSpawnAgent).not.toHaveBeenCalled(); + // commitAndPush is called after the loop + expect(mockCommitAndPush).toHaveBeenCalledTimes(1); + }); + + it("each call is independent — a used signal from one call does not affect the next", async () => { + const controller = new AbortController(); + controller.abort(); + + // First call with aborted signal — agent should not run + await processIssue(baseIssue, baseConfig, "/tmp/test", controller.signal); + expect(mockSpawnAgent).not.toHaveBeenCalled(); + + mockSpawnAgent.mockReset(); + mockSpawnAgent.mockImplementation(() => + Promise.resolve({ done: true, exitCode: 0, sessionId: "sess-2", usage: null, timedOut: false }) + ); + + // Second call with no signal — agent SHOULD run + const result = await processIssue(baseIssue, baseConfig, "/tmp/test"); + expect(result.success).toBe(true); + expect(mockSpawnAgent).toHaveBeenCalledTimes(1); + }); + + it("returns success with prUrl on happy path", async () => { + const result = await processIssue(baseIssue, baseConfig, "/tmp/test"); + expect(result.success).toBe(true); + expect(result.prUrl).toBe("https://github.com/owner/repo/pull/1"); + }); +}); + +describe("processContinue", () => { + it("runs the agent when no signal is provided", async () => { + const result = await processContinue(basePRContext, baseConfig, "/tmp/test"); + expect(result.success).toBe(true); + expect(mockSpawnAgent).toHaveBeenCalledTimes(1); + }); + + it("skips the agent loop when signal is already aborted", async () => { + const controller = new AbortController(); + controller.abort(); + + await processContinue(basePRContext, baseConfig, "/tmp/test", controller.signal); + expect(mockSpawnAgent).not.toHaveBeenCalled(); + }); + + it("each call is independent — aborted signal from prior call does not affect next call", async () => { + const controller = new AbortController(); + controller.abort(); + + await processContinue(basePRContext, baseConfig, "/tmp/test", controller.signal); + expect(mockSpawnAgent).not.toHaveBeenCalled(); + + mockSpawnAgent.mockReset(); + mockSpawnAgent.mockImplementation(() => + Promise.resolve({ done: true, exitCode: 0, sessionId: "sess-2", usage: null, timedOut: false }) + ); + + const result = await processContinue(basePRContext, baseConfig, "/tmp/test"); + expect(result.success).toBe(true); + expect(mockSpawnAgent).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/core/loop.ts b/src/core/loop.ts index f2a8030..bd96ae9 100644 --- a/src/core/loop.ts +++ b/src/core/loop.ts @@ -9,16 +9,11 @@ import { runChecks } from "./checks.js"; import { commentOnIssue } from "./github.js"; import { branchName, createBranch, checkoutBase, commitAndPush, openPR, checkoutExistingBranch, mergeBaseBranch } from "./pr.js"; -let stopRequested = false; - -export function requestStop() { - stopRequested = true; -} - async function runIterationLoop( issue: GitHubIssue, config: StormConfig, - cwd: string + cwd: string, + signal?: AbortSignal ): Promise<{ success: boolean; totalUsage: AgentUsage; lastSessionId?: string }> { const { maxIterations, delay, stopOnError } = config.defaults; @@ -27,7 +22,7 @@ async function runIterationLoop( const totalUsage: AgentUsage = { inputTokens: 0, outputTokens: 0, cacheReadTokens: 0, cacheCreationTokens: 0 }; for (let iteration = 1; iteration <= maxIterations; iteration++) { - if (stopRequested) { + if (signal?.aborted) { log.warn("Stop requested, finishing up..."); break; } @@ -118,7 +113,8 @@ async function runIterationLoop( export async function processIssue( issue: GitHubIssue, config: StormConfig, - cwd: string + cwd: string, + signal?: AbortSignal ): Promise<{ success: boolean; prUrl?: string }> { const start = Date.now(); const branch = branchName(issue); @@ -133,7 +129,7 @@ export async function processIssue( return { success: false }; } - const { success, totalUsage, lastSessionId } = await runIterationLoop(issue, config, cwd); + const { success, totalUsage, lastSessionId } = await runIterationLoop(issue, config, cwd, signal); if (!success) return { success: false }; // Commit and push @@ -166,7 +162,8 @@ export async function processIssue( export async function processIssueInWorktree( issue: GitHubIssue, config: StormConfig, - baseCwd: string + baseCwd: string, + signal?: AbortSignal ): Promise<{ success: boolean; prUrl?: string }> { const worktreeDir = `${baseCwd}/.storm-worktrees/issue-${issue.number}`; const branch = branchName(issue); @@ -185,7 +182,7 @@ export async function processIssueInWorktree( try { // Run the loop in the worktree (skip checkoutBase/createBranch since worktree handles it) - return await processIssueInDir(issue, config, worktreeDir); + return await processIssueInDir(issue, config, worktreeDir, signal); } finally { // Cleanup worktree await runCommandArgs(["git", "worktree", "remove", worktreeDir, "--force"], { @@ -197,12 +194,13 @@ export async function processIssueInWorktree( async function processIssueInDir( issue: GitHubIssue, config: StormConfig, - cwd: string + cwd: string, + signal?: AbortSignal ): Promise<{ success: boolean; prUrl?: string }> { const start = Date.now(); const branch = branchName(issue); - const { success, totalUsage, lastSessionId } = await runIterationLoop(issue, config, cwd); + const { success, totalUsage, lastSessionId } = await runIterationLoop(issue, config, cwd, signal); if (!success) return { success: false }; // Commit and push @@ -259,7 +257,8 @@ When done, output %%STORM_DONE%% on its own line. export async function processContinue( pr: PRReviewContext, config: StormConfig, - cwd: string + cwd: string, + signal?: AbortSignal ): Promise<{ success: boolean }> { const start = Date.now(); const { maxIterations, delay, stopOnError } = config.defaults; @@ -284,7 +283,7 @@ export async function processContinue( const totalUsage: AgentUsage = { inputTokens: 0, outputTokens: 0, cacheReadTokens: 0, cacheCreationTokens: 0 }; for (let iteration = 1; iteration <= maxIterations; iteration++) { - if (stopRequested) { + if (signal?.aborted) { log.warn("Stop requested, finishing up..."); break; }