fix: build persistent action ids without crypto.randomUUID

[akhileshthite]

Closes #100
Related okTurtles/todomvc#5

PersistentAction built its id with crypto.randomUUID, which browsers only
give you on https and localhost, so the first enqueue on any other plain http
origin threw and the action was lost.

Adds randomUUID to functions.ts: native when available, otherwise built
from getRandomValues, which is there on every origin. persistent-actions.ts
and files.ts both use it now, so the fallback lives in one place instead of
only in files.ts.

The files.ts boundary keeps the native path it had. Its fallback changes from
36 Math.random letters to the UUID, which is still a valid multipart
boundary, and it no longer reads self, which is undefined in Node. I also
dropped the TODO above it about randomUUID breaking the Cypress tests, since
that code already called native randomUUID whenever it existed and this
change keeps that path the same. Say the word if you want it back.

Testing

All tests are passing. The new ones cover the native path, the fallback,
uniqueness, and building a PersistentAction with randomUUID hidden, which
is the reported symptom. Taking the fallback back out makes that last one fail
with the error from the issue.

Also tested against TodoMVC, which is where we hit this. I packed this branch,
installed it there, and removed TodoMVC's own workaround so nothing but this
fix could do the work. On a plain http LAN address (isSecureContext false,
no randomUUID from the browser) signup worked, a write made while the server
was unreachable went into the queue with a real UUID id, and it landed once the
server was back. TodoMVC's own suite passed too.

AI Usage: Claude Opus 5 Max

Self review: Approved

[Copilot]

🟡 Changes recommended

Strengthen the native UUID test by asserting the delegated sentinel value.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes persistent action IDs on plain HTTP origins by adding a secure UUID fallback and sharing it across action and file handling.

Changes:

• Added native/fallback UUID generation.
• Updated persistent actions and multipart boundaries.
• Added UUID and regression tests.

File summaries

File	Summary
src/random-uuid.test.ts	Tests UUID behavior and persistence regression.
src/persistent-actions.ts	Uses the shared UUID generator for action IDs.
src/index.test.ts	Registers the new test suite.
src/functions.ts	Adds native/fallback UUID generation.
src/files.ts	Uses UUIDs for multipart boundaries.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[akhileshthite]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

Review: fix: build persistent action ids without crypto.randomUUID

I verified the fallback implementation independently (100k generated IDs: all valid RFC 9562 v4, all unique, valid as a multipart boundary token, prototype restore works) and type-checked the suggested simplification below with tsc --strict. The core fix is correct. Issues found:

---

1. 🟡 A self.crypto reference the PR description claims was removed still exists

• Addressed
• Dismissed

The PR description says of files.ts: "it no longer reads self, which is undefined in Node." That is only true for the boundary line. The same file still reads the browser-only global self at src/files.ts:194, inside the default cipher handler:

    if (!IKM) {
      IKM = new Uint8Array(33)
      self.crypto.getRandomValues(IKM)
    }

aes256gcmHandlers.upload is wired up as the default handler (src/files.ts:291), so any Node client calling chelonia/file/upload without an explicit IKM throws ReferenceError: self is not defined — the exact class of bug this PR set out to fix. Since the goal is "crypto access works on every origin/runtime", this leftover is inconsistent with the change and should use globalThis like the rest of the codebase:

    if (!IKM) {
      IKM = new Uint8Array(33)
      globalThis.crypto.getRandomValues(IKM)
    }

(getRandomValues itself is not secure-context-gated, so no fallback is needed here — only the self → globalThis change is required.)

---

2. 🟡 DRY: the UUIDV4 template-literal type is now declared three times, and the as cast is unnecessary

• Addressed
• Dismissed

src/persistent-actions.ts:14 already exports the exact type the new function returns:

export type UUIDV4 = `${string}-${string}-${string}-${string}-${string}`;

but src/functions.ts:86 re-declares it inline in the signature, and src/functions.ts:95-97 declares it a second time for the cast:

export const randomUUID = (): `${string}-${string}-${string}-${string}-${string}` => {

  return `${hex.slice(0, 8)}-${hex.slice(8, 12)}-${hex.slice(12, 16)}-` +
    `${hex.slice(16, 20)}-${hex.slice(20)}` as
    `${string}-${string}-${string}-${string}-${string}`

Two improvements:

1. Move UUIDV4 to src/types.ts (the natural home; avoid importing it from persistent-actions.js, which would create a module cycle with the new functions.js import), re-export it from persistent-actions.ts for backwards compatibility, and use it as the return type.
2. The two-part concatenation with the trailing as is only needed because contextual typing doesn't flow through +. A single template literal is contextually typed by the annotated return type, so the cast disappears entirely (verified with tsc --strict; the repo's max-len config has ignoreTemplateLiterals: true, so the long line is fine):

export const randomUUID = (): UUIDV4 => {
  if (typeof globalThis.crypto?.randomUUID === 'function') {
    return globalThis.crypto.randomUUID()
  }
  const bytes = globalThis.crypto.getRandomValues(new Uint8Array(16))
  // Version 4 and variant 1, the two fields RFC 9562 pins down.
  bytes[6] = (bytes[6] & 0x0f) | 0x40
  bytes[8] = (bytes[8] & 0x3f) | 0x80
  const hex = Array.from(bytes, (b) => b.toString(16).padStart(2, '0')).join('')
  return `${hex.slice(0, 8)}-${hex.slice(8, 12)}-${hex.slice(12, 16)}-${hex.slice(16, 20)}-${hex.slice(20)}`
}

---

3. ⚪️ Inconsistent guarding of globalThis.crypto

• Addressed
• Dismissed

src/functions.ts:87-90 uses optional chaining as if globalThis.crypto might be absent, then dereferences it unguarded two lines later:

  if (typeof globalThis.crypto?.randomUUID === 'function') {
    return globalThis.crypto.randomUUID()
  }
  const bytes = globalThis.crypto.getRandomValues(new Uint8Array(16))

If globalThis.crypto were ever undefined (Node < 19), the guard would fall through to line 90 and throw anyway — the ?. only delays the failure and misleads readers into thinking the fallback is more robust than it is. Either drop the ?. (all supported runtimes — Node 22, Deno, browsers — always expose crypto) or keep ?. and let it throw a clearer error. Low importance given the documented Node 22/Deno requirement, but pick one story.

---

4. ⚪️ Test helper restores the prototype method with plain assignment instead of its original descriptor

• Addressed
• Dismissed

src/random-uuid.test.ts:11-20 deletes randomUUID off Crypto.prototype and restores it by assignment:

const withoutNative = <T>(fn: () => T): T => {
  const proto = Object.getPrototypeOf(globalThis.crypto)
  const native = proto.randomUUID
  delete proto.randomUUID
  try {
    return fn()
  } finally {
    proto.randomUUID = native
  }
}

This works in Node today (Crypto.prototype.randomUUID is writable), but it silently mutates a shared builtin and would restore a subtly different property (own data property vs. whatever the original descriptor was). It's also only correct because fn is synchronous — if someone later passes an async callback, the prototype is restored before the body runs and the test's premise silently evaporates. A descriptor-based version is both more faithful and makes the sync-only constraint harder to violate accidentally:

// Runs `fn` with `crypto.randomUUID` hidden, the way a browser leaves it on a
// plain http origin. Only safe for synchronous `fn`.
const withoutNative = <T>(fn: () => T): T => {
  const proto = Object.getPrototypeOf(globalThis.crypto)
  const descriptor = Object.getOwnPropertyDescriptor(proto, 'randomUUID')!
  Object.defineProperty(proto, 'randomUUID', { value: undefined, configurable: true })
  try {
    return fn()
  } finally {
    Object.defineProperty(proto, 'randomUUID', descriptor)
  }
}

---

No other issues found: the v4 bit-twiddling and hyphen placement are correct, all remaining crypto.randomUUID call sites were converted (grep confirms only functions.ts touches it now), the import member sorting and index.test.ts placement follow existing conventions, and the removed Cypress TODO was safe to drop since the native code path is unchanged.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[Copilot]

🟡 Changes recommended

The regression test does not exercise the fallback path used by PersistentAction.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (1)

src/random-uuid.test.ts:60

• This test imports PersistentAction before withRandomUUID removes the native method, so persistent-actions.js has already cached the native branch. The constructor assertion therefore never exercises the plain-HTTP path that this PR fixes; the fallback tests only call a freshly imported functions.js export. Please load the persistent-actions module after hiding randomUUID (or otherwise inject the fallback) and construct the action in that state.

  it('gives a persistent action a usable id', () => {
    assert.match(new PersistentAction(['log', 'hello']).id, V4)

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The reviewed changes address the plain-HTTP UUID failure and include focused regression coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite