From 7018d71ee6393c8c25cb2aef3100e19d77aab5bb Mon Sep 17 00:00:00 2001 From: Casper Panduro Date: Mon, 16 Mar 2026 17:14:06 +0100 Subject: [PATCH] storm: implement #24 - Global stopRequested flag is never reset between runs --- src/commands/continue.ts | 6 +- src/commands/run.ts | 9 +- src/core/loop.test.ts | 199 +++++++++++++++++++++++++++++++++++++++ src/core/loop.ts | 26 +++-- 4 files changed, 219 insertions(+), 21 deletions(-) create mode 100644 src/core/loop.test.ts diff --git a/src/commands/continue.ts b/src/commands/continue.ts index bc73f54..8686011 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 @@ -146,7 +146,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 a3c0514..74c3785 100644 --- a/src/commands/run.ts +++ b/src/commands/run.ts @@ -1,6 +1,6 @@ import { loadConfig, validateConfig } from "../core/config.js"; import { fetchLabeledIssues, fetchIssue } from "../core/github.js"; -import { processIssue, processIssueInWorktree, requestStop } from "../core/loop.js"; +import { processIssue, processIssueInWorktree } from "../core/loop.js"; import { log } from "../core/output.js"; import { CONFIG_DIR } from "../core/constants.js"; import { loadContexts } from "../primitives/context.js"; @@ -31,9 +31,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; @@ -81,7 +82,7 @@ export async function runCommand( // Parallel via worktrees log.info("Running in parallel with git worktrees"); const results = await Promise.allSettled( - issues.map((issue) => processIssueInWorktree(issue, config, cwd)) + issues.map((issue) => processIssueInWorktree(issue, config, cwd, controller.signal)) ); let succeeded = 0; @@ -100,7 +101,7 @@ export async function runCommand( let succeeded = 0; let failed = 0; for (const issue of issues) { - const result = await processIssue(issue, config, cwd); + const result = await processIssue(issue, config, cwd, controller.signal); if (result.success) { succeeded++; } else { diff --git a/src/core/loop.test.ts b/src/core/loop.test.ts new file mode 100644 index 0000000..fd28022 --- /dev/null +++ b/src/core/loop.test.ts @@ -0,0 +1,199 @@ +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, + 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 76ab355..b3506d0 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 } from "./pr.js"; -let stopRequested = false; - -export function requestStop() { - stopRequested = true; -} - 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); @@ -39,7 +34,7 @@ export async function processIssue( 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; } @@ -146,7 +141,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); @@ -165,7 +161,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 runCommand(`git worktree remove "${worktreeDir}" --force`, { @@ -177,7 +173,8 @@ 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); @@ -188,7 +185,7 @@ async function processIssueInDir( const totalUsage: AgentUsage = { inputTokens: 0, outputTokens: 0, cacheReadTokens: 0, cacheCreationTokens: 0 }; for (let iteration = 1; iteration <= maxIterations; iteration++) { - if (stopRequested) break; + if (signal?.aborted) break; log.issue(issue.number, `Iteration ${iteration}/${maxIterations}`); const iterStart = Date.now(); @@ -296,7 +293,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; @@ -315,7 +313,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; }