feat: add log_pre_transform_request_data config to capture original client HTTP body before Bifrost transformations - #3964
Conversation
📝 WalkthroughWalkthroughThis PR adds a new feature to capture and log the original HTTP client request body prior to any Bifrost transformations. The change includes a context key, client configuration flag, database schema extensions with migrations, early body capture in the HTTP transport layer, logging plugin integration, configuration schema updates, test interface conformance, and UI controls for configuration and log viewing. ChangesLog Original Client Request Body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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" 🔧 Trivy (0.69.3)Trivy execution failed: 2026-06-02T07:34:40Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: fs filter error: fs filter error: walk error range error: stat .coderabbit-opengrep-fallback.yml: no such file or directory: range error: stat .coderabbit-opengrep-fallback.yml: no such file or directory Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
changed the title
log_pre_transform_request_data config to capture original client HTTP body before Bifrost transformations
30c135f to
f67aaef
Compare
60035e6 to
84840df
Compare
Confidence Score: 5/5Safe to merge; the new feature is fully opt-in, disabled by default, and the data path through context capture, logging plugin, migrations, and UI is complete and consistent with existing patterns. The body-capture copy is correctly placed before handler processing, the pointer-based live config reload follows the established pattern for DisableContentLogging and LoggingHeaders, the migrations are idempotent with rollback support, and the nullable column addition is a non-blocking DDL operation in PostgreSQL. No correctness, concurrency, or data integrity issues were found in the changed paths. No files require special attention; the minor comment-convention gap in core/schemas/bifrost.go and the missing inline comment in handlers/config.go are cosmetic only. Important Files Changed
Reviews (3): Last reviewed commit: "feat: add support for logging pre-transf..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if store != nil && store.ShouldLogPreTransformRequestData() { | ||
| if body := ctx.Request.Body(); len(body) > 0 { | ||
| bodyCopy := make([]byte, len(body)) | ||
| copy(bodyCopy, body) | ||
| bifrostCtx.SetValue(schemas.BifrostContextKeyOriginalClientBody, bodyCopy) | ||
| } | ||
| } |
There was a problem hiding this comment.
log_pre_transform_request_data in config.schema.json
Per the config-schema-source-of-truth rule, transports/config.schema.json is the canonical schema for all config fields. The new log_pre_transform_request_data field is wired through ClientConfig, the database tables, the UI, and this transport handler, but it was never added to config.schema.json. Operators using the file-based config (rather than the UI) have no schema entry to validate against, autocomplete against, or read documentation from. Add a log_pre_transform_request_data boolean property under the client object in transports/config.schema.json.
Rule Used: transports/config.schema.json is the source of tru... (source)
| // overwrites it. | ||
| func migrationAddAdditionalAttributesToPricing(ctx context.Context, db *gorm.DB) error { | ||
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | ||
| func migrationAddAdditionalAttributesToPricing(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ |
There was a problem hiding this comment.
migrationAddAdditionalAttributesToPricing
The newline between the function signature and its opening body was accidentally removed, placing the first statement on the same line as the {. This will be caught and reformatted by gofmt, failing any CI lint step that checks formatting. This appears to be an unintended side-effect of editing the file to add the new migration below.
| func migrationAddAdditionalAttributesToPricing(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | |
| func migrationAddAdditionalAttributesToPricing(ctx context.Context, db *gorm.DB) error { | |
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ |
Confidence Score: 3/5Not safe to merge as-is: the config schema file is out of sync with the new field, and a formatting corruption was introduced into an existing migration function that will fail gofmt checks. The core feature logic is sound — the fasthttp buffer copy, live-pointer pattern, and migration rollbacks are all correct. However,
Important Files Changed
|
| // overwrites it. | ||
| func migrationAddAdditionalAttributesToPricing(ctx context.Context, db *gorm.DB) error { | ||
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | ||
| func migrationAddAdditionalAttributesToPricing(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ |
There was a problem hiding this comment.
migrationAddAdditionalAttributesToPricing
The PR accidentally merged the closing { of the function signature and the m := migrator.New(...) statement onto a single line (separated by a tab), removing the newline that was there before. While syntactically valid Go, this fails gofmt and will break any CI step that runs gofmt -d or golangci-lint. This line needs to be split back to its original two-line form.
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!
84840df to
4444687
Compare
f67aaef to
d26c778
Compare
4444687 to
bbdef0d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@framework/configstore/clientconfig.go`:
- Around line 147-151: Add a migration step after
migrationAddLogPreTransformRequestDataColumn that recomputes and backfills the
config_hash for every existing config_client row so the new
LogPreTransformRequestData:false default is included without triggering
unnecessary reconciliation; implement this by selecting all client configs,
calling GenerateClientConfigHash(...) (or the same hashing logic used by
GenerateClientConfigHash) to produce the new hash, and updating the config_hash
column for each row in the migration; ensure the migration references
migrationAddLogPreTransformRequestDataColumn, uses the same struct/fields as
loadClientConfig/GenerateClientConfigHash to compute the hash, and include
idempotency so running it twice is safe.
In `@framework/configstore/migrations.go`:
- Around line 9009-9035: Add a follow-up DML migration named
migrationRefreshConfigHashAfterLogPreTransformRequestDataColumn that mirrors the
file's existing split DDL/DML pattern: create a migrator.New entry with ID
"refresh_config_hash_after_log_pre_transform_request_data_column", leave the
Migrate step to load existing tables.TableClientConfig rows (in batches to avoid
deadlocks), for each row rebuild the domain ClientConfig including the new
LogPreTransformRequestData field and recompute/persist config_hash only for rows
that currently have a non-empty config_hash, and implement a safe Rollback
(no-op or reverse if applicable); ensure the migration uses ctx via
tx.WithContext and follows the same batching/transaction strategy used by other
refresh migrations in this file to prevent locks on large tables.
In `@helm-charts/bifrost/values.yaml`:
- Line 418: Add the client-level default for the log_pre_transform_request_data
flag to keep config consistent with the schema: in the YAML client block (near
existing fields like enableLogging, disableContentLogging, dropExcessRequests,
initialPoolSize) add log_pre_transform_request_data: false so the client-level
setting mirrors plugins[logging].config.log_pre_transform_request_data and
ensures transports/config.schema.json expectations are met.
In `@plugins/logging/main.go`:
- Around line 710-715: The code unconditionally converts raw request bytes from
ctx.Value(schemas.BifrostContextKeyOriginalClientBody) into a Go string and
stores it in initialData.OriginalClientBody, which will corrupt non-UTF-8 or
binary payloads (multipart, audio, images). Change the logic in the block
guarded by p.logPreTransformRequestData to detect whether the []byte is valid
UTF-8 (or a text content type) before converting to string; if it is not valid
text, either skip logging the body or encode the bytes safely (e.g., base64) and
mark it as binary. Ensure you update the handling around
p.logPreTransformRequestData,
ctx.Value(schemas.BifrostContextKeyOriginalClientBody) and
initialData.OriginalClientBody accordingly so only safe text is stored in the
TEXT column (or binary-safe encoding is used).
- Around line 710-715: The code currently sets initialData.OriginalClientBody
whenever p.logPreTransformRequestData is true, which bypasses the privacy gate;
add an explicit guard that content logging is enabled (e.g. check the plugin's
disable_content_logging flag such as p.disableContentLogging or
p.config.DisableContentLogging) before reading
schemas.BifrostContextKeyOriginalClientBody and assigning
initialData.OriginalClientBody so the body is never captured when content
logging is disabled; keep the existing p.logPreTransformRequestData check but
require the disable flag to be false before converting the []byte to string and
assigning it.
In `@plugins/logging/writer.go`:
- Around line 445-446: The batch-size estimator isn't counting the pre-transform
request body passed through by buildCompleteLogEntryFromPending
(OriginalClientBody), so estimateLogEntrySize should be updated to include the
size of that field; locate the function estimateLogEntrySize and add logic to
account for entry.OriginalClientBody (and any nil/empty checks) when computing
byte size so maxBatchBytes remains a true cap and large OriginalClientBody
values force flushes as intended.
In `@transports/bifrost-http/lib/ctx_test.go`:
- Line 33: Add a positive-path unit test in
transports/bifrost-http/lib/ctx_test.go that uses a testHandlerStore where
ShouldLogPreTransformRequestData() returns true (override the current hardcoded
false) and calls ConvertToBifrostContext with a request that has a readable
body; assert that the stored pre-transform body in the resulting BifrostContext
is a deep copy that remains unchanged even after mutating or reusing the
original request body (e.g., read/close/replace the original Body, then re-read
or modify it). Follow the existing table-driven style: add a deterministic case
that sets up the request body bytes, expected stored bytes, and performs the
mutation/reuse steps, then verify equality between expected and the context's
stored pre-transform bytes to exercise the new branch.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 641-650: The body-copy block in ConvertToBifrostContext currently
reallocates on every invocation; guard it by checking whether the original body
has already been stored (e.g., via
bifrostCtx.Value(schemas.BifrostContextKeyOriginalClientBody)) before
allocating/copying and calling bifrostCtx.SetValue, and only perform the copy
when store.ShouldLogPreTransformRequestData() is true AND the stored value is
nil; update the logic around bifrostCtx.SetValue and the
store.ShouldLogPreTransformRequestData() check to avoid duplicate copies for the
same request.
In `@ui/app/workspace/config/views/loggingView.tsx`:
- Around line 153-158: The Switch for "log-pre-transform-request-data" in
loggingView.tsx is missing a data-testid required for E2E tests; update the
Switch component (id="log-pre-transform-request-data") to include a data-testid
attribute (e.g., data-testid="log-pre-transform-request-data") so the element
can be targeted by tests, ensuring the change is applied alongside the existing
checked and onCheckedChange props that call localConfig and handleConfigChange.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 33b60b0c-b040-4b65-bbcc-6b1bf3543955
📒 Files selected for processing (24)
core/schemas/bifrost.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/logstore/migrations.goframework/logstore/tables.gohelm-charts/bifrost/values.yamlplugins/logging/main.goplugins/logging/operations.goplugins/logging/writer.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/webrtc_realtime_test.gotransports/bifrost-http/handlers/wsresponses_test.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsonui/app/workspace/config/views/loggingView.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/lib/types/config.tsui/lib/types/logs.ts
| if c.LogPreTransformRequestData { | ||
| hash.Write([]byte("logPreTransformRequestData:true")) | ||
| } else { | ||
| hash.Write([]byte("logPreTransformRequestData:false")) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== References to the new field and config hash ===\n'
rg -n -C3 'log_pre_transform_request_data|logPreTransformRequestData|config_hash|GenerateClientConfigHash' framework/configstore
printf '\n=== Migration context ===\n'
fd 'migrations.go' framework/configstore -x sed -n '/log_pre_transform_request_data/,+120p' {}
printf '\n=== Client config reconciliation/update paths ===\n'
rg -n -C4 'GetClientConfig|UpdateClientConfig|GenerateClientConfigHash' framework/configstoreRepository: maximhq/bifrost
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== clientconfig.go around requested lines ==="
nl -ba framework/configstore/clientconfig.go | sed -n '120,190p'
echo
echo "=== Find GenerateClientConfigHash implementation ==="
rg -n --hidden --no-mmap 'GenerateClientConfigHash' framework/configstore
echo
echo "=== Show GenerateClientConfigHash definition + input fields ==="
# Print a reasonably small window around the definition(s)
rg -n 'func .*GenerateClientConfigHash' framework/configstore -S --no-mmap | head -n 20 | cut -d: -f1 | uniq
# Also show more context around any matches
rg -n -C3 'GenerateClientConfigHash' framework/configstore/tables framework/configstore | head -n 80
echo
echo "=== migrationAddAdditionalConfigHashColumns: ensure it backfills client configs incl. LogPreTransformRequestData ==="
# Print the whole client-config pre-populate block (should be <~200 lines)
rg -n 'migrationAddAdditionalConfigHashColumns' framework/configstore/migrations.go
# Show narrower slice around where the client config hash is generated
nl -ba framework/configstore/migrations.go | sed -n '2785,2865p'
echo
echo "=== Check client config table model field name ==="
rg -n 'LogPreTransformRequestData|log_pre_transform_request_data' framework/configstore/tables/clientconfig.go
nl -ba framework/configstore/tables/clientconfig.go | sed -n '1,120p'Repository: maximhq/bifrost
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== clientconfig.go around requested lines ==="
cat -n framework/configstore/clientconfig.go | sed -n '120,190p'
echo
echo "=== Find GenerateClientConfigHash implementation ==="
rg -n --no-mmap 'func .*GenerateClientConfigHash' framework/configstore
echo
rg -n --no-mmap 'GenerateClientConfigHash' framework/configstore/tables framework/configstore
echo
echo "=== migrationAddAdditionalConfigHashColumns: client config pre-populate block ==="
# Locate the specific section by searching for GenerateClientConfigHash usage near client config migration
rg -n --no-mmap 'migrationAddAdditionalConfigHashColumns|Add config_hash to client config|GenerateClientConfigHash\(\)|TableClientConfig' framework/configstore/migrations.go
echo
echo "=== Show the client-config hash backfill block (narrow slice) ==="
rg -n --no-mmap -C2 'Add config_hash to client config|Pre-populate hashes for existing client configs|GenerateClientConfigHash' framework/configstore/migrations.go | head -n 80Repository: maximhq/bifrost
Length of output: 20625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== migrations.go: where log_pre_transform_request_data column is added ==="
cat -n framework/configstore/migrations.go | sed -n '8960,9060p'
echo
echo "=== migrations.go: all occurrences of log_pre_transform_request_data / LogPreTransformRequestData ==="
rg -n 'log_pre_transform_request_data|LogPreTransformRequestData' framework/configstore/migrations.go
echo
echo "=== Search for client config reconciliation paths ==="
rg -n 'func (.*)GetClientConfig|func (.*)UpdateClientConfig|GetClientConfig|UpdateClientConfig' framework/configstore
echo
echo "=== framework/configstore/rdb.go: where config_hash is updated for client configs (context around earlier hit) ==="
cat -n framework/configstore/rdb.go | sed -n '1840,1925p'Repository: maximhq/bifrost
Length of output: 10674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for any migration that recomputes client config config_hash after adding log_pre_transform_request_data ==="
rg -n 'log_pre_transform_request_data|LogPreTransformRequestData' framework/configstore/migrations.go
echo
echo "=== Search where ClientConfig.ConfigHash is used in reconciliation/sync logic ==="
rg -n 'ConfigHash' framework/configstore | head -n 200
echo
echo "=== Find call sites of GetClientConfig / UpdateClientConfig ==="
rg -n 'GetClientConfig\(|UpdateClientConfig\(' . | head -n 200
echo
echo "=== Inspect framework/configstore/store.go interface types (ClientConfig fields) ==="
cat -n framework/configstore/store.go | sed -n '1,220p'
echo
echo "=== Inspect GetClientConfig and UpdateClientConfig implementations (hash-related parts) ==="
cat -n framework/configstore/rdb.go | sed -n '200,320p'
cat -n framework/configstore/rdb.go | sed -n '440,560p'Repository: maximhq/bifrost
Length of output: 44884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find GenerateClientConfigHash() usage outside framework ==="
rg -n 'GenerateClientConfigHash' .
echo
echo
echo "=== Inspect transports/bifrost-http/lib/config.go for config.json sync/hash compare ==="
rg -n -C3 'config_hash|ConfigHash|GenerateClientConfigHash|reconciliation|sync' transports/bifrost-http/lib/config.go
echo
echo "=== Inspect likely sync functions around those matches ==="
# print a few windows around key regions (bounded)
for ln in $(rg -n 'GenerateClientConfigHash|ConfigHash' transports/bifrost-http/lib/config.go | cut -d: -f1 | head -n 20); do
start=$((ln-40)); end=$((ln+80));
echo "--- window ${start}-${end} around line $ln ---"
sed -n "${start},${end}p" transports/bifrost-http/lib/config.go
doneRepository: maximhq/bifrost
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== transports/bifrost-http/handlers/config.go: UpdateClientConfig call context ==="
rg -n 'UpdateClientConfig\(ctx, updatedConfig\)|UpdateClientConfig\(ctx, config\.ClientConfig\)' transports/bifrost-http/handlers/config.go
# show around first match
line=$(rg -n 'UpdateClientConfig\(ctx, updatedConfig\)' transports/bifrost-http/handlers/config.go | cut -d: -f1 | head -n 1)
start=$((line-40)); end=$((line+80))
sed -n "${start},${end}p" transports/bifrost-http/handlers/config.go
echo
echo "=== transports/bifrost-http/handlers/config.go: other UpdateClientConfig usages ==="
rg -n 'UpdateClientConfig\(' transports/bifrost-http/handlers/config.go
echo
echo "=== transports/bifrost-http/lib/config.go: UpdateClientConfig invocations around lines ~905/940/953/1434 ==="
for l in 905 939 953 1434; do
# best-effort show windows by searching nearest "UpdateClientConfig" near those lines
:
done
rg -n 'UpdateClientConfig\(' transports/bifrost-http/lib/config.go
# print windows around each occurrence but capped
rg -n 'UpdateClientConfig\(' transports/bifrost-http/lib/config.go | head -n 10 | cut -d: -f1 | while read -r ln; do
start=$((ln-35)); end=$((ln+85));
echo "--- window ${start}-${end} around line ${ln} ---"
sed -n "${start},${end}p" transports/bifrost-http/lib/config.go
doneRepository: maximhq/bifrost
Length of output: 28517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== handlers/config.go: inspect client config update handler around UpdateClientConfig call ==="
# show larger window around the call site
line=$(rg -n 'UpdateClientConfig\(ctx, updatedConfig\)' transports/bifrost-http/handlers/config.go | cut -d: -f1 | head -n 1)
start=$((line-220)); end=$((line+60))
sed -n "${start},${end}p" transports/bifrost-http/handlers/config.go
echo
echo "=== handlers/config.go: search for ConfigHash handling in client config update ==="
rg -n 'ClientConfig.*ConfigHash|ConfigHash.*ClientConfig|preserv|Preserve.*ConfigHash|ConfigHash\s*=' transports/bifrost-http/handlers/config.goRepository: maximhq/bifrost
Length of output: 14855
Backfill config_hash for existing config_client rows when logPreTransformRequestData is added (default-false included in hash).
if c.LogPreTransformRequestData {
hash.Write([]byte("logPreTransformRequestData:true"))
} else {
hash.Write([]byte("logPreTransformRequestData:false"))
}GenerateClientConfigHash() now includes logPreTransformRequestData in the hash input, but the migration that adds the DB column (migrationAddLogPreTransformRequestDataColumn in framework/configstore/migrations.go) only adds the column and does not recompute config_hash for existing rows. On startup, loadClientConfig compares clientConfig.ConfigHash to fileHash, and any mismatch triggers “file takes precedence” sync via ConfigStore.UpdateClientConfig (which deletes/recreates the client config row), even when the effective client settings are unchanged aside from this new default-false field. Add a migration step to recompute/backfill config_hash for all existing config_client rows after introducing this field (and consider a test for the startup reconciliation behavior).
🤖 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 `@framework/configstore/clientconfig.go` around lines 147 - 151, Add a
migration step after migrationAddLogPreTransformRequestDataColumn that
recomputes and backfills the config_hash for every existing config_client row so
the new LogPreTransformRequestData:false default is included without triggering
unnecessary reconciliation; implement this by selecting all client configs,
calling GenerateClientConfigHash(...) (or the same hashing logic used by
GenerateClientConfigHash) to produce the new hash, and updating the config_hash
column for each row in the migration; ensure the migration references
migrationAddLogPreTransformRequestDataColumn, uses the same struct/fields as
loadClientConfig/GenerateClientConfigHash to compute the hash, and include
idempotency so running it twice is safe.
| func migrationAddLogPreTransformRequestDataColumn(ctx context.Context, db *gorm.DB) error { | ||
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | ||
| ID: "add_log_pre_transform_request_data_column", | ||
| Migrate: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| if !tx.Migrator().HasColumn(&tables.TableClientConfig{}, "log_pre_transform_request_data") { | ||
| if err := tx.Migrator().AddColumn(&tables.TableClientConfig{}, "LogPreTransformRequestData"); err != nil { | ||
| return fmt.Errorf("failed to add log_pre_transform_request_data column: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| Rollback: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| if tx.Migrator().HasColumn(&tables.TableClientConfig{}, "log_pre_transform_request_data") { | ||
| if err := tx.Migrator().DropColumn(&tables.TableClientConfig{}, "log_pre_transform_request_data"); err != nil { | ||
| return fmt.Errorf("failed to drop log_pre_transform_request_data column: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| }}) | ||
| if err := m.Migrate(); err != nil { | ||
| return fmt.Errorf("error running add_log_pre_transform_request_data_column migration: %s", err.Error()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Refresh config_hash for existing client configs after this column lands.
LogPreTransformRequestData is now part of the client-config hash, but this migration only adds the column. Existing config_client.config_hash values will remain stale after upgrade, so persisted client configs can look drifted until something rewrites them. Please add a follow-up hash-refresh migration after this DDL step, mirroring the split DDL/DML pattern already used elsewhere in this file.
Suggested shape
if err := migrationAddLogPreTransformRequestDataColumn(ctx, db); err != nil {
return err
}
+if err := migrationRefreshConfigHashAfterLogPreTransformRequestDataColumn(ctx, db); err != nil {
+ return err
+}Then implement migrationRefreshConfigHashAfterLogPreTransformRequestDataColumn like the other config-hash refresh migrations in this file: load existing tables.TableClientConfig rows, rebuild ClientConfig including LogPreTransformRequestData, and persist the new hash for rows that already have one.
As per coding guidelines, "When migrations are added or changed, verify they avoid deadlocks on large tables".
🤖 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 `@framework/configstore/migrations.go` around lines 9009 - 9035, Add a
follow-up DML migration named
migrationRefreshConfigHashAfterLogPreTransformRequestDataColumn that mirrors the
file's existing split DDL/DML pattern: create a migrator.New entry with ID
"refresh_config_hash_after_log_pre_transform_request_data_column", leave the
Migrate step to load existing tables.TableClientConfig rows (in batches to avoid
deadlocks), for each row rebuild the domain ClientConfig including the new
LogPreTransformRequestData field and recompute/persist config_hash only for rows
that currently have a non-empty config_hash, and implement a safe Rollback
(no-op or reverse if applicable); ensure the migration uses ctx via
tx.WithContext and follows the same batching/transaction strategy used by other
refresh migrations in this file to prevent locks on large tables.
| config: | ||
| disable_content_logging: false | ||
| logging_headers: [] | ||
| log_pre_transform_request_data: false |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding client-level default for consistency with schema structure.
The schema defines log_pre_transform_request_data in two locations: client.log_pre_transform_request_data and plugins[logging].config.log_pre_transform_request_data. For consistency with the schema structure and to provide complete default values, consider adding this field to the client section as well (around line 223, near other logging-related flags like enableLogging and disableContentLogging).
📝 Suggested addition to client section
Add to the client section (after line 222 or similar):
client:
dropExcessRequests: false
initialPoolSize: 300
allowedOrigins:
- "*"
enableLogging: true
disableContentLogging: false
+ logPreTransformRequestData: false
disableDbPingsInHealth: falseAs per coding guidelines: transports/config.schema.json exposes log_pre_transform_request_data at both client and plugins[logging].config levels that must stay consistent for this PR's end-to-end behavior.
🤖 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 `@helm-charts/bifrost/values.yaml` at line 418, Add the client-level default
for the log_pre_transform_request_data flag to keep config consistent with the
schema: in the YAML client block (near existing fields like enableLogging,
disableContentLogging, dropExcessRequests, initialPoolSize) add
log_pre_transform_request_data: false so the client-level setting mirrors
plugins[logging].config.log_pre_transform_request_data and ensures
transports/config.schema.json expectations are met.
| // Capture original client body if the toggle is enabled | ||
| if p.logPreTransformRequestData != nil && *p.logPreTransformRequestData { | ||
| if body, ok := ctx.Value(schemas.BifrostContextKeyOriginalClientBody).([]byte); ok && len(body) > 0 { | ||
| initialData.OriginalClientBody = string(body) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do not funnel arbitrary HTTP bytes into a string/TEXT column.
This captures the raw body for every request type, including multipart/audio/image uploads. Converting arbitrary bytes to string and persisting them via original_client_body text can corrupt non-UTF-8 payloads or fail on invalid UTF-8/NUL bytes, so enabling the flag can silently break logging for binary endpoints.
🤖 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 710 - 715, The code unconditionally
converts raw request bytes from
ctx.Value(schemas.BifrostContextKeyOriginalClientBody) into a Go string and
stores it in initialData.OriginalClientBody, which will corrupt non-UTF-8 or
binary payloads (multipart, audio, images). Change the logic in the block
guarded by p.logPreTransformRequestData to detect whether the []byte is valid
UTF-8 (or a text content type) before converting to string; if it is not valid
text, either skip logging the body or encode the bytes safely (e.g., base64) and
mark it as binary. Ensure you update the handling around
p.logPreTransformRequestData,
ctx.Value(schemas.BifrostContextKeyOriginalClientBody) and
initialData.OriginalClientBody accordingly so only safe text is stored in the
TEXT column (or binary-safe encoding is used).
Honor disable_content_logging before setting OriginalClientBody.
Once this field is populated, the new persistence path writes the full pre-transform body even when content logging is explicitly disabled. That bypasses the existing privacy gate and can still log secrets/PII from request payloads.
Suggested fix
- // Capture original client body if the toggle is enabled
- if p.logPreTransformRequestData != nil && *p.logPreTransformRequestData {
+ // Capture original client body only when both toggles allow request-content logging
+ if p.contentLoggingEnabled(ctx) &&
+ p.logPreTransformRequestData != nil &&
+ *p.logPreTransformRequestData {
if body, ok := ctx.Value(schemas.BifrostContextKeyOriginalClientBody).([]byte); ok && len(body) > 0 {
initialData.OriginalClientBody = string(body)
}
}As per coding guidelines, "Apply Go security practices: do not log secrets or sensitive request/response bodies by default."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Capture original client body if the toggle is enabled | |
| if p.logPreTransformRequestData != nil && *p.logPreTransformRequestData { | |
| if body, ok := ctx.Value(schemas.BifrostContextKeyOriginalClientBody).([]byte); ok && len(body) > 0 { | |
| initialData.OriginalClientBody = string(body) | |
| } | |
| } | |
| // Capture original client body only when both toggles allow request-content logging | |
| if p.contentLoggingEnabled(ctx) && | |
| p.logPreTransformRequestData != nil && | |
| *p.logPreTransformRequestData { | |
| if body, ok := ctx.Value(schemas.BifrostContextKeyOriginalClientBody).([]byte); ok && len(body) > 0 { | |
| initialData.OriginalClientBody = string(body) | |
| } | |
| } |
🤖 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 710 - 715, The code currently sets
initialData.OriginalClientBody whenever p.logPreTransformRequestData is true,
which bypasses the privacy gate; add an explicit guard that content logging is
enabled (e.g. check the plugin's disable_content_logging flag such as
p.disableContentLogging or p.config.DisableContentLogging) before reading
schemas.BifrostContextKeyOriginalClientBody and assigning
initialData.OriginalClientBody so the body is never captured when content
logging is disabled; keep the existing p.logPreTransformRequestData check but
require the disable flag to be false before converting the []byte to string and
assigning it.
| PassthroughRequestBody: pending.InitialData.PassthroughRequestBody, | ||
| OriginalClientBody: pending.InitialData.OriginalClientBody, |
There was a problem hiding this comment.
Update batch size estimation for OriginalClientBody.
buildCompleteLogEntryFromPending now carries the pre-transform body into the write queue, but estimateLogEntrySize never counts it. With large request bodies, maxBatchBytes stops being a real cap and batches can grow far past the intended memory budget before flushing.
Suggested fix
n := len(log.InputHistory) +
len(log.ResponsesInputHistory) +
len(log.OutputMessage) +
len(log.ResponsesOutput) +
len(log.EmbeddingOutput) +
len(log.RerankOutput) +
len(log.OCROutput) +
len(log.Params) +
len(log.Tools) +
len(log.ToolCalls) +
len(log.SpeechInput) +
len(log.SpeechOutput) +
len(log.TranscriptionInput) +
len(log.TranscriptionOutput) +
len(log.ImageGenerationInput) +
len(log.ImageGenerationOutput) +
len(log.VideoGenerationInput) +
len(log.VideoGenerationOutput) +
len(log.VideoRetrieveOutput) +
len(log.VideoDownloadOutput) +
len(log.VideoListOutput) +
len(log.VideoDeleteOutput) +
len(log.ListModelsOutput) +
len(log.TokenUsage) +
len(log.ErrorDetails) +
len(log.RawRequest) +
len(log.RawResponse) +
+ len(log.OriginalClientBody) +
len(log.PassthroughRequestBody) +
len(log.PassthroughResponseBody) +
len(log.ContentSummary) +
len(log.CacheDebug) +
len(log.RoutingEngineLogs)🤖 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/writer.go` around lines 445 - 446, The batch-size estimator
isn't counting the pre-transform request body passed through by
buildCompleteLogEntryFromPending (OriginalClientBody), so estimateLogEntrySize
should be updated to include the size of that field; locate the function
estimateLogEntrySize and add logic to account for entry.OriginalClientBody (and
any nil/empty checks) when computing byte size so maxBatchBytes remains a true
cap and large OriginalClientBody values force flushes as intended.
| func (s testHandlerStore) ShouldAllowPerRequestStorageOverride() bool { return false } | ||
| func (s testHandlerStore) ShouldAllowPerRequestRawOverride() bool { return false } | ||
| func (s testHandlerStore) ShouldAllowDirectKeys() bool { return s.allowDirectKeys } | ||
| func (s testHandlerStore) ShouldLogPreTransformRequestData() bool { return false } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a positive-path test for pre-transform body capture.
This helper hardcodes false, so ConvertToBifrostContext's new branch for copying the original client body still isn't exercised in this test file. Please add a case that enables the flag and verifies the stored value is a copy that survives later request-body mutation/reuse.
As per coding guidelines, **/*.go: "deterministic tests, and table-driven coverage for behavior changes."
🤖 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 `@transports/bifrost-http/lib/ctx_test.go` at line 33, Add a positive-path unit
test in transports/bifrost-http/lib/ctx_test.go that uses a testHandlerStore
where ShouldLogPreTransformRequestData() returns true (override the current
hardcoded false) and calls ConvertToBifrostContext with a request that has a
readable body; assert that the stored pre-transform body in the resulting
BifrostContext is a deep copy that remains unchanged even after mutating or
reusing the original request body (e.g., read/close/replace the original Body,
then re-read or modify it). Follow the existing table-driven style: add a
deterministic case that sets up the request body bytes, expected stored bytes,
and performs the mutation/reuse steps, then verify equality between expected and
the context's stored pre-transform bytes to exercise the new branch.
| // Capture the raw client body before any transformations if the toggle is enabled. | ||
| // We copy the bytes because fasthttp reuses its internal buffer after the handler returns, | ||
| // and logging runs asynchronously after that point. | ||
| if store != nil && store.ShouldLogPreTransformRequestData() { | ||
| if body := ctx.Request.Body(); len(body) > 0 { | ||
| bodyCopy := make([]byte, len(body)) | ||
| copy(bodyCopy, body) | ||
| bifrostCtx.SetValue(schemas.BifrostContextKeyOriginalClientBody, bodyCopy) | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard the body copy so it only happens once per request.
ConvertToBifrostContext intentionally reuses the same shared *schemas.BifrostContext across middleware and handlers, so this block can run multiple times for one request. As written, each call re-allocates and copies the full request body again, which is expensive on large payloads and gives you no new data after the first capture.
♻️ Proposed fix
// Capture the raw client body before any transformations if the toggle is enabled.
// We copy the bytes because fasthttp reuses its internal buffer after the handler returns,
// and logging runs asynchronously after that point.
- if store != nil && store.ShouldLogPreTransformRequestData() {
+ if store != nil &&
+ store.ShouldLogPreTransformRequestData() &&
+ bifrostCtx.Value(schemas.BifrostContextKeyOriginalClientBody) == nil {
if body := ctx.Request.Body(); len(body) > 0 {
bodyCopy := make([]byte, len(body))
copy(bodyCopy, body)
bifrostCtx.SetValue(schemas.BifrostContextKeyOriginalClientBody, bodyCopy)
}
}Based on learnings: ConvertToBifrostContext reuses the same shared *schemas.BifrostContext pointer across transport middleware and handlers on a single request.
🤖 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 `@transports/bifrost-http/lib/ctx.go` around lines 641 - 650, The body-copy
block in ConvertToBifrostContext currently reallocates on every invocation;
guard it by checking whether the original body has already been stored (e.g.,
via bifrostCtx.Value(schemas.BifrostContextKeyOriginalClientBody)) before
allocating/copying and calling bifrostCtx.SetValue, and only perform the copy
when store.ShouldLogPreTransformRequestData() is true AND the stored value is
nil; update the logic around bifrostCtx.SetValue and the
store.ShouldLogPreTransformRequestData() check to avoid duplicate copies for the
same request.
| <Switch | ||
| id="log-pre-transform-request-data" | ||
| size="md" | ||
| checked={localConfig.log_pre_transform_request_data ?? false} | ||
| onCheckedChange={(checked) => handleConfigChange("log_pre_transform_request_data", checked)} | ||
| /> |
There was a problem hiding this comment.
Add data-testid for E2E test compatibility.
The new Switch component is missing a data-testid attribute. Other switches in this file include data-testid attributes for E2E testing (e.g., lines 181, 204, 247). As per coding guidelines, add data-testid to all new interactive elements in React components for E2E test compatibility.
🧪 Proposed fix
<Switch
id="log-pre-transform-request-data"
+ data-testid="workspace-log-pre-transform-request-data-switch"
size="md"
checked={localConfig.log_pre_transform_request_data ?? false}
onCheckedChange={(checked) => handleConfigChange("log_pre_transform_request_data", checked)}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Switch | |
| id="log-pre-transform-request-data" | |
| size="md" | |
| checked={localConfig.log_pre_transform_request_data ?? false} | |
| onCheckedChange={(checked) => handleConfigChange("log_pre_transform_request_data", checked)} | |
| /> | |
| <Switch | |
| id="log-pre-transform-request-data" | |
| data-testid="workspace-log-pre-transform-request-data-switch" | |
| size="md" | |
| checked={localConfig.log_pre_transform_request_data ?? false} | |
| onCheckedChange={(checked) => handleConfigChange("log_pre_transform_request_data", checked)} | |
| /> |
🤖 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 `@ui/app/workspace/config/views/loggingView.tsx` around lines 153 - 158, The
Switch for "log-pre-transform-request-data" in loggingView.tsx is missing a
data-testid required for E2E tests; update the Switch component
(id="log-pre-transform-request-data") to include a data-testid attribute (e.g.,
data-testid="log-pre-transform-request-data") so the element can be targeted by
tests, ensuring the change is applied alongside the existing checked and
onCheckedChange props that call localConfig and handleConfigChange.
1 participant
Summary
Adds a new opt-in setting,
log_pre_transform_request_data, that captures the raw HTTP request body exactly as received from the client before Bifrost applies any transformations (e.g. compatibility conversions, prompt template injection). The captured body is stored asoriginal_client_bodyin log records and surfaced in the Raw tab of the log detail view in the UI.Changes
BifrostContextKeyOriginalClientBodycontext key to carry the raw client body through the request lifecycle.LogPreTransformRequestDatafield toClientConfigandTableClientConfig, wired throughGetClientConfig/UpdateClientConfig, and included in the config hash.client_configtable (log_pre_transform_request_datacolumn) and thelogstable (original_client_bodycolumn).ConvertToBifrostContext, whenShouldLogPreTransformRequestData()is true, the raw fasthttp request body is copied into the Bifrost context before any handler processing. The copy is necessary because fasthttp reuses its internal buffer after the handler returns and logging is asynchronous.PreLLMHookand populatesInitialLogData.OriginalClientBody, which flows through to the initial log insert and the complete log entry builder.ShouldLogPreTransformRequestData()added to theHandlerStoreinterface and implemented onConfig, with stub implementations added to all test stores.ConfigandLoggerPluginstructs now carry aLogPreTransformRequestDatapointer for live config reads without restart.log_pre_transform_request_datatoCoreConfigtype andDefaultCoreConfig, and rendersoriginal_client_bodyin the Raw tab of the log detail view above the existing raw request/response blocks.Type of change
Affected areas
How to test
log_pre_transform_request_datatoggle in Workspace → Config → Logging.original_client_bodyis captured on subsequent requests.New config field:
log_pre_transform_request_databoolfalseoriginal_client_bodyin log records.Breaking changes
Security considerations
original_client_bodymay contain sensitive user data or credentials embedded in request payloads. This feature is opt-in and disabled by default. Operators should ensure their log retention and access control policies account for the additional PII or secret material that may be stored when this setting is enabled.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Release Notes