[bartlomieju]

NODE-6: MIMEType and MIMEParams are now in TypeScript, reproducing the C++ parser and every probed result; the tests from the previous commit pass unchanged. The implementation calls no built-in methods after loading, so patched built-ins can't change its results. As in Node.js the classes are plain, so methods aren't enumerable and structuredClone() returns {} instead of throwing.

The tests pin the behavior of the C++ implementation and pass against both implementations; one more test patches built-ins.

The differences below change at every compatibility date, without a compatibility flag, and are accepted (see the commit message). immutable_api_prototypes only makes the constructor's prototype property read-only (V8's ReadOnlyPrototype()), which a class's prototype already is, so it has no effect on the difference in that row.

Differences from the C++ implementation

Probed at compat dates 2023-01-01 and 2026-10-01. Parsing (71 cases), argument conversion, the setters and the params operations otherwise give identical results.

Case	C++	TypeScript
Calling MIMEType() / MIMEParams() without new	TypeError: Failed to construct 'MIMEType': Please use the 'new' operator, ...	TypeError: Class constructor MIMEType cannot be invoked without 'new'
Assigning to essence / params	TypeError: Cannot assign to read only property 'essence' ...	TypeError: Cannot set property essence of ... which has only a getter
structuredClone() of either class	Throws DataCloneError	Returns {} (as in Node.js)
Prototype methods and accessors	Enumerable	Not enumerable (as in Node.js)
Function.prototype.toString()	function MIMEType() { [native code] }	Class source
prototype property of the constructor	Writable	Not writable
Iterator prototype key order	next, constructor, ...	constructor, next, ...

Patched built-ins: at old compat dates the C++ iterators were affected by a value setter on Object.prototype. The TypeScript implementation isn't affected at any compat date.

Performance

Medians of 3 runs of workerd bench, --config=benchmark, same machine.

Case	C++	TS	TS/C++
parse text/plain	190 ns	273 ns	1.44×
parse with 2 params	342 ns	1.27 µs	3.71×
parse with 20 params	2.13 µs	4.66 µs	2.19×
parse invalid (throws)	11.4 µs	744 ns	0.07×
toString(), 2 params	88 ns	312 ns	3.54×
toString(), 20 params	502 ns	1.35 µs	2.69×
essence	42 ns	13 ns	0.31×
params.get()	82 ns	49 ns	0.60×
params.set() + delete()	187 ns	56 ns	0.30×
iterate 20 params	3.25 µs	425 ns	0.13×
type setter	44 ns	45 ns	1.01×

[clanky-the-clanker]

💬 Approved with comments — The TypeScript port matches the C++ parser and is well tested. The only notes are informational: observable differences that the commit message explicitly accepts, and slower parse and toString() on some inputs.

Findings: 2 info

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #7968.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## INFO (2)

### [Code Quality] MIMEType/MIMEParams behavior changes (enumerability, structuredClone, read-only prototype) apply at every compat date without a flag. The commit message records the accepted risk.
- **File:** src/node/internal/internal_mime.ts

### [Performance] Building strings one character at a time makes parse and toString() 1.4–3.7x slower than the C++ version. Optional fast paths: skip replaceLoneSurrogates/asciiLowercase when there is nothing to change.
- **File:** src/node/internal/internal_mime.ts

---

📋 Code Quality: 💬 1 info

The port looks correct and is well tested. One informational note about behavior changes that the commit message accepts.

Recommendation: 💬 Optional Nits

• [INFO] MIMEType/MIMEParams behavior changes (enumerability, structuredClone, read-only prototype) apply at every compat date without a flag. The commit message records the accepted risk. (src/node/internal/internal_mime.ts)

View full review

I compared the parser, setters, params operations, iterators and error messages against the removed C++ code and found no logic errors. src/node/BUILD.bazel uses a glob for internal modules, so the new internal_mime.ts is picked up without further changes. Nothing under types/ refers to the removed C++ MIMEType.

• [INFO] src/node/internal/internal_mime.ts: MIMEType and MIMEParams now behave differently in a few observable ways, at every compatibility date and without a compatibility flag:

  • methods are not enumerable
  • structuredClone() returns {} instead of throwing
  • the constructor's prototype property is read-only
  • error messages and Function.prototype.toString() output differ

  The commit message records that this risk is accepted, which is what the repository's backward-compatibility policy requires. I'm noting it for visibility only. The enumerability and structuredClone() changes are the ones most likely to affect code that relied on the old behavior.

---

⚡ Performance: 💬 1 info

Linear-time and no leaks. Parse and toString() are 1.4–3.7x slower on some inputs.

Recommendation: 💬 Optional Nits

• [INFO] Building strings one character at a time makes parse and toString() 1.4–3.7x slower than the C++ version. Optional fast paths: skip replaceLoneSurrogates/asciiLowercase when there is nothing to change. (src/node/internal/internal_mime.ts)

View full review

• [INFO] src/node/internal/internal_mime.ts: the helpers below build strings one character at a time with result += str[i]:

  • substring()
  • asciiLowercase()
  • replaceLoneSurrogates()
  • the quoted-string loops

  This matches the slowdowns in the pull request's own benchmarks: about 3.7x for parsing with 2 params and about 3.5x for toString(). Absolute times stay small, and iteration, invalid input and accessors got faster, so this isn't a regression that blocks the change. If MIMEType ever becomes hot, two fast paths would help:

  • return the input unchanged when replaceLoneSurrogates has nothing to replace
  • skip asciiLowercase when the input has no uppercase characters

---

✅ All other checks passed

🔒 Security: ✅ LGTM

• Prototype pollution: internal maps and lookup tables are null-prototype objects, so keys like __proto__ and constructor are stored as plain own properties.
• Brand checks: private-field brand checks (#mime in obj) reject a spoofed this.
• Parser: linear and bounded, with no regexes and no dynamic evaluation.
• Patched built-ins: a new test covers this. The implementation calls no built-in methods after loading, so patched built-ins can't change its results.

The C++ changes only delete code.

---

📖 AGENTS.md: ✅ LGTM

No AGENTS.md updates needed.

---

📝 Documentation: ✅ LGTM

No documentation issues.

---

_{Reviewed commit: c2d60992}

[ask-bonk]

Since last review: 2 resolved, 0 still open, 1 new.
LGTM!

Updates MIME parameter deletion cleanup and the patched-built-ins implementation coverage.

Other findings

• [INFO] src/node/internal/internal_mime.ts:15-17: The template-literal coercion in toRuntimeString() still observes user-visible conversion hooks. For example, after replacing Object.prototype.toString, new MIMEType({}) parses that replacement's result, so patched built-ins can affect non-string arguments despite this guarantee.

  // Parsing, serialization and the parameter-table operations use only language syntax: string
  // indexing, `length`, operators, and objects with a null prototype. They call no built-in methods
  // or read globals, so patching built-ins after module evaluation cannot affect string inputs or
  // their internal work. Coercion of non-string API arguments still observes conversion hooks.
  

Not re-run: jsg-gc, kj-style, memory-safety (no author changes in their files since the last review)

---

_{Reviewed commit: c2d60992 · github run}

[auditbenchai]

Findings: 1, listed in the fix prompt below (they couldn't be placed as inline comments).

Merge risk: 🟢 Low · up to f9b3e28

Minor edge-case discrepancies such as error messages, prototype enumerability, or slight performance differences in parameter-heavy parsing could affect strict callers, though behavior is well-tested.

Merge readiness: ⛔ Not ready to merge

Pre-merge checks: ✅ 8 · ⚠️ 1 · ❌ 1

❌ Failed checks: 1 error, 1 warning

Check name	Status	Explanation	Resolution
CI checks	❌	7 of 30 failing: test (linux-arm, ubuntu-22.04-arm-16core) / bazel, test (linux, ubuntu-22.04-16core, -asan) / bazel, test-linux / bazel, test (macOS, macos-15-xlarge) / bazel….	Open the failing runs on GitHub and fix them before merging.
PR size	⚠️	1,655 lines changed, which is hard to review well.	Split it into smaller pull requests if you can.

✅ Passed checks: 8 passed

Check name	Status	Explanation
Merge conflicts	✅	Merges cleanly into main.
Secrets	✅	No keys, passwords or tokens in the changed code.
Blocking issues	✅	No critical or high-severity issues in the changed lines.
Tests	✅	Tests were changed alongside the code.
Title check	✅	The title clearly and concisely states the primary change made in the PR.
Description check	✅	The description thoroughly details the motivation, differences from C++, benchmark results, and testing strategy.
Linked issues check	✅	Mentions internal tracker NODE-6; no external GitHub issues are formally linked.
Out of scope changes check	✅	All changes are directly related to moving MIMEType and MIMEParams to TypeScript.

📝 Walkthrough

This pull request replaces the C++ implementation of node:util's MIMEType and MIMEParams with a TypeScript implementation. The new version is designed to match existing runtime behavior and WHATWG MIME Sniffing standards while avoiding built-in method calls to remain resilient against prototype pollution. It also updates internal type definitions and adds comprehensive tests covering behavior and patched built-ins.

File	Changes
src/node/internal/internal_mime.ts	Implements MIMEType and MIMEParams parsing, storage, and serialization in pure TypeScript.
src/node/internal/util.d.ts	Updates TypeScript declaration types for node:util internals.
src/node/util.ts	Exports the new TypeScript implementation of MIMEType and MIMEParams.
src/workerd/api/node/tests/mimetype-test.js	Adds tests verifying compatibility, parsing edge cases, and resilience to patched built-ins.

ℹ️ Review info

• Commit: f9b3e28
• Verdict: ✅ Pass
• Findings: 1 low
• Review: local checks plus AI review of the changed lines
• Scope: Reviewed 3/4 files (of 4 total) — 0 from cache, 0 needed no AI review. Found 1 finding(s), 0 potential secret(s). 1 file(s) couldn't be reviewed (src/node/internal/util.d.ts): the AI provider returned an error.

🤖 Prompt to fix review comments

Verify each finding against the current code and only fix it if it still applies.

In `@src/node/util.ts` around line 343: High cyclomatic complexity: styleText(). Complexity of 17 makes this function harder to test and reason about. Suggested fix: Split into smaller functions with single responsibilities.

💬 Reply to any inline comment and Audit Bench Ai will recheck that code and answer in the thread. Mention @auditbench in the conversation to ask about the whole PR.

🛠️ Open the full report and fix with AI

Thanks for using Audit Bench Ai! Free pull request reviews for open source: paste any public PR at auditbenchai.com/scan.

_{Reviewed by Audit Bench Ai · AI code review for security, logic and performance · Review any public PR free}

[piscisaureus]

Direction is fine.

Before merging:

• Fix the eslint failure (mimetype-test@eslint).
• Say in the commit message that the enumerability, structuredClone and immutable_api_prototypes changes are accepted without a compat flag.
• Actually delete stale keys in ParamTable.delete(); right now a long-lived MIMEParams that sets and deletes distinct names keeps growing.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 748 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.90%. Comparing base (2c37de0) to head (c2d6099).
⚠️ Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
src/workerd/api/node/tests/mimetype-test.js	0.00%	748 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7968      +/-   ##
==========================================
- Coverage   42.05%   41.90%   -0.16%     
==========================================
  Files        1061     1062       +1     
  Lines      301008   302159    +1151     
  Branches    29043    29026      -17     
==========================================
+ Hits       126599   126616      +17     
- Misses     157389   158537    +1148     
+ Partials    17020    17006      -14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bartlomieju]

@jasnell just to confirm - is this one fine to merge without an autogate, or should I add one?

[jasnell]

This should be fine with no autogate