BCH-1155: Populate ExecutionStreamUUID in WorkflowExecution.GetByID#401
BCH-1155: Populate ExecutionStreamUUID in WorkflowExecution.GetByID#401saltpy-cs wants to merge 1 commit into
Conversation
Add a computed ExecutionStreamUUID field to WorkflowExecution and derive it deterministically in GetByID using a pure SHA256-based helper that mirrors the algorithm already used by EvidenceIntegration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a computed ChangesExecution Stream UUID Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/swagger.json`:
- Around line 40223-40226: The schema for the computed field
execution_stream_uuid currently only declares "type": "string" but should
declare a UUID format to enforce the intended shape; update the JSON schema for
the execution_stream_uuid property to add "format": "uuid" alongside "type":
"string" so generated clients and validators will treat it as a UUID.
In `@docs/swagger.yaml`:
- Around line 8922-8924: The OpenAPI schema entry for execution_stream_uuid is
too permissive; update the execution_stream_uuid property (in docs/swagger.yaml)
to include format: uuid and readOnly: true while keeping type: string and the
existing description ("Computed field (not persisted)") so generated clients and
validators treat it as a server-computed UUID-only, read-only field.
In `@internal/service/relational/workflows/stream_uuid.go`:
- Around line 23-24: The code currently ignores the error from uuid.Parse when
building streamUUID (streamUUID, _ := uuid.Parse(...)); change it to capture and
handle the error (streamUUID, err := uuid.Parse(...)) and propagate it by
updating the function signature to return (uuid.UUID, error) or otherwise return
uuid.Nil with a wrapped error; update callers to handle the error so a malformed
UUID assembly cannot silently produce a uuid.Nil used for execution_stream_uuid.
In `@internal/service/relational/workflows/workflow_execution_service_test.go`:
- Around line 119-127: The test currently only asserts stability of
ExecutionStreamUUID across fetches; update it to also assert the canonical
expected UUID and guard against nil on the second retrieval. After calling
retrieved2, require.NoError(t, err) (already present) and add require.NotNil(t,
retrieved2.ExecutionStreamUUID) before dereferencing it, then compute the
expected UUID using the same deterministic generator the service uses (e.g., the
function that produces the execution stream UUID in your implementation — call
it directly, e.g., computeExecutionStreamUUID(execution) or
ExecutionStreamUUIDFor(execution)) and assert assert.Equal(t, expectedUUID,
*retrieved2.ExecutionStreamUUID) instead of only comparing retrieved to
retrieved2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1e82e79-2c59-4f5d-a466-1f6014476acc
📒 Files selected for processing (7)
docs/docs.godocs/swagger.jsondocs/swagger.yamlinternal/service/relational/workflows/stream_uuid.gointernal/service/relational/workflows/workflow_execution.gointernal/service/relational/workflows/workflow_execution_service.gointernal/service/relational/workflows/workflow_execution_service_test.go
| "execution_stream_uuid": { | ||
| "description": "Computed field (not persisted)", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add UUID format to strengthen the API contract.
execution_stream_uuid is documented as a UUID but only typed as string. Add "format": "uuid" so generated clients and validators enforce the intended shape consistently.
Suggested diff
"execution_stream_uuid": {
"description": "Computed field (not persisted)",
- "type": "string"
+ "type": "string",
+ "format": "uuid"
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/swagger.json` around lines 40223 - 40226, The schema for the computed
field execution_stream_uuid currently only declares "type": "string" but should
declare a UUID format to enforce the intended shape; update the JSON schema for
the execution_stream_uuid property to add "format": "uuid" alongside "type":
"string" so generated clients and validators will treat it as a UUID.
| execution_stream_uuid: | ||
| description: Computed field (not persisted) | ||
| type: string |
There was a problem hiding this comment.
Tighten API contract for computed UUID field
On Line 8924, execution_stream_uuid is documented as a plain string, but this field is semantically a UUID and computed server-side. Please add format: uuid and readOnly: true so generated clients and validators enforce the intended contract.
Suggested patch
execution_stream_uuid:
description: Computed field (not persisted)
type: string
+ format: uuid
+ readOnly: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| execution_stream_uuid: | |
| description: Computed field (not persisted) | |
| type: string | |
| execution_stream_uuid: | |
| description: Computed field (not persisted) | |
| type: string | |
| format: uuid | |
| readOnly: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/swagger.yaml` around lines 8922 - 8924, The OpenAPI schema entry for
execution_stream_uuid is too permissive; update the execution_stream_uuid
property (in docs/swagger.yaml) to include format: uuid and readOnly: true while
keeping type: string and the existing description ("Computed field (not
persisted)") so generated clients and validators treat it as a server-computed
UUID-only, read-only field.
| streamUUID, _ := uuid.Parse(h[:8] + "-" + h[8:12] + "-" + h[12:16] + "-" + h[16:20] + "-" + h[20:32]) | ||
| return streamUUID |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no ignored uuid.Parse errors remain in workflows package.
rg -nP 'uuid\.Parse\([^)]*\)' internal/service/relational/workflows -C2Repository: compliance-framework/api
Length of output: 545
Handle uuid.Parse errors explicitly in stream_uuid construction.
In internal/service/relational/workflows/stream_uuid.go (lines 23-24), uuid.Parse is called with a dropped error (streamUUID, _ := uuid.Parse(...)). While h is currently built as a fixed 32-hex-char string (sha256 → hash[:16] → hex.EncodeToString), ignoring the parse error makes this fragile: any future change to the UUID string assembly can silently produce uuid.Nil and generate broken execution_stream_uuid values.
Proposed fix
- streamUUID, _ := uuid.Parse(h[:8] + "-" + h[8:12] + "-" + h[12:16] + "-" + h[16:20] + "-" + h[20:32])
- return streamUUID
+ uuidStr := h[:8] + "-" + h[8:12] + "-" + h[12:16] + "-" + h[16:20] + "-" + h[20:32]
+ streamUUID, err := uuid.Parse(uuidStr)
+ if err != nil {
+ return uuid.Nil
+ }
+ return streamUUID📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| streamUUID, _ := uuid.Parse(h[:8] + "-" + h[8:12] + "-" + h[12:16] + "-" + h[16:20] + "-" + h[20:32]) | |
| return streamUUID | |
| uuidStr := h[:8] + "-" + h[8:12] + "-" + h[12:16] + "-" + h[16:20] + "-" + h[20:32] | |
| streamUUID, err := uuid.Parse(uuidStr) | |
| if err != nil { | |
| return uuid.Nil | |
| } | |
| return streamUUID |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/relational/workflows/stream_uuid.go` around lines 23 - 24,
The code currently ignores the error from uuid.Parse when building streamUUID
(streamUUID, _ := uuid.Parse(...)); change it to capture and handle the error
(streamUUID, err := uuid.Parse(...)) and propagate it by updating the function
signature to return (uuid.UUID, error) or otherwise return uuid.Nil with a
wrapped error; update callers to handle the error so a malformed UUID assembly
cannot silently produce a uuid.Nil used for execution_stream_uuid.
| // ExecutionStreamUUID must be populated and deterministic | ||
| require.NotNil(t, retrieved.ExecutionStreamUUID) | ||
| assert.NotEqual(t, uuid.Nil, *retrieved.ExecutionStreamUUID) | ||
|
|
||
| // Calling again should produce the same UUID | ||
| retrieved2, err := service.GetByID(execution.ID) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, *retrieved.ExecutionStreamUUID, *retrieved2.ExecutionStreamUUID) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert against the canonical computed UUID, not only repeatability.
Lines 124-126 only prove the value is stable across calls. A wrong-but-deterministic seed composition would still pass. Add an explicit expected-value assertion and a nil guard for the second retrieval pointer before dereference.
Proposed test hardening
// ExecutionStreamUUID must be populated and deterministic
require.NotNil(t, retrieved.ExecutionStreamUUID)
assert.NotEqual(t, uuid.Nil, *retrieved.ExecutionStreamUUID)
+ expected := ComputeExecutionStreamUUID(*workflowDef.ID, *instance.ID, *execution.ID)
+ assert.Equal(t, expected, *retrieved.ExecutionStreamUUID)
// Calling again should produce the same UUID
retrieved2, err := service.GetByID(execution.ID)
require.NoError(t, err)
+ require.NotNil(t, retrieved2.ExecutionStreamUUID)
assert.Equal(t, *retrieved.ExecutionStreamUUID, *retrieved2.ExecutionStreamUUID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/relational/workflows/workflow_execution_service_test.go`
around lines 119 - 127, The test currently only asserts stability of
ExecutionStreamUUID across fetches; update it to also assert the canonical
expected UUID and guard against nil on the second retrieval. After calling
retrieved2, require.NoError(t, err) (already present) and add require.NotNil(t,
retrieved2.ExecutionStreamUUID) before dereferencing it, then compute the
expected UUID using the same deterministic generator the service uses (e.g., the
function that produces the execution stream UUID in your implementation — call
it directly, e.g., computeExecutionStreamUUID(execution) or
ExecutionStreamUUIDFor(execution)) and assert assert.Equal(t, expectedUUID,
*retrieved2.ExecutionStreamUUID) instead of only comparing retrieved to
retrieved2.
1 participant
Add a computed ExecutionStreamUUID field to WorkflowExecution and derive it deterministically in GetByID using a pure SHA256-based helper that mirrors the algorithm already used by EvidenceIntegration.
Summary by CodeRabbit
New Features
execution_stream_uuidfield to workflow executions, enabling unique identification and tracking of each execution.Documentation
execution_stream_uuidfield.Tests