Repository navigation
Gate the template contract rules and fix the side-cart settings filter argument - #66
Merged
Merged
Conversation
The platform raises a 500 on every route once gift_product is set,
because settings.* cannot be used as a filter argument. Bind the
setting with {% with %} and pick the PK with {% firstof %} instead.
The data-gift-product-id attribute and the surrounding guard are
unchanged.
Refs #51 (item 4)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
check-templates.py now fails on three patterns a derived theme has
re-introduced after they were fixed:
- [settings-filter-argument]: settings.* on the right of a filter
(|default:settings.foo). The platform raises a 500 on every route
once the setting has a value. Proven against the pre-fix
partials/side_cart.html:37 before that line was rewritten.
- [firstof-object]: a {% firstof ... as X %} target later handed to
{% purchase_info_for_product %}. firstof always stores a string, so
X can never select a product object.
- [hardcoded-route]: a /products/ literal inside href or action.
Routes come from {% url %} or get_absolute_url.
theme-contract.json now also requires the runtime hooks the shipped JS
resolves by id or attribute (id="cart-badge", data-toggle="mobile-nav",
id="mobile-nav", <spark-cart-drawer). Removing any of them passed every
existing gate and failed only in the browser.
Refs #51 (item 3)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
design-block-authoring.md states what test_localization_contracts.py
enforces: a setting with a {% t %} fallback keeps an empty schema
default and empty settings_data, and starter design copy belongs on a
separate non-localized key.
The Figma mobile reference width is decided: match the Figma-defined
frame width (375 or 390) and capture QA at both. The plan and the
section roster now state it as a rule instead of an open choice.
Refs #51 (items 6 and 7)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
next-devin
marked this pull request as ready for review
September 21, 2026 08:30
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Incremental review (e707d54..06c367f, 4 commits)All four prior findings are addressed in this range:
The new logic is exercised by
No new issues found in the incremental range. Files Reviewed (8 files)
Previous Review Summaries (4 snapshots, latest commit e707d54)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e707d54)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 7f08660)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit f1aee22)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit dc9a35b)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Reviewed by minimax-m3 · Input: 0 · Output: 0 · Cached: 0 |
- settings-filter-argument: quoted string literals are blanked before matching, so "settings." inside a literal is text, not a lookup. - hardcoded-route: covers every platform storefront route root, not only /products/, and the message lists them. - firstof-object: the per-file scope is stated in the docstring and the message; dot-access on a firstof target stays flagged, with a test. - theme-contract.json: each runtime hook carries the release it first appeared in as its since, and the drawer needle is the closing tag so a differently named element cannot satisfy it by prefix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
/categories/ was never a storefront root: products and categories both resolve under /catalogue/, and accounts under /accounts/. The gate now lists the documented platform roots plus the foreign reflexes ported templates carry (/products/, /collections/, /account/), which 404 here. The test pins the message wording. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- hardcoded-route only matches a local absolute path, so an external URL that contains a route word passes. - firstof matching stops at the tag boundary, so a firstof with no as clause cannot claim the next tag's assignment. - The firstof check walks the file in order: calls before the firstof pass, and a with/for that rebinds the name shadows the string until its end tag, so the recommended remedy passes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A quoted default containing "as <word>" was read as a rebinding and could shadow a firstof string, hiding the defect the gate exists for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 21, 2026
Merged
4 tasks done
next-devin
added a commit
that referenced
this pull request
Sep 25, 2026
…oks bind Spark (#71) * feat: scope theme-contract requirements to the fleet or to Spark's own copy Every requirement in theme-contract.json now carries a required `scope`. `fleet` binds every Spark-derived store theme; `spark` binds Spark's own working copy only. check-theme-contract.py validates the field, enforces only fleet rules by default for a live theme (--store/--theme-id) and all rules by default for a working copy (--root), and takes --scope to override either. The gate's output names the scope and how many requirements ran, and a scope that selects nothing refuses rather than passing. `pixels` is the only fleet rule. The four runtime hooks #66 added (cart-badge, mobile-nav-toggle, mobile-nav, cart-drawer) are scoped to Spark: the first fleet sweep reported nine of thirteen live themes failing mobile-nav on the file rule while four of them served #mobile-nav from another file. Derived themes are forks; a file-location rule against the fleet reports forks, not faults. Tests cover both defaults, both overrides, the load errors, the empty selection, the block-override rule at fleet scope, and the exact invocation the next-mind fleet sweep makes, against a stub admin API that rejects the wrong path or key. * docs: document the two contract scopes and the invocation for each case theme-contract.md lists all five rules with their scope, explains why the runtime hooks bind Spark only, states the rule for promoting one to fleet, and shows the command for a fork's working copy, a derived live theme, and Spark's own copy on a dev store. The Makefile contract target's comment says the same. * chore: changelog entry and follow-up TODOs for contract scoping Unreleased entry for the scope field. TODOs for the three findings the pre-landing adversarial pass surfaced and this change does not take on: HTML-comment masking in the needle search, live-check transport hardening, and the fleet sweep passing --scope fleet explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #51, items 3, 4, 6 and 7. Items 1 and 2 already landed on main (#54, mobile-nav focus trap). Item 5, a Django-based DTL parse gate, is deferred and stays open on the issue.
What changed
partials/side_cart.htmlpassedsettings.gift_productas a|default:filter argument, which the platform 500s on once the setting is populated. It now binds the setting with{% with %}and picks the PK with{% firstof %}.scripts/check-templates.pygains three checks overlayouts/ templates/ partials/, somake verify-themeand the existing CI step pick them up with no wiring:settings.*as a filter argument, a{% firstof ... as X %}target later passed topurchase_info_for_product(firstof always yields a string), and hardcoded/products/routes inhref/action. Each has real-repo-passes, defective-fixture-fails (naming file and line), and legitimate-pattern-passes tests intests/test_ci_gates.py. The settings-filter check was proven against the pre-fix side-cart line before the fix was applied.theme-contract.jsonnow requiresid="cart-badge",data-toggle="mobile-nav",id="mobile-nav"and<spark-cart-drawer, each with awhyand a storefront verify snippet, reusing the Add a theme contract gate for derived themes #64 gate instead of a second mechanism. Deletingcart-badgepreviously passed every gate and failed only in the browser.docs/design-block-authoring.mdstates the ruletests/test_localization_contracts.pyenforces: localized settings keep empty defaults, design copy goes on a separate key. The mobile reference width is decided as "match the Figma-defined frame width, 375 or 390, QA at both" indocs/figma-section-library-plan.mdanddocs/section-roster.md.Verification
python3 -m unittest discover -s tests: 90 tests pass (78 on main)make verify-theme: css-check, tests, theme contract gate (62 templates) all passnode tests/js/*.test.js: 12 passReviewer notes
since; thepixelsentry says 1.3.0. Stamp the next release number if wanted.pixels. A derived theme that moves header markup to another partial fails with "does not contain".{% include ... with %}is not traced.🤖 Generated with Claude Code