feat(audit): record ingress rewriters that changed nothing - #594
Conversation
The request-rewrite chain only recorded a revision when a rewriter returned a new body, so an operator looking at an audit entry could not tell "compression ran and found nothing to dedupe" apart from "compression never ran at all" — the two look identical in the UI. Rewriters that run and leave the body alone now get a revision snapshot flagged NoChange (BytesAfter == BytesBefore, no body, no savings). The flag is positive rather than negated so entries written before this change still read correctly: an old revision always changed the body. The dashboard keeps these out of the tab strip — they are steps, not versions of the request — and renders them as a quiet dashed pill on the Request tab instead. Tab numbering is unaffected: panes are built from the changed revisions only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRequest rewrite auditing now records rewriters that leave the body unchanged, adds the ChangesRequest rewrite audit tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestRewriteMiddleware
participant AuditLog
participant Dashboard
Client->>RequestRewriteMiddleware: submit request
RequestRewriteMiddleware->>AuditLog: record changed and no-change revisions
AuditLog-->>Dashboard: expose request revision data
Dashboard-->>Dashboard: render changed panes and no-change pills
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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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 `@cmd/gomodel/docs/docs.go`:
- Line 8064: Update the RequestRevisions description annotation to state that
when all rewriters are no-ops, the original request body is forwarded because no
last changed revision exists, and document the no_change invariants: bytes_after
equals bytes_before, no body is captured, and savings is zero. Then regenerate
the generated OpenAPI documentation artifact.
In `@internal/server/request_rewrite_test.go`:
- Around line 440-501: The TestRequestRewriteMiddlewareRecordsNoChangeRevisions
test should use table-driven subtests to cover nil-result, header-only/nil-body,
and body-rewrite rewriter behaviors. Refactor the existing inline rewriters and
assertions into behavior-focused table entries while preserving verification of
revision metadata, response headers, body sizes, and savings.
🪄 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: 338acd19-f9c3-40ff-bf9c-1e3b0b339eff
⛔ Files ignored due to path filters (3)
internal/admin/dashboard/static/dist/assets/index-BuCjMNNr.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-D19L8xXa.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (8)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/auditlog/auditlog.gointernal/server/request_rewrite.gointernal/server/request_rewrite_test.goweb/dashboard/src/pages/audit-logs/AuditPaneTabs.svelteweb/dashboard/src/pages/audit-logs/audit-logic.jsweb/dashboard/tests/audit-list.test.js
| func TestRequestRewriteMiddlewareRecordsNoChangeRevisions(t *testing.T) { | ||
| // A rewriter that inspects the request and forwards it untouched is | ||
| // still a step operators need to see, so it gets a no-change revision. | ||
| quiet := &stubRewriter{name: "quiet"} | ||
| // Response headers without a body change (a rewriter annotating why it | ||
| // did nothing) must not turn the step into a real revision. | ||
| annotating := &stubRewriter{ | ||
| name: "annotating", | ||
| rewrite: func(ext.Input) (*ext.Result, error) { | ||
| header := http.Header{} | ||
| header.Set("X-Test-Rewriter", "skipped") | ||
| return &ext.Result{ResponseHeader: header}, nil | ||
| }, | ||
| } | ||
|
|
||
| auditLogger := &capturingAuditLogger{config: auditlog.Config{Enabled: true, LogBodies: true}} | ||
| srv := New(newRewriteTestProvider(), &Config{ | ||
| AuditLogger: auditLogger, | ||
| RequestRewriters: []ext.RequestRewriter{ | ||
| quiet, | ||
| replaceBodyRewriter("swap", "PING", "PONG"), | ||
| annotating, | ||
| }, | ||
| }) | ||
| rec := postJSON(t, srv, "/v1/chat/completions", | ||
| `{"model":"gpt-4o-mini","messages":[{"role":"user","content":"PING"}]}`) | ||
| if rec.Code != http.StatusOK { | ||
| t.Fatalf("expected 200, got %d (%s)", rec.Code, rec.Body.String()) | ||
| } | ||
| if rec.Header().Get("X-Test-Rewriter") != "skipped" { | ||
| t.Error("response headers from a no-change rewriter must still be applied") | ||
| } | ||
| if len(auditLogger.entries) == 0 { | ||
| t.Fatal("expected an audit entry") | ||
| } | ||
|
|
||
| revisions := auditLogger.entries[0].Data.RequestRevisions | ||
| if len(revisions) != 3 { | ||
| t.Fatalf("expected 3 revisions (2 no-change + 1 rewrite), got %d: %+v", len(revisions), revisions) | ||
| } | ||
| for i, want := range []struct { | ||
| rewriter string | ||
| noChange bool | ||
| }{{"quiet", true}, {"swap", false}, {"annotating", true}} { | ||
| got := revisions[i] | ||
| if got.Seq != i+1 || got.Rewriter != want.rewriter || got.NoChange != want.noChange { | ||
| t.Errorf("revision %d = %+v, want rewriter %q no_change=%v", i+1, got, want.rewriter, want.noChange) | ||
| } | ||
| } | ||
|
|
||
| quietRev := revisions[0] | ||
| if quietRev.BytesBefore == 0 || quietRev.BytesAfter != quietRev.BytesBefore { | ||
| t.Errorf("no-change revision must report equal sizes: %+v", quietRev) | ||
| } | ||
| if quietRev.Body != nil || quietRev.TokensSaved != 0 { | ||
| t.Errorf("no-change revision must carry no body or savings: %+v", quietRev) | ||
| } | ||
| // The trailing no-change step sees the body the previous rewriter produced. | ||
| if revisions[2].BytesBefore != revisions[1].BytesAfter { | ||
| t.Errorf("no-change revision must measure the current body: %+v", revisions[2]) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the new behavior coverage table-driven.
Model the nil-result, header-only/nil-body, and body-rewrite cases as table entries/subtests. 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/server/request_rewrite_test.go` around lines 440 - 501, The
TestRequestRewriteMiddlewareRecordsNoChangeRevisions test should use
table-driven subtests to cover nil-result, header-only/nil-body, and
body-rewrite rewriter behaviors. Refactor the existing inline rewriters and
assertions into behavior-focused table entries while preserving verification of
revision metadata, response headers, body sizes, and savings.
Source: Coding guidelines
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Confidence Score: 4/5The PR is safe to merge, with a non-blocking audit-detail preservation issue. No-change revisions correctly preserve execution order, byte counts, headers, and dashboard behavior, but structured detail returned alongside a nil body is omitted from the new audit snapshot. Files Needing Attention: internal/server/request_rewrite.go
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| if res == nil || res.Body == nil { | ||
| // The rewriter ran and left the request alone. Record the | ||
| // step anyway so the audit trail distinguishes "compression |
There was a problem hiding this comment.
When a rewriter returns a nil body with structured Detail, this branch records an unchanged revision without passing that detail, preventing the audit trail from explaining why the rewriter made no change.
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!
A rewriter can return a result with no body but a structured Detail — that is where it explains why it left the request alone. The no-change snapshot dropped it, so the audit trail recorded that the step ran but not what it concluded. Detail now carries over when the rewriter returned a result; a rewriter that declined outright (nil result) still has nothing to record. Also documents the all-no-op case on RequestRevisions (no last changed revision means the original body was forwarded) and the no-change invariants on the flag itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Why
The ingress request-rewrite chain only wrote an audit revision when a rewriter returned a new body. So in the audit log UI, "compression ran and found nothing to dedupe" and "compression never ran at all" look identical — there is simply no step. That ambiguity is what prompted this: real traffic with no repeated line runs produces no compression tab, and the only way to tell the difference was to check response headers or the DB.
What
RequestRevisionSnapshotgainsNoChange bool. Rewriters that run and leave the body alone now get a snapshot recorded withBytesAfter == BytesBefore, no body and no savings. The flag is positive rather than negated so pre-existing entries still read correctly: an old revision always changed the body.request_rewrite.gorecords the step on both no-op paths (res == nilandres.Body == nil), and still applies any response headers such a rewriter set. Byte counts measure the current body, so a no-op after an earlier rewrite reports the rewritten size.pro-token-compression: no change). Tab numbering is unchanged because panes are built from the changed revisions only.docs.go/openapi.jsonand the embedded dashboard bundle.Testing
TestRequestRewriteMiddlewareRecordsNoChangeRevisionscovers a mixed chain (no-op → body swap → header-only rewriter): order, the flag, equal byte sizes, nil body, header still applied, and that a trailing no-op measures the post-rewrite size.go test ./internal/server/... ./internal/auditlog/...and the 361-test dashboard suite pass; all pre-commit hooks green, including the dist-in-sync check.tokens_saved: 430, an incompressible one records{"rewriter":"pro-token-compression","bytes_before":1494,"bytes_after":1494,"no_change":true}.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests