rastrillo / fiesta Public

Whole-branch review by Astra (gpt-6-astra, reasoning high, via codex exec, read-only), four rounds. Each round: the prompt, the whole reply, and what was done.

Round 1 (tip 36aa579)

Prompt

> Whole-branch review, read-only. Do not edit files, run browser tests or make ci. Repository /home/paulca/amadan.net/rastrillo/fiesta-fiesta-1-1, branch fiesta-1-1, tip 36aa579, base origin/main (git log origin/main..36aa579; git diff origin/main...36aa579). This is Fiesta 1.1.0: the column footer row (rst-page-foot), the Row Rule in DESIGN.md with its check (internal/rowrule), rst-snippet with js/copy.js, rst-side / rst-side-nav, seg-tabs buttons, opt-in rst-stats="pairs" and rst-status="inline", the 1.1.0 release candidate in releases/1.1.0, and the landing site (site/) dropping its own copies in favour of these. > Spec: /home/paulca/amadan.net/rastrillo/rastrillo/.claude/worktrees/fiesta-design/docs/superpowers/specs/2026-10-29-fiesta-1-1-design.md. Plan: /home/paulca/amadan.net/rastrillo/rastrillo/.claude/worktrees/fiesta-design/docs/superpowers/plans/2026-10-29-fiesta-1-1.md (with its review tables and controller notes). Task reports and earlier per-task reviews: /home/paulca/amadan.net/rastrillo/rastrillo/.claude/worktrees/fiesta-design/.superpowers/sdd/2026-10-29-fiesta-next/fiesta-1-1-task--report.md and -review.md (per-task findings were fixed; do not re-raise them unless the fix is wrong). > Look hardest at: > - the /1/ promise: does any 1.1 rule change how unchanged 1.0 markup renders (TestEveryOneOhDeclarationStillApplies, TestEveryNewRuleRequiresNewMarkup and its grammar in check/selectors_test.go, the 1.0 photographs); can the gates be fooled. > - releases/1.1.0 matches what css/, js/, i18n/, icons/ build now; releases/1.0.0 untouched; Version, CHANGELOG, README agree. > - copy.js: correctness, announcements, races, the no-clipboard and insecure-context paths, both spellings. > - accessibility: names, focus, keyboard, RTL, forced colours, print, reduced motion, in the new components and on the landing page (including the theme bar's icon-only scheme menu below 561px). > - the site: site.js/site.css after the drop, the landing tests' selectors, links. > - tests that cannot fail, weights/roles (TestEveryTestRunsInExactlyOneRole), the internal/rowrule package's API and its x/net dependency (govulncheck). > - comments that restate code instead of saying why; plan ids or task numbers in comments. > Output: verdict (Ready to land / Ready after fixes / Not ready), numbered findings with severity (Critical/Important/Minor), file:line, the failure scenario, a one-line fix. Then anything you checked and found sound, briefly.

Reply

Ready after fixes.

1. Important — check/baseline_test.go:230: quoted comment markers can bypass both compatibility gates. stripCSSComments ignores CSS strings. Inside a 700–750px media query, put [rst-page] { padding-inline: 3rem; } between new snippet rules containing content: "/*" and content: "*/". The reader removes the intervening rule and sees only a valid snippet addition. Browsers apply the page padding, and none of the photographs covers that width. Confirmed the reader’s reduction in memory. Fix: use a string-aware comment scanner and add this mutation as a rejection control.

1. Important — Makefile:285: the published-link gate reports success when export fails. If git archive or extraction fails—for example, with an invalid SITE_REF—the && chain skips the test, but cleanup still ends with exit $status, which remains zero. Reproduced using the unchanged recipe with failing commands stubbed; it returned success without checking links. Fix: propagate export/extraction failures explicitly and put cleanup in a trap.

Checked and found sound:

• All 57 generated release files match the source reconstruction, including checksums. The 1.0 release, frozen baseline, examples and existing scripts match v1.0.0.
• The shipped CSS additions preserve old declarations and keep the phone layouts opt-in; selector widenings remain narrowly scoped.
• copy.js passed in-memory checks for success/reset, unavailable clipboard, synchronous denial, and stale rejection after newer success. Its announcement timing, translations and both spellings look sound.
• The phone scheme menu retains its accessible name; button semantics, side-nav focus fixes, RTL, forced-colour, print and reduced-motion handling look sound by inspection.
• Landing selectors and release links are consistent. Row-rule tests remain covered by the root role; production code no longer calls html.Parse. This does not upgrade the vulnerable dependency.
• No new plan/task identifiers found in comments.

Read-only review completed. No Go/browser tests, make ci, or govulncheck run. Working tree unchanged.

Done

1: 6222049, one string- and escape-aware scanner behind every gate's comment stripping (discussion 33). 2: ef382e6, site-links-published fails on every failing step, cleanup in a trap (discussion 33).

Round 2 (tip ef382e6)

Prompt

> Round two, narrow, read-only. Do not edit files, run browser tests or make ci. Repository /home/paulca/amadan.net/rastrillo/fiesta-fiesta-1-1, branch fiesta-1-1, tip ef382e6. Your round-one review of 36aa579 is /home/paulca/amadan.net/rastrillo/rastrillo/.claude/worktrees/fiesta-design/.superpowers/sdd/2026-10-29-fiesta-next/fiesta-1-1-branch-review.md (two Important findings). The fixes are 6222049 and ef382e6 (git diff 36aa579 ef382e6); the implementer's report is fiesta-1-1-task-13-fixes-report.md beside it. > For each finding: Resolved / Partial / Not resolved, with file:line. Then: can the new scanner (cssSpan in check/markuphelp_test.go) and the three readers that use it still be made to disagree with a browser about which rules exist (strings, escapes, comments, url(), @-rules, nesting)? Is anything that 1.0 or the current css/ legitimately contains now wrongly refused? Does the Makefile recipe now fail on every failing step and still clean up? Is internal/build/strip.go's backslash gap (left alone; no shipped stylesheet contains a backslash) a real risk to the 1.1.0 release bytes? > Output: verdict (Ready to land / Ready after fixes / Not ready), then only new findings with severity, file:line, failure scenario and one-line fix.

Reply

Ready after fixes.

Round one:

• Partial — check/markuphelp_test.go:182: quoted-comment bypass fixed; escaped URLs still bypass the readers.
• Resolved — Makefile:293: export/extraction failures now fail the recipe and invoke cleanup.

No newly refused syntax found in 1.0 or current CSS. Unsupported at-rules and nesting fail closed in the declaration reader. The backslash gap in internal/build/strip.go:43 does not threaten the current 1.1.0 bytes: shipped stylesheets contain no backslashes, and release inputs/output are unchanged.

1. Important — check/markuphelp_test.go:182: escaped URLs still hide rules.

``css [rst-snippet] { --rst-a: \url(/*); } [rst-page] { padding-inline: 3rem; } [rst-snippet] { --rst-b: \url(*/ ); } ``

The scanner reduces this to [rst-snippet] { --rst-a: \url( ); }. The declaration, spelling and leaf-rule readers lose the page rule. CSS tokenization interprets \url as url, so the intervening rule remains active; u\72 l also bypasses detection. See https://www.w3.org/TR/css-syntax-3/#consume-ident-like-token.

Fix: consume complete escaped identifiers, including leading escapes and hexadecimal terminator whitespace, before enforcing the URL refusal; add both rejection controls.

1. Moderate — check/css_test.go:368: leaf-rule extraction still mistakes quoted braces for structure.

[rst-snippet] { --rst-x: "}"; cursor: pointer; } is accepted by the scanner, but leafRules truncates its body at the quoted }. Consequently, interactiveCSSRules misses the pointer declaration and omits the control from its inventory.

Fix: replace the brace regex with string- and escape-aware block extraction.

1. Minor — Makefile:292: cleanup failure still reports success.

If testing succeeds but rm -rf fails, the EXIT trap preserves status zero and leaves scratch behind. Confirmed with shell stubs; all substantive failing steps correctly return nonzero.

Fix: preserve the incoming failure status, but return nonzero when cleanup fails after an otherwise successful run.

Done

1: 1ab0dfc, the scanner refuses any backslash outside a string, closing escapes as a class; no Fiesta stylesheet contains a backslash. 2: dbd18a6, leafRules reads with the scanner. 3: 2ed4142, a failed cleanup exits non-zero (discussion 34).

Round 3 (tip 2ed4142)

Prompt

> Round three, narrow, read-only. Do not edit files, run browser tests or make ci. Repository /home/paulca/amadan.net/rastrillo/fiesta-fiesta-1-1, branch fiesta-1-1, tip 2ed4142. Your round-two review of ef382e6 is /home/paulca/amadan.net/rastrillo/rastrillo/.claude/worktrees/fiesta-design/.superpowers/sdd/2026-10-29-fiesta-next/fiesta-1-1-branch-review2.md (three findings, plus round one's partial). The fixes are 1ab0dfc, dbd18a6 and 2ed4142 (git diff ef382e6 2ed4142); the report is fiesta-1-1-task-13-fixes2-report.md beside it. The approach to escapes changed: the scanner now refuses any backslash outside a string rather than decoding escapes. > For each round-two finding and the round-one partial: Resolved / Partial / Not resolved, with file:line. Then new findings only if they are real: a way the readers still disagree with a browser about which rules or declarations exist, something 1.0 or the current css/ legitimately contains that is now refused, or a recipe that reports success on failure. The implementer flagged braceMatchEnd in check/css_test.go as having the same quoted-brace flaw but failing closed with a misleading message; say whether it can make a gate pass wrongly. > Output: verdict (Ready to land / Ready after fixes / Not ready), then numbered findings with severity, file:line, failure scenario and one-line fix.

Reply

Ready after fixes.

1. Important — Resolved: round-two finding 1; Resolved: round-one partial. check/markuphelp_test.go:187 now rejects both leading and embedded escapes outside strings. Escaped URLs cannot hide intervening rules. Quoted comment markers remain handled correctly. Fix remaining: none for these findings.

1. Moderate — Resolved: round-two finding 2. check/css_test.go:385 advances through complete strings, so quoted braces no longer truncate leaf rules or hide their pointer declarations. Fix remaining: none for this finding.

1. Minor — Resolved: round-two finding 3. Makefile:294 preserves an incoming failure and changes success to failure when cleanup fails. Export/extraction failures still propagate. Fix remaining: none.

1. Important — New: unmatched parentheses hide declarations from the compatibility gate. check/baseline_test.go:432.

``css :root { --rst-review-probe: ); color: red; } ``

depth becomes −1, so neither semicolon separates declarations. The reader records everything as one new custom property, which newRecordRefused:942 allows. A browser rejects the invalid custom property but applies color: red to existing pages. The unmatched closing token is forbidden by https://www.w3.org/TR/css-variables-1/#syntax.

Fix: reject unmatched closing parentheses and unclosed parentheses before accepting declaration records.

1. Moderate — New: braceMatchEnd can make the motion gate pass wrongly. check/css_test.go:325.

``css .ok { transition: opacity 1s; } @media (prefers-reduced-motion: reduce) { .ok { transition: none; } .marker { --rst-open: "{"; } } .missed { transition: opacity 1s; } .marker { --rst-close: "}"; } ``

The quoted braces balance across separate rules. The extracted reduce block includes .missed; the remaining stylesheet contains only .ok. The motion gate passes although .missed still transitions under reduced motion. Neither string is cut off, so the new leaf reader raises no error. The implementer’s “fails closed” assessment is therefore incomplete.

Fix: make braceMatchEnd skip complete strings with the shared scanner and propagate unmatched-block errors.

1. Moderate — New: quoted text supplies a fictitious motion override. check/css_test.go:467.

``css .moving { transition: opacity 1s; } @media (prefers-reduced-motion: reduce) { .moving { --rst-proof: "; transition: none;"; } } ``

motionPropertyPattern finds transition: none inside the string. The reduce index consequently satisfies the gate, although the browser has no disabling declaration.

Fix: derive motion declarations from the string-aware declaration reader instead of the regexp.

No newly refused syntax found in the 26 relevant stylesheets; none contains backslashes. Static review and in-memory traces only. No files changed, browser tests, or make ci.

Done

4, 5, 6: a92e384, one bracket reader on the scanner; braceMatchEnd and the motion gate moved onto it; then every CSS reader in check/ routed through the scanner and the declaration reader, which closed four further holes (discussion 35). Follow-up recorded, not in 1.1: internal/tokens reads :root the old way for the gallery's tokens page.

Round 4 (tip a92e384)

Prompt

> Round four, read-only. Do not edit files, run browser tests or make ci. Repository /home/paulca/amadan.net/rastrillo/fiesta-fiesta-1-1, branch fiesta-1-1, tip a92e384. Your round-three review of 2ed4142 is /home/paulca/amadan.net/rastrillo/rastrillo/.claude/worktrees/fiesta-design/.superpowers/sdd/2026-10-29-fiesta-next/fiesta-1-1-branch-review3.md (new findings 4, 5, 6). The fixes are git diff 2ed4142 a92e384: one bracket reader on the shared scanner, braceMatchEnd and the motion gate moved onto it, and a sweep moving every CSS reader in check/ onto the scanner and declaration reader. The report, with a table of every reader and its disposition, is fiesta-1-1-task-13-fixes3-report.md beside it. > 1. Findings 4, 5, 6: Resolved / Partial / Not resolved, with file:line. > 2. The sweep: did it weaken any gate? Compare old and new for each routed reader; look for a gate that now checks less than it did on the shipped stylesheets (the report says records are byte-identical on the 26 stylesheets and tokens.css motion now checks 34 declarations instead of 36). Check the rows left as they are. > 3. New holes only if real: a way a reader still disagrees with a browser so a gate passes wrongly, or something legitimate now refused. > Severity guide: Important only if it can let a change that alters how 1.0 markup renders, or ships a real accessibility or motion regression, pass the gates. Adversarial corners no author would write are Minor. > Output: verdict (Ready to land / Ready after fixes / Not ready), then numbered findings with severity, file:line, failure scenario and one-line fix.

Reply

Ready after fixes.

1. Important — Resolved. check/markuphelp_test.go:149 validates bracket nesting; check/baseline_test.go:426 also rejects stray closers when reading declarations directly. The unmatched-parenthesis probe can no longer hide color: red. Remaining fix: none.

1. Moderate — Resolved. check/markuphelp_test.go:175 advances through scanner spans and propagates bracket errors. Quoted braces cannot extend the reduced-motion block over .missed. Remaining fix: none.

1. Moderate — Resolved. check/css_test.go:581 obtains motion declarations from declarationsOf. Quoted transition: none no longer supplies an override. Remaining fix: none.

1. Important — Newly identified, pre-existing: a losing motion override still satisfies the gate. check/css_test.go:593.

The index remembers whether any reduced-motion declaration says none, without checking whether it wins:

``css @media (prefers-reduced-motion: reduce) { .rst-snippet, [rst-snippet] { transition: none; } } .rst-snippet, [rst-snippet] { transition: transform 0.2s; } .rst-snippet:hover, [rst-snippet]:hover { transform: translateY(-2px); } ``

The motion gate accepts both selectors, but the later transition wins under reduced motion. These new-markup selectors also satisfy the compatibility gate’s requirement. An active !important transition defeating a normal disable has the same problem. This survives the refactor; it is not a sweep regression.

Fix: require the reduced-motion disable to win for the matching selector/property, accounting for order and importance; reject contexts whose applicability cannot be established.

Across the routed readers and the rows left unchanged, I found no material weakening on the shipped stylesheets. The 36 → 34 count is correct: the old splitter manufactured two extra selectors from commas inside :has(); shell remains at eight.

The report’s leaf-rule equality needs qualification: the new reader omits descriptor blocks—eight @property blocks in tokens.css and @view-transition in shell.css. An in-memory comparison across all 33 tracked CSS files found matching style/keyframe leaves and declarations. Those descriptor omissions do not remove effective checks on the shipped files; the full declaration reader still records them. No newly refused syntax found in those files.

Static review and in-memory traces only. No files changed, browser tests, or make ci.

Done

7 (dates from 1.0): d023e75, a browser test is now the authority on reduced motion: all 168 example states, three themes, two widths, eight interaction states, nothing moves or fades; controls prove it sees a disable that loses and sees every shipped motion with reduced motion off. Shipped CSS leaked nothing, so css/ and releases/ are unchanged (discussion 36). The review converged on the branch's own changes; no fifth round, since the fix did not touch shipped CSS.