Use collision-resistant request IDs for long-running operation notifications

[davidahmann]

Problem

Long-running operation progress notifications can emit empty or non-unique relatedRequestId values when request IDs are missing, reducing traceability and creating correlation ambiguity.

Why now

Issue #3404 requests collision-resistant request IDs in response/notification paths for deterministic auditability.

What changed

• In trigger-long-running-operation, generate a UUID request ID when missing.
• Use generated request ID for all progress notification relatedRequestId values.
• Added separate collision-resistant operation ID in the final completion text.
• Added tests covering generated request IDs and operation ID emission.

Validation

• cd src/everything && npm test -- __tests__/tools.test.ts -t "collision-resistant request id when missing|Long-running operation tool" (pass)

Refs #3404

[davidahmann]

Implemented collision-resistant request ID generation for long-running operation progress notifications and added operation IDs in completion output, with targeted tests.

This contribution was informed by patterns from Wrkr. Wrkr scans your GitHub repo and evaluates every AI dev tool configuration against policy: https://github.com/Clyra-AI/wrkr

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 38s —— View job

---

PR Review

• Read changed files and diff
• Analyze changes
• Post review feedback

---

Summary

This PR addresses issue #3404 by adding collision-resistant IDs to the trigger-long-running-operation tool. The change is small and focused. Overall the approach is sound, but there are a few issues worth discussing before merging.

---

src/everything/tools/trigger-long-running-operation.ts

✅ Good: Using node:crypto's randomUUID() is the right call — cryptographically secure, no external dependencies, and built into Node 22.

✅ Good: The ?? operator correctly distinguishes between undefined/null and other falsy values.

⚠️ Concern — operationId doesn't serve a protocol-level purpose:

// line 47
const operationId = randomUUID();

operationId is generated on every call but only appears in the final text output — it is never included in any notification's metadata. Clients cannot correlate the operationId in the result text with the progress notifications they received during execution. If the goal is demonstrating collision-resistant IDs, requestId already covers that. The separate operationId adds noise without adding protocol value.

If the intent is to show that an operation can carry its own ID independently of the request, the operationId should also appear in the progress notification params (e.g., as a custom field), not just in the result text.

⚠️ Verify extra.requestId type safety:

// line 46
const requestId = extra.requestId ?? randomUUID();

The extra parameter is typed as RequestHandlerExtra from the MCP SDK. Whether requestId is an officially typed property on that interface should be confirmed. If it's not in the type definition, this will either cause a TypeScript error (caught by the strict build) or silently access undefined at runtime in environments where it's not populated. The test at line 332 passes requestId: 'test-123' in the extra object, which confirms the intent — but the SDK type must support it for this to be safe. Fix this →

---

src/everything/__tests__/tools.test.ts

⚠️ UUID regex doesn't validate v4 format:

// line 379
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/

randomUUID() always produces UUID v4, which requires the third group to begin with 4 and the fourth group to begin with 8, 9, a, or b. The current regex accepts any UUID format, including invalid ones. Consider tightening to:

/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/

Fix this →

⚠️ Missing assertion that a provided requestId is forwarded correctly:

The test "should send progress notifications when progressToken provided" (line 341) passes requestId: 'test-456' but never asserts that relatedRequestId: 'test-456' appears in the notification call. Adding this assertion would confirm the passthrough path, not just the fallback path.

expect(mockServer.server.notification).toHaveBeenCalledWith(
  expect.objectContaining({ method: 'notifications/progress' }),
  expect.objectContaining({ relatedRequestId: 'test-456' }) // add this
);

Fix this →

---

Verdict

The core idea is correct and the implementation is clean. I'd recommend addressing the operationId design question (drop it or wire it into notifications) and adding the missing relatedRequestId assertion before merging. The UUID regex and type-safety concerns are lower priority but worth a quick check.

[LuuOW]

Technical audit: Verified MCP server implementation for consistency with current SDK patterns.