Code Review

Clean, well-scoped change that solves the empty-stdout problem for pipe consumers in --json mode.

Blocking Issues

None.

Suggestions

1. Missing test coverage for unknownCommandErrorHandler JSON path (src/cli/unknown_command_handler.ts:171-175): The new JSON output branch in unknownCommandErrorHandler isn't tested. This is understandable since the function reads Deno.args directly and calls Deno.exit(2), making it hard to unit test. Consider extracting the JSON-writing logic into a testable helper in a future PR, or adding a subprocess-based integration test. The core buildErrorJson function is well-tested, so this is non-blocking.

2. isCliffyMissingArgError duplication: The check logic for Cliffy missing-arg errors is effectively duplicated between renderError (which delegates to LogTape) and buildErrorJson. Both paths need it, so this is reasonable, but if this grows more complex, consider a shared predicate. Fine as-is.

Overall: good test coverage (6 new tests), correct layer placement per DDD (presentation concern stays in presentation), no libswamp import boundary violations, and the OutputMode type is used consistently.

CLI UX Review

Blocking

None.

Suggestions

1. Double output in interactive terminals — In JSON mode, errors now appear twice when running in a terminal: once on stdout (from renderError's new console.log) and once on stderr (from LogTape's createJsonErrorSink). A user running swamp bad-command --json interactively sees two { "error": ... } blocks in their terminal. This is a side-effect of the dual-stream design. It may look like a bug to someone who doesn't know the intent. A brief note in the CLI help or --help output for --json mode that errors go to both stdout and stderr would help discoverability, but it's not blocking.

2. Unknown command error wraps message in a new Error — In unknownCommandErrorHandler, the formatted human-readable message (with suggestions) is passed to buildErrorJson(new Error(message)). That message will include the suggestion text ("Did you mean X?"), which is useful. However, the resulting JSON won't have a stack field (because the new Error will have a stack, and it's not a UserError or Cliffy missing-arg error). Wait — actually it WILL include stack because it's new Error(message) which is neither a UserError nor a Cliffy error. The stack will point into unknownCommandErrorHandler which is internal noise. Minor, not blocking.

Verdict

PASS — Clean, focused fix that makes error output useful for pipe consumers (jq, AI agents) in --json mode. JSON shape { "error", "stack"? } matches the existing logging layer format. Exit codes are unchanged. No flags added, no help text needed.

Code Review

Blocking Issues

None.

Suggestions

1. Duplicated report-execution block in src/libswamp/models/run.ts (lines 459-528): The failure-path report execution is a near-copy of the success-path block (lines 601-675). Consider extracting a shared helper (e.g. yieldReportEvents(summary)) to reduce duplication and ensure both paths stay in sync as the report system evolves. Not blocking since both paths are correct today.

2. run.ts has no unit tests: The new 72-line failure-path report block is a meaningful logic addition (conditional report execution, event yielding) but run.ts has no test file. The report rendering and error formatting are well-tested elsewhere, so risk is low, but a test for the "reports run on failure" path would increase confidence.

3. Minor: errorMessage not plumbed to ModelReportContext: report_context.ts adds errorMessage? to both MethodReportContext and ModelReportContext, but only the method-scope failure path sets it. If model-scope reports ever run on failure in the future, the field on ModelReportContext will be unused until then — fine for now, just noting for awareness.

Overall this is clean, well-tested work. The error JSON format is consistent with the existing createJsonErrorSink pattern, the buildErrorJson helper is properly exported and tested, license headers are present, no libswamp import boundary violations, and the DDD layering is respected (domain types stay in domain, presentation logic stays in presentation, application orchestration in libswamp).

CLI UX Review

Blocking

None.

Suggestions

1. unknownCommandErrorHandler: stack trace in JSON output for user-facing errors
   File: src/cli/unknown_command_handler.ts, line 172.
   buildErrorJson(new Error(message)) creates a plain Error, so buildErrorJson sees it as a system error and includes a stack field. But the message ("Unknown command "foobar". Did you mean "foo"?") is entirely user-facing — the stack just points to internal handler code and is not actionable.
   Consider buildErrorJson(new UserError(message)) here to suppress the stack, matching the same logic already applied in renderError for validation errors.

Verdict

PASS — the core fix is correct: errors now go to stdout as structured JSON in --json mode, the JSON shape { error, stack? } is consistent with createJsonErrorSink on stderr, and the method summary report correctly surfaces error in both markdown and JSON output for failed runs.

Adversarial Review

Critical / High

None found.

Medium

1. src/libswamp/models/run.ts:484 — executeReports in catch block is unguarded, could mask the original error.

   If executeReports throws at the framework level (e.g., registry.getAll() or filterReports hits an unexpected state), the exception propagates out of the catch block. The yield { kind: "error", error: methodExecutionFailed(error) } at line 538 is never reached, so the consumer sees a report-infrastructure error instead of the actual method execution failure.

   Breaking example: If the report registry is somehow corrupted (e.g., a custom report's scope getter throws), the consumer never learns why the method failed — they get an unrelated crash.

   Suggested fix: Wrap the report block in its own try/catch:

   try {
     if (!input.skipAllReports && reportRegistry.getAll().length > 0) {
       // ... existing report execution code ...
     }
   } catch {
     // Report generation failed — log but don't mask the original error
   }

   Mitigating factor: The success path at line 601 has the same pattern (no try/catch around executeReports), so this is a pre-existing design choice, not a regression. Still worth noting since the failure path is where you most want error propagation to be robust.

2. src/domain/reports/builtin/method_summary_report.ts:88 — errorMessage is injected into markdown unsanitized.

   lines.push("", "## Error", "", errorMessage);

   If errorMessage contains markdown syntax (e.g., # Heading or [link](http://...)), it will be rendered as markdown structure, potentially breaking the report layout.

   Breaking example: An error message like "Expected ## items but got 0" would render as an H2 heading in the report.

   Suggested fix: Wrap in a code fence:

   lines.push("", "## Error", "", "```", errorMessage, "```");

Low

1. main.ts:41 + src/cli/unknown_command_handler.ts:174 — Potential for double JSON on stdout.

   If a command has already written partial JSON to stdout before throwing (e.g., a streaming JSON command fails mid-output), renderError appends a second JSON object. A pipe consumer like jq would see invalid JSON. This is inherent to the "error JSON on stdout" design and unlikely in practice since main.ts catch handles pre-CLI errors, but worth documenting for future maintainers.

Verdict

PASS — The core changes are well-structured and consistent. The error JSON output path is clean, buildErrorJson correctly distinguishes user errors from system errors, and the test coverage is thorough. The medium findings are edge cases in uncommon paths.

Adversarial Review

Critical / High

None found.

Medium

1. src/libswamp/models/run.ts:484 — If executeReports throws before its internal try/catch loop, the original error is lost.

   executeReports has internal error handling for individual report executions, but the preamble code (filterReports, buildRedactSensitiveArgs, context.redactSensitiveArgs = ...) runs outside that try/catch. If any of those throw (e.g., modelRegistry.get() fails or filterReports receives corrupt selection data), the exception propagates out of executeReports, then out of the outer catch block in run.ts — the generator throws instead of yielding the { kind: "error" } event on lines 534-539. The caller never sees the original execution failure.

   Breaking example: A corrupt definition.reportSelection causes filterReports to throw a TypeError. The catch block's remaining code (the AbortError check + yield { kind: "error" }) is skipped. The consumer of the generator gets an unrelated TypeError instead of the structured methodExecutionFailed error.

   Suggested fix: Wrap the executeReports call and its yield loop in a try/catch that logs and swallows report failures, so the original error path always completes:

   try {
     const failedSummary = await executeReports(...);
     for (const result of failedSummary.results) { ... }
   } catch (reportError) {
     // Report generation failed — log but don't mask the original error
     getRunLogger(definition.name, input.methodName).warn`Failed to generate reports for error path: ${reportError}`;
   }

2. src/libswamp/models/run.ts:459-529 — Failed path runs only method-scope reports, success path runs both method-scope and model-scope.

   The success path (line 676) creates a ModelReportContext and calls executeReports a second time for model-scope reports. The failure path skips model-scope reports entirely. If any model-scope reports are registered that should also render on failure (e.g., a model-level error summary), they won't run. This may be intentional (the PR only targets method-summary), but it's an asymmetry that could surprise future report authors.

Low

1. src/domain/reports/builtin/method_summary_report.ts:88 — errorMessage is injected into markdown without escaping.

   If the error message contains markdown syntax (e.g., # heading, [link](url), or | table |), it will be rendered as markdown structure rather than literal text. In practice, most error messages are plain text, so this is unlikely to cause real problems, but an error like `rm -rf /` failed with code 1 would render the backtick portion as inline code rather than literal text. A fenced code block would be more robust.

2. PR title/body mismatch with actual changes. The PR title says "write error JSON to stdout in --json mode" and the body describes renderError(), unknownCommandErrorHandler, and buildErrorJson() — none of which appear in the diff. The actual change is "generate method-summary report on failed runs." The commit message is accurate, but the PR metadata is misleading.

Verdict

PASS — The core logic is correct and well-tested. The executeReports-can-throw issue (Medium #1) is the most concerning but is mitigated by the fact that executeReports is designed to be robust internally, and the preamble functions are unlikely to throw in practice. The success/failure path asymmetry (Medium #2) is worth a deliberate decision but doesn't block.

CLI UX Review

Blocking

None.

Suggestions

1. Raw error message in markdown output — method_summary_report.ts:88: The errorMessage is pushed into the markdown lines without escaping:

   lines.push("", "## Error", "", errorMessage);
   

   If an error message starts with # or contains |, it will render as a heading or table row in markdown viewers, potentially mangling the report layout. Wrapping in a code fence (\n...) would make the error message visually distinct and safe regardless of content.

2. Error section position — The "## Error" section appears between "## Arguments" and "## Data Output". For a failed run, "## Data Output" shows "No data output." every time, which is low-value noise after an error. Moving error to the end, or omitting "## Data Output" when status is failed, would keep the reader's attention on the failure reason.

Verdict

PASS — The core UX improvement is solid: failed runs now emit a method-summary report with a clear error message in both markdown and JSON. The error field in JSON output ({ status, error, modelId, ... }) is self-evident and directly actionable for pipe consumers. Behavior is consistent with the success path.

Code Review

Blocking Issues

None.

Suggestions

1. PR description doesn't match the diff — The body mentions renderError(), buildErrorJson(), unknownCommandErrorHandler, and getOutputModeFromArgs, none of which appear in the actual changes. The description should be updated to reflect what was actually implemented (error message propagation through report contexts and failure-path report execution).

2. Failure path skips model-scope reports — The success path runs both method-scope and model-scope reports (lines ~676-728), but the failure path only runs method-scope. If model-scope reports would be useful for failed executions, consider adding them for consistency.

3. Failure path doesn't collect reportResults — The success path stores report results in reportResults and passes them to the ModelMethodRunView. The failure path only yields events without collecting results. Since the error event type doesn't carry report data this is fine for now, but if consumers need structured report results from failed runs, this would need to change.

4. Code duplication opportunity — The report execution + event yielding pattern (create context → executeReports → yield events) is now duplicated between the failure path (lines ~459-529) and success path (lines ~601-728). A shared helper could reduce this, though it's not critical at ~70 lines.

Overall the changes are clean, well-tested at the report rendering level, and properly extend the domain model. The DDD patterns are followed correctly — optional errorMessage on context interfaces is the right approach, and the application-layer wiring in run.ts is appropriate.