[stevehu]

Snapshot comparison used ordinary objects indexed by snapshot IDs, and request-ID paths could fall back to Math.random. Harden both cell dictionaries, remove weak randomness, and handle secure sign-in failures visibly.

Addresses #938 (all six frontend findings):

• A03 / rows 4–5: build regular/detail cell dictionaries with null prototypes. Tests cover proto, constructor, prototype and toString. Structured clone restores Object.prototype but preserves these as own data properties; construction is protected before transfer and the UI only reads the populated cells. Tests check those transfer semantics explicitly.
• A09 / row 24: chat uses a shared Web Crypto UUID helper; clientMessageId is a compatibility re-export. Native randomUUID is preferred, with UUID-v4 bytes from getRandomValues as fallback. If neither exists, preserve the draft and show an error, including expired-session renewal.
• A09 / row 26: generated JSON-RPC IDs use a per-page sequence. These IDs only correlate responses, so ordinary legacy queries/commands remain usable without Web Crypto. Explicit caller IDs are preserved. No Math.random fallback remains in this path.
• A09 / row 27: Knowledge command idempotency keys require the shared cryptographic helper and fail before dispatch if unavailable. Ordinary aggregate updates do not request randomness or an idempotency key. The existing payload test covers this without a duplicate test.
• A10 / row 25: repository history confirms that the former ProfileMenu generator protected OAuth state. It was previously replaced by native crypto.randomUUID in 7f200a3; signIn now uses the shared helper for its cryptographic fallback as well. The menu catches rejection, closes the modal menu, displays an accessible error and permits retry. Menu integration tests exercise the real signIn helper rather than mocking it, covering native/byte generation, exact stored/redirected state, absent crypto and retry.

The documentation maps all six rows, records same-repository historical evidence and states deployment closure requirements. It does not make pinned cross-repository backend claims. This is a scoped migration for the assessed paths, not repository-wide UUID consolidation: other native randomUUID calls, the already-secure form idempotency helper and unrelated Workflow code are unchanged. Caller tests verify integration/no-dispatch behavior; helper tests own the UUID bit/entropy-source details.

Validation on the revised source tree:

• npm run test:run: 166 files, 1,153 tests passed.
• npm run build: passed.
• npm run lint: zero errors, 215 warnings (same baseline count).
• node_modules/.bin/tsc --noEmit -p tsconfig.controller.json: passed.
• Full tsc --noEmit --pretty false: remains non-green with 172 baseline errors; diagnostics match untouched master after normalizing shifted line numbers. No new diagnostics.
• git diff --check: passed. GitNexus staged analysis reports LOW risk for this review update. The original HIGH knowledgeCommand impact scope was explicitly accepted before its change.

No live services, databases or credentials were changed. The exact partner scan revision, deployed artifacts, live OAuth/server validation, backend replay isolation and partner rescan remain deployment closure work; merging this PR does not establish those results.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

[stevehu]

Review of 8ac67b2. I read the diff and the surrounding code; I did not run the tests or CI.

The snapshot dictionary hardening and the chat/knowledge/JSON-RPC ID changes look sound. The main points are consistency and failure handling.

1. signIn.ts still calls crypto.randomUUID() directly (line 31), not the shared secureRandomUUID helper. On a plain-http origin sign-in throws a raw TypeError, and the new tests assert only a bare .rejects.toThrow().
2. ProfileMenu.tsx:37: handleSignIn returns signIn()'s promise with no catch. When crypto is unavailable the user clicks Sign In and nothing happens, with no message.
3. fetchClient.ts:47: The legacy-to-JSON-RPC conversion now throws without Web Crypto, and it runs for every legacy payload, queries included. The id is only a correlation id, so this turns a missing-crypto context into a total API outage for legacy callers.
4. ProfileMenu.security.test.tsx: It mocks signIn and only asserts it was called, so it cannot detect a weak state generator coming back in ProfileMenu, which the docs say it locks in.
5. Helper adopted at four sites: Direct crypto.randomUUID() calls remain in OperationalStoreAdmin, WorkflowStart, workflowAdminClient, WorkflowEditor, A2aBindings, workflowToolClient and KnowledgeBaseWorkspace (promotionId). Behavior in insecure contexts depends on which file generates the ID.
6. snapshotComparison.ts:40: The null prototype is lost at the worker boundary, since structuredClone restores Object.prototype, and the Record type hides that these objects have no Object methods. A Map would not depend on prototype state.
7. Minor:
   • clientMessageId is now a one-line wrapper over secureRandomUUID.
   • There are three overlapping crypto-ID helpers (secureRandomUUID, createIdempotencyKey, WorkflowEditor's inline getRandomValues).
   • One new knowledgeApi test duplicates an existing one.
8. docs/security/partner-checkmarx-938.md: The cross-repo backend observations (light-portal and light-fabric at pinned SHAs) cannot be verified or kept current from this PR.

[stevehu]

Follow-up review of 7a8a8cd. I read the diff; I did not run the tests or CI.

Fixed

• signIn.ts now uses secureRandomUUID, including the getRandomValues fallback.
• ProfileMenu catches a failed sign-in and shows an error alert; the user can retry.
• fetchClient uses a per-page sequence for legacy JSON-RPC correlation ids, so legacy calls no longer need Web Crypto. The docs state it is not an idempotency key, credential or OAuth state.
• The ProfileMenu test now drives the real flow: the state reaches the redirect URL and localStorage, Math.random is not used, and the failure is visible and retryable.
• clientMessageId is a plain re-export, and the duplicate knowledge-API test is merged.
• The doc no longer makes unverifiable claims about light-portal and light-fabric.

Skipped (accepted, now documented)

• Other direct crypto.randomUUID() call sites remain; the doc says the migration covers only the assessed paths.
• Null prototypes are still lost after worker transfer; a new doc section and test state that this PR does not promise otherwise.

Note: the sequence ids restart at 1 on every page load, so two tabs can use the same id. That is fine as long as the backend only echoes it.