Review: refactor confirmation-guarded mutations to libswamp pattern

Clean, well-structured refactor. The three-phase pattern (preview → prompt → execute) is consistent and the dependency injection via *Deps interfaces makes the generators testable. Architecture aligns well with DDD application service patterns — generators live in the application/libswamp layer and orchestrate domain objects.

No Blocking Issues

Suggestions

1. Missing renderer tests — The old src/presentation/output/*_test.ts files were deleted but no replacement tests were created for the four new renderers (data_gc.ts, model_delete.ts, vault_put.ts, workflow_delete.ts). Every other renderer in src/presentation/renderers/ has a corresponding _test.ts. The generator tests cover the core logic well, but the renderers have enough branching (log vs JSON mode, error handling) that tests would be valuable. Not blocking since the old tests' coverage is partially subsumed by the generator tests, and this is a refactor with identical output behavior.

2. workflowDelete generator doesn't re-validate the extension-only guard — The PR description says "generators re-validate preconditions… generators are self-contained." modelDelete does re-check workflow refs and data artifacts, but workflowDelete doesn't re-check pathExists (the extension-only guard). A direct caller bypassing the preview could delete an extension-only workflow. Consider adding the guard to the generator for consistency with the stated design.

3. ModelDeleteData dropped resourcePath — The old ModelDeleteJsonOutput conditionally included resourcePath in the JSON output. The new type and renderer omit it entirely. This is probably intentional (it was always undefined/false in practice), but worth confirming there are no downstream consumers expecting this field.

4. Error conversion pattern in CLI could be more precise — The catch blocks use "code" in (error as Record<string, unknown>) to detect SwampError. This would match any error object that happens to have a code property (e.g., Node/Deno system errors have code). A more precise check might use a type guard or check for the specific SwampError shape. Low risk in practice since the preview functions are well-defined.

5. promptConfirmation is duplicated across data_gc.ts, model_delete.ts, vault_put.ts, and workflow_delete.ts. Consider extracting to a shared CLI utility. Not blocking since this was pre-existing duplication.

Overall this is a solid refactor that cleanly separates concerns and improves testability. LGTM.

🤖 Generated with Claude Code

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. src/libswamp/workflows/delete.ts:122-150 — workflowDelete generator omits the extension-only guard, contradicting the stated "self-contained generator" design.

   The PR description explicitly states: "Generators are self-contained. A web UI adapter or API consumer might call the generator directly…" and "The library contract doesn't assume the preview ran first."

   modelDelete re-validates all preconditions (workflow refs, data artifact checks). But workflowDelete does NOT re-check the extension-only guard (pathExists). A non-CLI caller invoking workflowDelete directly (without calling workflowDeletePreview first) could delete an extension-only workflow's runs and evaluated data before deleteWorkflow either fails or succeeds in an unintended way — leaving a partially-deleted state.

   Breaking scenario: A future web UI adapter calls workflowDelete("ext-workflow") without calling workflowDeletePreview first. The generator deletes runs, deletes evaluated workflow, then calls deleteWorkflow which may throw (if the path doesn't exist on disk) — but runs are already gone.

   Suggested fix: Add the pathExists check inside the workflowDelete generator body, yielding an error event if the workflow is extension-only, matching the pattern used by modelDelete for its preconditions.

2. src/cli/commands/model_delete.ts:68-72, vault_put.ts:170-174, workflow_delete.ts:67-71 — Error discrimination via "code" in error is fragile.

   if ("code" in (error as Record<string, unknown>)) {
     throw new UserError((error as { message: string }).message);
   }

   This catches ANY error object with a code property — not just SwampError. Node-compatible system errors (e.g., ERR_* codes), DOMException, or any third-party error with a .code field would be silently swallowed into a UserError, losing the original stack trace and potentially exposing internal error messages to the user.

   Breaking scenario: A Deno.errors.PermissionDenied wrapping a Node-compat API that sets .code = "EACCES" would be caught here and re-thrown as a plain UserError("Permission denied") instead of propagating as a proper system error.

   Suggested fix: Use a dedicated type guard, e.g., check for a known SwampError code set or add a discriminant field like _swampError: true to the SwampError interface.

Low

1. src/libswamp/vaults/put.ts:91-94 — VaultPutInput.overwritten is caller-supplied, not re-verified by the generator. The generator trusts this display-only flag from the caller. A non-CLI consumer passing overwritten: false when the secret actually exists will produce inaccurate output metadata. The actual putSecret operation is unaffected, so this is cosmetic only.

2. src/libswamp/models/delete.ts:148-165 — Partial deletion on infrastructure failure. If deleteOutput succeeds for some outputs but deleteData or deleteDefinition throws, the model is left partially deleted with no rollback. This matches pre-refactor behavior and filesystem errors mid-loop are rare, so flagging only for awareness.

3. TOCTOU between preview and execute across all commands. State can change between the preview phase and the execute phase (e.g., new workflow references added, secrets created/deleted, new data artifacts). This is mitigated by design: modelDelete re-validates preconditions, dataGc delegates to the service which handles this internally, and vaultPut's put is an upsert. The only gap is workflowDelete (see Medium #1 above).

Verdict

PASS — This is a clean, well-structured refactor. The two medium findings are real but non-blocking: the workflow delete generator inconsistency is a design gap for future callers (not current CLI behavior), and the error discrimination pattern is fragile but works in practice with Deno's current error types. The code is consistent across all four commands, tests are thorough, and the preview/prompt/execute separation is sound.