Implement structured output for prompt steps (Phase 1)
Add schema validation and retry logic for AI responses:
- Add output_schema, max_retries, plain_text fields to PromptStep
- Add DEFAULT_OUTPUT_SCHEMA with output/reasoning fields
- Implement append_schema_instructions() for prompt augmentation
- Implement validate_schema() using jsonschema library
- Add retry logic with error feedback on validation failure
- Enhance substitute_variables() for nested field access ({var.field})
- Add jsonschema to dependencies
- Update GUI dialog with plain_text checkbox and max_retries spinner
- Add comprehensive tests for structured output and nested access
Tools using plain_text=True bypass validation (current behavior).
Tools without plain_text get structured JSON output with schema enforcement.
Migration Phase 1 complete. Next: Apply plain_text=True to all 72 tools.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit is contained in:
Claude Opus 4.5
+2
-2
@@ -184,7 +184,7 @@ class TestRunCommand:
|
||||
tool = Tool(
|
||||
name="summarize",
|
||||
steps=[
|
||||
PromptStep(prompt="Summarize: {input}", provider="mock", output_var="summary")
|
||||
PromptStep(prompt="Summarize: {input}", provider="mock", output_var="summary", plain_text=True)
|
||||
],
|
||||
output="{summary}"
|
||||
)
|
||||
@@ -223,7 +223,7 @@ class TestTestCommand:
|
||||
tool = Tool(
|
||||
name="test-me",
|
||||
steps=[
|
||||
PromptStep(prompt="Test: {input}", provider="claude", output_var="result")
|
||||
PromptStep(prompt="Test: {input}", provider="claude", output_var="result", plain_text=True)
|
||||
],
|
||||
output="{result}"
|
||||
)
|
||||
|
||||
+255
-9
@@ -108,6 +108,82 @@ Line 3: {var1} again"""
|
||||
assert result == "Enabled: True"
|
||||
|
||||
|
||||
class TestNestedFieldAccess:
|
||||
"""Tests for nested field access in variable substitution."""
|
||||
|
||||
def test_nested_dict_access(self):
|
||||
"""Access nested dict field via dot notation."""
|
||||
result = substitute_variables(
|
||||
"Answer: {result.output}",
|
||||
{"result": {"output": "Hello", "reasoning": "User said hi"}}
|
||||
)
|
||||
assert result == "Answer: Hello"
|
||||
|
||||
def test_nested_json_string_access(self):
|
||||
"""Access field in JSON string via dot notation."""
|
||||
result = substitute_variables(
|
||||
"Answer: {result.output}",
|
||||
{"result": '{"output": "Hello", "reasoning": "User said hi"}'}
|
||||
)
|
||||
assert result == "Answer: Hello"
|
||||
|
||||
def test_nested_deep_access(self):
|
||||
"""Access deeply nested fields."""
|
||||
result = substitute_variables(
|
||||
"Value: {data.nested.deep}",
|
||||
{"data": {"nested": {"deep": "found"}}}
|
||||
)
|
||||
assert result == "Value: found"
|
||||
|
||||
def test_nested_missing_field_unchanged(self):
|
||||
"""Missing nested field leaves placeholder unchanged."""
|
||||
result = substitute_variables(
|
||||
"Value: {result.missing}",
|
||||
{"result": {"output": "Hello"}}
|
||||
)
|
||||
assert result == "Value: {result.missing}"
|
||||
|
||||
def test_nested_on_non_dict_unchanged(self):
|
||||
"""Nested access on non-dict value leaves placeholder unchanged."""
|
||||
result = substitute_variables(
|
||||
"Value: {result.field}",
|
||||
{"result": "just a string"}
|
||||
)
|
||||
assert result == "Value: {result.field}"
|
||||
|
||||
def test_nested_invalid_json_unchanged(self):
|
||||
"""Invalid JSON string leaves placeholder unchanged."""
|
||||
result = substitute_variables(
|
||||
"Value: {result.field}",
|
||||
{"result": "not valid json {"}
|
||||
)
|
||||
assert result == "Value: {result.field}"
|
||||
|
||||
def test_nested_array_in_json(self):
|
||||
"""Nested array values should be JSON-serialized."""
|
||||
result = substitute_variables(
|
||||
"Items: {result.items}",
|
||||
{"result": '{"items": [1, 2, 3]}'}
|
||||
)
|
||||
assert result == "Items: [1, 2, 3]"
|
||||
|
||||
def test_nested_object_in_json(self):
|
||||
"""Nested object values should be JSON-serialized."""
|
||||
result = substitute_variables(
|
||||
"Data: {result.data}",
|
||||
{"result": '{"data": {"key": "value"}}'}
|
||||
)
|
||||
assert result == 'Data: {"key": "value"}'
|
||||
|
||||
def test_settings_access_unchanged(self):
|
||||
"""Settings access should use existing special handling."""
|
||||
result = substitute_variables(
|
||||
"Backend: {settings.backend}",
|
||||
{"settings": {"backend": "piper"}}
|
||||
)
|
||||
assert result == "Backend: piper"
|
||||
|
||||
|
||||
class TestExecutePromptStep:
|
||||
"""Tests for prompt step execution."""
|
||||
|
||||
@@ -118,10 +194,12 @@ class TestExecutePromptStep:
|
||||
success=True
|
||||
)
|
||||
|
||||
# Use plain_text=True to test legacy behavior (no schema enforcement)
|
||||
step = PromptStep(
|
||||
prompt="Summarize: {input}",
|
||||
provider="claude",
|
||||
output_var="summary"
|
||||
output_var="summary",
|
||||
plain_text=True
|
||||
)
|
||||
variables = {"input": "Some text to summarize"}
|
||||
|
||||
@@ -142,7 +220,7 @@ class TestExecutePromptStep:
|
||||
error="Provider error"
|
||||
)
|
||||
|
||||
step = PromptStep(prompt="Test", provider="claude", output_var="out")
|
||||
step = PromptStep(prompt="Test", provider="claude", output_var="out", plain_text=True)
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is False
|
||||
@@ -152,7 +230,7 @@ class TestExecutePromptStep:
|
||||
def test_mock_provider_used(self, mock_mock):
|
||||
mock_mock.return_value = ProviderResult(text="mock response", success=True)
|
||||
|
||||
step = PromptStep(prompt="Test", provider="mock", output_var="out")
|
||||
step = PromptStep(prompt="Test", provider="mock", output_var="out", plain_text=True)
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is True
|
||||
@@ -162,13 +240,180 @@ class TestExecutePromptStep:
|
||||
def test_provider_override(self, mock_call):
|
||||
mock_call.return_value = ProviderResult(text="response", success=True)
|
||||
|
||||
step = PromptStep(prompt="Test", provider="claude", output_var="out")
|
||||
step = PromptStep(prompt="Test", provider="claude", output_var="out", plain_text=True)
|
||||
execute_prompt_step(step, {"input": ""}, provider_override="gpt4")
|
||||
|
||||
# Should use override, not step's provider
|
||||
assert mock_call.call_args[0][0] == "gpt4"
|
||||
|
||||
|
||||
class TestStructuredOutput:
|
||||
"""Tests for structured output enforcement."""
|
||||
|
||||
@patch('cmdforge.runner.call_provider')
|
||||
def test_structured_output_valid_json(self, mock_call):
|
||||
"""Valid JSON matching default schema should succeed."""
|
||||
mock_call.return_value = ProviderResult(
|
||||
text='{"output": "Hello, world!", "reasoning": "Greeting requested"}',
|
||||
success=True
|
||||
)
|
||||
|
||||
step = PromptStep(
|
||||
prompt="Say hello",
|
||||
provider="claude",
|
||||
output_var="result"
|
||||
# plain_text defaults to False - structured output enabled
|
||||
)
|
||||
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is True
|
||||
# Output should be normalized JSON
|
||||
import json
|
||||
parsed = json.loads(output)
|
||||
assert parsed["output"] == "Hello, world!"
|
||||
assert parsed["reasoning"] == "Greeting requested"
|
||||
|
||||
@patch('cmdforge.runner.call_provider')
|
||||
def test_structured_output_strips_markdown_fences(self, mock_call):
|
||||
"""Markdown code fences should be stripped before parsing."""
|
||||
mock_call.return_value = ProviderResult(
|
||||
text='```json\n{"output": "Hello"}\n```',
|
||||
success=True
|
||||
)
|
||||
|
||||
step = PromptStep(
|
||||
prompt="Say hello",
|
||||
provider="claude",
|
||||
output_var="result"
|
||||
)
|
||||
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is True
|
||||
import json
|
||||
parsed = json.loads(output)
|
||||
assert parsed["output"] == "Hello"
|
||||
|
||||
@patch('cmdforge.runner.call_provider')
|
||||
def test_structured_output_retry_on_invalid_json(self, mock_call):
|
||||
"""Invalid JSON should trigger retry with error feedback."""
|
||||
# First call returns invalid JSON, second returns valid
|
||||
mock_call.side_effect = [
|
||||
ProviderResult(text="Not JSON at all", success=True),
|
||||
ProviderResult(text='{"output": "Fixed!"}', success=True),
|
||||
]
|
||||
|
||||
step = PromptStep(
|
||||
prompt="Test",
|
||||
provider="claude",
|
||||
output_var="result",
|
||||
max_retries=1
|
||||
)
|
||||
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is True
|
||||
assert mock_call.call_count == 2
|
||||
# Second call should include error feedback
|
||||
second_call_prompt = mock_call.call_args_list[1][0][1]
|
||||
assert "failed validation" in second_call_prompt.lower()
|
||||
|
||||
@patch('cmdforge.runner.call_provider')
|
||||
def test_structured_output_fails_after_retries(self, mock_call):
|
||||
"""Should fail after exhausting retries."""
|
||||
mock_call.return_value = ProviderResult(
|
||||
text="Still not JSON",
|
||||
success=True
|
||||
)
|
||||
|
||||
step = PromptStep(
|
||||
prompt="Test",
|
||||
provider="claude",
|
||||
output_var="result",
|
||||
max_retries=1 # 1 retry = 2 total attempts
|
||||
)
|
||||
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is False
|
||||
assert mock_call.call_count == 2
|
||||
|
||||
@patch('cmdforge.runner.call_provider')
|
||||
def test_structured_output_custom_schema(self, mock_call):
|
||||
"""Custom schema should be validated."""
|
||||
mock_call.return_value = ProviderResult(
|
||||
text='{"score": 0.8, "reasoning": "Good match"}',
|
||||
success=True
|
||||
)
|
||||
|
||||
custom_schema = {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"score": {"type": "number", "minimum": 0, "maximum": 1},
|
||||
"reasoning": {"type": "string"}
|
||||
},
|
||||
"required": ["score"]
|
||||
}
|
||||
|
||||
step = PromptStep(
|
||||
prompt="Score this",
|
||||
provider="claude",
|
||||
output_var="result",
|
||||
output_schema=custom_schema
|
||||
)
|
||||
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is True
|
||||
import json
|
||||
parsed = json.loads(output)
|
||||
assert parsed["score"] == 0.8
|
||||
|
||||
@patch('cmdforge.runner.call_provider')
|
||||
def test_structured_output_schema_violation_triggers_retry(self, mock_call):
|
||||
"""Schema violation should trigger retry."""
|
||||
# First call missing required 'output' field
|
||||
mock_call.side_effect = [
|
||||
ProviderResult(text='{"reasoning": "thinking..."}', success=True),
|
||||
ProviderResult(text='{"output": "Fixed!", "reasoning": "done"}', success=True),
|
||||
]
|
||||
|
||||
step = PromptStep(
|
||||
prompt="Test",
|
||||
provider="claude",
|
||||
output_var="result",
|
||||
max_retries=1
|
||||
)
|
||||
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is True
|
||||
assert mock_call.call_count == 2
|
||||
|
||||
@patch('cmdforge.runner.call_provider')
|
||||
def test_plain_text_bypasses_validation(self, mock_call):
|
||||
"""plain_text=True should skip all JSON validation."""
|
||||
mock_call.return_value = ProviderResult(
|
||||
text="Just plain text, no JSON here!",
|
||||
success=True
|
||||
)
|
||||
|
||||
step = PromptStep(
|
||||
prompt="Write prose",
|
||||
provider="claude",
|
||||
output_var="result",
|
||||
plain_text=True
|
||||
)
|
||||
|
||||
output, success = execute_prompt_step(step, {"input": ""})
|
||||
|
||||
assert success is True
|
||||
assert output == "Just plain text, no JSON here!"
|
||||
# Should only call provider once (no retries for plain text)
|
||||
assert mock_call.call_count == 1
|
||||
|
||||
|
||||
class TestExecuteCodeStep:
|
||||
"""Tests for code step execution."""
|
||||
|
||||
@@ -387,7 +632,8 @@ class TestRunTool:
|
||||
PromptStep(
|
||||
prompt="Summarize: {input}",
|
||||
provider="claude",
|
||||
output_var="summary"
|
||||
output_var="summary",
|
||||
plain_text=True # Use plain text mode for legacy behavior
|
||||
)
|
||||
],
|
||||
output="{summary}"
|
||||
@@ -423,7 +669,7 @@ class TestRunTool:
|
||||
name="multi-step",
|
||||
steps=[
|
||||
CodeStep(code="preprocessed = input.strip().upper()", output_var="preprocessed"),
|
||||
PromptStep(prompt="Process: {preprocessed}", provider="claude", output_var="response"),
|
||||
PromptStep(prompt="Process: {preprocessed}", provider="claude", output_var="response", plain_text=True),
|
||||
CodeStep(code="final = f'Result: {response}'", output_var="final")
|
||||
],
|
||||
output="{final}"
|
||||
@@ -439,7 +685,7 @@ class TestRunTool:
|
||||
tool = Tool(
|
||||
name="test",
|
||||
steps=[
|
||||
PromptStep(prompt="Test", provider="claude", output_var="response")
|
||||
PromptStep(prompt="Test", provider="claude", output_var="response", plain_text=True)
|
||||
],
|
||||
output="{response}"
|
||||
)
|
||||
@@ -458,7 +704,7 @@ class TestRunTool:
|
||||
tool = Tool(
|
||||
name="test",
|
||||
steps=[
|
||||
PromptStep(prompt="Test", provider="claude", output_var="response")
|
||||
PromptStep(prompt="Test", provider="claude", output_var="response", plain_text=True)
|
||||
],
|
||||
output="{response}"
|
||||
)
|
||||
@@ -475,7 +721,7 @@ class TestRunTool:
|
||||
tool = Tool(
|
||||
name="test",
|
||||
steps=[
|
||||
PromptStep(prompt="Test", provider="claude", output_var="response")
|
||||
PromptStep(prompt="Test", provider="claude", output_var="response", plain_text=True)
|
||||
],
|
||||
output="{response}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user