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 <noreply@anthropic.com>
This commit is contained in:
parent
5fa46104f4
commit
91ee71b8a1
3 changed files with 169 additions and 4 deletions
|
|
@ -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.
|
||||
*/
|
||||
|
|
|
|||
6
TODO.md
6
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
|
||||
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
// =========================================================================
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue