fix(schemas/responses): forward server-tool parameters on ResponsesTool#4543
Conversation
## Summary Fixes a bug where OTEL plugin headers were being overwritten with redacted placeholder values when saving a plugin configuration. After the multi-profile change, header values stored as plain strings inside the `profiles` array were not being restored from the database before saving, causing real credentials to be replaced with masked values like `****`. ## Changes - Extracted `restoreRedactedValue` as a standalone recursive helper, replacing the inline logic in `restoreRedactedFromExisting`. This allows the restoration logic to descend into both nested maps and slices. - Added slice traversal support (index-aligned) so that elements within arrays like the OTEL `profiles` array are individually checked and restored. - Added plain-string redaction detection so that header values stored as raw strings (rather than `EnvVar` objects) are also restored from the existing DB config when they carry a redaction artifact. Empty strings are intentionally left as-is to allow clearing a value. - Added `TestRestoreRedacted_OTELProfilesHeaders` to cover both failure modes: slice traversal and plain-string secret restoration. Also asserts that genuinely new (non-redacted) values pass through unchanged. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./transports/bifrost-http/handlers/... ``` Verify that saving an OTEL plugin configuration with multiple profiles, after a GET that returns redacted header values, does not overwrite the stored credentials in the database. Confirm that providing a genuinely new header value still persists correctly. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations This fix ensures that redacted credential placeholders returned to the client are never written back over real secrets stored in the database. The restoration logic only replaces values that are confirmed redaction artifacts; empty strings and non-redacted values are always passed through as-is, preserving the ability to clear a credential intentionally. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
ResponsesTool modeled typed tool variants but had no generic `parameters`
passthrough, so configuration for provider/server tools (e.g. OpenRouter's
"openrouter:web_search" with {"engine":"exa"}) was silently dropped during
(de)serialization. Add a `Parameters` field carried verbatim through
UnmarshalJSON/MarshalJSON.
Fixes maximhq#4541
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesResponsesTool parameters passthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/schemas/responses_test.go`:
- Around line 182-183: The current assertion using strings.Contains for
`"engine":"exa"` is fragile and can pass even if the parameter is in the wrong
location in the JSON structure. Replace the strings.Contains check with a proper
roundtrip validation: unmarshal the output bytes into an appropriate response
structure, then assert that both parameters.engine and parameters.max_results
are correctly preserved as nested fields within the parameters object. This
ensures the JSON structure is correct, not just that a substring exists
somewhere in the output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9520a1a7-e379-499e-94f7-35c0ca4b709d
📒 Files selected for processing (2)
core/schemas/responses.gocore/schemas/responses_test.go
| if !strings.Contains(string(out), `"engine":"exa"`) { | ||
| t.Fatalf("expected parameters to be preserved on marshal, got %s", string(out)) |
There was a problem hiding this comment.
Strengthen the roundtrip assertion to validate the parameters object, not just a substring.
The current strings.Contains check can false-pass if "engine":"exa" appears in the wrong location. Re-unmarshal out and assert both parameters.engine and parameters.max_results are preserved under parameters.
Proposed test hardening
out, err := tool.MarshalJSON()
if err != nil {
t.Fatalf("marshal: %v", err)
}
- if !strings.Contains(string(out), `"engine":"exa"`) {
- t.Fatalf("expected parameters to be preserved on marshal, got %s", string(out))
- }
+ var roundTripped ResponsesTool
+ if err := Unmarshal(out, &roundTripped); err != nil {
+ t.Fatalf("re-unmarshal: %v", err)
+ }
+ if roundTripped.Parameters == nil ||
+ roundTripped.Parameters["engine"] != "exa" ||
+ roundTripped.Parameters["max_results"] != float64(5) {
+ t.Fatalf("expected parameters roundtrip preservation, got %+v", roundTripped.Parameters)
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/schemas/responses_test.go` around lines 182 - 183, The current assertion
using strings.Contains for `"engine":"exa"` is fragile and can pass even if the
parameter is in the wrong location in the JSON structure. Replace the
strings.Contains check with a proper roundtrip validation: unmarshal the output
bytes into an appropriate response structure, then assert that both
parameters.engine and parameters.max_results are correctly preserved as nested
fields within the parameters object. This ensures the JSON structure is correct,
not just that a substring exists somewhere in the output.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if params, ok := raw["parameters"].(map[string]interface{}); ok { | ||
| t.Parameters = params | ||
| } |
There was a problem hiding this comment.
Parameters populated for function type tools
The raw["parameters"].(map[string]interface{}) type assertion succeeds for function tools too — their "parameters" field is a JSON Schema object and is a valid map[string]interface{}. After this code runs, t.Parameters will be non-nil for every function tool, even though the struct comment says this field is "for provider/server tools that are not modeled as a typed variant." Any future code that checks len(tool.Parameters) > 0 to decide whether to forward server-tool config will treat all function tools as having server parameters. The fix is to guard the assignment so it only fires when no typed variant claimed the type — i.e., wrap it in a check like if t.ResponsesToolFunction == nil after the switch block, or move the assignment into the default: branch of the type switch.
| if !strings.Contains(string(out), `"engine":"exa"`) { | ||
| t.Fatalf("expected parameters to be preserved on marshal, got %s", string(out)) | ||
| } |
There was a problem hiding this comment.
strings.Contains(string(out), "engine":"exa") passes even if "engine" appears at the top level of the marshaled tool rather than nested under "parameters". A stronger assertion such as strings.Contains(string(out), "parameters":{"engine":) would confirm that sjson.SetRawBytes placed the map under the "parameters" key rather than inlining its fields. Worth adding a companion test for a function type tool round-trip as well, to confirm that t.Parameters is not contaminated (and that the function schema marshals correctly when both fields are set).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
6 participants
What
ResponsesToolmodels typed tool variants (function, web_search, etc.) but has no genericparameterspassthrough. As a result, configuration for provider/server tools — e.g. OpenRouter'sopenrouter:web_searchwith{"engine":"exa","max_results":5}— is silently dropped during (de)serialization on/v1/responses.Fixes #4541.
Change
Add a
Parameters map[string]interface{}field toResponsesTool, captured inUnmarshalJSONand emitted (deterministically, viaMarshalSorted) inMarshalJSON, so server-tool parameters are forwarded verbatim.Test
TestResponsesTool_ParametersRoundTrip— unmarshal a tool withparameters, assert it is captured, marshal it back, assert the parameters survive on the wire.Note
Independent of #4530 / #4532 (the
filterUnsupportedToolswhitelist). Both are needed for OpenRouter server tools to fully work on/v1/responses: the type must pass the whitelist and its parameters must be forwarded.