DRAFT: tool composition integration tests

[jhrozek]

Not finished yet, want to get early feedback

[codecov]

Codecov Report

❌ Patch coverage is 0% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.25%. Comparing base (afe9b41) to head (ecd6480).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
test/integration/vmcp/helpers/backend.go	0.00%	45 Missing ⚠️
test/integration/vmcp/helpers/mcp_client.go	0.00%	11 Missing ⚠️
test/integration/vmcp/helpers/vmcp_server.go	0.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2922      +/-   ##
==========================================
- Coverage   56.36%   56.25%   -0.12%     
==========================================
  Files         323      323              
  Lines       31763    31828      +65     
==========================================
  Hits        17904    17904              
- Misses      12330    12395      +65     
  Partials     1529     1529              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

The step.OnError.Action config allows continue or abort, which implies whether the step is required for success. Similarly the condition implies whether the step is required. IMO if a downstream tool uses an input from a failed/skipped step, it should either:

1. require a default value
2. receive an empty value (instead of "<no value>")
   I don't think either of these is implemented. for 1) we could validate at the webhook, for 2) we could replace the at tool execution time.

I tend to think (1) is a better option, since we it can be validated early and provides more explicit behavior. Plus we don't have to guess what "<no value>" should be for non-string types.

[tgrunnagle]

I agree this would be surprising and we should change the behavior. Options I can think of

1. at composite tool validation time (in webhook), ensure required fields either a) reference values that cannot be missing (i.e. upstream step has OnError.Action = 'abort') or b) have a default value
2. fail at tool execution time if template expansion fails (<no value>) and there is no default value

Here again I think (1) is better since it can be validated early.

[tgrunnagle]

Mentioned in a previous comment, I think a good option is to require a 'default' value for any references to outputs that come from an OnError.Action = 'continue' step (validated in webhook)

[tgrunnagle]

as for handling dependsOn for failed upstream steps using "continue", we could

1. have an implied Condition for the downstream step on the upstream succeeding (as you mentioned), or
2. require a default value for the downstream input that refers to steps that "continue"

I'm wary of an implied Condition (1) because it's less flexible and less explicit behavior. (2) aligns better with what I'm thinking for the other discussion topics in this PR.

[slyt3]

I noticed NewSlowBackendTool it just doesn't respect context cancellation it just use time.Sleep(delay) but it just simply ignores the context parameter.

SO if the context is cancleleed during the some deloya, the sleep continues no matter what. this could cuase some tests hand unnecessarily

Should it be:

Handler: func(ctx context.Context, _ map[string]any) string {
    select {
    case <-time.After(delay):
        return result
    case <-ctx.Done():
        return "" // or return maybe some kind of error
    }
}