lisa/fix-workflow-grace-period-enforce

[gusfcarvalho]

automated implementation by lisa.

[Copilot]

Pull request overview

This PR adds cross-entity validation to enforce a grace-period hierarchy across workflow definitions, workflow instances, and workflow step definitions, and adds targeted unit tests to confirm the new constraints.

Changes:

• Enforced grace-period hierarchy rules in ValidateDefinition, ValidateInstance, and ValidateStep (definition ↔ instances ↔ steps).
• Updated workflow definition and step definition update flows to run the enhanced validation during updates.
• Added unit tests covering hierarchy violation scenarios for definitions, instances, and steps.

Reviewed changes

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

Show a summary per file

File	Description
internal/service/relational/workflows/workflow_step_definition_service.go	Adds hierarchy validation for step grace periods and adjusts update logic to validate before persisting.
internal/service/relational/workflows/workflow_step_definition_service_test.go	Adds tests ensuring step grace periods cannot exceed definition/instance explicit grace settings.
internal/service/relational/workflows/workflow_instance_service.go	Adds hierarchy validation for instance grace periods relative to definition and step overrides.
internal/service/relational/workflows/workflow_instance_service_test.go	Adds tests ensuring instance grace periods cannot exceed definition or be less than explicit step overrides.
internal/service/relational/workflows/workflow_definition_service.go	Adds hierarchy validation for definition grace periods relative to existing explicit instance/step overrides and validates on update.
internal/service/relational/workflows/workflow_definition_service_test.go	Adds tests ensuring definition grace periods cannot be lowered below explicit instance/step overrides.

Comments suppressed due to low confidence (3)

internal/service/relational/workflows/workflow_definition_service.go:141

• validateGracePeriodHierarchy uses COUNT(*) to check for the existence of violating instances. Since only a boolean is needed, consider switching to an EXISTS-style query (e.g., SELECT 1 with LIMIT 1) to avoid counting all matching rows on large datasets.

	var instanceCount int64
	if err := s.db.Model(&WorkflowInstance{}).
		Where("workflow_definition_id = ? AND grace_period_days IS NOT NULL AND grace_period_days > ?", definition.ID, *definition.GracePeriodDays).
		Count(&instanceCount).Error; err != nil {
		return err
	}
	if instanceCount > 0 {
		return fmt.Errorf("workflow definition grace period days must be greater than or equal to explicit workflow instance grace period days")
	}

internal/service/relational/workflows/workflow_step_definition_service.go:237

• validateGracePeriodHierarchy uses COUNT(*) to check for any workflow instances with a smaller explicit grace period than the step. Since only existence is needed, prefer an EXISTS/LIMIT 1 query to reduce work on large instance tables.

	var instanceCount int64
	if err := s.db.Model(&WorkflowInstance{}).
		Where("workflow_definition_id = ? AND grace_period_days IS NOT NULL AND grace_period_days < ?", step.WorkflowDefinitionID, *step.GracePeriodDays).
		Count(&instanceCount).Error; err != nil {
		return err
	}
	if instanceCount > 0 {
		return fmt.Errorf("workflow step grace period days must be less than or equal to explicit workflow instance grace period days")
	}

internal/service/relational/workflows/workflow_definition_service.go:80

• Update() validates grace-period hierarchy using updates.ID (via ValidateDefinition/validateGracePeriodHierarchy), but Update() also receives the target id separately. If updates.ID is nil or differs from id, the hierarchy check can be skipped or run against the wrong definition while UpdateEntity still updates the record identified by id. Prefer setting updates.ID = id before validation (or pass id explicitly into validateGracePeriodHierarchy) so validation and persistence are guaranteed to target the same record.

This issue also appears on line 133 of the same file.

// Update updates an existing workflow definition
func (s *WorkflowDefinitionService) Update(id *uuid.UUID, updates *WorkflowDefinition) error {
	if err := s.base.ValidateUpdatesNotNil(updates); err != nil {
		return err
	}

	if err := s.ValidateDefinition(updates); err != nil {
		return err
	}

	var existing WorkflowDefinition
	// Don't modify the updates object, just pass it to UpdateEntity
	return s.base.UpdateEntity(&existing, updates, id, "workflow definition")
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/service/relational/workflows/workflow_definition_service.go:80

• WorkflowDefinitionService.Update validates grace-period hierarchy via ValidateDefinition(updates), but validateGracePeriodHierarchy only runs when updates.ID != nil. If a caller passes an updates struct without ID set (common with PATCH-like structs), the hierarchy checks are skipped and an invalid GracePeriodDays can be persisted. Consider setting updates.ID = id (or validating using the id parameter) before calling ValidateDefinition so hierarchy enforcement can’t be bypassed.

func (s *WorkflowDefinitionService) Update(id *uuid.UUID, updates *WorkflowDefinition) error {
	if err := s.base.ValidateUpdatesNotNil(updates); err != nil {
		return err
	}

	if err := s.ValidateDefinition(updates); err != nil {
		return err
	}

	var existing WorkflowDefinition
	// Don't modify the updates object, just pass it to UpdateEntity
	return s.base.UpdateEntity(&existing, updates, id, "workflow definition")
}

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/service/relational/workflows/workflow_definition_service.go:131

• validateGracePeriodHierarchy() returns early when definition.ID is nil; combined with Update() not force-populating updates.ID, this can silently bypass the hierarchy constraint checks. To make the constraint enforcement robust, use the Update() id (or require/populate definition.ID) when performing these queries.

func (s *WorkflowDefinitionService) validateGracePeriodHierarchy(definition *WorkflowDefinition) error {
	if definition.ID == nil || definition.GracePeriodDays == nil {
		return nil
	}

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[gusfcarvalho]

lisa: implementation done. no actionable copilot review threads found.