fix(misc): cryptographic taskId, hoisted fs import, graceful shutdown, awaited transport

[sergei-aronsen]

Summary

Small reliability / hygiene fixes — independently small but unrelated, hence bundled. Happy to split.

`crypto.randomUUID` for task IDs

`generateTaskId` used `Math.random().toString(36).substring(2, 8)` — six base36 chars, ~31 bits, disambiguated only by the `Date.now()` prefix. Two concurrent callers in the same millisecond have a real birthday-collision risk, and `Math.random` triggers every standard "no insecure RNG" lint rule.

Switched to `crypto.randomUUID()` (Node ≥ 14.17). Same shape (`task__`), collision-free for any reasonable volume. Existing test regex updated to the UUID v4 shape.

Hoisted `fs` import

`comet_upload` did `const fs = await import('fs')` on every call. Cached after the first, but adds an unnecessary microtask each request and prevents bundlers from tree-shaking. Hoisted to a top-level `import { existsSync } from "fs"`.

Awaited `server.connect()`

`server.connect(transport)` was fire-and-forget. If the transport failed to attach (e.g. stdin already closed in a misconfigured parent) the promise silently rejected and the process exited with no logs. Now `await server.connect(transport).catch(...)` logs the error before exiting non-zero.

Graceful SIGINT / SIGTERM handler

Added handlers that call `cometClient.disconnect()` before exiting, so the WebSocket to Comet closes cleanly instead of sitting half-open until Comet times it out. Exits 130 on SIGINT (POSIX convention for Ctrl+C), 0 on SIGTERM.

Test plan

• `npm run test:unit` passes (updated session-state test included)
• Manual: Ctrl+C exits cleanly with no CDP error logs
• Manual: `kill ` exits cleanly
• Manual: 100 concurrent `comet_ask` task IDs are all unique

🤖 Generated with Claude Code

[RapierCraft]

Code Review: PR #20 — fix(misc): cryptographic taskId, hoisted fs import, graceful shutdown, awaited transport

Reviewed commit: 871befc

---

1. Cryptographic taskId (src/session-state.ts) — Approved

The switch from Math.random().toString(36).substring(2, 8) (~31 bits) to crypto.randomUUID() (~122 bits) is a clear improvement. randomUUID() is available since Node 14.17, and the project requires >=18.0.0, so no compatibility concern. Test regex correctly matches UUID v4 format.

---

2. Hoisted fs import (src/index.ts) — Approved

Replacing dynamic await import('fs') with top-level import { existsSync } from "fs" is correct. Node's fs is always available; the dynamic import added a needless microtask per call.

---

3. Awaited transport (src/index.ts) — Approved with Note

.catch() without await — still fire-and-forget at top level, but now with error handling. Acceptable bootstrap pattern. Rejection is caught and exits non-zero.

Subtle note: If server.connect() rejects before the event loop ticks, SIGINT/SIGTERM handlers below never fire. Unlikely with StdioServerTransport. Not blocking.

---

4. Graceful Shutdown (src/index.ts) — Approved with Recommendation

4a. Exit code comment (LOW): Comment says "exit code 0 for graceful signal" but code exits 130 for SIGINT. Code is correct (130 = 128+2, POSIX). Comment slightly misleading.

4b. disconnect() scope — Correct: Only closes CDP WebSocket. Appropriate since server doesn't own browser lifecycle.

4c. No timeout on disconnect (MEDIUM):

If cometClient.disconnect() hangs, process never exits. Suggest:

await Promise.race([
  cometClient.disconnect(),
  new Promise(resolve => setTimeout(resolve, 3000))
]);

Recommended hardening, not blocking.

---

Summary

Change	Verdict	Issues
Cryptographic taskId	Correct	None
Hoisted fs import	Correct	None
Awaited transport	Correct	Minor race (non-blocking)
Graceful shutdown	Correct	Recommend disconnect timeout

Overall: APPROVE — All changes are clear improvements. No security issues, bugs, or correctness problems. Safe to merge.

---

Single-reviewer analysis focused on: crypto randomness, import ordering, shutdown logic, transport awaiting.

[RapierCraft]

APPROVED: commit 871befc after code review (1 reviewer). 0 blocking issues. 1 non-blocking recommendation (disconnect timeout for graceful shutdown). Safe to merge.

[RapierCraft]

This PR has merge conflicts with main after recent merges (PRs #11, #12, #16, #18 landed).

The conflicts should be straightforward — crypto taskId, hoisted fs import, and shutdown logic don't overlap semantically with what merged. Please rebase against main.

[RapierCraft]

Review: PR #20 — fix(misc): cryptographic taskId, hoisted fs import, graceful shutdown, awaited transport

Reviewed commit: 5ff609e | Review type: Security + Correctness
Verdict: APPROVE with notes

---

Automated Checks

Check	Result
TypeScript	Pre-existing errors only (cdp-client.ts once/removeListener — PR #22 fixes)
Unit tests (vitest)	9/9 passed — session-state tests all green
Secrets detection	Clean

---

Change-by-Change Analysis

1. crypto.randomUUID() for task IDs — APPROVE

• Replaces Math.random().toString(36).substring(2,8) (~31 bits) with randomUUID() (~122 bits)
• Eliminates collision risk under sub-millisecond concurrent calls
• Test regex correctly updated to UUID v4 format
• Date.now() prefix retained for human-readable ordering

2. Hoisted fs import — APPROVE

• Removes per-call await import('fs') overhead in comet_upload
• existsSync now imported at top level alongside readFileSync
• Minor but correct cleanup

3. Awaited server.connect(transport) — APPROVE

• .catch() handler logs error before process.exit(1)
• Prevents silent failure when stdin is already closed at startup
• Good practice for stdio MCP servers

4. Graceful SIGINT/SIGTERM handler — APPROVE

• cometClient.disconnect() races a 3s timeout — prevents hung WebSocket from blocking exit
• Exit codes follow POSIX convention: 130 (SIGINT = 128+2), 143 (SIGTERM = 128+15)
• void gracefulShutdown(...) correctly handles the async promise without dangling

---

Advisory Notes

1. MEDIUM — No stdin/transport close handler (PR #23 addresses this)

This PR handles SIGINT/SIGTERM but not the case where the MCP client simply disconnects the stdio pipe without sending a signal. That's the more common orphan scenario (client timeout, session end). PR #23 adds transport.onclose + process.stdin.on("end") handlers. Both PRs are needed — they're complementary. They will conflict on the same lines and should be merged together.

2. INFO — Deep dependency chain

This PR's branch includes merged PRs #6, #7, #11, #12, #13, #16, #18, #19, #21. It cannot merge cleanly until those are merged to main first. This is expected given the author's contribution pattern but worth noting for merge planning.

3. INFO — SIGTERM exit code

The code uses 143 for SIGTERM, which is standard (128+15). However, some orchestrators (systemd, Docker) interpret exit code 143 as "normal termination by signal" and won't restart the process. This is the correct behavior for a graceful shutdown but worth documenting.

---

Summary

Four independent, well-scoped improvements. Each is correct and improves reliability or security. The cryptographic task ID is the most important change (eliminates a real collision vector). Tests are thorough and pass. The graceful shutdown complements PR #23's transport-close handler.

---

Review complete. Unit tests: 9/9 passed. 0 blocking findings.