Repository navigation
fix(headers): preserve mixed-case iterable values - #11347
Plastic-time wants to merge 1 commit into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
 [Medium impact] No blocking defects were found. The change preserves existing rewrite controls and prototype protection while correcting case-insensitive iterable aggregation. Summary
Plastic-time explicitly acknowledged that Firefox checks remain blocked and checks using the latest lockfile are still in progress. The PR remains a draft; those results are not claimed as complete. No blocking issues were found in the reviewed change. The focused results support merging this correction, subject to the project's remaining required checks. Reviews (1) · Last reviewed commit: "fix(headers): preserve mixed-case iterab..." · Reviewed by Greptile |
jasonsaayman
added
ai-assisted
jasonsaayman
left a comment
jasonsaayman
left a comment
There was a problem hiding this comment.
PR #11347 - fix(headers): preserve mixed-case iterable values
Repo: axios/axios Author: @Plastic-time Base: v1.x Files: 4 **+58/-0
CI: 4 success; no build/test workflow result is present in this snapshot. Mergeability: mergeable. Reviewed head: f8463a3684118df1f95c91c3cca348232663923c.
Repo Profile
Axios is an npm JavaScript HTTP client for browsers and Node, with XHR/Fetch/HTTP adapters, default and named exports, interceptors, headers/errors, and paired ESM/CJS declarations. The target uses Vitest plus browser and package/module tests; v1 packaging covers older Node CJS runtimes as well as current ESM, Bun and Deno. Contributions preserve transport/security boundaries, add regression coverage and prerelease records, and defer unreleased user docs to PRE_RELEASE_DOCS.md. This review uses the actual target-branch guidance and files, not the separate local v0 checkout.
Verdict
Request changes — No concrete source defect found in this review; the missing latest-head validation prevent merge approval.
Posted as a comment review for the validation/integration hold; no formal code-change request is implied beyond the specific items below.
Change Explanation And User Impact
AxiosHeaders accepts iterable name/value pairs, but its temporary accumulator previously grouped only exact spellings. Passing [["X-Item", "first"], ["x-item", "second"] let later normalization overwrite the first value. The new lookup finds an existing name case-insensitively before accumulating another value, so the normal setter sees one grouped header. Executed example: the current target produced {"X-Item":"second"}; this head produced {"X-Item":["first","second"]}. This affects iterable input, including Map/Set-derived pairs, rather than changing the ordinary object or rewrite policy.
Validation scope: source/diff and regression-test review. Examples explicitly marked executed used isolated source snapshots on Node 24.18.0 with the workspace dependency installation; these were focused checks, not a clean install, full CI matrix, or full test-suite run. Author-reported test results were not treated as independently executed.
Competing PRs
Search: axios/axios open PRs, including drafts; full open-list scan plus AxiosHeaders in:title,body, headers iterable in:title,body. Complementary searches were run across related APIs/issues; no query reached the 100-result cap and no search failed. The five explicitly excluded PRs were not reviewed or modified in this batch.
#11347 accumulates iterable names case-insensitively; #11248 fixes matcher/rewrite callbacks; #11288 narrows declarations. Shared files do not make these the same solution; the type narrowing has its own compatibility blocker.
Preferred PR: No competing implementation supersedes this PR; continue it independently.
Unique contributions: The submitted regression cases protect this specific boundary; preserve them independently of adjacent fixes.
Action: No duplicate closure: the candidates are complementary or no equivalent solution was found.
Project Fit
North star: Reliable HTTP transport and configuration across supported JavaScript environments, without embedding application-specific policy.
Benefits: Preserves all iterable header values across casing differences.
Tradeoffs: Case-insensitive searches add work while accumulating distinct names; this follows the existing header lookup strategy and adds no cache invalidation state.
Fit: Strong — The underlying problem belongs in the transport, serializer, declarations or their maintenance tests.
Adversarial Review
Strongest objection: Case-insensitive searches add work while accumulating distinct names; this follows the existing header lookup strategy and adds no cache invalidation state. Latest-head integration evidence is still needed.
Abuse/failure modes checked: Reviewed arrays, Maps, Sets, insertion order, mixed casing, rewrite true/false, special names and the browser regression. Verified the public AxiosHeaders example.
Result: No concrete source defect established in the examined cases; validation and integration limits remain below.
Disposition
Continue
Address the specific blockers below; no unrelated scope or complementary PR should be closed.
Semver
patch — Corrects existing behavior or strengthens internal tooling/tests without adding a public API.
Breaking Changes
None intended beyond the bug correction described above; no public API removal or new runtime requirement is introduced by this diff.
Documentation
PRE_RELEASE_CHANGELOG.md records the change. No new API reference or migration is required for this scoped correction; keep the release description consistent with the behavior and limits above.
Performance And Regression Risk
Hot path touched: Yes — request/response processing or its shared helpers.
Performance concern: No unbounded new work identified in the changed path. Case-insensitive searches add work while accumulating distinct names; this follows the existing header lookup strategy and adds no cache invalidation state.
Regression risk: low — the changed boundary needs regression and integration coverage, particularly after any rebase.
Evidence requested: The latest-head checks listed below; no extra benchmark requested for this scoped change.
Blocking Issues
- Latest-head build/test validation is unavailable in the returned GitHub checks. Run or attach the repository build, relevant regression suite and required package/type/browser checks before merge; review/security bots alone do not establish that they pass. This is a validation hold, not an invented source defect.
Non-Blocking Suggestions
- Consider a representative many-header measurement if this path becomes a performance target; no additional blocking source change found.
Looks Good
- Reviewed arrays, Maps, Sets, insertion order, mixed casing, rewrite true/false, special names and the browser regression. Verified the public AxiosHeaders example.
|
@jasonsaayman Following up on your validation review: I reran validation on the unchanged latest PR head f8463a3 on 2026-10-21. No source changes or rebase were made. Current target-branch contributor guidance was rechecked and is unchanged. Environment: Windows x64; Node 24.20.0, npm 11.19.0, Vitest 4.1.11, Playwright 1.64.0; Chromium 156.0.8078.4 (1248), Firefox 157.0 (1555), WebKit 27.2 (2370). This reuses the installation of this head's exact lockfile whose
The extra packed-consumer check used the actual unpacked tarball with runtime dependencies linked to the existing locked installation. An attempted standalone offline npm install lacked cached Browser results and limitations: An initial all-engine rerun had a Firefox connection timeout and a Chromium async-interceptor wait timeout followed by one affected subsequent assertion; I retained those failures rather than treating that incomplete run as passing. After build/unit work finished, I ran the full suite separately for each engine, with original assertions unchanged and a validation-only configuration using
Each run used Applicability: Under AGENTS.md's submission requirement 4, declaration/module tests are required for public type changes, and the package/module/smoke matrix is required for packaging/exports/build changes. This diff changes none of those, so those full matrices are not required local checks for this patch. I added the focused tarball checks above as extra runtime-export evidence. I have not run the full Node 12–26, Bun/Deno or Ubuntu CI matrix locally; Node 24 Windows results do not substitute for it. GitHub CI remains separate: Continuous integration, build reproducibility, Bundle Size and zizmor still report I also corrected only the stale Draft-status and missing-label statements in the description: the PR is already non-Draft and |
Summary
AxiosHeaders.set([['X-Item', 'first'], ['x-item', 'second']])losesfirst, although identical spellings accumulate both values. Resolve a string entry name against the temporary collection before aggregating values, preserving case-insensitive header identity, input value order and the first spelling.Linked issue
No linked issue. This is a bounded correction to existing iterable aggregation with a direct failing reproduction; no new API or architectural change. Reviewed actual overlapping diffs: #7456 normalizes CRLF, #11177 guards fetch iterator lookup, #11248 handles rewrite matchers, and #11288 changes type signatures. None fixes aggregation within a single iterable. Latest v1.x and release PR #11346 were also checked before submission.
Changes
Validation
Node 24.20.0 on Windows. The direct reproduction fails on the clean baseline and passes with this fix. Independent final-diff review passed 14 counterexample groups covering malformed entries, custom/frozen iterables, native Headers, disabled/rewrite behavior, input mutation and prototype pollution. A separate reviewer also checked the public browser regression.
Latest-base validation uses fceeb10 (the upstream update changes only package-lock.json); the reviewed production/test hashes remain identical. All checks below were rerun after installing that exact lockfile:
npm ci --ignore-scripts --offline: pass. Newly required official registry tarballs were checked against lockfile SHA-512 integrity and cached after the direct npm fetch stalled. Optional npm audit was disabled only for this local installation.npm run lint: pass.npm run build: pass.npm run test:vitest:unit: 75 files, 1,707 tests pass.npm run test:vitest:browser:headless: all three engines execute; 1,061 pass, 8 skip, 2 failures in the existing Date Link test (Chromium/WebKit). On this Windows locale, Date.toString includes a non-ASCII timezone label that the header normalization removes. The unchanged-source baseline with the same updated dependencies reproduces those exact two failures (61 other tests in that file pass).{locale: 'en-US', timezoneId: 'UTC'}and IPv4 test host: 81 files pass, 1,063 tests pass, 8 skip. Repository test configuration is unchanged. The earlier Firefox connection timeout no longer occurs with the updated Playwright/browser versions.Public declaration/module tests are not applicable because there are no API or declaration changes. Packed-package smoke tests are not applicable because packaging, exports and build configuration are unchanged. Header aggregation is synchronous and owns no request, listener or cancellation resources; existing adapter/cancellation suites are included in the full unit run.
For commit f8463a3, Continuous integration, build reproducibility, Bundle Size, and zizmor are action_required, awaiting maintainer approval. They have not executed and are not reported as passing tests. Automated review/security checks are separate from runtime CI. This PR is currently open for review; runtime CI has not executed.
Compatibility and documentation
Patch-level bug fix for Node and browser consumers. No intentional breaking behavior, API, export or type changes. Ordinary object replacement and existing-header rewrite controls are preserved. README already documents case-insensitive header names and first spelling, so no deferred release docs or ESM/CJS declaration updates are needed. The prerelease changelog records the correction.
AI assisted the implementation and description. A separate agent reviewed the final diff and ran counterexamples; no human review is claimed. The existing ai-assisted label is now applied.
Checklist
PRE_RELEASE_CHANGELOG.mdentry added