feat(doctor): add swamp doctor workflows subcommand (swamp-club#330)

[stack72]

Summary

• Add swamp doctor workflows subcommand that walks all workflow directories (primary, extension, source-mounted, pulled) and checks whether each YAML file can be parsed and constructed into a valid Workflow domain object
• Catches YAML syntax errors and schema construction failures that YamlWorkflowRepository.findAll() silently skips — meaning swamp workflow validate never sees these broken files
• Supports both --json (structured output for CI) and log (colored pass/fail per file) output modes; exits non-zero on any failure
• Updates the swamp-troubleshooting skill health-checks reference with full documentation

Test Plan

• 8 unit tests for the libswamp generator covering: valid workflows, YAML parse errors, schema validation failures, missing directories, non-YAML files, multiple directories, and mixed pass/fail
• 7 unit tests for the renderer covering: JSON and log mode output, overallStatus tracking, empty reports, and workflow-checked event handling
• Verified end-to-end with compiled binary against a scratch repo containing valid, broken-YAML, and schema-invalid workflow files
• deno check, deno lint, deno fmt all pass
• Full test suite: 5888 passed, 1 pre-existing failure (unrelated integration/workflow_test.ts)

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Summary line inconsistency with doctor extensions (src/presentation/renderers/workflow_doctor.ts): doctor extensions renders the final line as 0 passed, 5 failed — OVERALL: FAIL, while doctor workflows renders two lines: Summary: 2/3 workflows loaded successfully then Result: PASSED. Neither is wrong, but the inconsistency means users who run both commands see different summary idioms. Consider aligning toward the same N passed, M failed — OVERALL: PASS/FAIL shape for future consistency.

2. "file" field in JSON output is absolute, but docs show relative (.claude/skills/swamp-troubleshooting/references/health-checks.md line ~81): The skill reference shows "file": "workflows/workflow-abc.yaml" (repo-relative), but the actual implementation builds filePath = \${dir}/${entry.name}`wheredir` is an absolute path. A CI consumer parsing the JSON will see an absolute path. The doc example is slightly misleading; worth correcting to a realistic absolute path or noting that the value is absolute.

3. Empty-report log path has double blank line before Result: (workflow_doctor.ts, completed handler empty branch): The non-empty branch emits \n${bold(cyan("Summary:"))} ... (one blank line before summary, no blank line before Result:). The empty branch emits \n No workflow files found then \n${bold(cyan("Result:"))} ... — giving two \n-prefixed lines in a row, which produces an extra blank line before Result:. Cosmetic only.

Verdict

PASS — both output modes are correctly implemented, exit codes are right, help text and examples are present and consistent, and the command fills a real gap (findAll() silently drops broken files that workflow validate never sees).

[github-actions]

Code Review

Well-structured PR that follows the existing doctor subcommand pattern closely. The architecture is clean: libswamp async generator → CLI command → renderer, with proper separation of concerns. All files have the license header, imports from libswamp/mod.ts are correct, and both output modes are supported.

Blocking Issues

None.

Suggestions

1. Double YAML parse in src/libswamp/workflows/doctor.ts:126-129 — tryExtractName() calls parseYaml(content) and then line 129 calls parseYaml(content) again for the same file. You could restructure to parse once and use the result for both name extraction and Workflow.fromData(), falling back to fallbackName() only when the first parse throws. Not blocking since the extra parse is harmless for a diagnostic tool.

2. Path construction via string interpolation (src/libswamp/workflows/doctor.ts:107) — const filePath = \${dir}/${entry.name}`uses a forward-slash literal instead ofjoin(dir, entry.name)from@std/path. CLAUDE.md says to use @std/pathfor all path operations and never hand-roll with"/". basenameis already imported; addingjoinis trivial. This is a minor cross-platform concern — on Windowsdir` could have backslash separators, producing mixed-separator paths.

3. Test cleanup pattern — The tests use try/finally with bare await Deno.remove(tmpDir, { recursive: true }) for cleanup. The CLAUDE.md testing section notes that withTempDir cleanup uses .catch(() => {}) to absorb EBUSY on Windows, and the sibling doctor_repair_test.ts follows this pattern. Consider adding .catch(() => {}) to the Deno.remove calls or using a withTempDir helper to match the established convention.

4. Naming consistency — The new renderer file is workflow_doctor.ts while the existing one is doctor_extensions.ts (noun order reversed). Not worth changing now, but worth noting for future doctor subcommands.

All suggestions are minor and don't block merge. The core logic, test coverage (8 + 7 tests), DDD layering, and CI integration are solid.

[github-actions]

Adversarial Review

Critical / High

1. src/libswamp/workflows/doctor.ts:107 — Path construction uses string interpolation instead of @std/path.join()

   const filePath = `${dir}/${entry.name}`;

   The file already imports basename from @std/path but does not import join. The project's CLAUDE.md explicitly mandates: "Use @std/path (dirname, basename, join, fromFileUrl, SEPARATOR) for all path operations. Never hand-roll with lastIndexOf("/"), split("/").pop(), URL.pathname, or "/"-prefixed concatenation." The sibling extensions/doctor.ts correctly uses join from @std/path (line 21).

   Breaking example: On Windows, dir arrives from resolve() with \ separators (e.g. C:\repos\workflows). The template literal produces C:\repos\workflows/my-workflow.yaml — a mixed-separator path. This path is both passed to Deno.readTextFile (which may handle it, but it's fragile) and emitted in the user-facing DoctorWorkflowResult.file field, where it appears in log output and --json reports. Users trying to copy-paste that path on Windows will get inconsistent results.

   Fix: Add join to the existing @std/path import and replace line 107 with:

   const filePath = join(dir, entry.name);

Medium

1. src/libswamp/workflows/doctor.ts:95-98 — Non-NotFound readDir errors crash the generator with an unformatted stack trace

   Only Deno.errors.NotFound is caught and skipped. A PermissionDenied error on a workflow directory (plausible for source-mounted or pulled extension dirs with wrong permissions) would throw through the async generator, crash consumeStream, and produce a raw stack trace instead of the structured report the user expects.

   The DoctorWorkflowsEvent type already declares a { kind: "error" } variant, but it is never emitted. Consider catching permission errors and either yielding a per-directory error event or logging a warning and continuing.

2. src/libswamp/workflows/doctor.ts:126+129 — YAML is parsed twice on the success path

   tryExtractName(content, filePath) on line 126 calls parseYaml(content) to extract the name. Then line 129 calls parseYaml(content) again to get the full WorkflowData. On the success path both parses succeed, so the first is pure waste. On the failure path (broken YAML), the double-parse is harmless since tryExtractName catches the error. Not a correctness bug, but for repos with many workflows it's double the parse work for no benefit. A single parse with the name extracted from the parsed object would eliminate this.

Low

1. src/libswamp/workflows/doctor.ts:44-47 — The error event kind is declared but never emitted

   DoctorWorkflowsEvent includes { kind: "error"; error: SwampError } and the renderer handles it (throw new UserError(e.error.message)), but the generator never yields this event. This is dead code. It's not harmful, but it means the "error" handler in both renderers is unreachable.

Verdict

FAIL — The hardcoded / path separator at doctor.ts:107 violates the project's explicit cross-platform path convention and produces broken paths on Windows, where this code is required to run per CLAUDE.md testing policy. Fix the path construction (a one-line change) and this is ready to merge.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Log mode: missing blank line after header — The docs example in health-checks.md shows a blank line between Checking workflows... and the first file entry, but the renderer (src/presentation/renderers/workflow_doctor.ts:44) emits the header then the first file result with no gap. Cosmetic only; easy to fix by adding writeOutput('') after the header is printed.

2. health-checks.md: broken inline code span — Lines 220–221 of the updated file have a backtick code span split across two lines: swamp workflow\nvalidate. In most Markdown renderers this renders fine (newline → space), but it's fragile. Should be swamp workflow validate on a single line.

Verdict

PASS — both output modes are correctly implemented, flag names and option descriptions are consistent with doctor audit and doctor extensions, JSON shape includes all fields a script would need, and exit-on-failure behaviour is correct.

[github-actions]

Code Review

Well-structured PR that follows project conventions closely. The new swamp doctor workflows subcommand cleanly mirrors the existing doctor audit and doctor extensions patterns — application-service generator, dual-mode renderer, proper libswamp export boundary, license headers, and comprehensive test coverage.

Blocking Issues

None.

Suggestions

1. File filter mismatch with the actual workflow loader (src/libswamp/workflows/doctor.ts:96): The doctor scans all *.yaml files in workflow directories, but YamlWorkflowRepository.findAll() only loads files matching workflow-*.yaml (requires the workflow- prefix). This means the doctor will attempt to parse and construct YAML files that the loader would never touch — potentially producing false-positive failures for non-workflow YAML files that happen to live in a workflow directory. Consider either adding the workflow- prefix filter to align with the repository, or documenting the intentionally broader scope so users understand why the doctor flags files that workflow validate never sees.

2. Redundant YAML re-parse in error path (src/libswamp/workflows/doctor.ts:132-136): When Workflow.fromData() throws (not a YAML parse error), the catch block re-parses the same YAML content to extract the name. Since parsing succeeded in the try block, this second parse is guaranteed to succeed but is unnecessary work. You could capture data.name before calling fromData() to avoid the re-parse.

3. Potential directory overlap (src/cli/commands/doctor_workflows.ts:93-97): yamlWorkflowsDir is hardcoded to join(repoDir, "workflows") while workflowsDir comes from resolveWorkflowsDir(marker) which defaults to "extensions/workflows". If a user configures SWAMP_WORKFLOWS_DIR=workflows or sets workflowsDir: "workflows" in their marker, both would resolve to the same directory, causing every workflow to be checked twice. A new Set() dedup on the resolved absolute paths would be a cheap guard.

DDD Assessment

Clean layering: the doctorWorkflows async generator is a well-scoped application service in libswamp that orchestrates the Workflow domain entity for a single use case. DoctorWorkflowResult and DoctorWorkflowsReport are proper value objects. The renderer lives in the presentation layer and imports only from libswamp/mod.ts. No domain model leakage.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. Missing workflow- prefix filter — false positives on non-workflow YAML files
   src/libswamp/workflows/doctor.ts:96

   The existing YamlWorkflowRepository.findAll() filters files with entry.name.startsWith("workflow-") && entry.name.endsWith(".yaml") (yaml_workflow_repository.ts:87-88). The doctor command only checks .endsWith(".yaml"):

   .filter((e) => e.isFile && e.name.endsWith(".yaml"))

   Breaking example: If a user has a config.yaml, README.yaml, or values.yaml in their workflows directory, the doctor will attempt to parse them as workflows, report them as failures, and exit 1 — even though the actual workflow loader would never touch those files.

   Suggested fix: Add .startsWith("workflow-") to the filter to match the real loader's behavior, or document that the doctor intentionally scans all YAML (and explain why).

2. Potential duplicate directory scanning
   src/cli/commands/doctor_workflows.ts:74,76-79,93-97

   yamlWorkflowsDir is hardcoded to join(repoDir, "workflows"). If resolveWorkflowsDir(marker) returns "workflows" (via env var SWAMP_WORKFLOWS_DIR=workflows or marker.workflowsDir = "workflows"), then workflowsDir resolves to the same path. Both are added to workflowDirs at lines 93-97 without deduplication, so every file in that directory would be checked twice and reported twice — inflating counts and producing confusing output.

   Suggested fix: Deduplicate the workflowDirs array (e.g., [...new Set(workflowDirs)]).

Low

1. error event kind is defined but never emitted
   src/libswamp/workflows/doctor.ts:49

   DoctorWorkflowsEvent includes { kind: "error"; error: SwampError }, but the doctorWorkflows generator never yields an event with kind: "error". Unexpected errors (e.g., permission denied on a non-NotFound/non-PermissionDenied error) propagate as thrown exceptions rather than as error events. The error handlers in both renderers are dead code.

   This isn't a bug (the HasTerminals constraint requires the error kind in the union), but the throw-vs-yield split means the CLI command has no chance to render the error through the renderer's error handler — the exception bypasses the renderer entirely.

2. AbortController signal is never aborted
   src/cli/commands/doctor_workflows.ts:100

   The AbortController is created but never wired to SIGINT or any other cancellation source. Compare with doctor_audit.ts which explicitly listens for SIGINT and aborts the controller. For workflow directories with many files, Ctrl+C during scanning would leave the async generator mid-iteration rather than cleanly exiting.

   Not a correctness issue since the process exits anyway on SIGINT, but inconsistent with doctor_audit.ts.

Verdict

PASS — The implementation is solid, follows the established patterns well, has good test coverage, and handles the important error cases (missing directories, permission denied, broken YAML, invalid schemas). The medium findings are about false-positive noise and cosmetic duplication in output, not correctness or data integrity issues.