|
| 1 | +# Review: PR #274 header mutation forwarding |
| 2 | + |
| 3 | +Date: 2026-07-31 |
| 4 | +Reviewed: `proxy/server.mjs`, `proxy/upstream.mjs`, `test/proxy-server-header-propagation.test.mjs` at `4f2eb482083c2f722e28ad2b587612bc70ca6fe7` |
| 5 | +Round: 1 |
| 6 | +Label applied: approved-by-codex-agent |
| 7 | + |
| 8 | +## What Is Correct |
| 9 | +- `forwardRequest` only reads `clientReq.url`, `clientReq.headers`, and `clientReq.method` from its first argument, so the new `{ url, method, headers }` wrapper is sufficient and does not break the upstream transport contract. |
| 10 | +- Abort propagation and streaming behavior remain intact because the abort signal is still passed separately to `forwardRequest`, and streaming still operates on the returned `upstreamRes`; the wrapped request object is not part of that path. |
| 11 | +- `content-length` recomputation still works: `forwardRequest` rebuilds upstream headers, then overwrites `content-length` from the serialized `body`, so extension-mutated `content-length` values do not leak through. |
| 12 | +- Leaving `handlePassthrough` unchanged is correct. That path intentionally does not run the extension pipeline, so there is no mutated header state to preserve there. |
| 13 | +- The new regression test is a real wire-path test, not an intermediate-state assertion. It starts a real proxy, runs the real extension pipeline, forwards to a local HTTP upstream, and asserts on the headers the upstream actually received. |
| 14 | +- The author's “fails 4/5 on unfixed main” claim is accurate. Running `test/proxy-server-header-propagation.test.mjs` against the merge-base (`76d586d48255aa5eb5b3ff5aa7ced5baeff66640`) produced exactly 1 pass / 4 failures; on the PR head it produced 5 passes / 0 failures. |
| 15 | +- Full suite result on the PR head was clean: `1435` tests passed, `0` failed. |
| 16 | + |
| 17 | +## Blockers |
| 18 | +None. |
| 19 | + |
| 20 | +## What Needs Attention |
| 21 | +- Coverage is strongest on `/v1/messages`; the identical bootstrap wrapper path is changed but not directly exercised by a new bootstrap-specific regression. I do not consider that a blocker because the wrapper contract was independently verified against `proxy/upstream.mjs`, but it remains the only changed route without a direct test in this PR. |
| 22 | + |
| 23 | +## Bloat / Non-Functional |
| 24 | +None. |
| 25 | + |
| 26 | +## Recommendations |
| 27 | +- A follow-up bootstrap-focused regression test would close the remaining route-coverage gap by proving header mutations also reach the wire on `/api/claude_cli/bootstrap`. |
| 28 | +- Keep relying on `buildUpstreamHeaders()` as the sanitization boundary. This change intentionally allows trusted extensions to mutate end-to-end headers such as `authorization`, but `host`, hop-by-hop headers, `proxy-*`, and effective `content-length` are still normalized there. |
| 29 | + |
| 30 | +## Bottom Line |
| 31 | +The fix is correct and complete for the defect claimed in this PR. The request wrapper matches the actual `forwardRequest` contract, it preserves abort and streaming behavior, it keeps `content-length` authoritative, and the new regression test proves the bug on the old code and the fix on the new code at the wire level. This is safe to approve. — Codex review |
0 commit comments