Skip to content

feat(cli): add swamp issue get command (swamp-club#118)#1326

Merged
stack72 merged 2 commits intomainfrom
worktree-118
May 6, 2026
Merged

feat(cli): add swamp issue get command (swamp-club#118)#1326
stack72 merged 2 commits intomainfrom
worktree-118

Conversation

@stack72
Copy link
Copy Markdown
Contributor

@stack72 stack72 commented May 6, 2026

Summary

  • Adds swamp issue get <number> CLI command to fetch and display Lab issue details (title, type, status, author, body, assignees, comment count)
  • Supports both log (human-readable) and json (structured) output modes
  • Adds fetchIssue method to SwampClubClient using x-api-key auth (consistent with existing methods)
  • Updates swamp-issue skill with new command documentation and JSON output shape

Closes swamp-club#118

Test Plan

  • Unit tests for issueGet libswamp generator (src/libswamp/issues/get_test.ts)
  • CLI command structure test (src/cli/commands/issue_get_test.ts)
  • deno check — all files pass type checking
  • deno lint — no lint errors
  • deno fmt — all files formatted
  • deno run test — 5542 passed (1 pre-existing flaky failure in unrelated workflow test)
  • deno run compile — binary compiles successfully

🤖 Generated with Claude Code

…club#118)

Adds a lightweight CLI command to fetch and display existing Lab issue
details without creating a full lifecycle model instance. Supports both
log and json output modes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review

Critical / High

None found.

Medium

  1. Dead error event kind in IssueGetEvent (src/libswamp/issues/get.ts:38): The { kind: "error"; error: SwampError } variant is defined in the event union but never yielded by the generator. If deps.fetchIssue throws, it propagates as an uncaught exception — it does not become a { kind: "error" } event. The error handlers in both LogIssueGetRenderer and JsonIssueGetRenderer (src/presentation/renderers/issue_get.ts:56,69) are therefore unreachable dead code. This is consistent with the existing IssueCreateEvent and IssueCommentEvent patterns, so it's a codebase-wide convention rather than a PR-specific bug, but it means the EventHandlers type forces callers to handle an event that can never arrive.

Low

  1. No null guard on data.issue after res.json() (src/infrastructure/http/swamp_club_client.ts:291): If the server returns HTTP 200 with an unexpected JSON shape (e.g. {"success": true} with no issue field), data.issue is undefined and issue.number throws a raw TypeError instead of a friendly UserError. Example: a reverse proxy or CDN returning a cached/generic response. The same unguarded pattern exists in submitIssue (line 208) and submitComment (line 263), so this is pre-existing.

  2. No special handling for HTTP 401 in fetchIssue (src/infrastructure/http/swamp_club_client.ts:280-287): An expired or invalid API key produces Failed to fetch issue #42 (HTTP 401): <raw body> rather than a clear "not authenticated" message. Compare with whoami (line 162) which returns { authenticated: false } on 401. Not a correctness issue — the user gets an error either way — but the message quality differs from the whoami pattern.

Verdict

PASS — Clean, well-structured PR that follows existing codebase patterns. The generator, client method, CLI command, and renderers are all consistent with their siblings (issue create, issue ripple). Input validation matches the existing issue_ripple.ts pattern. No security, concurrency, or resource management concerns.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI UX Review

Blocking

  1. src/presentation/renderers/issue_get.ts — wrong output function for log mode

    LogIssueGetRenderer uses logger.info() (via getSwampLogger) for every line of user-facing output. Every other "get" renderer in the codebase — model_get.ts, data_get.ts, report_get.ts — uses writeOutput() instead. The writeOutput() function is explicitly documented:

    "Writes plain text to stdout with no decoration (no timestamp, level, or category prefix). Use for human-readable CLI 'log' mode output that should read like a document rather than a log stream."

    Because logger.info() routes through LogTape, users will see log metadata injected into what should be a clean issue record — lines like #42: My Issue become something like [issue][get] #42: My Issue or carry timestamp prefixes depending on log configuration. logger.info("") for blank lines is especially bad. Fix: replace logger.info(...) calls with writeOutput(...) and build up the output string similarly to how data_get.ts and model_get.ts do.

Suggestions

  1. No color formatting in log mode. Every other "get" renderer (model_get.ts, data_get.ts, audit_timeline.ts) uses bold, cyan, and dim from @std/fmt/colors to make labels scannable. The issue_get log output is entirely uncolored plain text. Consider formatting field labels with bold(cyan("Type:")) etc. to match the visual language of the rest of the CLI.

  2. Issue body is markdown — should use renderMarkdownToTerminal(). The body field of a Lab issue is markdown (the JSON example in output_shapes.md shows "## Problem\n\n..."). report_get.ts calls renderMarkdownToTerminal(r.markdown) for exactly this case. Rendering the body as raw markdown text makes ## headers and **bold** visible as literal punctuation in the terminal.

Verdict

NEEDS CHANGES — the log-mode renderer routes all user-facing output through logger.info() instead of writeOutput(), which will inject log metadata into what should be clean document-style output, inconsistent with every other "get" command.

github-actions[bot]
github-actions Bot previously approved these changes May 6, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Clean implementation that follows established patterns well. DDD layering is correct — the generator in libswamp defines its own IssueGetDeps interface for dependency inversion, the CLI command wires concrete infrastructure, and the renderer handles presentation. All imports from libswamp/mod.ts are correct (no internal path imports). Both log and json output modes are supported. AGPLv3 headers present on all new files. LogTape usage is correct (tagged template in generator, structured logging in renderer).

Blocking Issues

None.

Suggestions

  1. Missing renderer unit testsrc/presentation/renderers/issue_get_test.ts doesn't exist. The sibling issue_create renderer has a test file that captures console.log output and verifies the JSON shape. At minimum, testing the JSON renderer output would verify the shape matches what's documented in output_shapes.md.

  2. Missing SwampClubClient.fetchIssue test — The method has non-trivial response parsing (nested data.issue, assignee filtering, comment counting) and error handling (404 → UserError). The client test file exists but has no coverage for the new method.

  3. IssueGetDeps.fetchIssue return type duplicates IssueGetData — The inline return type on fetchIssue in IssueGetDeps (lines 45-55 of src/libswamp/issues/get.ts) is identical to IssueGetData defined just above. Could simplify to fetchIssue: (issueNumber: number) => Promise<IssueGetData> to reduce duplication.

Switch log-mode renderer from logger.info() to writeOutput(), add color
formatting with bold/cyan/dim, and render issue body as markdown via
renderMarkdownToTerminal(). Deduplicate IssueGetDeps return type.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI UX Review

Blocking

None.

Suggestions

  1. Cramped metadata line (src/presentation/renderers/issue_get.ts:34): Type:, Status:, and Author: are packed onto a single line with double-space separators. model_get.ts puts each label on its own line using bold(cyan(...)) — a user reading a long author name or status string could find the packed format harder to scan. Consider splitting into separate lines for consistency.

  2. Label color inconsistency (src/presentation/renderers/issue_get.ts): The issue number uses bold(cyan(...)) but the meta labels (Type:, Status:, Author:, etc.) use plain bold(...). Every other detail renderer (model_get, report_get) uses bold(cyan(...)) for all labels. Minor visual inconsistency.

  3. Comment count shown even when 0 (src/presentation/renderers/issue_get.ts:41): Comments: 0 is always displayed, while the similar Assignees: field is conditionally hidden when empty. Could apply the same pattern (if (d.commentCount > 0)) for a cleaner output on issues with no comments.

Verdict

PASS — both log and JSON modes work correctly, error messages are clear and actionable (Issue #N not found., Not logged in. Run "swamp auth login" first.), the JSON shape matches the documented output, and the command follows the existing issue-command conventions.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review

Critical / High

None.

Medium

  1. src/infrastructure/http/swamp_club_client.ts:291data.issue access without null guard: If the server returns a 200 response with a body where data.issue is undefined or null (e.g., {}), then issue.number on line 293 throws a raw TypeError: Cannot read properties of undefined (reading 'number') instead of a descriptive UserError. The individual fields (title, type, etc.) are guarded with ??, but the top-level issue key is not. Example: server deploys a breaking change that wraps the response differently → user sees an opaque stack trace. Fix: add a guard like if (!data?.issue) throw new UserError("Unexpected response from server."); before accessing fields. This is medium because it's a server contract violation scenario, not a user-input scenario.

  2. src/infrastructure/http/swamp_club_client.ts:285-287 — Server error bodies included verbatim in user-facing error messages: For non-404 errors (401, 403, 500), the full response body text is concatenated into the UserError message. If the server returns verbose debug output or HTML error pages, the user sees a wall of text. The whoami method (line 162-163) handles 401 specially without exposing the response body. This is inconsistent but not a correctness bug — it's a UX concern. Example: expired API key → Failed to fetch issue #42 (HTTP 401): <!DOCTYPE html>... or a JSON blob with internal server details.

Low

  1. src/cli/commands/issue_get.ts:45 — Redundant validation: The <number:integer> Cliffy argument type already guarantees issueNumber is an integer. The manual Number.isInteger(issueNumber) || issueNumber <= 0 check is defense-in-depth, which is fine. The <= 0 check does add value since Cliffy doesn't reject negative integers — swamp issue get -1 would be caught here rather than producing a confusing 404. Not a bug, just noting it's intentional defense-in-depth.

  2. src/libswamp/issues/get.ts:37-38error event kind declared but never yielded: The IssueGetEvent union includes { kind: "error"; error: SwampError } but the generator never yields this variant. Errors from deps.fetchIssue propagate as thrown exceptions, not as yielded events. This is consistent with issueComment in comment.ts which also lets dep errors throw, so it follows the established pattern. The error handler in renderers is required by EventHandlers<E> exhaustiveness but is effectively dead code for this generator.

Verdict

PASS — Clean, well-structured implementation that follows existing patterns (issueComment, submitIssue). The generator/renderer/CLI split is consistent with the codebase's architecture. No security, correctness, or resource management issues found.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Clean, well-structured implementation of swamp issue get. The architecture follows established patterns correctly:

  • DDD: IssueGetData is a proper value object, IssueGetDeps cleanly abstracts the infrastructure dependency via DI, the generator lives in the application/libswamp layer as expected.
  • Import boundary: Both the CLI command and presentation renderer import from src/libswamp/mod.ts — never internal paths. ✓
  • License headers: Present on all new .ts files. ✓
  • Output modes: Both log and json modes supported. ✓
  • Tracing: Generator uses withGeneratorSpan consistently with other generators. ✓
  • Security: Auth is verified before API call, API key via x-api-key header (consistent with existing methods), input validated, no injection vectors (issue number is an integer).

Blocking Issues

None.

Suggestions

  1. CLI command test missing logging init (src/cli/commands/issue_get_test.ts): CLAUDE.md states CLI command tests require initializeLogging({}) and import "../../domain/models/models.ts". This test imports issueGetCommand statically (which transitively pulls in libswamp/mod.ts). It works today because the issue path doesn't trigger model registration side effects, but adding the init would be consistent with the documented convention (see data_get_test.ts for the canonical pattern). Other issue command tests (bug, ripple) also skip it, so this is a codebase-wide gap rather than specific to this PR.

  2. No renderer test (src/presentation/renderers/issue_get.ts): Many renderers in the codebase have corresponding _test.ts files. A test verifying the log-mode output format and JSON-mode output structure would strengthen coverage, though this isn't blocking since the generator tests cover the data flow.

@stack72 stack72 merged commit 60b2e6d into main May 6, 2026
11 checks passed
@stack72 stack72 deleted the worktree-118 branch May 6, 2026 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant