[test] Add tests for middleware.tryApplyToolResponseFilter, rewriteFilteredTextPayload, and rewriteEnvelopeTextPayload#5696
Merged
Conversation
…edTextPayload, and rewriteEnvelopeTextPayload
These three internal functions in internal/middleware/jqschema.go had many
branches that were previously covered only indirectly through WrapToolHandler
integration tests. The new white-box test file directly exercises every branch:
rewriteEnvelopeTextPayload (10 cases):
- non-map data, map without content key
- []map content: empty, non-empty (rewrites first item, immutability)
- []interface{} content: empty, first-item-not-a-map, first-item-is-map (rewrites + immutability)
- unrecognised content type
rewriteFilteredTextPayload (5 cases):
- single content item with successful envelope rewrite
- multiple content items (trailing items preserved)
- envelope rewrite fails, valid JSON filteredText
- envelope rewrite fails, invalid JSON filteredText (original data fallback)
- IsError/Meta propagation
tryApplyToolResponseFilter (13 cases):
- nil filterCode (early return)
- empty Content slice (falls through to data-level filter)
- non-TextContent first item (falls through to data-level filter)
- TextContent with non-JSON text (falls through to data-level filter)
- TextContent filter error (returns original result/data)
- TextContent filter succeeds (rewrites result)
- TextContent trailing items preserved after text-level rewrite
- data-level filter error (returns original result/data)
- ConvertToCallToolResult error (returns original result/data)
- data-level success with trailing items appended
- data-level success single item (no trailing appended)
- IsError propagated for data-level success
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
@copilot fix linting errors |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds direct white-box tests for middleware response filtering and payload rewrite helpers in internal/middleware, improving coverage around jq filter application, envelope rewriting, fallback paths, and content preservation.
Changes:
- Adds tests for
rewriteEnvelopeTextPayloadacross supported and unsupported envelope shapes. - Adds tests for
rewriteFilteredTextPayloadrewrite/fallback behavior. - Adds tests for
tryApplyToolResponseFiltersuccess, fallback, and error paths.
Show a summary per file
| File | Description |
|---|---|
internal/middleware/filter_rewrite_test.go |
New direct unit tests for internal middleware filter/rewrite helpers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 9
Comment on lines
+228
to
+234
| data := []interface{}{} | ||
| result := &sdk.CallToolResult{ | ||
| Content: []sdk.Content{&sdk.TextContent{Text: `"x"`}}, | ||
| IsError: true, | ||
| } | ||
| got, _ := rewriteFilteredTextPayload(result, data, `"y"`) | ||
| assert.True(t, got.IsError) |
Comment on lines
+256
to
+288
| code, err := CompileToolResponseFilter(".") | ||
| require.NoError(t, err) | ||
| result := &sdk.CallToolResult{Content: []sdk.Content{}} | ||
| data := map[string]interface{}{"content": []interface{}{ | ||
| map[string]interface{}{"type": "text", "text": "hello"}, | ||
| }} | ||
|
|
||
| gotResult, _ := tryApplyToolResponseFilter(ctx, code, result, data, "tool", "q3") | ||
| require.NotNil(t, gotResult) | ||
| }) | ||
|
|
||
| t.Run("non-TextContent first item falls through to data-level filter", func(t *testing.T) { | ||
| code, err := CompileToolResponseFilter(".") | ||
| require.NoError(t, err) | ||
| result := makeNonTextResult() | ||
| data := map[string]interface{}{"content": []interface{}{ | ||
| map[string]interface{}{"type": "text", "text": "hello"}, | ||
| }} | ||
|
|
||
| gotResult, _ := tryApplyToolResponseFilter(ctx, code, result, data, "tool", "q4") | ||
| require.NotNil(t, gotResult) | ||
| }) | ||
|
|
||
| t.Run("TextContent with non-JSON text falls through to data-level filter", func(t *testing.T) { | ||
| code, err := CompileToolResponseFilter(".") | ||
| require.NoError(t, err) | ||
| result := makeTextResult("not valid json") | ||
| data := map[string]interface{}{"content": []interface{}{ | ||
| map[string]interface{}{"type": "text", "text": "hello"}, | ||
| }} | ||
|
|
||
| gotResult, _ := tryApplyToolResponseFilter(ctx, code, result, data, "tool", "q5") | ||
| require.NotNil(t, gotResult) |
| // tryApplyToolResponseFilter | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestTryApplyToolResponseFilter(t *testing.T) { |
Comment on lines
+418
to
+431
| result := &sdk.CallToolResult{ | ||
| Content: []sdk.Content{&sdk.ImageContent{Data: "x", MIMEType: "image/png"}}, | ||
| IsError: true, | ||
| } | ||
| data := map[string]interface{}{ | ||
| "content": []interface{}{ | ||
| map[string]interface{}{"type": "text", "text": "hi"}, | ||
| }, | ||
| } | ||
|
|
||
| gotResult, _ := tryApplyToolResponseFilter(ctx, code, result, data, "tool", "q13") | ||
|
|
||
| require.NotNil(t, gotResult) | ||
| assert.True(t, gotResult.IsError) |
Comment on lines
+275
to
+276
| gotResult, _ := tryApplyToolResponseFilter(ctx, code, result, data, "tool", "q4") | ||
| require.NotNil(t, gotResult) |
Comment on lines
+280
to
+288
| code, err := CompileToolResponseFilter(".") | ||
| require.NoError(t, err) | ||
| result := makeTextResult("not valid json") | ||
| data := map[string]interface{}{"content": []interface{}{ | ||
| map[string]interface{}{"type": "text", "text": "hello"}, | ||
| }} | ||
|
|
||
| gotResult, _ := tryApplyToolResponseFilter(ctx, code, result, data, "tool", "q5") | ||
| require.NotNil(t, gotResult) |
Comment on lines
+388
to
+394
| gotResult, _ := tryApplyToolResponseFilter(ctx, code, result, data, "tool", "q11") | ||
|
|
||
| require.NotNil(t, gotResult) | ||
| // trailing item must be appended because result.Content[0] is TextContent | ||
| // and len(result.Content) > 1. | ||
| assert.Greater(t, len(gotResult.Content), 1, "trailing content item must be appended") | ||
| assert.Equal(t, trailing, gotResult.Content[len(gotResult.Content)-1]) |
Comment on lines
+411
to
+412
| // Only one content item in original → no extra appended after filter | ||
| assert.GreaterOrEqual(t, len(gotResult.Content), 1) |
Comment on lines
+335
to
+337
| // The trailing TextContent should be preserved | ||
| require.GreaterOrEqual(t, len(gotResult.Content), 2, | ||
| "trailing content item must be preserved") |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test Coverage Improvement: jqschema filter/rewrite functions
Functions Analyzed
internal/middlewaretryApplyToolResponseFilter,rewriteFilteredTextPayload,rewriteEnvelopeTextPayloadWrapToolHandlerintegration tests)tryApplyToolResponseFilterhas 8+ conditional branches;rewriteEnvelopeTextPayloadhas a 3-level type-switch with 7 branchesWhy These Functions?
These three internal functions in
internal/middleware/jqschema.goimplement the core tool-response filter and payload-rewrite pipeline. Together they have:tryApplyToolResponseFilter(63 lines): 8 distinct code paths — nil filter, TextContent vs. non-TextContent, JSON parse failure, filter error, marshal error, envelope rewrite, ConvertToCallToolResult error, trailing-content preservationrewriteEnvelopeTextPayload(49 lines): 3-level type switch with 7 branches across[]map[string]interface{}and[]interface{}content layoutsrewriteFilteredTextPayload(25 lines): 3 data-return pathsAll were previously reached only through the much heavier
WrapToolHandlerpath, meaning many branches (especially all the error/fallback paths) were not reliably covered.Tests Added
rewriteEnvelopeTextPayloadIsErrorandMetapreservedTest Cases Summary
rewriteEnvelopeTextPayloadrewriteFilteredTextPayloadtryApplyToolResponseFilterGenerated by Test Coverage Improver
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgSee Network Configuration for more information.