refactor(middleware): define PayloadMetadata as struct with constant …#771
refactor(middleware): define PayloadMetadata as struct with constant …#771
Conversation
…for instructions
- Add PayloadMetadata struct for type-safe payload metadata responses
- Extract PayloadTruncatedInstructions as a constant
- Update applyJqSchema to return interface{} instead of JSON string
- Update tests with helper functions for struct-to-map conversion
There was a problem hiding this comment.
Pull request overview
Refactors the jqschema middleware’s large-payload metadata response to use a typed PayloadMetadata struct (and a shared instructions constant), and updates tests to accommodate applyJqSchema now returning a structured schema object instead of a JSON string.
Changes:
- Introduces
PayloadMetadatastruct andPayloadTruncatedInstructionsconstant; rewrites metadata construction to use the struct. - Changes
applyJqSchemato returninterface{}(schema object) rather than a marshaled JSON string. - Updates unit/integration tests with helpers to convert
PayloadMetadataintomap[string]interface{}for existing assertions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/middleware/jqschema.go | Switches schema output to an object and returns typed metadata via PayloadMetadata + constant instructions. |
| internal/middleware/jqschema_test.go | Adjusts schema assertions for non-string schema output; adds helper to adapt struct metadata to prior map-based assertions. |
| internal/middleware/jqschema_integration_test.go | Mirrors unit-test adjustments for integration coverage and adds similar struct->map helper. |
| .github/workflows/large-payload-tester-README.md | Removes the workflow README documentation file. |
| .github/agentics/large-payload-tester.md | Updates agent instructions to mention filesystem path behavior for oversized MCP payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Truncated: truncated, | ||
| Instructions: PayloadTruncatedInstructions, | ||
| } |
There was a problem hiding this comment.
PayloadTruncatedInstructions says the payload was “too large”, but Instructions is set unconditionally even when Truncated is false. This makes the metadata response misleading for small payloads. Either only set Instructions when truncated is true, or adjust the message/field name so it’s accurate for both truncated and non-truncated cases.
| // Calculate schema size for logging (marshal temporarily) | ||
| schemaBytes, _ := json.Marshal(schemaObj) | ||
| logger.LogDebug("payload", "Schema transformation completed: tool=%s, queryID=%s, schemaSize=%d bytes", | ||
| toolName, queryID, len(schemaJSON)) | ||
| toolName, queryID, len(schemaBytes)) |
There was a problem hiding this comment.
The error from json.Marshal(schemaObj) is ignored. If marshaling ever fails, this will log a schemaSize of 0 without any signal. Please handle the error (at least log it) or avoid marshaling just to compute size.
See below for a potential fix:
schemaBytes, err := json.Marshal(schemaObj)
if err != nil {
logMiddleware.Printf("Failed to marshal schema for size calculation: tool=%s, queryID=%s, sessionID=%s, error=%v", toolName, queryID, sessionID, err)
logger.LogWarn("payload", "Failed to marshal schema for size calculation: tool=%s, queryID=%s, error=%v",
toolName, queryID, err)
} else {
logger.LogDebug("payload", "Schema transformation completed: tool=%s, queryID=%s, schemaSize=%d bytes",
toolName, queryID, len(schemaBytes))
}
| // payloadMetadataToMap converts PayloadMetadata to map[string]interface{} for test assertions | ||
| // This allows tests to remain unchanged while working with the new struct type | ||
| func payloadMetadataToMap(t *testing.T, data interface{}) map[string]interface{} { | ||
| t.Helper() | ||
| pm, ok := data.(PayloadMetadata) | ||
| if !ok { | ||
| t.Fatalf("expected PayloadMetadata, got %T", data) | ||
| } | ||
| jsonBytes, err := json.Marshal(pm) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
The helper converts PayloadMetadata -> JSON -> map[string]interface{}. This loses numeric types (e.g., OriginalSize becomes float64) and forces callers to add casts, which is easy to miss. Since the production type is now a struct, it’s more robust for tests to assert on PayloadMetadata fields directly (or return the struct from this helper).
| // integrationPayloadMetadataToMap converts PayloadMetadata to map[string]interface{} for test assertions | ||
| func integrationPayloadMetadataToMap(t *testing.T, data interface{}) map[string]interface{} { | ||
| t.Helper() | ||
| pm, ok := data.(PayloadMetadata) | ||
| if !ok { | ||
| t.Fatalf("expected PayloadMetadata, got %T", data) | ||
| } | ||
| jsonBytes, err := json.Marshal(pm) | ||
| require.NoError(t, err) | ||
| var result map[string]interface{} | ||
| err = json.Unmarshal(jsonBytes, &result) | ||
| require.NoError(t, err) | ||
| return result |
There was a problem hiding this comment.
This helper duplicates payloadMetadataToMap from jqschema_test.go with only a name change. Consider moving a single shared helper into a common *_test.go file in this package and using it from both unit and integration tests to reduce duplication and keep assertions consistent.
| // integrationPayloadMetadataToMap converts PayloadMetadata to map[string]interface{} for test assertions | |
| func integrationPayloadMetadataToMap(t *testing.T, data interface{}) map[string]interface{} { | |
| t.Helper() | |
| pm, ok := data.(PayloadMetadata) | |
| if !ok { | |
| t.Fatalf("expected PayloadMetadata, got %T", data) | |
| } | |
| jsonBytes, err := json.Marshal(pm) | |
| require.NoError(t, err) | |
| var result map[string]interface{} | |
| err = json.Unmarshal(jsonBytes, &result) | |
| require.NoError(t, err) | |
| return result | |
| // Ensure encoding/json import is used even though this file delegates | |
| // metadata-to-map conversion to a shared test helper. | |
| var _ = json.Marshal | |
| // integrationPayloadMetadataToMap converts PayloadMetadata to map[string]interface{} for test assertions | |
| func integrationPayloadMetadataToMap(t *testing.T, data interface{}) map[string]interface{} { | |
| t.Helper() | |
| return payloadMetadataToMap(t, data) |
| QueryID: queryID, | ||
| PayloadPath: filePath, | ||
| Preview: preview, |
There was a problem hiding this comment.
PayloadPath is populated from filePath even when savePayload fails (in that case filePath is ""). Since the client only receives the rewritten metadata response, this can drop the actual tool payload and return an unusable/empty path. Consider short-circuiting: if filePath is empty (or saveErr != nil), return the original result/data instead of transforming the response, or include an explicit error and avoid rewriting.
…for instructions