fix: sanitize error details on log update path, set MCP raw-storage flag, remove redundant error serialization

[akshaydeo]

Summary

Two related logging correctness bugs are fixed: standalone MCP tool executions did not set the raw-storage context flag before calling PostMCPHook, so logging consumers received an uninitialized value; and the updateLogEntry path assigned ErrorDetails directly to ErrorDetailsParsed without sanitizing it first, allowing raw request/response payloads to reach the store even when content logging was disabled.

Changes

• Added ensureMCPRawStorageContext which computes and sets BifrostContextKeyShouldStoreRawInLogs on the context before standalone MCP tool executions (ExecuteChatMCPTool / ExecuteResponsesMCPTool). In-pipeline tool calls already carry this key from the LLM request path, so an existing value is never overwritten. Because there is no provider config on the standalone path, only the per-request override is honored.
• Removed applyErrorDetailsToEntry and applyErrorDetailsToMCPEntry. Their immediate-serialization behavior was based on a now-incorrect assumption that ErrorDetails (the string field) takes precedence; logstore.SerializeFields actually serializes ErrorDetailsParsed and overwrites ErrorDetails on every write path. Callers now assign sanitizeErrorForLogging(...) directly to ErrorDetailsParsed and let SerializeFields handle serialization.
• updateLogEntry now passes ErrorDetails through sanitizeErrorForLogging before assigning it to ErrorDetailsParsed, closing the update-path leak.
• applyStreamingOutputToEntry follows the same pattern for streaming error entries.
• Status re-derivation in the streaming Path B now checks only ErrorDetailsParsed != nil instead of also checking the string field, which was redundant after the above change.
• Tests updated to reflect that ErrorDetails is populated by SerializeFields rather than immediately, and a new end-to-end test (TestUpdateLogEntrySanitizesErrorDetails) verifies the update-path sanitization against a real store.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

go test ./core/... ./plugins/logging/...

The new TestUpdateLogEntrySanitizesErrorDetails test inserts an initial log entry, applies an update carrying raw payloads with content logging disabled, and asserts that the stored error_details column contains neither RAW_REQUEST_MARKER nor RAW_RESPONSE_MARKER while still containing the error message.

Breaking changes

• Yes
• No

Security considerations

Raw request and response payloads were leaking into the log store on two paths (standalone MCP executions and the updateLogEntry path) when disable_content_logging was set. Both leaks are now closed. No new secrets, auth surfaces, or PII handling is introduced.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: da72f3b7-67fa-4beb-8ea0-161e68d837a8

📥 Commits

Reviewing files that changed from the base of the PR and between 30e57eb and 77ab8c9.

📒 Files selected for processing (7)

• core/bifrost.go
• core/changelog.md
• plugins/logging/changelog.md
• plugins/logging/main.go
• plugins/logging/operations.go
• plugins/logging/operations_test.go
• plugins/logging/sanitize_test.go

📝 Walkthrough

Walkthrough

Adds an internal helper in core/bifrost.go to explicitly set the raw-storage-in-logs context flag for standalone MCP tool executions before invoking MCP tools. Reworks plugins/logging to replace helper-based error-detail marshaling with sanitizeErrorForLogging assignments to ErrorDetailsParsed, relying on SerializeFields for serialization, updating hooks, update paths, and tests accordingly.

Changes

MCP raw-storage flag and error sanitization

Layer / File(s)	Summary
Standalone MCP tool raw-storage context flag core/bifrost.go, core/changelog.md	Adds ensureMCPRawStorageContext and calls it from ExecuteChatMCPTool/ExecuteResponsesMCPTool to set an explicit ShouldStoreRawInLogs context value before tool execution.
Error sanitization invariant in logging hooks plugins/logging/main.go	Replaces removed helper-based ErrorDetails/ErrorDetailsParsed population across PostLLMHook and PostMCPHook branches with direct sanitizeErrorForLogging assignments to ErrorDetailsParsed, and simplifies the error-presence check.
Update and streaming error-detail sanitization plugins/logging/operations.go	Updates updateLogEntry and applyStreamingOutputToEntry to derive shouldStoreRaw from context and set ErrorDetailsParsed via sanitizeErrorForLogging.
Sanitization test coverage and changelog plugins/logging/operations_test.go, plugins/logging/sanitize_test.go, plugins/logging/changelog.md	Reworks and renames tests to validate sanitization/serialization ordering, adds errorWithRawPayloads helper and a new end-to-end update-path regression test, and updates the changelog.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• maximhq/bifrost#3389: Both PRs modify the MCP tool execution to PostMCPHook logging path, touching related context flags and hook gating.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR does not address #123 Files API Support and instead changes logging behavior, so it fails the linked issue objective.	Implement the Files API support requested in #123, including file upload endpoints and provider integration for fine-tuning and RAG workflows.
Out of Scope Changes check	⚠️ Warning	All substantive code changes are logging fixes and MCP context handling, which are unrelated to the linked Files API Support objective.	Remove or separate the logging-only changes, and keep this PR focused on the Files API support scope from #123.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title matches the main changes: log sanitization, MCP raw-storage context handling, and removal of redundant serialization.
Description check	✅ Passed	The description follows the template well and includes summary, changes, testing, impact, security, and checklist details.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/error-details-sanitize-followup

---

_{Comment @coderabbitai help to get the list of available commands.}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[akshaydeo]

• fix: sanitize error details on log update path, set MCP raw-storage flag, remove redundant error serialization #4913 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

plugins/logging/main.go (1)

1613-1621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fabricated nil-response error bypasses sanitizeErrorForLogging.

The doc comment at Line 115 states every ErrorDetailsParsed assignment (Log and MCPToolLog alike) must go through sanitizeErrorForLogging, but this branch constructs the error directly. It's harmless today (no ExtraFields populated), but it's an easy trap for a future edit that adds raw payload fields here without sanitization.

♻️ Suggested fix for consistency with the documented invariant

 	} else {
 		entry.Status = "error"
-		entry.ErrorDetailsParsed = &schemas.BifrostError{
+		entry.ErrorDetailsParsed = sanitizeErrorForLogging(&schemas.BifrostError{
 			IsBifrostError: true,
 			Error: &schemas.ErrorField{
 				Message: "MCP tool execution returned nil response",
 			},
-		}
+		}, p.contentLoggingEnabled(ctx), false)
 	}

🤖 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 `@plugins/logging/main.go` around lines 1613 - 1621, The nil-response error
branch in the logging path is constructing entry.ErrorDetailsParsed directly
instead of using the required sanitizeErrorForLogging flow. Update the MCP tool
logging logic that sets entry.Status and entry.ErrorDetailsParsed to route this
fabricated “MCP tool execution returned nil response” case through
sanitizeErrorForLogging, matching the invariant documented for
ErrorDetailsParsed assignments and keeping the behavior consistent with other
Log and MCPToolLog error handling.

🤖 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/bifrost.go`:
- Around line 2779-2800: ensureMCPRawStorageContext is mutating the shared
bifrost.ctx via BifrostContextKeyShouldStoreRawInLogs, which can leak the
raw-storage decision across unrelated standalone MCP calls. Update
ExecuteChatMCPTool/ExecuteResponsesMCPTool to use a request-scoped context copy
or compute the effective raw-storage value locally before PostMCPHook handling,
and keep ensureMCPRawStorageContext from writing onto the reused instance
context when ctx is nil or shared.

---

Nitpick comments:
In `@plugins/logging/main.go`:
- Around line 1613-1621: The nil-response error branch in the logging path is
constructing entry.ErrorDetailsParsed directly instead of using the required
sanitizeErrorForLogging flow. Update the MCP tool logging logic that sets
entry.Status and entry.ErrorDetailsParsed to route this fabricated “MCP tool
execution returned nil response” case through sanitizeErrorForLogging, matching
the invariant documented for ErrorDetailsParsed assignments and keeping the
behavior consistent with other Log and MCPToolLog error handling.

🪄 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: 1226fb79-7b9b-4874-8e35-a5c9184f8077

📥 Commits

Reviewing files that changed from the base of the PR and between 717ce67 and 30e57eb.

📒 Files selected for processing (7)

• core/bifrost.go
• core/changelog.md
• plugins/logging/changelog.md
• plugins/logging/main.go
• plugins/logging/operations.go
• plugins/logging/operations_test.go
• plugins/logging/sanitize_test.go

[greptile-apps]

Confidence Score: 5/5

Safe to merge — the two bug fixes are well-scoped, the logic is conservative (defaults to stripping raw payloads when flags are absent), and a new end-to-end regression test covers the previously untested update path.

All three changed code paths (standalone MCP, LLM PostLLMHook, and updateLogEntry) now consistently route error details through sanitizeErrorForLogging before writing to the store, closing the content-logging bypass. The removal of the immediate-serialization helpers is safe because SerializeFields runs synchronously before the batch writer commits. No regressions were identified in the test coverage.

No files require special attention; the aliasing concern in sanitizeErrorForLogging (returning the original pointer when both content logging and raw storage are enabled) was flagged in the prior review and is a latent hazard that does not affect correctness today.

Important Files Changed

Filename	Overview
core/bifrost.go	Adds ensureMCPRawStorageContext to stamp BifrostContextKeyShouldStoreRawInLogs on standalone MCP paths; correctly guards against mutating the shared bifrost.ctx by skipping when ctx is nil.
plugins/logging/main.go	Removes the now-redundant applyErrorDetailsToEntry / applyErrorDetailsToMCPEntry helpers and replaces four call sites with direct sanitizeErrorForLogging assignment; the streaming Path B status re-derivation is tightened to check only ErrorDetailsParsed != nil.
plugins/logging/operations.go	updateLogEntry now reads shouldStoreRaw from context and passes ErrorDetails through sanitizeErrorForLogging before assigning to ErrorDetailsParsed, closing the update-path raw-payload leak; applyStreamingOutputToEntry follows the same pattern.
plugins/logging/operations_test.go	Test updated to call SerializeFields explicitly before asserting ErrorDetails is non-empty, correctly reflecting the deferred-serialization model.
plugins/logging/sanitize_test.go	Tests renamed to reflect the new direct-sanitize pattern; new TestUpdateLogEntrySanitizesErrorDetails provides an end-to-end update-path regression test against a real store.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[BifrostError arrives at PostLLMHook / PostMCPHook / updateLogEntry] --> B{Which path?}

    B -->|Standalone MCP ExecuteChatMCPTool / ExecuteResponsesMCPTool| C[ensureMCPRawStorageContext\nstamps BifrostContextKeyShouldStoreRawInLogs\non request-scoped ctx]
    C --> D[PostMCPHook reads shouldStoreRaw from ctx]

    B -->|LLM pipeline requestWorker| E[requestWorker already stamps\nBifrostContextKeyShouldStoreRawInLogs]
    E --> F[PostLLMHook reads shouldStoreRaw from ctx]

    B -->|updateLogEntry path| G[Read shouldStoreRaw\nfrom context.Context]

    D --> H[sanitizeErrorForLogging\ncontentLoggingEnabled + shouldStoreRaw]
    F --> H
    G --> H

    H -->|contentLoggingEnabled AND shouldStoreRaw| I[return original err pointer\nraw payloads preserved]
    H -->|otherwise| J[shallow clone err\nclear RawRequest + RawResponse]

    I --> K[entry.ErrorDetailsParsed = sanitized err]
    J --> K

    K --> L[logstore.SerializeFields\nserializes ErrorDetailsParsed into ErrorDetails\non every write path]
    L --> M[Store: error_details column\nno raw payload leak]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[BifrostError arrives at PostLLMHook / PostMCPHook / updateLogEntry] --> B{Which path?}

    B -->|Standalone MCP ExecuteChatMCPTool / ExecuteResponsesMCPTool| C[ensureMCPRawStorageContext\nstamps BifrostContextKeyShouldStoreRawInLogs\non request-scoped ctx]
    C --> D[PostMCPHook reads shouldStoreRaw from ctx]

    B -->|LLM pipeline requestWorker| E[requestWorker already stamps\nBifrostContextKeyShouldStoreRawInLogs]
    E --> F[PostLLMHook reads shouldStoreRaw from ctx]

    B -->|updateLogEntry path| G[Read shouldStoreRaw\nfrom context.Context]

    D --> H[sanitizeErrorForLogging\ncontentLoggingEnabled + shouldStoreRaw]
    F --> H
    G --> H

    H -->|contentLoggingEnabled AND shouldStoreRaw| I[return original err pointer\nraw payloads preserved]
    H -->|otherwise| J[shallow clone err\nclear RawRequest + RawResponse]

    I --> K[entry.ErrorDetailsParsed = sanitized err]
    J --> K

    K --> L[logstore.SerializeFields\nserializes ErrorDetailsParsed into ErrorDetails\non every write path]
    L --> M[Store: error_details column\nno raw payload leak]

Loading

_{Reviews (3): Last reviewed commit: "Merge branch 'dev' into fix/error-detail..." | Re-trigger Greptile}

[akshaydeo]

Merge activity

• Jul 4, 9:12 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jul 4, 9:13 PM UTC: @akshaydeo merged this pull request with Graphite.