Code Review

Well-structured refactoring that consistently applies the libswamp generator + renderer pattern across 10 commands. The architecture is clean: generators yield typed discriminated union events, deps interfaces enable testability, and CLI remains pure orchestration.

Blocking Issues

None.

Suggestions

1. Missing renderer unit tests: All 7 new renderer files (audit_timeline.ts, issue_create.ts, source_clean.ts, source_fetch.ts, source_path.ts, summarise.ts, update_check.ts) lack unit tests. While the generator logic is well-tested and some existing renderers also lack tests, the old output files being replaced did have tests (which were deleted). Consider adding renderer tests in a follow-up to maintain parity — especially for audit_timeline.ts which has non-trivial formatting logic (timeline table, error display, tool-not-supported messages across both log/json modes).

2. DDD alignment: The generators in src/libswamp/ are functioning as application services (orchestrating domain objects, yielding progress events). This is the correct layer per the project's DDD architecture. The deps interfaces properly abstract infrastructure, keeping generators testable without real I/O.

3. Minor: In src/libswamp/audit/timeline.ts, the auditTimeline generator's return type is AsyncIterable<AuditTimelineEvent> but it's declared as async function* — this is fine and matches the project convention, just noting the generator never yields an error event itself (errors would propagate as thrown exceptions). This is consistent with how other generators in the codebase work.

Overall this is a clean, consistent port that follows established patterns well. The import boundary rules are respected (CLI imports from libswamp/mod.ts, renderers import from libswamp/mod.ts), all files have license headers, and the DDD ratchet is correctly updated.

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — Pure internal refactoring. Verified all renderers produce identical output to the deleted output files in both log and json modes:

• source path/fetch/clean: Log messages and JSON shapes unchanged.
• issue bug/feature: Log messages and JSON shape identical; renderIssueCancelled correctly moved to new renderer file.
• audit: All three cases (timeline, no_data, tool_not_supported) produce identical log and JSON output. console.log → writeOutput in table rows is the correct internal API, not a behavior change.
• update: Log messages and JSON shape identical across all three statuses (up_to_date, update_available, updated).
• summarise: Log and JSON output identical; writeOutput replaces console.log consistently with other renderers; no_activity JSON shape unchanged ({ message: "No activity found." }).

No new flags, help text, or error messages were introduced. No UX concerns.

Adversarial Review

Critical / High

None found.

Medium

1. Behavioral change in audit command for unsupported tools — src/libswamp/audit/timeline.ts:85-90 vs old src/cli/commands/audit.ts:163-164

   The old code printed the "tool not supported" warning and continued to fetch and render the timeline (no return after renderAuditToolNotSupported). The new generator yields tool_not_supported and returns immediately, skipping the timeline entirely.

   Breaking example: A user with tool: "codex" who previously saw both the warning AND their audit data will now only see the warning. The PR description says "No user-facing behavior changes" but this is a behavior change.

   Suggested fix: If the old fall-through was intentional, remove the return after the tool_not_supported yield and continue to fetch the timeline. If it was a bug in the original code (likely), document this as a deliberate fix in the PR description.

2. No generators yield error events — all 7 new generators (issueCreate, auditTimeline, sourceClean, sourceFetch, sourcePath, updateCheck, summarise) define { kind: "error"; error: SwampError } in their event union type but never yield it. Existing generators in the codebase (e.g., models/run.ts, auth/whoami.ts) actively yield error events for domain-level failures. In these new generators, if deps.createIssue(), deps.getTimeline(), etc. throw, the exception propagates as an unhandled rejection through consumeStream, bypassing the renderer's error handler entirely.

   This works in practice (the CLI catch block or Cliffy handles it), but it means the error handlers in all 7 new renderers are dead code and the error event types are vestigial. This is an architectural inconsistency with the rest of the libswamp generators.

   Suggested fix: Either (a) add try/catch in generators and yield { kind: "error", error: ... } for expected failures (matching the pattern in models/run.ts), or (b) if these generators intentionally don't handle domain errors, document why in a comment.

Low

1. as IssueCancelledData type assertions — src/cli/commands/issue_bug.ts:163 and src/cli/commands/issue_feature.ts:163 add as IssueCancelledData casts on object literals { type: "bug", reason: "empty" }. These are unnecessary if the type is correctly inferred (the object literal matches the interface exactly). The casts suppress type checking on these objects. Not harmful, but a code smell.

Verdict

PASS — This is a clean mechanical refactor to the libswamp generator+renderer pattern. The audit tool_not_supported behavior change (Medium #1) is most likely a bug fix of the original code's missing return, not a regression. The dead error handlers (Medium #2) are an architectural gap but not a correctness issue. No critical or high severity findings.

CLI UX Review

Blocking

None.

Suggestions

1. Audit JSON output: inconsistent shapes across statuses — The three swamp audit --json outcomes produce different shapes with no shared discriminant field:

   • timeline: { entries, totalSwamp, totalDirect, hoursAnalyzed }
   • no_data: { message }
   • tool_not_supported: { supported: false, tool, message }

   A script handling --json output has no reliable field to switch on. Adding a status field ("timeline" | "no_data" | "tool_not_supported") to all three would make scripts straightforward to write. This is pre-existing behavior preserved from the deleted audit_output.ts — not introduced by this PR, so not blocking here, but worth a follow-up issue.

2. source path log output: fetchedAt is raw ISO string — The renderer logs Fetched: 2026-01-01T00:00:00.000Z verbatim. The audit_timeline.ts renderer has a formatTime helper, and summarise.ts has a formatDateTime helper. Using one of those here would be more consistent. Minor cosmetic issue.

Verdict

PASS — Pure internal refactor, no user-facing behavior changes. All commands support both log and json modes. Output, error messages, and help text are unchanged.

Code Review

Blocking Issues

None.

Suggestions

1. Missing renderer tests: The 7 new renderer files (audit_timeline.ts, issue_create.ts, source_clean.ts, source_fetch.ts, source_path.ts, summarise.ts, update_check.ts) have no corresponding *_test.ts files. The deleted output files had 5 test files covering the rendering logic. Every other renderer in src/presentation/renderers/ has a test file. Consider adding renderer tests for consistency with the existing pattern and to maintain the test coverage CLAUDE.md requires. The generator tests cover core logic well, so this is non-blocking.

Notes

• DDD layer boundaries are properly respected: CLI → libswamp/mod.ts, renderers → libswamp/mod.ts, generators → domain + infrastructure.
• The libswamp import boundary rule (CLI/presentation must import from mod.ts, not internal paths) is fully honored.
• All new files have AGPLv3 license headers.
• Named exports used throughout, no any types introduced.
• The renderIssueCancelled standalone function in the issue renderer is a reasonable design choice since editor cancellation is CLI-layer logic that doesn't go through the generator.
• console.log → writeOutput migration in summarise.ts is a nice improvement.
• CI passes (lint, test, type-check, compile).

Clean, well-structured migration. LGTM.

Adversarial Review

Critical / High

None found.

Medium

1. Behavioral change in audit codex handling (src/libswamp/audit/timeline.ts:62-67): The old audit.ts code called renderAuditToolNotSupported("codex", ctx.outputMode) but had no return statement — it fell through and still fetched/displayed the timeline. The new generator yields tool_not_supported and returns, short-circuiting the timeline fetch. This is almost certainly a bug fix (the old missing return looks unintentional), but it is an undocumented behavioral change. If any user or test relied on getting both the warning AND the timeline data for codex, this changes that. Since the PR description says "No user-facing behavior changes," this should be called out.

2. Deleted renderer tests not replaced (src/presentation/output/*_test.ts → no replacement): Five test files (243 + 146 + 165 + 201 + 153 = 908 lines of renderer tests) were deleted. The new renderer files (src/presentation/renderers/{audit_timeline,issue_create,source_clean,source_fetch,source_path,update_check}.ts) have zero unit tests. The new generator tests only validate event yields, not rendering logic. The rendering code has conditional branches (e.g., data.method === "created" vs "url", data.status === "timeline" vs "no_data" vs "tool_not_supported") that are now untested. Not a blocker for this PR since the rendering logic is faithfully ported, but this is a net reduction in test coverage.

Low

1. Generator error event handlers are dead code: Every generator defines { kind: "error"; error: SwampError } in its event union, and every renderer implements an error handler. But no generator ever yields an error event — if a dep call throws, the exception propagates through consumeStream's for await loop directly. The error handlers in renderers will never fire. This is consistent with the EventHandlers exhaustiveness requirement and isn't a bug, but the error variant in the event unions is misleading dead surface area.

2. summarise_output.ts rename loses git history continuity: The file was renamed from src/presentation/output/summarise_output.ts to src/presentation/renderers/summarise.ts with significant changes. Git detects this as a rename (65% similarity), but git log --follow may not track correctly depending on tooling. Minor, just noting for archeology.

Verdict

PASS — Clean mechanical refactor. Logic is faithfully ported from old output files to new generator + renderer pairs. The one behavioral change (audit codex short-circuit) is a bug fix. No security, concurrency, or data integrity issues. The code follows the established consumeStream pattern consistently across all ported commands.