Repository navigation
Conversation
Measure download and upload speed, latency, and jitter between this machine and Cloudflare's network with `cf speedtest` (alias `cf speed-test`). It times requests to speed.cloudflare.com's public `__down` and `__up` endpoints using the method of `@cloudflare/speedtest`: server time is subtracted from time to first byte, latency is the median and jitter the mean consecutive difference of 20 pings, and bandwidth is the 90th percentile of growing request sets, which stop once a whole set takes over a second. The library itself expects a browser, and its WebRTC packet-loss test cannot run in Node, so packet loss is omitted. In a terminal each reading updates live with a sparkline of its samples; stdout carries only the JSON result. No Cloudflare login is required. To support this, `withProgress` now passes its task an `update` callback that replaces the spinner message, leaving the last update on screen when the task succeeds, and root hand-written commands can declare yargs aliases, which their `meta.ts` sidecars also list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Hi @olipayne. Thanks for contributing to During the open beta, we’re being very intentional about the changes we accept so that we can prioritise stability for GA. We’re limiting pull requests to contributions coordinated with the team in advance, so this PR has been automatically closed. We’d still love to hear about your use case, so please make sure it’s captured in a |
|
💬 Approved with comments — Clean, well-tested hand-written root with no blocking or warning-level issues. Two optional suggestions: a cosmetic sparkline glitch on non-finite samples, and AGENTS.md inventory lists that don't include the new command yet. Findings: 2 suggestions The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📖 AGENTS.md: 💬 1 suggestionAGENTS.md describes the new command but doesn't add it to the existing hand-written command location lists. Recommendation: 💬 Optional Nits
View full review
📋 Code Quality: 💬 1 suggestionOne cosmetic robustness nit in the sparkline helper. The rest of the change is correct and covered by tests. Recommendation: 💬 Optional Nits
View full review
Overall, the measurement maths (percentile interpolation, jitter, ✅ All other checks passed🚀 Release: ✅ LGTMRisk: Low. The changeset is present and its bump type fits a new command. The rate-limit open question in the description (speed.cloudflare.com returns 429 to unauthenticated clients after repeated runs, and there is no 🔒 Security: ✅ LGTMRequests go only to the fixed ⚡ Performance: ✅ LGTMThe command is loaded lazily as a root, so it doesn't affect startup. Download bodies are streamed and counted rather than buffered, and each upload set allocates one buffer that all its requests reuse. No issues found. 📝 Documentation: ✅ LGTMThe command help, epilogue, changeset, and AGENTS.md description match the behaviour. Reviewed commit: |
|
clanker review |
commit: |
petebacondarwin
left a comment
There was a problem hiding this comment.
Do we actually need the speed-test alias? Can we just use speedtest as this would simplify the PR and remove invasive changes to the general mechanics.
I see in other chats the idea of having speedtest run. We should make that decision before landing this.
In the PR description there is no handling for Retry-After responses. I think we should support this on first landing.
I would actually propose to go further... Would it be possible to make some small changes to the @cloudflare/speed-test package so that it was Node.js/CLI compatible? This would avoid having to add a lot of (duplicated) logic for the speed test client code in cf. (I would really not have cf own that code, and have to keep it maintained and/or in sync with the library).
| rootHandWrittenCommands().map(rootCommandName) | ||
| rootHandWrittenCommands().flatMap((command) => [ | ||
| rootCommandName(command), | ||
| ...(command.aliases ?? []), |
There was a problem hiding this comment.
Do we want to run each of the aliases through rootCommandName too?
There was a problem hiding this comment.
Perhaps we can bundle the alias handling into rootCommandName():
Something like:
export function rootCommandNames(command: RootHandWrittenCommand): string[] {
return [command.command, ...command.aliases].map(command => command.split(/\s+/)[0] ?? command);
}| command.load as LazyCommandImporter, | ||
| command.telemetry | ||
| ), | ||
| aliases: command.aliases, |
There was a problem hiding this comment.
Could we move the alias handling into the lazyCommand function?
There was a problem hiding this comment.
Perhaps change lazyCommand so that instead of taking command: string it takes command: string|string[] and treat the extra items as aliases?
There was a problem hiding this comment.
So that would give:
lazyCommand(
[command.command, ...command.aliases ?? []],
...
Summary
Adds
cf speedtest(aliascf speed-test): measure download and upload speed, latency, and jitter between this machine and Cloudflare's network, straight from the CLI. No Cloudflare login is required.In a terminal, each reading updates live on stderr with a sparkline of its samples and stays on screen once measured. stdout carries only the JSON result, so scripts and agents get the same output as every other
cfcommand:How it measures
It times requests to speed.cloudflare.com's public
__downand__upendpoints, following the method of@cloudflare/speedtest:__down?bytes=0requests after one warm-up request that opens the connection. Server time fromServer-Timing(cfReqDur, falling back to thecfSpeed*sum) is subtracted from time to first byte. Latency is the median; jitter is the mean difference between consecutive pings.cf-ray.The library is not used directly because it expects a browser: it reads Resource Timing entries (which Node's
fetchdoes not create) andwindow.location, logs every measurement withconsole.log(which would land on stdout), and its packet-loss test needs WebRTC. Packet loss and loaded latency are therefore not measured.Implementation notes
cf speedtestis a hand-written root, likecf docs search: there is no platform API operation behind it. Code is insrc/commands/speedtest/, registered inhand-written.ts,root-commands.ts, andhand-written-meta.ts, and documented in AGENTS.md.RootHandWrittenCommandgains an optionalaliasesfield, whichsrc/index.tspasses to yargs and also uses so--localis still rejected under an alias. Themeta.tssidecar lists the alias too, ascf access tcpdoes for its leaf aliases.withProgressnow passes its task an optionalupdate(message)callback that replaces the spinner message; the last update stays on screen when the task succeeds. Existing callers are unchanged, and updates are ignored when the spinner isn't drawn (non-TTY, CI,CF_QUIET=1).Open questions
__downrequests from my IP with HTTP 429, andcf speedtestcurrently fails with a rate-limit message when that happens. The website sends a Turnstile-backedAuthorizationtoken (cfspeedAuthEnabled) and the CLI can't. I couldn't tell whether unauthenticated clients get lower limits, so this probably needs a conversation with the speed.cloudflare.com owners before release. The library retries 429s up to three times usingRetry-After; this PR doesn't yet.cf speedtestsits next to the existingcf speed(Observatory page tests), which could confuse people. I'm happy to rename it.Testing
pnpm checkpasses.packages/clisuite passes (147 files, 1963 tests). New tests cover the measurement maths (percentile, jitter,Server-Timingandcf-rayparsing, sparkline), the full command against MSW with a mocked clock (including the per-direction stop rule and that requests carry noAuthorizationheader), the alias, HTTP, rate-limit, network, and timeout errors, andwithProgressupdates.Note: two tests in
src/__tests__/lib/telemetry/dispatcher.test.tsfail when the suite runs inside an AI coding agent session, because agent detection reads variables such asCLAUDECODEandAI_AGENTfrom the environment. They pass once those are unset; this is unrelated to this change.🤖 Generated with Claude Code