fix: include output schema in tool approval hash migration#527
Merged
Dumbris merged 5 commits intoMay 26, 2026
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extends tool metadata and approval hashing to include MCP tool outputSchema, and adds a storage/runtime migration to avoid falsely flagging previously-approved tools as changed when the hash formula upgrades.
Changes:
- Capture, persist, and index each tool’s
outputSchemaas normalized JSON (OutputSchemaJSON). - Update tool change-detection hashes to include output schema (new hash functions and updated call sites).
- Add storage schema versioning + runtime backfill migration + tests to rebaseline approved tools.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/upstream/core/output_schema.go | Adds helper to capture/normalize tool output schema JSON from upstream MCP tools. |
| internal/upstream/core/client.go | Populates OutputSchemaJSON in tool metadata and includes it in tool hash computation. |
| internal/storage/models.go | Bumps schema version and extends tool approval records with output schema + hash schema version. |
| internal/storage/manager.go | Adds SetSchemaVersion to support migrations writing schema version. |
| internal/storage/bbolt.go | Persists schema version only on new DBs; adds SetSchemaVersion. |
| internal/runtime/tool_quarantine.go | Includes output schema in approval hashing + adds output-schema hash migration/backfill logic. |
| internal/runtime/tool_quarantine_output_schema_test.go | Adds test coverage for the migration/backfill and post-migration behavior. |
| internal/index/bleve.go | Stores and returns output_schema_json from the search index. |
| internal/hash/hash.go | Introduces output-schema-aware tool hash functions and updates existing helpers. |
| internal/config/config.go | Extends ToolMetadata with OutputSchemaJSON. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+237
to
+259
| if existing != nil && existing.Status == storage.ToolApprovalStatusApproved && outputSchemaHashMigration && existing.HashSchemaVersion < storage.OutputSchemaHashSchemaVersion { | ||
| // One-time output-schema hash backfill: previously approved tools did | ||
| // not store outputSchema in the approved contract hash. Rebaseline the | ||
| // approved/current hash using the currently observed output schema so the | ||
| // algorithm upgrade does not masquerade as an upstream rug-pull. | ||
| existing.ApprovedHash = currentHash | ||
| existing.CurrentHash = currentHash | ||
| existing.HashSchemaVersion = storage.OutputSchemaHashSchemaVersion | ||
| existing.CurrentDescription = tool.Description | ||
| existing.CurrentSchema = schemaJSON | ||
| existing.CurrentOutputSchema = outputSchemaJSON | ||
| if saveErr := r.storageManager.SaveToolApproval(existing); saveErr != nil { | ||
| r.logger.Debug("Failed to backfill output schema approval hash", | ||
| zap.String("server", serverName), | ||
| zap.String("tool", toolName), | ||
| zap.Error(saveErr)) | ||
| } else { | ||
| r.logger.Info("Tool approval hash backfilled with output schema", | ||
| zap.String("server", serverName), | ||
| zap.String("tool", toolName)) | ||
| } | ||
| continue | ||
| } |
Comment on lines
+20
to
+33
| @@ -21,8 +29,8 @@ func ToolHash(serverName, toolName, description string, parametersSchema interfa | |||
| } | |||
| } | |||
|
|
|||
| // Combine server name, tool name, description, and schema JSON | |||
| combined := serverName + toolName + description + string(schemaBytes) | |||
| // Combine server name, tool name, description, input schema JSON, and output schema JSON | |||
| combined := serverName + toolName + description + string(schemaBytes) + outputSchemaJSON | |||
Comment on lines
+639
to
+664
| func (r *Runtime) markOutputSchemaHashMigrationCompleteIfReady() { | ||
| if r.storageManager == nil { | ||
| return | ||
| } | ||
|
|
||
| records, err := r.storageManager.ListToolApprovals("") | ||
| if err != nil { | ||
| r.logger.Debug("Failed to list tool approvals for output schema hash migration", | ||
| zap.Error(err)) | ||
| return | ||
| } | ||
|
|
||
| for _, record := range records { | ||
| if record.Status == storage.ToolApprovalStatusApproved && record.HashSchemaVersion < storage.OutputSchemaHashSchemaVersion { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| if err := r.storageManager.SetSchemaVersion(storage.OutputSchemaHashSchemaVersion); err != nil { | ||
| r.logger.Debug("Failed to mark output schema hash migration complete", | ||
| zap.Error(err)) | ||
| return | ||
| } | ||
|
|
||
| r.logger.Info("Output schema hash migration completed") | ||
| } |
Comment on lines
+18
to
+22
| data, err := json.Marshal(tool.OutputSchema) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return normalizeRawJSON(data) |
The output-schema segment was written unconditionally, changing the approval hash for every tool — including those with no outputSchema — which tripped TestCalculateToolApprovalHash_Stability and forced an unnecessary re-baseline of all approved tools on upgrade. Gate the segment on a non-empty normalized output schema so tools without an outputSchema keep their original hash (no re-baseline, no re-quarantine). Tools that do expose an outputSchema still get a new hash, handled by the version-gated migration in checkToolApprovals.
Dumbris
approved these changes
May 26, 2026
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Dumbris
added a commit
that referenced
this pull request
May 26, 2026
…ints (#529) Follow-up to #526 and #527, addressing review concerns. call_tool_* variants and direct mode (#526) duplicated the pending/changed TOOL_QUARANTINED payload and independently derived config-denial — a second source of truth that could drift from isToolCallable/blockedToolMessage. - Extract toolPendingApprovalResult/toolChangedApprovalResult/toolPolicyJSONResult as the single source of truth; call_tool_* and direct mode both use them. - Add MCPProxyServer.isToolConfigDenied as the one config-denial authority (prefers live runtime config, falls back to stored server config); both blockedToolMessage and the direct evaluator route through it. - captureOutputSchemaJSON (#527) returns "" on marshal failure instead of baking an error payload into the contract hash (which would spuriously flip the tool to "changed"). - Consolidate the three JSON normalizers (runtime.normalizeJSON, core.normalizeRawJSON) onto a single exported hash.NormalizeJSON. Net -33 lines. Behavior-preserving: the approval-hash stability canary and all direct/call_tool/quarantine tests pass unchanged; adds drift-guard tests for the shared builders and NormalizeJSON.
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.
Summary
Includes MCP
outputSchemain the tool approval contract hash and adds a versioned migration so existing approved tools do not get falsely re-quarantined on upgrade.Output schema is part of the tool contract because it describes the data shape returned to agents. A silent output-schema change should therefore be treated as a contract change and require human review.
Changes
outputSchemafrom upstream tools.OutputSchemaJSONin tool metadata and Bleve index documents.internal/hashtool hash computationHashSchemaVersionCurrentOutputSchemaPreviousOutputSchema3.SetSchemaVersionsupport to storage.Migration behavior
For databases with schema version
< 3, approved tools are rebaselined using the currently observed output schema.This avoids a false-positive mass re-quarantine caused only by the hash algorithm changing.
Pending and changed approval records are not rebaselined or auto-approved; they still require human action.
One caveat: tools approved before this migration have no historical output-schema baseline, so the migration necessarily blesses the output schema observed at upgrade time. After migration, any real output-schema drift is detected as a changed tool contract.
Validation
Targeted runtime migration tests:
go test ./internal/runtime -run OutputSchema -count=1