[andre-j3sus]

The default config pointed packet loss at a TURN server that no longer exists (turn.speed.cloudflare.com:50000) and a credentials endpoint that only Cloudflare's own origins can use. As a result, the default packetLoss measurement always failed for third parties.

• packetLoss is removed from the default measurements, and turnServerUri / turnServerCredsApiUrl now default to null. speed.cloudflare.com sets its own measurements and TURN config, so it isn't affected.
• If PacketLossEngine rejects the TURN config, the error is caught and reported through onError (after construction, so it isn't lost with autoStart), and the run moves on to the next measurement. Before, the constructor threw from inside play(), which would stop the test once the defaults were gone. The engine also fails straight away, rather than after the ICE timeout, when neither turnServerUri nor the credentials API provides a server. Failed packet loss measurements are now marked finished, so results.isFinished is accurate after an error.
• AIM scores: when no packetLoss measurement is configured, packet loss is left out of the scores. A failed measurement still counts as 0 points (Set 0 points to packet loss AIM score in case of ICE connection error #20).
• README: packet loss is opt-in and needs your own TURN server, or turn.cloudflare.com:3478 with your own credentials.

Default results change, so this should ship in a minor release.

[ask-bonk]

LGTM!

github run

[clanky-the-clanker]

💬 Approved with comments — The packet loss opt-in, the early failure on a missing TURN server, setup errors reported through onError, and leaving packet loss out of AIM scores are all correct and tested. The only comment is an optional README note about how the option types are written.

Findings: 1 suggestion

_{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 #167.

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

## SUGGESTION (1)

### [Documentation] README lists turnServerUri / turnServerCredsApiUrl as string, but they are string | null with a null default. Optional, since the table uses the same style for other nullable options.
- **File:** README.md:49

---

📝 Documentation: 💬 1 suggestion

Optional: the README lists the TURN options as string, but they also accept null.

Recommendation: 💬 Optional Nits

• [SUGGESTION] README lists turnServerUri / turnServerCredsApiUrl as string, but they are string | null with a null default. Optional, since the table uses the same style for other nullable options. (README.md:49)

View full review

• README.md:49-50 lists turnServerUri and turnServerCredsApiUrl as string, but their type is string | null (src/config/defaultConfig.ts:60-62) and their documented default is now null. Other options in the table, such as authorizationToken, are written the same way, so this follows the README's current style. It is optional.

---

✅ All other checks passed

🚀 Release: ✅ LGTM

Risk: Low. The default results change, so this should ship in a minor release, as the description says.

---

🔒 Security: ✅ LGTM

No security concerns. The change removes the default TURN endpoints and does not add any new network destinations or new handling of credentials.

---

📖 AGENTS.md: ✅ LGTM

This change follows the repository guidelines.

---

📋 Code Quality: ✅ LGTM

I found no correctness, error-handling or test-coverage problems. I checked these paths:

• Setup errors (src/index.ts:464-478): when the TURN config is rejected, the measurement result is marked started, finished and errored in a microtask.
  • The microtask does nothing if the run was restarted in the meantime.
  • It only moves on to the next measurement while the engine is running, so pausing still works.
  • The tests cover restart, pause and autoStart.
• Credentials and connection failures (src/index.ts:495-511): both handlers now set finished = true, so results.isFinished is accurate after a failure.
• Engine (PacketLossEngine.ts:115-121): when neither turnServerUri nor the credentials API provides a server, the engine fails straight away instead of waiting for the ICE timeout.
• Scores: packet loss is left out only when no packetLoss measurement is configured. A configured measurement that fails still scores 0 points, as the description says.

---

⚡ Performance: ✅ LGTM

No performance concerns. The change removes a default measurement, adds early-failure paths and filters a few array entries when calculating scores. None of these is on a hot path.

---

_{Reviewed commit: 8e5a2e60}

[andre-j3sus]

AI Code Reviewer finding:

README lists turnServerUri / turnServerCredsApiUrl as string, but they are typed string | null and now default to null.

Local Agent verdict:

Not changing. The README config table lists nullable options as plain string and shows null in the Default column (e.g. authorizationToken, turnServerUser, turnServerPass). Changing only these two rows would make the table inconsistent, and the null default already tells readers the value can be null.

AI Code Reviewer finding:

getScores() note says that without packetLoss "scores come out lower". A connection with 25% loss or more would score -10/-20 instead, so leaving the measurement out can raise its score. Say "lower than a measured low-loss result".

Local Agent verdict:

Valid. Reworded to "scores come out lower than for a connection measured with under 5% loss", which matches the scoring thresholds. Fixed in c4445f5.

[andre-j3sus]

AI Code Reviewer finding:

The new TURN-config guard is tested only with every TURN option null. Add cases for a URI with no credentials (should error) and for a credentials API URL (should build the engine).

Local Agent verdict:

Valid. Added three cases: a URI with no credentials and a URI with only a username both report "no turn server configured" and the run continues. With a credentials API URL, the engine is built and requests credentials from it; the stubbed response has none, so the error is the engine's own credentials failure, not the guard's. A URI with full static credentials is not covered, because it goes straight to RTCPeerConnection, which is unavailable in the Node unit tests. Fixed in 8c65add.

[andre-j3sus]

AI Code Reviewer finding:

README config table lists turnServerUri/turnServerCredsApiUrl as string, though they now default to null (string | null); consider noting the nullable type.

Local Agent verdict:

Not changing. Same finding as the previous round: the README table writes nullable options as plain string with null in the Default column (see #167 (comment)).

AI Code Reviewer finding:

getScores() note says "5–25% loss" scores 0 points; the threshold is 5% to under 25% (exactly 25% scores -10).

Local Agent verdict:

Valid. scaleThreshold moves to the next band when the value reaches a threshold, so exactly 25% scores -10. Reworded to "5% to under 25% loss". Fixed in 7c9b21f.

[andre-j3sus]

AI Code Reviewer finding:

Local-dev TURN instructions set only turnServerCredsApiUrl. The worker doesn't return a server field and turnServerUri now defaults to null, so the engine connects to turn:undefined and packet loss fails. Add turnServerUri to this line.

Local Agent verdict:

Valid. The line now also says to keep turnServerUri set to turn.cloudflare.com:3478, so it works on its own. Fixed in 59643ad.

AI Code Reviewer finding:

The README options table still lists turnServerUri and turnServerCredsApiUrl as string, but they are now string | null with a default of null.

Local Agent verdict:

Not changing; this repeats an earlier finding. The table writes nullable options as plain string with null in the Default column (see #167 (comment)).

[andre-j3sus]

AI Code Reviewer finding:

Deferred PacketLossEngine error callback calls this.#next() even if pause() ran in the same tick, so the run moves on while paused and the next play() skips a measurement. Consider this.#running && this.#next() as at line 582.

Local Agent verdict:

Valid. One correction: the effect is that the next measurement starts while paused, rather than being skipped. An upload is pausable, so the next play() resumes it. The deferred step now uses this.#running && this.#next(), matching the bandwidth handlers, and a test checks that no upload request is made while paused and that it runs once after play(). Fixed in 8e5a2e6. The other packet loss handlers (onFinished, onConnectionError, onCredentialsFailure) also call #next() without checking #running; that predates this PR and is left out of scope.