refactor: split validateDefinition into focused validators #448

New issue

Open

opened 2026-07-25 23:31:19 +02:00 by dries · 0 comments

dries commented

2026-07-25 23:31:19 +02:00

Owner

Code-quality audit (P2). internal/workflows/service.go:2342-2504 — validateDefinition is 163 lines in one body.

It validates pools, limits, workspace, triggers, per-node type/agent/repeat/resource/lease rules, and runs a Kahn topological sort for cycle detection:

func validateDefinition(definition Definition) error {
	if definition.ID == "" || definition.Name == "" || definition.Version == "" { ... }
	if definition.Concurrency <= 0 { ... }
	...
	if visited != len(nodes) {
		return fmt.Errorf("workflow contains a cycle")   // :2500
	}
	return nil
}

Why it matters

Validation is the workflow API's entire input trust boundary. A 163-line body with mixed concerns makes it hard to see which inputs are unchecked, and hard to add a rule without re-reading everything. The graph walk in particular is independently testable logic buried at the bottom.

Suggested fix

The split precedent already exists — validateTrigger, validateLease, validateSecrets, validateCommandNode are already separate. Extract to match:

validatePools(definition) error — pools, limits, workspace.
validateNodes(definition) error — per-node type/agent/repeat/resource/lease checks.
validateGraph(definition) error — dependency existence + Kahn cycle detection.

Acceptance criteria

validateDefinition becomes a short sequence of validate* calls.
Each extracted validator has direct unit tests including its rejection cases.
No validation rule is lost — existing validation tests pass unchanged.

Effort: M.

Code-quality audit (P2). `internal/workflows/service.go:2342-2504` — `validateDefinition` is 163 lines in one body. It validates pools, limits, workspace, triggers, per-node type/agent/repeat/resource/lease rules, **and** runs a Kahn topological sort for cycle detection: ```go func validateDefinition(definition Definition) error { if definition.ID == "" || definition.Name == "" || definition.Version == "" { ... } if definition.Concurrency <= 0 { ... } ... if visited != len(nodes) { return fmt.Errorf("workflow contains a cycle") // :2500 } return nil } ``` ## Why it matters Validation is the workflow API's entire input trust boundary. A 163-line body with mixed concerns makes it hard to see which inputs are unchecked, and hard to add a rule without re-reading everything. The graph walk in particular is independently testable logic buried at the bottom. ## Suggested fix The split precedent already exists — `validateTrigger`, `validateLease`, `validateSecrets`, `validateCommandNode` are already separate. Extract to match: - `validatePools(definition) error` — pools, limits, workspace. - `validateNodes(definition) error` — per-node type/agent/repeat/resource/lease checks. - `validateGraph(definition) error` — dependency existence + Kahn cycle detection. ## Acceptance criteria - [ ] `validateDefinition` becomes a short sequence of `validate*` calls. - [ ] Each extracted validator has direct unit tests including its rejection cases. - [ ] No validation rule is lost — existing validation tests pass unchanged. Effort: M.