From 91ee71b8a16009064e46404cc27102dad6ab2e94 Mon Sep 17 00:00:00 2001 From: darbs-claude Date: Mon, 23 Feb 2026 11:48:27 +0000 Subject: [PATCH] fix: improve template variable error messages (#30) Enhance `validateVariables()` in `PlanTemplateService` to produce actionable errors instead of the generic "Required variable '...' is missing". Changes: - Extracted `buildVariableError()` helper that composes the message from the variable's `description`, `format`, `example`, and `examples` fields - Added `naming_convention` key to the returned array so callers have a constant reminder that variable names use snake_case - Added a `NAMING_CONVENTION` private const to avoid string duplication Tests (6 new cases in `PlanTemplateServiceTest`): - description included in error message - single `example` value included - `examples` list (first two) included - `format` hint included alongside example - `naming_convention` present in both valid and invalid results - bare variable (no description) still produces useful "missing" message Closes #30 Co-Authored-By: Claude Sonnet 4.6 --- Services/PlanTemplateService.php | 51 +++++++++- TODO.md | 6 +- tests/Feature/PlanTemplateServiceTest.php | 116 ++++++++++++++++++++++ 3 files changed, 169 insertions(+), 4 deletions(-) diff --git a/Services/PlanTemplateService.php b/Services/PlanTemplateService.php index 1d8fa8f..9117ea0 100644 --- a/Services/PlanTemplateService.php +++ b/Services/PlanTemplateService.php @@ -329,13 +329,18 @@ class PlanTemplateService /** * Validate variables against template requirements. + * + * Returns a result array with: + * - valid: bool + * - errors: string[] – actionable messages including description and examples + * - naming_convention: string – reminder that variable names use snake_case */ public function validateVariables(string $templateSlug, array $variables): array { $template = $this->get($templateSlug); if (! $template) { - return ['valid' => false, 'errors' => ['Template not found']]; + return ['valid' => false, 'errors' => ['Template not found'], 'naming_convention' => self::NAMING_CONVENTION]; } $errors = []; @@ -344,16 +349,58 @@ class PlanTemplateService $required = $varDef['required'] ?? true; if ($required && ! isset($variables[$name]) && ! isset($varDef['default'])) { - $errors[] = "Required variable '{$name}' is missing"; + $errors[] = $this->buildVariableError($name, $varDef); } } return [ 'valid' => empty($errors), 'errors' => $errors, + 'naming_convention' => self::NAMING_CONVENTION, ]; } + /** + * Naming convention reminder included in validation results. + */ + private const NAMING_CONVENTION = 'Variable names use snake_case (e.g. project_name, api_key)'; + + /** + * Build an actionable error message for a missing required variable. + * + * Incorporates the variable's description, example values, and expected + * format so the caller knows exactly what to provide. + */ + private function buildVariableError(string $name, array $varDef): string + { + $message = "Required variable '{$name}' is missing"; + + if (! empty($varDef['description'])) { + $message .= ": {$varDef['description']}"; + } + + $hints = []; + + if (! empty($varDef['format'])) { + $hints[] = "expected format: {$varDef['format']}"; + } + + if (! empty($varDef['example'])) { + $hints[] = "example: '{$varDef['example']}'"; + } elseif (! empty($varDef['examples'])) { + $exampleValues = is_array($varDef['examples']) + ? array_slice($varDef['examples'], 0, 2) + : [$varDef['examples']]; + $hints[] = "examples: '".implode("', '", $exampleValues)."'"; + } + + if (! empty($hints)) { + $message .= ' ('.implode('; ', $hints).')'; + } + + return $message; + } + /** * Get templates by category. */ diff --git a/TODO.md b/TODO.md index 6118fae..5df8747 100644 --- a/TODO.md +++ b/TODO.md @@ -126,9 +126,11 @@ Production-quality task list for the AI agent orchestration package. - Issue: Empty API key creates provider that fails on first use - Fix: `Log::warning()` emitted for each provider registered without an API key; message names the env var to set -- [ ] **DX-003: Plan template variable errors not actionable** +- [x] **DX-003: Plan template variable errors not actionable** (FIXED 2026-02-23) - Location: `Services/PlanTemplateService.php::validateVariables()` - - Fix: Include expected format, examples in error messages + - Fix: Error messages now include variable description, example/examples, and expected format + - Added `naming_convention` field to result; extracted `buildVariableError()` helper + - New tests: description in error, example value, multiple examples, format hint, naming_convention field ### Code Quality diff --git a/tests/Feature/PlanTemplateServiceTest.php b/tests/Feature/PlanTemplateServiceTest.php index b5416c8..da062fc 100644 --- a/tests/Feature/PlanTemplateServiceTest.php +++ b/tests/Feature/PlanTemplateServiceTest.php @@ -630,6 +630,122 @@ describe('variable validation', function () { ->and($result['errors'])->toHaveCount(1) ->and($result['errors'][0])->toContain('var2'); }); + + it('includes description in error message', function () { + createTestTemplate('desc-in-error', [ + 'name' => 'Test', + 'variables' => [ + 'project_name' => [ + 'required' => true, + 'description' => 'The name of the project being planned', + ], + ], + 'phases' => [], + ]); + + $result = $this->service->validateVariables('desc-in-error', []); + + expect($result['errors'][0]) + ->toContain('project_name') + ->toContain('The name of the project being planned'); + }); + + it('includes example value in error message', function () { + createTestTemplate('example-in-error', [ + 'name' => 'Test', + 'variables' => [ + 'api_key' => [ + 'required' => true, + 'description' => 'Your API key', + 'example' => 'sk-abc123', + ], + ], + 'phases' => [], + ]); + + $result = $this->service->validateVariables('example-in-error', []); + + expect($result['errors'][0]) + ->toContain('api_key') + ->toContain('sk-abc123'); + }); + + it('includes multiple examples in error message', function () { + createTestTemplate('examples-list-in-error', [ + 'name' => 'Test', + 'variables' => [ + 'environment' => [ + 'required' => true, + 'description' => 'Deployment environment', + 'examples' => ['production', 'staging', 'development'], + ], + ], + 'phases' => [], + ]); + + $result = $this->service->validateVariables('examples-list-in-error', []); + + expect($result['errors'][0]) + ->toContain('environment') + ->toContain('production') + ->toContain('staging'); + }); + + it('includes expected format in error message', function () { + createTestTemplate('format-in-error', [ + 'name' => 'Test', + 'variables' => [ + 'release_date' => [ + 'required' => true, + 'description' => 'Date of the planned release', + 'format' => 'YYYY-MM-DD', + 'example' => '2026-03-01', + ], + ], + 'phases' => [], + ]); + + $result = $this->service->validateVariables('format-in-error', []); + + expect($result['errors'][0]) + ->toContain('release_date') + ->toContain('YYYY-MM-DD') + ->toContain('2026-03-01'); + }); + + it('returns naming convention in all results', function () { + createTestTemplate('naming-convention', [ + 'name' => 'Test', + 'variables' => [ + 'my_var' => ['required' => true], + ], + 'phases' => [], + ]); + + $invalid = $this->service->validateVariables('naming-convention', []); + $valid = $this->service->validateVariables('naming-convention', ['my_var' => 'value']); + + expect($invalid)->toHaveKey('naming_convention') + ->and($invalid['naming_convention'])->toContain('snake_case') + ->and($valid)->toHaveKey('naming_convention') + ->and($valid['naming_convention'])->toContain('snake_case'); + }); + + it('error message without description is still actionable', function () { + createTestTemplate('no-desc-error', [ + 'name' => 'Test', + 'variables' => [ + 'bare_var' => ['required' => true], + ], + 'phases' => [], + ]); + + $result = $this->service->validateVariables('no-desc-error', []); + + expect($result['errors'][0]) + ->toContain('bare_var') + ->toContain('missing'); + }); }); // =========================================================================