Skip to content

[wrangler] Categorise deploy positional path in command telemetry#14587

Open
MattieTK wants to merge 1 commit into
mainfrom
telemetry-deploy-positional-arg-category
Open

[wrangler] Categorise deploy positional path in command telemetry#14587
MattieTK wants to merge 1 commit into
mainfrom
telemetry-deploy-positional-arg-category

Conversation

@MattieTK

@MattieTK MattieTK commented Jul 7, 2026

Copy link
Copy Markdown
Member

What this does

wrangler deploy (and wrangler versions upload) accept an optional first positional argument – the path to a Worker entry point or a directory of static assets. Today that positional is dropped from telemetry entirely (positionals are excluded by sanitizeArgKeys unless passed as a --flag), so we have no signal about how people use it.

This adds a coarse, non-identifying category for that positional to the command event under sanitizedArgs.path:

  • file – resolves to a file
  • directory – resolves to a directory
  • current-dir. or ./
  • parent-relative.., ../example, etc.
  • not-found – provided but doesn't resolve on disk
  • null – no positional passed

The raw path is never sent, only the category. Categorisation mirrors how deploy actually interprets the positional (statSync → directory vs file), with relational references (., ..) surfaced as their own categories so they can be analysed separately.

How

Rather than special-casing the generic command-event wrapper with deploy knowledge, this extends the existing per-command allow-list in metrics/sanitization.ts:

  • A new Categoriser allow-list value type (alongside ALLOW/REDACT/fixed arrays).
  • categorisePositionalPath() – maps a raw path to a category (returns null when absent so the property is always present).
  • categoriseArgs() – applies categorisers against the full args object, which is what lets positional args be safely categorised.
  • The deploy and versions upload allow-list entries opt their path positional into categorisation.
  • register-yargs-command.ts spreads categoriseArgs(args, allowedArgs) into sanitizedArgs.

  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: this is an internal telemetry change with no change to user-facing behaviour, CLI surface, or configuration.

A picture of a cute animal (not mandatory, but encouraged)

🦫


Open in Devin Review

Record a coarse, non-identifying category for the first positional
argument of `wrangler deploy` and `wrangler versions upload` under
`sanitizedArgs.path` (file/directory/current-dir/parent-relative/
not-found, or null when absent). The raw path is never sent.

Positionals are otherwise excluded from telemetry, so this adds a
Categoriser transform to the sanitization allow-list and applies it via
categoriseArgs against the full args object.
@MattieTK MattieTK requested a review from workers-devprod as a code owner July 7, 2026 17:57
@changeset-bot

changeset-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 41e1e9b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
wrangler Minor
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Jul 7, 2026
@workers-devprod workers-devprod requested review from a team and james-elicx and removed request for a team July 7, 2026 17:57
@workers-devprod

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/wrangler
Show detailed file reviewers
  • .changeset/deploy-positional-path-telemetry.md: [@cloudflare/wrangler]
  • packages/wrangler/src/tests/deploy/entry-points.test.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/tests/metrics/sanitization.test.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/tests/versions/versions.upload.test.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/core/register-yargs-command.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/metrics/sanitization.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/metrics/types.ts: [@cloudflare/wrangler]

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

✅ All changesets look good

@ask-bonk

ask-bonk Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

The changeset is correct: minor for a telemetry collection change (per repo rules, "If the change collects more analytics, it should be a minor"), uses proper format, no forbidden headers.

I've thoroughly reviewed this PR:

  • Security/privacy (the main risk for a telemetry change): only fixed category labels are ever emitted; the raw path is never sent. statSync errors (which can contain paths) are caught and discarded. Verified.
  • Correctness: categorisation faithfully mirrors deploy's actual interpretation in deploy-args.ts:223-237 (statSync → directory vs file), and args.path is not mutated by validateArgs, so the categoriser reads the original value.
  • No collisions: path is a positional (excluded by sanitizeArgKeys), so categoriseArgs and sanitizeArgValues don't clash. types.path is an array, not a categoriser, so it's skipped.
  • Windows separators (.\\, ..\\) handled.
  • Tests: comprehensive unit + integration coverage across all categories, including the "raw path never sent" assertion.
  • Changeset: correct type/format.

No actionable issues found.

LGTM

github run

@pkg-pr-new

pkg-pr-new Bot commented Jul 7, 2026

Copy link
Copy Markdown
@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@14587

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@14587

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@14587

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@14587

miniflare

npm i https://pkg.pr.new/miniflare@14587

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@14587

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@14587

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@14587

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@14587

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@14587

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@14587

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@14587

wrangler

npm i https://pkg.pr.new/wrangler@14587

commit: 41e1e9b

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

2 participants