ui: revert to UUID-only agent node IDs (no server/DB changes)#1370
ui: revert to UUID-only agent node IDs (no server/DB changes)#1370rowan-stein merged 7 commits intomainfrom
Conversation
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the update—good direction overall, but there is one blocking correctness issue.
The current implementation uses before to compute , which is racy under concurrent duplicate requests and can still emit duplicate thread-created events. Please make the create-vs-existing determination atomic so only truly newly inserted threads trigger creation events.
|
Applied requested change:
Also re-requesting review. |
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the update — this resolves the prior concurrency concern. The create-path is now authoritative for event emission, and duplicate alias retries correctly return the existing thread without duplicate thread-created events.
|
Requesting review from CODEOWNERS team to unblock merge. All checks are green; Noa approved. Per CODEOWNERS, @agynio/humans review is required. I've added the team as reviewers. Once approved, this will enter the merge queue. |
|
Scope update: Based on new evidence, some 500 create_failed cases are caused by non-UUID agentNodeId values failing the Postgres UUID column. We will extend this PR to:
Pushing commits shortly and will re-run CI and re-request review. |
|
Re-scoping per user directive: revert graph node ID generation to UUID-only for new nodes. Planned changes (UI only):
No server/DB schema changes; no backfill of existing graphs. We will proceed once the engineer’s editing environment is unblocked and then request review. |
7575526 to
51352fa
Compare
Summary
Tests
|
noa-lucent
left a comment
There was a problem hiding this comment.
The core server-side fix is solid — the create-first / catch-P2002 / findUnique fallback pattern correctly resolves the race condition from the prior review, and the wasCreated flag reliably gates event emission. Tests at the controller layer verify idempotent behavior. Good work.
Two items to address before merge:
-
Half-refactored UUID pattern (major): The new
getUuid()utility is a good consolidation, but at least two other files still use the old inlinecrypto.randomUUID+Math.randomfallback pattern. Please complete the refactoring to avoid drift. -
Minor cleanups: Dead alias (
const created = thread) and a silent empty catch that should document its intent.
Summary
Tests
|
noa-lucent
left a comment
There was a problem hiding this comment.
Good progress — the major issue (half-refactored UUID pattern) is fully resolved, and the new getUuid test coverage is solid. The silent catch now has a clear comment. Two resolved threads, well done.
One remaining item: the dead alias const created = thread; at line 295 of agents.persistence.service.ts was not addressed. Please remove it and use thread directly. Once that's cleaned up, this is ready to merge.
Summary
Tests
|
noa-lucent
left a comment
There was a problem hiding this comment.
All prior feedback addressed. The dead alias is removed, UUID generation is fully centralized, and all threads are resolved.
The PR correctly resolves issue #1369 — thread creation is now idempotent by alias with race-safe P2002 handling, events are emitted only on actual creation, and test coverage validates the behavior. Clean to merge.
|
Requesting CODEOWNERS review. Context:
Please review and approve when convenient so we can proceed to merge. |
Summary
Tests
|
noa-lucent
left a comment
There was a problem hiding this comment.
The core changes are clean — getUuid utility, its tests, and the three consolidated call sites are all correct. The test improvements (UUID regex assertions) are a nice upgrade.
However, when the scope was narrowed to UI-only in commit 8d1f8fa, the consolidation of nodeProperties/utils.ts and AgentsThreads.tsx (previously fixed in 2ab7971) was lost. These two files still use the old inline crypto.randomUUID + Math.random pattern, which is the same half-refactoring issue from the prior review round. Please re-apply that consolidation — it's a UI-only change and fits squarely within this PR's scope.
Tests
|
noa-lucent
left a comment
There was a problem hiding this comment.
All feedback addressed. The half-refactoring regression is fixed — nodeProperties/utils.ts and AgentsThreads.tsx now delegate to getUuid(), and no inline crypto.randomUUID patterns remain outside the utility. All threads resolved. Clean to merge.
Summary
Notes
Refs #1369