feat(coderd/x/chatd/chatdebug): add recorder, transport, and redaction#23915
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56bd768449
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThomasK33
force-pushed
the
devtools/02-types-context-model-normalization
branch
from
524ffa6 to
e897b1c
Compare
ThomasK33
force-pushed
the
devtools/03-recorder-transport-redaction
branch
2 times, most recently
from
25352e2 to
83e32a4
Compare
ThomasK33
force-pushed
the
devtools/02-types-context-model-normalization
branch
2 times, most recently
from
f4b6ab8 to
93e4b64
Compare
ThomasK33
force-pushed
the
devtools/03-recorder-transport-redaction
branch
from
83e32a4 to
40cf3a2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40cf3a2bb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThomasK33
force-pushed
the
devtools/03-recorder-transport-redaction
branch
from
40cf3a2 to
7a48e47
Compare
ThomasK33
force-pushed
the
devtools/02-types-context-model-normalization
branch
from
7d62a24 to
7777092
Compare
ThomasK33
force-pushed
the
devtools/03-recorder-transport-redaction
branch
from
7a48e47 to
3bd356b
Compare
ThomasK33
force-pushed
the
devtools/02-types-context-model-normalization
branch
from
7777092 to
d4c98a0
Compare
ThomasK33
force-pushed
the
devtools/03-recorder-transport-redaction
branch
from
3bd356b to
0f95b7f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f95b7f53c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThomasK33
force-pushed
the
devtools/03-recorder-transport-redaction
branch
from
0f95b7f to
e0fa471
Compare
ThomasK33
force-pushed
the
devtools/02-types-context-model-normalization
branch
from
d4c98a0 to
39891e7
Compare
|
@codex review |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 10. All 10 previously-open findings are addressed. The author has been responsive and thorough: F1-F7 (both P1s, all P2s) were fixed with appropriate tests, F9-F11 and F23-F25 from later rounds likewise closed. The contested F8 (dead fields on stepHandle) was evaluated by the full panel: 4/4 accept the author's defense that the fields serve as test observation points.
New this round: 3 P3 test coverage gaps from Bisky (mid-stream read error, maxDrainBytes safety valve, 204/304 no-body cases), 1 P3 from Ging-Go (wg.Go modernization), and 2 Nits. No P0-P2. The code has been hardened across 10 rounds.
"I tried to build a case against this change and could not. The problem is correctly understood, the solution is proportional, and the fix is at the right causal level." - Pariston, after evaluating four alternative diagnostic framings
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 700120b897
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 11. All 5 findings from R10 addressed. F27-F29 (test gaps for mid-stream error, drain cap, 204/304) now have dedicated tests. F30 (wg.Go) and F31 (slices.Sort) modernized. The author continues to be responsive.
Two new P3s from Bisky and one Nit from Netero. The drain cap test (F28's fix) is a costume gem: the assertion proves the code path executes but not that the safety valve works. The inner Close() error path has zero coverage. And an em-dash snuck in.
Mafuuu, Pariston, and Razor found nothing new. Pariston's differential diagnosis evaluated four alternative framings and confirmed the transport-layer approach is the right call.
"Every other status assertion in this file uses require.Equal. This one uses require.NotEmpty. It would pass if the status were 'completed', 'failed', or 'banana'." - Bisky, on the maxDrainBytes test
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 12. Review blocked. Three findings from R11 have no author response:
F33 (Nit): Em-dash (U+2014) at transport.go:291. Replace with hyphen-minus.
F34 (P3): MaxDrainBytes test asserts nothing about the drain cap. The require.NotEmpty assertion would pass with the safety valve removed.
F35 (P3): No test for inner Close() returning an error. The closeErr != nil path at transport.go:249-256 has zero coverage.
Further review is blocked until the author responds to or addresses these findings.
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ec8c3fb33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4338c44425
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 15. All 3 findings from R11 addressed (F33 em-dash, F34 drain test, F35 close-error test). Netero: clean. The code has been through 35 findings across 12 review rounds; all are resolved, closed, dropped, or deferred with ticket.
One new P3 from Zoro (wildcard): 17 lines of buffer capture logic duplicated verbatim between Read() and drainToEOF(). Two Nits: a test name that misrepresents the mechanism it exercises, and a 5-line media-type extraction block duplicated between isJSONLikeContentType and isNDJSONContentType.
"After 15 rounds of review (35 findings, all resolved), the code handles edge cases I would not have thought to test: mid-stream read errors, close-before-EOF with JSON completeness checks, maxDrainBytes safety valve, double-close idempotency, 204/304 no-body responses, NDJSON per-line redaction, SSE preservation, and URL credential sanitization in error messages." - Pariston
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0690d1918e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 16. All 3 findings from R15 addressed (F36 buffer dedup, F37 test rename, F38 media-type helper). Netero: clean. Chopper (wildcard): clean.
One new P3 from Bisky: isJSONLikeContentType has an untested +json suffix branch. Content types like application/vnd.api+json or application/ld+json only exercise the HasSuffix path. If that check is removed, bodies with those types skip JSON redaction and secrets are stored unredacted.
The code has been through 16 rounds with 39 tracked findings. 36 resolved by author, 1 closed by panel, 1 deferred with ticket, 1 open.
"After fifteen rounds it wears its coverage like a crown. Let me look closer." - Bisky, before finding the one missing gem
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e8e5df24c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 17. F39 (+json suffix test) addressed. Netero: clean. Mafuuu, Pariston, Knov: no findings.
One new P3 from Bisky: 10 of 34 transport tests don't assert the attempt's Status field. The canonical CaptureResponse test verifies ResponseStatus, ResponseHeaders, and ResponseBody but not whether the attempt was recorded as "completed" vs "failed". A regression in the completion path would pass these tests silently.
39 findings across 17 rounds. 37 resolved by author, 1 closed by panel, 1 deferred with ticket, 1 open (F40).
"After 17 rounds it's been polished like a real stone. But I have my loupe out." - Bisky
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2066c8259
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 18. F40 (Status assertions) addressed. Netero and panel: clean except for two Nits.
The suite now has 34 transport tests with Status assertions on every path. Bisky confirms: "After eighteen rounds, the costume jewelry is gone. What's left is genuine."
Two Nits: trackingReadCloser.closed is set but never read (Netero), and PrematureCloseMarksFailed is the only failure-path test omitting the Error assertion (Bisky). 42 findings tracked, 40 resolved, 1 deferred (#24137), 2 Nits open.
"One stone has a small inclusion." - Bisky, on the lone missing Error assertion among ten failure-path tests
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac68b57dfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
Round 19. F41 and F42 addressed. Netero: clean. Panel (Mafuuu, Bisky, Pariston, Razor): no findings.
42 findings across 19 rounds. 40 resolved by author, 1 closed by panel (F8, 4/4 accept), 1 deferred with ticket (#24137). Zero open.
The recording transport, step recorder, and redaction layer are well-designed, well-tested, and have been hardened through systematic review. The redaction layer handles headers, JSON bodies, NDJSON, URLs, and error messages with appropriate fail-closed behavior. The transport's close semantics correctly handle known-length, unknown-length JSON, truncation, SSE, HEAD/204/304, mid-stream errors, close errors, and the drain safety valve. Concurrency is sound: sync.Once for finish and record, mutex for shared state, reference-counted cleanup for step counters.
"After eighteen rounds, the costume jewelry is gone. What's left is genuine." - Bisky
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e953aeece0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 20. Second consecutive clean round. Netero: clean. Panel (Mafuuu, Bisky, Pariston, Meruem): no findings.
42 findings across 20 rounds, all resolved. This PR is review-complete.
🤖 This review was automatically generated with Coder Agents.
Change-Id: Ibbc67a85ba78201c0778ccb5c8675b15e90b1cdf Signed-off-by: Thomas Kosiewski <tk@coder.com>
…#23915 - Skip JSON redaction for non-JSON content types (SSE, text/plain) to preserve debug content instead of replacing with diagnostic placeholder. Falls back to redaction for unknown/missing types. - Sanitize transport error strings by redacting embedded URLs that may contain credentials (userinfo, query parameters). - Add test cleanup to TestNextStepNumber_Concurrent for consistency. - Add tests for SSE read-to-EOF, SSE closed-early, text/plain preservation, and URL-containing transport error sanitization. Change-Id: I4f59a18e1fb10240fdd8fbb176ea953e08a9cd2a Signed-off-by: Thomas Kosiewski <tk@coder.com>
Store content type from resp.Header.Get() (case-insensitive) in recordingBody instead of doing case-sensitive map lookup on the redacted headers. Prevents non-canonical header casing from bypassing JSON redaction. Change-Id: I3c9721be18536fe421eee26982f005e2a922bede Signed-off-by: Thomas Kosiewski <tk@coder.com>
3 participants
Summary
Add the recording transport that wraps HTTP round-trips to capture LLM request/response pairs, the step recorder that manages debug step lifecycle, and the redaction layer that strips sensitive headers and truncates large payloads before persistence.
This is PR 3/9 in the chat debug logging stack.
Changes
coderd/x/chatd/chatdebug/transport.go):http.RoundTripperwrapper that snapshots outbound request headers/body and tees inbound response headers/body without breaking the provider SDK's stream consumption.coderd/x/chatd/chatdebug/recorder.go): manages the lifecycle of a debug step — creation, attempt tracking, normalized data attachment, and finalization.coderd/x/chatd/chatdebug/redaction.go):RedactHeadersfunction that masksauthorization,x-api-key,cookie, and other sensitive header patterns while explicitly preserving rate-limit headers (e.g.anthropic-ratelimit-*).coderd/x/chatd/chatdebug/redaction_test.go,transport_test.go): unit tests for redaction rules and transport capture behavior.Stack overview
Generated with
mux• Model:anthropic:claude-opus-4-6• Thinking:xhigh