fix(composer): expand template expressions in elicitation messages#4313
fix(composer): expand template expressions in elicitation messages#4313saschabuehrle wants to merge 2 commits intostacklok:mainfrom
Conversation
jerm-dro
left a comment
There was a problem hiding this comment.
Thanks for picking this up! Two requests before merging: avoid mutating the step definition (use an immutable assignment pattern), and restructure the test as table-driven with a few more cases. Details in the inline comments.
pkg/vmcp/composer/workflow_engine.go
Outdated
| // Expand template expressions in elicitation message (e.g. {{.params.owner}}) | ||
| if step.Elicitation.Message != "" { | ||
| wrapper := map[string]any{"message": step.Elicitation.Message} | ||
| expanded, expandErr := e.templateExpander.Expand(ctx, wrapper, workflowCtx) | ||
| if expandErr != nil { | ||
| err := fmt.Errorf("%w: failed to expand elicitation message for step %s: %v", | ||
| ErrTemplateExpansion, step.ID, expandErr) | ||
| workflowCtx.RecordStepFailure(step.ID, err) | ||
| return err | ||
| } | ||
| if msg, ok := expanded["message"].(string); ok { | ||
| step.Elicitation.Message = msg | ||
| } | ||
| } | ||
|
|
||
| // Request elicitation (synchronous - blocks until response or timeout) | ||
| // Per MCP 2025-06-18: SDK handles JSON-RPC ID correlation internally | ||
| response, err := e.elicitationHandler.RequestElicitation(ctx, workflowCtx.WorkflowID, step.ID, step.Elicitation) |
There was a problem hiding this comment.
This mutates step.Elicitation.Message in place. Since step is a pointer to the workflow definition, the original template could be lost after expansion — e.g., if workflows are re-executed or steps retried, the template wouldn't be available for re-expansion.
Compare with executeToolStep (line 404) which stores the expanded result in a local expandedArgs variable and never modifies step.Arguments.
Suggest constructing the config immutably via an immediately-invoked function, and passing the result to RequestElicitation:
// Expand template expressions in elicitation message (e.g. {{.params.owner}})
elicitCfg, err := func() (*ElicitationConfig, error) {
if step.Elicitation.Message == "" {
return step.Elicitation, nil
}
wrapper := map[string]any{"message": step.Elicitation.Message}
expanded, expandErr := e.templateExpander.Expand(ctx, wrapper, workflowCtx)
if expandErr != nil {
return nil, fmt.Errorf("%w: failed to expand elicitation message for step %s: %v",
ErrTemplateExpansion, step.ID, expandErr)
}
if msg, ok := expanded["message"].(string); ok {
cfgCopy := *step.Elicitation
cfgCopy.Message = msg
return &cfgCopy, nil
}
return step.Elicitation, nil
}()
if err != nil {
workflowCtx.RecordStepFailure(step.ID, err)
return err
}
response, err := e.elicitationHandler.RequestElicitation(ctx, workflowCtx.WorkflowID, step.ID, elicitCfg)|
Thanks for the review — I’ve gone through the requested changes and queued follow-up updates. I’ll push the revision in a dedicated follow-up pass. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4313 +/- ##
==========================================
- Coverage 68.77% 68.47% -0.30%
==========================================
Files 473 478 +5
Lines 47919 48499 +580
==========================================
+ Hits 32955 33211 +256
- Misses 12299 12409 +110
- Partials 2665 2879 +214 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Pushed an update for both points. The step config stays immutable now (message expansion happens on a copied elicitation config), and the integration coverage is table-driven with both templated and plain message cases. Greetings, saschabuehrle |
Bug
#4312 — Template expressions like
{{.params.owner}}in elicitation stepmessagefields are passed through as raw placeholders instead of being expanded.Fix
Adds template expansion for
step.Elicitation.MessageinexecuteElicitationStep()before passing it toRequestElicitation(). This mirrors howexecuteToolStep()already expandsstep.Argumentsthrough the template expander (line ~404).The expansion wraps the message string in a temporary map to reuse the existing public
TemplateExpander.Expand()interface, keeping the change minimal and consistent with the established expansion pattern.Testing
TestWorkflowEngine_ElicitationMessageTemplateExpansionintegration test that verifies{{.params.repo}}and{{.params.env}}placeholders in an elicitation message are expanded to their resolved values before reaching the SDK elicitation requester.Happy to address any feedback.
Greetings, saschabuehrle