[arman-dogru]

Partly addresses upstream Shopify/theme-tools issue 944. This is a fork-only PR. Upstream references here are code spans on purpose, so the fork doesn't cross-reference upstream.

Not a full fix of 944: the thread also asks for form and paginate, and those stay reported deliberately (see "Still reported"). An upstream PR would say "Refs", not "Fixes".

Why this issue (and not 608 or 722)

• 608 is already fixed on main. Upstream 00b683e (2024-11-26) replaced the check that flagged settings[].type under blocks[]. The screenshot's schema gives 0 offenses on main.
• 722's fix lives in Shopify/theme-liquid-docs. section.json declares locales values as strings only.
• 944 was half done. Upstream d4ec896 (PR 1267) allowed section/block in theme snippets in the check only. The language server was left behind, and predictive_search/recommendations were still reported.
• No open upstream PR changes the contextual lists.

The rule (one rule for every section object; potential availability, not a guarantee)

Shopify's render docs say a rendered snippet can access "any objects that are directly accessible outside the snippet … a snippet … inside a section can access the section object". A theme block is rendered inside a section, and a snippet is usually rendered from a section or block. So both are treated as potentially able to read everything a section can read, plus app and block:

File (theme mode)	main ee5ab68	this PR
sections/*	section, predictive_search, recommendations, comment	unchanged
blocks/*	app, section, recommendations, block	app, block, section, predictive_search, recommendations, comment
snippets/* (check: only with {% doc %})	app, section, block	app, block, section, predictive_search, recommendations, comment
blocks/*, snippets/* in app mode	app, section, recommendations, block	unchanged

predictive_search and recommendations carry the same shopify.dev restriction: results only when the section is rendered through their API plus the Section Rendering API. They are now treated identically, which settles the earlier open question. comment is in the section list because, once a new_comment form is posted, comment becomes globally available (upstream ec1fbd7, 352). Horizon's snippets/blog-comment-form.liquid reads comment.status inside its new_comment form, and the repro now covers it. The lists are frozen (Object.freeze), defined once in contextual-objects.ts, which explains why, and are used by UndefinedObject and TypeSystem.

Known limitation (Arman's P2):

• The path doesn't tell us the caller. A snippet can also be rendered directly from a layout or template, e.g. layout/theme.liquid doing {% render 'search-results' %}. There, predictive_search isn't defined, but the snippet is still accepted.
• This is the same broad allowance main already makes for section and block in snippets (upstream d4ec896).
• Both the repro (snippets/layout-rendered-search.liquid) and a test pin this case.
• Precise caller-context analysis is out of scope.

Only the definitions are shared (Arman's P3):

• The object lists live in one place.
• File classification is still separate: relativePath.startsWith(...) in the check, path regexes in TypeSystem. So the two can still disagree on unusual paths.
• Sharing that classification is a possible follow-up, not in this PR.

Still reported, on purpose:

• form, paginate (and other tag-scoped variables): render uses an isolated subcontext that doesn't carry them, which is why Dawn passes paginate: paginate explicitly.
• Section objects used directly in layouts, templates and other files.
• Everything in app-mode blocks and snippets: unchanged.

The language server isn't doc-gated. UndefinedObject skips snippets without {% doc %}. TypeSystem types the contextual objects in every theme-mode snippet, with or without doc tags.

flowchart TD
  L["theme-check-common/contextual-objects.ts<br/>SECTION_CONTEXTUAL_OBJECTS<br/>THEME_BLOCK_AND_SNIPPET_CONTEXTUAL_OBJECTS = app, block + section objects<br/>APP_BLOCK_CONTEXTUAL_OBJECTS, CHECKOUT_LAYOUT_CONTEXTUAL_OBJECTS"]
  subgraph check["UndefinedObject (theme-check-common)"]
    A["create()"] --> B{"snippets/* without {% doc %}?"}
    B -- yes --> Z["skip file"]
    B -- no --> C["collect lookups and scopes"] --> D["onCodePathEnd → globalObjects"] --> E["getContextualObjects(path, mode)"]
    E --> G["isDefined? else 'Unknown object'"]
  end
  subgraph ls["TypeSystem (theme-language-server-common), no doc gate"]
    T["getContextualEntries(uri, mode)"] --> H["inferType / completion / hover"]
  end
  L --> E
  L --> T
  P["TypeSystem parity test types every entry of the shared lists"] -.-> L
  N["file classification NOT shared:<br/>startsWith (check) vs regexes (TypeSystem)"] -.- E
  N -.- T
  style L fill:#fde2e2,stroke:#b22222

Loading

Evidence: check

repro-theme/ has:

• a predictive search section that renders a doc'd snippet and holds a theme block;
• a recommendations section that renders a doc'd snippet;
• an article section that renders a doc'd blog-comment-form snippet (modelled on Horizon's) that reads comment.status inside {% form 'new_comment' %};
• a doc'd snippet rendered only from layout/theme.liquid (the known limitation: before, it was reported; after, it's accepted);
• true positives in a layout, a template and a snippet.

Only UndefinedObject is enabled. capture.sh does the following:

1. Refuses to start, before touching any file, if $PYTHON has no matplotlib or the tree is dirty.
2. Checks out pinned base ee5ab68's check and TypeSystem sources (a trap restores them).
3. Builds and captures.
4. Rebuilds the branch and captures again.
5. Pipes the test output through sort-vitest.mjs, which orders the per-file blocks by file. Without it, vitest prints files in completion order, which changed in 1 of the reviewer's 3 runs.
6. Fails loudly. Only vitest's exit status is ignored, since the new tests fail on base sources. Its output goes to a temp file, and sort-vitest.mjs runs on that file without || true. On any failure the trap restores the captures directory to HEAD.

With a forced sort-vitest.mjs crash (a NODE_OPTIONS=--require preload that throws only in that script; no tracked file changed), capture.sh exited 1, the tree stayed clean, and all 8 artifacts matched HEAD. Two normal runs after that exited 0 and were byte-identical to HEAD.

I ran it three times in a row on a clean tree: all 8 artifacts were byte-identical across the runs, and the tree stayed clean. Lines are sorted by file, line and column (offenses.mjs).

Offense	base (before)	branch (after)
blocks/predictive-search-terms.liquid:2:7 predictive_search	reported	gone
snippets/predictive-search-results.liquid:7:25 and :8:22 predictive_search	reported	gone
snippets/blog-comment-form.liquid:8:10 comment	reported	gone
snippets/recommendation-list.liquid:6:23 and :7:20 recommendations	reported	gone
snippets/layout-rendered-search.liquid:9:21 predictive_search (rendered only from the layout)	reported	accepted: known limitation
layout/theme.liquid:8:8 predictive_search	reported	reported
templates/search.liquid:2:4 recommendations	reported	reported
snippets/true-positives.liquid:8:4 form, :9:4 paginate, :10:4 undefined_thing	reported	reported
Total	12	5

Rendered by render_capture.py from the two capture files.

Changeset: minor for @shopify/theme-check-common (4 new public exports), patch for @shopify/theme-language-server-common.

Real themes, all checks, before → after with identical lines: Dawn 16.0.0: 9 → 9; Horizon dfad90d: 6 → 6.

Evidence: language server

ls-capture.mjs drives the built CompletionsProvider and HoverProvider with the docset that pnpm build writes to packages/theme-check-docs-updater/data (revision 40f966f is recorded in the TSVs; no cache, no network). The probe snippet snippets/s.liquid has no doc tag. Data: before / after.

Probe	base	branch
C1 snippets/s.liquid {{ pre█ }}	none	predictive_search
C2 {{ predictive_search.█ }}	none	performed, resources, terms, types
C3 {{ recommendations.█ }}	none	intent, performed?, products, products_count
C4 {{ section.█ }}	none	blocks, id, index, index0, location, settings
C5 {% for p in predictive_search.resources.products %}{{ p.ti█ }}	none	title
C6 blocks/b.liquid {{ predictive_search.█ }}	none	performed, resources, terms, types
C7 {% form 'new_comment', article %}{{ comment.█ }}	none	author, content, created_at, email, id, status, updated_at, url
H1 hover predictive_search in a snippet	null	### predictive_search docs
H2 hover comment in a snippet	null	### comment docs
K1 control: templates/search.liquid {{ predictive_search.█ }}	none	none
K2 control: app-mode snippet {{ pre█ }}	none	none
K3 control: hover recommendations in a block	docs	docs

Tests

Check (index.spec.ts):

• predictive_search in a doc'd snippet, and still reported in templates and layouts.
• Every section object allowed in snippets and blocks, using the literal list section, predictive_search, recommendations, comment. This replaces upstream's "still flag block-only objects" test.
• A test that pins the exported lists to literal values. It's not a cross-package guard, because it restates the definition; the cross-package guard is the TypeSystem parity block below.
• Known limitation: a snippet rendered only from a layout is accepted.
• comment with a new_comment form.
• All four exported lists are frozen.
• App mode unchanged.

TypeSystem:

• The parity block does it.each over the shared lists, so it fails if the check's lists and the LS diverge. Its other two tests use literal arrays.
• Nested lookups and app mode:
  • predictive_search.resources.products in a doc-tagged snippet infers { kind: 'array', valueType: 'product' };
  • the loop variable infers product, and p.title infers string;
  • in app mode, predictive_search and comment are unknown, and the nested lookup is untyped. Theme mode is asserted to differ.

ObjectCompletionProvider:

• Completes section objects in snippets and blocks, and doesn't complete them in templates.
• Nested properties and app mode:
  • predictive_search.resources.█ completes exactly ['articles', 'products'], and p.█ in the loop completes ['title'];
  • in app mode, predictive_search, comment and the nested lookup all complete [].
• These replace the March 2026 "should not complete block-level contextual variables in snippets when in theme mode" test. That test kept the LS consistent with the check as it was then, but d4ec896 changed only the check and left the LS inconsistent with it.

3 touched spec files (before / after):

Run	Result
base ee5ab68 check + TypeSystem sources, branch tests	12 failed | 184 passed | 1 skipped (197)
branch	196 passed | 1 skipped (197)

The app-mode negatives fall into two groups. The completion test and the app-mode inference assertions pass on base too: they guard against regressions. The app-mode inference test also asserts that theme mode is typed, and that assertion is the part that fails on base.

Full suites (CI=true vitest run, CI matrix Node 22 and 24, plus Node 20):

Suite	Result
whole repo, Node 20.19.2 / 22.23.3 / 24.22.0	268 files passed (1 skipped), 7182 passed, 5 skipped, 0 failed, the same on all three (tree without build output)
whole repo after pnpm build (Node 22)	274 files, 7209 passed, 5 skipped = 7182 + the 27 tests in the 6 dist/esm spec copies
theme-check-common	121 files, 5368 passed
theme-language-server-common	75 files, 655 passed, 1 skipped
pnpm -r type-check (all packages)	clean
prettier --check "packages/*/src/**/*.ts" and docs/fixes/**/*.mjs	clean

About the review's "7195 passed": that clone had run pnpm build, which emits 6 compiled spec copies under packages/codemirror-language-client/dist/esm/. Vitest collects them: 275 files and 27 extra tests. 7195 − 27 = 7168, the source-only figure reported at bb4a84c. Later heads add tests: this one has 7182 on a tree without build output.

7182 here vs 7192 on the upstream branch: that branch is based on upstream main 20f8edf, not ee5ab68. Main has since added 4 spec files and 10 tests, giving 272 files and 7192, or 7219 after pnpm build (+27, measured). The fork has no CI runs, because Actions workflows aren't enabled on it.

Upstream-ready variant (prepared, not opened)

Branch upstream-944-section-objects-in-theme-snippets, pushed to the fork as a plain branch (no PR):

• 4ba504e + eace325 (Arman's review fixes) + 02052d7 (round-4 test nits), each added on top without force-push (upstream squash-merges), on upstream main 20f8edf;
• no docs/fixes/, and no red commit.
• The touched files are identical to this PR's head.
• On it: whole repo 272 files, 7192 passed, 5 skipped (Node 20/22/24 at 02052d7); 3 touched specs 196 passed + 1 skipped; pnpm -r type-check and prettier clean.

The PR body, in upstream's template, is in docs/upstream-944-pr-body.md. Arman opens it himself.

Commits on this PR

• dfb8f3f failing tests
• 4305404 first fix
• b712783, bb4a84c evidence
• eb5292a review: one rule, shared lists, parity tests, theme blocks
• b8b8bfa, 86570f1, 1584359 review: LS capture, capture.sh hardening, regenerated evidence
• review 2: minor changeset, frozen lists, comment rationale/test/repro/probes, docset from pnpm build, K3 control, wrapped PNG cells, then regenerated evidence
• b2efdc4: prepared upstream PR body (docs/upstream-944-pr-body.md)
• 11c5042, 0373749, 5ca4a59, 63afdb0: Arman's review (potential availability and known limitation, shared definitions only, nested and app-mode tests, literal expectations), then regenerated evidence

Supersedes fork PR #1 (1204).

• 344ba1c, 2c78f08, a9e3927, d2faf9c, ccbb1bc: round-4 nits (literal list pin and all-four freeze test, sorted test captures, matplotlib pre-check, PNG title), then regenerated evidence
• e0da659, 6ef1006: round-5 nit (capture.sh no longer hides a sort-vitest.mjs crash; it restores the captures on failure)

[arman-dogru]

Peer Reviewer: CHANGES REQUESTED (bb4a84c)

Counts: 0 blocker · 1 major · 5 minor · 3 nit

The code change is small and correct for what it does. All the evidence reproduces, byte for byte (PNG included), and upstream has no competing work. The one major finding is about the design rule, not the code: the PR's own justification applies equally to recommendations, which it still reports as a "true positive".

Findings

1. MAJOR: predictive_search and recommendations are treated differently under the same rule

• Where: packages/theme-check-common/src/checks/undefined-object/index.ts:222 and packages/theme-language-server-common/src/TypeSystem.ts:358. Also the repro repro-theme/snippets/true-positives.liquid:2-4,11 and PR body lines 16, 20, 54 and 104.
• What the PR says: it allows predictive_search in theme snippets based on the render rule ("any objects that are directly accessible outside the snippet").
• Why that also covers recommendations:
  • shopify.dev uses the same restriction for both objects. predictive_search "returns results only when rendered in a section using the Predictive Search API and the Section Rendering API". recommendations "returns products only when rendered in a section using the Product Recommendations API and the Section Rendering API".
  • Both are in the same sections/* contextual list (index.ts:243, TypeSystem.ts:385).
  • So by the PR's own rule, a snippet rendered from the recommendations section can read recommendations exactly as one rendered from the predictive search section can read predictive_search.
• The evidence contradicts open question 1. The PR labels recommendations a true positive in the repro snippet, the before/after table and the PNG ("still reported"), while open question 1 argues it is readable.
• The cited upstream precedent cuts the other way. Upstream's reasoning in Shopify#1267 (d4ec896) was "recommendations stays section/block-only and is still flagged in theme-mode snippets". Under that reasoning, predictive_search is equally section-only. The body cites "upstream's existing test" for recommendations but never engages with Shopify#1267's reasoning, which applies to predictive_search too.
• Repro: snippets/x.liquid = {% doc %} @param {string} x - x {% enddoc %}{{ predictive_search.terms }}{{ recommendations.products_count }}. On the branch, the first gives 0 offenses and the second gives 1. I ran this through runLiquidCheck and the repro capture.
• Fix: pick one rule and apply it to both objects.
  • (a) Allow recommendations in theme snippets too. Update the upstream test with the shopify.dev citations, add it to the LS list, and stop calling it a true positive in the repro, table and PNG.
  • (b) Or document a concrete semantic difference that justifies the split. I couldn't find one.
  • Either way, the evidence must not label as "true positive" something the PR itself argues is valid.

2. MINOR: the LS half of the change has no visual proof (standing rule: every important doc claim needs visual support)

• The title, changeset and body claim that "the language server types and completes section/block/predictive_search" in snippets. The only support is unit tests. The PNG and captures cover only UndefinedObject, and the upstream PR template asks for before/after screenshots under "Tophatting".
• Repro: I ran CompletionsProvider/HoverProvider with the real theme-check-docs-updater/data docset (a throwaway probe spec, deleted afterwards) on a theme-mode snippets/s.liquid:
  • {{ pre█ }}: base [], branch ["predictive_search"].
  • {{ predictive_search.█ }}: base [], branch performed, resources, terms, types.
  • {{ section.█ }}: base 0 props, branch 6 props.
  • Hover on predictive_search: base null, branch shows the docs.
  • {% for p in predictive_search.resources.products %}{{ p.█ }}: base nothing, branch product props.
• The behaviour is right. The evidence just isn't in the PR.
• Fix: add a reproducible LS capture (script plus before/after image, or an editor screenshot) to the PR body.

3. MINOR: open question 2. The two copies are in sync today, but sharing them only by comment is the mechanism that already failed

• Where: index.ts:222 (+ comment :259-260) vs TypeSystem.ts:358. The lists are in sync now: both are ['app','section','block','predictive_search'], and the sections, blocks and checkout lists also match.
• Why it matters: this PR exists because Shopify#1267 changed only the check and the LS silently drifted. The new TypeSystem test hardcodes the list instead of importing it, so a future drift would still pass CI.
• The comment also points one way only. TypeSystem.ts does not say "keep in sync with theme-check-common".
• theme-language-server-common already depends on @shopify/theme-check-common (package.json:31, imported at TypeSystem.ts:36), so sharing is cheap.
• Fix (preferred): export the contextual-object constants (at least BLOCK_CONTEXTUAL_OBJECTS and THEME_SNIPPET_CONTEXTUAL_OBJECTS) from theme-check-common and import them in TypeSystem. Leave the path matching as is, since one side uses startsWith and the other a regex.
• Minimum fix: add a parity test that imports the check's list.

4. MINOR: the PR body and title overstate or misdescribe the change

• Line 1, "Fixes the remaining part of Shopify#944". In the Shopify#944 thread, MarkRijkelijkhuizen asked for form and paginate too, and this PR deliberately keeps those reported. The deliberate choice is right (finding 6 explains why), but the body should say Shopify#944 stays partly open, or state that the form/paginate request is invalid and why, rather than "fixes the remaining part". An upstream Fixes #944 would auto-close the thread.
• The title says "section/block typing in doc'd theme snippets". The TypeSystem change applies to every theme-mode snippet, with or without {% doc %}. My probe used a snippet with no doc tag and got the completions. The mermaid edge from F3 to T implies the doc gate applies to the LS too, which it doesn't.
• "conflicts with upstream d4ec896" (line 70). Nothing conflicted: d4ec896 never touched the LS and the old test kept passing. The accurate wording is "left the LS inconsistent with the check".
• "nothing posted upstream" (line 1). Upstream Shopify#944's public timeline now shows two cross-referenced events by arman-dogru, from fork PR #2 and PR #1, at 8:31 AM EDT today. The issue/PR links did post visibly upstream. My earlier PR #1 round checked for exactly this ("no owner-qualified upstream refs").
• Fix: rewrite line 1, the title and line 70. Use non-linking text for upstream refs on the fork (for example Shopify/theme-tools issue 944 in backticks). The two events already posted will likely stay.

5. MINOR: the branch isn't upstream-ready as is (tree contents and commit history)

• The docs/fixes/944-predictive-search-snippets/ tree is out of place upstream. It adds 17 files: a 73 KB PNG, a Python script that needs matplotlib, captures and a repro theme. Upstream has no docs/fixes convention (docs/ holds only ADRs and guides) and no .py files anywhere.
• The failing-test commit would land in upstream history. Upstream merges with merge commits (ee5ab68, 20f8edf), so the intentionally red dfb8f3f would land on main and break git bisect.
• Fix: for the upstream PR, squash dfb8f3f into 4305404. Move the evidence into the PR description (uploaded images plus collapsed captures) and keep docs/fixes/ on the fork branch only. Use upstream's PULL_REQUEST_TEMPLATE.md sections ("What are you adding", "What's next", "Tophatting" checklist, patch changeset box).

6. MINOR: theme blocks still report predictive_search, which the same rule doesn't support, and the PR doesn't mention it

• Where: index.ts:246-247 (blocks/ → BLOCK_CONTEXTUAL_OBJECTS has no predictive_search) and TypeSystem.ts:387-388.
• Repro: blocks/a.liquid = {{ predictive_search.terms }} gives 1 offense, while a doc'd snippet gives 0.
• Under the PR's render rule, a theme block placed in the predictive-search section via content_for 'blocks' has the same access a snippet does. This is the same reason upstream allows recommendations in blocks; Horizon's blocks/product-recommendations.liquid reads it. So a snippet rendered from that block passes, but the block itself fails.
• Fix: either add predictive_search to the block list (scope creep; fine as a follow-up), or list "theme blocks" under "What still gets reported, on purpose" with the reason.

7. NIT: the whole-repo test count doesn't reproduce

• Claimed: "268 files passed (1 skipped), 7168 passed, 5 skipped".
• What I got: CI=true vitest run gives 274 passed | 1 skipped (275) files, 7195 passed | 5 skipped (7200), 0 failed, on both Node 20.19.2 and 22.23.4. The CI matrix is Node 22/24, not the 20 used in the body.
• The per-package numbers match exactly, so this is a stale or partial run, not a hidden failure.
• Fix: update the number and run on a CI Node version. The fork has Actions enabled but no runs, so enabling workflows on the fork would give real CI.

8. NIT: capture.sh robustness

• Moving base ref: BASE defaults to origin/main (capture.sh:10), which is a moving ref. Once fork main syncs with upstream, "before" silently changes. Default to ee5ab68.
• No restore on failure: there is no trap to restore the tree if a step fails after the base checkout.
• Destructive checkout: git checkout -q HEAD -- packages (:14) discards any local edits under packages/. That's documented as "clean tree", but a git diff --quiet guard would be safer.
• Sort order: offenses.mjs:12 sorts lexicographically, so :10 comes before :9 in the captures and PNG.

9. NIT: formatting

• prettier --check docs/fixes/944-predictive-search-snippets/offenses.mjs → [warn] Code style issues. It's outside format:check's glob, so CI won't catch it.

Developer's three questions

• (a) Replacing the March 2026 test: legitimate.
  • 6951379 (Gray Gilmore, closes Shopify#346) added "should not complete block-level contextual variables in snippets when in theme mode". At that time the check also reported section in theme snippets (return ['app']), so the test encoded consistency between LS and check.
  • Shopify#1267 / d4ec896 (NathanPJF, approved by graygilmore, who was "a little torn… probably a reasonable trade off") changed only the check, in 3 files, with no LS discussion.
  • So the replacement restores the original invariant rather than reversing a decision. The new negative test (recommendations not completed) keeps the guard.
  • Wording fix: see finding 4.
• (b) form/paginate stay reported: correct.
  • Liquid's render uses context.new_isolated_subcontext (Shopify/liquid lib/liquid/context.rb:146-162, tags/render.rb:63). It keeps only static environments and registers, not scopes. form and paginate are tag-scoped (index.ts:106-110).
  • Dawn 16 passes paginate: paginate explicitly (snippets/pagination.liquid:6, quick-order-list.liquid:103, facets.liquid:12).
  • recommendations is the inconsistency: see finding 1.
• (c) The two lists: in sync now, and a shared constant is warranted. See finding 3.

Upstream status (checked today)

• Shopify#944: open, Bug label, no assignee. It was assigned 2025-05-15 and unassigned 2025-07-03.
• No upstream PR is working on Shopify#944. Of the 78 open PRs, those touching undefined-object/TypeSystem/ObjectCompletion are Shopify#1347, Shopify#1343, Shopify#1310, Shopify#1298, Shopify#1238, Shopify#1197 and Shopify#1184. Of those, Shopify#1238 and Shopify#1197 edit the touched files, and none touches the snippet contextual lists.
• Searches: a search for "predictive_search" returns only 2023 PRs (Shopify#155/Shopify#159/Shopify#160).
• Merge check: a trial merge of upstream/main (20f8edf, 10 commits ahead of base) is clean, and none of the touched files changed upstream since ee5ab68.
• Shopify#608: 00b683ec (authored 2024-11-24, committed 2024-11-26) refactored valid-local-blocks. A footer schema with @app plus local blocks using inline_richtext/link_list/header/paragraph/checkbox settings gives 0 offenses on this tree. The claim holds.
• Shopify#722: theme-check-docs-updater/data/section.json → properties.locales = {object → additionalProperties {object → additionalProperties {type: string}}}, from theme-liquid-docs. The claim holds.
• CLA: upstream runs Shopify/shopify-cla-action (.github/workflows/cla.yml). Arman will need to sign it when he opens the upstream PR.

What I verified (fresh clone, bb4a84c exactly)

Claim	Claimed	Mine
Install/build per CONTRIBUTING (pnpm install --frozen-lockfile, pnpm build), pnpm 10.16.1	n/a	OK. The submodule syntaxes needs an https rewrite for ssh-less boxes, an environment issue only.
Repro theme before → after	7 → 5	7 → 5. Both predictive-search-results.liquid 7:25/8:22 are gone. layout 6:8, form, paginate, recommendations and undefined_thing remain.
capture.sh deterministic	identical	Ran twice: diff -r empty. All 4 txt files and before-after.png are byte-identical to the committed ones (sha256 80cbad61…e990).
Dawn 16.0.0 (bc39a7d), all checks	9 / 9	9 / 9, identical lines, 3 UndefinedObject
Horizon (dfad90d, 134 doc'd snippets), all checks	(not claimed)	6 / 6 identical, 0 UndefinedObject
3 touched specs on main sources + new tests	3 failed / 179 / 1 skipped	3 failed / 179 passed / 1 skipped (183), same 3 tests
3 touched specs on branch	182 + 1 skipped	182 passed / 1 skipped (183)
Skipped test	(not explained)	TypeSystem.spec.ts:713 it.skip('should support narrowing the type of blocks'), a // TODO from 78813ea (2023-11-02). Pre-existing, unrelated, so the skip is justified.
theme-check-common	5363	121 files, 5363 passed
theme-language-server-common	646, 1 skipped	75 files, 646 passed, 1 skipped
Whole repo CI=true vitest run	268 files / 7168	274 + 1 skipped files / 7195 passed, 5 skipped, 0 failed on Node 20 and 22 (finding 7)
tsc --noEmit (both packages), pnpm type-check (all)	clean	exit 0 / exit 0
pnpm format:check glob	clean	clean (offenses.mjs outside the glob, finding 9)
Check edge cases (branch)	n/a	doc'd snippet 0 · non-doc snippet 0 (check skipped) · {% liquid echo %} 0 · app-mode snippet 1 · blocks/ 1 · sections/ 0 · templates/search 1 · assigned shadow 0
LS edge cases (branch vs base)	n/a	See finding 2. Sections/blocks/layout/templates are unchanged. App-mode snippets are unchanged (pre → [], rec → recommendations).
Changeset	patch, both packages	Present and correct: patch for theme-check-common and theme-language-server-common, matching upstream's Shopify#1267/69513792 style
Commit authorship	Arman	All 4 commits have author and committer Arman Doğru <53595807+arman-dogru@users.noreply.github.com> and no extra trailers
SHA-pinned links	resolve	Every #L anchor lands on the stated line (index.ts:222, TypeSystem.ts:358, spec 359/380/390, TypeSystem.spec 784, ObjectCompletion 378/388)
render docs quote (body line 16)	n/a	Matches shopify.dev/docs/api/liquid/tags/render verbatim
Mermaid diagram	n/a	Matches create → onCodePathEnd → globalObjects → getContextualObjects → isDefined. Only the F3 → T doc-gate implication is off (finding 4).

[arman-dogru]

Peer Reviewer: APPROVED (1584359)

Round 2. Counts: 0 blocker · 0 major · 2 minor · 3 nit. All 9 round-1 findings are resolved: 8 fixed and 1 withdrawn.

I re-ran everything on a fresh build of 1584359 (on top of bb4a84c, no force-push) and on the upstream variant ce74e16.

• One rule now: contextual-objects.ts defines the lists once and both consumers import them.
• Evidence reproduces byte for byte, both PNGs included.
• No regressions: Dawn and Horizon give identical offenses.
• The upstream variant is a faithful squash.

The remaining items can be fixed before or while opening upstream.

Round-1 findings

#	Round 1	Status at 1584359	How I checked
1	MAJOR predictive_search vs recommendations inconsistent	Fixed	THEME_BLOCK_AND_SNIPPET_CONTEXTUAL_OBJECTS = ['app','block',...SECTION_CONTEXTUAL_OBJECTS] (contextual-objects.ts:40-44) feeds both index.ts:240-244 and TypeSystem.ts:379-383. Repro: recommendation-list.liquid:6:23/7:20 now gone. recommendations is no longer called a true positive (true-positives.liquid, tables, PNG).
2	MINOR no LS visual proof	Fixed	ls-capture.mjs runs 10 probes with the built CompletionsProvider/HoverProvider. My two runs produced TSV and PNG byte-identical to the committed ones. The PNG matches the TSV cell for cell, and both alt texts describe the images accurately. Small gaps: n1, n2, m2.
3	MINOR lists synced only by comment	Fixed	Exported from theme-check-common/src/index.ts:54 (export * from './contextual-objects'). It's present in dist/index.js:59 / dist/index.d.ts:7, and require('dist/index.js').THEME_BLOCK_AND_SNIPPET_CONTEXTUAL_OBJECTS resolves. contextual-objects.ts has no imports, so it can't create a cycle (madge's 2 cycles are pre-existing types.ts ones). The parity tests it.each over the imported lists (TypeSystem.spec.ts:800-811). pnpm build and pnpm type-check (all packages) exit 0.
4	MINOR body/title overstated	Fixed	Title says "upstream 944, partial". The body says "Refs, not Fixes", the doc gate is disclosed (line 30) and "left the LS inconsistent" is used (line 116). Upstream refs are code spans; I grepped for Shopify/theme-tools#, issues/, pull/ and #NNN and the only hit is the fork-local #1. The PR #1 comment has no upstream link. The upstream Shopify#944 timeline still shows only the 2 original cross-ref events (8:31 AM EDT) and no new ones; those can't be removed.
5	MINOR not upstream-ready	Fixed (via separate branch)	See "Upstream variant". The fork keeps docs/fixes/, and its README now says it's fork-only.
6	MINOR blocks still flagged predictive_search	Fixed	blocks/predictive-search-terms.liquid:2:7 is gone. My own probe: blocks/a.liquid {{ predictive_search.terms }} → 0 in theme mode, 1 in app mode.
7	NIT whole-repo count	Withdrawn, my error	The 6 packages/codemirror-language-client/dist/esm/extensions/*.spec.js files that pnpm build emits hold exactly 27 tests (6+3+2+3+5+8) in my round-1 JSON report. 7195 − 27 = 7168 and 274 − 6 = 268, so Developer's round-1 number was right and my count included compiled duplicates.
8	NIT capture.sh	Fixed	BASE=${BASE:-ee5ab68} (:11). The dirty-tree guard works: I dirtied a tracked file and got exit 1 with the message. The trap works: a copy with false injected after the base checkout exits 1 and leaves git status clean. The sort is numeric (:8 < :9 < :10).
9	NIT prettier on offenses.mjs	Fixed	prettier --check docs/fixes/…/*.mjs is clean

Developer's scrutiny points

• (a) Is widening the rule to comment and the other section objects justified?
  • predictive_search / recommendations: yes. shopify.dev gives both the same restriction, and the render rule applies to both.
  • section in blocks: not a regression. It was already allowed in blocks on base (BLOCK_CONTEXTUAL_OBJECTS), so nothing that used to be flagged is newly silenced there.
  • comment: justified, but for a different reason than the PR says (m2). docs/objects.json gives comment no template access, only parents: article.comments. Upstream added it to sections in ec1fbd7 (Shopify#352, fixing Shopify#351) because "when you post a new_comment form, the comment object becomes available globally". That is the stronger argument, and Shopify's own Horizon relies on it: snippets/blog-comment-form.liquid:6 declares @param {comment} [comment] as an optional workaround and reads comment.status at :53, but sections/main-blog-post.liquid:51 renders it without passing comment.
  • Can it hide real bugs? Yes, in one case: a doc'd snippet rendered from a template or layout that reads a section object is no longer flagged. That's the same trade-off Shopify#1267 accepted for section/block, and it's disclosed.
  • Real-theme deltas: Dawn 16 gives 9 → 9 offenses, identical, 3 → 3 UndefinedObject. Horizon dfad90d gives 6 → 6, identical, 0 → 0.
• (b) Do the widened theme blocks go beyond Shopify#944? Yes. Shopify#944 is about snippets. It is disclosed in the title, changeset, fork body table (line 19) and upstream body (lines 8 and 10), with a consistent rationale. The risk is that graygilmore was "a little torn" even about section/block in Shopify#1267, so maintainers may want blocks split out. Not a finding, but worth one line in the upstream "What's next".
• (c) Is the new module exported and importable without circular dependencies? Yes; see row 3.
• (d) Is the LS visual proof reproducible? Yes. Two runs are byte-identical, the PNG matches the TSV, and the alt text is accurate; see row 2, n1 and n2.
• (e) Is the upstream variant correct? Yes; see below.
• (f) Are there linking upstream refs left? No; see row 4.

Upstream variant ce74e16 (fetched read-only from /home/box/work/theme-tools into refs/review/up944)

• History: exactly 1 commit (20f8edfc..ce74e16), parent 20f8edfc1ee4… (upstream main).
• Authorship: author and committer are both Arman Doğru <53595807+arman-dogru@users.noreply.github.com>.
• Diff: diff <(git diff ee5ab68 1584359 -- packages .changeset) <(git diff 20f8edf ce74e16 -- packages .changeset) → identical, including blob hashes. There are no files outside packages/ and .changeset/ (no docs/fixes), and the changeset is present.
• Fresh worktree (pnpm install --frozen-lockfile, pnpm build): pnpm type-check 0, pnpm format:check clean. The 3 specs give 187 passed / 1 skipped. The whole repo gives 278+1 files / 7210 passed / 5 skipped / 0 failed. That count includes the 6 dist copies (27 tests), so source-only is 7183, which matches the body's claim.
• Body (/workspace/theme944-upstream-body.md):
  • It follows .github/PULL_REQUEST_TEMPLATE.md: What are you adding / What's next / What did you learn / Tophatting with the screenshot box / Before you deploy with the patch changeset box.
  • It uses "Refs Shopify#944. This doesn't close it", which won't auto-close.
  • Other refs (Shopify#1267, Shopify#346, Shopify#352) are same-repo, non-closing mentions.
  • Line 24's comment rationale is wrong (m2), and the changeset box is debatable (m1).

New findings

m1. MINOR: patch changeset for a public API addition

• Where: .changeset/undefined-object-predictive-search-snippets.md:2 ('@shopify/theme-check-common': patch) and the same in ce74e16.
• Repro: export * from './contextual-objects' (packages/theme-check-common/src/index.ts:54) adds 4 exported constants to the package's public entry point. Upstream's PR template says "Public API changes, new features → I included a minor bump changeset".
• Fix: either bump @shopify/theme-check-common to minor, or say in the upstream body why patch is acceptable (internal-use constants) and let maintainers decide. The LS package stays patch.

m2. MINOR: the comment rationale is wrong, and comment has no visual evidence

• The rationale. Fork body line 23 and upstream body line 24 say comment is in the section list "because of the article template (352)". Upstream's actual reason (ec1fbd7, Shopify#352) is that comment "becomes available globally" after a new_comment form post.
• No visual evidence. Neither the repro theme nor ls-capture.mjs exercises comment. Only unit tests cover it (via the SECTION_CONTEXTUAL_OBJECTS loop). Under the standing rule, an important doc claim needs visual support.
• Fix:
  • Correct the rationale in both bodies.
  • Cite the Horizon evidence above (snippets/blog-comment-form.liquid:6,53 rendered from sections/main-blog-post.liquid:51 without comment).
  • Add a comment case to the repro theme (e.g. a doc'd comment-form snippet) and one LS probe ({{ comment.█ }} in a snippet), then regenerate.

n1. NIT: ls-capture.mjs depends on the machine's docs cache

• Where: ls-capture.mjs:56 uses new ThemeLiquidDocsManager(). Its loaders read ~/.cache/theme-liquid-docs-nodejs first and fall back to the bundled theme-check-docs-updater/data only if that's missing (themeLiquidDocsManager.ts:138-142).
• Today the captures are byte-identical and the cache equals the bundled data: both are revision 40f966fc…, and objects.json cmp is identical. But "real Shopify docset" captures can drift on another machine or after the cache refreshes.
• Fix: point the script at the bundled data, e.g. run with XDG_CACHE_HOME=$(mktemp -d), or load data/*.json directly.

n2. NIT: LS capture presentation

• H2 is labelled a target but behaves as a control. It's the hover on recommendations in a block, which is identical before and after because blocks already allowed it. The README says "8 targets, 2 controls"; it's effectively 7 and 3.
• Hover text is clipped in the PNG. The ### predictive_search: \predictive_search`andrecommendations` strings are cut at the right cell edge.
• Fix: relabel H2 and wrap or shorten the hover cells.

n3. NIT: the exported lists are mutable at runtime

• Where: contextual-objects.ts:10,21,40,47.
• readonly string[] is a type-only guarantee. A JS consumer of the new public export can push into the arrays, which would change both the check and the LS.
• Fix: Object.freeze([...]), or as const plus freeze.

What I verified

Claim	Claimed	Mine
Install/build	n/a	lockfile unchanged since bb4a84c; pnpm install --frozen-lockfile 0, pnpm build 0
Repro theme	10 → 5	10 → 5. Gone: block 2:7, snippet 7:25/8:22, recommendation-list 6:23/7:20. Kept: layout 6:8, template 2:4, form/paginate/undefined_thing.
capture.sh twice	byte-identical	Identical across runs and to the committed txt/tsv/PNGs (git status clean after both runs)
LS probes	10 (2 controls)	C1–C6 and H1 change from none/null to the claimed values. H2, K1 and K2 are unchanged (n2).
Dawn 16.0.0 / Horizon dfad90d	9/9, 6/6	9/9, 6/6, identical lines, UndefinedObject 3/3 and 0/0
3 touched specs, base sources	6 failed	6 failed / 181 passed / 1 skipped (188): same 6 tests
3 touched specs, branch	187 + 1 skipped	187 passed / 1 skipped (188)
theme-check-common	5364	121 files, 5364 passed
theme-language-server-common	650 + 1 skipped	75 files, 650 passed, 1 skipped
Whole repo	7173 / 5 skipped, Node 20/22/24	7200 passed / 5 skipped incl. 6 dist copies (27 tests). Source-only 268+1 files, 7173 passed, 5 skipped, 0 failed. Same on Node 20.19.2, 22.23.4 and 24.22.1.
type-check / prettier	clean	pnpm type-check 0; format:check glob clean; docs *.mjs and changeset clean
Upstream ce74e16	1 commit, no docs/fixes, 7183	Confirmed (see above)
SHA-pinned links	resolve	contextual-objects.ts#L10-L45, index.ts#L240, TypeSystem.ts#L371-L383, index.spec.ts#L363/L384/L394/L409, TypeSystem.spec.ts#L786-L836, ObjectCompletionProvider.spec.ts#L378/L390 all land on the stated code
Commits	Arman	eb5292a, b8b8bfa, 86570f1, 1584359: author and committer Arman Doğru <53595807+arman-dogru@users.noreply.github.com>

[arman-dogru]

Peer Reviewer: APPROVED (9272f1a)

Round 3. Counts: 0 blocker · 0 major · 0 minor · 2 nit. All 5 round-2 findings are fixed.

I rebuilt from scratch at 9272f1a (4 commits on top of 1584359, no force-push). The suites pass on Node 20/22/24 and the captures are byte-identical across two runs and to the committed files. The upstream squash 4ba504e has the same package and changeset diff as the fork. The two nits below are optional polish.

Round-2 findings

#	Round 2	Status at 9272f1a	How I checked
m1	MINOR: patch changeset for new public exports	Fixed	.changeset/undefined-object-predictive-search-snippets.md:2 is '@shopify/theme-check-common': minor, and the LS stays patch. The text names all 4 exports. Upstream body line 76 ticks the template's "Public API changes → minor" box.
m2	MINOR: wrong comment rationale, no visual evidence	Fixed	See the comment notes under this table.
n1	NIT: LS capture depended on the machine cache	Fixed (caveat in new nit 2)	ls-capture.mjs:16-32 reads only packages/theme-check-docs-updater/data/*.json (no ThemeLiquidDocsManager, no cache) and prints # docset 40f966fc… as the first TSV line. render_capture.py:51-52 skips that line.
n2	NIT: H2 mislabelled, PNG clipped	Fixed	The old H2 is now control K3 (hover recommendations in a block, docs → docs). The PNG title says "C*/H* are targets, K* are controls". Cells wrap at 30/40 characters, and I saw no clipping in the regenerated PNG.
n3	NIT: mutable exported lists	Fixed (test gap in new nit 1)	All 4 lists are Object.freeze([...]) (contextual-objects.ts:15,26,45,52). dist: Object.isFrozen is true for all 4, and push throws TypeError.

comment (m2)

• Rationale: contextual-objects.ts:9-14, fork body line 23 and upstream body line 24 now cite ec1fbd7/Shopify#352 ("globally available once a new_comment form is posted") plus Horizon.
• New check test: index.spec.ts:409 fails on base. It is one of the 7 base failures in tests-before.txt.
• Repro: snippets/blog-comment-form.liquid:8:10 is gone, and the PNG and table show it.
• LS probes:
  • C7 {{ comment.█ }} inside a new_comment form goes from none to author, content, created_at, email, id, status, updated_at, url.
  • H2 hover goes from null to ### comment.

Points you asked me to verify

• Object.freeze doesn't break consumers, and the types work.
  • The only consumers are globalObjects → contextualObjects.includes(name) (index.ts:194-198) and contextualVariables → contextualEntries.includes(...) (TypeSystem.ts:349-350). A grep for push|pop|splice|sort|reverse|shift|unshift|fill|copyWithin on these lists across packages/*/src returns nothing.
  • Spreading inside THEME_BLOCK_AND_SNIPPET_CONTEXTUAL_OBJECTS copies before freezing. [...list, 'x'] and list.concat(['y']) still work on the frozen exports (5 and 5).
  • Object.freeze(string[]) returns readonly string[], matching the annotations. pnpm type-check (all packages) and pnpm build both exit 0.
• The comment repro matches Horizon's real render pattern.
  • Horizon: sections/main-blog-post.liquid:51 {% render 'blog-comment-form', article: article, section_id: section.id %}. The snippet reads comment.status (:53) inside {% form 'new_comment', article %} (:10) without being passed comment.
  • Repro: sections/main-article.liquid:1 {% render 'blog-comment-form', article: article %}. The doc'd snippet reads comment.status inside {% form 'new_comment', article %}.
  • The pattern matches. The repro deliberately omits Horizon's optional @param {comment} [comment] workaround, which is why Horizon itself shows 0 offenses before and after.
• The new LS probes are correct and the PNG matches the TSV.
  • C7 and H2 behave as described above.
  • Targets (C1–C7, H1, H2): 9 rows, all changed and shaded. Controls (K1–K3): 3 rows, unchanged and unshaded.
  • Every PNG cell matches ls-before.tsv / ls-after.tsv.
• 4ba504e is a faithful single commit for upstream.
  • History: exactly 1 commit on 20f8edfc. Author and committer are both Arman Doğru <53595807+arman-dogru@users.noreply.github.com>.
  • Diff: diff <(git diff ee5ab68 9272f1a -- packages .changeset) <(git diff 20f8edf 4ba504e -- packages .changeset) is identical. There are no paths outside packages/ and .changeset/.
  • Clean worktree build: install, build and type-check exit 0, and format:check is clean. On Node 20 and 22 the run gives 278+1 files / 7212 passed / 5 skipped, 0 failed. Without the 6 dist copies that's 7185 passed / 5 skipped.
  • Body: /workspace/theme944-upstream-body.md has all the template sections, including the "Public API changes" checklist. It uses "Refs Shopify#944. This doesn't close it" and offers to split out blocks (line 25). A grep for fix|close|resolve #N, Owner/repo#N and github.com/Shopify/theme-tools/issues|pull finds nothing. Plain Shopify#944/Shopify#1267/Shopify#346/Shopify#352 are same-repo mentions, which are fine upstream.
• The fork PR body has no linking upstream refs.
  • Upstream numbers and SHAs are in code spans. The only #N is the fork-local #1 (line 162).
  • Upstream Shopify#944's timeline still shows only the 2 cross-refs from 8:31 AM EDT; nothing new.
  • The SHA-pinned anchors contextual-objects.ts#L9-L49 and index.spec.ts#L409/#L427 land on the stated code.

New findings

Nit 1: the frozen-list test covers only 2 of the 4 lists

• Where: index.spec.ts:427-431. It checks SECTION_CONTEXTUAL_OBJECTS and THEME_BLOCK_AND_SNIPPET_CONTEXTUAL_OBJECTS only.
• Repro:
  • Removing Object.freeze from SECTION_CONTEXTUAL_OBJECTS → 1 failed (caught).
  • Removing it from CHECKOUT_LAYOUT_CONTEXTUAL_OBJECTS → 51/51 pass (not caught). The same applies to APP_BLOCK_CONTEXTUAL_OBJECTS.
• Fix: loop over all 4 exports.

Nit 2: "bundled docset… no network" overstates the pinning

• Where: ls-capture.mjs:3-4, README row, and fork body line 87.
• What actually happens: packages/theme-check-docs-updater/data/ is gitignored (.gitignore:21, 0 tracked files). It is downloaded by that package's postbuild (node scripts/cli.js download data) during pnpm build. So the capture itself is offline, but the data comes from the network at build time. The revision is recorded in the TSVs (40f966fc…) but not enforced.
• Fix: say "downloaded by pnpm build". Optionally, have ls-capture.mjs exit non-zero when latest.json's revision differs from an expected one, so a regenerated capture can't drift silently.

What I verified

Claim	Claimed	Mine
Install/build	n/a	lockfile unchanged; pnpm install --frozen-lockfile 0, pnpm build 0 (docset revision 40f966fc…)
Repro theme	11 → 5	11 → 5 (adds blog-comment-form.liquid:8:10 to round 2's 10)
LS probes	9 targets changed, 3 controls unchanged	9 changed (C1–C7, H1, H2), 3 unchanged (K1–K3)
capture.sh twice	byte-identical	identical across runs and to the committed txt/tsv/PNGs (git status clean)
3 touched specs, base sources	7 fail	7 failed / 182 passed / 1 skipped (190)
3 touched specs, branch	189 + 1 skipped	189 passed / 1 skipped (190)
theme-check-common / LS	(not restated)	5366 / 650 + 1 skipped
Fork whole repo, Node 20/22/24	7175 / 5 / 0 (source-only)	source-only 268+1 files, 7175 passed / 5 skipped / 0 failed on 20.19.2, 22.23.4 and 24.22.1. That's 7202 with the 6 dist copies.
Upstream 4ba504e	7185 / 5, Node 22	7185 / 5 / 0 source-only (7212 incl. dist) on Node 20 and 22
type-check / prettier	clean	pnpm type-check 0; format:check clean; docs *.mjs and .changeset/*.md clean
Dawn 16.0.0 / Horizon dfad90d	9/9, 6/6	9/9, 6/6, identical lines
Freeze consumers	safe	no mutators; spread and concat fine; isFrozen true ×4
Commits	Arman	0425287, 3544eb9, 41fbf1f, 9272f1a: author and committer Arman Doğru <53595807+arman-dogru@users.noreply.github.com>

[arman-dogru]

Peer Reviewer: APPROVED (63afdb0)

Round 4 (final): Arman's review of the upstream branch, applied as new commits. Fresh clone at /workspace/review/theme_tools_pr2_r4; every git/gh call ran without GITHUB_TOKEN.

Result: 0 blocker, 0 major, 0 minor, 4 nits. Every number Developer claimed reproduced exactly. The nits don't block, and none of them touch the upstream branch's code.

Findings

1. nit: tests-before.txt isn't stable across capture runs (docs/fixes/944-predictive-search-snippets/capture.sh:33)

• Repro: I ran capture.sh 3 times on a clean tree. In one run, tests-before.txt listed the failing spec files in a different order (undefined-object/index.spec.ts came before TypeSystem.spec.ts), a 5+/5- line diff. The other two runs were byte-identical to HEAD. The order follows vitest's completion order across the shared/isolated projects.
• Fix: make the order deterministic, e.g. run each spec file separately and append in a fixed order, or sort the ❯ |project| file blocks in strip(). Alternatively, say "byte-identical apart from file order" in the body.

2. nit: capture.sh overwrites the captures before it finds out matplotlib is missing, then refuses to rerun (capture.sh:16-19, :40)

• Repro: run it with a python3 that has no matplotlib. The .txt and .tsv files are rewritten, then render_capture.py fails with ModuleNotFoundError. The tree is now dirty, so the next run exits with "tracked files have uncommitted changes".
• Fix: check "${PYTHON:-python3}" -c 'import matplotlib' before the first git checkout, next to the dirty-tree check.

3. nit: the check-side "parity" test is a tautology (packages/theme-check-common/src/checks/undefined-object/index.spec.ts:406-410 vs contextual-objects.ts:56-60)

• The test asserts THEME_BLOCK_AND_SNIPPET_CONTEXTUAL_OBJECTS == ['app', 'block', ...SECTION_CONTEXTUAL_OBJECTS], which is exactly how that list is defined, so the test can only fail if someone edits the definition itself.
• The real cross-package guard is the TypeSystem.spec.ts it.each (L786-835), which did fail under mutation (see below).
• Fix: none needed. Optionally rename it to "theme snippet/block list = app + block + section objects", and in docs/upstream-944-pr-body.md:20 say "the TypeSystem parity test" rather than "one parity test per side".

4. nit (carried from round 3, still open): the freeze test covers 2 of 4 lists, and "bundled docset" wording remains

• index.spec.ts:448 covers only SECTION_ and THEME_BLOCK_AND_SNIPPET_.
• The ls-before-after.png title still says "(bundled docset)". The body text now explains it correctly ("written by pnpm build, revision 40f966f").
• Fix: add APP_BLOCK_CONTEXTUAL_OBJECTS and CHECKOUT_LAYOUT_CONTEXTUAL_OBJECTS to the loop, and change the PNG title to "docset built by pnpm build".

Also a nit: PR body "Upstream-ready variant" links docs/upstream-944-pr-body.md by branch name (blob/fix/944-…), not by SHA. Every other link is pinned to 5ca4a59. Pin it to 63afdb0.

Verified

History and authorship

• Fork PR #2 head is 63afdb0c21017fb1524bb69f9a5ec8f1578d7860.
  • 9272f1a is an ancestor (no rewrite).
  • The new commits are 11c5042, 0373749, 5ca4a59 and 63afdb0 (on top of b2efdc4).
• The upstream branch is at eace32587b81ba6d7db78683553db3aae0a87daa.
  • 4ba504e is an ancestor (no force-push).
  • Against Shopify main 20f8edf it is ahead 2 / behind 0, with 8 files (+480/−75).
  • 11c5042 and eace325 have the same git patch-id (a40adff…), so it is a clean cherry-pick. All 8 files are blob-identical to the fork head.
• Every commit since 9272f1a and since upstream/main is authored and committed by Arman Doğru <53595807+arman-dogru@users.noreply.github.com>. No Co-authored-by or tool trailers. No #NNN in the new commit messages.
• Upstream:
  • gh pr list -R Shopify/theme-tools --author arman-dogru --state all returns [].
  • Upstream Shopify#944's timeline has no events after 13:35 UTC, so nothing from this round.

Wording (potential availability)

• contextual-objects.ts (doc block: "potentially", "This is not a guarantee", plus the layout example), the UndefinedObject comment (index.ts:236-242), the TypeSystem.ts comment (L366-372), the changeset (L8, L10), the PR #2 body ("Known limitation", rule heading) and the upstream body (L16, L18, L20) all describe potential availability and say only the definitions are shared. The startsWith/regex split is listed as a follow-up (upstream body L33).
• The production code diff since 9272f1a is comments only. Behaviour is identical to round 3, so the round-3 Dawn 9/9 and Horizon 6/6 still hold.
• Changeset: minor for theme-check-common (4 new public exports) and patch for theme-language-server-common. Correct.

Tests

Claim	Measured
3 specs on base sources (ee5ab68 fork / 20f8edf upstream)	12 failed | 184 passed | 1 skipped on both
3 specs on the branch	196 passed | 1 skipped on both
Whole repo, fork, Node 20.19.2	268 files (+1 skipped), 7182 passed, 5 skipped, after removing the 6 codemirror-language-client/dist/esm/*.spec.js build copies. With them it is 7209 (= 7182 + 27).
Whole repo, upstream branch, Node 24.22.0	272 files (+1 skipped), 7192 passed, 5 skipped
Package counts	theme-check-common 121 files / 5368. theme-language-server-common 75 files / 655 + 1 skipped. These match the body.
pnpm -r type-check	exit 0 on both
prettier (packages/*/src/**/*.ts, docs/fixes/**/*.mjs, changeset)	clean on both

• Mutation M1: theme-mode snippets/blocks set back to main's ['app','section','block'] in both index.ts and TypeSystem.ts. 12 fail, including:
  • both nested inference tests and the nested completion test;
  • the known-limitation test;
  • the app-mode test's theme-contrast assertion.
• Mutation M2: app mode given the theme list. 3 fail: the app-mode inference, completion and check tests. So the app-mode negatives are real guards.

Repro and visual proof

• With matplotlib 3.11.2, 2 clean capture.sh runs were byte-identical to each other and to HEAD for all 8 artifacts (both PNGs, both TSVs, 4 txt). See nit 1 for a third run.
• theme-check-before.txt has 12 offenses and -after.txt has 5: 6 fixed, layout-rendered-search.liquid:9:21 accepted, and 5 true positives remain.
• before-after.png (1247×686) and ls-before-after.png (1464×1129) are both legible.
  • The amber "now accepted (known limitation)" row and the "before 12 / after 5" footer are visible.
  • In the LS capture, C1-C7 and H1-H2 changed, and K1-K3 are unchanged.
• The repro theme has the layout-only snippet and the {% render %} from layout/theme.liquid.
• The test claims match the code: exact ['articles','products'] / ['title'], {kind:'array', valueType:'product'} → product → string, and app mode unknown/untyped/unknown. The #L anchors at 5ca4a59 point to the right tests.

Bodies and hygiene

• Upstream body:
  • The AI disclosure is still at L8.
  • It says Refs #944 (not "Fixes").
  • The hidden draft note correctly names 4ba504e + eace325 on 20f8edf.
  • Both "Drag in" slots point to existing PNGs at 5ca4a59.
• The PR #2 body uses code spans for upstream refs, so nothing new cross-links.
• No secrets, tokens or local paths in either diff, and no stray or log files. The upstream branch has no docs/ files.

Upstream branch eace325 is ready to open (squash merge, CLA signature needed).

[arman-dogru]

Peer Reviewer: APPROVED (ccbb1bc)

Round 5. This checks the round-4 nit fixes. I used a fresh clone at /workspace/review/theme_tools_pr2_r5 and ran every git/gh command without GITHUB_TOKEN.

Result: 0 blocker, 0 major, 0 minor, 1 nit. All 4 round-4 nits are fixed and I checked each one, plus the unpinned link. All claimed numbers reproduce.

Findings

1. nit: capture.sh hides a crash in sort-vitest.mjs. At docs/fixes/944-predictive-search-snippets/capture.sh:39, the pipeline is npx vitest run … | strip | node $D/sort-vitest.mjs > $D/tests-$1.txt || true. The || true is there so the expected red run on base doesn't stop the script. But it also swallows a crash in the sort step, so the script exits 0 and leaves an empty or partial capture.

• Repro: bash -c 'set -euo pipefail; printf "x\n" | node -e "process.exit(3)" > out.txt || true; echo continues' prints continues and leaves out.txt at 0 bytes.
• Fix: write vitest's output to a temp file first (… | strip > $tmp || true), then run node $D/sort-vitest.mjs < $tmp > $D/tests-$1.txt without || true. Optional, and it doesn't block.

Round-4 nits: verified fixed

1. Capture order. capture.sh now pipes the test output through sort-vitest.mjs.
   • I ran capture.sh 3 times in a row (PYTHON = a matplotlib 3.11.2 venv). Every run exited 0 with 0 dirty files. All 8 artifacts were byte-identical across the 3 runs and to HEAD.
   • I also fed the sorter shuffled input. With the per-file summary blocks reversed, and separately with the FAIL detail blocks moved as whole-file groups, it reproduced the committed tests-before.txt byte for byte.
2. matplotlib pre-check (capture.sh:18-21, before the dirty-tree check and any checkout).
   • With python3 lacking matplotlib, the script exits 1 with "python3 has no matplotlib", leaving 0 dirty files.
   • With PYTHON=/nonexistent/python, it also exits 1 with 0 dirty files.
3. Check-side test (index.spec.ts:408). It's renamed, pinned to literal arrays, and commented as "not a cross-package parity guard".
   • Mutations: removing comment from SECTION_CONTEXTUAL_OBJECTS → 4 tests fail, including this one. Swapping app and block → only this test fails.
   • The PR body (L127, L134), the mermaid label (L57) and the upstream body L22 all name the TypeSystem parity test as the guard.
4. Freeze test and wording.
   • The freeze test (index.spec.ts:463) covers all 4 lists, checking isFrozen and that push throws TypeError. Unfreezing each list in turn fails this test and only this test, 4 of 4 times.
   • The ls-before-after.png title reads "Language server (docset built by pnpm build)", and it's legible.
   • The body link to docs/upstream-944-pr-body.md is pinned to ccbb1bc.

History, authorship and upstream

• Fork PR #2: head is ccbb1bca9681e60e399dac2410b51cdfb788c64b, and 63afdb0 is an ancestor. The new commits are 344ba1c, 2c78f08, a9e3927, d2faf9c and ccbb1bc.
• Upstream branch: at 02052d7c6558de8fc2665f85ade33502c2a57377, and eace325 is an ancestor.
  • Against Shopify main 20f8edf: ahead 3 / behind 0, 8 files, +503/−75.
  • 344ba1c and 02052d7 have the same patch-id (67bd820…).
  • All 8 files are blob-identical to the fork head.
  • The only file this round touches on the upstream branch is index.spec.ts. There are no docs/ files on it.
• Authorship: every new commit is authored and committed by Arman Doğru <53595807+arman-dogru@users.noreply.github.com>. No commit message contains # (so no #944), and there are no Co-authored-by or tool trailers.
• Upstream: gh pr list -R Shopify/theme-tools --author arman-dogru --state all returns []. The last event on upstream Shopify#944 is still from 2026-10-21 13:35 UTC, so nothing new.

Numbers

Claim	Measured
3 specs on base sources (ee5ab68 fork / 20f8edf upstream)	12 failed | 184 passed | 1 skipped on both
3 specs on the branch	196 passed | 1 skipped on both
Fork, whole repo, Node 20.19.2, no build output	268 files (+1 skipped), 7182 passed, 5 skipped
Fork after pnpm build	274 files, 7209 passed, 5 skipped (+27)
Upstream branch, whole repo, Node 24.22.0, no build output	272 files (+1 skipped), 7192 passed, 5 skipped
Upstream branch after pnpm build	278 files, 7219 passed, 5 skipped (+27)
pnpm -r type-check	exit 0 on both
prettier (packages/*/src/**/*.ts, docs/fixes/**/*.mjs, changeset)	clean on both

Both bodies explain the 7182 vs 7192 difference (main added 4 spec files and 10 tests after ee5ab68), and that matches the figures above.

Links and bodies

• I extracted every URL from both bodies: 37 unique, all HTTP 200. 36 of them are fork URLs pinned to a full SHA (d2faf9c or ccbb1bc). Developer said 34; the difference is only in how they're counted.
• The #L anchors at d2faf9c land on the named tests (L365, 386, 396, 408, 428, 445, 463, 476).
• The upstream body still has:
  • the AI disclosure at L10;
  • Refs #944;
  • a draft note listing all 3 commits;
  • both "Drag in" slots pinned to d2faf9c.
• No secrets or local paths in the diff, and no noise files.

Upstream branch 02052d7 is ready for Arman to open (upstream squash-merges; CLA signature needed).

[arman-dogru]

Peer Reviewer, round 6 at 6ef1006: APPROVED (0 blocker, 0 major, 0 minor, 0 nit)

These two commits (e0da659, 6ef1006) fix the round-5 nit, where capture.sh:39 used || true and so also hid a sort-vitest.mjs crash. Only docs/fixes files changed (capture.sh +18/-2, README +1/-1). Production code is untouched since ccbb1bc.

What I ran (Node 20, fresh run of the script on my machine):

• Clean run at 6ef1006: exit 0, and git status is empty afterwards. So all regenerated captures, including both PNGs, are byte-identical to the committed ones.
• Failure injection: I made a local, never-pushed commit that adds process.exit(3) to the top of sort-vitest.mjs, then ran the script. It exits 3, prints capture.sh: failed (exit 3); restored docs/fixes/944-predictive-search-snippets to HEAD, leaves no modified or untracked files, and restores the check/TypeSystem sources.
• The script still runs with set -euo pipefail, so a strip failure in the pipe also aborts the run now. Only vitest's own exit status is ignored, and the README row says exactly that.
• Both commits are authored as Arman Doğru with the noreply address.

Not checked this round: the box-only upstream branch, which these docs-only commits don't touch as far as I can tell. Nothing blocks the upstream submission from my side.