fix: two product bugs (1x1x1 hang, exponential PerfectAgent) + ops hardening - #6
Merged
Merged
Conversation
….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
…ual 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
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
…tional 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
…e 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
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
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
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
rtkelly13
changed the title
rtkelly13
changed the base branch from
main
to
claude/split-3-testability-3a6h5m
July 25, 2026 21:29
…sm 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.
…testability-3a6h5m
…2-determinism-3a6h5m
…sm-hosting-eval-3a6h5m
…sm-hosting-eval-3a6h5m
…sm-hosting-eval-3a6h5m # Conflicts: # AGENTS.md # docs/TESTING.md
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.
…2-determinism-3a6h5m # Conflicts: # .gitignore
…testability-3a6h5m
…sm-hosting-eval-3a6h5m
…2-determinism-3a6h5m
…testability-3a6h5m
…sm-hosting-eval-3a6h5m
#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.
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.
Owner
Author
|
/preview |
|
🔗 Preview deployed: https://maze-3tkv7zvv7-rtkelly13s-projects.vercel.app |
|
✅ Smoke test passed on the preview (boots, cross-origin isolated, canvas rendered). |
…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.
`/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.
…nes 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.
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.
…a6h5m Both this branch and main had extracted the composite action's inline PowerShell into script files; this branch's extraction (17 scripts, mechanically lifted and diffed) supersedes main's 8, so its versions are kept and main's now-unreferenced download-editor.ps1 is removed. Re-adds main-only .gitignore entries and the AGENTS.md Repository Conventions section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Stack 4 of 4. Rebased onto #10 so this now shows only its own diff — the four earlier concerns are split into #8 → #9 → #10 → this. Same branch and history; only the base changed.
Two genuine product bugs, both found by exploratory sweeping rather than by reading code, plus the two remaining ops mitigations.
Bug 1 — generating a 1×1×1 maze hung forever
MazeModelFactory.BuildMazepicked the end point with:A single-cell maze has exactly one point, so a distinct end point can never be drawn. A hang, not a crash — no stack trace, no error, just a frozen app. Reachable by importing a
SIZE 1 1 1file, which the format explicitly permits.Every neighbouring size (1×2×1, 2×1×1, 1×1×2, 2×2×1) was already fine, which is why nothing caught it.
Fixed with a cell-count guard plus bounded retries and a deterministic fallback. "Retry until different" is unsafe in general:
PickType.RandomEdgesamples a small subset of cells, so the unbounded form was a latent stall on larger mazes too.Bug 2 —
PerfectAgentwas exponentialIt tracked visited cells per path (
previousPoints.Any(...), a linear scan of the current route) rather than once per search, so a cell reachable by several routes was re-explored once per route. It also rebuilt the whole path per branch.120011, 1941, 14602, 120034, 3225, 1960, 1848, 25880ms — two hit the 120s cap2590, 1896, 1981, 2167, 1911, 1847, 2036, 2824ms — zero timeoutsFull suite dropped 8s → 1s. Now an iterative DFS with a shared visited set; the iterative form also removes a stack-overflow ceiling that scaled with path length (the app allows 50 cells per axis).
Behaviour deliberately unchanged: still "the first route DFS finds". 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.
The workaround from #9 is removed, restoring the 4 dropped test cases so the tests run against the full sample set again. The 60s cap and
timeout-minutes: 15stay as tripwires.How both were found
Combination sweeping — the technique that found the
GraphBuildercrash in #10.MazeSessionmakes a full generate → wrap → graph → serialise → import → stats cycle a few lines of plain C#, so the option space can be swept in seconds. After the fixes: 418 combinations, 0 failures (4 algorithms × 3 maze types × 7 sizes including degenerate axes × wall-removal × door placement, plus every agent and solver/heuristic pairing).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's now indocs/TESTING.md.Ops hardening
Test bridge auto-detection (
smoke.mjs) — whether the bridge works under the patched template was the one thing unit and scene tests couldn't answer. The smoke test now loads?test=1and reports the answer in the job log and step summary. Reported, not asserted: a build without the bridge is still a working build, so failing a deploy over it would block releases for no user-visible reason.Editor mirror (
mirror-editor.yml) — the export depends on a third-party fork's release staying published. The workflow downloads the pinned editor, verifies it against the pinned SHA-256 before uploading (mirroring unverified bytes would launder a compromised upstream into a trusted location), and republishes it here. Idempotent, resolves the tag fromweb-toolchain.envso a mirror can't drift.workflow_dispatchrun (~165 MB upload), then pointGODOT_FORK_REPOat this repo — a one-line change.Verification
Timeoutso a hang regression fails the fixture rather than stalling the job; +4 restored cases).Still open
/previewcan't be triggered from here — the workflow requires anOWNERcomment. Once you run one, the smoke test reports whether to flipMAZE_TEST_BRIDGE=1and un-skip 14 browser tests.