Skip to content

[Pending internal discussions] Exclude trailing comments from _redirects line length validation#12490

Draft
vicb wants to merge 4 commits intomainfrom
vicb/fix-comments
Draft

[Pending internal discussions] Exclude trailing comments from _redirects line length validation#12490
vicb wants to merge 4 commits intomainfrom
vicb/fix-comments

Conversation

@vicb
Copy link
Copy Markdown
Contributor

@vicb vicb commented Feb 9, 2026

Follow up for #12467

The main idea of this PR is that the maximum line length of a _redirects should not take comments into account.

There are also a few other changes.

There was a single MAX_LINE_LENGTH used for both the _headers and _redirects while the first one should be 2000 and second one 1000 (See opened questions below for that one). The new constants are MAX_HEADER_LINE_LENGTH and MAX_REDIRECT_LINE_LENGTH.

Note that other constants have been renamed to clearly indicated when they relates to either HEADER or REDIRECT.

Also a bit of cleanup in the tests to make them consistent.

Questions

  • Pages doc has no explicit max line length. Workers has 1000. EWC has 1024. What is the right value? I have used 1000 in this PR (from 2000 before). But we should align EWC should not error on something this code allow and vice versa
  • Should we bump the _redirects parser version, not sure how it is used - it is defined both here and EWC
  • Should we add a global file size limit i.e. to prevent OOM? (or do we already have something in place?)

future work

  • Update EWC to also trailing comments (draft MR pending)

Usage of _redirects (by Claude and I)

1. Workers Assets deploy (wrangler deploy / wrangler versions upload)

  • Read: packages/wrangler/src/assets.ts:469maybeGetFile() reads raw text
  • Passed through: packages/wrangler/src/deploy/deploy.ts:867 (deploy),
    packages/wrangler/src/versions/upload.ts:726 (versions upload)
  • Uploaded: packages/wrangler/src/deployment-bundle/create-worker-upload-form.ts:90
    raw string embedded in AssetConfigMetadata JSON inside the FormData metadata field
  • Parsed by parseRedirects? No — the Cloudflare API parses it server-side

2. Workers Assets remote dev (wrangler dev --remote)

  • Passed through: packages/wrangler/src/dev/remote.ts:197
  • Same as deploy — raw text sent to the API
  • Parsed by parseRedirects? No

3. Pages deploy (wrangler pages deploy)

  • Read: packages/wrangler/src/api/pages/deploy.ts:136readFileSync()
  • Uploaded: packages/wrangler/src/api/pages/deploy.ts:297-299 — appended as a File
    object in FormData (not inside JSON)
  • Parsed by parseRedirects? No — the Pages API parses it server-side

4. Miniflare local dev (Workers Assets, wrangler dev --local)

  • Read & parsed: packages/miniflare/src/plugins/assets/index.ts:99-124
  • parseRedirects()constructRedirects()RedirectsSchema.parse() → injected as
    JSON CONFIG binding into the local workerd asset-worker
  • Parsed by parseRedirects? Yes

5. Pages local dev (wrangler pages dev)

  • Read & parsed: packages/wrangler/src/miniflare-cli/assets.ts:150-161
  • parseRedirects()createMetadataObject()constructRedirects()
  • Also watches for file changes and re-parses (lines 177-200)
  • Parsed by parseRedirects? Yes

6. Vite plugin dev (experimental)

  • Read & parsed: packages/vite-plugin-cloudflare/src/asset-config.ts:105-117
  • parseRedirects()constructRedirects()RedirectsSchema.parse() → passed to
    Miniflare options
  • Gated behind experimental.headersAndRedirectsDevModeSupport
  • Also detects changes via hasAssetsConfigChanged() (line 27-46)
  • Parsed by parseRedirects? Yes

7. Runtime: Workers asset-worker

  • File: packages/workers-shared/asset-worker/src/handler.ts:48,999-1050
  • Matching engine: packages/workers-shared/asset-worker/src/utils/rules-engine.ts:116-160
  • Receives pre-parsed structured configuration.redirects via CONFIG binding
  • Applies staticRedirectsMatcher() (O(1) dict lookup) then generateRedirectsMatcher()
    (regex-based dynamic matching) on each request
  • Status 200 → rewrite (proxy), 301/302/303/307/308 → HTTP redirect response

8. Runtime: Pages asset-server

  • File: packages/pages-shared/asset-server/handler.ts:202-243
  • Same two-phase matching as Workers asset-worker, but using metadata.redirects
  • Metadata generated by packages/pages-shared/metadata-generator/createMetadataObject.ts
  • Receives both the parsedRedirects and the raw file

  • 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: fix

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


Open with Devin

@vicb vicb requested review from a team as code owners February 9, 2026 15:19
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Feb 9, 2026

🦋 Changeset detected

Latest commit: b2d4420

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

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

devin-ai-integration[bot]

This comment was marked as resolved.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Feb 9, 2026

create-cloudflare

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-editor-shared

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

@cloudflare/workers-utils

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

wrangler

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

commit: 0265977

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

The "valid" test needs fixing.

Comment thread packages/workers-shared/utils/configuration/parseRedirects.ts Outdated
Comment thread packages/workers-shared/utils/tests/parseRedirects.valid.test.ts Outdated
@github-project-automation github-project-automation Bot moved this from Untriaged to In Review in workers-sdk Feb 9, 2026
@vicb vicb changed the title fix: inline comments in _redirects file [Pending internal discussions] fix: inline comments in _redirects file Feb 10, 2026
@vicb vicb changed the title [Pending internal discussions] fix: inline comments in _redirects file [Pending internal discussions] tweak to the parsing of _redirects file Feb 12, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

@vicb vicb force-pushed the vicb/fix-comments branch from 0ded52c to 35096b4 Compare February 12, 2026 16:56
@vicb vicb changed the title [Pending internal discussions] tweak to the parsing of _redirects file [Pending internal discussions] tweak to the parsing of _redirects files Feb 12, 2026
@vicb vicb changed the title [Pending internal discussions] tweak to the parsing of _redirects files [Pending internal discussions] tweak the parsing of _redirects files Feb 12, 2026
@vicb vicb force-pushed the vicb/fix-comments branch from 35096b4 to 09e3bb1 Compare February 12, 2026 17:05
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Feb 12, 2026

Claude encountered an error —— View job


Changeset Review

Reviewing changeset files...

  • Read changeset guidelines
  • Read changeset file
  • Validate changeset
  • Check for cute animals
  • Post review

@vicb vicb changed the title [Pending internal discussions] tweak the parsing of _redirects files [Pending internal discussions] Exclude trailing comments from _redirects line length validation Feb 23, 2026

This is consistent with full line comments where the line length is not limited.

Additionally, the maximum line length for redirect directives has been corrected to 1000 characters (matching the Workers platform limit), down from 2000.
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.

Unconvinced by this line. Won't this break people in prod today with 1001 characters?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See the opened questions in the PR description

@petebacondarwin petebacondarwin marked this pull request as draft April 19, 2026 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

4 participants