Harden C++ toolchain pin and WebGPU device/surface policy - #457
Conversation
- Bump .emsdk-version to 5.0.7 (the line build-info.json shows actually linking emdawnwebgpu) and stop tracking build/cpp's machine-specific build-info.json/compile_commands.json/.o files — committing those is what caused the pin-vs-reality drift in the first place. - Reorder TETRIS_CPP_WEBGPU=auto to try --use-port=emdawnwebgpu before the legacy -s USE_WEBGPU=1 flag (removed upstream around emsdk 5.0), so a normal build no longer wastes a failed link attempt. - Add a release wasm size budget (256 KB, TETRIS_CPP_WASM_BUDGET_BYTES to override) enforced by build-cpp.mjs, and have CI assert wasmBytes/jsBytes are present in build-info.json. - gpuContext.ts: add requiredLimits (resolveRequiredLimits, clamped to the adapter) matching the renderer's actual compute usage, centralize GPUCanvasContext.configure() into buildCanvasConfiguration(), and wire GameSettings.gpuPower into requestGpuAdapterAndDevice() so the settings UI and ?gpu= resolve through one policy instead of two disconnected paths. - Add "type": "module" to package.json to quiet ESLint's MODULE_TYPELESS_PACKAGE_JSON warning; rename the one CommonJS script (scripts/screenshot.js) to .cjs to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NsXNcchXYG3RQU5hpoT5Ar
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThe PR pins Emscripten 5.0.7, changes WebGPU backend selection, adds release artifact and WASM-size validation, centralizes GPU device policies, updates documentation, and introduces Playwright screenshot automation. ChangesWebGPU build and runtime updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/README.md`:
- Line 312: Update the description adjacent to the RENDERER=webgpu-cpp
screenshot command in the README to identify the tooling as Playwright rather
than Puppeteer, matching the playwright import used by scripts/screenshot.cjs.
In `@scripts/screenshot.cjs`:
- Line 2: Add Playwright to the package manifest’s consumed dependencies, using
the existing package-manager conventions, and regenerate the lockfile so the
require in scripts/screenshot.cjs resolves after a fresh install.
- Around line 34-54: Make theme selection a hard failure in the loop around the
theme-switching click, rather than catching and logging click errors in
`page.click`. Remove the swallowing `.catch` behavior or rethrow after logging
so a failed `#${t.id}` selection stops the script before saving a screenshot;
preserve the existing capture logic for successfully selected themes.
In `@src/webgpu/gpuContext.ts`:
- Around line 74-108: Do not clamp renderer-required limits in
resolveRequiredLimits: validate that the adapter provides every value in
REQUIRED_GPU_LIMITS at or above the requirement, and reject insufficient
adapters before requestDevice(). In src/webgpu/gpuContext.ts lines 228-250,
ensure any retry removes only optional features/limits while retaining
requiredLimits. Update tests/gpu-context.test.ts lines 81-101 and 180-203 to
expect unsupported-adapter rejection rather than downgraded limits, and update
cpp/README.md lines 247-252 to document the rejection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1f90300-b871-4a11-bc1e-bcdffcd48a69
⛔ Files ignored due to path filters (3)
build/cpp/gpu_renderer.ois excluded by!**/*.obuild/cpp/playfield_draw.ois excluded by!**/*.obuild/cpp/renderer.ois excluded by!**/*.o
📒 Files selected for processing (13)
.emsdk-version.github/workflows/cpp-renderer.yml.gitignorebuild/cpp/build-info.jsonbuild/cpp/compile_commands.jsoncpp/README.mddocs/NATIVE_RENDERER_RESEARCH.mdpackage.jsonscripts/build-cpp.mjsscripts/screenshot.cjssrc/webgpu/gpuContext.tstests/build-cpp.test.tstests/gpu-context.test.ts
💤 Files with no reviewable changes (2)
- build/cpp/compile_commands.json
- build/cpp/build-info.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/README.md`:
- Line 312: Update the description adjacent to the RENDERER=webgpu-cpp
screenshot command in the README to identify the tooling as Playwright rather
than Puppeteer, matching the playwright import used by scripts/screenshot.cjs.
In `@scripts/screenshot.cjs`:
- Line 2: Add Playwright to the package manifest’s consumed dependencies, using
the existing package-manager conventions, and regenerate the lockfile so the
require in scripts/screenshot.cjs resolves after a fresh install.
- Around line 34-54: Make theme selection a hard failure in the loop around the
theme-switching click, rather than catching and logging click errors in
`page.click`. Remove the swallowing `.catch` behavior or rethrow after logging
so a failed `#${t.id}` selection stops the script before saving a screenshot;
preserve the existing capture logic for successfully selected themes.
In `@src/webgpu/gpuContext.ts`:
- Around line 74-108: Do not clamp renderer-required limits in
resolveRequiredLimits: validate that the adapter provides every value in
REQUIRED_GPU_LIMITS at or above the requirement, and reject insufficient
adapters before requestDevice(). In src/webgpu/gpuContext.ts lines 228-250,
ensure any retry removes only optional features/limits while retaining
requiredLimits. Update tests/gpu-context.test.ts lines 81-101 and 180-203 to
expect unsupported-adapter rejection rather than downgraded limits, and update
cpp/README.md lines 247-252 to document the rejection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1f90300-b871-4a11-bc1e-bcdffcd48a69
⛔ Files ignored due to path filters (3)
build/cpp/gpu_renderer.ois excluded by!**/*.obuild/cpp/playfield_draw.ois excluded by!**/*.obuild/cpp/renderer.ois excluded by!**/*.o
📒 Files selected for processing (13)
.emsdk-version.github/workflows/cpp-renderer.yml.gitignorebuild/cpp/build-info.jsonbuild/cpp/compile_commands.jsoncpp/README.mddocs/NATIVE_RENDERER_RESEARCH.mdpackage.jsonscripts/build-cpp.mjsscripts/screenshot.cjssrc/webgpu/gpuContext.tstests/build-cpp.test.tstests/gpu-context.test.ts
💤 Files with no reviewable changes (2)
- build/cpp/compile_commands.json
- build/cpp/build-info.json
🛑 Comments failed to post (4)
cpp/README.md (1)
312-312: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label this as Playwright, not Puppeteer.
scripts/screenshot.cjsimportsplaywright; update the adjacent command description to avoid sending users to the wrong tooling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/README.md` at line 312, Update the description adjacent to the RENDERER=webgpu-cpp screenshot command in the README to identify the tooling as Playwright rather than Puppeteer, matching the playwright import used by scripts/screenshot.cjs.scripts/screenshot.cjs (2)
2-2: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail jq '{dependencies, devDependencies}' package.json rg -n '"playwright"|"`@playwright/test`"' package.json package-lock.json npm-shrinkwrap.json 2>/dev/null || trueRepository: ford442/Tetris_WebGPU
Length of output: 582
Declare the Playwright dependency.
scripts/screenshot.cjsrequiresplaywright, butpackage.jsondoes not declare it under dependencies or devDependencies and no lockfile entry is present. A fresh install cannot run this script until Playwright is added to the package’s consumed dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/screenshot.cjs` at line 2, Add Playwright to the package manifest’s consumed dependencies, using the existing package-manager conventions, and regenerate the lockfile so the require in scripts/screenshot.cjs resolves after a fresh install.
34-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Locate script and inspect relevant lines" if [ -f scripts/screenshot.cjs ]; then wc -l scripts/screenshot.cjs cat -n scripts/screenshot.cjs | sed -n '1,140p' else fd -a 'screenshot\.cjs$' . fi echo echo "Search theme-related selectors/symbols" rg -n "canvaswebgpu|theme|themes|Switching theme|waitForTimeout|screenshot" . \ --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' \ --glob '!screenshots/**' || trueRepository: ford442/Tetris_WebGPU
Length of output: 50377
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Locate theme selector definitions" rg -n "neon-theme|pastel-theme|futuristic-theme|image-sampled-theme|`#neon-theme`|`#pastel-theme`|`#futuristic-theme`|id=.neon-theme|class=.neon-theme" . \ --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' \ --glob '!public/cpp/tetris_renderer.js' || true echo echo "Locate current screenshot capture code with failures in try/catch via JS read-only model" node - <<'JS' const script = ` (async () => { const themes = [ { id: 'neon-id', name: 'neon' }, { id: 'missing-id', name: 'missing' } ]; const page = { click: async (selector) => { if (selector.includes('missing-id')) throw new Error('selector not found') } }; const saved = []; for (const t of themes) { await page.click(\`#\${t.id}\`).catch((err) => { console.log(\`Warning: Could not switch to theme \${t.name}:\`, err.message); }); saved.push(\`screenshots/\${t.name}.png\`); } console.log(JSON.stringify(saved)); })(); `; eval(script.replace("console.log(", "")); JSRepository: ford442/Tetris_WebGPU
Length of output: 3014
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "theme/class selectors across tracked files" git ls-files | grep -v '/public/cpp/.*/tetris_renderer.js$' | xargs rg -n "id=[\"']neon-theme|id=[\"']pastel-theme|id=[\"']futuristic-theme|id=[\"']image-sampled-theme|className\s*=|body\.className|tetris_theme_class|theme.*selector|`#image-sampled-theme`|neon-theme|pastel-theme|futuristic-theme" || true echo echo "compare screenshot.cjs vs capture-screenshot.mjs theme IDs" for f in scripts/screenshot.cjs scripts/capture-screenshot.mjs; do if [ -f "$f" ]; then echo "--- $f" cat -n "$f" | sed -n '1,80p' fi done echo echo "static behavior probe" node - <<'JS' const state = { body: '', saved: [] }; const themes = [ { id: 'missing-id', name: 'neon' } ]; for (const t of themes) { let clickErr = null; const selectors = [`#${t.id}`]; const selector = selectors[0]; if (!selector) clickErr = Error('selector not found'); if (clickErr) { console.log(`Warning: Could not switch to theme ${t.name}:`, clickErr.message); } state.saved.push(`screenshots/${t.name}.png`); } console.log(JSON.stringify({ body: state.body, saved: state.saved })); JSRepository: ford442/Tetris_WebGPU
Length of output: 8846
Fail when theme selection cannot be applied.
page.click('#neon-theme'),page.click('#pastel-theme')andpage.click('#future')are ignored via.catch(...), but the loop still savesscreenshots/<theme>.pngand exits successfully. Make theme selection a hard failure or switch by an existing control before capturing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/screenshot.cjs` around lines 34 - 54, Make theme selection a hard failure in the loop around the theme-switching click, rather than catching and logging click errors in `page.click`. Remove the swallowing `.catch` behavior or rethrow after logging so a failed `#${t.id}` selection stops the script before saving a screenshot; preserve the existing capture logic for successfully selected themes.src/webgpu/gpuContext.ts (1)
74-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the WebGPU specification, what happens when GPUAdapter.requestDevice() receives requiredLimits larger than adapter.limits, and can a device requested with lower limits validly run shaders that require higher limits?💡 Result:
According to the WebGPU specification, if you call GPUAdapter.requestDevice with a requiredLimits object containing values that exceed the adapter's supported limits, the request will fail [1][2]. Specifically, the returned Promise will reject with an OperationError because the requested limits are not supported by that adapter [1][2]. Regarding the second part of your query, a device requested with lower limits cannot validly run shaders that require higher limits [3][4]. WebGPU enforces strict validation based on the specific limits determined at the time of the device's creation [3][5]. Once a GPUDevice is created, it operates only within the exact set of limits you requested (or the default limits if none were specified) [4]. Even if the underlying hardware (the adapter) supports higher values, the API will not allow you to exceed the specific limits defined for that device [3][4]. Therefore, if a shader requires resources that exceed the device's assigned limits, that shader will fail validation [3][5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/GPUAdapter/requestDevice
- 2: https://docs.w3cub.com/dom/gpuadapter/requestdevice
- 3: https://www.w3.org/TR/2025/CRD-webgpu-20250613/
- 4: https://www.w3.org/TR/2026/CRD-webgpu-20260109/
- 5: https://www.w3.org/TR/webgpu/
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate files ==" git ls-files | rg '(^src/webgpu/gpuContext\.ts$|^tests/gpu-context\.test\.ts$|^cpp/README\.md$)' || true echo "== outline gpuContext ==" ast-grep outline src/webgpu/gpuContext.ts --view compact || true echo "== relevant gpuContext sections ==" sed -n '1,140p' src/webgpu/gpuContext.ts echo "---" sed -n '200,270p' src/webgpu/gpuContext.ts echo "== relevant tests ==" sed -n '70,110p' tests/gpu-context.test.ts echo "---" sed -n '165,210p' tests/gpu-context.test.ts echo "== README relevant ==" sed -n '238,260p' cpp/README.mdRepository: ford442/Tetris_WebGPU
Length of output: 13399
Do not downgrade renderer-required WebGPU limits.
Clamping required limits below
REQUIRED_GPU_LIMITScan create a device with capabilities that don’t match the renderer’s compute/storage-buffer requirements, leading to validation failures later. Reject the adapter when required limits are unavailable, and keep the fallback for optional features only:
resolveRequiredLimits(): returnREQUIRED_GPU_LIMITSunchanged or reject insufficient adapters beforerequestDevice().- Retry path: request without optional features/limits only; retain
requiredLimitsif they are a hard renderer requirement.- Tests/docs: update clamping expectations to assert unsupported-adapter rejection.
📍 Affects 3 files
src/webgpu/gpuContext.ts#L74-L108(this comment)src/webgpu/gpuContext.ts#L228-L250tests/gpu-context.test.ts#L81-L101tests/gpu-context.test.ts#L180-L203cpp/README.md#L247-L252🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/webgpu/gpuContext.ts` around lines 74 - 108, Do not clamp renderer-required limits in resolveRequiredLimits: validate that the adapter provides every value in REQUIRED_GPU_LIMITS at or above the requirement, and reject insufficient adapters before requestDevice(). In src/webgpu/gpuContext.ts lines 228-250, ensure any retry removes only optional features/limits while retaining requiredLimits. Update tests/gpu-context.test.ts lines 81-101 and 180-203 to expect unsupported-adapter rejection rather than downgraded limits, and update cpp/README.md lines 247-252 to document the rejection behavior.
2 participants
Summary
Toolchain/device-policy hardening ahead of further C++ and WebGPU work, per the 2026-07-22 audit:
.emsdk-versionbumped from4.0.10→5.0.7— the line the committedbuild/cpp/build-info.jsonshowedemccactually linkingemdawnwebgpuwith. That stale, machine-specific artifact (pluscompile_commands.jsonand.ointermediates) is now untracked and gitignored — committing non-portable build metadata is what caused the "pin says X, local build says Y" confusion in the first place. Bumping the pin also busts the CI emsdk cache key automatically (no manual step needed).autoWebGPU backend order flipped: tries--use-port=emdawnwebgpufirst, falls back to legacy-s USE_WEBGPU=1only if that fails (that flag is gone upstream around the emsdk 5.0 line), so a normal build no longer burns a doomed link attempt.requestGpuAdapterAndDevicehardened (src/webgpu/gpuContext.ts):requiredLimits, derived from the renderer's actual compute usage (4 storage buffers/stage for line-clear compute,workgroup_size(64)) and clamped to what the adapter reports — an adapter that can't meet them now fails loudly atrequestDevice()instead of deep inside a compute dispatch.GPUCanvasContext.configure()into onebuildCanvasConfiguration()used by both initial acquisition and resize, so they can't drift apart.GameSettings.gpuPower(the settings-UI value) into the adapter request — previously it was persisted but never actually consulted;?gpu=and the in-game GPU power selector now resolve through the same policy.scripts/build-cpp.mjsnow fails a release build iftetris_renderer.wasmexceeds 256 KB (override viaTETRIS_CPP_WASM_BUDGET_BYTES), and CI's artifact-verification step assertswasmBytes/jsBytesare present inbuild-info.json.MODULE_TYPELESS_PACKAGE_JSONwarning quieted: added"type": "module"topackage.json; the one CommonJS script (scripts/screenshot.js) renamed to.cjsto match (it already required a sibling.cjshelper).Docs updated:
cpp/README.md(pin, backend order, size budget, a new "device request policy" table) anddocs/NATIVE_RENDERER_RESEARCH.md(drift note resolved).Test plan
npm run typecheck— passesnpx eslint .— passes, noMODULE_TYPELESS_PACKAGE_JSONwarningnpm test— 202/202 passing (added tests forresolveRequiredLimits,GameSettings.gpuPowerwiring,autobackend order, and the size-budget constants)npm run build— production bundle builds successfullynpm run cpp:release— noemccin this sandbox; confirmed it still skips cleanly (exit 0) and logs the new5.0.7pinnpm run cpp:releasewith a real emsdk 5.0.7 install (needs CI or a machine with emsdk — not available in this sandbox)🤖 Generated with Claude Code
https://claude.ai/code/session_01NsXNcchXYG3RQU5hpoT5Ar
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests