fix(reports): persist fallback error artifact when report execute() throws (swamp-club#365)

[stack72]

Summary

• When a report's execute() throws, swamp now persists a built-in fallback error artifact (markdown + JSON) instead of silently discarding the output
• Workflow status and exit code are unchanged — report failures remain advisory (a broken report must not fail an infra workflow with downstream success triggers)
• The WRN log now reads "Report X failed, fell back to default error report: ..." so users understand what happened
• Error artifacts are discoverable via swamp data get report-<name>-json with { error: true, reportName, scope, message }

Closes swamp-club#365

Test plan

• deno check passes
• deno lint passes
• deno fmt --check passes
• deno run test — 5959 passed, 0 failed
• deno run compile succeeds
• Verified against reproduction: workflow with throwing report shows "succeeded", exit 0, and error artifact persisted at report-<name>-json
• Verified swamp data get --workflow <wf> report-<name>-json returns { error: true, ... }

🤖 Generated with Claude Code

[github-actions]

Code Review

Clean, well-scoped PR. The approach of persisting fallback error artifacts rather than silently discarding output is sound. The advisory semantics (no workflow failure, no exit code change) are correctly preserved throughout. DDD alignment is good — buildReportErrorResult is a pure domain function producing a value object (ReportResult), correctly placed in builtin/ alongside other built-in report logic.

Blocking Issues

None.

Suggestions

1. Duplicate imports — src/domain/reports/builtin/report_error_report.ts:20-21 has two separate import type lines from "../report.ts". Could be combined into import type { ReportResult, ReportScope } from "../report.ts";. Minor style nit.

2. Silent catch in fallback persistence — src/domain/reports/report_execution_service.ts line ~449 has } catch { that silently swallows errors when the fallback artifact itself fails to persist. The design doc acknowledges this is intentional, but a debug-level log here (consistent with the runFailed catch blocks in method_report_runner.ts:313 and workflow_report_runner.ts:163) would make failures more diagnosable without masking the original error.

Both are non-blocking. Test coverage is thorough — unit tests for the builder function, integration tests for the executeReports error fallback path including data handle propagation and callback wiring. License headers present on new files. Import boundaries respected (domain-internal imports only, presentation renderers import from libswamp/mod.ts).

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Warning message could include the data key — src/presentation/renderers/model_method_run.ts:158 and workflow_run.ts:173 now emit "Report {reportName} failed, fell back to default error report: {error}". This is already much better than the old → ✗ format, but the message doesn't tell users where the fallback data lives. Something like "…fell back to default error report — retrieve with: swamp data get report-{name}-json: {error}" would make the warning immediately actionable without needing to consult docs.

2. JSON mode silently drops report failures — JsonModelMethodRunRenderer and JsonWorkflowRunRenderer both have report_failed: () => {}. This is pre-existing behavior, not introduced here, but the new fallback data handles are now surfaced in log mode via the warning while JSON mode gets nothing. A follow-up could expose the error artifact names in JSON output when report failures occur.

Verdict

PASS — the warning message improvement is clear and accurate, the fallback artifact is correctly persisted and documented, and the behavioral change (exit code / workflow status unchanged) is consistent with advisory-report semantics.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. src/presentation/renderers/model_method_run.ts:158 and workflow_run.ts:173 — log message claims fallback succeeded even when it didn't. The warn message reads "Report {reportName} failed, fell back to default error report: {error}" unconditionally, regardless of whether the fallback artifact was actually persisted. In report_execution_service.ts:449, the inner catch silently absorbs persistence failures, leaving errorDataHandles as undefined. The renderer has no way to distinguish "fallback persisted" from "fallback also failed" — it always claims the fallback happened. This produces misleading diagnostics when persistence is broken (e.g., disk full). Suggested fix: either split into two log messages (one for fallback-succeeded, one for fallback-also-failed) by including dataHandles on the report_failed event, or soften the message to "Report {reportName} failed (error artifact may have been persisted): {error}".

2. src/domain/reports/report_execution_service.ts:431-451 — partial artifact persistence on the success→catch transition. If report.execute() succeeds but persistReportData (line 399) throws after writing the markdown artifact but before writing the JSON artifact, we enter the catch block. The fallback persistReportData call (line 439) then overwrites the markdown with error content, which is correct. However, if the fallback persist also fails (caught at line 449), the success-path markdown artifact remains in the data store while the result says success: false with no dataHandles. A consumer querying report-{name} by name would find the successful markdown content despite the result reporting failure. This is a very unlikely double-failure scenario but worth documenting in the design doc.

Low

1. src/domain/workflows/method_report_runner.ts:255 — runFailed path discards error fallback data handles. The _dataHandles parameter is deliberately ignored, and runFailed returns []. The artifacts are still persisted to the data store and discoverable by name, so this is functionally correct. But it means error fallback artifacts from reports-on-a-failed-method won't appear in the step's DataArtifactRef[] list, while error fallback artifacts from reports-on-a-succeeded-method (line 150) will. This asymmetry is a deliberate design choice per the comments, but worth noting for future maintainers.

2. src/domain/reports/builtin/report_error_report.ts:35 — raw errorMessage interpolated into markdown. If the error message contains markdown syntax (headings, links, code fences), it will be interpreted as markdown structure. This is purely cosmetic since the content is rendered in a terminal, not a browser, and error messages are internal strings — but a multi-line error with --- could produce unexpected horizontal rules in the rendered output.

Verdict

PASS. The core logic is correct: error fallback artifacts are persisted, the workflow status and exit code are unaffected, the onReportFailed signature change is backwards-compatible (optional parameter), and all three code paths (workflow runner, method runner success, method runner failure) handle the new data handles appropriately. Tests cover the key scenarios. The medium findings are edge cases in unlikely failure paths, not correctness bugs in normal operation.