CLI UX Review

Blocking

None.

Suggestions

1. auth_login renderer: console.log in browser_open_failed handler
   src/presentation/renderers/auth_login.ts:173
   The browser_open_failed handler calls console.log(e.message) directly while all other event handlers use the writeOutput helper or the Spinner. Minor inconsistency — should use writeOutput for uniform output routing.

2. auth_login JSON output omits name and email
src/presentation/renderers/auth_login.ts:216–224
   Log mode shows Name and Email when available; JSON mode only emits { authenticated, serverUrl, username }. Scripts querying user identity can't get the same info without switching to log mode. Consider including optional name/email fields (the API key is correctly excluded).

3. extension_push --json produces two sequential JSON objects
   src/presentation/renderers/extension_push.ts:244–295
   A successful push emits two separate JSON blobs to stdout: first from renderResolved (resolved data), then from the completed handler (push result). --dry-run --json similarly emits renderResolved + renderDryRun. If this is the intended streaming/NDJSON pattern (consistent with extension_update which also emits multiple objects), no change needed. If not, consider a single top-level JSON envelope.

4. extension_update --json double-outputs on extension_not_installed
src/presentation/renderers/extension_update.ts:69–74
   In JSON mode, the extension_not_installed handler emits { error: "..." } to stdout, and then the error event handler immediately throws a UserError with a similar message. A script sees a JSON blob AND a thrown error — two outputs for one failure condition. The extension_not_installed JSON handler could be a no-op and let the error event carry the structured failure.

Verdict

PASS — this is a clean internal refactor. Command flags, help text, error messages, and log/JSON output shapes are preserved or improved. No blocking UX regressions.

Code Review

Well-structured refactoring that consistently ports 5 commands to the established generator + renderer + CLI orchestration pattern. The separation of concerns is clean: generators own domain orchestration with deps injection for testability, renderers handle log/json output, and CLI commands are reduced to pure wiring.

Blocking Issues

None.

Suggestions

1. Duck-typed isSwampError guard in extension_push.ts:415-418 — The check "code" in error && "message" in error would match any Error subclass with a code property (e.g., Node-style errors). Consider adding a discriminant field to SwampError (e.g., readonly _tag: "SwampError") or exporting isSwampError from src/libswamp/errors.ts so all CLI commands use a canonical guard. The same duck-typing appears in extension_yank.ts:87 with a slightly different pattern.

2. as casts on error.details in extension_push.ts:280-310 — The catch block dispatches on details?.safetyErrors, details?.qualityErrors, etc. using as casts from unknown. This works but is fragile if a new error detail key is added without updating this block. A discriminated union on SwampError.code (matching the error code to its expected details shape) would be more type-safe. Low priority since this is CLI-layer glue code.

3. Duplicate resolveServerUrl() functions — push.ts, update.ts, and yank.ts in src/libswamp/extensions/ each define their own resolveServerUrl() with identical logic. Could be extracted to a shared infrastructure helper. Not blocking since this matches the pre-existing pattern.

4. No renderer tests for the new renderers — The old src/presentation/output/*_test.ts files are deleted but no replacement tests exist for the new renderer classes in src/presentation/renderers/. The generators are well-tested, and the renderers are thin, but renderer tests would catch regressions in JSON output shape (which external consumers may depend on). Worth adding in a follow-up.

DDD Assessment

The generator+deps pattern aligns well with the Application Service building block — each generator orchestrates domain objects for a use case, with infrastructure injected through the deps interface. The two-phase pattern for extensionPush (plain async prepare + generator execute) is a reasonable split given the interactive prompts between phases. Domain types (ExtensionManifest, SafetyCheckResult, QualityCheckResult) remain in the domain layer and are not duplicated.

Adversarial Review

Critical / High

None found.

Medium

1. src/libswamp/extensions/yank.ts:140-146 — Uncaught exception from deps.yankExtension() bypasses renderer error handling.
   The extensionYank generator awaits deps.yankExtension() without a try/catch. If the API call fails (network timeout, 403 Forbidden, 500 Server Error), the exception propagates out of the generator and through consumeStream, bypassing the renderer's error handler entirely. In --json mode, consumers would get a raw error message instead of structured JSON output.
   Breaking example: Server returns 403 during yank → CLI outputs an unstructured stack trace instead of {"error": "..."}.
   Suggested fix: Wrap the deps.yankExtension() call in try/catch and yield { kind: "error", error: validationFailed(...) }, matching the pattern used in extensionPush generator (lines 660-674, 679-689, 694-709).

2. src/cli/commands/extension_push.ts:415-419 — isSwampError duck-type guard is overly permissive.
   The guard checks "code" in error && "message" in error. Since SwampError is a plain object (not an Error subclass), this would also match any non-Error object that happens to have both properties — e.g., HTTP client error objects from third-party libraries that include { code: "ECONNRESET", message: "..." }. If a network error from deps.fetchCollectives or deps.extractContentMetadata matches this guard, the code enters the SwampError handling branch, tries to inspect details, finds nothing, and falls through to throw new UserError(error.message) — which is a reasonable fallback. However, the details branching logic (safetyErrors, qualityErrors, etc.) could accidentally fire if an unrelated error object happens to have a matching details key.
   Suggested fix: Add a discriminant field (e.g., _tag: "SwampError") to the SwampError interface and check for it, or use instanceof with a class.

3. src/libswamp/auth/login.ts:227-231 — server.shutdown() error in finally masks the original server.token rejection.
   If server.token rejects (e.g., auth timeout) AND server.shutdown() also throws (e.g., the server is already closed), the finally block's error replaces the original rejection. The user sees a cryptic "server already closed" error instead of "authentication timed out".
   Suggested fix: Wrap server.shutdown() in a try/catch within the finally block, similar to the createArchive cleanup pattern at push.ts:1052-1056.

4. src/presentation/renderers/extension_update.ts:79-86 — JSON renderer emits multiple top-level JSON objects in the updating + completed sequence.
   The updating handler emits {"status":"updating",...} for each extension, then completed emits the full result. A consumer doing JSON.parse(stdout) would fail on multi-extension updates. This differs from other renderers that emit a single JSON document.
   Suggested fix: Either suppress the updating handler in JSON mode (emit nothing, let completed be the sole output) or collect into an array/JSONL explicitly.

Low

5. src/cli/commands/extension_yank.ts:87 — Weaker duck-type check: "code" in (error as Record<string, unknown>).
   This only checks for code (not message), making it even more permissive than the push guard. Any thrown object with a code property would be treated as a SwampError. Same suggested fix as #2.

6. src/libswamp/extensions/push.ts:1020-1023 — Additional files with the same basename silently overwrite each other in the archive.
   basename(af) is used as the destination, so foo/README.md and bar/README.md would collide. This is carried over from the original code and not a regression, but worth noting.

Verdict

PASS — This is a clean refactoring that correctly ports five commands to the libswamp generator/renderer pattern. The code is well-structured with proper dependency injection and good test coverage. The medium findings are edge cases in error handling paths, not logic errors in happy paths. None warrant blocking the merge.