feat(raw): capture raw LLM traffic via SDK middleware (opt-in) - #1109
feat(raw): capture raw LLM traffic via SDK middleware (opt-in)#1109yingjiexu2002 wants to merge 3 commits into
Conversation
Add opt-in raw capture (OCR_RAW_LOGGING=1) built on the SDK HTTP middleware seam: every real HTTP attempt against the LLM endpoint is recorded verbatim - raw request body after extra_body merging and session-key expansion, raw response body (SSE bodies stored as text), redacted headers, model, duration and per-attempt identity from RequestMeta - to ~/.opencodereview/raw/<repo>/<session>.jsonl. The holder/writer split mirrors the RetryCollector pattern: the holder is created with the client in loadLLMRuntime, the per-session writer is bound from the cmd layer once the session exists. Default off mounts no middleware and changes nothing.
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| var reqBody []byte | ||
| if req.Body != nil { | ||
| reqBody, _ = io.ReadAll(req.Body) | ||
| req.Body = io.NopCloser(bytes.NewReader(reqBody)) | ||
| } |
There was a problem hiding this comment.
When io.ReadAll(req.Body) fails, the error is silently discarded and the request body is replaced with a potentially truncated buffer. Unlike the response body error path (which replays the error via errReader so the SDK sees it), this path corrupts the request silently — the SDK sends a partial body to the LLM API with no indication anything went wrong.
While SDK-constructed request bodies are in-memory and read failures are extremely unlikely, the asymmetry with the response error handling suggests this was overlooked rather than intentional. If a read error does occur, consider either:
- Logging the error to stderr (consistent with other best-effort warnings in this codebase), or
- Skipping body capture but still forwarding the original request unchanged (requires reading into a buffer that can be replayed on error, or restructuring to avoid consuming the body until after the read succeeds).
There was a problem hiding this comment.
Keeping this path as-is. The middleware sits inside the SDK retry loop, where req.Body is an in-memory buffer built by the SDK, so io.ReadAll cannot fail and nothing truncated can be forwarded. The response-side error replay exists because network streams can genuinely fail mid-read; request bodies have no such failure mode.
| if err := w.encoder.Encode(rec); err != nil { | ||
| return | ||
| } | ||
| _ = w.writer.Flush() |
There was a problem hiding this comment.
If encoder.Encode fails due to an I/O error (e.g., transient disk pressure), it may have already written partial JSON bytes into the buffered writer. Since the method returns without flushing or resetting state, the next successful Encode will append after those stale bytes, producing a corrupted JSONL line that downstream post-processing cannot parse.
Consider either recreating the encoder after an encode error, or using json.Marshal + explicit writer.Write + writer.WriteByte('\n') (as the session JSONL writer in persist.go does) so that a failed write leaves no partial state in the encoder. The marshal-then-write approach also makes the error boundary cleaner: marshal cannot fail for this struct, so any error is purely I/O and the buffer is untouched on failure.
|
CI / windows (pull_request)Failing after 2m |
Finalize the session in the bind tests so the session JSONL handle is released before TempDir cleanup, which fails on Windows while the file is open. Replace the Unix-only HOME failure injection in TestRawFileWriter_OpenFailure with a cross-platform squat on the target raw directory path. Marshal each record in memory before writing so a failed disk write cannot leave a partial JSONL line that corrupts the next record.
1. Register the raw middleware before the retry observer. Registering it after placed it innermost in the SDK middleware chain, so the observer's next() covered raw's full-body read and synchronous disk write; under streaming that turned DurationToHeadersMS from a headers-only measurement into the whole generation time whenever OCR_RAW_LOGGING=1. Add a regression test with a deliberately slow writer that turns red under the old order. 2. An empty request body produced a zero-length RawMessage, which fails json.Marshal and made RawFileWriter silently drop the whole record. Leave Request nil in that case so the record is written with "request": null. Extend the empty-bodies test with an encodability assertion. 3. Add status_code to RawRecord so HTTP-level failures are visible without parsing the body, and document the capture-point semantics: on Bedrock the records show the pre-signing Anthropic-shaped request and the SSE-normalized streaming response, because the bedrock adaptation runs closer to the wire. Drop the "exactly what was sent" claim the Bedrock path never honoured. 4. UseTestSessions now redirects rawSubDir to "test-raw" alongside the session redirect, and TestRawSubDirIsSeparateFromSessions pins the literals instead of comparing two already-redirected variables. 5. A request-body read failure is now replayed to the SDK via errReader, mirroring the response side, instead of handing it a clean truncated body that turns a client-side fault into a server-side 400. The record carries the error and leaves Request nil, since the partial bytes are not valid JSON and would fail Encode. 6. A failed write no longer kills later captures: records go straight to the file, a failed encode or write warns once on stderr and drops only that record, and the next one retries. The truncate/offset recovery machinery was removed entirely: after a failed Truncate the anchor could drift and a later recovery could cut into records already on disk, and a partial line from a rare short write is acceptable — readers skip unparseable lines. Also correct the reopen test's stale claim that resume reuses the session ID; every run gets a fresh one. 7. Repeated request headers survive as an RFC 9110 comma-joined list instead of being truncated to the first value. 8. When the request-body read fails and next() fails too, the record keeps both errors instead of letting next's error overwrite the root cause. 9. Non-JSON request bytes fall back to a request_text field, mirroring the response side, so a malformed body cannot fail the record's encode and drop it whole. Unreachable while the SDK marshals every body; the guard completes the writer-level invariant that every record encodes. 10. The raw writer's closer detaches the holder before closing the file (holder.Set(nil)), so LLM calls that run after the closer bypass capture instead of writing to a closed file. The no-write-after-close guarantee was structurally absent and only held through defer registration order. 11. The telemetry docs (five languages) now state that with raw capture on, streaming responses are read in full before the capture hands them on to the rest of the processing, and that capture redacts request headers but records request and response bodies as-is.
b450d03 to
3d01ad3
Compare
2 participants
Description
Add an opt-in raw capture channel for LLM traffic. With
OCR_RAW_LOGGING=1, every real HTTP attempt against the LLM endpoint is recorded as one JSONL line under~/.opencodereview/raw/<repo>/<session-id>.jsonl.The capture mounts as SDK HTTP middleware inside the retry loop, so each attempt, including retries, gets its own record. A record carries the raw request body after extra_body merging and session-key expansion, the raw response body (SSE streams are stored as text), request headers with credential-bearing ones redacted, model, duration, and per-attempt identity (session_id, request_id, plus the file_path/task_type/request_no triple). When telemetry is on, the OTel trace_id is stamped too, so raw records can be joined to the exported span tree. When it is off, the field is simply omitted.
The holder/writer split follows the existing RetryCollector pattern. The holder is created with the client in loadLLMRuntime, before any session exists, and the per-session writer is bound from the cmd layer once the session is known. Raw capture must never break a review, so every capture step tolerates errors and a writer open failure degrades the run to no capture.
The default is off. With the switch unset, no middleware is mounted, no files are created, and nothing about existing behavior changes.
The feature supports deep debugging of what was actually sent to and returned by the LLM endpoint, beyond what the structured session transcripts keep.
Type of Change
How Has This Been Tested?
Unit tests cover the middleware (capture, redaction, retry-per-attempt records, duration semantics, body read errors), the JSONL writer (path layout, per-record flush, append across reopen, concurrency, open failure), and the cmd-layer binding (switch gating, nil-holder no-op, session file creation).
make testpasses locallyManual run with
OCR_RAW_LOGGING=1against a real review confirmed records land under~/.opencodereview/raw/<repo>/<session-id>.jsonlwith request/response bodies, redacted headers, identity fields and trace_id (telemetry on). A run without the switch confirmed no middleware is mounted and no files are created.Checklist
go fmt,go vet)Related Issues
None