ci: upgrade GitHub Actions to current majors and pin to commit SHAs - #7
Merged
Conversation
GitHub now warns that actions/checkout@v4 and actions/setup-dotnet@v4 target Node 20 and are being force-run on Node 24. This moves every action to its current major, all of which declare the node24 runtime. actions/checkout v4 -> v7 (5 uses) actions/setup-dotnet v4 -> v6 (2 uses) actions/setup-node v4 -> v7 (2 uses) actions/cache v4 -> v6 (1 use) actions/upload-artifact v4 -> v7 (1 use) These are multi-major jumps, so the release notes were checked for breaking changes rather than bumping blind. Four are relevant, none of them bite here: - checkout v5+ refuses to check out fork PR code by default, needing `allow-unsafe-pr-checkout: true` to restore the old behaviour. The guard fires only on `pull_request_target` and `workflow_run`. This repo triggers on push, pull_request, workflow_dispatch and issue_comment, so nothing changes -- notably the /preview job's PR-head checkout is unaffected. (That job's own trust boundary is unchanged by this upgrade: it still runs owner-only.) - setup-dotnet v5+ dropped support for older .NET versions. This repo pins 9.0.x and targets net8.0/net9.0, so it is unaffected. - setup-node v5+ enables automatic caching when package.json has a `packageManager` field, narrowed to npm in v6. No package.json in this repo sets that field, so no caching behaviour changes and no lockfile is required. - cache/upload-artifact v5+ require Actions Runner 2.327.1+. Only GitHub-hosted runners are used, which are well past that. Verified: fetched action.yml at each new major and confirmed every input this repo passes still exists (11 steps cross-checked programmatically), that cache@v6 still exposes the `cache-hit` output the export action reads, and that upload-artifact@v7 still accepts `if-no-files-found`. All three YAML files parse. Coverage note: only test.yml runs on a pull request, so these checks exercise checkout@v7 and setup-dotnet@v6 alone. The setup-node, cache and upload-artifact bumps live in web-export.yml, which runs only on merge to main or an explicit /preview -- they are validated by schema, not by execution. Worth a /preview before merging if you want them exercised first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3
Adopts the pinning convention already used across the other repos in this account -- `uses: <action>@<40-char commit sha> # v<semver>` -- which this repo was the only one not following (0 of 13 pinned, against 100% in blog, github-actions, platform, rights-application, sentric-frontends, workspace and data-platform, and the large majority in the-vault-system and shared-utilities). A moving major tag can be repointed at new code by whoever controls the action, so the tag alone is not a supply-chain control. This repo already checksum-pins the third-party Godot editor binary for exactly that reason; the actions were the remaining gap. actions/checkout @3d3c42e5 # v7.0.1 (5 uses) actions/setup-dotnet @a98b5685 # v6.0.0 (2 uses) actions/setup-node @82076278 # v7.0.0 (2 uses) actions/cache @55cc8345 # v6.1.0 (1 use) actions/upload-artifact @043fb46d # v7.0.1 (1 use) Full semver in the comment, matching the dominant form in the other repos (platform, github-actions, the-vault-system) rather than the bare-major form in blog, since the exact patch is the thing a reader wants to check. Verification -- the failure mode of this convention is a comment that drifts from its SHA, so both directions were checked: - Resolved every SHA from upstream tags, then confirmed the method by resolving four pins that already exist in the other repos (checkout v7.0.0, setup-node v7.0.0, cache v6.1.0, setup-dotnet v5.2.0) and matching them byte for byte. - Re-derived each committed pin from its own comment: 5 distinct pins, 0 mismatched. - Two SHAs are byte-identical to pins already in platform (setup-node v7.0.0, cache v6.1.0), so those are corroborated by a second source. - All three YAML files parse; no unpinned third-party actions remain. Where this differs from the other repos: checkout and upload-artifact go to v7.0.1 rather than the v7.0.0 pinned elsewhere, and setup-dotnet to v6.0.0 rather than v5.2.0 -- these are the current latest, and a fresh pin may as well start there. Say the word to align them downward instead. No Dependabot config added: no repo in this account has one, so these pins are maintained by hand and this change does not alter that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3
rtkelly13
changed the title
rtkelly13
pushed a commit
that referenced
this pull request
Jul 26, 2026
This branch and `main` had no Node or Playwright ignore rules, so nothing stopped an `npm ci` in tests/visual/ from being committed. That is exactly how 170 files / 17.7 MiB of node_modules got into the GitHub-Actions branch (#7), which is also based on `main`. Later branches in this stack do carry `tests/visual/`-anchored rules, added with the visual-regression harness. These are deliberately repo-wide rather than anchored: the trap is a node_modules appearing somewhere the anchored pattern doesn't cover. Baseline screenshots under *-snapshots/ stay tracked; only regenerated run output is ignored.
This branch committed 170 files / 17.7 MiB of tests/visual/node_modules, plus a Playwright test-results/.last-run.json, because neither this branch nor `main` had any Node or Playwright ignore rules. The blobs are removed from this branch's history rather than deleted in a follow-up commit, so they never reach `main` at all — a later deletion would leave 17.7 MiB permanently in the repository. Identical to the block added on the #8 stack, so whichever merges to main first, the other merges without conflict. Baseline screenshots under *-snapshots/ stay tracked.
rtkelly13
force-pushed
the
claude/upgrade-github-actions-3a6h5m
branch
from
July 26, 2026 08:23
54fabad to
baf4ca9
Compare
rtkelly13
pushed a commit
that referenced
this pull request
Jul 26, 2026
#7 pinned every action to a commit SHA, but the jobs added on this stack (scene-tests, visual-harness, mirror-editor, the visual/functional deploy jobs) were written against floating v4 tags and merged in unpinned. Same SHAs as main, so the convention holds across every workflow rather than most of them.
rtkelly13
added a commit
that referenced
this pull request
Jul 27, 2026
…odot 4.7.1 (#8) * ci(web): single-source toolchain versions, patch-level drift guard, 4.7.1 bump Closes three gaps in the experimental C#/WASM export pipeline and adds a plan for getting it onto officially supported foundations. Fixes: - Production and preview could build different toolchains. The editor tag and template version were duplicated in four places (composite action defaults, workflow_dispatch defaults, and inline fallbacks in the production job) while the preview job passed nothing and inherited the action defaults. Updating one and not the others meant /preview validated a toolchain production never used. All versions now come from .github/web-toolchain.env; action inputs default to empty and act purely as deliberate overrides. - The drift guard compared only major.minor, so a 4.7.0-vs-4.7.1 mismatch passed silently -- the most likely kind of bump. It now normalises all three spellings of a version (4.7.1 / 4.7.1.stable.mono / 4.7.1-stable), requires exact agreement including the editor tag, and handles Godot's convention that x.y releases carry no patch component. It also verifies a checksum is pinned for the target asset before downloading a 165 MB editor rather than after. - Checksum verification matched the first zip found in the cache directory; it now requires the expected filename, so a restored cache cannot substitute a differently-named editor. Cache key includes the fork repo as well as the tag. Toolchain bump to 4.7.1-stable (upstream 4.7.1 is a stability-only release with no known incompatibilities with 4.7). Superseded checksums are retained so a rollback needs no checksum work. Docs: - docs/WEB_EXPORT_ROADMAP.md (new): upstream status with evidence, definition of done, four-phase migration plan, risk register, quarterly monitoring routine and rollback procedure. Records that LibGodot Core (#110863) merged and shipped in Godot 4.6, that the approach pivoted away from the stalled #106125 to LibGodot, and that #121502 and #118976 remain open at milestone 4.x -- so official C# web export is still unscheduled. - docs/WEB_EXPORT.md: corrected the DNS section (maze.ryankelly.dev resolves), softened the fork-abandonment risk (the fork matched upstream 4.7.1 within two days), and repointed the version matrix at the new single source of truth. - AGENTS.md: added the web-only failure modes (invariant globalization, missing crypto BCL APIs, no GDExtension), which pass desktop CI and break only in the browser. Not verified here: no .NET SDK in this environment, so the desktop build against Godot.NET.Sdk 4.7.1 is unbuilt. The version-resolution and drift-guard logic was executed against all drift permutations, and both editor checksums were computed from the published assets (the 4.7-stable hash reproduces the existing pin). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix(test): run one fixture instance per test case, fixing a parallelism data race `[assembly: Parallelizable(ParallelScope.All)]` was combined with NUnit's default SingleInstance lifecycle, so test cases in a fixture ran concurrently against one shared instance. Every field assigned in `[SetUp]` was therefore a data race. It surfaced as two NullReferenceExceptions in RandomValueTests on CI, on a commit whose other runs of the same job were green — re-running the identical commit passed, confirming a race rather than an environment difference. The mechanism: _mazePointFactory = new Mock<IMazePointFactory>(); // (a) bare mock _mazePointFactory.Setup(x => x.MakePoint(...)).Returns(...); // (b) configures it _randomPoint = new RandomPointGenerator(_random, _mazePointFactory.Object); // (c) re-reads the field Another test's (a) landing between this test's (b) and (c) makes (c) capture an unconfigured mock. Moq then returns default(MazePoint) — null — and the test dies dereferencing `point.X`, nowhere near the actual cause. Measured with a trace harness (NUnit's per-test console capture reorders output and hides this): with SingleInstance, four concurrent `[SetUp]` bodies share one fixture instance; with InstancePerTestCase, four concurrent tests get four distinct instances. Parallelism is unchanged, the unsafe sharing is gone. Also drops MovementHelperTests' `[NonParallelizable]`, which was a local workaround for this same root cause, and adds a guard test so removing the attribute fails immediately by name instead of resurfacing as an occasional unexplained flake. * chore: gitignore node_modules and Playwright run output This branch and `main` had no Node or Playwright ignore rules, so nothing stopped an `npm ci` in tests/visual/ from being committed. That is exactly how 170 files / 17.7 MiB of node_modules got into the GitHub-Actions branch (#7), which is also based on `main`. Later branches in this stack do carry `tests/visual/`-anchored rules, added with the visual-regression harness. These are deliberately repo-wide rather than anchored: the trap is a node_modules appearing somewhere the anchored pattern doesn't cover. Baseline screenshots under *-snapshots/ stay tracked; only regenerated run output is ignored. * refactor(ci): extract export-web inline PowerShell into scripts/*.ps1 Each composite-action step now invokes a dedicated script under .github/actions/export-web/scripts/, keeping action.yml to wiring (env, ids, outputs). The drift-guard error message becomes a plain here-string now that it lives outside a YAML block scalar. All scripts parse clean; resolve-toolchain and check-version-drift verified locally including the drift-failure path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
rtkelly13
added a commit
that referenced
this pull request
Jul 27, 2026
#9) * ci(web): single-source toolchain versions, patch-level drift guard, 4.7.1 bump Closes three gaps in the experimental C#/WASM export pipeline and adds a plan for getting it onto officially supported foundations. Fixes: - Production and preview could build different toolchains. The editor tag and template version were duplicated in four places (composite action defaults, workflow_dispatch defaults, and inline fallbacks in the production job) while the preview job passed nothing and inherited the action defaults. Updating one and not the others meant /preview validated a toolchain production never used. All versions now come from .github/web-toolchain.env; action inputs default to empty and act purely as deliberate overrides. - The drift guard compared only major.minor, so a 4.7.0-vs-4.7.1 mismatch passed silently -- the most likely kind of bump. It now normalises all three spellings of a version (4.7.1 / 4.7.1.stable.mono / 4.7.1-stable), requires exact agreement including the editor tag, and handles Godot's convention that x.y releases carry no patch component. It also verifies a checksum is pinned for the target asset before downloading a 165 MB editor rather than after. - Checksum verification matched the first zip found in the cache directory; it now requires the expected filename, so a restored cache cannot substitute a differently-named editor. Cache key includes the fork repo as well as the tag. Toolchain bump to 4.7.1-stable (upstream 4.7.1 is a stability-only release with no known incompatibilities with 4.7). Superseded checksums are retained so a rollback needs no checksum work. Docs: - docs/WEB_EXPORT_ROADMAP.md (new): upstream status with evidence, definition of done, four-phase migration plan, risk register, quarterly monitoring routine and rollback procedure. Records that LibGodot Core (#110863) merged and shipped in Godot 4.6, that the approach pivoted away from the stalled #106125 to LibGodot, and that #121502 and #118976 remain open at milestone 4.x -- so official C# web export is still unscheduled. - docs/WEB_EXPORT.md: corrected the DNS section (maze.ryankelly.dev resolves), softened the fork-abandonment risk (the fork matched upstream 4.7.1 within two days), and repointed the version matrix at the new single source of truth. - AGENTS.md: added the web-only failure modes (invariant globalization, missing crypto BCL APIs, no GDExtension), which pass desktop CI and break only in the browser. Not verified here: no .NET SDK in this environment, so the desktop build against Godot.NET.Sdk 4.7.1 is unbuilt. The version-resolution and drift-guard logic was executed against all drift permutations, and both editor checksums were computed from the published assets (the 4.7-stable hash reproduces the existing pin). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * feat(test): seed-deterministic generation, regression foundation, visual regression harness Makes maze generation reproducible from a seed, which is the prerequisite for any regression testing of generated output, and adds the Playwright visual-regression harness for the web build. Why: the existing 421 tests could only assert invariants (valid, connected), never what was generated -- because nothing was reproducible. Measured on the previous code at 12x12x2 with identical settings over 8 runs: all four algorithms produced 8/8 distinct mazes, start and end points were 8/8 distinct, and shortest-path length ranged [45, 1, 33, 25, 25, 104, 43, 151] -- min 1, max 151. An algorithm change that made mazes measurably worse would have passed every test. Three independent sources of ungoverned randomness fed generation, none seedable: 1. RandomValueGenerator used a [ThreadStatic] Random seeded from Environment.TickCount. 2. ArrayHelper.Shuffle used Random.Shared -- reached statically from the backtracker, growing-tree and random carver, so there was no injection seam at all. 3. RandomAgent and PerfectAgent called Random.Shared.Shuffle directly. Changes: - IRandomValueGenerator is now the single source of randomness (GetNext, Shuffle, Reseed), injected everywhere, holding per-instance state. The thread-static indirection was also unnecessary: nothing in the pipeline is concurrent. - MazeGenerationSettings.Seed pins a run; MazeGenerationFactory reseeds once before anything consumes randomness, so start/end placement, carving order, wall removal and agent walks are all covered. - MazeGenerationResults.Seed always reports the seed used. Unseeded runs still draw and report a concrete seed, so a maze found by chance is reproducible. - ArrayHelper deleted: its shuffle overloads were the trap and its Average was dead code. Benchmarks now exercise the production shuffle path with a fixed seed. - ISystemClock replaces a DateTime.Now read in MazeStatsSerializer that made serialized stats differ every run -- a golden-file blocker found by the new discipline guard rather than by inspection. Guards: - DeterminismTests: same seed gives identical mazes across all four algorithms; different seeds still differ (seeding must not collapse the output space); a reported seed reproduces its maze; wall removal and agent walks are seed-stable; reusing a container across seeds doesn't leak state. - RandomnessDisciplineTests: scans scripts/maze and rejects Random.Shared, new Random(, ArrayHelper.Shuffle, Guid.NewGuid and DateTime.Now/UtcNow, exempting only the two designated injected sources. Visual regression (tests/visual, @playwright/test toHaveScreenshot): - Harness with canvas-boot detection, COOP/COEP assertions and pixel-stability polling that fails rather than screenshotting a still-animating canvas. - harness.spec.ts self-tests the harness against a local deterministic canvas, so it stays verified without needing a web build (which requires the patched Windows editor). Measured there: a seeded render is byte-identical across fresh pages and across browser launches, but reusing one page across loads perturbs ~1.6% of pixels -- so every case loads into a fresh page. An early version of this suite reused a page and looked like rendering nondeterminism; it was the fixture. - maze.spec.ts targets the deployed build and is SKIPPED until the build accepts generation parameters from the query string (docs/VISUAL_REGRESSION.md specifies the contract). Skipped rather than failing so it cannot report a false red. - CI: visual-harness on every PR (no deploy needed); visual-production/-preview after the existing smoke tests, with report artifacts on failure. Docs: docs/REGRESSION_TESTING.md (four-layer plan, recommended order, open questions), docs/VISUAL_REGRESSION.md (measurements, baseline policy, prerequisite), AGENTS.md (the randomness rule contributors must follow). Verified: full suite 443 passing (421 existing plus 22 new) on .NET 8; benchmarks and experiments projects still compile; visual harness self-test green across repeated runs; all workflow YAML parses. Not verified: the desktop/Godot build (no Godot SDK or .NET 9 available here) and the maze visual suite (needs a deployed build). Separately, suite wall-time under parallel execution is erratic on a 4-core box -- runs from 2.8s to over 300s -- but this reproduces on the pre-change baseline too (122s observed), sequential runs are clean on both sides, and per-test timings match, so it is pre-existing and not from this change. Worth its own investigation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix(test): narrow PerfectAgent sample to stop the suite hanging in CI CI run 9 on this branch hung for 40 minutes with no output after "Found 3 maze files" and was cancelled, leaving the required `test` check red. Root cause, isolated by running the two agent families separately over 8 runs each on the sample mazes: RandomAgent 1768 1662 1724 1738 1702 1755 1666 1644 ms (flat) PerfectAgent 120011 1941 14602 120034 3225 1960 1848 25880 ms (2 timed out) PerfectAgent's search is worst-case exponential: it tracks visited cells per-path via previousPoints.Any(...) -- a linear scan of the current path -- rather than with a shared visited set, so the same cell is re-explored along different paths, and it copies the whole path per branch. Whether it terminates quickly depends entirely on the shuffled direction order, which is why it is intermittent. This is pre-existing, not introduced here: the same workflow recorded 1002s on a push to main on 2026-07-01, against durations of 29-57s for neighbouring runs. Fix (symptom, not cause): - SampleMazeTests: PerfectAgent tests now draw from PerfectAgentMazeFiles() capped at 200 cells, keeping 10x10x1 and dropping 20x20x3 (1200 cells) and 20x20x4 (1600). Exactly 4 test cases removed, verified by diffing --list-tests output; nothing else changed. Total 443 -> 439. - Those two tests carry Timeout(60_000) so a regression fails with attribution rather than stalling. - test.yml jobs get timeout-minutes: 15, so a future hang costs 15 minutes and a clear failure instead of 40 minutes and a bare cancellation. Other PerfectAgent callers are on 5x5x1 (25 cells) or seeded, so unaffected. Verified: 12 consecutive full-suite runs at 8.7-9.7s with zero timeouts, against a pre-fix spread that reached 120s+ on the agent tests alone. 439 passing. The algorithm is still exponential and will resurface on any larger maze -- including in the app, where it would hang the UI rather than a test. Recorded in docs/REGRESSION_TESTING.md as the fix that still needs doing (shared visited set). Also observed once, on the pre-narrowing tree: a single test failure that did not reproduce across 16 subsequent runs, so it is unidentified rather than diagnosed. Noted here rather than left silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix(test): run one fixture instance per test case, fixing a parallelism data race `[assembly: Parallelizable(ParallelScope.All)]` was combined with NUnit's default SingleInstance lifecycle, so test cases in a fixture ran concurrently against one shared instance. Every field assigned in `[SetUp]` was therefore a data race. It surfaced as two NullReferenceExceptions in RandomValueTests on CI, on a commit whose other runs of the same job were green — re-running the identical commit passed, confirming a race rather than an environment difference. The mechanism: _mazePointFactory = new Mock<IMazePointFactory>(); // (a) bare mock _mazePointFactory.Setup(x => x.MakePoint(...)).Returns(...); // (b) configures it _randomPoint = new RandomPointGenerator(_random, _mazePointFactory.Object); // (c) re-reads the field Another test's (a) landing between this test's (b) and (c) makes (c) capture an unconfigured mock. Moq then returns default(MazePoint) — null — and the test dies dereferencing `point.X`, nowhere near the actual cause. Measured with a trace harness (NUnit's per-test console capture reorders output and hides this): with SingleInstance, four concurrent `[SetUp]` bodies share one fixture instance; with InstancePerTestCase, four concurrent tests get four distinct instances. Parallelism is unchanged, the unsafe sharing is gone. Also drops MovementHelperTests' `[NonParallelizable]`, which was a local workaround for this same root cause, and adds a guard test so removing the attribute fails immediately by name instead of resurfacing as an occasional unexplained flake. * chore: gitignore node_modules and Playwright run output This branch and `main` had no Node or Playwright ignore rules, so nothing stopped an `npm ci` in tests/visual/ from being committed. That is exactly how 170 files / 17.7 MiB of node_modules got into the GitHub-Actions branch (#7), which is also based on `main`. Later branches in this stack do carry `tests/visual/`-anchored rules, added with the visual-regression harness. These are deliberately repo-wide rather than anchored: the trap is a node_modules appearing somewhere the anchored pattern doesn't cover. Baseline screenshots under *-snapshots/ stay tracked; only regenerated run output is ignored. --------- Co-authored-by: Claude <noreply@anthropic.com>
rtkelly13
added a commit
that referenced
this pull request
Jul 27, 2026
…Session split (#10) * ci(web): single-source toolchain versions, patch-level drift guard, 4.7.1 bump Closes three gaps in the experimental C#/WASM export pipeline and adds a plan for getting it onto officially supported foundations. Fixes: - Production and preview could build different toolchains. The editor tag and template version were duplicated in four places (composite action defaults, workflow_dispatch defaults, and inline fallbacks in the production job) while the preview job passed nothing and inherited the action defaults. Updating one and not the others meant /preview validated a toolchain production never used. All versions now come from .github/web-toolchain.env; action inputs default to empty and act purely as deliberate overrides. - The drift guard compared only major.minor, so a 4.7.0-vs-4.7.1 mismatch passed silently -- the most likely kind of bump. It now normalises all three spellings of a version (4.7.1 / 4.7.1.stable.mono / 4.7.1-stable), requires exact agreement including the editor tag, and handles Godot's convention that x.y releases carry no patch component. It also verifies a checksum is pinned for the target asset before downloading a 165 MB editor rather than after. - Checksum verification matched the first zip found in the cache directory; it now requires the expected filename, so a restored cache cannot substitute a differently-named editor. Cache key includes the fork repo as well as the tag. Toolchain bump to 4.7.1-stable (upstream 4.7.1 is a stability-only release with no known incompatibilities with 4.7). Superseded checksums are retained so a rollback needs no checksum work. Docs: - docs/WEB_EXPORT_ROADMAP.md (new): upstream status with evidence, definition of done, four-phase migration plan, risk register, quarterly monitoring routine and rollback procedure. Records that LibGodot Core (#110863) merged and shipped in Godot 4.6, that the approach pivoted away from the stalled #106125 to LibGodot, and that #121502 and #118976 remain open at milestone 4.x -- so official C# web export is still unscheduled. - docs/WEB_EXPORT.md: corrected the DNS section (maze.ryankelly.dev resolves), softened the fork-abandonment risk (the fork matched upstream 4.7.1 within two days), and repointed the version matrix at the new single source of truth. - AGENTS.md: added the web-only failure modes (invariant globalization, missing crypto BCL APIs, no GDExtension), which pass desktop CI and break only in the browser. Not verified here: no .NET SDK in this environment, so the desktop build against Godot.NET.Sdk 4.7.1 is unbuilt. The version-resolution and drift-guard logic was executed against all drift permutations, and both editor checksums were computed from the published assets (the 4.7-stable hash reproduces the existing pin). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * feat(test): seed-deterministic generation, regression foundation, visual regression harness Makes maze generation reproducible from a seed, which is the prerequisite for any regression testing of generated output, and adds the Playwright visual-regression harness for the web build. Why: the existing 421 tests could only assert invariants (valid, connected), never what was generated -- because nothing was reproducible. Measured on the previous code at 12x12x2 with identical settings over 8 runs: all four algorithms produced 8/8 distinct mazes, start and end points were 8/8 distinct, and shortest-path length ranged [45, 1, 33, 25, 25, 104, 43, 151] -- min 1, max 151. An algorithm change that made mazes measurably worse would have passed every test. Three independent sources of ungoverned randomness fed generation, none seedable: 1. RandomValueGenerator used a [ThreadStatic] Random seeded from Environment.TickCount. 2. ArrayHelper.Shuffle used Random.Shared -- reached statically from the backtracker, growing-tree and random carver, so there was no injection seam at all. 3. RandomAgent and PerfectAgent called Random.Shared.Shuffle directly. Changes: - IRandomValueGenerator is now the single source of randomness (GetNext, Shuffle, Reseed), injected everywhere, holding per-instance state. The thread-static indirection was also unnecessary: nothing in the pipeline is concurrent. - MazeGenerationSettings.Seed pins a run; MazeGenerationFactory reseeds once before anything consumes randomness, so start/end placement, carving order, wall removal and agent walks are all covered. - MazeGenerationResults.Seed always reports the seed used. Unseeded runs still draw and report a concrete seed, so a maze found by chance is reproducible. - ArrayHelper deleted: its shuffle overloads were the trap and its Average was dead code. Benchmarks now exercise the production shuffle path with a fixed seed. - ISystemClock replaces a DateTime.Now read in MazeStatsSerializer that made serialized stats differ every run -- a golden-file blocker found by the new discipline guard rather than by inspection. Guards: - DeterminismTests: same seed gives identical mazes across all four algorithms; different seeds still differ (seeding must not collapse the output space); a reported seed reproduces its maze; wall removal and agent walks are seed-stable; reusing a container across seeds doesn't leak state. - RandomnessDisciplineTests: scans scripts/maze and rejects Random.Shared, new Random(, ArrayHelper.Shuffle, Guid.NewGuid and DateTime.Now/UtcNow, exempting only the two designated injected sources. Visual regression (tests/visual, @playwright/test toHaveScreenshot): - Harness with canvas-boot detection, COOP/COEP assertions and pixel-stability polling that fails rather than screenshotting a still-animating canvas. - harness.spec.ts self-tests the harness against a local deterministic canvas, so it stays verified without needing a web build (which requires the patched Windows editor). Measured there: a seeded render is byte-identical across fresh pages and across browser launches, but reusing one page across loads perturbs ~1.6% of pixels -- so every case loads into a fresh page. An early version of this suite reused a page and looked like rendering nondeterminism; it was the fixture. - maze.spec.ts targets the deployed build and is SKIPPED until the build accepts generation parameters from the query string (docs/VISUAL_REGRESSION.md specifies the contract). Skipped rather than failing so it cannot report a false red. - CI: visual-harness on every PR (no deploy needed); visual-production/-preview after the existing smoke tests, with report artifacts on failure. Docs: docs/REGRESSION_TESTING.md (four-layer plan, recommended order, open questions), docs/VISUAL_REGRESSION.md (measurements, baseline policy, prerequisite), AGENTS.md (the randomness rule contributors must follow). Verified: full suite 443 passing (421 existing plus 22 new) on .NET 8; benchmarks and experiments projects still compile; visual harness self-test green across repeated runs; all workflow YAML parses. Not verified: the desktop/Godot build (no Godot SDK or .NET 9 available here) and the maze visual suite (needs a deployed build). Separately, suite wall-time under parallel execution is erratic on a 4-core box -- runs from 2.8s to over 300s -- but this reproduces on the pre-change baseline too (122s observed), sequential runs are clean on both sides, and per-test timings match, so it is pre-existing and not from this change. Worth its own investigation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix(test): narrow PerfectAgent sample to stop the suite hanging in CI CI run 9 on this branch hung for 40 minutes with no output after "Found 3 maze files" and was cancelled, leaving the required `test` check red. Root cause, isolated by running the two agent families separately over 8 runs each on the sample mazes: RandomAgent 1768 1662 1724 1738 1702 1755 1666 1644 ms (flat) PerfectAgent 120011 1941 14602 120034 3225 1960 1848 25880 ms (2 timed out) PerfectAgent's search is worst-case exponential: it tracks visited cells per-path via previousPoints.Any(...) -- a linear scan of the current path -- rather than with a shared visited set, so the same cell is re-explored along different paths, and it copies the whole path per branch. Whether it terminates quickly depends entirely on the shuffled direction order, which is why it is intermittent. This is pre-existing, not introduced here: the same workflow recorded 1002s on a push to main on 2026-07-01, against durations of 29-57s for neighbouring runs. Fix (symptom, not cause): - SampleMazeTests: PerfectAgent tests now draw from PerfectAgentMazeFiles() capped at 200 cells, keeping 10x10x1 and dropping 20x20x3 (1200 cells) and 20x20x4 (1600). Exactly 4 test cases removed, verified by diffing --list-tests output; nothing else changed. Total 443 -> 439. - Those two tests carry Timeout(60_000) so a regression fails with attribution rather than stalling. - test.yml jobs get timeout-minutes: 15, so a future hang costs 15 minutes and a clear failure instead of 40 minutes and a bare cancellation. Other PerfectAgent callers are on 5x5x1 (25 cells) or seeded, so unaffected. Verified: 12 consecutive full-suite runs at 8.7-9.7s with zero timeouts, against a pre-fix spread that reached 120s+ on the agent tests alone. 439 passing. The algorithm is still exponential and will resurface on any larger maze -- including in the app, where it would hang the UI rather than a test. Recorded in docs/REGRESSION_TESTING.md as the fix that still needs doing (shared visited set). Also observed once, on the pre-narrowing tree: a single test failure that did not reproduce across 16 subsequent runs, so it is unidentified rather than diagnosed. Noted here rather than left silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * feat(test): in-app test bridge, headless scene tests, Playwright functional suite Adds functional (behavioural) testing, not just visual, across the two layers that had no coverage at all: the Godot scene/UI layer and the deployed browser build. WHY A BRIDGE WAS NEEDED A Godot web export renders everything into a single <canvas>. Menus, buttons and labels are drawn inside it, so Playwright's locator model -- getByRole, getByText, toBeVisible -- sees nothing but one element. Playwright can already *drive* the app with no changes (real key and mouse events reach the engine); what was missing was any way to *observe* it. Without a hook the only possible assertions are "a canvas exists" and pixel comparison. scripts/testing/TestBridge.cs supplies that, opt-in via ?test=1 (or any ?seed=): window.__mazeTestApi "1" when live, absent when disabled window.__mazeState JSON string of app state, republished on change window.__mazeCommand function(jsonString), fire-and-forget Design choices are constraints, not preferences, and each is documented: - Fire-and-forget commands. Godot's docs never state that a create_callback return value reaches the JS caller and no example returns one, so results are observed via state rather than depending on unspecified behaviour. Callbacks must also take exactly one Array argument, which a single JSON entry point satisfies. - No JavaScriptBridge.Eval. The docs say eval "may be disabled in custom export templates" and this project uses a patched template, so GetInterface plus property assignment avoids that path entirely. - Hand-rolled JSON. The web build is trimmed; a reflection-based serializer is exactly what trimming breaks, and it would fail only in the browser. - Protocol split into TestBridgeProtocol (Godot-free) so the NUnit suite can cover the fiddly parsing -- 54 new tests. The Node itself only runs inside the engine. - Also seeds from the URL, which was the outstanding prerequisite for the visual suite. SCENE TESTS, AND WHY NOT gdUnit4Net gdUnit4Net was tried first. On Godot 4.7.1 with gdUnit4.api 5.1.0-rc5 and gdUnit4.test.adapter 3.1.1: the project restores and builds with no conflict, and logic-only [TestCase] tests pass -- but every [RequireGodotRuntime] test fails with "Starting GodotRuntimeExecutor failed. The operation has timed out. Failed to connect: Connection timeout". Setting GodotProjectDir and pre-importing did not help. Isolated the cause: Godot 4.7.1 runs this project headless and executes its C# fine (the GameState autoload's _Ready fires). So the blocker is gdUnit4's own executor, not Godot and not this project -- consistent with its stated support stopping at Godot 4.4.1, last release June 2025. So tests/scene/SceneTestRunner.cs is a ~200-line in-engine runner: a list of checks, a tally, and a process exit code. Compiled only under -p:IncludeSceneTests=true so it never ships in a game export. Delete it in favour of gdUnit4Net once that supports 4.7+; the checks port over almost verbatim. This is the first real coverage of scripts/ui/ (~4300 lines). The nearest thing before was a test reading menu.tscn as *text* and asserting it contained the string "ComparisonButton" -- proving a node name appears in a file, not that it is a Button or that the scene instantiates. VERIFIED BY EXECUTION Correcting an earlier claim of mine: Godot code *is* buildable here. GodotSharp 4.7.1 targets net8.0, so the available .NET 8 SDK compiles it, and the official Godot 4.7.1 Linux build runs it headless. So this is executed, not reasoned: - 493 NUnit tests pass (was 439; +54 for the bridge wire format). - Godot project builds clean, with and without -p:IncludeSceneTests=true, which also proves the runner is excluded from a normal build. - 7/7 scene tests pass in real Godot 4.7.1 headless, exit code 0. Failure path checked too by deliberately breaking a check: 6/7, exit code 1, so CI genuinely gates. - Playwright: all 18 tests across 3 files enumerate (TypeScript compiles); the 4 harness self-tests still pass. - benchmarks and experiments still build; all workflow YAML parses. NOT VERIFIED: whether the bridge behaves under the *patched* web export template. That needs a deploy, and it is the one thing unit and scene tests cannot answer. Both browser suites are therefore gated on MAZE_TEST_BRIDGE=1 and currently skip rather than risk a false red -- see docs/TEST_BRIDGE.md "Enabling in CI" for the three checks to run on a /preview before flipping it. CI: new scene-tests job on every PR, downloading the official upstream Linux editor checksum-pinned in .github/editor-checksums.txt exactly like the patched Windows one. The fork is only needed for the web export. The functional suite runs post-deploy alongside the visual one. Docs: docs/TESTING.md (layer map, push-tests-down rationale, gdUnit4Net findings, known gaps), docs/TEST_BRIDGE.md (contract, per-decision rationale, verification status, security notes), plus VISUAL_REGRESSION.md, REGRESSION_TESTING.md and AGENTS.md updated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * refactor: extract MazeSession from GameState, cover the newly testable behaviour Segregates application behaviour from Godot so it can be integration-tested without the engine, and covers the code that segregation exposes. 493 -> 550 tests. WHY: the codebase was already nearly segregated; the test project just didn't include it. Measured Godot-coupling density in scripts/ui/: AnimationController.cs 254 lines, 0 Godot refs (no `using Godot;`) ImportExportResult.cs 54 lines, 0 MazeImportExport.cs 363 lines, 9 (FileAccess, OS only) PathVisualizationSettings.cs 206 lines, 27 (all one type: Color) MazeMain.cs 1473 lines, 156 (Input. x28, GetNode x24, AddChild x13) GraphViewRenderer.cs 631 lines, 73 (genuine drawing) And GameState -- the app's state hub, 266 lines -- had an *entire* Godot surface of `: Node`, `_Ready`, `_ExitTree`, one GD.Print and one Mathf.Clamp. Nearly all logic, none of it testable, because the NUnit suite builds without the Godot SDK. CHANGES - scripts/session/MazeSession.cs (new): all session state and operations -- settings, current maze, level navigation, alternative paths, animation state, generation and import -- with no Godot dependency. Takes an optional ServiceContainer so tests can inject one. Mathf.Clamp becomes Math.Clamp. - GameState becomes a thin adapter forwarding to it. Its public API is unchanged, so nothing in scripts/ui or scripts/testing needed editing. - PathVisualizationSettings deliberately stays on the node: it is presentation config built on Godot's Color, and its DecisionDetailLevel enum lives in the same file, so moving it would reintroduce the dependency the split removes. GameState resets those presentation flags itself after delegating the session reset. - Test project now compiles MazeSession, AnimationController, ImportExportResult and GraphLayoutType. Adding a file to that list is a claim that it is Godot-free, and the build enforces the claim. TESTS - AnimationControllerTests (30 tests): a 254-line playback state machine that had zero coverage -- not because it was hard to test but because of where it lived. Time is a Update(deltaTime) parameter, so no clock or waiting is involved. Covers transport controls, stepping bounds, time-driven advance, speed clamping, notifications, and pins two real behaviours: a large frame delta advances only one step, and replay after finishing rewinds. - MazeSessionTests (24 tests): whole flows, previously impossible -- generate then navigate then regenerate, import adopting its own size, import clearing state left from the previous maze, animation driven through session state. A REAL BUG, FOUND ON THE FIRST RUN GameState.LoadImportedMaze applies dead-end wrapping and then builds a graph. That combination threw "Nullable object must have a value", so **importing any maze crashed the app**. GraphBuilder.GetGraphEdges walks a corridor until it reaches a junction or the start/end. A dead-end cell has exactly one direction -- the one you arrived from -- so it is neither a junction (needs more than two) nor a terminus, and the walk dereferenced a null Direction?. Dead-end wrapping *creates* such cells by hiding passages. Isolated it: GetGraph on a plain maze passes; after DoDeadEndWrapping it throws, on generated and imported mazes alike. Several existing tests did wrapping, and several did graph building -- none did both, which is how it survived. Fixed by dropping the edge when a corridor dead-ends, rather than pointing it at the dead-end cell: the graph's node set is junctions plus start/end, and consumers look edges up with graph.Nodes[edge.Point], so an edge to a non-node would trade one crash for a KeyNotFoundException. GraphBuilderDeadEndTests covers it, including that every edge still targets a known node and that wrapped mazes remain solvable. VERIFIED BY EXECUTION - 550 NUnit tests pass (was 493). - Reverting only the GraphBuilder guard fails 10 tests while the plain-maze control still passes, so the regression tests are genuinely coupled to the fix. - Godot project builds clean; 7/7 headless scene tests still pass in Godot 4.7.1; benchmarks and experiments build. Docs: docs/TESTING.md gains the coupling measurements, the three-tier plan, the MazeSession rationale and the bug write-up; AGENTS.md records the rule that behaviour belongs in plain C# with Node subclasses as thin adapters. Next in this direction, deliberately not done here: abstract FileAccess/OS in MazeImportExport so import/export round-trips become testable, then pull orchestration out of MazeMain incrementally. GraphViewRenderer's drawing should stay where it is -- render code's contract is the pixels, which is what the visual suite is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix(test): run one fixture instance per test case, fixing a parallelism data race `[assembly: Parallelizable(ParallelScope.All)]` was combined with NUnit's default SingleInstance lifecycle, so test cases in a fixture ran concurrently against one shared instance. Every field assigned in `[SetUp]` was therefore a data race. It surfaced as two NullReferenceExceptions in RandomValueTests on CI, on a commit whose other runs of the same job were green — re-running the identical commit passed, confirming a race rather than an environment difference. The mechanism: _mazePointFactory = new Mock<IMazePointFactory>(); // (a) bare mock _mazePointFactory.Setup(x => x.MakePoint(...)).Returns(...); // (b) configures it _randomPoint = new RandomPointGenerator(_random, _mazePointFactory.Object); // (c) re-reads the field Another test's (a) landing between this test's (b) and (c) makes (c) capture an unconfigured mock. Moq then returns default(MazePoint) — null — and the test dies dereferencing `point.X`, nowhere near the actual cause. Measured with a trace harness (NUnit's per-test console capture reorders output and hides this): with SingleInstance, four concurrent `[SetUp]` bodies share one fixture instance; with InstancePerTestCase, four concurrent tests get four distinct instances. Parallelism is unchanged, the unsafe sharing is gone. Also drops MovementHelperTests' `[NonParallelizable]`, which was a local workaround for this same root cause, and adds a guard test so removing the attribute fails immediately by name instead of resurfacing as an occasional unexplained flake. * docs(test): record the fixture-lifecycle requirement and how it was diagnosed * docs: refresh unit-test count for the lifecycle guard test * chore: gitignore node_modules and Playwright run output This branch and `main` had no Node or Playwright ignore rules, so nothing stopped an `npm ci` in tests/visual/ from being committed. That is exactly how 170 files / 17.7 MiB of node_modules got into the GitHub-Actions branch (#7), which is also based on `main`. Later branches in this stack do carry `tests/visual/`-anchored rules, added with the visual-regression harness. These are deliberately repo-wide rather than anchored: the trap is a node_modules appearing somewhere the anchored pattern doesn't cover. Baseline screenshots under *-snapshots/ stay tracked; only regenerated run output is ignored. --------- Co-authored-by: Claude <noreply@anthropic.com>
rtkelly13
added a commit
that referenced
this pull request
Jul 27, 2026
…rdening (#6) * ci(web): single-source toolchain versions, patch-level drift guard, 4.7.1 bump Closes three gaps in the experimental C#/WASM export pipeline and adds a plan for getting it onto officially supported foundations. Fixes: - Production and preview could build different toolchains. The editor tag and template version were duplicated in four places (composite action defaults, workflow_dispatch defaults, and inline fallbacks in the production job) while the preview job passed nothing and inherited the action defaults. Updating one and not the others meant /preview validated a toolchain production never used. All versions now come from .github/web-toolchain.env; action inputs default to empty and act purely as deliberate overrides. - The drift guard compared only major.minor, so a 4.7.0-vs-4.7.1 mismatch passed silently -- the most likely kind of bump. It now normalises all three spellings of a version (4.7.1 / 4.7.1.stable.mono / 4.7.1-stable), requires exact agreement including the editor tag, and handles Godot's convention that x.y releases carry no patch component. It also verifies a checksum is pinned for the target asset before downloading a 165 MB editor rather than after. - Checksum verification matched the first zip found in the cache directory; it now requires the expected filename, so a restored cache cannot substitute a differently-named editor. Cache key includes the fork repo as well as the tag. Toolchain bump to 4.7.1-stable (upstream 4.7.1 is a stability-only release with no known incompatibilities with 4.7). Superseded checksums are retained so a rollback needs no checksum work. Docs: - docs/WEB_EXPORT_ROADMAP.md (new): upstream status with evidence, definition of done, four-phase migration plan, risk register, quarterly monitoring routine and rollback procedure. Records that LibGodot Core (#110863) merged and shipped in Godot 4.6, that the approach pivoted away from the stalled #106125 to LibGodot, and that #121502 and #118976 remain open at milestone 4.x -- so official C# web export is still unscheduled. - docs/WEB_EXPORT.md: corrected the DNS section (maze.ryankelly.dev resolves), softened the fork-abandonment risk (the fork matched upstream 4.7.1 within two days), and repointed the version matrix at the new single source of truth. - AGENTS.md: added the web-only failure modes (invariant globalization, missing crypto BCL APIs, no GDExtension), which pass desktop CI and break only in the browser. Not verified here: no .NET SDK in this environment, so the desktop build against Godot.NET.Sdk 4.7.1 is unbuilt. The version-resolution and drift-guard logic was executed against all drift permutations, and both editor checksums were computed from the published assets (the 4.7-stable hash reproduces the existing pin). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * feat(test): seed-deterministic generation, regression foundation, visual regression harness Makes maze generation reproducible from a seed, which is the prerequisite for any regression testing of generated output, and adds the Playwright visual-regression harness for the web build. Why: the existing 421 tests could only assert invariants (valid, connected), never what was generated -- because nothing was reproducible. Measured on the previous code at 12x12x2 with identical settings over 8 runs: all four algorithms produced 8/8 distinct mazes, start and end points were 8/8 distinct, and shortest-path length ranged [45, 1, 33, 25, 25, 104, 43, 151] -- min 1, max 151. An algorithm change that made mazes measurably worse would have passed every test. Three independent sources of ungoverned randomness fed generation, none seedable: 1. RandomValueGenerator used a [ThreadStatic] Random seeded from Environment.TickCount. 2. ArrayHelper.Shuffle used Random.Shared -- reached statically from the backtracker, growing-tree and random carver, so there was no injection seam at all. 3. RandomAgent and PerfectAgent called Random.Shared.Shuffle directly. Changes: - IRandomValueGenerator is now the single source of randomness (GetNext, Shuffle, Reseed), injected everywhere, holding per-instance state. The thread-static indirection was also unnecessary: nothing in the pipeline is concurrent. - MazeGenerationSettings.Seed pins a run; MazeGenerationFactory reseeds once before anything consumes randomness, so start/end placement, carving order, wall removal and agent walks are all covered. - MazeGenerationResults.Seed always reports the seed used. Unseeded runs still draw and report a concrete seed, so a maze found by chance is reproducible. - ArrayHelper deleted: its shuffle overloads were the trap and its Average was dead code. Benchmarks now exercise the production shuffle path with a fixed seed. - ISystemClock replaces a DateTime.Now read in MazeStatsSerializer that made serialized stats differ every run -- a golden-file blocker found by the new discipline guard rather than by inspection. Guards: - DeterminismTests: same seed gives identical mazes across all four algorithms; different seeds still differ (seeding must not collapse the output space); a reported seed reproduces its maze; wall removal and agent walks are seed-stable; reusing a container across seeds doesn't leak state. - RandomnessDisciplineTests: scans scripts/maze and rejects Random.Shared, new Random(, ArrayHelper.Shuffle, Guid.NewGuid and DateTime.Now/UtcNow, exempting only the two designated injected sources. Visual regression (tests/visual, @playwright/test toHaveScreenshot): - Harness with canvas-boot detection, COOP/COEP assertions and pixel-stability polling that fails rather than screenshotting a still-animating canvas. - harness.spec.ts self-tests the harness against a local deterministic canvas, so it stays verified without needing a web build (which requires the patched Windows editor). Measured there: a seeded render is byte-identical across fresh pages and across browser launches, but reusing one page across loads perturbs ~1.6% of pixels -- so every case loads into a fresh page. An early version of this suite reused a page and looked like rendering nondeterminism; it was the fixture. - maze.spec.ts targets the deployed build and is SKIPPED until the build accepts generation parameters from the query string (docs/VISUAL_REGRESSION.md specifies the contract). Skipped rather than failing so it cannot report a false red. - CI: visual-harness on every PR (no deploy needed); visual-production/-preview after the existing smoke tests, with report artifacts on failure. Docs: docs/REGRESSION_TESTING.md (four-layer plan, recommended order, open questions), docs/VISUAL_REGRESSION.md (measurements, baseline policy, prerequisite), AGENTS.md (the randomness rule contributors must follow). Verified: full suite 443 passing (421 existing plus 22 new) on .NET 8; benchmarks and experiments projects still compile; visual harness self-test green across repeated runs; all workflow YAML parses. Not verified: the desktop/Godot build (no Godot SDK or .NET 9 available here) and the maze visual suite (needs a deployed build). Separately, suite wall-time under parallel execution is erratic on a 4-core box -- runs from 2.8s to over 300s -- but this reproduces on the pre-change baseline too (122s observed), sequential runs are clean on both sides, and per-test timings match, so it is pre-existing and not from this change. Worth its own investigation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix(test): narrow PerfectAgent sample to stop the suite hanging in CI CI run 9 on this branch hung for 40 minutes with no output after "Found 3 maze files" and was cancelled, leaving the required `test` check red. Root cause, isolated by running the two agent families separately over 8 runs each on the sample mazes: RandomAgent 1768 1662 1724 1738 1702 1755 1666 1644 ms (flat) PerfectAgent 120011 1941 14602 120034 3225 1960 1848 25880 ms (2 timed out) PerfectAgent's search is worst-case exponential: it tracks visited cells per-path via previousPoints.Any(...) -- a linear scan of the current path -- rather than with a shared visited set, so the same cell is re-explored along different paths, and it copies the whole path per branch. Whether it terminates quickly depends entirely on the shuffled direction order, which is why it is intermittent. This is pre-existing, not introduced here: the same workflow recorded 1002s on a push to main on 2026-07-01, against durations of 29-57s for neighbouring runs. Fix (symptom, not cause): - SampleMazeTests: PerfectAgent tests now draw from PerfectAgentMazeFiles() capped at 200 cells, keeping 10x10x1 and dropping 20x20x3 (1200 cells) and 20x20x4 (1600). Exactly 4 test cases removed, verified by diffing --list-tests output; nothing else changed. Total 443 -> 439. - Those two tests carry Timeout(60_000) so a regression fails with attribution rather than stalling. - test.yml jobs get timeout-minutes: 15, so a future hang costs 15 minutes and a clear failure instead of 40 minutes and a bare cancellation. Other PerfectAgent callers are on 5x5x1 (25 cells) or seeded, so unaffected. Verified: 12 consecutive full-suite runs at 8.7-9.7s with zero timeouts, against a pre-fix spread that reached 120s+ on the agent tests alone. 439 passing. The algorithm is still exponential and will resurface on any larger maze -- including in the app, where it would hang the UI rather than a test. Recorded in docs/REGRESSION_TESTING.md as the fix that still needs doing (shared visited set). Also observed once, on the pre-narrowing tree: a single test failure that did not reproduce across 16 subsequent runs, so it is unidentified rather than diagnosed. Noted here rather than left silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * feat(test): in-app test bridge, headless scene tests, Playwright functional suite Adds functional (behavioural) testing, not just visual, across the two layers that had no coverage at all: the Godot scene/UI layer and the deployed browser build. WHY A BRIDGE WAS NEEDED A Godot web export renders everything into a single <canvas>. Menus, buttons and labels are drawn inside it, so Playwright's locator model -- getByRole, getByText, toBeVisible -- sees nothing but one element. Playwright can already *drive* the app with no changes (real key and mouse events reach the engine); what was missing was any way to *observe* it. Without a hook the only possible assertions are "a canvas exists" and pixel comparison. scripts/testing/TestBridge.cs supplies that, opt-in via ?test=1 (or any ?seed=): window.__mazeTestApi "1" when live, absent when disabled window.__mazeState JSON string of app state, republished on change window.__mazeCommand function(jsonString), fire-and-forget Design choices are constraints, not preferences, and each is documented: - Fire-and-forget commands. Godot's docs never state that a create_callback return value reaches the JS caller and no example returns one, so results are observed via state rather than depending on unspecified behaviour. Callbacks must also take exactly one Array argument, which a single JSON entry point satisfies. - No JavaScriptBridge.Eval. The docs say eval "may be disabled in custom export templates" and this project uses a patched template, so GetInterface plus property assignment avoids that path entirely. - Hand-rolled JSON. The web build is trimmed; a reflection-based serializer is exactly what trimming breaks, and it would fail only in the browser. - Protocol split into TestBridgeProtocol (Godot-free) so the NUnit suite can cover the fiddly parsing -- 54 new tests. The Node itself only runs inside the engine. - Also seeds from the URL, which was the outstanding prerequisite for the visual suite. SCENE TESTS, AND WHY NOT gdUnit4Net gdUnit4Net was tried first. On Godot 4.7.1 with gdUnit4.api 5.1.0-rc5 and gdUnit4.test.adapter 3.1.1: the project restores and builds with no conflict, and logic-only [TestCase] tests pass -- but every [RequireGodotRuntime] test fails with "Starting GodotRuntimeExecutor failed. The operation has timed out. Failed to connect: Connection timeout". Setting GodotProjectDir and pre-importing did not help. Isolated the cause: Godot 4.7.1 runs this project headless and executes its C# fine (the GameState autoload's _Ready fires). So the blocker is gdUnit4's own executor, not Godot and not this project -- consistent with its stated support stopping at Godot 4.4.1, last release June 2025. So tests/scene/SceneTestRunner.cs is a ~200-line in-engine runner: a list of checks, a tally, and a process exit code. Compiled only under -p:IncludeSceneTests=true so it never ships in a game export. Delete it in favour of gdUnit4Net once that supports 4.7+; the checks port over almost verbatim. This is the first real coverage of scripts/ui/ (~4300 lines). The nearest thing before was a test reading menu.tscn as *text* and asserting it contained the string "ComparisonButton" -- proving a node name appears in a file, not that it is a Button or that the scene instantiates. VERIFIED BY EXECUTION Correcting an earlier claim of mine: Godot code *is* buildable here. GodotSharp 4.7.1 targets net8.0, so the available .NET 8 SDK compiles it, and the official Godot 4.7.1 Linux build runs it headless. So this is executed, not reasoned: - 493 NUnit tests pass (was 439; +54 for the bridge wire format). - Godot project builds clean, with and without -p:IncludeSceneTests=true, which also proves the runner is excluded from a normal build. - 7/7 scene tests pass in real Godot 4.7.1 headless, exit code 0. Failure path checked too by deliberately breaking a check: 6/7, exit code 1, so CI genuinely gates. - Playwright: all 18 tests across 3 files enumerate (TypeScript compiles); the 4 harness self-tests still pass. - benchmarks and experiments still build; all workflow YAML parses. NOT VERIFIED: whether the bridge behaves under the *patched* web export template. That needs a deploy, and it is the one thing unit and scene tests cannot answer. Both browser suites are therefore gated on MAZE_TEST_BRIDGE=1 and currently skip rather than risk a false red -- see docs/TEST_BRIDGE.md "Enabling in CI" for the three checks to run on a /preview before flipping it. CI: new scene-tests job on every PR, downloading the official upstream Linux editor checksum-pinned in .github/editor-checksums.txt exactly like the patched Windows one. The fork is only needed for the web export. The functional suite runs post-deploy alongside the visual one. Docs: docs/TESTING.md (layer map, push-tests-down rationale, gdUnit4Net findings, known gaps), docs/TEST_BRIDGE.md (contract, per-decision rationale, verification status, security notes), plus VISUAL_REGRESSION.md, REGRESSION_TESTING.md and AGENTS.md updated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * refactor: extract MazeSession from GameState, cover the newly testable behaviour Segregates application behaviour from Godot so it can be integration-tested without the engine, and covers the code that segregation exposes. 493 -> 550 tests. WHY: the codebase was already nearly segregated; the test project just didn't include it. Measured Godot-coupling density in scripts/ui/: AnimationController.cs 254 lines, 0 Godot refs (no `using Godot;`) ImportExportResult.cs 54 lines, 0 MazeImportExport.cs 363 lines, 9 (FileAccess, OS only) PathVisualizationSettings.cs 206 lines, 27 (all one type: Color) MazeMain.cs 1473 lines, 156 (Input. x28, GetNode x24, AddChild x13) GraphViewRenderer.cs 631 lines, 73 (genuine drawing) And GameState -- the app's state hub, 266 lines -- had an *entire* Godot surface of `: Node`, `_Ready`, `_ExitTree`, one GD.Print and one Mathf.Clamp. Nearly all logic, none of it testable, because the NUnit suite builds without the Godot SDK. CHANGES - scripts/session/MazeSession.cs (new): all session state and operations -- settings, current maze, level navigation, alternative paths, animation state, generation and import -- with no Godot dependency. Takes an optional ServiceContainer so tests can inject one. Mathf.Clamp becomes Math.Clamp. - GameState becomes a thin adapter forwarding to it. Its public API is unchanged, so nothing in scripts/ui or scripts/testing needed editing. - PathVisualizationSettings deliberately stays on the node: it is presentation config built on Godot's Color, and its DecisionDetailLevel enum lives in the same file, so moving it would reintroduce the dependency the split removes. GameState resets those presentation flags itself after delegating the session reset. - Test project now compiles MazeSession, AnimationController, ImportExportResult and GraphLayoutType. Adding a file to that list is a claim that it is Godot-free, and the build enforces the claim. TESTS - AnimationControllerTests (30 tests): a 254-line playback state machine that had zero coverage -- not because it was hard to test but because of where it lived. Time is a Update(deltaTime) parameter, so no clock or waiting is involved. Covers transport controls, stepping bounds, time-driven advance, speed clamping, notifications, and pins two real behaviours: a large frame delta advances only one step, and replay after finishing rewinds. - MazeSessionTests (24 tests): whole flows, previously impossible -- generate then navigate then regenerate, import adopting its own size, import clearing state left from the previous maze, animation driven through session state. A REAL BUG, FOUND ON THE FIRST RUN GameState.LoadImportedMaze applies dead-end wrapping and then builds a graph. That combination threw "Nullable object must have a value", so **importing any maze crashed the app**. GraphBuilder.GetGraphEdges walks a corridor until it reaches a junction or the start/end. A dead-end cell has exactly one direction -- the one you arrived from -- so it is neither a junction (needs more than two) nor a terminus, and the walk dereferenced a null Direction?. Dead-end wrapping *creates* such cells by hiding passages. Isolated it: GetGraph on a plain maze passes; after DoDeadEndWrapping it throws, on generated and imported mazes alike. Several existing tests did wrapping, and several did graph building -- none did both, which is how it survived. Fixed by dropping the edge when a corridor dead-ends, rather than pointing it at the dead-end cell: the graph's node set is junctions plus start/end, and consumers look edges up with graph.Nodes[edge.Point], so an edge to a non-node would trade one crash for a KeyNotFoundException. GraphBuilderDeadEndTests covers it, including that every edge still targets a known node and that wrapped mazes remain solvable. VERIFIED BY EXECUTION - 550 NUnit tests pass (was 493). - Reverting only the GraphBuilder guard fails 10 tests while the plain-maze control still passes, so the regression tests are genuinely coupled to the fix. - Godot project builds clean; 7/7 headless scene tests still pass in Godot 4.7.1; benchmarks and experiments build. Docs: docs/TESTING.md gains the coupling measurements, the three-tier plan, the MazeSession rationale and the bug write-up; AGENTS.md records the rule that behaviour belongs in plain C# with Node subclasses as thin adapters. Next in this direction, deliberately not done here: abstract FileAccess/OS in MazeImportExport so import/export round-trips become testable, then pull orchestration out of MazeMain incrementally. GraphViewRenderer's drawing should stay where it is -- render code's contract is the pixels, which is what the visual suite is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix: bound end-point selection so a 1x1x1 maze cannot hang generation MazeModelFactory.BuildMaze picked the end point with an unbounded retry: while (startPoint.Equals(endPoint)) endPoint = _randomPointGenerator.RandomPoint(settings.Size, pickType); A single-cell maze has exactly one point, so a distinct end point can never be drawn and generation spins forever. A hang rather than a crash, so it produced no stack trace and no error -- just a frozen app. Reachable by importing a `.maze` file with `SIZE 1 1 1`, which the format explicitly permits ("all values must be positive integers"). FIX PickDistinctEndPoint replaces the loop with two guards: - A maze of one cell returns the start point. Degenerate but finite, and callers already tolerate start == end (agents check before moving). - Otherwise retries are bounded (100 draws) and fall back to a deterministic neighbour. "Retry until different" is unsafe in general: random draws can be arbitrarily unlucky, and PickType.RandomEdge samples a small subset of cells, so the unbounded form was a latent stall on larger mazes too. HOW IT WAS FOUND An exploratory sweep over sizes and option combinations -- the same technique that surfaced the GraphBuilder dead-end crash, and cheap to run now that MazeSession makes a full generate/wrap/graph/serialise/import cycle a few lines of plain C#. Isolating it showed generation itself hangs at 1x1x1 regardless of agent, while every neighbouring size (1x2x1, 2x1x1, 1x1x2, 2x2x1) was already fine -- which is why nothing caught it. Worth recording: the first sweep appeared to produce no output at all, because a hung test never flushes TestContext.Out. Writing each case label to a file *before* running it made the last line name the culprit. That technique is now in docs/TESTING.md. VERIFIED - 556 NUnit tests pass (was 550; +6 degenerate-size regression tests, each with a Timeout so a hang regression fails this fixture instead of stalling the whole job). - The sweep now completes clean: 418 combinations, 0 failures -- 4 algorithms x 3 maze types x 7 sizes including degenerate axes x wall-removal x door placement, plus every agent and solver/heuristic pairing. Before the fix it hung on the first case. - Two-cell mazes still get distinct endpoints, so the guard does not over-apply. - Godot project builds clean; 7/7 headless scene tests pass in Godot 4.7.1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * perf(agent): make PerfectAgent linear instead of exponential PerfectAgent tracked visited cells *per path* -- previousPoints.Any(x => ...), a linear scan of the current route -- rather than once per search. A cell reachable by several routes was re-explored once per route, making the search exponential in the worst case. It also rebuilt the whole path per branch via Concat().ToList(), an O(path) allocation per step. Measured before, 8 runs of the two sample-maze tests (1200 and 1600 cells): 120011 1941 14602 120034 3225 1960 1848 25880 ms (two hit the 120s cap) RandomAgent over the same runs was flat at 1.6-1.8s, which is how the cause was narrowed to this class. This is what made CI wall-time range from 29s to a 40-minute hang on identical code, including a 1002s run on main long before this work. CHANGES Rewritten as an iterative depth-first search with a shared visited set: - Shared visited set: each cell is explored at most once, so the walk is linear in the number of cells rather than exponential in the number of routes. - Explicit stack rather than recursion: depth previously tracked path length, so a large maze risked a stack overflow independently of the blowup -- the app allows 50 cells per axis, tens of thousands of cells. - Path is mutated with backtracking instead of copied per branch. Behaviour deliberately unchanged: still "the first route DFS finds", not a guaranteed shortest path. In a perfect maze exactly one simple route exists between two cells, so that route is the shortest, which is what the existing tests assert. With wall removal the maze has loops and DFS may return a longer route -- equally true before. Direction order is still shuffled through the injected generator, so a seed still reproduces the walk. Also returns an empty walk when no route exists, rather than a partial one, so a caller cannot mistake a dead search for a real route. WORKAROUND REMOVED The earlier mitigation narrowed the PerfectAgent sample to <= 200 cells, dropping 4 test cases. With the cause fixed those are restored, so the tests now run against the full sample set including the 1200- and 1600-cell mazes. The 60s per-test cap and timeout-minutes: 15 on the CI job stay as tripwires. VERIFIED after: 2590 1896 1981 2167 1911 1847 2036 2824 ms (zero timeouts) - 560 tests pass (was 556; +4 restored cases). Full suite 8s -> 1s. - Godot project builds clean; 7/7 headless scene tests pass in Godot 4.7.1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * ci: auto-detect the test bridge on deploy, add editor mirror workflow Closes the two remaining roadmap mitigations that did not need upstream progress. TEST BRIDGE AUTO-DETECTION (.github/smoke/smoke.mjs) Whether the bridge works under the *patched* web export template was the one thing unit and scene tests could not answer -- it needed a deploy, and therefore a human remembering to check. The smoke test now loads ?test=1 after its existing checks and reports the answer in the job log and the GitHub step summary: TEST BRIDGE PRESENT -- set MAZE_TEST_BRIDGE=1 to enable the browser suites TEST BRIDGE ABSENT -- window.__mazeTestApi never became "1" Reported, not asserted. The bridge is an automation aid and a build without it is still a working build, so failing a deploy over it would block releases for no user-visible reason. Every deploy now answers the question for free instead of waiting on a manual check. EDITOR MIRROR (.github/workflows/mirror-editor.yml) The export needs a Windows editor build from a third-party fork. That fork is actively maintained -- it matched upstream 4.7.1 within two days -- but "maintained today" is not "available forever", and a deleted or re-published release breaks the export outright. The checksum pin turns a silent substitution into a hard failure but cannot bring the bytes back. The workflow downloads the pinned editor, **verifies it against the SHA-256 in .github/editor-checksums.txt before uploading**, and republishes it as a release here. Verifying first matters: mirroring unverified bytes would launder a compromised upstream into a trusted location, which is worse than having no mirror. Idempotent, so re-running replaces the asset. Resolves the tag and source repo from .github/web-toolchain.env by default, so a mirror cannot drift from the version actually being built with. Deliberately workflow_dispatch rather than scheduled: it is a ~165 MB upload done once per pinned version. **Still needs one manual run**, after which GODOT_FORK_REPO in web-toolchain.env points at this repo -- a one-line change, since the export action already treats the repo as config. Docs: TEST_BRIDGE.md documents the auto-detection; WEB_EXPORT_ROADMAP.md marks the mirror built, with the remaining manual step called out rather than implied. Verified: 560 tests pass; smoke.mjs passes node --check; all workflow YAML parses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwhKDz2xEVEtYECxT7q5F3 * fix(test): run one fixture instance per test case, fixing a parallelism data race `[assembly: Parallelizable(ParallelScope.All)]` was combined with NUnit's default SingleInstance lifecycle, so test cases in a fixture ran concurrently against one shared instance. Every field assigned in `[SetUp]` was therefore a data race. It surfaced as two NullReferenceExceptions in RandomValueTests on CI, on a commit whose other runs of the same job were green — re-running the identical commit passed, confirming a race rather than an environment difference. The mechanism: _mazePointFactory = new Mock<IMazePointFactory>(); // (a) bare mock _mazePointFactory.Setup(x => x.MakePoint(...)).Returns(...); // (b) configures it _randomPoint = new RandomPointGenerator(_random, _mazePointFactory.Object); // (c) re-reads the field Another test's (a) landing between this test's (b) and (c) makes (c) capture an unconfigured mock. Moq then returns default(MazePoint) — null — and the test dies dereferencing `point.X`, nowhere near the actual cause. Measured with a trace harness (NUnit's per-test console capture reorders output and hides this): with SingleInstance, four concurrent `[SetUp]` bodies share one fixture instance; with InstancePerTestCase, four concurrent tests get four distinct instances. Parallelism is unchanged, the unsafe sharing is gone. Also drops MovementHelperTests' `[NonParallelizable]`, which was a local workaround for this same root cause, and adds a guard test so removing the attribute fails immediately by name instead of resurfacing as an occasional unexplained flake. * docs(test): record the fixture-lifecycle requirement and how it was diagnosed * docs: refresh unit-test count for the lifecycle guard test * chore: gitignore node_modules and Playwright run output This branch and `main` had no Node or Playwright ignore rules, so nothing stopped an `npm ci` in tests/visual/ from being committed. That is exactly how 170 files / 17.7 MiB of node_modules got into the GitHub-Actions branch (#7), which is also based on `main`. Later branches in this stack do carry `tests/visual/`-anchored rules, added with the visual-regression harness. These are deliberately repo-wide rather than anchored: the trap is a node_modules appearing somewhere the anchored pattern doesn't cover. Baseline screenshots under *-snapshots/ stay tracked; only regenerated run output is ignored. * ci: pin the actions used by this stack's new jobs #7 pinned every action to a commit SHA, but the jobs added on this stack (scene-tests, visual-harness, mirror-editor, the visual/functional deploy jobs) were written against floating v4 tags and merged in unpinned. Same SHAs as main, so the convention holds across every workflow rather than most of them. * feat: About screen reporting the build hash, and verify it end to end Adds an About screen (menu → About, or Escape to leave) that shows the commit the running build was produced from, so a served build can be identified rather than assumed. The full 40-character SHA is shown, not a prefix: the screen exists to be compared against a workflow run, and abbreviating weakens that for no gain. WHY A COMPILED-IN STAMP, NOT A DATA FILE The obvious design — ship build-info.json inside the app and read it at startup — does not survive this project's web export. export_presets.cfg uses export_filter="all_resources" with an empty include_filter, so a plain .json/.txt file is never packed into the PCK. It would work on desktop and silently report "unknown" in the browser, which is the one place the information is needed. The values are therefore MSBuild-generated constants (GenerateBuildStamp target). CI passes them as ENVIRONMENT variables rather than -p: arguments, because the export re-enters MSBuild through the Godot editor, whose command line the workflow does not control; MSBuild reads env vars as properties, so that is the one channel reaching both the explicit dotnet build and the editor's own. NOT LYING IS THE POINT BuildInfo.IsOfficial requires both a full 40-char hex SHA and a CI run id, since only CI can supply the latter. An unstamped build shows an explicit "local build — not produced by CI" row instead of presenting a stale or placeholder hash as verifiable. Both directions are asserted. VERIFICATION BEYOND THE APP Deploying is not serving: Vercel promotes an alias after upload, so a partial promotion leaves the old build live while the job still reports success. - build-info.json is written beside index.html, so the served build can be identified with curl without downloading ~96 MB of WASM. - The test bridge publishes buildCommit/buildBranch/buildTime/buildRunId, unconditionally — a check needs them before the app has done anything. - The smoke test now ASSERTS the served commit equals the one just built, and cross-checks the sidecar JSON against the commit compiled into the WASM. They come from the same inputs, so a disagreement means the two halves of the deploy are from different builds, which checking either alone would miss. Asserted rather than reported, unlike the bridge probe: serving the wrong build is a real user-facing defect. Also fixes .gitignore: the unanchored `build/` rule matched any directory of that name at any depth and was silently excluding scripts/build/ from git. Anchored to /build/, which is what the "web export output" comment intended. Verified by execution: env-var injection reaches the generated constants; unset properties still compile; WriteOnlyWhenDifferent keeps incremental builds incremental; building stamped made the scene test's "unstamped ⇒ Provenance" case fail and only that case, proving the value travels through engine, autoload and UI; 9/9 headless scene tests pass in Godot 4.7.1 in both stamped and unstamped builds; 584 unit tests pass. * fix(ci): self-derive the build stamp, and make the smoke test's boot check real Both problems were found by the /preview run on this PR; neither was visible from any green check. 1. THE BUILD STAMP ARRIVED EMPTY build-info.json on the preview deploy: { "commit": "", "branch": "", "repository": "", "runId": "", "builtUtc": "2026-07-26T12:12:49Z", "godotEditor": "4.7.1-stable" } Only the fields computed inside the composite action survived. Cause: for issue_comment events GitHub loads the WORKFLOW FILE from the default branch, while a composite action is read from the checked-out PR head. The `with: build_commit:` block added to web-export.yml on this branch was therefore ignored entirely — the export succeeded and honestly reported itself as an untraceable build. The action now derives the stamp itself: the commit from `git rev-parse HEAD` (the thing actually checked out, by definition), the repository and run from the github context. Caller inputs remain as overrides. A detached checkout leaves the branch blank rather than reporting "HEAD", since a wrong branch is worse than none. It also exposes a build_commit output, and both smoke jobs now assert against that instead of github.sha — which is the default branch, not the PR head, on an issue_comment run. smoke-preview previously asserted no commit at all. 2. THE SMOKE TEST NEVER CHECKED THAT ANYTHING BOOTED () => { const c = document.querySelector("canvas"); return c && c.width > 0 && c.height > 0; } An untouched <canvas> defaults to 300x150, so this was true before the engine did anything. On the preview it returned in 0.02s, logged canvas: {"w":300,"h":150} — the default — and printed SMOKE PASS. Every "the deploy boots" result this check ever produced was unproven, on main as well as here. It now waits for the canvas to stop being the default size, which only the engine can cause, and on timeout reports the stuck dimensions and where to look. Also hardens the build-info.json read: a host answering an unknown path with index.html and HTTP 200 (an SPA catch-all) made `.json()` throw a bare SyntaxError and would have failed a deploy for the wrong reason. Found while testing the fix. Verified against a local server with COOP/COEP, four cases: a canvas that never resizes fails with the diagnostic; one that resizes passes; a commit mismatch fails naming both commits; a non-JSON sidecar warns and passes. 584 unit tests still pass. * ci: let an automated caller issue a preview deploy via workflow_dispatch `/preview` is an issue_comment trigger gated on author_association == 'OWNER', so an agent with GitHub API access but no owner identity cannot use it. workflow_dispatch can do the job, and for verifying a branch it is strictly better: an issue_comment run always takes the workflow file AND the checkout from the default branch, so it cannot exercise a branch's own changes to this workflow or to .github/smoke/ — which is precisely how a build stamp added on a branch arrived empty and a vacuous boot check survived unnoticed. Two things had to change before this was safe and useful: DEPLOY TARGET IS NOW STATED, NOT INFERRED The deploy step chose production by testing `github.ref == refs/heads/main`. With dispatch as a normal way to get a preview, that left a dispatch on the wrong branch one step from silently publishing to maze.ryankelly.dev. There is now a deploy_target input defaulting to `preview`; `production` is rejected on any ref other than main with an explicit message. push-to-main still deploys production, so existing behaviour is unchanged. THE RESULT IS READABLE WITHOUT PARSING LOGS The URL previously existed only as a job output and a line amid vercel's chatter. The deploy step now writes a step-summary table (URL, commit, link to build-info.json) and prints DEPLOY_URL=/DEPLOY_TARGET= on their own lines; the smoke step adds a "Served build" table with the served commit, build time, branch and editor tag. A caller inspecting the run reads one short summary instead of a whole job log. Also documents, in AGENTS.md and docs/BUILD_VERIFICATION.md, that a sandboxed agent may be unable to reach *.vercel.app at all (this environment denies it at the proxy) and that the correct response is to read the CI verdict rather than route around the network policy — which is why those checks are asserted in CI in the first place. * test(web): enable the functional browser suite; gate visual on baselines instead The bridge is confirmed working inside the patched web export — verified on a real preview deploy (run 30216793098): window.__mazeTestApi became "1" and __mazeState reported ready:true with the build stamp. That was the last unknown blocking these suites and the reason both were pinned to MAZE_TEST_BRIDGE=0, so the functional suite is now on for every deploy. The visual suite is NOT enabled, for a reason that has nothing to do with the bridge: no baseline PNGs are committed (only the harness self-test one), and Playwright fails a missing snapshot on CI rather than writing one. Flipping it would have reddened every deploy for a reason unrelated to the build. It now gates on its own flag, MAZE_VISUAL_BASELINES, so the two prerequisites cannot be confused again — and the remaining work is a documented procedure rather than a vague "skipped until confirmed". Baselines have to be captured from a deploy, since the web build cannot be produced on Linux. docs/VISUAL_REGRESSION.md now gives the steps, including two things worth stating explicitly: inspect every generated PNG before committing (a baseline taken from a broken build makes the breakage the expected result, so the suite would then go red only when the bug is fixed), and commit the flag flip in the same commit as the files so they cannot disagree. Corrects the now-stale claims in TEST_BRIDGE.md ("❌ unproven — needs a deploy", "Before flipping it, confirm..."), TESTING.md and VISUAL_REGRESSION.md. Verified: specs enumerate (18 tests, 3 files); with MAZE_VISUAL_BASELINES=0 the 5 visual tests skip, with =1 they run. * refactor(ci): move step bodies out of YAML into script files Every `run:` body beyond a one-line command now lives in a real file — 10 scripts beside the composite action, 7 under .github/scripts — invoked from a one-line `run:`. action.yml drops 416 -> 218 lines, mirror-editor.yml 117 -> 70. WHY, CONCRETELY A long script inside a YAML block scalar cannot be linted, cannot be run outside CI, and some constructs do not survive it at all: a PowerShell here-string needs its terminator at column 0, which ends the block. This repo already hit that once and worked around it by building an array and joining with newlines. EXTRACTED MECHANICALLY, NOT RETYPED Bodies were lifted verbatim by a script and then diffed against the originals, so the move cannot have quietly changed logic. All 17 check out; the only intended difference is the preview deploy, which now shares the production script. THREE THINGS THIS SURFACED - `run: & "..."` is invalid YAML. A bare `&` is the anchor indicator, so the invocations are single-quoted. Caught by parsing the files, not in CI. - A script cannot see `${{ ... }}`, so BUNDLE_DIR, GODOT_EXE and IN_TARGET had to become `env:`. Missing one does not error — the script reads an empty string and does something subtly wrong. - The two Vercel deploy steps were near-duplicates. They now share deploy-vercel.ps1, so a /preview gains the same target guard, DEPLOY_URL marker and step summary the dispatch path already had. IN_TARGET is pinned to "preview" there rather than left unset, so a /preview cannot reach production and the behaviour does not depend on how PowerShell reads an absent variable. A COMMITTED GUARD, NOT A ONE-OFF CHECK .github/scripts/check-workflow-scripts.py asserts that every step invoking a script names one that exists and provides every environment variable it reads. It runs on every PR. Verified in both directions: 18/18 satisfied as committed, and removing BUNDLE_DIR makes it exit 1 naming the step and the variable. Scripts also set `$ErrorActionPreference`/`$PSNativeCommandUseErrorActionPreference` (pwsh) and `set -euo pipefail` (bash) explicitly rather than inheriting them from Actions' shell wrapper, so they behave the same run by hand and a failing native command is not ignored. Not executed here: no pwsh in this environment, so the PowerShell is verified by faithful-move diffing, YAML parsing and the env-wiring guard rather than by running it. The bash scripts pass `bash -n`. A real dispatch follows. --------- Co-authored-by: Claude <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
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.
GitHub warns that
actions/checkout@v4andactions/setup-dotnet@v4target Node 20 and are being force-run on Node 24. This moves every action to its current major (allnode24) and pins each to a commit SHA with a version comment, matching the convention already used across the other repos in this account.actions/checkout3d3c42e5actions/setup-dotneta98b5685actions/setup-node82076278actions/cache55cc8345actions/upload-artifact043fb46dFiles:
.github/workflows/test.yml,.github/workflows/web-export.yml,.github/actions/export-web/action.yml. Version and pin changes only.Why pin
A moving major tag can be repointed at new code by whoever controls the action, so the tag alone isn't a supply-chain control. This repo already checksum-pins the third-party Godot editor binary for exactly that reason — the actions were the remaining gap, and this repo was the only one in the account not pinning (0 of 13, against 100% in
blog,github-actions,platform,rights-application,sentric-frontends,workspace,data-platform).Full semver in the comment, matching the dominant form in
platform/github-actions/the-vault-systemrather than the bare-major form inblog— the exact patch is what a reader wants to check.Breaking changes checked
Multi-major jumps, so the release notes were read rather than bumping blind. Four changes are relevant; none bite here:
checkoutv5+ refuses to check out fork PR code by default, needingallow-unsafe-pr-checkout: true. I checked the exact condition because the/previewjob checks out PR head with secrets in scope — the guard fires only onpull_request_targetandworkflow_run, neither of which this repo uses, so/previewis unaffected.setup-dotnetv5+ dropped older .NET. Repo pins9.0.x, targets net8.0/net9.0 — fine.setup-nodev5+ auto-enables caching whenpackage.jsonhas apackageManagerfield (npm-only from v6). Nothing here sets it, so no new lockfile requirement.cache/upload-artifactv5+ need Actions Runner 2.327.1+. GitHub-hosted only, well past it.Verification
The failure mode of this convention is a comment that drifts from its SHA, so both directions were checked:
checkoutv7.0.0,setup-nodev7.0.0,cachev6.1.0,setup-dotnetv5.2.0) from upstream tags and matched them byte for byte.platform(setup-nodev7.0.0,cachev6.1.0) — corroborated by a second source.action.ymlat each new major and cross-checked all 11 steps: every input this repo passes still exists,cache@v6still exposes thecache-hitoutput the export action reads,upload-artifact@v7still acceptsif-no-files-found.testcheck green (421 passed), and the Node 20 deprecation warnings are gone from the log.test.ymlruns on a pull request, so the checks exercisecheckout@v7.0.1andsetup-dotnet@v6.0.0only. Thesetup-node,cacheandupload-artifactpins live inweb-export.yml, which runs only on merge tomainor an explicit/preview— validated by schema, not execution. Worth a/previewbefore merging if you want them exercised.Notes
checkout/upload-artifactgo to v7.0.1 rather than the v7.0.0 pinned elsewhere, andsetup-dotnetto v6.0.0 rather than v5.2.0. These are current latest; happy to align downward instead if cross-repo uniformity matters more.mainso it stands alone, but fix: two product bugs (1x1x1 hang, exponential PerfectAgent) + ops hardening #6 adds new jobs usingsetup-node/upload-artifactat floating v4 tags. Whichever merges second needs aligning, and both touch overlapping regions of the same two files. I'd merge this first, then rebase fix: two product bugs (1x1x1 hang, exponential PerfectAgent) + ops hardening #6 onto it.