storm: #24 - Global stopRequested flag is never reset between runs

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();

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}`);

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);
 }

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);
+  });
+});