auto-fix batch 2026-04-30 (CI gates + safe DOM swap)#498
Merged
Conversation
eslint.config.js banned waitForTimeout but no runner enforced it. Add `lint` script to package.json and dedicated `lint` job to ci.yml. placement: ci.yml not e2e.yml. - e2e.yml's npm install hides inside scripts/setup-e2e.sh after the 30-min just test-e2e-full step starts, no dep-install reuse to win. - ci.yml runs parallel jobs on every PR, lint surfaces in seconds. local verify: npm ci + npm run lint -> exit 0 (existing per-file eslint-disable headers tracking #458 keep tree green). Refs #491
scripts/check-no-test-hooks-in-prod.sh asserts prod trunk build does not leak WillowTestHooks (third-party JS could otherwise read DAG heads via window.__willow.snapshot). Previously only run by `just check-all` -> never enforced on PRs / deploys. ci.yml: dedicated `test-hooks-guard` job, parallel with clippy/test/ wasm/browser. fail-fast on PRs. deploy.yml: belt-and-braces re-run immediately before `Build web app`, catches force-pushed bypass past the CI gate. pattern A (dedicated job) over B (`just check-all` in matrix): - B drags `just test-browser` (Firefox + geckodriver + wasm-pack) into the same job and balloons wall-clock; browser tests already run via the existing `wasm-pack test` job. - matches the sibling 22fc51b eslint wiring (parallel job, fail-fast). action pins copied verbatim from existing ci.yml jobs (full SHAs, trunk@0.21.14 matching deploy.yml). no new action authors -> no dependabot surface bump. note: script uses `trunk build --release --offline`, so the job warms trunk's tool cache (wasm-bindgen-cli) with a non-`--offline` build first; deploy.yml's existing `Build web app` step is now preceded by the guard, so the guard's own trunk build doubles as the cache-warm there. local verify: trunk + just not in sandbox; script aborts at first trunk invocation (exit 127), confirming fail-loud rather than silent-pass on missing toolchain. YAML validated via `python3 -c "import yaml; yaml.safe_load(...)"`. cargo fmt / clippy untouched (workflow YAML only). Refs #490
The `on_pinned_jump` callback in `crates/web/src/app.rs` interpolated `msg_id` into a `js_sys::eval(format!(...))` string with only single- quote stripping as sanitization. Reachable risk was low (today the id is `EventHash::to_string()`, hex-only), but band-aid sanitization rots and the pattern invites copy-paste. Swap for `web_sys::window() → document() → get_element_by_id() → scroll_into_view_with_scroll_into_ view_options(...)` so adversarial ids become literal lookup keys with no JS context to escape. The other `js_sys::eval` sites in the web crate (theme bootstrap, focus helpers, relay-url probe) all use static JS strings with no interpolation and are out of scope for this fix; the `unsafe-eval` CSP directive comment in `crates/web/tests/static_assets.rs` already tracks them under #171 / #425. Test coverage at the lowest tier covering DOM behavior — wasm-pack browser tests in `crates/web/tests/browser.rs::pinned_jump_safe_scroll`: realistic hex id (positive), missing id (negative, was implicit via old `.ok()`), and adversarial ids with double-quotes / backslashes / newlines / brackets (the cases the old `replace('\'', "")` band-aid did not cover). web-sys 0.3.95 already exposed `ScrollIntoViewOptions` with the setter-style API the issue body proposed (`set_behavior`, `set_block`), and the required features (`Window`, `Document`, `Element`, `ScrollIntoViewOptions`, `ScrollBehavior`, `ScrollLogicalPosition`) were already enabled in `crates/web/Cargo.toml` — no API adaptation, no feature additions needed. Verified: `cargo fmt --check` clean, `cargo check --target wasm32-unknown-unknown -p willow-web [--tests]` clean, `cargo test -p willow-web --lib` passes (73 tests). Browser tests compile-checked via `cargo check --target wasm32-unknown-unknown -p willow-web --tests`; not run locally (sandbox lacks wasm-pack/Firefox/geckodriver) — CI will execute the headless run. Pre-existing clippy warnings in `day_separator.rs` are unrelated and on master HEAD; not touched. Refs #425
native clippy + wasm `cargo check` together miss wasm-only lints. seen this batch in day_separator.rs (3x `as u32` on `js_sys::Date::get_month()` which already returns u32 on wasm). filed as #497. add explicit wasm-clippy step to step 7 gate so future implementers don't repeat. Refs auto-fix batch claude/friendly-maxwell-MVZZK
Owner
Author
|
resolve conflicts |
Conflict in crates/web/tests/browser.rs: both branches appended new test modules at end of file. #425 added `mod pinned_jump_safe_scroll` (DOM lookup tests for the eval -> safe-scroll swap); #496 added `mod data_state_lifecycle` (transitionend lifecycle tests on grove_drawer). Modules are orthogonal — kept both, ours first then theirs, with the same blank-line separator main used. verified: cargo fmt clean, cargo check --target wasm32-unknown-unknown -p willow-web --tests clean, cargo clippy -p willow-web --all-targets -- -D warnings clean.
This reverts commit b34d820. Reverting because the new test-hooks-guard CI job hits a pre-existing wasm-streams duplicate-symbol link error (issue tracked by PR #387): rust-lld: error: duplicate symbol: __wbg_intounderlyingbytesource_free rust-lld: error: duplicate symbol: __wbindgen_describe_intounderlyingbytesource_* ... Two transitive crates pull conflicting versions: wasm-streams 0.4.2 <- server_fn 0.7.8 <- leptos 0.7.8 wasm-streams 0.5.0 <- reqwest 0.13.2 <- iroh-relay Trunk's prod build (cargo build --release --target wasm32-unknown-unknown on the bin target) fails to link. The existing 'browser' CI job hits the same bug but masks it via 'wasm-pack test ... | tee' under bash -e without pipefail (tee's exit 0 swallows wasm-pack's failure). My new step is a single command with no pipe -> bash -e propagates the failure -> CI red. Won't mask via tee; per CLAUDE.md 'No swallowing errors'. #490 has to wait for #387 (leptos 0.7 -> 0.8 upgrade collapses the wasm-streams duplication) to land. Filing follow-up issue noting the dependency chain. Leaving the eslint CI wiring (#491) and the js_sys::eval safe-DOM swap (#425) in place; both are unaffected by the wasm-streams bug. Refs #490, #387
intendednull
pushed a commit
that referenced
this pull request
Apr 30, 2026
… wasm clippy CI gate caveman go: get_month already u32 on wasm32. cast bad. clippy yell. fix yell. three sites swept in crates/web/src/components/message_row/day_separator.rs: - ts_m: drop `as u32` - now_m: drop `as u32` - y_m: drop `as u32` comment block above also tweaked to reflect that cast is no longer the NaN-collapse vector for `get_month` / `get_date` (it's the implicit u32 return that does the collapsing now); behaviour identical, doc just true. CI guard wired per issue Option 1 (no extra job, just a step in the existing wasm job). add `components: clippy` to the toolchain action + new step `cargo clippy --target wasm32-unknown-unknown -p willow-web --all-targets -- -D warnings` after the existing wasm cargo check. won't hit the wasm-streams duplicate-symbol blocker that reverted PR #498's CI guard: clippy is check-style, never links the wasm binary, so the duplicate-symbol link error can't fire here. verified locally — `cargo clippy --target wasm32-unknown-unknown -p willow-web --all-targets -- -D warnings` finishes clean, zero warnings. Refs #497
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
2 participants
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.
Auto-fix batch run via
/resolving-issuesskill. Three audit-finding issues resolved sequentially on this branch — single PR, single CI run, single human review.Fixes
lintjob inci.yml,lintscript inpackage.json). Closes the gap whereeslint.config.js'sno-restricted-syntaxban onwaitForTimeoutwas never enforced. Commit22fc51b.scripts/check-no-test-hooks-in-prod.shinto CI + deploy (dedicatedtest-hooks-guardjob inci.yml, belt-and-braces step indeploy.yml). Closes the gap where the symbol-leak guard was wired only into localjust check-all, leaving prod ship paths unprotected against an accidentaldefault = ["test-hooks"]flip. Commitb34d820.js_sys::eval(format!(...))pinned-message scroll withweb_sysDOM lookup +ScrollIntoViewOptions. Removes band-aid single-quote sanitization, eliminates the eval-injection surface, and adds three wasm-pack browser tests covering positive / missing / adversarial msg ids. Commitf1c07a1.Already-Fixed
None this run.
Parked
None this run — all three attempted issues landed.
Skill Evolution
6095f27 docs(skill): wasm-clippy gate when dual-target lib crate touched— adds an explicitcargo clippy --target wasm32-unknown-unknown <scope> --all-targets -- -D warningsstep to the implementer's local merge gate. Surfaced this run because pre-existing wasm-only clippy errors incrates/web/src/components/message_row/day_separator.rs(3× unnecessaryas u32onjs_sys::Date::get_month()) only fire on the wasm target — native clippy + wasmcargo checkmiss them together. Filed as separate follow-up #497 (out of scope for this batch; CI doesn't currently gate on wasm-clippy either, so it's not blocking).Lessons Learned
just check-allmatrix step). [F1] test-hooks symbol-leak guard not wired into CI / deploy workflows #490's implementer cited22fc51bdirectly when picking — sibling consistency emerged organically without coordinator nudging.ci:-titled commit editedeslint.config.js/justfilebut never.github/workflows/. Worth keeping that pattern explicit in the audit's standard sweep.ci.ymljobs verbatim — zero new action authors.git addsomeone else's WIP — wait for the implementer's commit + push, then verify. Not a skill defect, just a coordination artifact between async agents and the harness's stop hook. Kept this run by explicitly explaining the state in the user-visible message after each hook fire.as u32casts (wasm32-only) #497 surfaced a wasm-only lint that neither native clippy (CI's gate) nor wasmcargo check(CI's gate) catches. Fixed via skill edit6095f27— the wasm-clippy step is now in the implementer's local gate. CI still doesn't gate on wasm-clippy; that's a separate concern tracked in [wasm-clippy] day_separator.rs: 3 unnecessaryas u32casts (wasm32-only) #497.Test plan
CI on this PR runs the full gate:
fmtjob —cargo fmt --checkworkspace-widelintjob — new from [F2] eslint config bans waitForTimeout but no CI step or npm script runs eslint #491, runsnpm run lintagainste2e/. Should pass: existing per-fileeslint-disableheaders (tracking e2e: migrate remaining specs to event-based waits #458) keep the tree green; the rule is in place to fail-fast on any futurewaitForTimeoutaddition.clippyjob —cargo clippy --workspace --all-targets -- -D warnings(native target). Pre-existing pass.test-hooks-guardjob — new from [F1] test-hooks symbol-leak guard not wired into CI / deploy workflows #490, runstrunk build --release+scripts/check-no-test-hooks-in-prod.sh. Should pass:crates/web/Cargo.toml'sdefault = []keepstest-hooksoff in prod builds.wasmcheck job — pre-existing pass.browserjob — runswasm-pack test --headless --firefox crates/webagainst the newpinned_jump_safe_scrollbrowser tests from [WS-1] Web: js_sys::eval(format!()) for pinned-message scroll uses band-aid sanitization #425 (positive/negative/adversarial msg-id cases). Compile-checked locally; full headless run is CI's responsibility.e2eworkflow — pre-existing pass.deployworkflow gate — new from [F1] test-hooks symbol-leak guard not wired into CI / deploy workflows #490, runs the symbol-leak guard immediately before the prod build on the deploy job (only fires on the deploy event, not on this PR).Manual smoke: open a server with pinned messages, click a pinned message in the pin sheet, confirm scroll-to-message still works (the behavior should be identical to the old
eval-based path).Generated by Claude Code