Skip to content

fix: accept a comma-delimited list in req.acceptsCharsets - #7451

Closed
marceli1404 wants to merge 1 commit into
expressjs:masterfrom
marceli1404:fix/accepts-charsets-comma-list
Closed

marceli1404 wants to merge 1 commit into
expressjs:masterfrom
marceli1404:fix/accepts-charsets-comma-list

Conversation

@marceli1404

Copy link
Copy Markdown

Bug

req.acceptsCharsets() silently ignores the documented comma-delimited form. The jsdoc (added by merged PR #6088) documents:

req.acceptsCharsets('utf-8, utf-16');
// => utf-8

...but the implementation passes the arguments straight through to accepts().charsets(...), which never splits commas. So a client following the documented API gets a hard false:

// with request header `Accept-Charset: iso-8859-1, utf-8`
req.acceptsCharsets('iso-8859-1, utf-8');   // => false   (bug; iso-8859-1 IS acceptable)
req.acceptsCharsets('iso-8859-1', 'utf-8'); // => iso-8859-1  (multi-arg works)

Fix

Expand comma-delimited args before passing them along, so req.acceptsCharsets('iso-8859-1, utf-8') behaves like the documented multi-arg form:

req.acceptsCharsets = function(...charsets) {
  const accept = accepts(this);
  return accept.charsets(...charsets.flatMap((charset) =>
    Array.isArray(charset) ? charset : String(charset).split(',').map((s) => s.trim())
  ));
};

Verification

  • New regression tests fail on the old code and pass on this change.
  • Existing behavior unchanged: no-arg (returns the acceptable list), single-arg, multi-arg, and array inputs are byte-identical.
  • npm test (mocha, full suite): 1262 passing, 0 failing.
  • eslint clean on the changed files.

@krzysdz

krzysdz commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

As far as I can tell, the docs are wrong. Comma delimited string has not been supported since cec0c06, according to #6936 (comment). You'll find more details and an attempt at fixing the docs in #7452.

@krzysdz krzysdz closed this Sep 2, 2026
stvwng added a commit to stvwng/cd14715-claude-code-classroom-1 that referenced this pull request Sep 6, 2026
Every report in reports/ is now produced by the current orchestrator, which no
longer bypasses SDK permissions. Previously the four expressjs/express reports
predated that change; only the octocat one had been refreshed by the verification
run.

This also serves as a broader check that removing permissionMode
'bypassPermissions' and allowDangerouslySkipPermissions did not degrade behaviour
across a range of pull requests. It did not: all four runs completed normally, each
spawned all three subagents through the Task tool, and no run stalled waiting on a
permission prompt.

  octocat/Hello-World#1     59/100   1 file   3 subagent runs
  expressjs/express#7453    88/100   1 file   3 subagent runs   clean fix
  expressjs/express#7451    82/100   2 files  6 subagent runs   fix with tests
  expressjs/express#7446    72/100   1 file   3 subagent runs   error handling
  expressjs/express#7435    65/100   3 files  9 subagent runs   multi-file feature

Scores shifted from the previous set (90/64/66/71) because agent output is
non-deterministic; the spread is normal run-to-run variance, not a behavioural
change. #7435 selected three files this time rather than five, which is within the
--max-files 5 cap. README and SUBMISSION_NOTES tables updated to match.

Verified across all 18 files: valid JSON, every finding carries a positive integer
line number, all three subagents present in every trace, HTML parses with balanced
tags, Markdown fences balanced, Mermaid trace embedded throughout.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AssyVR3HQdtvVAFZ8pRVP3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants