fix(auditlog): keep request revisions on streamed entries - #595
Conversation
CreateStreamEntry rebuilds LogData with a field whitelist, and the stream observer completes and persists that copy — the base entry is never written. RequestRevisions was missing from the whitelist, so the ingress rewrite chain recorded by EnrichEntryWithRequestRevision was discarded on every streamed request. The audit UI's "Rewritten" pane therefore never appeared for successful streams, while surviving on the non-streamed error path. Measured on a live gateway: of 72 requests where a rewriter reported token savings into the usage table, 0 retained a revision snapshot; all 6 non-streamed failures in the same window kept theirs. RequestBodyTooBigToHandle was dropped the same way — also a request-side fact known before the stream opens. Response-side fields (ResponseBody, ErrorMessage, ErrorCode, ResponseBodyTooBigToHandle) stay omitted on purpose: the observer fills them once the stream closes. Adds a reflection-based drift guard that walks LogData and fails when any request-side field does not survive the copy, since the whitelist will keep attracting this bug otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesStreamed audit entry field preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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 `@internal/auditlog/stream_entry_request_fields_test.go`:
- Around line 25-128: Convert TestCreateStreamEntryPreservesRequestRevisions
into table-driven cases covering absent and populated RequestRevisions and
request metadata, asserting each case’s expected copied values and ownership
behavior. Retain TestCreateStreamEntryCopiesEveryRequestSideField as the
reflection-based drift guard, and keep the cases focused on CreateStreamEntry
behavior.
- Around line 61-65: Update the regression test around the RequestRevisions copy
to mutate an existing element in base.Data.RequestRevisions after the stream
entry is created, then assert the corresponding streamed element remains
unchanged. Keep the append assertion if useful, but ensure the test specifically
detects shared backing arrays rather than only reallocation behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3554e603-5721-440d-9192-a6bae2569f48
📒 Files selected for processing (2)
internal/auditlog/stream_entry_request_fields_test.gointernal/auditlog/stream_wrapper.go
| func TestCreateStreamEntryPreservesRequestRevisions(t *testing.T) { | ||
| base := &LogEntry{ | ||
| ID: "entry-1", | ||
| Path: "/v1/chat/completions", | ||
| Data: &LogData{ | ||
| RequestRevisions: []RequestRevisionSnapshot{{ | ||
| Seq: 1, | ||
| Rewriter: "pro-token-compression", | ||
| BytesBefore: 65209, | ||
| BytesAfter: 64418, | ||
| TokensSaved: 189, | ||
| Detail: map[string]any{"chars_removed": 757}, | ||
| }}, | ||
| RequestBodyTooBigToHandle: true, | ||
| }, | ||
| } | ||
|
|
||
| streamEntry := CreateStreamEntry(base) | ||
| if streamEntry == nil || streamEntry.Data == nil { | ||
| t.Fatal("expected a stream entry with data") | ||
| } | ||
|
|
||
| got := streamEntry.Data.RequestRevisions | ||
| if len(got) != 1 { | ||
| t.Fatalf("RequestRevisions dropped: got %d revisions, want 1", len(got)) | ||
| } | ||
| if got[0].Rewriter != "pro-token-compression" || got[0].TokensSaved != 189 { | ||
| t.Fatalf("revision not copied faithfully: %+v", got[0]) | ||
| } | ||
| if got[0].BytesBefore != 65209 || got[0].BytesAfter != 64418 { | ||
| t.Fatalf("revision byte counts not copied: %+v", got[0]) | ||
| } | ||
| if !streamEntry.Data.RequestBodyTooBigToHandle { | ||
| t.Error("RequestBodyTooBigToHandle dropped") | ||
| } | ||
|
|
||
| // The copy must own its slice, so later appends to the base entry cannot | ||
| // reach into the entry the observer is writing. | ||
| base.Data.RequestRevisions = append(base.Data.RequestRevisions, RequestRevisionSnapshot{Seq: 2}) | ||
| if len(streamEntry.Data.RequestRevisions) != 1 { | ||
| t.Error("stream entry shares its revision backing array with the base entry") | ||
| } | ||
| } | ||
|
|
||
| // CreateStreamEntry builds LogData with a field whitelist, so any request-side | ||
| // field added to LogData later is silently dropped until someone remembers to | ||
| // extend that literal. This walks LogData by reflection and fails when a | ||
| // request-side field does not survive, which is how RequestRevisions went | ||
| // missing in the first place. | ||
| func TestCreateStreamEntryCopiesEveryRequestSideField(t *testing.T) { | ||
| populated := &LogData{} | ||
| v := reflect.ValueOf(populated).Elem() | ||
| typ := v.Type() | ||
|
|
||
| for i := range typ.NumField() { | ||
| field := typ.Field(i) | ||
| if !v.Field(i).CanSet() { | ||
| continue | ||
| } | ||
| if !setRecognizableValue(v.Field(i)) { | ||
| t.Fatalf("test needs a sample value for LogData.%s (%s)", field.Name, field.Type) | ||
| } | ||
| } | ||
|
|
||
| streamEntry := CreateStreamEntry(&LogEntry{ID: "entry-1", Data: populated}) | ||
| if streamEntry == nil || streamEntry.Data == nil { | ||
| t.Fatal("expected a stream entry with data") | ||
| } | ||
|
|
||
| copied := reflect.ValueOf(streamEntry.Data).Elem() | ||
| for i := range typ.NumField() { | ||
| name := typ.Field(i).Name | ||
| if responseSideLogDataFields[name] { | ||
| continue | ||
| } | ||
| if copied.Field(i).IsZero() { | ||
| t.Errorf("LogData.%s is a request-side field but CreateStreamEntry dropped it", name) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // setRecognizableValue fills one field with a non-zero value so a dropped | ||
| // field shows up as the zero value on the other side of the copy. | ||
| func setRecognizableValue(field reflect.Value) bool { | ||
| switch field.Kind() { | ||
| case reflect.String: | ||
| field.SetString("x") | ||
| case reflect.Bool: | ||
| field.SetBool(true) | ||
| case reflect.Map: | ||
| m := reflect.MakeMap(field.Type()) | ||
| m.SetMapIndex(reflect.ValueOf("k"), reflect.ValueOf("v")) | ||
| field.Set(m) | ||
| case reflect.Slice: | ||
| field.Set(reflect.MakeSlice(field.Type(), 1, 1)) | ||
| case reflect.Pointer: | ||
| field.Set(reflect.New(field.Type().Elem())) | ||
| case reflect.Interface: | ||
| field.Set(reflect.ValueOf(map[string]any{"k": "v"})) | ||
| default: | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use table-driven cases for the new behavior.
Cover at least absent and populated request revisions/metadata as table cases, while retaining the reflection drift guard. As per coding guidelines, “Add or update table-driven tests 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 `@internal/auditlog/stream_entry_request_fields_test.go` around lines 25 - 128,
Convert TestCreateStreamEntryPreservesRequestRevisions into table-driven cases
covering absent and populated RequestRevisions and request metadata, asserting
each case’s expected copied values and ownership behavior. Retain
TestCreateStreamEntryCopiesEveryRequestSideField as the reflection-based drift
guard, and keep the cases focused on CreateStreamEntry behavior.
Source: Coding guidelines
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Confidence Score: 5/5The change appears safe to merge with no actionable correctness or security issues identified. The streamed audit copy retains the missing request metadata, and ownership and field-preservation tests cover the corrected behavior without exposing a reachable regression.
 T-Rex LogsWhat T-Rex did
|
The append-based check could not fail: the source literal has no spare capacity, so append reallocates and leaves the copy untouched whether or not the two share a backing array. Verified by swapping the clone for a shallow assignment — the test still passed. Writing through an existing element does distinguish them, and now fails against that same shallow assignment. Also covers the absent-revisions case, which is what most streamed requests hit: it must stay nil rather than becoming an empty slice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — the aliasing point was correct and it was a real hole in the test, not just a style nit. I confirmed it before fixing: swapping I also added the absent-revisions case, which is the common path — most streamed requests carry no rewriter, and the field must stay On the table-driven suggestion: skipping it. The two cases here assert different things (one checks field-by-field fidelity plus slice ownership, the other checks a nil stays nil), so folding them into a table would mean a case struct of mostly-unused fields and a body of conditionals — less legible than two named tests, not more. The reflection drift guard already provides the exhaustive per-field coverage a table would be reaching for. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/auditlog/stream_entry_request_fields_test.go (1)
30-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the sequence and detail fields too.
The test initializes
SeqandDetailbut never verifies them, so a copy implementation that drops either field would still pass.Proposed assertion adjustment
- if got[0].Rewriter != "pro-token-compression" || got[0].TokensSaved != 189 { + if got[0].Seq != 1 || + got[0].Rewriter != "pro-token-compression" || + got[0].TokensSaved != 189 || + got[0].Detail["chars_removed"] != 757 { t.Fatalf("revision not copied faithfully: %+v", got[0]) }🤖 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 `@internal/auditlog/stream_entry_request_fields_test.go` around lines 30 - 56, Extend the assertions in the streamEntry RequestRevisions test to verify that the copied revision preserves both Seq and Detail, alongside the existing Rewriter, TokensSaved, and byte-count checks. Compare Seq to 1 and Detail to the initialized chars_removed value, while keeping the current failure reporting style.
🤖 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.
Outside diff comments:
In `@internal/auditlog/stream_entry_request_fields_test.go`:
- Around line 30-56: Extend the assertions in the streamEntry RequestRevisions
test to verify that the copied revision preserves both Seq and Detail, alongside
the existing Rewriter, TokensSaved, and byte-count checks. Compare Seq to 1 and
Detail to the initialized chars_removed value, while keeping the current failure
reporting style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7ca9295-d56d-427c-bfda-c08541ce58cc
📒 Files selected for processing (1)
internal/auditlog/stream_entry_request_fields_test.go
A streamed request is persisted from the
CreateStreamEntrycopy — the base entry is never written — so an ingress rewrite chain recorded byEnrichEntryWithRequestRevisionwas lost the moment the request streamed.CreateStreamEntrybuildsLogDatawith a field whitelist, andRequestRevisionswas not on it.The visible effect: the "Rewritten" audit pane was empty for every successful streamed request, while the same request on the non-streamed error path kept its snapshot.
Evidence
Successful chat completions in a local database, cross-referenced against whether a request rewriter actually fired:
The bug row — fired but no revision — exists only for streamed requests. Non-streaming never lost one, because
CreateStreamEntryhas only two callers and both are streaming paths.Changes
CreateStreamEntrycopiesRequestRevisions(throughslices.Clone, so the copy owns its slice) andRequestBodyTooBigToHandle.LogDataand fails when any request-side field does not survive the whitelist copy. Response-side fields are exempted by name — that list is the only thing a future field has to be added to, instead of silently vanishing the wayRequestRevisionsdid.Operational note
Streamed requests will now each persist a revision, and a revision carries a full copy of the rewritten body (gated by
LogBodies, capped atMaxBodyCapture= 1 MiB). Audit storage will grow faster than before: from storing ~1% of streamed rewrites to 100% of them.🤖 Generated with Claude Code
Summary by CodeRabbit