[arman-dogru]

Problem

LiquidHTMLSyntaxError flags a valid for/tablerow loop when the collection is a lookup with a quoted key containing a space, e.g. product.options_by_name['hello world'].values. That's the documented way to read a product option whose name contains spaces. The offense's autofix then deletes part of the collection expression, so "fix on save" turns valid Liquid into broken Liquid. This is upstream issue 1204 in Shopify's theme-tools repo (reported against Theme Check 3.94.1, still reproducible on main @ ee5ab68).

This matters more when an agent is the one editing: a validator that reports a valid line and offers a one-click "fix" that breaks the template will mislead any edit-validate loop that trusts autofixes.

Minimal repro

{%- for value in product.options_by_name['hello world'].values -%}
  {{ value }}
{%- endfor -%}

Actual output on main:

snippets/options-by-name.liquid:1:49  [LiquidHTMLSyntaxError] Arguments must be provided in the format `for in <array> <positional arguments> <named arguments>`. Invalid/Unknown arguments: world'].values  (autofixable)

What the autofix writes:

-{%- for value in product.options_by_name['hello world'].values -%}
+{%- for value in product.options_by_name['hello  -%}

The same thing happens with double quotes, with tablerow, with commas in the key (['Size, Fit']), with spaces inside the brackets ([ 'Size' ]), and when valid arguments follow the lookup (... .values limit: 2 is reported as Chart"].values).

This is real theme-check-node output on docs/fixes/1204-quoted-lookup-keys/repro-theme, captured verbatim in theme-check-before.txt and theme-check-after.txt and only coloured for the image. Before: 6 offenses, 3 of them false positives, and the autofix corrupts all 5 loops that use a quoted key. After: the 3 real mistakes are still reported, and their autofixes leave the collection intact.

Root cause

The Liquid parser already handles this markup correctly. ForMarkup.collection is a VariableLookup with the right position. The false positive comes from the secondary loop-argument check in liquid-html-syntax-error/checks/InvalidLoopArguments.ts:

1. It ignores the parsed AST, re-tokenizes the whole markup with getFragmentsInMarkup, and assumes fragments 0–2 are <variable> in <collection>.
2. That tokenizer's catch-all REGULAR_TOKEN is [^\s,]+, which splits on any whitespace or comma, including inside ['...'].

So product.options_by_name['hello world'].values turns into two fragments. The second one, world'].values, is treated as an unknown positional argument, and the fix replaces the "argument span" with the valid arguments, deleting the tail of the collection.

The same tokenizer (via getValuesInMarkup) backs the MultipleAssignValues and InvalidEchoValue sub-checks. Those report correctly, but their autofixes cut the lookup too: {% assign x = a['b c'] b %} was "fixed" to {% assign x = a['b %}.

Fix

Two changes, both needed. Each one has tests that fail without it.

1. Anchor on the AST when it exists (InvalidLoopArguments.ts). If the markup parsed (typeof markup !== 'string'), only the text between markup.collection.position.end and markup.position.end is tokenized as arguments. The check no longer guesses where the collection ends, so collections the tokenizer can't model are handled too, e.g. ( 1 .. product.options_by_name['hello world'].values.size ) or a[ b[c[d]] ]. The string fallback for unparseable markup keeps the existing "skip the first three fragments" behaviour.
2. Keep bracketed lookups in one token (utils.ts). REGULAR_TOKEN becomes (?:BRACKETED_LOOKUP|[^\s,])+. BRACKETED_LOOKUP matches a balanced [...] with quoted strings matched whole (so a quoted ] or [ doesn't end it) and one level of nested brackets. An unbalanced [ or ] falls through to the single-character branch, so no input is ever dropped. This fixes the unparsed-markup path (... .values !!), named-argument values (limit: settings['max items']), and the assign/echo autofixes.

A patch changeset for @shopify/theme-check-common is included.

Tests

New cases are in InvalidLoopArguments.spec.ts, utils.spec.ts, MultipleAssignValues.spec.ts and InvalidEchoValue.spec.ts. They were committed first and failing in a0b3593, before the fix.

• No false positive (negative cases): the exact snippet from the issue; double quotes; tablerow; commas and inner spaces in the key; nested lookups; range bounds with quoted keys (with and without spaces around ..); inside {% liquid %}; and valid reversed, limit:, offset: and cols: after the lookup, including comma-separated forms and a bracketed named-argument value.
• True positives still fire, and the fixes are now safe: limit1: 2, reversed after a named argument, unparseable !!, rows: 3 on tablerow, and a trailing bad['x y']. Each one asserts the message and the exact autofix output, and the collection is never touched. The assign/echo trailing-value offenses still fire, and their fix keeps a['b c'] whole.
• Tokenizer unit tests: exact fragment lists; quoted [ and ] inside keys; unbalanced brackets (a[ b, a] b) still yield every character; plain space/comma splitting is unchanged.

Run (vitest run packages/theme-check-common, Node 20.19.2)	Files	Tests
main (ee5ab68), its own tests	121 passed	5361 passed
main source + this PR's tests	4 failed	10 failed, 5363 passed
This PR	121 passed	5373 passed

• Whole repo, CI=true pnpm exec vitest run (CI config, which excludes the prettier plugin): 268 files passed, 1 skipped; 7176 tests passed, 5 skipped, 0 failed
• pnpm --filter @shopify/theme-check-common type-check: clean
• pnpm format:check: clean
• Mutation check: reverting only the AST anchor fails the spaced-range case. Reverting only the tokenizer fails 6 tests (valid arguments after a lookup, safe fixes, and the tokenizer units).
• Performance: tokenizing 5–15k-character adversarial strings ([ × 5000, a[ b × 3000, unterminated ['…) took ≤ 3 ms each, in the same range as main (≤ 5 ms), so there's no catastrophic backtracking.
• Real theme: Theme Check over Dawn 16.0.0 with Dawn's own config returns the same 9 offenses before and after (identical list).

How to verify

pnpm install && pnpm build:ts
pnpm exec vitest run packages/theme-check-common/src/checks/liquid-html-syntax-error
pnpm exec vitest run packages/theme-check-common
pnpm --filter @shopify/theme-check-common type-check && pnpm format:check
node docs/fixes/1204-quoted-lookup-keys/repro.mjs . docs/fixes/1204-quoted-lookup-keys/repro-theme

Known open / out of scope

• In unparseable markup, the tokenizer only models one level of nested brackets inside a lookup that contains spaces. Parsed markup doesn't depend on it because of the AST anchor.
• Existing behaviour I left alone: the loop-argument autofix leaves a double space where it removed arguments (... .values -%}), and the message reads for in <array> without the loop variable. Existing tests assert both.
• docs/fixes/1204-quoted-lookup-keys/ is review evidence for this fork only. It would be dropped from an upstream submission.
• This branch hasn't run on CI. All the numbers above come from local runs on Node 20.19.2.

[arman-dogru]

Peer Reviewer: CHANGES REQUESTED (46bd3e0)

The core fix is real, and it's a clear improvement on issue 1204. One regression blocks approval: the new AST-anchored path adds a false positive with a destructive autofix on plain loops that use a comma with no following space (array,limit:2). That's the same class of bug this PR is meant to remove, on markup that main handles correctly. It's a small fix, but it has to land with tests before this can be approved.

What I verified (fresh clone, HEAD = 46bd3e0, Node 20.19.2, pnpm 10.16.1)

• vitest run packages/theme-check-common: 121 files / 5373 tests passed (matches the claim).
• main (ee5ab68) with its own tests: 5361 passed (matches).
• main source with this PR's tests: 4 files / 10 tests failed, 5363 passed (matches). The new tests really do fail without the fix.
• Whole repo, CI=true vitest run: 268 files passed, 1 skipped; 7176 passed, 5 skipped (matches).
• type-check (theme-check-common) and format:check: clean.
• Reverting only the AST anchor fails 1 test (the quoted-key/spaced-range block), so that part is real and tested. Reverting only the tokenizer fails 8 tests, not the 6 the PR body claims (see 3).
• repro.mjs on this branch gives output byte-identical to theme-check-after.txt. On main's utils.ts and InvalidLoopArguments.ts (rebuilt), the output is byte-identical to theme-check-before.txt. Re-running render_capture.py gives a before-after.png byte-identical to the committed one, so the image is genuine and accurate.
• Dawn 16.0.0 (258f00f): 9 offenses before and 9 after, identical lists.
• Adversarial tokenizer inputs (5k–15k chars: [×5000, a[ b ×3000, unterminated ['…, [[a]×3000, a["]"×3000, k:[×3000, and others) all run in ≤ 1.8 ms. I found no catastrophic backtracking.
• No secrets, local paths or tokens in the diff. No owner-qualified upstream references in the diff, commits or PR body. All three commits are authored under the noreply address.

Findings

1. MAJOR: regression, false positive and destructive autofix on <collection>,<named-arg> with no space

packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidLoopArguments.ts:40-43 together with utils.ts:34 (KEY_VALUE_PAIR = (\S+):…)

The parsed path now tokenizes node.source.slice(collection.position.end, …), which starts at the comma. KEY_VALUE_PAIR's (\S+) then takes ,limit as the key, isSupportedTagArgument rejects it, and the "fix" deletes a valid argument. On main the whole array,limit:2 was one fragment (index 2), so it was never reported.

The parser accepted this markup (that's why it took the AST path), and Liquid accepts comma-separated attributes, so this is valid code.

Repro, using the spec's testCheck docset:

Source	main	this PR
{% for value in array,limit:2 %}{% endfor %}	no offense	Invalid/Unknown arguments: ,limit:2, fix gives {% for value in array %}
{% for value in array,limit: 2 %}{% endfor %}	no offense	same; limit deleted
{% tablerow value in array,cols:2 %}{% endtablerow %}	no offense	,cols:2 reported; cols deleted
{% liquid\n for value in array,limit:2 … %}	no offense	,limit:2 reported; limit deleted
{% for value in a['x y'],limit:2 %}	corrupts the key	,limit:2 reported; limit deleted

The autofix changes runtime behaviour: the loop goes from 2 items to all of them. The PR body says comma-separated forms are covered, but the only test (.values, limit: 2) has a space after the comma, which hides the problem.

Fix: in the parsed branch, advance argumentsOffset past /^[\s,]*/ before tokenizing. Better still, tighten the key in KEY_VALUE_PAIR to ([\w-]+), so a leading or embedded comma stays a separator. That also fixes the pre-existing limit:2,offset:1 and reversed,limit:2 false positives (see 6). Add tests for every row above, asserting zero offenses.

2. MINOR: the fallback path still corrupts the collection with ≥2 levels of nested brackets

utils.ts:20-25 (BRACKETED_LOOKUP, one nesting level) and InvalidLoopArguments.ts:47-56

{% for v in a[ b[c[d]] ] bad %}{% endfor %} reports Invalid/Unknown arguments: b[c[d]], ], bad, and the fix writes {% for v in a[ %}.

This is listed as known-open, but the framing "parsed markup doesn't depend on it" undersells it. In my probes an unknown positional argument (bad) put the markup on the string fallback. Unknown arguments are the main true-positive class, so they're exactly the case where the autofix fires.

Fix: make the fallback non-destructive by construction. If any of the first three fragments, or any removed fragment, has unbalanced [/] or an unterminated quote, still report the offense but return no fix. Add the repro above as a test.

3. MINOR: the PR body's mutation numbers are off

The body says "Reverting only the tokenizer fails 6 tests." Reverting utils.ts to main while keeping the AST anchor fails 8 tests in 4 files: 2 in InvalidLoopArguments, 4 in utils, 1 each in MultipleAssignValues and InvalidEchoValue. Please correct it; every number in the body should be reproducible.

4. MINOR: shared-tokenizer blast radius isn't tested or mentioned

utils.ts is also used by InvalidConditionalNode.ts:161 (getValuesInMarkup) and InvalidConditionalNodeParenthesis.ts:17 (getFragmentsInMarkup). The changeset and body only mention loop, assign and echo.

I probed if/unless/case with a['b c'] == 'd e', a['(b)'], c['d (e)'] and a['b c'] and, and saw no behaviour change against main. Lock that in with one or two conditional regression tests, and mention the conditional checks in the changeset or body.

5. NIT: the Dawn run doesn't exercise this check

All 9 Dawn offenses come from other checks (UndefinedObject, VariableName, UnusedAssign, LiquidComplexity, OrphanedSnippet). LiquidHTMLSyntaxError reports zero before and after, so "same 9 offenses" proves no collateral damage elsewhere, but says nothing about this change. State that plainly in the docs README and PR body.

6. NIT (pre-existing, not introduced here): comma-joined named args

{% for value in array limit:2,offset:1 %} and {% for value in array reversed,limit:2 %} are reported on both main and this PR, and the autofix deletes valid arguments. They have the same root cause as 1 ((\S+): key). The [\w-]+ change in 1 should fix these too; add them to the tests if you take it.

7. NIT: before/after image legibility

before-after.png is 1856×1324 with a 13 px font. At PR-body width it shrinks to roughly half size and can't be read without clicking through. The same text is already in the body, so it's not misleading. Consider cropping to the two autofix diffs, or rendering two narrower panels.

8. NIT: #1204 in commit subjects and test titles

The fork has issues disabled, so (#1204) doesn't link to anything here. If fork issues are ever enabled it will autolink to the wrong thing. Prefer "issue 1204" in new text.

What's good

• The root-cause analysis is correct, and anchoring on markup.collection.position.end is the right design. It fixes the spaced-range and deep-nesting cases that the tokenizer alone can't.
• The tests assert exact autofix output, not just offense counts, and the fail-first commit ordering is real.
• The repro captures are verbatim and reproducible, and the PNG is generated from them deterministically.

To get to APPROVED

Fix 1, with tests for every row in its table. Address 2 (fix suppression on unbalanced fragments), and correct the numbers in 3. 4–8 are optional, but cheap.

[arman-dogru]

Superseded by #2. This fix for upstream issue 1204 overlaps upstream PR 1320, so the Theme Check redo moved to upstream issue 944 (section objects in theme snippets and blocks). Closing without merging; the branch fix/1204-quoted-lookup-keys-in-loops is kept.