Skip to content

Commit 4781354

Browse files
tgrunnagletgrunnagle
committed
Address PR feedback
1 parent 36bdd32 commit 4781354

File tree

3 files changed

+260
-16
lines changed

3 files changed

+260
-16
lines changed

‎pkg/vmcp/composer/output_constructor.go‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,16 @@ import (
1111
"github.com/stacklok/toolhive/pkg/vmcp/config"
1212
)
1313

14+
const (
15+
// Type constants for output properties
16+
typeString = "string"
17+
typeInteger = "integer"
18+
typeNumber = "number"
19+
typeBoolean = "boolean"
20+
typeObject = "object"
21+
typeArray = "array"
22+
)
23+
1424
// constructOutputFromConfig builds the workflow output from the output configuration.
1525
// This expands templates in the Value fields, deserializes JSON for object types,
1626
// applies default values on expansion failure, and validates the final output.
@@ -34,12 +44,16 @@ func (e *workflowEngine) constructOutputFromConfig(
3444
output[propertyName] = value
3545
}
3646

37-
// Validate required fields are present
47+
// Validate required fields are present and non-nil
3848
if len(outputConfig.Required) > 0 {
3949
for _, requiredField := range outputConfig.Required {
40-
if _, exists := output[requiredField]; !exists {
50+
value, exists := output[requiredField]
51+
if !exists {
4152
return nil, fmt.Errorf("required output field %q is missing", requiredField)
4253
}
54+
if value == nil {
55+
return nil, fmt.Errorf("required output field %q is nil", requiredField)
56+
}
4357
}
4458
}
4559

@@ -103,7 +117,7 @@ func (e *workflowEngine) constructOutputPropertyFromValue(
103117
}
104118

105119
// For object types, attempt JSON deserialization
106-
if propertyDef.Type == "object" { //nolint:goconst // Type literals are clearer than constants here
120+
if propertyDef.Type == typeObject {
107121
var obj map[string]any
108122
if err := json.Unmarshal([]byte(expandedStr), &obj); err != nil {
109123
// JSON deserialization failed - try default value
@@ -117,7 +131,7 @@ func (e *workflowEngine) constructOutputPropertyFromValue(
117131
}
118132

119133
// For array types, attempt JSON deserialization
120-
if propertyDef.Type == "array" {
134+
if propertyDef.Type == typeArray {
121135
var arr []any
122136
if err := json.Unmarshal([]byte(expandedStr), &arr); err != nil {
123137
// JSON deserialization failed - try default value
@@ -173,26 +187,26 @@ func (e *workflowEngine) constructOutputPropertyFromProperties(
173187
// coerceStringToType converts a string value to the specified type.
174188
func (*workflowEngine) coerceStringToType(value string, targetType string) (any, error) {
175189
switch targetType {
176-
case "string":
190+
case typeString:
177191
return value, nil
178192

179-
case "integer":
193+
case typeInteger:
180194
// Try to parse as integer
181195
intVal, err := strconv.ParseInt(value, 10, 64)
182196
if err != nil {
183197
return nil, fmt.Errorf("cannot coerce %q to integer: %w", value, err)
184198
}
185199
return intVal, nil
186200

187-
case "number":
201+
case typeNumber:
188202
// Try to parse as float
189203
floatVal, err := strconv.ParseFloat(value, 64)
190204
if err != nil {
191205
return nil, fmt.Errorf("cannot coerce %q to number: %w", value, err)
192206
}
193207
return floatVal, nil
194208

195-
case "boolean":
209+
case typeBoolean:
196210
// Try to parse as boolean
197211
switch value {
198212
case "true", "True", "TRUE", "1": //nolint:goconst // Boolean literals are clearer than constants
@@ -220,14 +234,14 @@ func (*workflowEngine) coerceDefaultValue(defaultVal any, targetType string) (an
220234

221235
// If default is already the correct type, return as-is
222236
switch targetType {
223-
case "string":
237+
case typeString:
224238
if str, ok := defaultVal.(string); ok {
225239
return str, nil
226240
}
227241
// Convert other types to string
228242
return fmt.Sprintf("%v", defaultVal), nil
229243

230-
case "integer":
244+
case typeInteger:
231245
// Handle various integer representations
232246
switch v := defaultVal.(type) {
233247
case int:
@@ -237,16 +251,26 @@ func (*workflowEngine) coerceDefaultValue(defaultVal any, targetType string) (an
237251
case int64:
238252
return v, nil
239253
case float64:
240-
return int64(v), nil
254+
// Check for potential truncation
255+
intVal := int64(v)
256+
if float64(intVal) != v {
257+
logger.Warnf("Potential precision loss converting float64 %v to int64 %d", v, intVal)
258+
}
259+
return intVal, nil
241260
case float32:
242-
return int64(v), nil
261+
// Check for potential truncation
262+
intVal := int64(v)
263+
if float32(intVal) != v {
264+
logger.Warnf("Potential precision loss converting float32 %v to int64 %d", v, intVal)
265+
}
266+
return intVal, nil
243267
case string:
244268
return strconv.ParseInt(v, 10, 64)
245269
default:
246270
return nil, fmt.Errorf("cannot coerce default value %v (type %T) to integer", defaultVal, defaultVal)
247271
}
248272

249-
case "number":
273+
case typeNumber:
250274
// Handle various number representations
251275
switch v := defaultVal.(type) {
252276
case float64:
@@ -265,7 +289,7 @@ func (*workflowEngine) coerceDefaultValue(defaultVal any, targetType string) (an
265289
return nil, fmt.Errorf("cannot coerce default value %v (type %T) to number", defaultVal, defaultVal)
266290
}
267291

268-
case "boolean":
292+
case typeBoolean:
269293
switch v := defaultVal.(type) {
270294
case bool:
271295
return v, nil
@@ -285,7 +309,7 @@ func (*workflowEngine) coerceDefaultValue(defaultVal any, targetType string) (an
285309
return nil, fmt.Errorf("cannot coerce default value %v (type %T) to boolean", defaultVal, defaultVal)
286310
}
287311

288-
case "object":
312+
case typeObject:
289313
// For objects, accept maps or JSON strings
290314
if objMap, ok := defaultVal.(map[string]any); ok {
291315
return objMap, nil
@@ -299,7 +323,7 @@ func (*workflowEngine) coerceDefaultValue(defaultVal any, targetType string) (an
299323
}
300324
return nil, fmt.Errorf("cannot coerce default value %v (type %T) to object", defaultVal, defaultVal)
301325

302-
case "array":
326+
case typeArray:
303327
// For arrays, accept slices or JSON strings
304328
if arr, ok := defaultVal.([]any); ok {
305329
return arr, nil

‎pkg/vmcp/composer/output_constructor_test.go‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,148 @@ func TestConstructOutputFromConfig(t *testing.T) {
246246
wantErr: true,
247247
errMsg: "failed to deserialize JSON",
248248
},
249+
{
250+
name: "empty string value from template",
251+
outputCfg: &config.OutputConfig{
252+
Properties: map[string]config.OutputProperty{
253+
"message": {
254+
Type: "string",
255+
Description: "Empty message",
256+
Value: "{{.steps.step1.output.empty}}",
257+
},
258+
},
259+
},
260+
workflowCtx: &WorkflowContext{
261+
Steps: map[string]*StepResult{
262+
"step1": {
263+
Status: StepStatusCompleted,
264+
Output: map[string]any{
265+
"empty": "",
266+
},
267+
},
268+
},
269+
},
270+
want: map[string]any{
271+
"message": "",
272+
},
273+
wantErr: false,
274+
},
275+
{
276+
name: "missing field with no value placeholder and no default",
277+
outputCfg: &config.OutputConfig{
278+
Properties: map[string]config.OutputProperty{
279+
"result": {
280+
Type: "string",
281+
Description: "Result",
282+
Value: "{{.steps.step1.output.nonexistent}}",
283+
},
284+
},
285+
},
286+
workflowCtx: &WorkflowContext{
287+
Steps: map[string]*StepResult{
288+
"step1": {
289+
Status: StepStatusCompleted,
290+
Output: map[string]any{
291+
"data": "value",
292+
},
293+
},
294+
},
295+
},
296+
// Without default, <no value> is returned as-is
297+
want: map[string]any{
298+
"result": "<no value>",
299+
},
300+
wantErr: false,
301+
},
302+
{
303+
name: "missing field with no value placeholder and default",
304+
outputCfg: &config.OutputConfig{
305+
Properties: map[string]config.OutputProperty{
306+
"result": {
307+
Type: "string",
308+
Description: "Result",
309+
Value: "{{.steps.step1.output.nonexistent}}",
310+
Default: "default_value",
311+
},
312+
},
313+
},
314+
workflowCtx: &WorkflowContext{
315+
Steps: map[string]*StepResult{
316+
"step1": {
317+
Status: StepStatusCompleted,
318+
Output: map[string]any{
319+
"data": "value",
320+
},
321+
},
322+
},
323+
},
324+
// With default, the default value should be used instead of <no value>
325+
want: map[string]any{
326+
"result": "default_value",
327+
},
328+
wantErr: false,
329+
},
330+
{
331+
name: "integer field with no value placeholder and default",
332+
outputCfg: &config.OutputConfig{
333+
Properties: map[string]config.OutputProperty{
334+
"count": {
335+
Type: "integer",
336+
Description: "Count",
337+
Value: "{{.steps.step1.output.missing_count}}",
338+
Default: 42,
339+
},
340+
},
341+
},
342+
workflowCtx: &WorkflowContext{
343+
Steps: map[string]*StepResult{
344+
"step1": {
345+
Status: StepStatusCompleted,
346+
Output: map[string]any{
347+
"other": "value",
348+
},
349+
},
350+
},
351+
},
352+
want: map[string]any{
353+
"count": int64(42),
354+
},
355+
wantErr: false,
356+
},
357+
{
358+
name: "empty string is different from no value",
359+
outputCfg: &config.OutputConfig{
360+
Properties: map[string]config.OutputProperty{
361+
"value1": {
362+
Type: "string",
363+
Description: "Empty string from backend",
364+
Value: "{{.steps.step1.output.empty}}",
365+
Default: "should_not_be_used",
366+
},
367+
"value2": {
368+
Type: "string",
369+
Description: "Missing field",
370+
Value: "{{.steps.step1.output.missing}}",
371+
Default: "should_be_used",
372+
},
373+
},
374+
},
375+
workflowCtx: &WorkflowContext{
376+
Steps: map[string]*StepResult{
377+
"step1": {
378+
Status: StepStatusCompleted,
379+
Output: map[string]any{
380+
"empty": "", // Explicit empty string
381+
},
382+
},
383+
},
384+
},
385+
want: map[string]any{
386+
"value1": "", // Empty string preserved
387+
"value2": "should_be_used", // Default used for missing field
388+
},
389+
wantErr: false,
390+
},
249391
}
250392

251393
for _, tt := range tests {

‎pkg/vmcp/composer/output_validator.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,13 @@ func validateOutputProperty(name string, prop config.OutputProperty, depth int)
147147
}
148148
}
149149

150+
// Validate default value type matches declared type
151+
if prop.Default != nil {
152+
if err := validateDefaultValueType(prop.Default, prop.Type, name); err != nil {
153+
return err
154+
}
155+
}
156+
150157
return nil
151158
}
152159

@@ -182,3 +189,74 @@ func validateTemplateSyntax(tmpl string) error {
182189

183190
return nil
184191
}
192+
193+
// validateDefaultValueType validates that a default value is compatible with the declared type.
194+
// This performs basic type checking to catch configuration errors early.
195+
func validateDefaultValueType(defaultVal any, targetType string, propertyName string) error {
196+
switch targetType {
197+
case "string":
198+
// Strings accept any type (will be converted via fmt.Sprintf)
199+
return nil
200+
201+
case "integer":
202+
// Accept integer types and numeric types that can be converted
203+
switch defaultVal.(type) {
204+
case int, int32, int64, float32, float64, string:
205+
return nil
206+
default:
207+
return NewValidationError("output.properties.default",
208+
fmt.Sprintf("property %q has default value of type %T, expected integer-compatible type", propertyName, defaultVal),
209+
nil)
210+
}
211+
212+
case "number":
213+
// Accept numeric types
214+
switch defaultVal.(type) {
215+
case float32, float64, int, int32, int64, string:
216+
return nil
217+
default:
218+
return NewValidationError("output.properties.default",
219+
fmt.Sprintf("property %q has default value of type %T, expected number-compatible type", propertyName, defaultVal),
220+
nil)
221+
}
222+
223+
case "boolean":
224+
// Accept boolean types and convertible types
225+
switch defaultVal.(type) {
226+
case bool, int, int32, int64, string:
227+
return nil
228+
default:
229+
return NewValidationError("output.properties.default",
230+
fmt.Sprintf("property %q has default value of type %T, expected boolean-compatible type", propertyName, defaultVal),
231+
nil)
232+
}
233+
234+
case "object":
235+
// Accept map or string (JSON)
236+
switch defaultVal.(type) {
237+
case map[string]any, string:
238+
return nil
239+
default:
240+
return NewValidationError("output.properties.default",
241+
fmt.Sprintf("property %q has default value of type %T, expected object or JSON string",
242+
propertyName, defaultVal),
243+
nil)
244+
}
245+
246+
case "array":
247+
// Accept slice or string (JSON)
248+
switch defaultVal.(type) {
249+
case []any, string:
250+
return nil
251+
default:
252+
return NewValidationError("output.properties.default",
253+
fmt.Sprintf("property %q has default value of type %T, expected array ([]any) or JSON string", propertyName, defaultVal),
254+
nil)
255+
}
256+
257+
default:
258+
return NewValidationError("output.properties.type",
259+
fmt.Sprintf("property %q has unsupported type %q", propertyName, targetType),
260+
nil)
261+
}
262+
}

0 commit comments

Comments
 (0)