Skip to content

lisa/feat/workflow-evidence-for-dashboards#411

Merged
gusfcarvalho merged 18 commits into
mainfrom
lisa/feat/workflow-evidence-for-dashboards
Jun 1, 2026
Merged

lisa/feat/workflow-evidence-for-dashboards#411
gusfcarvalho merged 18 commits into
mainfrom
lisa/feat/workflow-evidence-for-dashboards

Conversation

@ccf-lisa
Copy link
Copy Markdown
Contributor

@ccf-lisa ccf-lisa Bot commented Jun 1, 2026

automated implementation by lisa.

Summary by CodeRabbit

  • New Features

    • Profile compliance progress now includes workflow completion evidence and respects evidence expiration; control statuses remain stable when newer executions are pending.
    • Workflow definition operations now synchronize deterministic filters and control memberships (case-insensitive ID matching) atomically.
    • Evidence routing updated: started vs completed executions are recorded differently; completed evidence receives workflow coverage labels and expiration.
  • Tests

    • Added integration and unit tests covering compliance progress, filter sync, evidence labeling/expiration, and workflow-definition deletion cleanup.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds 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.

Changes

Workflow Filter Synchronization and Evidence Tracking

Layer / File(s) Summary
Filter Service Core Implementation
internal/service/relational/workflows/constants.go, internal/service/relational/workflows/filter_service.go
FilterSyncService, workflow evidence label constants, deterministic filter UUID generation, SyncFilterForDefinition, and DeleteFilterForDefinition.
FilterSyncService tests
internal/service/relational/workflows/filter_service_test.go
In-memory DB setup and tests for sync idempotency, case-insensitive control ID matching, and delete/idempotency.
Control Relationship Handler Transactions
internal/api/handler/workflows/control_relationship.go
Create/Update/Delete/Activate/Deactivate now run inside DB transactions and call SyncFilterForDefinition transactionally.
Workflow Definition Handler Transactions
internal/api/handler/workflows/workflow_definition.go
Handler stores DB and wraps Create/Update/Delete with transactions that call SyncFilterForDefinition/DeleteFilterForDefinition.
Evidence Completion Expiration and Labeling
internal/workflow/evidence.go, internal/workflow/evidence_test.go
EvidenceIntegration selects streams by status, appends workflow policy/plugin labels for completed/failed instance-stream evidence, and computes Expires from cadence/cron+grace. Tests validate labels, statuses, deterministic UUIDs, and expiration logic.
Integration & Handler Test Updates
internal/service/relational/workflows/filter_service_test.go, internal/api/handler/workflows/common_test.go, internal/api/handler/oscal/profile_compliance_workflow_integration_test.go, internal/api/handler/workflows/workflow_definition_integration_test.go
DB migrations include relational.Control; new/extended tests cover filter sync, handler transactional flows, evidence labeling/expiration, and profile compliance progress including workflow completion evidence.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • gusfcarvalho

🐰 Filters hum, UUIDs align,

Labels nest where completions shine,
Cadence ticks the expiry chime,
Transactions keep the sync in time,
A rabbit cheers: "Tests pass—fine!"

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The PR title is vague and does not clearly describe the main change; it uses a feature branch name format rather than a descriptive summary of the actual implementation. Revise the title to be more descriptive and specific about the primary change, such as 'Add workflow evidence integration for compliance progress tracking' or similar.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Include description in the partial update map.

UpdateControlRelationshipRequest still exposes description, 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 win

Use completion timestamps for completion evidence (description and End).

For "completed" evidence, the code still uses StartedAt in the description and sets End to StartedAt. This misrepresents event timing and can skew latest-evidence selection that orders by end 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 win

Guard unsupported status values before dereferencing stream.

If status is neither "started" nor "completed", stream stays nil and UUID: stream.UUID panics at runtime. Add a default case 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 win

Fix 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

📥 Commits

Reviewing files that changed from the base of the PR and between b771d5e and 6cee15c.

📒 Files selected for processing (10)
  • internal/api/handler/oscal/profile_compliance_workflow_integration_test.go
  • internal/api/handler/workflows/common_test.go
  • internal/api/handler/workflows/control_relationship.go
  • internal/api/handler/workflows/workflow_definition.go
  • internal/api/handler/workflows/workflow_definition_integration_test.go
  • internal/service/relational/workflows/constants.go
  • internal/service/relational/workflows/filter_service.go
  • internal/service/relational/workflows/filter_service_test.go
  • internal/workflow/evidence.go
  • internal/workflow/evidence_test.go

Comment thread internal/service/relational/workflows/filter_service_test.go Outdated
Comment thread internal/service/relational/workflows/filter_service.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 computed Expires timestamps to workflow completion evidence, plus tests for expiry/cadence behavior.
  • Introduce FilterSyncService and 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.

Comment thread internal/workflow/evidence.go
Comment thread internal/service/relational/workflows/filter_service.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, End is currently set to *execution.StartedAt. The latest-evidence selection logic uses evidences.end desc per stream (see relational.GetLatestEvidenceStreamsQuery), so using StartedAt here can cause completion evidence to never be considered the latest record for the instance stream. It also risks a nil dereference if StartedAt is 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, End is currently set to *execution.StartedAt. The latest-evidence selection logic uses evidences.end desc per stream (see relational.GetLatestEvidenceStreamsQuery), so using StartedAt here can cause completion evidence to never be considered the latest record for the instance stream. It also risks a nil dereference if StartedAt is nil.
	// Create evidence record
	evidence := &relational.Evidence{
		UUID:        stream.UUID,
		Title:       title,
		Description: description,
		Start:       *execution.StartedAt,
		End:         *execution.StartedAt,
		Labels: []relational.Labels{

Comment thread internal/workflow/evidence.go
Comment thread internal/workflow/evidence.go
Comment thread internal/api/handler/workflows/workflow_definition.go
Comment thread internal/api/handler/workflows/control_relationship.go
Comment thread internal/workflow/evidence.go
Comment thread internal/workflow/evidence.go
Comment thread internal/api/handler/workflows/workflow_definition.go
Comment thread internal/api/handler/workflows/control_relationship.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Description field from request is not applied.

UpdateControlRelationshipRequest defines a Description field (line 52), but it's not added to the updates map. Requests with description will 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 value

Read outside transaction may cause stale data on concurrent updates.

GetByID on 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 win

Fix workflow_execution_completed evidence End to use execution.CompletedAt

In internal/workflow/evidence.go (lines 244-266), the evidence record is created with Start: *execution.StartedAt and End: *execution.StartedAt and, in the status == "completed" branch, End is never updated to *execution.CompletedAt. This makes completion evidence a zero-length window and makes Evidence.End/evidence_end reflect StartedAt (unlike AddExecutionCompletionEvidence, which sets End: *execution.CompletedAt). The existing completed-evidence test only checks the description, so it won’t catch this.

🐛 Set End to CompletedAt for completed evidence
 	if 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8e156a7 and e5732eb.

📒 Files selected for processing (4)
  • internal/api/handler/workflows/control_relationship.go
  • internal/api/handler/workflows/workflow_definition.go
  • internal/workflow/evidence.go
  • internal/workflow/evidence_test.go

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comment thread internal/workflow/evidence.go
@ccf-lisa
Copy link
Copy Markdown
Contributor Author

ccf-lisa Bot commented Jun 1, 2026

No new automated feedback in the past 64 minutes. Waiting for a human review before continuing.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread internal/workflow/evidence.go
Comment thread internal/workflow/evidence.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Comment thread internal/workflow/evidence.go
Comment thread internal/workflow/evidence.go
Comment thread internal/workflow/evidence.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread internal/workflow/evidence.go Outdated
Comment thread internal/workflow/evidence.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comment thread internal/workflow/evidence.go Outdated
Comment thread internal/service/relational/workflows/filter_service.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Comment thread internal/workflow/evidence.go Outdated
Comment thread internal/service/relational/workflows/filter_service.go
Comment thread internal/workflow/evidence.go Outdated
Comment thread internal/service/relational/workflows/filter_service.go
Comment thread internal/workflow/evidence.go Outdated
Comment thread internal/service/relational/workflows/filter_service.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comment thread internal/workflow/evidence.go
Comment thread internal/workflow/evidence.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comment thread internal/workflow/evidence.go
Comment thread internal/service/relational/workflows/filter_service.go
@ccf-lisa
Copy link
Copy Markdown
Contributor Author

ccf-lisa Bot commented Jun 1, 2026

PR approved. Marking as ready for e2e.

@gusfcarvalho gusfcarvalho merged commit fda3ff3 into main Jun 1, 2026
5 checks passed
@gusfcarvalho gusfcarvalho deleted the lisa/feat/workflow-evidence-for-dashboards branch June 1, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants