fix(shell): honor AbortSignal so --timeout cancels in practice (swamp-club#247)

integration/shell_timeout_test.ts

@@ -0,0 +1,127 @@
+// Swamp, an Automation Framework
+// Copyright (C) 2026 System Initiative, Inc.
+//
+// This file is part of Swamp.
+//
+// Swamp is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License version 3
+// as published by the Free Software Foundation, with the Swamp
+// Extension and Definition Exception (found in the "COPYING-EXCEPTION"
+// file).
+//
+// Swamp is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with Swamp.  If not, see <https://www.gnu.org/licenses/>.
+
+/**
+ * In-repo regression guard for swamp-club#247: built-in command/shell must
+ * honor `--timeout` end-to-end. Drives the CLI binary against a long-running
+ * shell command and asserts the run aborts well before the natural sleep
+ * duration. Catches regressions in PRs before swamp-uat picks them up on
+ * a separate release cadence.
+ */
+
+import { assertEquals } from "@std/assert";
+import { initializeTestRepo, runCliCommand } from "./test_helpers.ts";
+import { YamlDefinitionRepository } from "../src/infrastructure/persistence/yaml_definition_repository.ts";
+import { Definition } from "../src/domain/definitions/definition.ts";
+import { SHELL_MODEL_TYPE } from "../src/domain/models/command/shell/shell_model.ts";
+
+async function withTempDir(fn: (dir: string) => Promise<void>): Promise<void> {
+  const dir = await Deno.makeTempDir({ prefix: "swamp-shell-timeout-" });
+  try {
+    await fn(dir);
+  } finally {
+    if (Deno.build.os === "windows") {
+      await Deno.remove(dir, { recursive: true }).catch(() => {});
+    } else {
+      await Deno.remove(dir, { recursive: true });
+    }
+  }
+}
+
+Deno.test({
+  name:
+    "CLI: --timeout aborts a long-running command/shell run well before completion",
+  // Uses POSIX `sleep`. PowerShell variant via `Start-Sleep` is plausible
+  // but would need a separate model definition; defer until needed.
+  ignore: Deno.build.os === "windows",
+  fn: async () => {
+    await withTempDir(async (repoDir) => {
+      await initializeTestRepo(repoDir);
+
+      // Sleep 30s — picked so a working --timeout cannot be confused with
+      // natural completion under any reasonable CI jitter.
+      const definitionRepo = new YamlDefinitionRepository(repoDir);
+      const definition = Definition.create({
+        name: "long-sleep",
+        methods: {
+          execute: {
+            arguments: {
+              run: "sleep 30",
+              workingDir: "/tmp",
+            },
+          },
+        },
+      });
+      await definitionRepo.save(SHELL_MODEL_TYPE, definition);
+
+      const start = performance.now();
+      const result = await runCliCommand(
+        [
+          "model",
+          "method",
+          "run",
+          "long-sleep",
+          "execute",
+          "--repo-dir",
+          repoDir,
+          "--timeout",
+          "1s",
+          "--json",
+        ],
+        Deno.cwd(),
+      );
+      const elapsed = performance.now() - start;
+
+      // Non-zero exit confirms the run did not complete naturally.
+      assertEquals(
+        result.code !== 0,
+        true,
+        `expected non-zero exit on --timeout abort, got ${result.code}. ` +
+          `stdout: ${result.stdout}\nstderr: ${result.stderr}`,
+      );
+      // Generous 10s ceiling absorbs CI jitter and CLI startup time
+      // (worktree may need to compile fresh deps); still well below the
+      // 30s natural duration that would indicate the abort never fired.
+      assertEquals(
+        elapsed < 10_000,
+        true,
+        `expected --timeout to abort within 10s, took ${elapsed.toFixed(0)}ms`,
+      );
+      // The output should mention the abort/cancellation. The exact
+      // envelope shape (`code: "cancelled"` vs generic
+      // `method_execution_failed`) depends on libswamp's TimeoutError
+      // handling — match loosely so this guards the user-visible
+      // contract without pinning the wire format.
+      // The JSON envelope must carry `code: "cancelled"` so callers can
+      // distinguish a deadline abort from a generic execution failure.
+      // Without the driver-layer AbortError preservation, this surfaces
+      // as `method_execution_failed` and the contract regresses.
+      const envelope = JSON.parse(result.stdout) as {
+        code?: string;
+        error?: string;
+      };
+      assertEquals(
+        envelope.code,
+        "cancelled",
+        `expected envelope.code === "cancelled", got ${envelope.code}. ` +
+          `full stdout: ${result.stdout}`,
+      );
+    });
+  },
+});

src/cli/commands/model_method_run.ts

@@ -138,7 +138,7 @@ export const modelMethodRunCommand = new Command()
   )
   .option(
     "--timeout <duration:string>",
-    "Cancellation deadline — seconds (e.g. 30, 1800) or duration string (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.",
+    "Cancels the run when reached — seconds (e.g. 30, 1800) or duration string (e.g. 30s, 5m, 1h). Extension model methods must check ctx.signal to honor cancellation.",
   )
   .action(
     // @ts-expect-error - Cliffy custom type returns unknown instead of string

src/cli/commands/workflow_run.ts

@@ -129,7 +129,7 @@ export const workflowRunCommand = new Command()
   )
   .option(
     "--timeout <duration:string>",
-    "Cancellation deadline — seconds (e.g. 30, 1800) or duration string (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.",
+    "Cancels the run when reached — seconds (e.g. 30, 1800) or duration string (e.g. 30s, 5m, 1h). Extension model methods must check ctx.signal to honor cancellation.",
   )
   // @ts-expect-error - Cliffy custom type returns unknown instead of string
   .action(async function (options: AnyOptions, workflowIdOrName: string) {

src/domain/drivers/execution_driver.ts

@@ -89,6 +89,13 @@ export interface ExecutionResult {
   status: "success" | "error";
   /** Error message if status is "error". */
   error?: string;
+  /**
+   * True when the error originated from an aborted AbortSignal (e.g.
+   * --timeout fired). Lets the caller distinguish cancellation from a
+   * generic execution failure even though `error` flattens the exception
+   * to a string.
+   */
+  cancelled?: boolean;
   /** Outputs produced during execution. */
   outputs: DriverOutput[];
   /** Log lines captured during execution. */

src/domain/drivers/raw_execution_driver.ts

@@ -200,9 +200,12 @@ export class RawExecutionDriver implements ExecutionDriver {
         handle,
       }));
 
+      const cancelled = error instanceof DOMException &&
+        error.name === "AbortError";
       return {
         status: "error",
         error: error instanceof Error ? error.message : String(error),
+        cancelled,
         outputs,
         logs,
         durationMs,

src/domain/drivers/raw_execution_driver_test.ts

@@ -444,3 +444,61 @@ Deno.test("RawExecutionDriver: explicit queryData wins over dataQueryService der
   assertEquals(usedExplicit, true);
   assertEquals(usedDerived, false);
 });
+
+// AbortError preservation across the driver boundary — guards swamp-club#247.
+// The driver flattens exceptions to a string for the wire result, but it must
+// also set `cancelled: true` when the failure was an AbortError so callers
+// (method_execution_service) can re-throw an AbortError DOMException, which
+// libswamp's run handler routes to the `cancelled` envelope.
+
+Deno.test(
+  "RawExecutionDriver: marks result.cancelled=true when method throws AbortError",
+  async () => {
+    const executor: MethodExecutor = {
+      execute: () => {
+        throw new DOMException("The operation was aborted.", "AbortError");
+      },
+    };
+
+    const context = createMockContext();
+    const driver = new RawExecutionDriver(
+      executor,
+      testDefinition,
+      testMethod,
+      testModelDef,
+      context,
+      "test",
+    );
+
+    const result = await driver.execute(createMockRequest());
+
+    assertEquals(result.status, "error");
+    assertEquals(result.cancelled, true);
+  },
+);
+
+Deno.test(
+  "RawExecutionDriver: leaves result.cancelled falsy for non-abort errors",
+  async () => {
+    const executor: MethodExecutor = {
+      execute: () => {
+        throw new Error("boom");
+      },
+    };
+
+    const context = createMockContext();
+    const driver = new RawExecutionDriver(
+      executor,
+      testDefinition,
+      testMethod,
+      testModelDef,
+      context,
+      "test",
+    );
+
+    const result = await driver.execute(createMockRequest());
+
+    assertEquals(result.status, "error");
+    assertEquals(result.cancelled, false);
+  },
+);

src/domain/models/command/shell/shell_model.ts

@@ -125,6 +125,7 @@ async function executeCommand(
       timeoutMs: args.timeout,
       logger: context.logger,
       redactor: context.redactor,
+      signal: context.signal,
       onOutput: context.onEvent
         ? (line: string, stream: "stdout" | "stderr") =>
           context.onEvent!({ type: "output", line, stream })
@@ -136,6 +137,12 @@ async function executeCommand(
     exitCode = result.exitCode;
     durationMs = result.durationMs;
   } catch (error) {
+    // AbortError must escape ahead of the swallow paths below so that
+    // libswamp's run.ts handler can convert it to a `cancelled` envelope.
+    // Without this, --timeout aborts get buried as exit code -1.
+    if (error instanceof DOMException && error.name === "AbortError") {
+      throw error;
+    }
     // Handle execution errors (command not found, timeout, etc.)
     const rawError = error instanceof Error ? error.message : String(error);
     stderr = redact(rawError);

src/domain/models/command/shell/shell_model_test.ts

@@ -533,6 +533,75 @@ posixOnlyTest(
   },
 );
 
+// Abort signal propagation — guards swamp-club#247.
+// shell_model must surface AbortError ahead of its silent-swallow paths so
+// libswamp's run.ts can convert it to a `cancelled` envelope.
+
+posixOnlyTest(
+  "shellModel.methods.execute surfaces AbortError when ctx.signal aborts mid-run",
+  async () => {
+    const controller = new AbortController();
+    const args: ShellInputAttributes = { run: "sleep 5" };
+
+    const { context } = createTestContext({ signal: controller.signal });
+    setTimeout(() => controller.abort(), 100);
+
+    const caught = await assertRejects(() =>
+      shellModel.methods.execute.execute(args, context)
+    );
+    // Must be an AbortError — NOT the generic "Command exited with code -1"
+    // wrapper that the catch block swallows other errors into.
+    assertEquals(caught instanceof DOMException, true);
+    assertEquals((caught as DOMException).name, "AbortError");
+  },
+);
+
+posixOnlyTest(
+  "shellModel.methods.execute writes no data record on abort",
+  async () => {
+    const controller = new AbortController();
+    const args: ShellInputAttributes = { run: "sleep 5" };
+
+    const { context, getResults } = createTestContext({
+      signal: controller.signal,
+    });
+    setTimeout(() => controller.abort(), 100);
+
+    await assertRejects(() =>
+      shellModel.methods.execute.execute(args, context)
+    );
+
+    // AbortError must escape before context.writeResource is called, so no
+    // data artifacts should have been persisted for the aborted run.
+    assertEquals(getResults().length, 0);
+  },
+);
+
+posixOnlyTest(
+  "shellModel.methods.execute does not let ignoreExitCode swallow AbortError",
+  async () => {
+    // ignoreExitCode is a data-plane signal ("don't throw on non-zero exit").
+    // A timeout is a control-plane signal — the deadline elapsed. The two
+    // must not be conflated: even with ignoreExitCode=true, an aborted run
+    // must surface AbortError so the user sees a `cancelled` envelope, not
+    // a "successful" record with exit code 143.
+    const controller = new AbortController();
+    const args: ShellInputAttributes = {
+      run: "sleep 5",
+      ignoreExitCode: true,
+    };
+
+    const { context } = createTestContext({ signal: controller.signal });
+    setTimeout(() => controller.abort(), 100);
+
+    const caught = await assertRejects(() =>
+      shellModel.methods.execute.execute(args, context)
+    );
+    assertEquals(caught instanceof DOMException, true);
+    assertEquals((caught as DOMException).name, "AbortError");
+  },
+);
+
 Deno.test("ShellInputAttributesSchema rejects invalid attributes", () => {
   const result = ShellInputAttributesSchema.safeParse({ notRun: "value" });
   assertEquals(result.success, false);

src/domain/models/method_execution_service.ts

@@ -668,9 +668,16 @@ export class DefaultMethodExecutionService implements MethodExecutionService {
         currentHandles = await processDriverOutputs(driverResult.outputs);
 
         if (driverResult.status === "error") {
-          const err = new Error(
-            driverResult.error ?? "Method execution failed",
-          );
+          // Preserve AbortError type so libswamp's run handler routes it to
+          // the `cancelled` envelope. Drivers flatten exceptions to a string
+          // for the wire result, so use the explicit `cancelled` flag rather
+          // than string-matching the message.
+          const err: Error = driverResult.cancelled
+            ? new DOMException(
+              driverResult.error ?? "The operation was aborted.",
+              "AbortError",
+            )
+            : new Error(driverResult.error ?? "Method execution failed");
           (err as unknown as Record<string, unknown>).dataHandles =
             currentHandles;
           throw err;
@@ -724,6 +731,16 @@ export class DefaultMethodExecutionService implements MethodExecutionService {
           }));
 
         if (driverResult.status === "error") {
+          // Preserve AbortError type so libswamp's run handler routes it to
+          // the `cancelled` envelope. Drivers flatten exceptions to a string
+          // for the wire result, so use the explicit `cancelled` flag rather
+          // than string-matching the message.
+          if (driverResult.cancelled) {
+            throw new DOMException(
+              driverResult.error ?? "The operation was aborted.",
+              "AbortError",
+            );
+          }
           throw new Error(driverResult.error ?? "Driver execution failed");
         }