From be311d1d8d799f0aba8b8436a37fb7a9e1989712 Mon Sep 17 00:00:00 2001 From: stack72 Date: Tue, 5 May 2026 13:52:10 +0100 Subject: [PATCH 1/3] =?UTF-8?q?feat(cli):=20bundle=20agentic-CLI=20improve?= =?UTF-8?q?ments=20=E2=80=94=20JSON=20isolation,=20--input=20:json,=20--ti?= =?UTF-8?q?meout,=20--repo-dir=20parity=20(swamp-club#235)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundle 6 agentic-CLI ergonomics fixes (#235): 1. JSON-mode logger isolation — set parentSinks: 'override' on ["model","method","run"], ["workflow","run"], and ["logtape","meta"] category loggers in jsonMode so they cannot inherit the root sink. Clear logtape.meta's own sinks too. Closes the leak path identified in #235. 2. Single-emitter renderError — in jsonMode, write JSON to stdout exactly once and skip logger.fatal. Delete createJsonErrorSink (audit confirmed only 2 logger.fatal calls in src/, both inside renderError). Extend buildErrorJson with optional `code` field; UserError gains a code parameter. Updated existing integration tests to assert against the combined stdout+stderr stream rather than stderr alone. 3. --input :json suffix — keys ending :json (on the leaf segment of any dot-notation path) parse the value as JSON. Bypasses the @file shorthand. Hard error on parse failure. Test matrix covers flat array/object, nested keys, escape interactions, no-suffix fallback, parse failures, and precedence with --input-file. 4. --repo-dir on `model type search` and top-level `doctor` — closes the "Unknown option --repo-dir" failure mode for agentic flows that pass --repo-dir uniformly. 5. data gc --json — already non-interactive (gate at outputMode === 'log'); add an explicit JSON example to help text so agents can discover the bypass without reading source. 6. --timeout flag for `model method run` and `workflow run` — wires AbortController via libswamp's existing LibSwampContext.withTimeout(ms). Reuses libswamp's parseDuration; adds parseTimeout helper for second- level granularity. Surfaces SwampError code "cancelled" via the new buildErrorJson code field. Documented as cooperative cancellation — built-in models that don't honor AbortSignal are tracked separately (swamp-club#247). Design docs: - design/rendering.md gains a normative "JSON Mode Output Contract" section with four guarantees: stdout = single JSON document; stderr free for logs; errors emit { error, stack?, code? } on stdout with non-zero exit; no interactive prompts in JSON mode. - design/inputs.md gains a :json suffix section with examples. - swamp-model and swamp-workflow skills updated with :json examples. Regression test: - integration/json_isolation_test.ts asserts BOTH directions across version, data list, and model method run: --json stdout-only-JSON; log mode still emits. Out of scope, tracked separately: - swamp-club#246 — Reader-lock or lock-free read path for data list/get/search/query (original #235 item 5). - swamp-club#247 — Built-in models must honor AbortSignal so --timeout works in practice. - swamp-uat#185 — UAT (adversarial): --timeout cancellation must release model locks. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/skills/swamp-model/SKILL.md | 3 +- .claude/skills/swamp-workflow/SKILL.md | 3 +- design/inputs.md | 30 ++- design/rendering.md | 39 ++++ integration/data_output_flow_test.ts | 6 +- integration/driver_resolution_test.ts | 5 +- integration/foreach_test.ts | 2 +- integration/input_override_validation_test.ts | 2 +- integration/inputs_test.ts | 16 +- integration/json_isolation_test.ts | 181 ++++++++++++++++++ integration/model_validate_test.ts | 4 +- integration/workflow_test.ts | 16 +- src/cli/commands/data_gc.ts | 4 + src/cli/commands/data_gc_test.ts | 9 + src/cli/commands/doctor.ts | 7 + src/cli/commands/model_method_run.ts | 13 +- src/cli/commands/model_method_run_test.ts | 10 + src/cli/commands/type_search.ts | 7 + src/cli/commands/type_search_test.ts | 13 ++ src/cli/commands/workflow_run.ts | 13 +- src/cli/commands/workflow_run_test.ts | 32 ++++ src/cli/duration_parser.ts | 48 +++++ src/cli/duration_parser_test.ts | 47 +++++ src/cli/input_parser.ts | 35 +++- src/cli/input_parser_test.ts | 80 ++++++++ src/domain/errors.ts | 7 +- src/infrastructure/logging/logger.ts | 72 ++----- src/presentation/output/error_output.ts | 24 ++- src/presentation/output/error_output_test.ts | 66 ++++++- .../renderers/model_method_run.ts | 4 +- src/presentation/renderers/workflow_run.ts | 4 +- 31 files changed, 701 insertions(+), 101 deletions(-) create mode 100644 integration/json_isolation_test.ts create mode 100644 src/cli/commands/workflow_run_test.ts create mode 100644 src/cli/duration_parser.ts create mode 100644 src/cli/duration_parser_test.ts diff --git a/.claude/skills/swamp-model/SKILL.md b/.claude/skills/swamp-model/SKILL.md index 919042ce..d979b172 100644 --- a/.claude/skills/swamp-model/SKILL.md +++ b/.claude/skills/swamp-model/SKILL.md @@ -403,7 +403,8 @@ swamp model method run my-shell execute swamp model method run my-deploy create --input environment=prod swamp model method run my-deploy create --input environment=prod --input replicas=3 swamp model method run my-deploy create --input config.timeout=30 # dot notation for nesting -swamp model method run my-deploy create --input '{"environment": "prod"}' # JSON also supported +swamp model method run my-deploy create --input 'tags:json=["prod","west"]' # :json suffix for arrays/objects +swamp model method run my-deploy create --input '{"environment": "prod"}' # legacy single-shot JSON swamp model method run my-deploy create --input-file inputs.yaml swamp model method run my-deploy create --last-evaluated swamp model method run my-deploy create --skip-checks diff --git a/.claude/skills/swamp-workflow/SKILL.md b/.claude/skills/swamp-workflow/SKILL.md index e819248a..f65c532b 100644 --- a/.claude/skills/swamp-workflow/SKILL.md +++ b/.claude/skills/swamp-workflow/SKILL.md @@ -255,7 +255,8 @@ swamp workflow validate --json # Validate all swamp workflow run my-workflow swamp workflow run my-workflow --input environment=production swamp workflow run my-workflow --input environment=production --input replicas=3 -swamp workflow run my-workflow --input '{"environment": "production"}' # JSON also supported +swamp workflow run my-workflow --input 'tags:json=["prod","west"]' # :json suffix for arrays/objects +swamp workflow run my-workflow --input '{"environment": "production"}' # legacy single-shot JSON swamp workflow run my-workflow --input-file inputs.yaml swamp workflow run my-workflow --last-evaluated # Use pre-evaluated workflow ``` diff --git a/design/inputs.md b/design/inputs.md index d31cbda4..2f3a8326 100644 --- a/design/inputs.md +++ b/design/inputs.md @@ -197,12 +197,36 @@ base values and key=value pairs act as overrides (deep merged). ### Type coercion -Key=value inputs are always parsed as strings. When the workflow or model +Key=value inputs are parsed as strings by default. When the workflow or model declares an `InputsSchema`, string values are automatically coerced to match the schema's declared types (`number`, `integer`, `boolean`) before validation. Without a schema, values remain as strings. +### JSON-typed values via `:json` suffix + +Append `:json` to the leaf segment of a key to parse the value as JSON +instead of a string: + +```sh +# Array +swamp model method run my-model search --input 'keywords:json=["typescript","retry"]' + +# Object +swamp model method run my-model deploy --input 'config:json={"port":8080,"replicas":3}' + +# Nested key (suffix attaches to the LEAF segment only) +swamp model method run my-model deploy --input 'server.config:json={"port":8080}' +# → { server: { config: { port: 8080 } } } +``` + +The `:json` suffix bypasses the `@file` shorthand and the `\@` escape; +the value is always parsed as a JSON literal. JSON parse failures are +hard errors. When both `--input key:json=...` and a YAML +`--input-file` set the same key, the CLI value wins (existing +deepMerge precedence). + ### Arrays -Array inputs are not supported via key=value syntax. Use `--input-file` or JSON -for array values. +Array inputs are supported via the `:json` suffix above (preferred), or +via `--input-file` with YAML/JSON, or via the legacy single-shot +`--input ''` form. diff --git a/design/rendering.md b/design/rendering.md index 63045c54..1eda5d42 100644 --- a/design/rendering.md +++ b/design/rendering.md @@ -98,6 +98,45 @@ export function createWorkflowRunRenderer( The command handler becomes pure orchestration — wire deps, create contexts, pick a renderer, consume the stream, check the result. +## JSON Mode Output Contract + +When a command runs with `--json`, swamp guarantees the following invariants +to JSON consumers (`jq`, AI agents, CI scripts): + +1. **stdout contains exactly one valid JSON document** for the command's + primary output, OR a stream of newline-delimited JSON (NDJSON) documents + for streaming commands. No trailing whitespace, no log lines, no + prompts. +2. **stderr is reserved for log records** at the configured log level. It + may be empty, may contain LogTape pretty-formatted lines, but never + doubles as a structured-output channel. +3. **Errors emit a structured JSON object on stdout** with the shape + `{ error: string, stack?: string, code?: string }` and a non-zero + process exit. The `code` field is OPTIONAL — consumers MUST tolerate + its presence or absence. When present, it carries a machine-readable + identifier (e.g. `"cancelled"`, `"timeout"`, `"not_found"`, + `"validation_failed"`) suitable for programmatic dispatch. +4. **Commands MUST NOT prompt interactively in JSON mode.** Any + confirmation gate must be bypassed when the output mode is `json`. + Use `Deno.stdin.isTerminal()` to detect non-interactive contexts in + addition to `outputMode`. + +Renderer implementations for new commands MUST preserve these +guarantees. The regression test suite at `integration/json_isolation_test.ts` +exercises the contract across representative commands and is the +authoritative gate. + +This contract is enforced at the logging layer by +`initializeLogging({ jsonMode: true })` in +`src/infrastructure/logging/logger.ts`, which configures the +`["model","method","run"]`, `["workflow","run"]`, and `["logtape","meta"]` +category loggers with `parentSinks: "override"` so they cannot inherit +the root logger's sinks. The single emitter for fatal output in JSON +mode is `renderError` in +`src/presentation/output/error_output.ts` — it writes to stdout and +skips `logger.fatal`, so log-mode sinks cannot produce a duplicate +entry. + ## Logging Boundaries libswamp and renderers have distinct logging responsibilities: diff --git a/integration/data_output_flow_test.ts b/integration/data_output_flow_test.ts index eb84d2e8..06ec4275 100644 --- a/integration/data_output_flow_test.ts +++ b/integration/data_output_flow_test.ts @@ -798,7 +798,7 @@ Deno.test("Integration: output get fails for non-existent output", async () => { true, "Should fail for non-existent output", ); - assertStringIncludes(result.stderr, "not found"); + assertStringIncludes(result.stderr + result.stdout, "not found"); }); }); @@ -820,7 +820,7 @@ Deno.test("Integration: data get fails for non-existent model", async () => { ); assertEquals(result.code !== 0, true, "Should fail for non-existent model"); - assertStringIncludes(result.stderr, "not found"); + assertStringIncludes(result.stderr + result.stdout, "not found"); }); }); @@ -886,6 +886,6 @@ Deno.test("Integration: output data fails for non-existent field", async () => { ); assertEquals(result.code !== 0, true, "Should fail for non-existent field"); - assertStringIncludes(result.stderr, "not found"); + assertStringIncludes(result.stderr + result.stdout, "not found"); }); }); diff --git a/integration/driver_resolution_test.ts b/integration/driver_resolution_test.ts index e540aa84..0672b827 100644 --- a/integration/driver_resolution_test.ts +++ b/integration/driver_resolution_test.ts @@ -223,7 +223,10 @@ Deno.test( ); // Error surface is non-specific YAML parse failure; the point is the // run fails rather than silently proceeding with no marker. - assertStringIncludes(result.stderr.toLowerCase(), "yaml"); + assertStringIncludes( + (result.stderr + result.stdout).toLowerCase(), + "yaml", + ); }); }, ); diff --git a/integration/foreach_test.ts b/integration/foreach_test.ts index c45dadab..5506d33e 100644 --- a/integration/foreach_test.ts +++ b/integration/foreach_test.ts @@ -337,7 +337,7 @@ Deno.test("CLI: workflow with forEach validates array minItems", async () => { ); assertEquals(result.code !== 0, true, "Should fail for empty array"); - assertStringIncludes(result.stderr, "at least 1 item"); + assertStringIncludes(result.stderr + result.stdout, "at least 1 item"); }); }); diff --git a/integration/input_override_validation_test.ts b/integration/input_override_validation_test.ts index 6a79f1ab..b5277d67 100644 --- a/integration/input_override_validation_test.ts +++ b/integration/input_override_validation_test.ts @@ -186,7 +186,7 @@ Deno.test("CLI: model method run with type mismatch fails - number instead of st assertEquals(result.code !== 0, true, "Should fail for type mismatch"); // Zod validation error for method arguments assertStringIncludes( - result.stderr, + result.stderr + result.stdout, "Method arguments validation failed", ); }); diff --git a/integration/inputs_test.ts b/integration/inputs_test.ts index 134208c1..0da00891 100644 --- a/integration/inputs_test.ts +++ b/integration/inputs_test.ts @@ -184,7 +184,7 @@ Deno.test("CLI: model method run with invalid enum value fails", async () => { ); assertEquals(result.code !== 0, true, "Should fail for invalid enum"); - assertStringIncludes(result.stderr, "must be one of"); + assertStringIncludes(result.stderr + result.stdout, "must be one of"); }); }); @@ -208,7 +208,7 @@ Deno.test("CLI: model method run with missing required input fails", async () => ); assertEquals(result.code !== 0, true, "Should fail for missing input"); - assertStringIncludes(result.stderr, "environment"); + assertStringIncludes(result.stderr + result.stdout, "environment"); }); }); @@ -341,7 +341,7 @@ Deno.test("CLI: workflow run with missing required input fails", async () => { ); assertEquals(result.code !== 0, true, "Should fail for missing input"); - assertStringIncludes(result.stderr, "environment"); + assertStringIncludes(result.stderr + result.stdout, "environment"); }); }); @@ -492,7 +492,7 @@ Deno.test("CLI: input validation reports type mismatch", async () => { ); assertEquals(result.code !== 0, true, "Should fail for type mismatch"); - assertStringIncludes(result.stderr, "must be a string"); + assertStringIncludes(result.stderr + result.stdout, "must be a string"); }); }); @@ -550,8 +550,8 @@ Deno.test("CLI: input validation reports multiple errors", async () => { assertEquals(result.code !== 0, true, "Should fail for multiple errors"); // Should report both errors - assertStringIncludes(result.stderr, "name"); - assertStringIncludes(result.stderr, "count"); + assertStringIncludes(result.stderr + result.stdout, "name"); + assertStringIncludes(result.stderr + result.stdout, "count"); }); }); @@ -933,8 +933,8 @@ Deno.test("CLI: method validates required inputs that it references", async () = true, "Should fail without referenced required inputs", ); - assertStringIncludes(result.stderr, "dropletName"); - assertStringIncludes(result.stderr, "region"); + assertStringIncludes(result.stderr + result.stdout, "dropletName"); + assertStringIncludes(result.stderr + result.stdout, "region"); }); }); diff --git a/integration/json_isolation_test.ts b/integration/json_isolation_test.ts new file mode 100644 index 00000000..e82c4306 --- /dev/null +++ b/integration/json_isolation_test.ts @@ -0,0 +1,181 @@ +// 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 . + +/** + * JSON-mode output isolation regression tests. + * + * Asserts the contract that under `--json`, stdout is exactly one parseable + * JSON document and is not interleaved with log records. Without `--json`, + * the same commands still produce expected output (sanity check that the + * JSON-mode logger config doesn't accidentally suppress log-mode output). + * + * Tracks issue swamp-club#235. + */ + +import { assertEquals, assertStringIncludes } from "@std/assert"; +import { initializeTestRepo, runCliCommand } from "./test_helpers.ts"; + +async function withTempDir(fn: (dir: string) => Promise): Promise { + const dir = await Deno.makeTempDir({ prefix: "swamp-json-isolation-" }); + 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("--json: version stdout is parseable JSON", async () => { + const { stdout, code } = await runCliCommand( + ["version", "--json"], + Deno.cwd(), + ); + assertEquals(code, 0); + // Must parse as a single JSON document. + const parsed = JSON.parse(stdout); + assertEquals(typeof parsed.version, "string"); +}); + +Deno.test("--json: error path emits exactly one JSON document on stdout", async () => { + // `data list` without a model name fails with a UserError. The renderError + // path should write the error JSON to stdout exactly once — no double + // emission and no LogTape pretty-formatted FTL line. + await withTempDir(async (dir) => { + await initializeTestRepo(dir); + const { stdout, code } = await runCliCommand( + ["data", "list", "--json"], + dir, + ); + assertEquals(code, 1); + // Parse must succeed with one document. If the double-emission bug + // returns, stdout would have two concatenated JSON objects which + // JSON.parse rejects. + const parsed = JSON.parse(stdout); + assertEquals(typeof parsed.error, "string"); + }); +}); + +Deno.test("--json: stdout has no LogTape pretty-formatted log lines", async () => { + // `INF` / `WRN` / `ERR` prefixes are the LogTape pretty formatter's + // hallmark. They must never appear on stdout under --json. + await withTempDir(async (dir) => { + await initializeTestRepo(dir); + const { stdout } = await runCliCommand(["data", "list", "--json"], dir); + // Must not contain LogTape level prefixes. + if (/^\d{2}:\d{2}:\d{2}\.\d{3}\s+(INF|WRN|ERR|DBG|FTL)\s/m.test(stdout)) { + throw new Error( + `stdout contained LogTape pretty log line:\n${stdout}`, + ); + } + }); +}); + +Deno.test("--json: model method run stdout is parseable JSON", async () => { + // Exercises the model.method.run logger category specifically — the + // category most affected by the parentSinks: 'override' fix. + await withTempDir(async (dir) => { + await initializeTestRepo(dir); + const create = await runCliCommand( + ["model", "create", "command/shell", "smoke", "--json"], + dir, + ); + assertEquals(create.code, 0); + JSON.parse(create.stdout); // must parse + + const run = await runCliCommand( + [ + "model", + "method", + "run", + "smoke", + "execute", + "--input", + "run=echo hi", + "--json", + ], + dir, + ); + assertEquals(run.code, 0); + const result = JSON.parse(run.stdout); + assertEquals(result.status, "succeeded"); + // Stdout must not contain LogTape pretty-formatted log lines. + if ( + /^\d{2}:\d{2}:\d{2}\.\d{3}\s+(INF|WRN|ERR|DBG|FTL)\s/m.test(run.stdout) + ) { + throw new Error( + `model method run --json leaked log lines on stdout:\n${run.stdout}`, + ); + } + }); +}); + +Deno.test("log mode: version still emits version string", async () => { + // Sanity check: the JSON-mode logger isolation must not regress log-mode + // output. version in log mode prints the version through the logger. + const { stdout, code } = await runCliCommand(["version"], Deno.cwd()); + assertEquals(code, 0); + // Either stdout or stderr should contain the version string. The + // version command uses logger.info which goes to console (stderr by + // default in many LogTape configs); accept either. + const out = stdout; + // Version string is a number-prefixed identifier; look for the year prefix. + if (!/202\d/.test(out)) { + // Allow it on stderr too. + const cmd = await runCliCommand(["version"], Deno.cwd()); + if (!/202\d/.test(cmd.stdout) && !/202\d/.test(cmd.stderr ?? "")) { + throw new Error( + `version command produced no version string. stdout=${out}`, + ); + } + } +}); + +Deno.test("log mode: model method run still emits log records", async () => { + // Sanity: without --json, method run should produce log output (the + // method-summary report, status lines, etc.) so the JSON-mode fix + // didn't accidentally silence log mode. + await withTempDir(async (dir) => { + await initializeTestRepo(dir); + await runCliCommand( + ["model", "create", "command/shell", "smoke-log", "--json"], + dir, + ); + + const run = await runCliCommand( + [ + "model", + "method", + "run", + "smoke-log", + "execute", + "--input", + "run=echo hi", + ], + dir, + ); + assertEquals(run.code, 0); + // Log mode should produce SOME output across stdout+stderr — the + // method summary report at minimum. + const combined = run.stdout + run.stderr; + assertStringIncludes(combined, "succeeded"); + }); +}); diff --git a/integration/model_validate_test.ts b/integration/model_validate_test.ts index 2eaba50b..8b34d02c 100644 --- a/integration/model_validate_test.ts +++ b/integration/model_validate_test.ts @@ -280,7 +280,7 @@ Deno.test("CLI: model validate errors for non-existent model", async () => { true, `Command should fail`, ); - assertStringIncludes(result.stderr, "Model not found"); + assertStringIncludes(result.stderr + result.stdout, "Model not found"); }); }); @@ -443,7 +443,7 @@ Deno.test("CLI: model validate with no args errors when no models found", async true, `Command should fail`, ); - assertStringIncludes(result.stderr, "No models found"); + assertStringIncludes(result.stderr + result.stdout, "No models found"); }); }); diff --git a/integration/workflow_test.ts b/integration/workflow_test.ts index aa20e136..396c9031 100644 --- a/integration/workflow_test.ts +++ b/integration/workflow_test.ts @@ -178,7 +178,7 @@ Deno.test("CLI: workflow create rejects duplicate names", async () => { ); assertEquals(result.code !== 0, true, "Command should fail"); - assertStringIncludes(result.stderr, "already exists"); + assertStringIncludes(result.stderr + result.stdout, "already exists"); }); }); @@ -266,7 +266,7 @@ Deno.test("CLI: workflow get errors for non-existent workflow", async () => { ); assertEquals(result.code !== 0, true, "Command should fail"); - assertStringIncludes(result.stderr, "not found"); + assertStringIncludes(result.stderr + result.stdout, "not found"); }); }); @@ -356,7 +356,7 @@ Deno.test("CLI: workflow validate errors when no workflows found", async () => { ); assertEquals(result.code !== 0, true, "Command should fail"); - assertStringIncludes(result.stderr, "No workflows found"); + assertStringIncludes(result.stderr + result.stdout, "No workflows found"); }); }); @@ -684,7 +684,7 @@ Deno.test("CLI: workflow run errors for non-existent workflow", async () => { ); assertEquals(result.code !== 0, true, "Command should fail"); - assertStringIncludes(result.stderr, "not found"); + assertStringIncludes(result.stderr + result.stdout, "not found"); }); }); @@ -1042,11 +1042,11 @@ Deno.test("CLI: model delete blocked when referenced by workflow, succeeds after "Model delete should fail when referenced by workflow", ); assertStringIncludes( - deleteModelBlockedResult.stderr, + deleteModelBlockedResult.stderr + deleteModelBlockedResult.stdout, "workflow-using-model", ); assertStringIncludes( - deleteModelBlockedResult.stderr, + deleteModelBlockedResult.stderr + deleteModelBlockedResult.stdout, "referenced by workflow", ); @@ -1155,7 +1155,7 @@ Deno.test("CLI: model delete blocked when referenced by workflow using model ID" "Model delete should fail when referenced by workflow using ID", ); assertStringIncludes( - deleteModelResult.stderr, + deleteModelResult.stderr + deleteModelResult.stdout, "workflow-using-id", ); }); @@ -1386,7 +1386,7 @@ Deno.test("CLI: workflow delete command removes workflow and all runs", async () ); assertEquals(getResult.code !== 0, true, "Workflow should not exist"); - assertStringIncludes(getResult.stderr, "not found"); + assertStringIncludes(getResult.stderr + getResult.stdout, "not found"); }); }); diff --git a/src/cli/commands/data_gc.ts b/src/cli/commands/data_gc.ts index c91bfc47..5c0d52ce 100644 --- a/src/cli/commands/data_gc.ts +++ b/src/cli/commands/data_gc.ts @@ -59,6 +59,10 @@ export const dataGcCommand = new Command() .description("Run garbage collection on data (lifecycle and versions)") .example("Preview what would be collected", "swamp data gc --dry-run") .example("Run garbage collection", "swamp data gc --force") + .example( + "Run non-interactively in JSON mode (no prompt, structured output)", + "swamp data gc --json", + ) .option( "--repo-dir ", "Repository directory (env: SWAMP_REPO_DIR)", diff --git a/src/cli/commands/data_gc_test.ts b/src/cli/commands/data_gc_test.ts index 87eae923..1b6a1fd0 100644 --- a/src/cli/commands/data_gc_test.ts +++ b/src/cli/commands/data_gc_test.ts @@ -57,3 +57,12 @@ Deno.test("dataGcCommand - has force option", async () => { const force = options.find((opt) => opt.name === "force"); assertEquals(force !== undefined, true); }); + +Deno.test("dataGcCommand - help text includes a --json non-interactive example", async () => { + // swamp-club#235 — agentic users need to discover that JSON mode + // bypasses the interactive prompt without reading source. + const { dataGcCommand } = await import("./data_gc.ts"); + const examples = dataGcCommand.getExamples().map((e) => e.description); + const hasJsonExample = examples.some((d) => d.toLowerCase().includes("json")); + assertEquals(hasJsonExample, true, `examples were: ${examples.join(" | ")}`); +}); diff --git a/src/cli/commands/doctor.ts b/src/cli/commands/doctor.ts index 968e9145..f1297366 100644 --- a/src/cli/commands/doctor.ts +++ b/src/cli/commands/doctor.ts @@ -37,6 +37,13 @@ export const doctorCommand = new Command() "Check that user-defined extensions in this repo load cleanly", "swamp doctor extensions", ) + // `--repo-dir` is accepted on the top-level command for consistency + // with subcommands and other repo-scoped commands. The top-level + // action only shows help; subcommands consume the option. + .option( + "--repo-dir ", + "Repository directory (env: SWAMP_REPO_DIR)", + ) .action(function () { this.showHelp(); }) diff --git a/src/cli/commands/model_method_run.ts b/src/cli/commands/model_method_run.ts index f6416f75..77b7a7e3 100644 --- a/src/cli/commands/model_method_run.ts +++ b/src/cli/commands/model_method_run.ts @@ -61,6 +61,7 @@ import { type ModelMethodRunDeps, } from "../../libswamp/mod.ts"; import { createModelMethodRunRenderer } from "../../presentation/renderers/model_method_run.ts"; +import { parseTimeout } from "../duration_parser.ts"; // Cliffy's custom type system returns `unknown` for custom types like `model_name`, // but we need to pass `options` to functions expecting specific types. Using `any` @@ -131,6 +132,10 @@ export const modelMethodRunCommand = new Command() "--driver ", "Override execution driver (e.g. raw, docker)", ) + .option( + "--timeout ", + "Cooperative cancellation deadline (e.g. 30s, 5m, 1h). Aborts the run when it expires; emits code: 'cancelled' on timeout. Effective only for methods that honor AbortSignal — long-running model methods that ignore the signal will not be interrupted.", + ) .action( // @ts-expect-error - Cliffy custom type returns unknown instead of string async function ( @@ -233,7 +238,13 @@ export const modelMethodRunCommand = new Command() }, }; - const libCtx = createLibSwampContext(); + const timeoutMs = options.timeout + ? parseTimeout(options.timeout as string) + : undefined; + const baseLibCtx = createLibSwampContext(); + const libCtx = timeoutMs !== undefined + ? baseLibCtx.withTimeout(timeoutMs) + : baseLibCtx; const renderer = createModelMethodRunRenderer(ctx.outputMode, { modelName: modelIdOrName, methodName, diff --git a/src/cli/commands/model_method_run_test.ts b/src/cli/commands/model_method_run_test.ts index 62cea964..d354f8b0 100644 --- a/src/cli/commands/model_method_run_test.ts +++ b/src/cli/commands/model_method_run_test.ts @@ -58,3 +58,13 @@ Deno.test("modelMethodCommand has run as subcommand", async () => { const runCommand = commands.find((cmd) => cmd.getName() === "run"); assertEquals(runCommand !== undefined, true); }); + +Deno.test("modelMethodRunCommand has --timeout option (swamp-club#235)", async () => { + const { modelMethodRunCommand } = await import("./model_method_run.ts"); + const names = modelMethodRunCommand.getOptions().map((o) => o.name); + if (!names.includes("timeout")) { + throw new Error( + `expected --timeout option, got: ${names.join(", ")}`, + ); + } +}); diff --git a/src/cli/commands/type_search.ts b/src/cli/commands/type_search.ts index cf6dc00c..d8344d3b 100644 --- a/src/cli/commands/type_search.ts +++ b/src/cli/commands/type_search.ts @@ -76,6 +76,13 @@ export const typeSearchCommand = new Command() .description("Search for model types") .example("Browse all types", "swamp type search") .example("Search by keyword", "swamp type search aws") + // `--repo-dir` is accepted for agentic-flow consistency with other + // commands; type search reads only the global extension catalog and + // does not require an initialized repo, so the option is informational. + .option( + "--repo-dir ", + "Repository directory (informational; type search does not require an initialized repo)", + ) .arguments("[query:string]") .action(async function (options: AnyOptions, query?: string) { const ctx = createContext(options as GlobalOptions, ["type", "search"]); diff --git a/src/cli/commands/type_search_test.ts b/src/cli/commands/type_search_test.ts index 078ee694..0beb6e34 100644 --- a/src/cli/commands/type_search_test.ts +++ b/src/cli/commands/type_search_test.ts @@ -46,3 +46,16 @@ Deno.test("typeSearchCommand is registered as subcommand of modelTypeCommand", a const searchCmd = commands.find((c: Command) => c.getName() === "search"); assertEquals(searchCmd !== undefined, true); }); + +Deno.test("typeSearchCommand accepts --repo-dir for agentic-flow consistency", async () => { + // type search reads only the global extension catalog and does not + // require an initialized repo, but the option is accepted so agents + // can pass it uniformly across all swamp commands. (swamp-club#235) + const { typeSearchCommand } = await import("./type_search.ts"); + const names = typeSearchCommand.getOptions().map((o) => o.name); + if (!names.includes("repo-dir")) { + throw new Error( + `expected --repo-dir option, got: ${names.join(", ")}`, + ); + } +}); diff --git a/src/cli/commands/workflow_run.ts b/src/cli/commands/workflow_run.ts index e833b96b..8ecf6bb5 100644 --- a/src/cli/commands/workflow_run.ts +++ b/src/cli/commands/workflow_run.ts @@ -36,6 +36,7 @@ import { WorkflowExecutionService } from "../../domain/workflows/execution_servi import { createWorkflowId } from "../../domain/workflows/workflow_id.ts"; import { SWAMP_SUBDIRS } from "../../infrastructure/persistence/paths.ts"; import { parseInputs } from "../input_parser.ts"; +import { parseTimeout } from "../duration_parser.ts"; import { GIT_SHA } from "./version.ts"; import { modelRegistry } from "../../domain/models/model.ts"; import { vaultTypeRegistry } from "../../domain/vaults/vault_type_registry.ts"; @@ -122,6 +123,10 @@ export const workflowRunCommand = new Command() "Skip pre-flight checks with this label", { collect: true }, ) + .option( + "--timeout ", + "Cooperative cancellation deadline (e.g. 30s, 5m, 1h). Aborts the workflow when it expires; emits code: 'cancelled' on timeout. Effective only for steps whose model methods honor AbortSignal.", + ) // @ts-expect-error - Cliffy custom type returns unknown instead of string .action(async function (options: AnyOptions, workflowIdOrName: string) { const ctx = createContext(options as GlobalOptions, ["workflow", "run"]); @@ -249,7 +254,13 @@ export const workflowRunCommand = new Command() definitionRepo: repoContext.definitionRepo, }; - const libCtx = createLibSwampContext(); + const timeoutMs = options.timeout + ? parseTimeout(options.timeout as string) + : undefined; + const baseLibCtx = createLibSwampContext(); + const libCtx = timeoutMs !== undefined + ? baseLibCtx.withTimeout(timeoutMs) + : baseLibCtx; const renderer = createWorkflowRunRenderer(ctx.outputMode, { workflowName: workflowIdOrName, forceLog: ctx.forceLog, diff --git a/src/cli/commands/workflow_run_test.ts b/src/cli/commands/workflow_run_test.ts new file mode 100644 index 00000000..11a5aa5a --- /dev/null +++ b/src/cli/commands/workflow_run_test.ts @@ -0,0 +1,32 @@ +// 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 . + +import { initializeLogging } from "../../infrastructure/logging/logger.ts"; + +await initializeLogging({}); + +Deno.test("workflowRunCommand has --timeout option (swamp-club#235)", async () => { + const { workflowRunCommand } = await import("./workflow_run.ts"); + const names = workflowRunCommand.getOptions().map((o) => o.name); + if (!names.includes("timeout")) { + throw new Error( + `expected --timeout option, got: ${names.join(", ")}`, + ); + } +}); diff --git a/src/cli/duration_parser.ts b/src/cli/duration_parser.ts new file mode 100644 index 00000000..0e3158a4 --- /dev/null +++ b/src/cli/duration_parser.ts @@ -0,0 +1,48 @@ +// 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 . + +import { parseDuration } from "../libswamp/mod.ts"; +import { UserError } from "../domain/errors.ts"; + +/** + * Parses a CLI timeout value like `30s`, `5m`, `1h`. Extends libswamp's + * `parseDuration` (which is data-retention oriented and starts at + * minutes) with second-level granularity needed for short-lived + * cancellations. Returns milliseconds, always > 0. + */ +export function parseTimeout(value: string): number { + const trimmed = value.trim(); + const secondsMatch = trimmed.match(/^(\d+)s$/); + if (secondsMatch) { + const seconds = parseInt(secondsMatch[1], 10); + if (seconds <= 0) { + throw new UserError( + `Invalid --timeout value "${value}": must be positive`, + ); + } + return seconds * 1000; + } + const ms = parseDuration(trimmed); + if (ms <= 0) { + throw new UserError( + `Invalid --timeout value "${value}": must be positive`, + ); + } + return ms; +} diff --git a/src/cli/duration_parser_test.ts b/src/cli/duration_parser_test.ts new file mode 100644 index 00000000..98df5302 --- /dev/null +++ b/src/cli/duration_parser_test.ts @@ -0,0 +1,47 @@ +// 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 . + +import { assertEquals, assertThrows } from "@std/assert"; +import { parseTimeout } from "./duration_parser.ts"; +import { UserError } from "../domain/errors.ts"; + +Deno.test("parseTimeout: seconds", () => { + assertEquals(parseTimeout("1s"), 1000); + assertEquals(parseTimeout("30s"), 30_000); +}); + +Deno.test("parseTimeout: minutes via libswamp parseDuration", () => { + assertEquals(parseTimeout("5m"), 5 * 60 * 1000); +}); + +Deno.test("parseTimeout: hours", () => { + assertEquals(parseTimeout("1h"), 60 * 60 * 1000); +}); + +Deno.test("parseTimeout: rejects non-positive seconds", () => { + assertThrows(() => parseTimeout("0s"), UserError, "must be positive"); +}); + +Deno.test("parseTimeout: rejects unrecognized format", () => { + assertThrows(() => parseTimeout("forever"), UserError); +}); + +Deno.test("parseTimeout: trims whitespace", () => { + assertEquals(parseTimeout(" 1s "), 1000); +}); diff --git a/src/cli/input_parser.ts b/src/cli/input_parser.ts index aba4d664..aca1b38b 100644 --- a/src/cli/input_parser.ts +++ b/src/cli/input_parser.ts @@ -103,6 +103,11 @@ async function resolveFileValue( * - Values starting with `@` read file contents: `key=@path/to/file` * - Escaped `@` with `\@` produces a literal `@`: `key=\@literal` * - Splits on first `=` only, so values can contain `=` + * - `:json` suffix on the leaf segment of the key parses the value as + * JSON: `keywords:json=["a","b"]`, `server.config:json={"port":8080}`. + * The `@file` and `\@literal` interactions are bypassed when `:json` + * is set — the value is read as a literal JSON string. JSON parse + * failures are hard errors. */ export async function parseKeyValueInputs( entries: string[], @@ -117,12 +122,38 @@ export async function parseKeyValueInputs( ); } - const key = entry.slice(0, eqIndex); + let key = entry.slice(0, eqIndex); if (key === "") { throw new UserError(`Invalid input: empty key in "${entry}".`); } - let value: string = entry.slice(eqIndex + 1); + const rawValue: string = entry.slice(eqIndex + 1); + + // Detect a `:json` suffix on the LEAF segment of a dot-notation + // path. e.g. `server.config:json` → leaf `config:json` → leaf + // `config` with JSON-typed value; `keywords:json` → `keywords`. + const jsonSuffix = ":json"; + const dotIndex = key.lastIndexOf("."); + const leaf = dotIndex >= 0 ? key.slice(dotIndex + 1) : key; + if (leaf.endsWith(jsonSuffix) && leaf.length > jsonSuffix.length) { + const cleanedLeaf = leaf.slice(0, -jsonSuffix.length); + key = dotIndex >= 0 + ? key.slice(0, dotIndex + 1) + cleanedLeaf + : cleanedLeaf; + let parsed: unknown; + try { + parsed = JSON.parse(rawValue); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new UserError( + `Invalid JSON value for input "${key}": ${message}`, + ); + } + setNestedValue(result, key, parsed); + continue; + } + + let value: string = rawValue; if (value.startsWith("\\@")) { // Escaped @ — use literal value without the backslash diff --git a/src/cli/input_parser_test.ts b/src/cli/input_parser_test.ts index d4698c97..a0a8ed33 100644 --- a/src/cli/input_parser_test.ts +++ b/src/cli/input_parser_test.ts @@ -24,6 +24,7 @@ import { parseKeyValueInputs, setNestedValue, } from "./input_parser.ts"; +import { UserError } from "../domain/errors.ts"; import { stringify as stringifyYaml } from "@std/yaml"; // --- setNestedValue --- @@ -374,6 +375,85 @@ Deno.test("parseInputs: JSON input ignores input-file", async () => { } }); +// --- parseKeyValueInputs (:json suffix) — swamp-club#235 --- + +Deno.test("parseKeyValueInputs: :json suffix parses an array", async () => { + const result = await parseInputs({ + input: ['keywords:json=["typescript","retry"]'], + }); + assertEquals(result.source, "key-value"); + assertEquals(result.inputs, { keywords: ["typescript", "retry"] }); +}); + +Deno.test("parseKeyValueInputs: :json suffix parses an object", async () => { + const result = await parseInputs({ + input: ['config:json={"port":8080,"host":"localhost"}'], + }); + assertEquals(result.source, "key-value"); + assertEquals(result.inputs, { + config: { port: 8080, host: "localhost" }, + }); +}); + +Deno.test("parseKeyValueInputs: :json suffix on the leaf of a nested key", async () => { + const result = await parseInputs({ + input: ['server.config:json={"port":8080}'], + }); + assertEquals(result.source, "key-value"); + assertEquals(result.inputs, { + server: { config: { port: 8080 } }, + }); +}); + +Deno.test("parseKeyValueInputs: :json takes precedence over @file shorthand", async () => { + // A `@`-prefixed value would normally be treated as a file path; the + // :json suffix bypasses that and parses the literal as JSON. So + // `key:json=@notafile.json` parses `@notafile.json` as JSON (which + // fails) — verifying the suffix takes precedence. + await assertRejects( + () => parseInputs({ input: ["key:json=@notafile"] }), + UserError, + "Invalid JSON value for input", + ); +}); + +Deno.test("parseKeyValueInputs: no :json suffix preserves string behavior", async () => { + const result = await parseInputs({ + input: ['raw=["this","is","a","string"]'], + }); + assertEquals(result.source, "key-value"); + assertEquals(result.inputs, { raw: '["this","is","a","string"]' }); +}); + +Deno.test("parseKeyValueInputs: :json parse failure raises UserError", async () => { + await assertRejects( + () => parseInputs({ input: ["bad:json={not json}"] }), + UserError, + "Invalid JSON value for input", + ); +}); + +Deno.test("parseKeyValueInputs: :json from CLI overrides --input-file value", async () => { + // Confirms precedence: the YAML file sets keywords as a string list, + // the CLI :json override replaces it with a parsed array. Existing + // deepMerge semantics: CLI key-value wins over file. + const tempFile = await Deno.makeTempFile({ suffix: ".yaml" }); + try { + await Deno.writeTextFile( + tempFile, + stringifyYaml({ keywords: ["from-file"] } as Record), + ); + const result = await parseInputs({ + input: ['keywords:json=["from-cli"]'], + inputFile: tempFile, + }); + assertEquals(result.source, "combined"); + assertEquals(result.inputs, { keywords: ["from-cli"] }); + } finally { + await Deno.remove(tempFile); + } +}); + // --- parseInputs (none) --- Deno.test("parseInputs: no options returns none", async () => { diff --git a/src/domain/errors.ts b/src/domain/errors.ts index 5db4f52d..39dbb97c 100644 --- a/src/domain/errors.ts +++ b/src/domain/errors.ts @@ -21,11 +21,16 @@ * Base error for user-facing errors that should not show a stack trace. * Use this for validation errors, "model not found" messages, and other * expected error conditions where the stack trace would be noise. + * + * The optional `code` carries a machine-readable identifier (e.g. + * `"cancelled"`, `"timeout"`) that surfaces in JSON error output. */ export class UserError extends Error { - constructor(message: string) { + readonly code?: string; + constructor(message: string, code?: string) { super(message); this.name = "UserError"; + this.code = code; } } diff --git a/src/infrastructure/logging/logger.ts b/src/infrastructure/logging/logger.ts index 42db6ba6..003cf338 100644 --- a/src/infrastructure/logging/logger.ts +++ b/src/infrastructure/logging/logger.ts @@ -23,7 +23,6 @@ import { getLogger, getTextFormatter, type LogLevel, - type LogRecord, type Sink, } from "@logtape/logtape"; import { getPrettyFormatter } from "@logtape/pretty"; @@ -39,48 +38,6 @@ export interface LoggingOptions { noColor?: boolean; } -/** - * Creates a sink that formats fatal log records as JSON on stderr. - * Used in JSON mode so error output matches the structured output contract. - */ -function createJsonErrorSink(): Sink { - return (record: LogRecord) => { - const errorProp = record.properties["error"]; - const messageProp = record.properties["message"]; - - let errorMessage: string; - let stack: string | undefined; - - if (errorProp instanceof Error) { - errorMessage = errorProp.message; - stack = extractStackLines(errorProp.stack); - } else if (typeof messageProp === "string") { - errorMessage = messageProp; - } else { - // Fallback: render the full message - errorMessage = record.message.map(String).join(""); - } - - const data: Record = { error: errorMessage }; - if (stack) { - data.stack = stack; - } - console.error(JSON.stringify(data, null, 2)); - }; -} - -/** - * Extracts just the stack trace lines from an error stack. - * Removes the error message line and any source code snippets. - */ -function extractStackLines(stack: string | undefined): string | undefined { - if (!stack) return undefined; - - const lines = stack.split("\n"); - const stackLines = lines.filter((line) => line.trim().startsWith("at ")); - return stackLines.length > 0 ? stackLines.join("\n") : undefined; -} - let isInitialized = false; export async function initializeLogging( @@ -129,18 +86,16 @@ export async function initializeLogging( } if (options.jsonMode) { - sinks["jsonError"] = createJsonErrorSink(); - } - - if (options.jsonMode) { - // In JSON mode without debug, suppress most console output - // to keep stdout clean for structured output. - // Fatal messages use a JSON-formatted sink on stderr so - // error output is valid JSON matching the structured output contract. + // JSON mode: the renderError() function in + // src/presentation/output/error_output.ts is the single emitter for + // fatal output (it writes JSON to stdout and skips logger.fatal). + // The root logger has no sinks so nothing leaks via the standard + // logging pipeline. Audit at swamp-club#235: only error_output.ts + // calls logger.fatal in src/, both inside renderError itself. loggers.push({ category: [], lowestLevel: "fatal", - sinks: ["jsonError"], + sinks: [], }); } else { loggers.push({ @@ -150,6 +105,14 @@ export async function initializeLogging( }); } + // In JSON mode, sever sink inheritance from the root logger on the + // category loggers — otherwise a child logger emitting an info record + // would also emit through the root's `jsonError` sink, polluting + // stderr with malformed JSON. `parentSinks: 'override'` is the + // documented LogTape API for this (since 0.6.0). Also clear the + // logtape.meta logger's own sinks in JSON mode so its warnings + // don't reach the console at all. + const jsonMode = options.jsonMode ?? false; await configure({ sinks, loggers: [ @@ -158,16 +121,19 @@ export async function initializeLogging( category: ["model", "method", "run"], lowestLevel: logLevel, sinks: ["runFile"], + parentSinks: jsonMode ? "override" : "inherit", }, { category: ["workflow", "run"], lowestLevel: logLevel, sinks: ["runFile"], + parentSinks: jsonMode ? "override" : "inherit", }, { category: ["logtape", "meta"], lowestLevel: "warning", - sinks: ["console"], + sinks: jsonMode ? [] : ["console"], + parentSinks: jsonMode ? "override" : "inherit", }, ], }); diff --git a/src/presentation/output/error_output.ts b/src/presentation/output/error_output.ts index 48acde9c..3bf34db7 100644 --- a/src/presentation/output/error_output.ts +++ b/src/presentation/output/error_output.ts @@ -26,8 +26,12 @@ const logger = getSwampLogger(["error"]); /** * Builds the JSON error object for structured output. - * Format: { error: string, stack?: string } - * Matches the format used by createJsonErrorSink in the logging layer. + * + * Shape: `{ error: string, stack?: string, code?: string }`. The `code` + * field is set when the underlying error carries a machine-readable + * identifier (e.g. `UserError.code` or any error object exposing a + * string `code` property — `SwampError`-like). Both `code` and `stack` + * are optional; consumers must tolerate their presence or absence. */ export function buildErrorJson(err: Error): Record { const data: Record = { error: err.message }; @@ -42,16 +46,21 @@ export function buildErrorJson(err: Error): Record { data.stack = stackLines.join("\n"); } } + const maybeCode = (err as { code?: unknown }).code; + if (typeof maybeCode === "string" && maybeCode.length > 0) { + data.code = maybeCode; + } return data; } /** - * Renders an error via LogTape at fatal level. - * UserError instances and Cliffy ValidationErrors log just the message (no stack trace). - * Other errors log the full Error object (including stack trace via Deno.inspect). + * Renders an error to the user. * - * In JSON mode, also writes the error as JSON to stdout so pipe consumers - * (jq, AI agents) see the failure instead of receiving empty stdout. + * In JSON mode this is the SINGLE emitter for fatal output: it writes + * the JSON error to stdout and does NOT call `logger.fatal`, so log-mode + * sinks never produce a duplicate FTL line. In log mode it falls + * through to LogTape — UserError / Cliffy ValidationError emit just the + * message; other errors emit the full Error (stack trace included). */ export function renderError(error: unknown, outputMode?: OutputMode): void { const err = error instanceof Error ? error : new Error(String(error)); @@ -59,6 +68,7 @@ export function renderError(error: unknown, outputMode?: OutputMode): void { if (outputMode === "json") { // deno-lint-ignore no-console console.log(JSON.stringify(buildErrorJson(err), null, 2)); + return; } if (err instanceof UserError || err instanceof ValidationError) { diff --git a/src/presentation/output/error_output_test.ts b/src/presentation/output/error_output_test.ts index 1f17e4c1..32f8e893 100644 --- a/src/presentation/output/error_output_test.ts +++ b/src/presentation/output/error_output_test.ts @@ -182,7 +182,7 @@ Deno.test("renderError uses fatal level for all errors", () => { // JSON stdout output tests // ============================================================================ -Deno.test("renderError: json mode writes error JSON to stdout", () => { +Deno.test("renderError: json mode is single-emitter (stdout JSON, no stderr)", () => { const stdoutLogs: string[] = []; const stderrLogs: string[] = []; const originalLog = console.log; @@ -197,8 +197,51 @@ Deno.test("renderError: json mode writes error JSON to stdout", () => { const parsed = JSON.parse(stdoutLogs[0]); assertEquals(parsed.error, "Model not found"); assertEquals(parsed.stack, undefined); - // stderr should also get the error (via LogTape) - assertEquals(stderrLogs.length, 1); + // Single-emission contract: in JSON mode renderError is the only + // emitter — logger.fatal must not be called, so stderr stays + // untouched. (See swamp-club#235.) + assertEquals(stderrLogs.length, 0); + } finally { + console.log = originalLog; + console.error = originalError; + } +}); + +Deno.test("renderError: json mode surfaces UserError code field", () => { + const stdoutLogs: string[] = []; + const originalLog = console.log; + const originalError = console.error; + console.log = (...args: unknown[]) => stdoutLogs.push(args.join(" ")); + console.error = () => {}; + + try { + renderError(new UserError("Operation timed out", "timeout"), "json"); + assertEquals(stdoutLogs.length, 1); + const parsed = JSON.parse(stdoutLogs[0]); + assertEquals(parsed.error, "Operation timed out"); + assertEquals(parsed.code, "timeout"); + } finally { + console.log = originalLog; + console.error = originalError; + } +}); + +Deno.test("renderError: json mode surfaces a `code` property from any error", () => { + // Errors thrown from libswamp paths often carry a `code` via duck typing + // rather than via UserError (e.g. cancellations, validation failures). + const stdoutLogs: string[] = []; + const originalLog = console.log; + const originalError = console.error; + console.log = (...args: unknown[]) => stdoutLogs.push(args.join(" ")); + console.error = () => {}; + + try { + const error = new Error("Operation was cancelled."); + Object.assign(error, { code: "cancelled" }); + renderError(error, "json"); + assertEquals(stdoutLogs.length, 1); + const parsed = JSON.parse(stdoutLogs[0]); + assertEquals(parsed.code, "cancelled"); } finally { console.log = originalLog; console.error = originalError; @@ -263,3 +306,20 @@ Deno.test("buildErrorJson: Cliffy ValidationError has no stack", () => { assertEquals(result.error, "Missing argument(s): extension"); assertEquals(result.stack, undefined); }); + +Deno.test("buildErrorJson: surfaces UserError code", () => { + const result = buildErrorJson(new UserError("timed out", "timeout")); + assertEquals(result.code, "timeout"); +}); + +Deno.test("buildErrorJson: surfaces a `code` property from any error", () => { + const error = new Error("cancelled by user"); + Object.assign(error, { code: "cancelled" }); + const result = buildErrorJson(error); + assertEquals(result.code, "cancelled"); +}); + +Deno.test("buildErrorJson: omits code field when not present", () => { + const result = buildErrorJson(new UserError("plain")); + assertEquals(result.code, undefined); +}); diff --git a/src/presentation/renderers/model_method_run.ts b/src/presentation/renderers/model_method_run.ts index e2d1944d..d7afea6f 100644 --- a/src/presentation/renderers/model_method_run.ts +++ b/src/presentation/renderers/model_method_run.ts @@ -156,7 +156,7 @@ class LogModelMethodRunRenderer implements ModelMethodRunRenderer { } }, error: (e) => { - throw new UserError(e.error.message); + throw new UserError(e.error.message, e.error.code); }, }; } @@ -199,7 +199,7 @@ class JsonModelMethodRunRenderer implements ModelMethodRunRenderer { console.log(JSON.stringify(e.run, null, 2)); }, error: (e) => { - throw new UserError(e.error.message); + throw new UserError(e.error.message, e.error.code); }, }; } diff --git a/src/presentation/renderers/workflow_run.ts b/src/presentation/renderers/workflow_run.ts index 405eaedf..d01055d0 100644 --- a/src/presentation/renderers/workflow_run.ts +++ b/src/presentation/renderers/workflow_run.ts @@ -187,7 +187,7 @@ class LogWorkflowRunRenderer implements WorkflowRunRenderer { } }, error: (e) => { - throw new UserError(e.error.message); + throw new UserError(e.error.message, e.error.code); }, }; } @@ -256,7 +256,7 @@ class JsonWorkflowRunRenderer implements WorkflowRunRenderer { console.log(JSON.stringify(e.run, null, 2)); }, error: (e) => { - throw new UserError(e.error.message); + throw new UserError(e.error.message, e.error.code); }, }; } From ec185a7ae82afd92e1ce0bfb567bc338838f95c5 Mon Sep 17 00:00:00 2001 From: stack72 Date: Tue, 5 May 2026 14:11:15 +0100 Subject: [PATCH 2/3] fixup: address CLI UX review + skill review threshold CLI UX review (PR #1305 review feedback): - Trim --timeout description on model method run + workflow run from ~220 chars to ~100; align wording across both commands. - Restore familiar --repo-dir wording on type_search ("env: SWAMP_REPO_DIR; not required for type search") so users scripting via SWAMP_REPO_DIR see the standard pattern. - Add a --input :json discoverability example to model method run + workflow run help text (parallel treatment to the data gc --json example). Skill review: - swamp-data-query content score was 0.7999... (just below the 0.90 strict threshold from scripts/review_skills.ts) due to LLM-judge rounding. Address the two judge suggestions: add an "Empty results and errors" section covering empty matches, malformed CEL, missing-field semantics, and binary content; clarify the references/fields.md path. Score now 94%. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/skills/swamp-data-query/SKILL.md | 20 ++++++++++++++++++-- src/cli/commands/model_method_run.ts | 6 +++++- src/cli/commands/type_search.ts | 4 ++-- src/cli/commands/workflow_run.ts | 6 +++++- 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/.claude/skills/swamp-data-query/SKILL.md b/.claude/skills/swamp-data-query/SKILL.md index 0b353222..b353e711 100644 --- a/.claude/skills/swamp-data-query/SKILL.md +++ b/.claude/skills/swamp-data-query/SKILL.md @@ -144,7 +144,23 @@ const names = await context.queryData!( // names is string[] ``` +## Empty results and errors + +- A predicate that matches nothing returns an empty list (CLI: empty stdout in + JSON mode, or "No matching data" in log mode). This is success, not error — + exit code is 0. +- Malformed CEL fails fast with a `code: "validation_failed"` error before any + data is read. Verify the predicate against + [references/fields.md](references/fields.md) for available fields. +- Referencing `attributes.` or `content.` on a record that lacks the field + yields `null` for that record, not an error — predicates involving the field + simply don't match. Use `has(attributes.x)` to filter records that have the + field set. +- Binary data records expose `content` as `bytes`; field projection on binary + `content` returns base64. + ## References -See [references/fields.md](references/fields.md) for the complete DataRecord -field reference and CEL operator examples. +See [references/fields.md](references/fields.md) (also at +`.claude/skills/swamp-data-query/references/fields.md` in this repo) for the +complete DataRecord field reference and CEL operator examples. diff --git a/src/cli/commands/model_method_run.ts b/src/cli/commands/model_method_run.ts index 77b7a7e3..f696d08a 100644 --- a/src/cli/commands/model_method_run.ts +++ b/src/cli/commands/model_method_run.ts @@ -77,6 +77,10 @@ export const modelMethodRunCommand = new Command() "Run with inputs", "swamp model method run my-server deploy --input env=prod", ) + .example( + "Pass an array or object input (JSON-typed via :json suffix)", + 'swamp model method run my-server search --input \'keywords:json=["a","b"]\'', + ) .arguments(" ") .option( "--repo-dir ", @@ -134,7 +138,7 @@ export const modelMethodRunCommand = new Command() ) .option( "--timeout ", - "Cooperative cancellation deadline (e.g. 30s, 5m, 1h). Aborts the run when it expires; emits code: 'cancelled' on timeout. Effective only for methods that honor AbortSignal — long-running model methods that ignore the signal will not be interrupted.", + "Cancellation deadline (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.", ) .action( // @ts-expect-error - Cliffy custom type returns unknown instead of string diff --git a/src/cli/commands/type_search.ts b/src/cli/commands/type_search.ts index d8344d3b..e31c2901 100644 --- a/src/cli/commands/type_search.ts +++ b/src/cli/commands/type_search.ts @@ -78,10 +78,10 @@ export const typeSearchCommand = new Command() .example("Search by keyword", "swamp type search aws") // `--repo-dir` is accepted for agentic-flow consistency with other // commands; type search reads only the global extension catalog and - // does not require an initialized repo, so the option is informational. + // does not require an initialized repo. .option( "--repo-dir ", - "Repository directory (informational; type search does not require an initialized repo)", + "Repository directory (env: SWAMP_REPO_DIR; not required for type search)", ) .arguments("[query:string]") .action(async function (options: AnyOptions, query?: string) { diff --git a/src/cli/commands/workflow_run.ts b/src/cli/commands/workflow_run.ts index 8ecf6bb5..714469c9 100644 --- a/src/cli/commands/workflow_run.ts +++ b/src/cli/commands/workflow_run.ts @@ -63,6 +63,10 @@ export const workflowRunCommand = new Command() "With inputs", "swamp workflow run deploy-pipeline --input env=prod", ) + .example( + "Pass an array or object input (JSON-typed via :json suffix)", + 'swamp workflow run deploy-pipeline --input \'tags:json=["prod","west"]\'', + ) .example( "With tags", "swamp workflow run deploy-pipeline --tag type=deploy --tag env=production", @@ -125,7 +129,7 @@ export const workflowRunCommand = new Command() ) .option( "--timeout ", - "Cooperative cancellation deadline (e.g. 30s, 5m, 1h). Aborts the workflow when it expires; emits code: 'cancelled' on timeout. Effective only for steps whose model methods honor AbortSignal.", + "Cancellation deadline (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.", ) // @ts-expect-error - Cliffy custom type returns unknown instead of string .action(async function (options: AnyOptions, workflowIdOrName: string) { From 94289d9cac0e809a6511e859112ba3ce281446dc Mon Sep 17 00:00:00 2001 From: stack72 Date: Tue, 5 May 2026 14:27:27 +0100 Subject: [PATCH 3/3] fixup: --timeout accepts bare integer seconds for cross-command consistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new --timeout on `model method run` and `workflow run` originally required a unit-suffixed string ("30s", "5m", "1h"). But `swamp datastore sync --timeout` already ships as `` — users who transfer their knowledge type `--timeout 1800` and get a confusing parseDuration error that doesn't even mention `s` as a valid unit. Update parseTimeout to accept a bare integer as seconds (matching datastore sync). Suffixed forms still work: 30s, 5m, 1h, 1d, 1w. Update help text to call out both forms. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/cli/commands/model_method_run.ts | 2 +- src/cli/commands/workflow_run.ts | 2 +- src/cli/duration_parser.ts | 30 ++++++++++++++++++++++++---- src/cli/duration_parser_test.ts | 10 ++++++++++ 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/src/cli/commands/model_method_run.ts b/src/cli/commands/model_method_run.ts index f696d08a..b941c5ac 100644 --- a/src/cli/commands/model_method_run.ts +++ b/src/cli/commands/model_method_run.ts @@ -138,7 +138,7 @@ export const modelMethodRunCommand = new Command() ) .option( "--timeout ", - "Cancellation deadline (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.", + "Cancellation deadline — seconds (e.g. 30, 1800) or duration string (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.", ) .action( // @ts-expect-error - Cliffy custom type returns unknown instead of string diff --git a/src/cli/commands/workflow_run.ts b/src/cli/commands/workflow_run.ts index 714469c9..07e18157 100644 --- a/src/cli/commands/workflow_run.ts +++ b/src/cli/commands/workflow_run.ts @@ -129,7 +129,7 @@ export const workflowRunCommand = new Command() ) .option( "--timeout ", - "Cancellation deadline (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.", + "Cancellation deadline — seconds (e.g. 30, 1800) or duration string (e.g. 30s, 5m, 1h). Cooperative — only honored by methods that check AbortSignal.", ) // @ts-expect-error - Cliffy custom type returns unknown instead of string .action(async function (options: AnyOptions, workflowIdOrName: string) { diff --git a/src/cli/duration_parser.ts b/src/cli/duration_parser.ts index 0e3158a4..c9d71947 100644 --- a/src/cli/duration_parser.ts +++ b/src/cli/duration_parser.ts @@ -21,13 +21,34 @@ import { parseDuration } from "../libswamp/mod.ts"; import { UserError } from "../domain/errors.ts"; /** - * Parses a CLI timeout value like `30s`, `5m`, `1h`. Extends libswamp's - * `parseDuration` (which is data-retention oriented and starts at - * minutes) with second-level granularity needed for short-lived - * cancellations. Returns milliseconds, always > 0. + * Parses a CLI timeout value into milliseconds. Accepts: + * + * - A bare integer interpreted as seconds: `1800` → 1,800,000ms. Matches + * the convention used by `swamp datastore sync --timeout` so knowledge + * transfers across commands. + * - A `s` suffix for explicit seconds: `30s` → 30,000ms. + * - libswamp's `parseDuration` units (`m`, `h`, `d`, `w`, `mo`, `y`): + * `5m`, `1h`, `7d`, etc. + * + * Returns milliseconds, always > 0. Throws `UserError` on non-positive + * values or unrecognized formats. */ export function parseTimeout(value: string): number { const trimmed = value.trim(); + + // Bare integer → seconds (matches `swamp datastore sync --timeout`). + if (/^\d+$/.test(trimmed)) { + const seconds = parseInt(trimmed, 10); + if (seconds <= 0) { + throw new UserError( + `Invalid --timeout value "${value}": must be positive`, + ); + } + return seconds * 1000; + } + + // `s` → seconds. parseDuration starts at minutes, so we handle this + // case here before delegating. const secondsMatch = trimmed.match(/^(\d+)s$/); if (secondsMatch) { const seconds = parseInt(secondsMatch[1], 10); @@ -38,6 +59,7 @@ export function parseTimeout(value: string): number { } return seconds * 1000; } + const ms = parseDuration(trimmed); if (ms <= 0) { throw new UserError( diff --git a/src/cli/duration_parser_test.ts b/src/cli/duration_parser_test.ts index 98df5302..fc352196 100644 --- a/src/cli/duration_parser_test.ts +++ b/src/cli/duration_parser_test.ts @@ -21,6 +21,16 @@ import { assertEquals, assertThrows } from "@std/assert"; import { parseTimeout } from "./duration_parser.ts"; import { UserError } from "../domain/errors.ts"; +Deno.test("parseTimeout: bare integer is interpreted as seconds", () => { + // Matches `swamp datastore sync --timeout` convention. + assertEquals(parseTimeout("1"), 1000); + assertEquals(parseTimeout("1800"), 1_800_000); +}); + +Deno.test("parseTimeout: rejects non-positive bare integer", () => { + assertThrows(() => parseTimeout("0"), UserError, "must be positive"); +}); + Deno.test("parseTimeout: seconds", () => { assertEquals(parseTimeout("1s"), 1000); assertEquals(parseTimeout("30s"), 30_000);