CLI UX Review

Blocking

None.

Suggestions

1. Email log output is minimal — src/presentation/renderers/issue_create.ts:46-49: After Opening email client to submit {type} report... there's no address or next steps shown. If the email client doesn't open (e.g., no mail app configured), the user sees only that one line with no indication of where to send the report manually. Consider adding the support address: Check your email client, or send manually to [email protected].

2. Misleading prompt label for option 1 — src/cli/commands/issue_submit.ts:107: The label says "Log in now" but the command actually exits and tells the user to retry. The parenthetical clarifies this, but the primary label creates a false expectation that login will happen inline. Consider "Cancel and log in" or just "Log in first (then retry)".

3. Security + email: no privacy caveat — The security subcommand description promises content is visible only to you and admins, and the Lab submission reinforces this with a privacy note. But when using --email, that guarantee doesn't apply — it's just a regular email with no access controls. A one-line note (Note: email submissions are not private) after opening the mail client would set correct expectations.

4. --email --json silently opens a browser — resolveDestination skips the JSON-mode check when emailFlag is true, so --email --json will both open the email client and emit JSON. This is probably not a real use case, but for consistency it's worth either blocking the combination with a UserError or documenting it as intentional.

Verdict

PASS — clean migration from GitHub to Lab API with solid flow. The auth-before-editor design is good UX. No blocking issues.

Code Review

Blocking Issues

1. Missing tests for new files. src/cli/commands/issue_submit.ts and src/cli/commands/issue_security.ts are both new files with no corresponding _test.ts files. CLAUDE.md requires comprehensive unit test coverage with tests living next to source files (foo.ts → foo_test.ts). Key functions to test:
   • resolveDestination() — the three paths (email flag, logged-in, not-logged-in)
   • submitIssue() — lab, email, and abort branches
   • parseSecurityContent() — valid input, empty input, unchanged template, missing title
2. Missing test for SwampClubClient.submitIssue(). The existing test file (swamp_club_client_test.ts) covers signIn, createApiKey, and whoami but not the new submitIssue method. At minimum, test the success path and the error (non-ok response) path.

Suggestions

1. Duplicate createLibSwampContext in submitIssue abort path (issue_submit.ts:129,133). Line 129 creates libCtx, then the abort branch on line 133 creates a second context as logger. The abort branch could use the existing libCtx instead.
2. Stale module doc comment (issue_submit.ts:23). The JSDoc says "shared submission logic for swamp issue bug and swamp issue feature" but the module is also used by swamp issue security now.
3. Redundant narrowing in feature test (create_test.ts:97). The if (completed.data.method === "lab") guard on line 97 is unnecessary — the assertion on the line above already confirms it. The bug test at line 57 doesn't use this pattern and is cleaner.

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. src/cli/commands/issue_submit.ts:92-96 — buildMailtoUrl uses URLSearchParams which encodes spaces as + instead of %20

   URLSearchParams.toString() uses application/x-www-form-urlencoded encoding, which represents spaces as +. However, mailto: URIs follow RFC 6068 which requires standard percent-encoding (%20 for spaces). Some email clients (particularly on Linux, older Outlook, and Thunderbird versions) interpret + literally, producing subjects like [bug]+CLI+crashes+on+empty+input instead of [bug] CLI crashes on empty input.

   Breaking example: swamp issue bug --email --title "CLI crashes on empty input" --body "Steps to reproduce" produces mailto:[email protected]?subject=%5Bbug%5D+CLI+crashes+on+empty+input&body=Steps+to+reproduce — note the + instead of %20.

   Suggested fix: Replace URLSearchParams with manual percent-encoding:

   const subject = encodeURIComponent(`[${type}] ${title}`);
   const body = encodeURIComponent(body);
   return `mailto:${SUPPORT_EMAIL}?subject=${subject}&body=${bodyEncoded}`;

2. src/cli/commands/issue_submit.ts:60-64 — promptLoginOrEmail defaults to "email" on any non-"1" input including EOF/error

   When Deno.stdin.read returns null (stdin is closed/EOF) or the user enters anything other than exactly "1" (e.g., "y", "login", an accidental newline), the function defaults to "email". This could surprise users who intended to choose "login" but mistyped. Since this is the interactive path, an invalid-input retry loop or at least accepting more inputs for the login choice would be more robust.

   Breaking example: User types 1 (with trailing space, which trim() handles) — OK. But user types login or l — gets email instead of login.

Low

1. src/infrastructure/http/swamp_club_client.ts:167-188 — submitIssue doesn't accept an AbortSignal parameter unlike whoami

   Per the project's CLAUDE.md convention ("For outbound network calls, pass an AbortSignal with a timeout so the caller controls cancellation"), the new submitIssue method should accept an optional signal parameter. The private fetch wrapper provides a default 15s timeout so this isn't a functional issue, but it's inconsistent with whoami and prevents callers from setting custom timeouts.

2. TOCTOU on auth credentials between resolveDestination and submitIssue — Credentials are loaded and validated at resolve time (before the editor opens), but the actual API call happens after the user finishes editing, which could be minutes later. If the API key is revoked or the server URL changes in the interim, the submission fails with an HTTP error. The error message is adequate, but the user would lose their content (already typed into the editor). Not a practical concern in most usage, but worth noting.

Verdict

PASS — The code is well-structured with a clean two-phase design (resolve-then-submit). The mailto URL encoding issue (Medium #1) should be fixed before or shortly after merge since it affects every email submission, but it's not a blocker since email is the fallback path, not the primary one.

Code Review

Blocking Issues

None.

Suggestions

1. Dead code: GitHubIssueService — After this PR, src/infrastructure/github/github_issue_service.ts (and its test) are no longer imported by anything outside themselves. Consider removing them in a follow-up to avoid confusion.

2. Defensive parsing of Lab API response — SwampClubClient.submitIssue accesses data.issue.number and data.issue.id without validating the response shape. If the API returns an unexpected body on a 200, this will throw a confusing TypeError. A quick guard or descriptive error at this system boundary would improve debuggability.

3. Document email JSON output shape — The PR description documents the --json output for Lab submissions but not for the --email path. The email path emits { method: "email", mailtoUrl, type, title } to JSON consumers — worth documenting for completeness.

DDD Assessment

The refactoring is well-structured from a DDD perspective. issueCreate in libswamp remains a clean application service that accepts a dependency interface (IssueCreateDeps) — the CLI layer now wires the real SwampClubClient through this interface, keeping infrastructure out of the domain/application layer. The two-phase resolveDestination / submitIssue split in the CLI layer is a sensible orchestration pattern.

Overall

Clean PR. Imports respect the libswamp boundary (src/libswamp/mod.ts only), new files have the AGPLv3 header, tests cover the parsers and mailto builder, and the SwampClubClient.submitIssue goes through the existing this.fetch() which already applies an AbortSignal.timeout. Good to merge.

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. src/infrastructure/http/swamp_club_client.ts:189-190 — No validation of API response shape before destructuring.
   If the Lab API returns a 200 with an unexpected body (e.g., {}, or { "issue": null }, or { "issue": {} }), data.issue.number and data.issue.id will be undefined, silently producing { number: undefined, id: undefined }. This propagates to the renderer which will display "Submitted bug report #undefined" — confusing but not a crash.
   Example: Server returns {"ok": true} → data.issue is undefined → Cannot read properties of undefined (reading 'number') → unhandled throw from inside consumeStream.
   Suggested fix: Add a guard: if (!data?.issue?.number) throw new UserError("Unexpected response from Lab API");

2. src/cli/commands/issue_submit.ts:113-114 — Deno.stdin.read returns null on closed stdin, not just when empty.
   When stdin is closed (piped empty input, < /dev/null), Deno.stdin.read(buf) returns null. The current code handles this correctly (n ? ... : ""), and the default is "login" which is the safe path. However, in a CI/piped context where the user reaches promptLoginOrEmail() (non-JSON mode but stdin is a pipe, not a TTY), the prompt is invisible and silently aborts — the user gets "Run swamp auth login first" with no explanation of why. This is an uncommon path but could confuse users running swamp issue bug --title x --body y in a script without being logged in.
   Suggested fix: Consider checking Deno.stdin.isTerminal() and treating non-TTY the same as JSON mode (throw UserError).

3. src/cli/commands/issue_submit.ts:147 — openBrowser failure is unhandled in the email path.
   If openBrowser(mailtoUrl) throws (e.g., no browser available in a headless/SSH/CI environment), the error propagates uncaught. The "send manually to..." fallback message is only printed after the openBrowser call succeeds and consumeStream runs. So the user gets an opaque error instead of the helpful fallback.
   Suggested fix: Wrap openBrowser in try/catch so the manual email fallback is still rendered.

Low

1. src/cli/commands/issue_submit.ts:91-93 — Very long issue bodies may produce mailto URLs that exceed OS/browser limits.
   Mailto URLs over ~2000 characters are frequently truncated by mail clients and OSes. A detailed bug report with reproduction steps could easily exceed this. The body would be silently truncated.
   Note: This is inherent to the mailto: approach and not really fixable without switching to a different mechanism, but worth being aware of.

2. src/cli/commands/issue_security.ts:78-79 — parseSecurityContent regex [^\n#<][^\n]* won't match a title that starts with #, <, or newline.
   A user writing a title like <script> injection test or #1234 regression would get null (treated as empty/cancelled). The character class exclusion is intentional (to skip HTML comments), but # and < at the start of real titles is a false negative.
   Impact: Uncommon titles would be silently rejected.

Verdict

PASS — The code is well-structured with a clean two-phase design (resolveDestination → submitIssue). The RFC 6068 mailto fix, security privacy caveat, and default-to-login on bad input are all good defensive choices. The medium findings are edge cases in uncommon paths (malformed API response, piped stdin, headless openBrowser) that don't block the merge but would improve robustness if addressed.

CLI UX Review

Blocking

None.

Suggestions

1. JSON output missing constructed URL for lab submissions — src/presentation/renderers/issue_create.ts: the log renderer builds and displays ${serverUrl}/lab/${number}, but the JSON output only includes serverUrl and number as separate fields. A script consuming JSON has to know the URL format to construct a link. Consider adding a url field to the JSON data so it matches what the log mode surfaces.

2. "Login" prompt choice does nothing visible — src/cli/commands/issue_submit.ts:promptLoginOrEmail: option 1 is labelled "Log in first (then retry this command)", but selecting it just prints "Run swamp auth login first, then retry this command." and exits. The user just answered a question for no interactive gain. Consider collapsing this to a direct message ("You're not logged in. Run swamp auth login and retry, or use --email.") and skipping the numbered prompt entirely. The two-option prompt implies two paths but option 1 is just a message.

Verdict

PASS — new swamp issue security subcommand, --email flag, and auth-before-editor flow all have clear help text, consistent flag names, actionable error messages, and correct behavior in both log and JSON modes.