lisa/feat/workflow-evidence-for-dashboards#411
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds FilterSyncService and deterministic relational.Filter lifecycles for workflow definitions; wraps workflow and control-relationship writes in DB transactions that sync/delete filters; augments EvidenceIntegration to label completed/failed workflow evidence and compute completion expirations; tests added/extended for these behaviors. ChangesWorkflow Filter Synchronization and Evidence Tracking
Sequence DiagramsequenceDiagram
participant Client
participant ControlRelationshipHandler
participant DB_Transaction
participant FilterSyncService
participant Database
Client->>ControlRelationshipHandler: Create/Update/Delete relationship
ControlRelationshipHandler->>DB_Transaction: begin transaction
DB_Transaction->>Database: write relationship changes
DB_Transaction->>FilterSyncService: SyncFilterForDefinition(definitionID)
FilterSyncService->>Database: load definition + relationships
FilterSyncService->>Database: resolve relational.Controls
FilterSyncService->>Database: upsert relational.Filter and replace Controls
DB_Transaction->>Database: commit
ControlRelationshipHandler->>Client: 2xx / 4xx
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/api/handler/workflows/control_relationship.go (1)
227-251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
descriptionin the partial update map.
UpdateControlRelationshipRequeststill exposesdescription, but this path never writes it, so clients can send a new description and still get a 200 with the old value unchanged.Proposed fix
if req.Strength != nil { updates["strength"] = *req.Strength } + if req.Description != nil { + updates["description"] = *req.Description + } var relationship *workflows.ControlRelationship err = h.db.Transaction(func(tx *gorm.DB) error {🤖 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/api/handler/workflows/control_relationship.go` around lines 227 - 251, The partial-update logic builds an updates map but omits the description field, so UpdateControlRelationshipRequest.Description is never written; add a check for req.Description != nil and set updates["description"] = *req.Description alongside relationship_type and strength before calling tx.Model(...).Updates(updates) (preserving the existing use of the updates map and transaction in the handler and keeping GetByID/SyncFilterForDefinition calls unchanged).internal/workflow/evidence.go (3)
233-246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse completion timestamps for completion evidence (
descriptionandEnd).For
"completed"evidence, the code still usesStartedAtin the description and setsEndtoStartedAt. This misrepresents event timing and can skew latest-evidence selection that orders byend DESC.Suggested fix
case "completed": stream, err = e.GetOrCreateInstanceStream(ctx, execution.WorkflowInstanceID) if err != nil { return fmt.Errorf("failed to get instance stream: %w", err) } + if execution.CompletedAt == nil { + return fmt.Errorf("workflow execution completed_at is nil for completed evidence") + } title = fmt.Sprintf("Workflow Execution Completed: %s", definition.Name) description = fmt.Sprintf("Workflow execution '%s' completed at %s", execution.ID.String(), - execution.StartedAt.Format(time.RFC3339), + execution.CompletedAt.Format(time.RFC3339), ) } + endTime := *execution.StartedAt + if status == "completed" && execution.CompletedAt != nil { + endTime = *execution.CompletedAt + } evidence := &relational.Evidence{ UUID: stream.UUID, Title: title, Description: description, Start: *execution.StartedAt, - End: *execution.StartedAt, + End: endTime,Also applies to: 255-261
🤖 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/workflow/evidence.go` around lines 233 - 246, The "completed" evidence is using execution.StartedAt for the description and End, which misrepresents timing; update the completed branch so the description uses execution.CompletedAt.Format(time.RFC3339) instead of execution.StartedAt.Format(...), and set evidence.End = *execution.CompletedAt (instead of *execution.StartedAt); also make the same change in the similar block around the other occurrence (the later branch at lines ~255-261) so both description and End reference execution.CompletedAt for completion events.
217-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard unsupported
statusvalues before dereferencingstream.If
statusis neither"started"nor"completed",streamstays nil andUUID: stream.UUIDpanics at runtime. Add adefaultcase returning an error.Suggested fix
var title string var description string var stream *relational.Evidence switch status { case "started": stream, err = e.GetOrCreateExecutionStream(ctx, execution.ID) if err != nil { return fmt.Errorf("failed to get execution stream: %w", err) } title = fmt.Sprintf("Workflow Execution Started: %s", definition.Name) description = fmt.Sprintf("Workflow execution '%s' started at %s", execution.ID.String(), execution.StartedAt.Format(time.RFC3339), ) case "completed": stream, err = e.GetOrCreateInstanceStream(ctx, execution.WorkflowInstanceID) if err != nil { return fmt.Errorf("failed to get instance stream: %w", err) } title = fmt.Sprintf("Workflow Execution Completed: %s", definition.Name) description = fmt.Sprintf("Workflow execution '%s' completed at %s", execution.ID.String(), execution.StartedAt.Format(time.RFC3339), ) + default: + return fmt.Errorf("unsupported workflow execution evidence status: %s", status) }🤖 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/workflow/evidence.go` around lines 217 - 242, The switch on status in the function can leave stream nil if status is not "started" or "completed", causing a panic when creating the Evidence with stream.UUID; add a default case to the switch that returns a descriptive error (e.g., fmt.Errorf("unsupported status: %s", status)) to guard against unsupported values, ensuring GetOrCreateExecutionStream/GetOrCreateInstanceStream are only used when stream is valid before constructing the relational.Evidence.
200-202:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix incomplete error message text.
Line 201 currently returns
"workflow execution is not in status"(missing expected statuses), which makes debugging harder.🤖 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/workflow/evidence.go` around lines 200 - 202, The error message returned from the check in the evidence workflow (the conditional that tests status == "completed" && execution.Status != "in_progress" && execution.Status != "completed") is missing the expected statuses; update the fmt.Errorf call in that block (the return in the function around execution.Status) to include the expected states (e.g., "in_progress" or "completed") so it reads something like "workflow execution is not in expected status (in_progress or completed), status: %s" to make debugging clearer.
🤖 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 `@internal/service/relational/workflows/filter_service_test.go`:
- Around line 79-80: The test is accessing embedded condition fields via
Scope.Condition.Label and Scope.Condition.Value which triggers staticcheck
QF1008; update the assertions in filter_service_test.go to use the promoted
fields Scope.Label and Scope.Value instead (i.e., replace
filter.Filter.Data().Scope.Condition.Label with filter.Filter.Data().Scope.Label
and filter.Filter.Data().Scope.Condition.Value with
filter.Filter.Data().Scope.Value) so the test uses the embedded fields directly.
In `@internal/service/relational/workflows/filter_service.go`:
- Around line 3-15: The DB lookup for control IDs uses exact equality against
relational.Control.ID (e.g., where "id = ?"), causing mismatches when stored IDs
differ by case; change those queries to be case-insensitive by comparing
lowercased values (e.g., use WHERE LOWER(id) = ? with strings.ToLower(id) or
GORM's Where("LOWER(id) = ?", strings.ToLower(id))). Update every lookup in the
filter_service.go block that resolves synced controls (the queries that
reference relational.Control.ID / "id = ?") so they use LOWER(...) comparisons
to ensure AC-1 vs ac-1 match.
---
Outside diff comments:
In `@internal/api/handler/workflows/control_relationship.go`:
- Around line 227-251: The partial-update logic builds an updates map but omits
the description field, so UpdateControlRelationshipRequest.Description is never
written; add a check for req.Description != nil and set updates["description"] =
*req.Description alongside relationship_type and strength before calling
tx.Model(...).Updates(updates) (preserving the existing use of the updates map
and transaction in the handler and keeping GetByID/SyncFilterForDefinition calls
unchanged).
In `@internal/workflow/evidence.go`:
- Around line 233-246: The "completed" evidence is using execution.StartedAt for
the description and End, which misrepresents timing; update the completed branch
so the description uses execution.CompletedAt.Format(time.RFC3339) instead of
execution.StartedAt.Format(...), and set evidence.End = *execution.CompletedAt
(instead of *execution.StartedAt); also make the same change in the similar
block around the other occurrence (the later branch at lines ~255-261) so both
description and End reference execution.CompletedAt for completion events.
- Around line 217-242: The switch on status in the function can leave stream nil
if status is not "started" or "completed", causing a panic when creating the
Evidence with stream.UUID; add a default case to the switch that returns a
descriptive error (e.g., fmt.Errorf("unsupported status: %s", status)) to guard
against unsupported values, ensuring
GetOrCreateExecutionStream/GetOrCreateInstanceStream are only used when stream
is valid before constructing the relational.Evidence.
- Around line 200-202: The error message returned from the check in the evidence
workflow (the conditional that tests status == "completed" && execution.Status
!= "in_progress" && execution.Status != "completed") is missing the expected
statuses; update the fmt.Errorf call in that block (the return in the function
around execution.Status) to include the expected states (e.g., "in_progress" or
"completed") so it reads something like "workflow execution is not in expected
status (in_progress or completed), status: %s" to make debugging clearer.
🪄 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: d0921715-ab4c-4772-9e0f-2bdc473515db
📒 Files selected for processing (10)
internal/api/handler/oscal/profile_compliance_workflow_integration_test.gointernal/api/handler/workflows/common_test.gointernal/api/handler/workflows/control_relationship.gointernal/api/handler/workflows/workflow_definition.gointernal/api/handler/workflows/workflow_definition_integration_test.gointernal/service/relational/workflows/constants.gointernal/service/relational/workflows/filter_service.gointernal/service/relational/workflows/filter_service_test.gointernal/workflow/evidence.gointernal/workflow/evidence_test.go
There was a problem hiding this comment.
Pull request overview
This PR extends the workflow subsystem so dashboard/compliance-progress views can treat workflow executions as evidence, including expiration based on cadence/grace periods, and ensures workflow definitions keep an up-to-date relational.Filter reflecting their active control relationships (case-insensitive control ID matching).
Changes:
- Add workflow coverage labels (
_policy,_plugin) and computedExpirestimestamps to workflow completion evidence, plus tests for expiry/cadence behavior. - Introduce
FilterSyncServiceand wire it into workflow-definition and control-relationship handlers to create/update/delete the corresponding workflow filter and its associated controls. - Add integration tests validating workflow completion evidence impacts profile compliance progress and that workflow filter cleanup occurs on definition deletion.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/workflow/evidence.go | Adds workflow coverage labels and expiry computation for completion evidence; adjusts labeling behavior across streams. |
| internal/workflow/evidence_test.go | Expands evidence tests for new labels/status/expiry and failure-evidence labeling behavior. |
| internal/service/relational/workflows/filter_service.go | New service to deterministically create/update/delete workflow filters and sync their control associations. |
| internal/service/relational/workflows/filter_service_test.go | Unit tests for filter sync behavior, including case-insensitive control ID matching and delete cleanup. |
| internal/service/relational/workflows/constants.go | Introduces workflow evidence label constants and policy value helper. |
| internal/api/handler/workflows/workflow_definition.go | Wraps create/update/delete in DB transactions and triggers filter sync/cleanup. |
| internal/api/handler/workflows/workflow_definition_integration_test.go | Ensures deleting a workflow definition also deletes the synced filter and join-table rows. |
| internal/api/handler/workflows/control_relationship.go | Syncs workflow filter controls after relationship create/update/delete/activate/deactivate within transactions. |
| internal/api/handler/workflows/common_test.go | Adds Control/Filter migrations needed by workflow handler tests. |
| internal/api/handler/oscal/profile_compliance_workflow_integration_test.go | New integration test asserting compliance progress includes workflow completion evidence and remains stable when newer executions are pending. |
Comments suppressed due to low confidence (1)
internal/workflow/evidence.go:246
- For the "completed" status, the description timestamp is using StartedAt and the evidence End time is always set to StartedAt. This makes completion evidence look like it ended when it started and can produce misleading completion timestamps; it also breaks expiry calculations when CompletedAt is present.
title = fmt.Sprintf("Workflow Execution Completed: %s", definition.Name)
description = fmt.Sprintf("Workflow execution '%s' completed at %s",
execution.ID.String(),
execution.StartedAt.Format(time.RFC3339),
)
}
// Create evidence record
evidence := &relational.Evidence{
UUID: stream.UUID,
Title: title,
Description: description,
Start: *execution.StartedAt,
End: *execution.StartedAt,
Labels: []relational.Labels{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
internal/workflow/evidence.go:246
- For "completed" workflow execution evidence,
Endis currently set to*execution.StartedAt. The latest-evidence selection logic usesevidences.end descper stream (seerelational.GetLatestEvidenceStreamsQuery), so usingStartedAthere can cause completion evidence to never be considered the latest record for the instance stream. It also risks a nil dereference ifStartedAtis nil.
// Create evidence record
evidence := &relational.Evidence{
UUID: stream.UUID,
Title: title,
Description: description,
Start: *execution.StartedAt,
End: *execution.StartedAt,
Labels: []relational.Labels{
internal/workflow/evidence.go:246
- For "completed" workflow execution evidence,
Endis currently set to*execution.StartedAt. The latest-evidence selection logic usesevidences.end descper stream (seerelational.GetLatestEvidenceStreamsQuery), so usingStartedAthere can cause completion evidence to never be considered the latest record for the instance stream. It also risks a nil dereference ifStartedAtis nil.
// Create evidence record
evidence := &relational.Evidence{
UUID: stream.UUID,
Title: title,
Description: description,
Start: *execution.StartedAt,
End: *execution.StartedAt,
Labels: []relational.Labels{
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/api/handler/workflows/control_relationship.go (1)
226-232:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Descriptionfield from request is not applied.
UpdateControlRelationshipRequestdefines aDescriptionfield (line 52), but it's not added to the updates map. Requests withdescriptionwill silently ignore the value.🐛 Proposed fix
if req.Strength != nil { updates["strength"] = *req.Strength } + if req.Description != nil { + updates["description"] = *req.Description + }🤖 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/api/handler/workflows/control_relationship.go` around lines 226 - 232, The request's Description field is never applied to the updates map; modify the UpdateControlRelationship handler to mirror the existing pattern for RelationshipType and Strength by checking if req.Description != nil and then set updates["description"] = *req.Description (using the same updates map and nil-check pattern used for req.RelationshipType and req.Strength in the function that handles UpdateControlRelationshipRequest).internal/api/handler/workflows/workflow_definition.go (1)
175-201: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRead outside transaction may cause stale data on concurrent updates.
GetByIDon line 175 reads the definition outside the transaction boundary, while the update on line 197 happens inside the transaction. If a concurrent request modifies the definition between the read and the transaction commit, those changes could be overwritten.Consider moving the initial read inside the transaction for consistency with the pattern used in
control_relationship.go:♻️ Move read inside transaction
- definition, err := h.service.GetByID(id) - if err != nil { - return h.HandleServiceError(ctx, err, "get", "workflow definition") - } - - if req.Name != nil { - definition.Name = *req.Name - } - if req.Description != nil { - definition.Description = *req.Description - } - if req.Version != nil { - definition.Version = *req.Version - } - if req.SuggestedCadence != nil { - definition.SuggestedCadence = *req.SuggestedCadence - } - if req.GracePeriodDays != nil { - definition.GracePeriodDays = req.GracePeriodDays - } - + var definition *workflows.WorkflowDefinition err = h.db.Transaction(func(tx *gorm.DB) error { - if err := workflows.NewWorkflowDefinitionService(tx).Update(id, definition); err != nil { + svc := workflows.NewWorkflowDefinitionService(tx) + var err error + definition, err = svc.GetByID(id) + if err != nil { + return err + } + + if req.Name != nil { + definition.Name = *req.Name + } + if req.Description != nil { + definition.Description = *req.Description + } + if req.Version != nil { + definition.Version = *req.Version + } + if req.SuggestedCadence != nil { + definition.SuggestedCadence = *req.SuggestedCadence + } + if req.GracePeriodDays != nil { + definition.GracePeriodDays = req.GracePeriodDays + } + + if err := svc.Update(id, definition); err != nil { return err } return workflows.NewFilterSyncService(tx, h.sugar).SyncFilterForDefinition(*id) })🤖 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/api/handler/workflows/workflow_definition.go` around lines 175 - 201, The read of the workflow definition (h.service.GetByID) happens outside the DB transaction and can be stale; move the GetByID call into the h.db.Transaction closure so the record is loaded within the same transaction used by workflows.NewWorkflowDefinitionService(tx).Update and workflows.NewFilterSyncService(tx).SyncFilterForDefinition; inside the transaction, use the tx-backed service to fetch the definition, apply req.* changes to that local definition, then call Update(id, definition) and SyncFilterForDefinition(*id) before returning to ensure atomic read-modify-write semantics.internal/workflow/evidence.go (1)
244-266:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
workflow_execution_completedevidenceEndto useexecution.CompletedAtIn
internal/workflow/evidence.go(lines 244-266), the evidence record is created withStart: *execution.StartedAtandEnd: *execution.StartedAtand, in thestatus == "completed"branch,Endis never updated to*execution.CompletedAt. This makes completion evidence a zero-length window and makesEvidence.End/evidence_endreflectStartedAt(unlikeAddExecutionCompletionEvidence, which setsEnd: *execution.CompletedAt). The existing completed-evidence test only checks the description, so it won’t catch this.🐛 Set
EndtoCompletedAtfor completed evidenceif status == "completed" { evidence.Status = datatypes.NewJSONType(oscalTypes_1_1_3.ObjectiveStatus{ State: "satisfied", }) + evidence.End = *execution.CompletedAt evidence.Labels = append(evidence.Labels, e.buildWorkflowCoverageLabels(*definition.ID)...) evidence.Expires = e.calculateCompletionEvidenceExpires(execution.CompletedAt, instance, definition) }🤖 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/workflow/evidence.go` around lines 244 - 266, The evidence record is incorrectly initialized with End set to *execution.StartedAt* causing completed evidence to have zero-length duration; in the evidence creation block that constructs the *relational.Evidence* (the struct assigned to variable `evidence`), update the End field to use *execution.CompletedAt* when `status == "completed"` (i.e., set `evidence.End = *execution.CompletedAt` inside the `if status == "completed"` branch), and ensure this happens before you call `e.calculateCompletionEvidenceExpires(...)` and `e.buildWorkflowCoverageLabels(...)` so completed evidence has the correct End timestamp (mirror the behavior in `AddExecutionCompletionEvidence`).
🤖 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.
Outside diff comments:
In `@internal/api/handler/workflows/control_relationship.go`:
- Around line 226-232: The request's Description field is never applied to the
updates map; modify the UpdateControlRelationship handler to mirror the existing
pattern for RelationshipType and Strength by checking if req.Description != nil
and then set updates["description"] = *req.Description (using the same updates
map and nil-check pattern used for req.RelationshipType and req.Strength in the
function that handles UpdateControlRelationshipRequest).
In `@internal/api/handler/workflows/workflow_definition.go`:
- Around line 175-201: The read of the workflow definition (h.service.GetByID)
happens outside the DB transaction and can be stale; move the GetByID call into
the h.db.Transaction closure so the record is loaded within the same transaction
used by workflows.NewWorkflowDefinitionService(tx).Update and
workflows.NewFilterSyncService(tx).SyncFilterForDefinition; inside the
transaction, use the tx-backed service to fetch the definition, apply req.*
changes to that local definition, then call Update(id, definition) and
SyncFilterForDefinition(*id) before returning to ensure atomic read-modify-write
semantics.
In `@internal/workflow/evidence.go`:
- Around line 244-266: The evidence record is incorrectly initialized with End
set to *execution.StartedAt* causing completed evidence to have zero-length
duration; in the evidence creation block that constructs the
*relational.Evidence* (the struct assigned to variable `evidence`), update the
End field to use *execution.CompletedAt* when `status == "completed"` (i.e., set
`evidence.End = *execution.CompletedAt` inside the `if status == "completed"`
branch), and ensure this happens before you call
`e.calculateCompletionEvidenceExpires(...)` and
`e.buildWorkflowCoverageLabels(...)` so completed evidence has the correct End
timestamp (mirror the behavior in `AddExecutionCompletionEvidence`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34829cf5-9dc4-4ad7-8050-7b71e47d1a95
📒 Files selected for processing (4)
internal/api/handler/workflows/control_relationship.gointernal/api/handler/workflows/workflow_definition.gointernal/workflow/evidence.gointernal/workflow/evidence_test.go
|
No new automated feedback in the past 64 minutes. Waiting for a human review before continuing. |
|
PR approved. Marking as ready for e2e. |
automated implementation by lisa.
Summary by CodeRabbit
New Features
Tests