Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped fix. Six allowlist entries for the six per-modality rate-limit headers, six test assertions matching 1:1. The fail-closed design (unknown "token" headers are still redacted) means the security posture is unaffected. Tests pass.
1 P3, 1 Nit, 1 P4. No blockers.
Bisky: "Each assertion would fail if you removed the corresponding allowlist entry because strings.Contains(lowerName, "token") at line 207 would catch it. The test proves behavior, not implementation. Lovely."
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Status: APPROVED
All R1 findings addressed. DEREM-1 (Priority Tier headers) fixed with both Priority Tier and fast-mode headers added. DEREM-2 (doc comment) fixed. DEREM-3 (enumerative vs. prefix allowlist) closed by panel vote: 5/5 accept the author's defense that fail-closed is the correct default for a credential filter.
1 new P4. No blockers.
P4 [DEREM-5] coderd/x/chatd/chatdebug/redaction_test.go:87 The "known safe rate limit headers containing token are not redacted" subtest manually enumerates all 24 token-containing headers. If someone adds a new entry to safeRateLimitHeaderNames but forgets the test assertion, the new entry ships untested. A data-driven loop over the map entries (filtering for those containing "token") would eliminate this class of gap. Not urgent since all 24 entries are currently covered 1:1. (Bisky)
Pariston: "I tried to build a case against this change and could not. The problem is correctly understood. The solution is proportional. The fix is at the right level."
🤖 This review was automatically generated with Coder Agents.
ThomasK33
force-pushed
the
fix/chatdebug-sse-record-on-eof
branch
from
6c896e5 to
efc7c88
Compare
ThomasK33
force-pushed
the
thomask33/fix/chatdebug-anthropic-modality-ratelimit
branch
from
1125b66 to
fc9325a
Compare
Merge activity
|
ThomasK33
changed the base branch from
fix/chatdebug-sse-record-on-eof
to
graphite-base/24592
… headers Anthropic returns input-tokens and output-tokens rate-limit headers alongside the existing tokens/requests variants. They fall through to the generic "token" substring check and get redacted as sensitive. Add them to the safe allowlist so the debug panel shows the limits. Change-Id: I303854c6c0f0297f84d2345d1de22be6d7f64cfb Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ken headers
Anthropic's Priority Tier (`anthropic-priority-{input,output}-tokens-*`) and fast-mode (`anthropic-fast-{input,output}-tokens-*`) response headers have the same false-redaction problem as the per-modality ratelimit headers: they contain "token" in the name and hit the generic credential filter.
Add the 12 header names to `safeRateLimitHeaderNames` and document the allowlist's purpose so future maintainers know where to add new entries.
Change-Id: I3f935aa8831490f7eadd1caee67340abc5054070
Signed-off-by: Thomas Kosiewski <tk@coder.com>
ThomasK33
force-pushed
the
thomask33/fix/chatdebug-anthropic-modality-ratelimit
branch
from
fc9325a to
640c868
Compare
ThomasK33
deleted the
thomask33/fix/chatdebug-anthropic-modality-ratelimit
branch
2 participants
Previously, Anthropic's per-modality, Priority Tier, and fast-mode rate-limit headers (
Anthropic-Ratelimit-Input-Tokens-*,Anthropic-Ratelimit-Output-Tokens-*,Anthropic-Priority-Input-Tokens-*,Anthropic-Priority-Output-Tokens-*,Anthropic-Fast-Input-Tokens-*, andAnthropic-Fast-Output-Tokens-*) were shown as[REDACTED]in the Debug panel because they contain"token"in the name and fell through the generic credential filter.Add them to the allowlist in
coderd/x/chatd/chatdebug/redaction.goalongside the existingAnthropic-Ratelimit-Tokens-*entries so the limits/remaining/reset values surface in the raw response view.