Repository navigation
IIT-21-Shifting-Mosaic-Solver - #21
Conversation
Runs the checks in .clang-tidy on every TU and fails the build on any
finding, under Ninja/Makefiles. Differs from the usual snippet in four
ways, each a real failure first:
* no -checks argument, which would override .clang-tidy entirely and
enable the contradictory llvm/fuchsia/altera families
* find_program, since clang-tidy is not on PATH here
* a generator gate, because the Visual Studio generator silently
ignores CXX_CLANG_TIDY
* --extra-arg=-EHsc on MSVC: clang-tidy discards every slash-prefixed
flag after the `--` separator, so /EHsc never arrived and the tree
was analysed as -fno-exceptions, making every try/throw in the tests
a clang-diagnostic-error
Enforcement is pinned to the clang-tidy major the profile was generated
against; other hosts warn and build unchecked rather than failing on an
unknown check name. Every gate degrades to a warning, never a configure
error.
Also resolves the findings the profile surfaced, with no suppressions
added: validated env parsing in place of atoi, seeded test RNGs via
testSeed(), member initialisers, and a wasm-only #if around the heap
tripwire.
Verified: full Ninja build clean with -warnings-as-errors=* in both
Debug and Release, gate proven live by a deliberate violation, 71/71
tests in Release.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main() was 545 lines at cognitive complexity 174 against SonarLint's limit of 25, plus three nesting-depth findings, three nested ternaries, four uncommented empty callbacks and a loop whose counter the body advanced. Moving the 41-branch flag chain would not have helped: chained branches cost the same wherever they live. It becomes a table instead, so the parse loop is a lookup, and the seven-rung cascade becomes a stage table for the same reason. Engine setups move to BenchEngines.cpp, options to BenchOptions.cpp, and main() is now the order they run in. Behaviour is unchanged, verified against a binary built from the parent commit: 45 differential cases (8 deterministic engines x 2 fixtures, 27 individual flags, 2 arms) produce byte-identical JSON once timing fields are excluded; --generate, --dump and --verify produce identical files; the four error paths keep their exit codes. Only 12 of the 27 flags were observable on that fixture, so the flag -> field mapping of all 41 was additionally compared mechanically against the old chain. One intentional difference: a missing or malformed option value now also prints the usage line before exiting 2, where before it exited 2 with only the specific error. stdout is untouched, so the bench harnesses that parse it are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
runGenerate computed to roughly 50 against the limit of 25, most of it from three lambdas whose bodies nest inside their owner. Extracting them to free functions (randomShape, tryPlace/overlapsAt, shuffleBoard, fixtureJson) both lowers the count and settles the depth question: the placement retry is three levels of loop-and-test that only reads as three once it is not also nested inside runGenerate. The order of draws from the shared mt19937 is load-bearing, since fuzzShiftingMosaic.ts addresses boards by seed. Verified unchanged across 40 --generate seed/shuffle pairs (10 seeds x 4 shuffle lengths): fixture bytes and summaries identical to the pre-refactor binary. The 45-case CLI differential still passes too. BenchEngines.cpp had two findings the IDE reported and I had not predicted: S1186 wants the explanatory comment INSIDE an empty body, not above it, and a const std::string& parameter should be a string_view. Both fixed; isKnownEngine got the same treatment for consistency. The build's clang-tidy gate separately caught a raw loop in the new overlapsAt that should be std::ranges::any_of. main.cpp, BenchOptions.cpp and BenchEngines.cpp now report zero diagnostics in the IDE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`guard-- > 0` as the right-hand operand of && is only evaluated when the left side holds, so the retry count could only be read correctly by reasoning about short-circuiting. The decrement moves to the first statement of the body. Moving it into a for-header instead was worse: the condition also tests cells.size(), which the body grows, and that trips the "loop is error-prone" rule. A while loop avoids both. Iteration count is unchanged, and --generate still reproduces byte -identical fixtures across the 40 seed/shuffle pairs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tryRunPairCancellation was ~30 against the limit of 25 and, more to the point, five levels of nesting: pair loop, pair loop, overlap loop, copy loop, skip test. Extracting the candidate builder alone would have fixed the count and left the depth, so the per-pair attempt becomes tryCancelPair and the outer function is just the pair enumeration. tryReorderMerge was ~29, almost all of it the two inline candidate builders; they become runPulledEarlier and runPushedLater. The odd comparison of countSteps() against runs.size() is preserved as-is -- it mixes two different notions of "step", but that is existing behaviour and not this change's business. Verified: 8 optimizer unit tests pass and the 45-case CLI differential is still byte-identical, which covers this since the reported turns/steps/dirRuns are post-processed values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
S2245 fired on every std::mt19937 mention: 5 in BenchCommands.cpp and 12 more across the test files. Its suggested fix -- libsodium randombytes_uniform(), Botan randomize() -- is not applicable here, and not because of speed. Those seed themselves from the OS and cannot be seeded deterministically, and seedability is the whole point: --generate --seed N has to keep producing the same board because fuzzShiftingMosaic.ts addresses boards by seed, and a failing fuzz board is only debuggable if it re-runs from its seed. Nothing drawn here guards a secret. So the engine moves behind SeededRng, which reduces 17 findings to one suppression on a single member declaration, with the reasoning stated once. Every call site stays fully analysed rather than carrying its own NOSONAR. draw() forwards to dist(engine), so reusable distributions keep their exact draw sequences. Verified: --generate is byte-identical across 40 seed/shuffle pairs, and 71/71 tests pass -- including the ones seeded with the same value as the oracle tests, which only agree if instance generation is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AStar.cpp:317 was changed to std::vector(total, UINT16_MAX) in 2c3f580 by applying a "template arguments can be deduced" suggestion. UINT16_MAX is an int, so that deduces vector<int>, which does not match the vector<vector<uint16_t>> member. MSVC accepts it and clang does not, so the native build, the 71-test suite and the clang-tidy gate all stayed green while the shipped browser build could not compile at all. The element type is spelled out again with a comment saying why it must not be deduced. The other 17 CTAD sites declare the variable itself, so the deduced type IS the variable's type; this one was an argument to assign(), where the target type was already fixed. That is the shape to watch for. wasm_bindings.cpp: solve() was ~228 lines. Its arms were lambdas, which read like extraction but are measured as part of their owner, so they become real functions over a SolveContext. The nested conditional picking the drag entry point becomes a DragMode enum and an if-chain, and each cascade (threaded and sequential) is its own function behind the existing #ifdef. Verified: em++ compiles both the pthread and non-pthread variants, the full wasm build succeeds for all four targets, and 44/44 wasm tests pass -- with test37 reporting the same 531->419 moves / 110->100 steps as the native binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The loop counter is size_t so it cannot be too small for its bound, but block ids are uint8_t, so passing it straight to removeBlock/floodFill narrows implicitly. Cast once at the use, matching how the other loops in this file already handle it. Reported by clangd rather than SonarLint or the build gate: MSVC does not warn here, and clang-tidy only errors on diagnostics the compiler flags enable, so this class of finding is visible in the IDE only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cognitive complexity 68 against the limit of 25, plus a nesting-depth
finding and S135 (at most one break/continue per loop) on the innermost
slide loop, which had three breaks and a continue.
Three extractions:
* slideToNextState -- the slide itself. Its two blocked exits became
returns, so the loop keeps a single break, the one meaning "reached
a real successor".
* searchLimitReached -- the budget and ceiling guards. Deliberately
NOT a per-iteration call: the mask test stays at the call site and
the node-cap compare stays inline, so this runs once per 256
iterations. Hoisting the whole guard set into a per-iteration call
is what made an earlier attempt at this 8% slower. Cadences are
unchanged, since any multiple of 4096 is also a multiple of 256.
* expandNode -- successor generation. Required rather than optional:
the while loop is itself nesting level 1, so the two `for`s and any
`if` inside them were already four deep and nothing done INSIDE the
loop could fix that. Templated on the store and heap types so
AStar.h need not name HeapEntry, which is file-local here.
Verified by interleaved A/B, min-of-5, alternating which binary runs
first: unit +0.1%, drag -0.0%, hier +0.1% against a +5% threshold, with
drag and hier serving as untouched controls that calibrate the noise
floor. nodesExpanded is identical on every leg, so the search visits the
same states. 71/71 tests, and em++ compiles both the pthread and
non-pthread variants.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One 460-line function carrying the whole drag-space A*. Split into named phases declared in DragSolver.h and defined beside their only caller, so the compiler still sees every definition and inlines the hot ones. Two phases take a small context struct rather than a parameter list: runAStarDrag already sits at the 7-parameter limit, so passing its locals individually would have traded one rule for another. The 1-in-256 budget mask test stays at the call site -- it exists so the common iteration never pays for the call. Only the node-cap check moved. Extracting blockIsParked surfaced a latent shift hazard the old comment already knew about: `slept >> i` is undefined for i >= 32 and was safe only via "por implies shapes_.size() <= 32", an invariant not visible locally. Both shifts now share one `i < 32` guard -- behaviour-identical, since por cannot be true when i >= 32. Interleaved A/B, min-of-5: drag -4.6%, hier -3.2%, unit (untouched control leg) -3.5%, so no regression; node counts identical on all three legs, which is the correctness assertion. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e12cb14 split AStar and DragSolver into several .cpp files each and added rolling-blocks' AStarGoals.cpp, but test.yml last changed in the earlier 555b7da and still named only the pre-split files. Both em++ steps would have failed at link time with undefined symbols on any PR to main; local builds were unaffected because they go through src/util/buildWasm.ts, which has the correct lists. Mirrors buildWasm.ts exactly: 13 sources for shifting-mosaic (held in a bash array so the list appears once across the four variants) and the missing AStarGoals.cpp for rolling-blocks. Verified by parsing the YAML, replaying the step's shell logic with em++ stubbed out, and checking every listed file exists -- the only .cpp left out are the bench CLI's, which are not part of the module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
computeJamField 67, computeDisplacementFields 69 and computeMovableSet 36 against a limit of 25, plus eight nesting findings. Each is now a small driver over named phases; the two geometry structs (JamGeometry, DispGeometry) carry what the old by-reference capture lambdas closed over, which keeps the helpers inside the 7-parameter limit. Four loops also tripped S1994 by testing something other than their counter -- `d < 4 && !canMove`, `i < n && !hasBlocker`, `r < rows.size() && clear`. Those become any_of/none_of or a helper that returns early. The displacement BFS is the exception: it walks a worklist the body pushes onto, so a range-for would be wrong; it is now an explicit index walk under a while, which is what it always meant. lockedWallRows() replaces two byte-identical inline builds of the same locked-block wall mask, one here and one in initSearchScratch. Benched at 9 reps after a 5-rep run showed hier +7.2%: it reproduces at -0.3%, worst leg +0.5%, so that was noise and no bisect was needed. Added a jam leg first -- jamGuideWeight defaults to 0, so no existing leg called computeJamField at all -- and the harness now diffs the whole result JSON, not just node counts, since minJamTerm is what proves the field still computes the same values. Identical on every leg. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
computeMovableSet 38, countPathBlockers 33 and axisAwareBlockerCost 32 against a limit of 25, plus three nesting findings in the movable fixpoint. computeMovableSet gets the same decomposition its DragSolver twin got in e72ea4e, so the two now read alike. Three dedups fell out rather than being sought: - axisAwareBlockerCost and countSweepRectangleBlockers built the SAME sweep rectangle and ran the SAME bbox-then-cell overlap test. Both are now goalCorridor() + blockOverlapsRect(); the latter shrinks to a count_if. - countPathBlockers' anchorPassable lambda was the goal block's own inBounds() spelled out a second time, so it is gone rather than moved. - the occupancy build is shared, with skipGoal explicit because the two callers genuinely differ: the path walk must exclude the goal block, the movable fixpoint must include it. Two behaviour-preserving wins: `stuck` now short-circuits past the vertical probe when the horizontal one succeeds (canStepAlong is pure), and the occupancy index is built after the BFS instead of before, so the unreachable `return 4` path no longer builds one it never reads. The BFS only ever consulted bounds, never occupancy. 9 reps: unit (the affected leg) -0.2%, worst leg +0.1%, result shape identical everywhere. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Complexity 57 and 29 against a limit of 25, plus a nesting finding and another S1994 worklist loop (`head < q.size() && !found`, where the body pushes onto q) -- now an explicit index walk under a while, same as the displacement BFS. Almost all of this was already written elsewhere. relevanceMaskOf's `valid` lambda is anchorClearOfWalls() with the four captures spelled out, so it reuses DispGeometry outright; its ring dilation and updateSleepEnvelope's envelope dilation are both dilateRow(), which now serves three call sites; and its path-footprint walk is the jam sweep's, so accumulatePathFootprint() replaces both and buildJamSweepRows shrinks to a guard plus one call. The ring-0 and ring-k passes were the same absorb-blocks-touching-a-mask loop twice, now absorbBlocksOnMask(); its `mask >> b & 1u` guard subsumes ring 0's explicit goal skip because the caller pre-sets the goal bit. Added a --por --relevant leg first: sleepSets and relevantOnly both default to false, so NO existing leg called either refactored function. Verified the flags actually engage -- statesStored drops 446125 -> 233591 on the same fixture -- rather than assuming the leg covers the code. 9 reps: por +0.2%, worst leg +0.2%, result shape identical on all five legs. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Complexity 141 and 72 against a limit of 25 -- the two biggest functions in the tree -- plus seven nesting findings, two S1994 loop headers, two S135 nested-break findings and a redundant lambda return type. Both are now drivers over named phases, with BeamState/BeamCand/BeamRound and Frame/HierStep lifted into the header so the phases can name them. Two loops needed the S1994/S135 conflict resolved rather than one rule traded for the other: a for header that tests state the body writes trips S1994, and moving those tests into the body as `break`s trips S135. Both became while loops whose condition carries the tests -- the beam depth loop now has zero breaks where it had four. Frame gets an explicit noexcept move constructor. This is not cosmetic: std::unordered_set's move is not noexcept, so the implicit one was not either, and std::vector<Frame> was therefore deep-COPYING every frame (banned set included) on each reallocation instead of moving it. Moving a set whose allocator is std::allocator only steals buckets, so the promise holds. Retiring a frame now also moves just its anchors rather than the whole Frame, since the anchors are all the ban needs. Added a beam leg to the bench -- hier covered searchHierarchical but nothing covered searchBeamJam. 9 reps: beam -1.1%, worst leg +0.7%, result shape identical on all six legs. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Complexity 80 against a limit of 25, plus three nesting findings, a nested ternary, an S135 four-break loop and a C-style ROUNDS array. Same S1994/S135 resolution as the beam arm: the four-break `for (uint32_t r = 0;; r++)` becomes a while whose condition carries the stop tests, leaving zero breaks. The nested ternary picking the round's start anchors becomes jamStartAnchors(), three plain returns. ROUNDS is now a std::array behind jamRounds(). Round is private, so a file-scope constant in the .cpp could not name it; the table returns from a static member function instead of moving 70 lines into the header. The jam round's start state (elite, ridge-hopper drags, the anchors they reached) travels as one JamRound instead of four parallel locals, which also localises the elite-pointer lifetime: bankElite reads jr.from only while building the new Elite, before the push_back that would invalidate it -- the same order the inline code relied on implicitly. beamRoundBudgetSpent was byte-identical to what this arm needed, so it is now roundBudgetSpent and shared. 9 reps: jam (the affected leg) +0.2%, worst leg +1.0%, result shape identical on all six legs. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Complexity 149 against a limit of 25 -- the largest single function in the tree -- plus six nesting findings, an S135 three-break loop, a 38-line lambda and an init-statement finding. It is now a driver over named phases with AsmFrame/AsmRun/AsmOutcome in the header. The DFS loop had three breaks meaning three different things: cancelled, this packing is spent, everything is packed. They are now an AsmOutcome the driver switches on, so the loop has none -- the tri-state was already there, spelled as break plus two flags checked after the loop. Two more duplicates fell out. applyTurns() replaces the joint endgame's and the per-piece placement's identical anchor-stepping loops, and postProcess() replaces the optimizer tail that searchAssembly and finalizePlan each carried a copy of; finalizePlan already guaranteed a non-empty plan, so its missing !empty() guard was not a difference. The bench had no assembly leg at all, so this was the one arm whose refactor nothing measured. It declines without real cuts and a packing, so the leg runs test41 rather than test37 -- where it SOLVES, which makes turns/steps/dirRuns in the shape check assertions over the entire pipeline rather than just a node count. 9 reps: assembly -1.0%, worst leg +0.6%, result shape identical on all seven legs. 71/71 Release tests, clang-tidy gate green (it caught backtrackAssembly needing static), all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
floodFill walked its worklist with a for whose header tested queue_.size() while the body pushed onto it -- the S1994 + range-for pair already fixed in the two structurally identical BFS loops in DragSolverHeuristics and DragSolverExpand. Now an explicit index walk under a while. DX/DY/DIRS in AStar.h become std::array (cpp:S5945). In BitGrid.h they deliberately do NOT: that conversion cost a measured 3.4% on the jam leg and 3.3% on the beam leg, reproduced twice at 9 interleaved reps, and reverting only that declaration recovered all of it (jam +3.4 -> +0.9, beam +3.3 -> +0.1). Those two are indexed in floodFill's and runJamDijkstra's 4-neighbour scans, the hottest loops in the solver. The site now carries the measurement. NOTE ON VERIFICATION: this file cannot be checked the way the .cpp files were. SonarLint does not analyse a header standalone -- a deliberate nested ternary planted in BitGrid.h produced no finding over four minutes -- and the SonarQube server has never analysed this tree at all: of 78 open issues on the project, ZERO are under shifting-mosaic-solver/a-star. So the rules applied here are the ones the sibling rolling-blocks headers demonstrably trip (S5945 is reported on their AStar.h:54), not IDE output. 9 reps: worst leg +2.4% (assembly, 206->211ms on the shortest leg, inside its spread today), jam +1.0%, every other leg <= 0.4%. Result shape identical on all seven legs. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y one The sibling rolling-blocks NodeKey.h carries five open cpp:S5025 findings (raw new/delete) and one cpp:S6022 (byte-oriented access); ours has the same shape. Only one of the two is worth taking. S6022 is free: the FNV loop now walks std::byte and folds with std::to_integer instead of reinterpret_cast-ing to uint8_t. Benched at +0.6% worst leg, i.e. nothing. S5025 is not. std::unique_ptr<uint16_t[]> for the spill buffer is exactly the same 48 bytes -- verified by compiling the struct before and after -- but it cost 3.7% on the jam leg and 2.6% on unit. A three-point bisect separated the two changes cleanly: unique_ptr + byte jam +4.1% unit +2.2% unique_ptr only jam +3.7% unit +2.6% byte only jam +0.6% unit -0.2% so the whole cost is the smart pointer, not the hash. NodeKey is the hash key of every state map in every arm; its move and destroy paths are as hot as code gets here. The raw pointer stays, with the measurement at the declaration. Each figure is min-of-9 interleaved reps, and the unique_ptr result was reproduced across two independent runs before bisecting -- the first reading was +4.1%, under the +5% threshold that would have let it through unexamined. 71/71 Release tests, clang-tidy gate green, all four wasm variants build, result shape identical on all seven legs throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FIRST: the wasm build was broken. A CLion CTAD quick-fix had turned std::vector<uint16_t>(total, UINT16_MAX) back into std::vector(total, UINT16_MAX) which deduces vector<int>. That is the exact mistake the comment directly above the line warns about, from 9b219b3. MSVC accepts it, so the native build, the clang-tidy gate and all 71 tests stayed green while em++ failed with "no known conversion from vector<int> to vector<unsigned short>". Reverted, and the comment now records that it has been undone once by a quick-fix. Then the warnings: AStar 43, computeBlockReachability 80 and computeGoalAnchorBfs 28, plus eight nesting findings and the transparent-hasher suggestion. All now named phases. The hasher finding is gone because the string-keyed unordered_map is gone: same-shape blocks are grouped by comparing normalised cell lists directly. There are at most a few dozen blocks, so the pairwise scan is free at construction, and it drops the serialise-to-string step too. Group order changes from hash order to first-appearance order, which nothing reads -- signatureFromAnchors sorts each group independently. lockedCellMask() replaces the identical locked-cell sweep that the reachability pass and the goal-anchor BFS each carried. PERFORMANCE: this costs ~3.5% on the unit leg and nothing anywhere else. Established properly rather than guessed: a freshly-built HEAD benches within 0.7% of the stored control, reverting only these two files recovers the delta, and nothing that was split runs per node. The cost is this TU's codegen shifting around lpDisplacementCost, the one per-node function in it. Two attempts to recover it made things worse -- sharing footprintClear() with its corridor test, and moving it to AStarHeuristics.cpp (unit +3.7%, beam +4.2%). Accepted, unlike today's two style-only refusals, because this one buys the actual reductions; the file header carries the measurement. 9 reps against a freshly-built HEAD: unit +3.5%, every other leg within +/-0.5%, result shape identical on all seven. 71/71 Release tests, clang-tidy gate green, all four wasm variants build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Authored in CLion, not by the refactor pass; committed here after the same verification everything else got. All are local tidies with no behavioural content: - BenchEngines/BenchOptions: designated initialisers for the CASCADE stage table and the flag table, so each entry names its fields. - BenchCommands: redundant parentheses dropped, and shuffleBoard's ShuffleStats consumed by structured binding. - DragSolverArms: drops an include of Node.h that the searchJamRestarts split had already made unused. - DragSolverAssembly / DragSolverExpand: CTAD for a std::unordered_set<uint64_t> and a std::vector<int32_t> whose element types are unambiguous from the initialiser. - main.cpp: argc is const. The one CTAD quick-fix that was NOT safe -- AStar.cpp's std::vector(total, UINT16_MAX), which deduces vector<int> and breaks the wasm build -- was reverted separately in cf811d7. Verified as one tree: clang-tidy gate green, 71/71 Release tests, all four wasm variants build, bench shape identical on all seven legs, and a 2000-case fuzz sweep at 2000/2000 solved with 0 invalid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file was already deleted in the working tree when this session started; this only records that deletion. It is a per-machine Claude Code permissions file -- the ".local" name is the convention for exactly the kind of file a repo should not carry -- and nothing in the project reads it, so tracking it only produced churn between machines. Kept as its own commit because, unlike everything around it, this is a tooling-config change rather than solver work: `git revert` here restores the file without touching any code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 5d0c2e4, which untracked .claude/settings.local.json: without this the tool recreates the file and it comes straight back as untracked noise. Scoped to the one file rather than the .claude/ directory, so a shared .claude/settings.json (or skills, commands) can still be tracked if the project ever wants them. Verified with git check-ignore: the .local file matches, .claude/settings.json and .claude/skills/ do not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a complete shifting-mosaic solver workflow, including C++ A*/drag engines, WASM builds and runtime selection, editor configuration and solution views, native benchmarking/fuzzing tools, CI coverage, fixtures, and extensive unit/E2E tests. ChangesShifting Mosaic solver
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (35)
.github/workflows/test.yml (2)
27-38: 🧹 Nitpick | 🔵 TrivialNew shifting-mosaic C++ test step mirrors rolling-blocks pattern.
Correctly wired build+ctest steps. Note per the comment, this makes CI hard-require a 4-8 min fixture (test37) plus ~25s (test41) plus 42 others on every run — a real CI duration cost, but the author has explicitly documented and accepted this tradeoff, so no action needed unless CI times become a problem.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 27 - 38, No code changes are required; the shifting-mosaic C++ build and ctest steps are correctly configured, and the documented fixture runtime tradeoff is explicitly accepted.
128-135: 🧹 Nitpick | 🔵 TrivialThe heap-limit regression guard is opt-in and never runs in this CI workflow.
e2e/shiftingMosaicHeapLimit.slow.test.ts(the test that specifically guards against theRuntimeError: Aborted()regression on thethreads-mem64variant) is skipped unlessSM_SLOW_E2Eis set, and this workflow never sets it. The threads+mem64 build is only compile-checked here (per the comment at lines 118-120), so the actual runtime regression this test protects against currently has no automated coverage. Consider adding a scheduled/nightly job that runs withSM_SLOW_E2E=1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 128 - 135, Add scheduled/nightly CI coverage for the heap-limit regression by introducing a job or workflow step that sets SM_SLOW_E2E=1 and runs the relevant slow end-to-end test, including the threads-mem64 runtime path. Keep the existing compile and standard test steps unchanged, and ensure the scheduled job executes e2e/shiftingMosaicHeapLimit.slow.test.ts rather than leaving it opt-in only.e2e/rollingBlocksSolver.placement.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate, unused
setupSolvedPuzzlehelper.This helper is byte-identical to the one in
e2e/rollingBlocksSolver.solution.test.tsbut is never called by the single test in this file. Extract to a shared test-helper module, or drop it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.placement.test.ts` around lines 3 - 31, Remove the unused setupSolvedPuzzle helper from this placement test, since it duplicates the helper in rollingBlocksSolver.solution.test.ts and is not called here.e2e/rollingBlocksSolver.solution.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate helper — candidate for extraction.
Same
setupSolvedPuzzleimplementation is duplicated verbatim ine2e/rollingBlocksSolver.placement.test.ts(where it's unused). Move to a shared helper module imported by both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.solution.test.ts` around lines 3 - 31, Extract the duplicated setupSolvedPuzzle helper into a shared e2e helper module, then import and use it from both rollingBlocksSolver.solution.test.ts and rollingBlocksSolver.placement.test.ts. Preserve the existing setup behavior and remove the unused local duplicate from the placement test.e2e/shiftingMosaicSolver.config.test.ts (1)
1-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test fixtures/helpers, one unused.
Same block as
e2e/shiftingMosaicSolver.blocks.test.tslines 1-59 — see that comment.paintWithGapis unused here too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicSolver.config.test.ts` around lines 1 - 59, Remove the duplicated SAMPLE_CONFIG, Cell type, and cellAt, cellCenter, paintShape, and unused paintWithGap helpers from this test file; reuse the corresponding shared fixtures and helpers from shiftingMosaicSolver.blocks.test.ts instead, updating imports or references as needed.e2e/shiftingMosaicSolver.blocks.test.ts (1)
1-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test fixtures/helpers, one unused.
SAMPLE_CONFIG,Cell,cellAt,cellCenter,paintShape, andpaintWithGapare byte-identical to the same block ine2e/shiftingMosaicSolver.config.test.ts. Additionally,paintWithGapis not called anywhere in either file. Recommend extracting the shared pieces to a common e2e helper module and droppingpaintWithGapif it's genuinely unused, or adding a test that exercises it (e.g., verifying gap-drag placement behavior) if it was meant to be used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicSolver.blocks.test.ts` around lines 1 - 59, Extract the duplicated SAMPLE_CONFIG, Cell type, cellAt, cellCenter, and paintShape fixtures/helpers from the shifting mosaic tests into a shared e2e helper module, then import and reuse them in both shiftingMosaicSolver.blocks.test.ts and shiftingMosaicSolver.config.test.ts. Remove paintWithGap if it is not needed; only retain it by adding a test that exercises its gap-drag behavior.src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css (1)
591-649: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolution-view colors are hardcoded literals rather than theme tokens.
#c84ad8,#1f9d57,#b88600and#ffffffbypass the--md-sys-color-*system used elsewhere in this file, so these markers won't adapt to a dark/high-contrast theme and their contrast against--md-sys-color-surface-*is unverified. Consider mapping them to theme tokens (or defining them once as custom properties with per-scheme overrides).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css` around lines 591 - 649, Replace the hardcoded solution-view marker colors in .sm-moving-label, .sm-dest-label, .sm-moving, .sm-dest::after, and .sm-goal-edge-* with existing --md-sys-color-* theme tokens, or define shared custom properties with light/dark/high-contrast overrides. Preserve the current visual roles while ensuring marker contrast adapts to the surrounding surface colors.e2e/shiftingMosaicWasmVariants.test.ts (1)
209-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSubstring assertions on emscripten glue are brittle.
maximum:131072n,address:"i64",shared:trueassume the exact minified spacing/quoting emscripten currently emits. A toolchain bump that emitsmaximum: 131072nor single quotes breaks this test without any real regression. Tolerant regexes keep the same guarantee.♻️ Suggested loosening
- expect(glue).toContain("maximum:131072n"); - expect(glue).toContain('address:"i64"'); - expect(glue).toContain("shared:true"); + expect(glue).toMatch(/maximum:\s*131072n/); + expect(glue).toMatch(/address:\s*['"]i64['"]/); + expect(glue).toMatch(/shared:\s*true/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicWasmVariants.test.ts` around lines 209 - 216, Update the glue assertions in the test around the fetched astar.threads.mem64.mjs content to use whitespace- and quote-tolerant regular expressions instead of exact substring matches for maximum, address, and shared. Preserve validation of the same values and flags while allowing equivalent Emscripten formatting changes.test/shiftingMosaicSolver/wasm.test.ts (1)
183-224: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo internal deadline: a test-level timeout leaves every arm worker alive.
racePortfolioonly settles ondone/error/exit. If an arm neither finishes nor exits, the Bun per-test timeout fires while the promise is still pending, sofinish()never runs and the nine workers (each holding a wasm heap) survive into the next fixture — the exact leak theexithandler at Line 220 was added to avoid. A watchdog slightly under the test timeout closes the remaining path.🛡️ Suggested guard
return new Promise(resolve => { const workers = arms.map(() => new ArmWorker(workerPath)); let pending = workers.length; let settled = false; + let watchdog: ReturnType<typeof setTimeout>; const finish = (turns: WasmTurn[]) => { if (settled) return; settled = true; + clearTimeout(watchdog); for (const w of workers) void w.terminate(); resolve(turns); }; + // Slightly under the enclosing test timeout so the arms are always + // torn down by us rather than abandoned by the runner. + watchdog = setTimeout(() => finish([]), budgetMs * 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/wasm.test.ts` around lines 183 - 224, Add an internal watchdog within racePortfolio that fires slightly before the test-level timeout, calls finish([]) to terminate all still-running arm workers, and settles the promise. Ensure the watchdog is cleared when finish completes, while preserving the existing done/error/exit retirement behavior.src/pages/shifting-mosaic-solver/a-star/CMakeLists.txt (1)
54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded CLion paths and pinned LLVM 23 make linting effectively local-only.
The hint list pins one machine's CLion install, and
SM_CLANG_TIDY_PROFILE_LLVM 23(a dev snapshot no distro ships) means every CI host takes the skip branch — the "tree cannot drift out of compliance" goal isn't enforced anywhere automated. Consider letting users point at their own binary, e.g. honoring a cache/env override before the hints.♻️ Optional override hook
find_program(SM_CLANG_TIDY_EXE NAMES clang-tidy - HINTS "$ENV{ProgramFiles}/JetBrains/CLion 2024.1.4/bin/clang/win/x64/bin" + HINTS "$ENV{SM_CLANG_TIDY_DIR}" + "$ENV{ProgramFiles}/JetBrains/CLion 2024.1.4/bin/clang/win/x64/bin" "$ENV{ProgramFiles}/JetBrains/CLion/bin/clang/win/x64/bin" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/CMakeLists.txt` around lines 54 - 62, Update the SM_CLANG_TIDY_EXE discovery in CMakeLists.txt to honor a user-provided cache or environment override for the clang-tidy executable before applying CLion-specific HINTS, and avoid making lint enforcement depend solely on the pinned LLVM 23 profile. Preserve the existing fallback discovery while allowing CI and other hosts to supply their own compatible clang-tidy binary.e2e/rollingBlocksSolver.tools.test.ts (1)
922-922: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
setupSolvedPuzzlehelper
e2e/rollingBlocksSolver.tools.test.ts:3no longer references it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.tools.test.ts` at line 922, Remove the unused setupSolvedPuzzle helper from rollingBlocksSolver.tools.test.ts, including its definition and any associated import or setup code, since no references remain.test/shiftingMosaicSolver/board.pointer.test.ts (1)
1-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
Boardtest harness instead of triplicating it.The
editorMockshape, DOM stub, andBoardconstruction are copy-pasted identically across three files. A shared helper would reduce upkeep when theBoard/editor interface changes.
test/shiftingMosaicSolver/board.pointer.test.ts#L1-L31: import a sharedcreateBoardTestContext()helper instead of inlining the mock/Board setup.test/shiftingMosaicSolver/board.render.test.ts#L1-L31: same — replace the duplicatedbeforeEachblock with the shared helper.test/shiftingMosaicSolver/board.state.test.ts#L1-L31: same — replace the duplicatedbeforeEachblock with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/board.pointer.test.ts` around lines 1 - 31, Extract the duplicated editor mock, DOM setup, and Board initialization into a shared createBoardTestContext() helper, then update the beforeEach setup in test/shiftingMosaicSolver/board.pointer.test.ts:1-31, test/shiftingMosaicSolver/board.render.test.ts:1-31, and test/shiftingMosaicSolver/board.state.test.ts:1-31 to use it while preserving each test’s board and internal context access.src/pages/shifting-mosaic-solver/a-star/AStar.h (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffClass AStar exceeds the 20-field cohesion threshold (22 fields, per SonarCloud).
Consider grouping the board-analysis caches (
blockValidAnchorMask_,initialGoalPathCells_,blockSafeAnchorDist_,goalAnchorBfsDist_) into a small nested struct to reduce the member count and improve cohesion. Given AStar.cpp's own notes that restructuring this file has measurably regressed performance before, treat this as low priority and re-measure if attempted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStar.h` at line 16, Optionally reduce AStar’s 22-member count by grouping blockValidAnchorMask_, initialGoalPathCells_, blockSafeAnchorDist_, and goalAnchorBfsDist_ into a small nested cache struct within AStar, updating all AStar.cpp accesses accordingly. Treat this as low priority, preserve behavior and performance, and re-measure before keeping the change.Source: Linters/SAST tools
src/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cpp (1)
239-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
tryReorderMerge's improvement check compares against the wrong step metric (currently safe, but fragile).
baseStepsis computed viacomputeRuns(turns).size()— which splits on direction changes within a block — while the accept condition compares it againstcountSteps(cand), which only splits on block-ID changes (the coarser, actual player-step metric per this function's own doc comment: "the player-step count drops by one").runs.size() >= countSteps(turns)always, so this check is a looser bound than intended.Right now this can't produce a wrong accept: reaching this branch requires an intervening different-block run between
ri/rj(proven by theprevRunOfBlocktracking), which guarantees the merge always yields exactlycountSteps(original) - 1. But the check doesn't actually verify that invariant — comparing againstcountSteps(turns)directly would make the code self-validating against future changes to the run-tracking logic instead of relying on an implicit proof.♻️ Proposed fix to compare against the actual pre-merge step count
bool AStar::tryReorderMerge(std::vector<Turn> &turns) const { const auto runs = computeRuns(turns); - const size_t baseSteps = runs.size(); + const size_t baseSteps = countSteps(turns); std::vector prevRunOfBlock(shapes_.size(), -1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cpp` around lines 239 - 266, Update tryReorderMerge to compute the baseline using countSteps(turns) rather than runs.size(), then compare both runPulledEarlier and runPushedLater candidates against that pre-merge player-step count while retaining replaySolves validation and the existing successful replacement behavior.src/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.h (1)
205-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
aloneis dead — confirm the goal-target check is intentionally bbox-only.
aloneis built and assigned but never read; only the bbox test below decides the target. If the leftover was meant to feed anoracleCanPlace(p, 0, t, alone)check, that check is missing; otherwise drop the two lines.♻️ Remove the unused vector
const Position t = {.x = static_cast<int8_t>(rng.draw(xDist)), .y = static_cast<int8_t>(rng.draw(yDist))}; - std::vector<Position> alone(p.anchors.size(), {.x = 127, .y = 127}); - alone[0] = t; // Target just needs the goal's bbox in-grid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.h` around lines 205 - 220, Remove the unused alone vector and its assignment in the goal-target generation loop around p.goalAnchor. Keep the existing bounding-box-only validity check and target selection behavior unchanged.src/pages/shifting-mosaic-solver/a-star/test/dragsolver_search_test.cpp (1)
159-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWall-clock budget with a hard success assertion can flake on loaded CI.
ASSERT_FALSE(turns.empty())for every oracle-solvable board is bounded by a 10s wall-clock budget, exactly the machine-speed dependence you avoided inJamRoundShapingStaysValidAndDefaultsUnchangedby switching to a node cap (Lines 116-120). Bounding these by nodes instead (searchHierarchical(0, N)) would make the pass/fail deterministic. Same applies toJamGuideAndJitterStayValidAndComplete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/test/dragsolver_search_test.cpp` around lines 159 - 173, The solvable-board tests use a wall-clock search budget, making success assertions machine-speed dependent. In HierarchicalSolvesSolvableBoards and JamGuideAndJitterStayValidAndComplete, replace the time-based searchHierarchical limit with a sufficiently large node cap via the node-budget argument (for example, searchHierarchical(0, N)), preserving the existing solvability and replay-validity assertions.src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp (1)
138-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
writeTurnsFilesilently swallows write failures.Unlike
writeFixtureAt, this never checksis_open()/stream state, so an unwritable path loses the plan with no signal — awkward when the turns file is the artifact a fuzz repro depends on.♻️ Return success like `writeFixtureAt`
-void writeTurnsFile(const std::vector<Turn> &turns, const std::string &path) { +bool writeTurnsFile(const std::vector<Turn> &turns, const std::string &path) { json tj = json::array(); for (const auto &[blockId, direction] : turns) tj.push_back( {{"blockId", blockId}, {"direction", static_cast<int>(std::to_underlying(direction))}}); std::ofstream f(path); + if (!f.is_open()) + return false; f << tj.dump(); + return f.good(); }Header signature in
BenchFixture.hneeds the matching update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp` around lines 138 - 146, Update writeTurnsFile to return a success status like writeFixtureAt: verify the output stream opened successfully and remains valid after writing, returning false on failure and true only when the turns JSON is written successfully. Update the matching declaration in BenchFixture.h and adjust all call sites to handle the returned status.src/pages/shifting-mosaic-solver/a-star/BenchFixture.h (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
BenchFixture.hself-contained. Add<cstddef>and change thesize_treturn types tostd::size_tinstead of relying on transitive/global declarations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchFixture.h` around lines 6 - 8, Add <cstddef> to BenchFixture.h and update all size_t return types in the BenchFixture declarations to std::size_t, ensuring the header is self-contained and no longer depends on transitive or global declarations.src/pages/shifting-mosaic-solver/a-star/BenchOptions.cpp (2)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the hard-coded array size.
std::array<FlagSpec, 41>must be updated by hand whenever a flag is added/removed; a mismatch is a compile error at best, and it is pure bookkeeping.constexpr FlagSpec FLAGS[]{...}(or CTAD viastd::to_array) deduces it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchOptions.cpp` at line 57, Update the FLAGS declaration in BenchOptions to use a size-deduced array instead of the hard-coded std::array<FlagSpec, 41>, such as a deduced constexpr array or std::to_array, while preserving the existing flag entries and constexpr behavior.
209-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider bailing out as soon as the cursor records an error.
cur.error()is only inspected after the whole argv is consumed, so a malformed value keeps applying later flags (andvalue()returning""writes empty paths intoopt). Behaviour is still correct becauseparseBenchOptionsreturns false andmain.cppaborts before use, but an earlyif (!cur.error().empty()) break;inside the loop makes the contract obvious.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchOptions.cpp` around lines 209 - 226, Update parseBenchOptions to check cur.error() immediately after each spec->apply(opt, cur) call and stop processing further arguments when an error is recorded. Keep the existing final error reporting and false return unchanged, while preventing later flags from modifying opt after a malformed value.src/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cpp (1)
130-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify the score bit-packing has room for the jam term.
jamChildis capped at1u << 19and shifted left 14, so the top field occupies bits 14–33 of auint64_t— fine — buth << 10only leaves bits 10–13 for a value that is documented as 0/1, and the jitter is masked to 10 bits. Worth a brief comment recording the field widths so a future change toh(or a wider jitter) doesn't silently collide.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cpp` around lines 130 - 137, Add a concise comment immediately above the score construction documenting the bit layout: jamChild uses bits 14–33, h uses bits 10–13, and beamJitter uses the low 10 bits. Keep the existing packing unchanged and note the expected h range so future width changes are checked for overlap.src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp (2)
53-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the
40reachability threshold.Every other tuned constant in this file is a named
constexprwith a rationale comment (UNFREEZE_K,ENDGAME_K,MAX_JOINT,MAX_BACKTRACKS); this one is a bare literal that silently decides "big enough free region ⇒ approachable".♻️ Suggested change
+ // A flood region this large is effectively open space; treat the piece as + // approachable without checking its current anchor. + constexpr size_t OPEN_REGION_CELLS = 40; if (const auto &reached = grid_.floodFill(piece, slotPos(piece)); - reached.size() >= 40) + reached.size() >= OPEN_REGION_CELLS) return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp` around lines 53 - 66, Name the flood-fill reachability threshold used by DragSolver::approachable as a file-level constexpr, replacing the bare 40 literal. Add a concise rationale comment documenting that it represents the minimum free-region size considered large enough for approachability, and use the named constant in the reached.size() comparison.
206-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSolver progress goes to raw
std::cout.This code also runs inside the wasm build where the arms race concurrently; unconditional
std::coutfrom every arm interleaves and cannot be silenced by callers. A verbosity flag onConfig(or a small logging hook alongsideonProgress) would keep the diagnostics without forcing them on embedders.Also applies to: 274-279, 440-466
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp` around lines 206 - 218, The solver’s progress messages in the joint-endgame and other cited paths write unconditionally to std::cout, preventing callers from suppressing or safely collecting output. Route these diagnostics through a Config verbosity flag or logging/progress hook alongside onProgress, and guard or dispatch all affected messages—including the locations around jointEndgame and lines 440-466—through that mechanism instead of direct std::cout.src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp (1)
211-252: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider avoiding
std::functionfor the hot packing DFS.
dfsis called up toPACK_NODE_CAP(5M) times and every recursive step pays an indirect call throughstd::functionplus the type-erasure barrier that blocks inlining. A small local recursive struct (or atemplate <typename Self>lambda / explicit worklist) keeps the same shape without the indirection.♻️ Sketch
- std::function<bool(size_t)> dfs = [&](const size_t fromRank) -> bool { + const auto dfs = [&](this const auto &self, const size_t fromRank) -> bool {(deducing-
thisrecursive lambda if the toolchain is C++23; otherwise a localstruct Dfs { ... bool run(size_t); }.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp` around lines 211 - 252, Replace the std::function-based recursive dfs in the packing search with a directly callable recursive implementation, such as a self-passing templated lambda or local recursive struct, while preserving the existing DFS behavior, state updates, and PACK_NODE_CAP handling. Ensure recursive calls avoid std::function type erasure and indirect dispatch.Source: Linters/SAST tools
src/pages/shifting-mosaic-solver/a-star/DragSolverArms.cpp (1)
176-178: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the unsigned subtraction in the remaining-node computation.
maxNodes - stats_.nodesExpandedwraps if expansions ever overshootmaxNodes(rounds are capped, not clamped), and the wrapped value makesstd::mina no-op — the opposite of the intent. The loop guard currently makes this unreachable, but a saturating form removes the dependency.♻️ Proposed change
- if (maxNodes != 0) - cap = std::min<uint32_t>(cap, maxNodes - stats_.nodesExpanded); + if (maxNodes != 0) + cap = std::min<uint32_t>(cap, maxNodes > stats_.nodesExpanded + ? maxNodes - stats_.nodesExpanded + : 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverArms.cpp` around lines 176 - 178, Update the remaining-node calculation near the return cap logic to saturate maxNodes - stats_.nodesExpanded at zero when expansions meet or exceed maxNodes, then apply std::min with cap only to the non-underflowing remainder. Preserve the existing unlimited behavior when maxNodes is zero.src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp (3)
40-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
operator==onDragHeapEntryappears dead code.The heap only needs ordering (via
std::greater<>, which is satisfiable fromoperator<=>);operator==isn't otherwise invoked in this file. SonarCloud's "unused member function" flag looks correct here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp` around lines 40 - 49, Remove the unused operator== overload from DragHeapEntry, leaving operator<=> unchanged so std::greater<> continues to provide the heap ordering.Source: Linters/SAST tools
300-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReference-identity shortcut for mask comparison — fragile if
cutSuffixRows_is ever resized mid-search.if (&maskN == &exp.maskP) return 1 + exp.baseCount;This relies on
cutSuffixRows_elements never moving between the timeexp.maskPwas bound and this call. Given it's a member fixed up-front percomputeDisplacementFields, this is presumably safe today, but it's a non-obvious invariant for future maintainers touchingcutSuffixRows_'s lifetime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp` around lines 300 - 318, Replace the address-based mask comparison in the displacement-count logic with a value-based comparison between maskN and exp.maskP, preserving the existing 1 + exp.baseCount result when the masks are equivalent. Update the code in the method containing the DragExpansion parameter without changing the surrounding counting behavior.
20-51: 📐 Maintainability & Code Quality | 🔵 TrivialAlign the analyzer with C++23
src/pages/shifting-mosaic-solver/a-star/CMakeLists.txtalready setsCMAKE_CXX_STANDARD 23, and this codebase deliberately uses<=>,std::ranges, and.contains(). The static-analysis job needs the same standard flag instead of reporting these as source defects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp` around lines 20 - 51, Update the analyzer configuration in the a-star CMake setup to use C++23, matching the existing CMAKE_CXX_STANDARD 23 setting. Ensure the static-analysis invocation receives the equivalent C++23 standard flag so valid usage of DragHeapEntry’s operator<=> and other C++23 APIs is analyzed correctly.test/shiftingMosaicSolver/aStar.search.test.ts (1)
216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
toHaveLengthfor length assertions.SonarCloud suggests
expect(turns).toHaveLength(1)overexpect(turns.length).toBe(1)for clearer failure reporting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.search.test.ts` at line 216, Update the assertion in the relevant A* search test to use toHaveLength directly on turns, preserving the expected length of 1.Source: Linters/SAST tools
src/pages/shifting-mosaic-solver/aStar.ts (4)
106-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove field declarations above the constructor.
movableBlockIndices,unsolvableAtStart, andgoalAnchorBfsDistare declared with initializers after the constructor that populates them (lines 75-92). This is safe (field initializers run before the constructor body), but it reads as if the constructor's work gets reset — worth moving these declarations above the constructor for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 106 - 110, Move the field declarations movableBlockIndices, unsolvableAtStart, and goalAnchorBfsDist above the constructor in the solver class, preserving their existing initializers and types. Do not change the constructor’s initialization logic or field behavior.
152-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReduce cognitive complexity of
search.SonarCloud flags this function at cognitive complexity 39 (limit 15). Logic itself checks out (dedup via closedSet/gScore, timeout budget, successor generation), but consider extracting the successor-expansion inner loop (Lines 220-254) and/or the timeout/goal checks into named helpers to aid readability and future maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 152 - 259, Reduce cognitive complexity in the AStar search method by extracting the successor-generation logic from the movableBlockIndices/direction loops into a named helper, and extract additional timeout or goal-check handling if needed to meet the configured threshold. Preserve the existing closedSet/gScore deduplication, cameFrom updates, heap insertion, timeout behavior, deadlock pruning, and goal-path reconstruction exactly.Source: Linters/SAST tools
377-429: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReduce cognitive complexity of
computeMovableSet.SonarCloud flags this function at cognitive complexity 37 (limit 15). The fixpoint logic is correct, but consider extracting the per-block "can this direction be freed" check (Lines 405-414) into a helper method to shrink nesting depth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 377 - 429, Reduce cognitive complexity in computeMovableSet by extracting the per-direction occupancy check into a focused helper method that determines whether a block can move in a given direction, using the existing anchors, shape, occupancy, and movable set. Keep the fixpoint iteration and its current behavior unchanged, and have computeMovableSet delegate the directional checks to the helper.Source: Linters/SAST tools
436-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce cognitive complexity of
isDeadlocked.SonarCloud flags cognitive complexity 21 (limit 15). The "does block i occupy a goal-final cell" check is duplicated between the
hasBlockerscan and the final loop — extracting it into a helper both fixes the duplication and reduces nesting.♻️ Proposed refactor
+ private blockOccupiesGoalFinalCells(shapeIndex: number, anchor: Position): boolean { + for (const cell of this.shapes[shapeIndex]!) { + const enc = (anchor.x + cell.x) * this.gridHeight + (anchor.y + cell.y); + if (this.goalBlockFinalCells.has(enc)) return true; + } + return false; + } + private isDeadlocked(anchors: Position[]): boolean { - const stride = this.gridHeight; - let hasBlocker = false; - for (let i = 0; i < anchors.length; i++) { - if (i === this.goalIndex) continue; - const a = anchors[i]!; - for (const cell of this.shapes[i]!) { - const enc = (a.x + cell.x) * stride + (a.y + cell.y); - if (this.goalBlockFinalCells.has(enc)) { - hasBlocker = true; - break; - } - } - if (hasBlocker) break; - } - if (!hasBlocker) return false; + const hasBlocker = anchors.some( + (a, i) => i !== this.goalIndex && this.blockOccupiesGoalFinalCells(i, a), + ); + if (!hasBlocker) return false; const movable = this.computeMovableSet(anchors); - for (let i = 0; i < anchors.length; i++) { - if (i === this.goalIndex) continue; - if (movable.has(i)) continue; - const a = anchors[i]!; - for (const cell of this.shapes[i]!) { - const enc = (a.x + cell.x) * stride + (a.y + cell.y); - if (this.goalBlockFinalCells.has(enc)) return true; - } - } - return false; + return anchors.some( + (a, i) => + i !== this.goalIndex && + !movable.has(i) && + this.blockOccupiesGoalFinalCells(i, a), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 436 - 465, Reduce cognitive complexity in isDeadlocked by extracting the duplicated check for whether a block occupies any cell in goalBlockFinalCells into a private helper. Reuse that helper in both the initial hasBlocker scan and the final immovable-block scan, preserving the existing goalIndex exclusions and deadlock behavior.Source: Linters/SAST tools
test/shiftingMosaicSolver/aStar.collision.test.ts (2)
55-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmpty test asserts nothing.
This test builds shapes, comments through several false starts, then only does
void shapeA; void shapeB; void shapeBReal;— noblocksCollidecall, noexpect(...). It always passes without testing anything, which is misleading since the next test (Lines 83-112) already covers the intended bbox-overlap-but-no-cell-collision scenario. Recommend removing this test.♻️ Proposed fix
- test("two L-shapes whose bboxes overlap but whose cells do not are not colliding", () => { - // Shape A: L pointing top-right - // X X - // X . - const shapeA: Position[] = [ - { x: 0, y: 0 }, - { x: 1, y: 0 }, - { x: 0, y: 1 }, - ]; - // Shape B: just bottom-right cell - // . X - const shapeB: Position[] = [{ x: 1, y: 1 }]; - // We use a 2x2 bbox shape B by also including {x:0,y:0} for bbox - // ... but we want a shape that fits the (1,1) corner exactly. - // Easier: use a real bbox-overlapping case where the cell at (1,1) is empty. - const shapeBReal: Position[] = [ - { x: 1, y: 1 }, - { x: 0, y: 1 }, - ]; - // shapeBReal's bbox is 2x2 (because we have x∈{0,1}, y∈{0,1}... wait y=1 only) - // Actually bbox is 2 wide, 1 tall (since min(y)=1, but bbox calc uses max(y)+1 = 2) - // Hmm bbox calc is just max+1 — let me redo with normalized shape. - // Skip this — use a more direct test below instead. - void shapeA; - void shapeB; - void shapeBReal; - }); - test("L-shape and single cell — bbox overlaps, cells overlap", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.collision.test.ts` around lines 55 - 81, Remove the empty test case that defines shapeA, shapeB, and shapeBReal but performs no collision check or assertion. Keep the subsequent test covering bbox overlap without cell collision unchanged.
12-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated test helpers across two files.
makeAStarandapplyTurnsare copy-pasted verbatim between the two test files; extracting them into a shared test-utils module (e.g.test/shiftingMosaicSolver/testHelpers.ts) avoids drift as more AStar unit-test files are added to this suite.
test/shiftingMosaicSolver/aStar.collision.test.ts#L12-L51: importmakeAStar/applyTurnsfrom a new shared helper module instead of defining them locally.test/shiftingMosaicSolver/aStar.search.test.ts#L12-L51: same — import from the shared helper module instead of the duplicate local definitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.collision.test.ts` around lines 12 - 51, Extract the duplicated makeAStar and applyTurns helpers into a shared test utility module. In test/shiftingMosaicSolver/aStar.collision.test.ts lines 12-51 and test/shiftingMosaicSolver/aStar.search.test.ts lines 12-51, remove the local definitions and import both helpers from the shared module, preserving their existing behavior and required dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 302fa3b5-73de-4b7d-b3b1-287957c48865
📒 Files selected for processing (182)
.claude/settings.local.json.github/workflows/test.yml.gitignoree2e/phasicDialSolver.solvable.test.tse2e/phasicDialSolver.states.test.tse2e/rollingBlocksSolver.placement.test.tse2e/rollingBlocksSolver.solution.test.tse2e/rollingBlocksSolver.tools.test.tse2e/shiftingMosaicHeapLimit.slow.test.tse2e/shiftingMosaicSolver.blocks.test.tse2e/shiftingMosaicSolver.config.test.tse2e/shiftingMosaicSolver.editing.test.tse2e/shiftingMosaicSolver.solving.test.tse2e/shiftingMosaicSolver.test.tse2e/shiftingMosaicWasmVariants.test.tspackage.jsonplaywright.config.tssrc/common/coi-serviceworker.jssrc/common/coiRegister.tssrc/common/common.csssrc/pages/phasic-dial-solver/phasicDialSolver.tssrc/pages/rolling-blocks-solver/a-star/AStar.cppsrc/pages/rolling-blocks-solver/a-star/AStar.hsrc/pages/rolling-blocks-solver/a-star/AStarGoals.cppsrc/pages/rolling-blocks-solver/a-star/CMakeLists.txtsrc/pages/rolling-blocks-solver/a-star/GoalCluster.hsrc/pages/rolling-blocks-solver/a-star/Node.hsrc/pages/rolling-blocks-solver/a-star/NodeKey.hsrc/pages/rolling-blocks-solver/a-star/test/CMakeLists.txtsrc/pages/rolling-blocks-solver/a-star/wasm_bindings.cppsrc/pages/rolling-blocks-solver/aStar.tssrc/pages/rolling-blocks-solver/bfs.tssrc/pages/rolling-blocks-solver/board.tssrc/pages/rolling-blocks-solver/goalClusters.tssrc/pages/rolling-blocks-solver/rollingBlocksSolver.tssrc/pages/shifting-mosaic-solver/a-star/.clang-tidysrc/pages/shifting-mosaic-solver/a-star/AStar.cppsrc/pages/shifting-mosaic-solver/a-star/AStar.hsrc/pages/shifting-mosaic-solver/a-star/AStarHeuristics.cppsrc/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cppsrc/pages/shifting-mosaic-solver/a-star/AStarSearch.cppsrc/pages/shifting-mosaic-solver/a-star/BenchCommands.cppsrc/pages/shifting-mosaic-solver/a-star/BenchCommands.hsrc/pages/shifting-mosaic-solver/a-star/BenchEngines.cppsrc/pages/shifting-mosaic-solver/a-star/BenchEngines.hsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.cppsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.hsrc/pages/shifting-mosaic-solver/a-star/BenchOptions.cppsrc/pages/shifting-mosaic-solver/a-star/BenchOptions.hsrc/pages/shifting-mosaic-solver/a-star/BitGrid.hsrc/pages/shifting-mosaic-solver/a-star/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/ClangdCompat.hsrc/pages/shifting-mosaic-solver/a-star/DragSolver.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolver.hsrc/pages/shifting-mosaic-solver/a-star/DragSolverArms.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverConfig.hsrc/pages/shifting-mosaic-solver/a-star/DragSolverExpand.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverHeuristics.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cppsrc/pages/shifting-mosaic-solver/a-star/MemoryProbe.hsrc/pages/shifting-mosaic-solver/a-star/Node.hsrc/pages/shifting-mosaic-solver/a-star/NodeKey.hsrc/pages/shifting-mosaic-solver/a-star/PARALLEL.mdsrc/pages/shifting-mosaic-solver/a-star/ParallelCascade.hsrc/pages/shifting-mosaic-solver/a-star/SeededRng.hsrc/pages/shifting-mosaic-solver/a-star/SolverClock.cppsrc/pages/shifting-mosaic-solver/a-star/SolverClock.hsrc/pages/shifting-mosaic-solver/a-star/StateTable.hsrc/pages/shifting-mosaic-solver/a-star/Types.hsrc/pages/shifting-mosaic-solver/a-star/bench/HARD-BOARDS.mdsrc/pages/shifting-mosaic-solver/a-star/bench/after-cascade.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/after-luby.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/baseline-unit.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/before-cascade.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/drag-settled-w2.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-1022-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-10276-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-11386-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-12366-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-23444-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-29534-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-3058-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-3086-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-3870-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-4722-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-8697-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-9164-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/main.cppsrc/pages/shifting-mosaic-solver/a-star/test/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.hsrc/pages/shifting-mosaic-solver/a-star/test/astar_unit_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/dragsolver_search_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/dragsolver_unit_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/json_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/optimizer_test.cppsrc/pages/shifting-mosaic-solver/a-star/wasm_bindings.cppsrc/pages/shifting-mosaic-solver/aStar.tssrc/pages/shifting-mosaic-solver/board.tssrc/pages/shifting-mosaic-solver/boardGeometry.tssrc/pages/shifting-mosaic-solver/config.tssrc/pages/shifting-mosaic-solver/directions.tssrc/pages/shifting-mosaic-solver/editorHelpers.tssrc/pages/shifting-mosaic-solver/index.htmlsrc/pages/shifting-mosaic-solver/layout.tssrc/pages/shifting-mosaic-solver/node.tssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.csssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.tssrc/pages/shifting-mosaic-solver/solutionView.tssrc/pages/shifting-mosaic-solver/turn.tssrc/pages/shifting-mosaic-solver/wasm/astar.worker.jssrc/pages/shifting-mosaic-solver/wasmBridge.tssrc/util/benchShiftingMosaic.tssrc/util/benchWasmSimd.tssrc/util/build.tssrc/util/buildWasm.tssrc/util/fuzzShiftingMosaic.tssrc/util/minHeap.tssrc/util/plugins.tssrc/util/serve.tssrc/util/types.tstest/phasic-dial-solver/phasicDialSolver.test.tstest/resources/shiftingMosaicTest.jsontest/resources/shiftingMosaicTest1.jsontest/resources/shiftingMosaicTest10.jsontest/resources/shiftingMosaicTest11.jsontest/resources/shiftingMosaicTest12.jsontest/resources/shiftingMosaicTest13.jsontest/resources/shiftingMosaicTest14.jsontest/resources/shiftingMosaicTest15.jsontest/resources/shiftingMosaicTest16.jsontest/resources/shiftingMosaicTest17.jsontest/resources/shiftingMosaicTest18.jsontest/resources/shiftingMosaicTest19.jsontest/resources/shiftingMosaicTest2.jsontest/resources/shiftingMosaicTest20.jsontest/resources/shiftingMosaicTest21.jsontest/resources/shiftingMosaicTest22.jsontest/resources/shiftingMosaicTest23.jsontest/resources/shiftingMosaicTest24.jsontest/resources/shiftingMosaicTest25.jsontest/resources/shiftingMosaicTest26.jsontest/resources/shiftingMosaicTest27.jsontest/resources/shiftingMosaicTest28.jsontest/resources/shiftingMosaicTest29.jsontest/resources/shiftingMosaicTest3.jsontest/resources/shiftingMosaicTest30.jsontest/resources/shiftingMosaicTest31.jsontest/resources/shiftingMosaicTest32.jsontest/resources/shiftingMosaicTest33.jsontest/resources/shiftingMosaicTest34.jsontest/resources/shiftingMosaicTest35.jsontest/resources/shiftingMosaicTest36.jsontest/resources/shiftingMosaicTest37.jsontest/resources/shiftingMosaicTest38.jsontest/resources/shiftingMosaicTest39.jsontest/resources/shiftingMosaicTest4.jsontest/resources/shiftingMosaicTest40.jsontest/resources/shiftingMosaicTest41.jsontest/resources/shiftingMosaicTest42.jsontest/resources/shiftingMosaicTest43.jsontest/resources/shiftingMosaicTest5.jsontest/resources/shiftingMosaicTest6.jsontest/resources/shiftingMosaicTest7.jsontest/resources/shiftingMosaicTest8.jsontest/resources/shiftingMosaicTest9.jsontest/rollingBlocksSolver/block.musttouch.test.tstest/rollingBlocksSolver/block.roll.test.tstest/rollingBlocksSolver/block.validity.test.tstest/shiftingMosaicSolver/aStar.collision.test.tstest/shiftingMosaicSolver/aStar.primitives.test.tstest/shiftingMosaicSolver/aStar.search.test.tstest/shiftingMosaicSolver/board.geometry.test.tstest/shiftingMosaicSolver/board.pointer.test.tstest/shiftingMosaicSolver/board.render.test.tstest/shiftingMosaicSolver/board.state.test.tstest/shiftingMosaicSolver/board.test.tstest/shiftingMosaicSolver/config.test.tstest/shiftingMosaicSolver/mem64.node.test.mjstest/shiftingMosaicSolver/portfolio.worker.tstest/shiftingMosaicSolver/wasm.test.ts
💤 Files with no reviewable changes (2)
- e2e/shiftingMosaicSolver.test.ts
- .claude/settings.local.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 17
🧹 Nitpick comments (35)
.github/workflows/test.yml (2)
27-38: 🧹 Nitpick | 🔵 TrivialNew shifting-mosaic C++ test step mirrors rolling-blocks pattern.
Correctly wired build+ctest steps. Note per the comment, this makes CI hard-require a 4-8 min fixture (test37) plus ~25s (test41) plus 42 others on every run — a real CI duration cost, but the author has explicitly documented and accepted this tradeoff, so no action needed unless CI times become a problem.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 27 - 38, No code changes are required; the shifting-mosaic C++ build and ctest steps are correctly configured, and the documented fixture runtime tradeoff is explicitly accepted.
128-135: 🧹 Nitpick | 🔵 TrivialThe heap-limit regression guard is opt-in and never runs in this CI workflow.
e2e/shiftingMosaicHeapLimit.slow.test.ts(the test that specifically guards against theRuntimeError: Aborted()regression on thethreads-mem64variant) is skipped unlessSM_SLOW_E2Eis set, and this workflow never sets it. The threads+mem64 build is only compile-checked here (per the comment at lines 118-120), so the actual runtime regression this test protects against currently has no automated coverage. Consider adding a scheduled/nightly job that runs withSM_SLOW_E2E=1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 128 - 135, Add scheduled/nightly CI coverage for the heap-limit regression by introducing a job or workflow step that sets SM_SLOW_E2E=1 and runs the relevant slow end-to-end test, including the threads-mem64 runtime path. Keep the existing compile and standard test steps unchanged, and ensure the scheduled job executes e2e/shiftingMosaicHeapLimit.slow.test.ts rather than leaving it opt-in only.e2e/rollingBlocksSolver.placement.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate, unused
setupSolvedPuzzlehelper.This helper is byte-identical to the one in
e2e/rollingBlocksSolver.solution.test.tsbut is never called by the single test in this file. Extract to a shared test-helper module, or drop it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.placement.test.ts` around lines 3 - 31, Remove the unused setupSolvedPuzzle helper from this placement test, since it duplicates the helper in rollingBlocksSolver.solution.test.ts and is not called here.e2e/rollingBlocksSolver.solution.test.ts (1)
3-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate helper — candidate for extraction.
Same
setupSolvedPuzzleimplementation is duplicated verbatim ine2e/rollingBlocksSolver.placement.test.ts(where it's unused). Move to a shared helper module imported by both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.solution.test.ts` around lines 3 - 31, Extract the duplicated setupSolvedPuzzle helper into a shared e2e helper module, then import and use it from both rollingBlocksSolver.solution.test.ts and rollingBlocksSolver.placement.test.ts. Preserve the existing setup behavior and remove the unused local duplicate from the placement test.e2e/shiftingMosaicSolver.config.test.ts (1)
1-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test fixtures/helpers, one unused.
Same block as
e2e/shiftingMosaicSolver.blocks.test.tslines 1-59 — see that comment.paintWithGapis unused here too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicSolver.config.test.ts` around lines 1 - 59, Remove the duplicated SAMPLE_CONFIG, Cell type, and cellAt, cellCenter, paintShape, and unused paintWithGap helpers from this test file; reuse the corresponding shared fixtures and helpers from shiftingMosaicSolver.blocks.test.ts instead, updating imports or references as needed.e2e/shiftingMosaicSolver.blocks.test.ts (1)
1-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test fixtures/helpers, one unused.
SAMPLE_CONFIG,Cell,cellAt,cellCenter,paintShape, andpaintWithGapare byte-identical to the same block ine2e/shiftingMosaicSolver.config.test.ts. Additionally,paintWithGapis not called anywhere in either file. Recommend extracting the shared pieces to a common e2e helper module and droppingpaintWithGapif it's genuinely unused, or adding a test that exercises it (e.g., verifying gap-drag placement behavior) if it was meant to be used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicSolver.blocks.test.ts` around lines 1 - 59, Extract the duplicated SAMPLE_CONFIG, Cell type, cellAt, cellCenter, and paintShape fixtures/helpers from the shifting mosaic tests into a shared e2e helper module, then import and reuse them in both shiftingMosaicSolver.blocks.test.ts and shiftingMosaicSolver.config.test.ts. Remove paintWithGap if it is not needed; only retain it by adding a test that exercises its gap-drag behavior.src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css (1)
591-649: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolution-view colors are hardcoded literals rather than theme tokens.
#c84ad8,#1f9d57,#b88600and#ffffffbypass the--md-sys-color-*system used elsewhere in this file, so these markers won't adapt to a dark/high-contrast theme and their contrast against--md-sys-color-surface-*is unverified. Consider mapping them to theme tokens (or defining them once as custom properties with per-scheme overrides).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css` around lines 591 - 649, Replace the hardcoded solution-view marker colors in .sm-moving-label, .sm-dest-label, .sm-moving, .sm-dest::after, and .sm-goal-edge-* with existing --md-sys-color-* theme tokens, or define shared custom properties with light/dark/high-contrast overrides. Preserve the current visual roles while ensuring marker contrast adapts to the surrounding surface colors.e2e/shiftingMosaicWasmVariants.test.ts (1)
209-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSubstring assertions on emscripten glue are brittle.
maximum:131072n,address:"i64",shared:trueassume the exact minified spacing/quoting emscripten currently emits. A toolchain bump that emitsmaximum: 131072nor single quotes breaks this test without any real regression. Tolerant regexes keep the same guarantee.♻️ Suggested loosening
- expect(glue).toContain("maximum:131072n"); - expect(glue).toContain('address:"i64"'); - expect(glue).toContain("shared:true"); + expect(glue).toMatch(/maximum:\s*131072n/); + expect(glue).toMatch(/address:\s*['"]i64['"]/); + expect(glue).toMatch(/shared:\s*true/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/shiftingMosaicWasmVariants.test.ts` around lines 209 - 216, Update the glue assertions in the test around the fetched astar.threads.mem64.mjs content to use whitespace- and quote-tolerant regular expressions instead of exact substring matches for maximum, address, and shared. Preserve validation of the same values and flags while allowing equivalent Emscripten formatting changes.test/shiftingMosaicSolver/wasm.test.ts (1)
183-224: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo internal deadline: a test-level timeout leaves every arm worker alive.
racePortfolioonly settles ondone/error/exit. If an arm neither finishes nor exits, the Bun per-test timeout fires while the promise is still pending, sofinish()never runs and the nine workers (each holding a wasm heap) survive into the next fixture — the exact leak theexithandler at Line 220 was added to avoid. A watchdog slightly under the test timeout closes the remaining path.🛡️ Suggested guard
return new Promise(resolve => { const workers = arms.map(() => new ArmWorker(workerPath)); let pending = workers.length; let settled = false; + let watchdog: ReturnType<typeof setTimeout>; const finish = (turns: WasmTurn[]) => { if (settled) return; settled = true; + clearTimeout(watchdog); for (const w of workers) void w.terminate(); resolve(turns); }; + // Slightly under the enclosing test timeout so the arms are always + // torn down by us rather than abandoned by the runner. + watchdog = setTimeout(() => finish([]), budgetMs * 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/wasm.test.ts` around lines 183 - 224, Add an internal watchdog within racePortfolio that fires slightly before the test-level timeout, calls finish([]) to terminate all still-running arm workers, and settles the promise. Ensure the watchdog is cleared when finish completes, while preserving the existing done/error/exit retirement behavior.src/pages/shifting-mosaic-solver/a-star/CMakeLists.txt (1)
54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded CLion paths and pinned LLVM 23 make linting effectively local-only.
The hint list pins one machine's CLion install, and
SM_CLANG_TIDY_PROFILE_LLVM 23(a dev snapshot no distro ships) means every CI host takes the skip branch — the "tree cannot drift out of compliance" goal isn't enforced anywhere automated. Consider letting users point at their own binary, e.g. honoring a cache/env override before the hints.♻️ Optional override hook
find_program(SM_CLANG_TIDY_EXE NAMES clang-tidy - HINTS "$ENV{ProgramFiles}/JetBrains/CLion 2024.1.4/bin/clang/win/x64/bin" + HINTS "$ENV{SM_CLANG_TIDY_DIR}" + "$ENV{ProgramFiles}/JetBrains/CLion 2024.1.4/bin/clang/win/x64/bin" "$ENV{ProgramFiles}/JetBrains/CLion/bin/clang/win/x64/bin" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/CMakeLists.txt` around lines 54 - 62, Update the SM_CLANG_TIDY_EXE discovery in CMakeLists.txt to honor a user-provided cache or environment override for the clang-tidy executable before applying CLion-specific HINTS, and avoid making lint enforcement depend solely on the pinned LLVM 23 profile. Preserve the existing fallback discovery while allowing CI and other hosts to supply their own compatible clang-tidy binary.e2e/rollingBlocksSolver.tools.test.ts (1)
922-922: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
setupSolvedPuzzlehelper
e2e/rollingBlocksSolver.tools.test.ts:3no longer references it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/rollingBlocksSolver.tools.test.ts` at line 922, Remove the unused setupSolvedPuzzle helper from rollingBlocksSolver.tools.test.ts, including its definition and any associated import or setup code, since no references remain.test/shiftingMosaicSolver/board.pointer.test.ts (1)
1-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
Boardtest harness instead of triplicating it.The
editorMockshape, DOM stub, andBoardconstruction are copy-pasted identically across three files. A shared helper would reduce upkeep when theBoard/editor interface changes.
test/shiftingMosaicSolver/board.pointer.test.ts#L1-L31: import a sharedcreateBoardTestContext()helper instead of inlining the mock/Board setup.test/shiftingMosaicSolver/board.render.test.ts#L1-L31: same — replace the duplicatedbeforeEachblock with the shared helper.test/shiftingMosaicSolver/board.state.test.ts#L1-L31: same — replace the duplicatedbeforeEachblock with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/board.pointer.test.ts` around lines 1 - 31, Extract the duplicated editor mock, DOM setup, and Board initialization into a shared createBoardTestContext() helper, then update the beforeEach setup in test/shiftingMosaicSolver/board.pointer.test.ts:1-31, test/shiftingMosaicSolver/board.render.test.ts:1-31, and test/shiftingMosaicSolver/board.state.test.ts:1-31 to use it while preserving each test’s board and internal context access.src/pages/shifting-mosaic-solver/a-star/AStar.h (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffClass AStar exceeds the 20-field cohesion threshold (22 fields, per SonarCloud).
Consider grouping the board-analysis caches (
blockValidAnchorMask_,initialGoalPathCells_,blockSafeAnchorDist_,goalAnchorBfsDist_) into a small nested struct to reduce the member count and improve cohesion. Given AStar.cpp's own notes that restructuring this file has measurably regressed performance before, treat this as low priority and re-measure if attempted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStar.h` at line 16, Optionally reduce AStar’s 22-member count by grouping blockValidAnchorMask_, initialGoalPathCells_, blockSafeAnchorDist_, and goalAnchorBfsDist_ into a small nested cache struct within AStar, updating all AStar.cpp accesses accordingly. Treat this as low priority, preserve behavior and performance, and re-measure before keeping the change.Source: Linters/SAST tools
src/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cpp (1)
239-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
tryReorderMerge's improvement check compares against the wrong step metric (currently safe, but fragile).
baseStepsis computed viacomputeRuns(turns).size()— which splits on direction changes within a block — while the accept condition compares it againstcountSteps(cand), which only splits on block-ID changes (the coarser, actual player-step metric per this function's own doc comment: "the player-step count drops by one").runs.size() >= countSteps(turns)always, so this check is a looser bound than intended.Right now this can't produce a wrong accept: reaching this branch requires an intervening different-block run between
ri/rj(proven by theprevRunOfBlocktracking), which guarantees the merge always yields exactlycountSteps(original) - 1. But the check doesn't actually verify that invariant — comparing againstcountSteps(turns)directly would make the code self-validating against future changes to the run-tracking logic instead of relying on an implicit proof.♻️ Proposed fix to compare against the actual pre-merge step count
bool AStar::tryReorderMerge(std::vector<Turn> &turns) const { const auto runs = computeRuns(turns); - const size_t baseSteps = runs.size(); + const size_t baseSteps = countSteps(turns); std::vector prevRunOfBlock(shapes_.size(), -1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cpp` around lines 239 - 266, Update tryReorderMerge to compute the baseline using countSteps(turns) rather than runs.size(), then compare both runPulledEarlier and runPushedLater candidates against that pre-merge player-step count while retaining replaySolves validation and the existing successful replacement behavior.src/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.h (1)
205-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
aloneis dead — confirm the goal-target check is intentionally bbox-only.
aloneis built and assigned but never read; only the bbox test below decides the target. If the leftover was meant to feed anoracleCanPlace(p, 0, t, alone)check, that check is missing; otherwise drop the two lines.♻️ Remove the unused vector
const Position t = {.x = static_cast<int8_t>(rng.draw(xDist)), .y = static_cast<int8_t>(rng.draw(yDist))}; - std::vector<Position> alone(p.anchors.size(), {.x = 127, .y = 127}); - alone[0] = t; // Target just needs the goal's bbox in-grid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.h` around lines 205 - 220, Remove the unused alone vector and its assignment in the goal-target generation loop around p.goalAnchor. Keep the existing bounding-box-only validity check and target selection behavior unchanged.src/pages/shifting-mosaic-solver/a-star/test/dragsolver_search_test.cpp (1)
159-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWall-clock budget with a hard success assertion can flake on loaded CI.
ASSERT_FALSE(turns.empty())for every oracle-solvable board is bounded by a 10s wall-clock budget, exactly the machine-speed dependence you avoided inJamRoundShapingStaysValidAndDefaultsUnchangedby switching to a node cap (Lines 116-120). Bounding these by nodes instead (searchHierarchical(0, N)) would make the pass/fail deterministic. Same applies toJamGuideAndJitterStayValidAndComplete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/test/dragsolver_search_test.cpp` around lines 159 - 173, The solvable-board tests use a wall-clock search budget, making success assertions machine-speed dependent. In HierarchicalSolvesSolvableBoards and JamGuideAndJitterStayValidAndComplete, replace the time-based searchHierarchical limit with a sufficiently large node cap via the node-budget argument (for example, searchHierarchical(0, N)), preserving the existing solvability and replay-validity assertions.src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp (1)
138-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
writeTurnsFilesilently swallows write failures.Unlike
writeFixtureAt, this never checksis_open()/stream state, so an unwritable path loses the plan with no signal — awkward when the turns file is the artifact a fuzz repro depends on.♻️ Return success like `writeFixtureAt`
-void writeTurnsFile(const std::vector<Turn> &turns, const std::string &path) { +bool writeTurnsFile(const std::vector<Turn> &turns, const std::string &path) { json tj = json::array(); for (const auto &[blockId, direction] : turns) tj.push_back( {{"blockId", blockId}, {"direction", static_cast<int>(std::to_underlying(direction))}}); std::ofstream f(path); + if (!f.is_open()) + return false; f << tj.dump(); + return f.good(); }Header signature in
BenchFixture.hneeds the matching update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchFixture.cpp` around lines 138 - 146, Update writeTurnsFile to return a success status like writeFixtureAt: verify the output stream opened successfully and remains valid after writing, returning false on failure and true only when the turns JSON is written successfully. Update the matching declaration in BenchFixture.h and adjust all call sites to handle the returned status.src/pages/shifting-mosaic-solver/a-star/BenchFixture.h (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
BenchFixture.hself-contained. Add<cstddef>and change thesize_treturn types tostd::size_tinstead of relying on transitive/global declarations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchFixture.h` around lines 6 - 8, Add <cstddef> to BenchFixture.h and update all size_t return types in the BenchFixture declarations to std::size_t, ensuring the header is self-contained and no longer depends on transitive or global declarations.src/pages/shifting-mosaic-solver/a-star/BenchOptions.cpp (2)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the hard-coded array size.
std::array<FlagSpec, 41>must be updated by hand whenever a flag is added/removed; a mismatch is a compile error at best, and it is pure bookkeeping.constexpr FlagSpec FLAGS[]{...}(or CTAD viastd::to_array) deduces it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchOptions.cpp` at line 57, Update the FLAGS declaration in BenchOptions to use a size-deduced array instead of the hard-coded std::array<FlagSpec, 41>, such as a deduced constexpr array or std::to_array, while preserving the existing flag entries and constexpr behavior.
209-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider bailing out as soon as the cursor records an error.
cur.error()is only inspected after the whole argv is consumed, so a malformed value keeps applying later flags (andvalue()returning""writes empty paths intoopt). Behaviour is still correct becauseparseBenchOptionsreturns false andmain.cppaborts before use, but an earlyif (!cur.error().empty()) break;inside the loop makes the contract obvious.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/BenchOptions.cpp` around lines 209 - 226, Update parseBenchOptions to check cur.error() immediately after each spec->apply(opt, cur) call and stop processing further arguments when an error is recorded. Keep the existing final error reporting and false return unchanged, while preventing later flags from modifying opt after a malformed value.src/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cpp (1)
130-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify the score bit-packing has room for the jam term.
jamChildis capped at1u << 19and shifted left 14, so the top field occupies bits 14–33 of auint64_t— fine — buth << 10only leaves bits 10–13 for a value that is documented as 0/1, and the jitter is masked to 10 bits. Worth a brief comment recording the field widths so a future change toh(or a wider jitter) doesn't silently collide.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cpp` around lines 130 - 137, Add a concise comment immediately above the score construction documenting the bit layout: jamChild uses bits 14–33, h uses bits 10–13, and beamJitter uses the low 10 bits. Keep the existing packing unchanged and note the expected h range so future width changes are checked for overlap.src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp (2)
53-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the
40reachability threshold.Every other tuned constant in this file is a named
constexprwith a rationale comment (UNFREEZE_K,ENDGAME_K,MAX_JOINT,MAX_BACKTRACKS); this one is a bare literal that silently decides "big enough free region ⇒ approachable".♻️ Suggested change
+ // A flood region this large is effectively open space; treat the piece as + // approachable without checking its current anchor. + constexpr size_t OPEN_REGION_CELLS = 40; if (const auto &reached = grid_.floodFill(piece, slotPos(piece)); - reached.size() >= 40) + reached.size() >= OPEN_REGION_CELLS) return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp` around lines 53 - 66, Name the flood-fill reachability threshold used by DragSolver::approachable as a file-level constexpr, replacing the bare 40 literal. Add a concise rationale comment documenting that it represents the minimum free-region size considered large enough for approachability, and use the named constant in the reached.size() comparison.
206-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSolver progress goes to raw
std::cout.This code also runs inside the wasm build where the arms race concurrently; unconditional
std::coutfrom every arm interleaves and cannot be silenced by callers. A verbosity flag onConfig(or a small logging hook alongsideonProgress) would keep the diagnostics without forcing them on embedders.Also applies to: 274-279, 440-466
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cpp` around lines 206 - 218, The solver’s progress messages in the joint-endgame and other cited paths write unconditionally to std::cout, preventing callers from suppressing or safely collecting output. Route these diagnostics through a Config verbosity flag or logging/progress hook alongside onProgress, and guard or dispatch all affected messages—including the locations around jointEndgame and lines 440-466—through that mechanism instead of direct std::cout.src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp (1)
211-252: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider avoiding
std::functionfor the hot packing DFS.
dfsis called up toPACK_NODE_CAP(5M) times and every recursive step pays an indirect call throughstd::functionplus the type-erasure barrier that blocks inlining. A small local recursive struct (or atemplate <typename Self>lambda / explicit worklist) keeps the same shape without the indirection.♻️ Sketch
- std::function<bool(size_t)> dfs = [&](const size_t fromRank) -> bool { + const auto dfs = [&](this const auto &self, const size_t fromRank) -> bool {(deducing-
thisrecursive lambda if the toolchain is C++23; otherwise a localstruct Dfs { ... bool run(size_t); }.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp` around lines 211 - 252, Replace the std::function-based recursive dfs in the packing search with a directly callable recursive implementation, such as a self-passing templated lambda or local recursive struct, while preserving the existing DFS behavior, state updates, and PACK_NODE_CAP handling. Ensure recursive calls avoid std::function type erasure and indirect dispatch.Source: Linters/SAST tools
src/pages/shifting-mosaic-solver/a-star/DragSolverArms.cpp (1)
176-178: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the unsigned subtraction in the remaining-node computation.
maxNodes - stats_.nodesExpandedwraps if expansions ever overshootmaxNodes(rounds are capped, not clamped), and the wrapped value makesstd::mina no-op — the opposite of the intent. The loop guard currently makes this unreachable, but a saturating form removes the dependency.♻️ Proposed change
- if (maxNodes != 0) - cap = std::min<uint32_t>(cap, maxNodes - stats_.nodesExpanded); + if (maxNodes != 0) + cap = std::min<uint32_t>(cap, maxNodes > stats_.nodesExpanded + ? maxNodes - stats_.nodesExpanded + : 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverArms.cpp` around lines 176 - 178, Update the remaining-node calculation near the return cap logic to saturate maxNodes - stats_.nodesExpanded at zero when expansions meet or exceed maxNodes, then apply std::min with cap only to the non-underflowing remainder. Preserve the existing unlimited behavior when maxNodes is zero.src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp (3)
40-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
operator==onDragHeapEntryappears dead code.The heap only needs ordering (via
std::greater<>, which is satisfiable fromoperator<=>);operator==isn't otherwise invoked in this file. SonarCloud's "unused member function" flag looks correct here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp` around lines 40 - 49, Remove the unused operator== overload from DragHeapEntry, leaving operator<=> unchanged so std::greater<> continues to provide the heap ordering.Source: Linters/SAST tools
300-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReference-identity shortcut for mask comparison — fragile if
cutSuffixRows_is ever resized mid-search.if (&maskN == &exp.maskP) return 1 + exp.baseCount;This relies on
cutSuffixRows_elements never moving between the timeexp.maskPwas bound and this call. Given it's a member fixed up-front percomputeDisplacementFields, this is presumably safe today, but it's a non-obvious invariant for future maintainers touchingcutSuffixRows_'s lifetime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp` around lines 300 - 318, Replace the address-based mask comparison in the displacement-count logic with a value-based comparison between maskN and exp.maskP, preserving the existing 1 + exp.baseCount result when the masks are equivalent. Update the code in the method containing the DragExpansion parameter without changing the surrounding counting behavior.
20-51: 📐 Maintainability & Code Quality | 🔵 TrivialAlign the analyzer with C++23
src/pages/shifting-mosaic-solver/a-star/CMakeLists.txtalready setsCMAKE_CXX_STANDARD 23, and this codebase deliberately uses<=>,std::ranges, and.contains(). The static-analysis job needs the same standard flag instead of reporting these as source defects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cpp` around lines 20 - 51, Update the analyzer configuration in the a-star CMake setup to use C++23, matching the existing CMAKE_CXX_STANDARD 23 setting. Ensure the static-analysis invocation receives the equivalent C++23 standard flag so valid usage of DragHeapEntry’s operator<=> and other C++23 APIs is analyzed correctly.test/shiftingMosaicSolver/aStar.search.test.ts (1)
216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
toHaveLengthfor length assertions.SonarCloud suggests
expect(turns).toHaveLength(1)overexpect(turns.length).toBe(1)for clearer failure reporting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.search.test.ts` at line 216, Update the assertion in the relevant A* search test to use toHaveLength directly on turns, preserving the expected length of 1.Source: Linters/SAST tools
src/pages/shifting-mosaic-solver/aStar.ts (4)
106-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove field declarations above the constructor.
movableBlockIndices,unsolvableAtStart, andgoalAnchorBfsDistare declared with initializers after the constructor that populates them (lines 75-92). This is safe (field initializers run before the constructor body), but it reads as if the constructor's work gets reset — worth moving these declarations above the constructor for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 106 - 110, Move the field declarations movableBlockIndices, unsolvableAtStart, and goalAnchorBfsDist above the constructor in the solver class, preserving their existing initializers and types. Do not change the constructor’s initialization logic or field behavior.
152-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReduce cognitive complexity of
search.SonarCloud flags this function at cognitive complexity 39 (limit 15). Logic itself checks out (dedup via closedSet/gScore, timeout budget, successor generation), but consider extracting the successor-expansion inner loop (Lines 220-254) and/or the timeout/goal checks into named helpers to aid readability and future maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 152 - 259, Reduce cognitive complexity in the AStar search method by extracting the successor-generation logic from the movableBlockIndices/direction loops into a named helper, and extract additional timeout or goal-check handling if needed to meet the configured threshold. Preserve the existing closedSet/gScore deduplication, cameFrom updates, heap insertion, timeout behavior, deadlock pruning, and goal-path reconstruction exactly.Source: Linters/SAST tools
377-429: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReduce cognitive complexity of
computeMovableSet.SonarCloud flags this function at cognitive complexity 37 (limit 15). The fixpoint logic is correct, but consider extracting the per-block "can this direction be freed" check (Lines 405-414) into a helper method to shrink nesting depth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 377 - 429, Reduce cognitive complexity in computeMovableSet by extracting the per-direction occupancy check into a focused helper method that determines whether a block can move in a given direction, using the existing anchors, shape, occupancy, and movable set. Keep the fixpoint iteration and its current behavior unchanged, and have computeMovableSet delegate the directional checks to the helper.Source: Linters/SAST tools
436-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce cognitive complexity of
isDeadlocked.SonarCloud flags cognitive complexity 21 (limit 15). The "does block i occupy a goal-final cell" check is duplicated between the
hasBlockerscan and the final loop — extracting it into a helper both fixes the duplication and reduces nesting.♻️ Proposed refactor
+ private blockOccupiesGoalFinalCells(shapeIndex: number, anchor: Position): boolean { + for (const cell of this.shapes[shapeIndex]!) { + const enc = (anchor.x + cell.x) * this.gridHeight + (anchor.y + cell.y); + if (this.goalBlockFinalCells.has(enc)) return true; + } + return false; + } + private isDeadlocked(anchors: Position[]): boolean { - const stride = this.gridHeight; - let hasBlocker = false; - for (let i = 0; i < anchors.length; i++) { - if (i === this.goalIndex) continue; - const a = anchors[i]!; - for (const cell of this.shapes[i]!) { - const enc = (a.x + cell.x) * stride + (a.y + cell.y); - if (this.goalBlockFinalCells.has(enc)) { - hasBlocker = true; - break; - } - } - if (hasBlocker) break; - } - if (!hasBlocker) return false; + const hasBlocker = anchors.some( + (a, i) => i !== this.goalIndex && this.blockOccupiesGoalFinalCells(i, a), + ); + if (!hasBlocker) return false; const movable = this.computeMovableSet(anchors); - for (let i = 0; i < anchors.length; i++) { - if (i === this.goalIndex) continue; - if (movable.has(i)) continue; - const a = anchors[i]!; - for (const cell of this.shapes[i]!) { - const enc = (a.x + cell.x) * stride + (a.y + cell.y); - if (this.goalBlockFinalCells.has(enc)) return true; - } - } - return false; + return anchors.some( + (a, i) => + i !== this.goalIndex && + !movable.has(i) && + this.blockOccupiesGoalFinalCells(i, a), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/aStar.ts` around lines 436 - 465, Reduce cognitive complexity in isDeadlocked by extracting the duplicated check for whether a block occupies any cell in goalBlockFinalCells into a private helper. Reuse that helper in both the initial hasBlocker scan and the final immovable-block scan, preserving the existing goalIndex exclusions and deadlock behavior.Source: Linters/SAST tools
test/shiftingMosaicSolver/aStar.collision.test.ts (2)
55-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmpty test asserts nothing.
This test builds shapes, comments through several false starts, then only does
void shapeA; void shapeB; void shapeBReal;— noblocksCollidecall, noexpect(...). It always passes without testing anything, which is misleading since the next test (Lines 83-112) already covers the intended bbox-overlap-but-no-cell-collision scenario. Recommend removing this test.♻️ Proposed fix
- test("two L-shapes whose bboxes overlap but whose cells do not are not colliding", () => { - // Shape A: L pointing top-right - // X X - // X . - const shapeA: Position[] = [ - { x: 0, y: 0 }, - { x: 1, y: 0 }, - { x: 0, y: 1 }, - ]; - // Shape B: just bottom-right cell - // . X - const shapeB: Position[] = [{ x: 1, y: 1 }]; - // We use a 2x2 bbox shape B by also including {x:0,y:0} for bbox - // ... but we want a shape that fits the (1,1) corner exactly. - // Easier: use a real bbox-overlapping case where the cell at (1,1) is empty. - const shapeBReal: Position[] = [ - { x: 1, y: 1 }, - { x: 0, y: 1 }, - ]; - // shapeBReal's bbox is 2x2 (because we have x∈{0,1}, y∈{0,1}... wait y=1 only) - // Actually bbox is 2 wide, 1 tall (since min(y)=1, but bbox calc uses max(y)+1 = 2) - // Hmm bbox calc is just max+1 — let me redo with normalized shape. - // Skip this — use a more direct test below instead. - void shapeA; - void shapeB; - void shapeBReal; - }); - test("L-shape and single cell — bbox overlaps, cells overlap", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.collision.test.ts` around lines 55 - 81, Remove the empty test case that defines shapeA, shapeB, and shapeBReal but performs no collision check or assertion. Keep the subsequent test covering bbox overlap without cell collision unchanged.
12-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated test helpers across two files.
makeAStarandapplyTurnsare copy-pasted verbatim between the two test files; extracting them into a shared test-utils module (e.g.test/shiftingMosaicSolver/testHelpers.ts) avoids drift as more AStar unit-test files are added to this suite.
test/shiftingMosaicSolver/aStar.collision.test.ts#L12-L51: importmakeAStar/applyTurnsfrom a new shared helper module instead of defining them locally.test/shiftingMosaicSolver/aStar.search.test.ts#L12-L51: same — import from the shared helper module instead of the duplicate local definitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shiftingMosaicSolver/aStar.collision.test.ts` around lines 12 - 51, Extract the duplicated makeAStar and applyTurns helpers into a shared test utility module. In test/shiftingMosaicSolver/aStar.collision.test.ts lines 12-51 and test/shiftingMosaicSolver/aStar.search.test.ts lines 12-51, remove the local definitions and import both helpers from the shared module, preserving their existing behavior and required dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 302fa3b5-73de-4b7d-b3b1-287957c48865
📒 Files selected for processing (182)
.claude/settings.local.json.github/workflows/test.yml.gitignoree2e/phasicDialSolver.solvable.test.tse2e/phasicDialSolver.states.test.tse2e/rollingBlocksSolver.placement.test.tse2e/rollingBlocksSolver.solution.test.tse2e/rollingBlocksSolver.tools.test.tse2e/shiftingMosaicHeapLimit.slow.test.tse2e/shiftingMosaicSolver.blocks.test.tse2e/shiftingMosaicSolver.config.test.tse2e/shiftingMosaicSolver.editing.test.tse2e/shiftingMosaicSolver.solving.test.tse2e/shiftingMosaicSolver.test.tse2e/shiftingMosaicWasmVariants.test.tspackage.jsonplaywright.config.tssrc/common/coi-serviceworker.jssrc/common/coiRegister.tssrc/common/common.csssrc/pages/phasic-dial-solver/phasicDialSolver.tssrc/pages/rolling-blocks-solver/a-star/AStar.cppsrc/pages/rolling-blocks-solver/a-star/AStar.hsrc/pages/rolling-blocks-solver/a-star/AStarGoals.cppsrc/pages/rolling-blocks-solver/a-star/CMakeLists.txtsrc/pages/rolling-blocks-solver/a-star/GoalCluster.hsrc/pages/rolling-blocks-solver/a-star/Node.hsrc/pages/rolling-blocks-solver/a-star/NodeKey.hsrc/pages/rolling-blocks-solver/a-star/test/CMakeLists.txtsrc/pages/rolling-blocks-solver/a-star/wasm_bindings.cppsrc/pages/rolling-blocks-solver/aStar.tssrc/pages/rolling-blocks-solver/bfs.tssrc/pages/rolling-blocks-solver/board.tssrc/pages/rolling-blocks-solver/goalClusters.tssrc/pages/rolling-blocks-solver/rollingBlocksSolver.tssrc/pages/shifting-mosaic-solver/a-star/.clang-tidysrc/pages/shifting-mosaic-solver/a-star/AStar.cppsrc/pages/shifting-mosaic-solver/a-star/AStar.hsrc/pages/shifting-mosaic-solver/a-star/AStarHeuristics.cppsrc/pages/shifting-mosaic-solver/a-star/AStarOptimizer.cppsrc/pages/shifting-mosaic-solver/a-star/AStarSearch.cppsrc/pages/shifting-mosaic-solver/a-star/BenchCommands.cppsrc/pages/shifting-mosaic-solver/a-star/BenchCommands.hsrc/pages/shifting-mosaic-solver/a-star/BenchEngines.cppsrc/pages/shifting-mosaic-solver/a-star/BenchEngines.hsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.cppsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.hsrc/pages/shifting-mosaic-solver/a-star/BenchOptions.cppsrc/pages/shifting-mosaic-solver/a-star/BenchOptions.hsrc/pages/shifting-mosaic-solver/a-star/BitGrid.hsrc/pages/shifting-mosaic-solver/a-star/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/ClangdCompat.hsrc/pages/shifting-mosaic-solver/a-star/DragSolver.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolver.hsrc/pages/shifting-mosaic-solver/a-star/DragSolverArms.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverAssembly.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverConfig.hsrc/pages/shifting-mosaic-solver/a-star/DragSolverExpand.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverHeuristics.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverSearch.cppsrc/pages/shifting-mosaic-solver/a-star/MemoryProbe.hsrc/pages/shifting-mosaic-solver/a-star/Node.hsrc/pages/shifting-mosaic-solver/a-star/NodeKey.hsrc/pages/shifting-mosaic-solver/a-star/PARALLEL.mdsrc/pages/shifting-mosaic-solver/a-star/ParallelCascade.hsrc/pages/shifting-mosaic-solver/a-star/SeededRng.hsrc/pages/shifting-mosaic-solver/a-star/SolverClock.cppsrc/pages/shifting-mosaic-solver/a-star/SolverClock.hsrc/pages/shifting-mosaic-solver/a-star/StateTable.hsrc/pages/shifting-mosaic-solver/a-star/Types.hsrc/pages/shifting-mosaic-solver/a-star/bench/HARD-BOARDS.mdsrc/pages/shifting-mosaic-solver/a-star/bench/after-cascade.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/after-luby.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/baseline-unit.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/before-cascade.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/drag-settled-w2.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-1022-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-10276-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-11386-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-12366-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-23444-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-29534-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-3058-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-3086-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-3870-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-4722-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-8697-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/bench/fuzz-9164-hard.jsonsrc/pages/shifting-mosaic-solver/a-star/main.cppsrc/pages/shifting-mosaic-solver/a-star/test/CMakeLists.txtsrc/pages/shifting-mosaic-solver/a-star/test/DragSolverTestOracle.hsrc/pages/shifting-mosaic-solver/a-star/test/astar_unit_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/dragsolver_search_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/dragsolver_unit_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/json_test.cppsrc/pages/shifting-mosaic-solver/a-star/test/optimizer_test.cppsrc/pages/shifting-mosaic-solver/a-star/wasm_bindings.cppsrc/pages/shifting-mosaic-solver/aStar.tssrc/pages/shifting-mosaic-solver/board.tssrc/pages/shifting-mosaic-solver/boardGeometry.tssrc/pages/shifting-mosaic-solver/config.tssrc/pages/shifting-mosaic-solver/directions.tssrc/pages/shifting-mosaic-solver/editorHelpers.tssrc/pages/shifting-mosaic-solver/index.htmlsrc/pages/shifting-mosaic-solver/layout.tssrc/pages/shifting-mosaic-solver/node.tssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.csssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.tssrc/pages/shifting-mosaic-solver/solutionView.tssrc/pages/shifting-mosaic-solver/turn.tssrc/pages/shifting-mosaic-solver/wasm/astar.worker.jssrc/pages/shifting-mosaic-solver/wasmBridge.tssrc/util/benchShiftingMosaic.tssrc/util/benchWasmSimd.tssrc/util/build.tssrc/util/buildWasm.tssrc/util/fuzzShiftingMosaic.tssrc/util/minHeap.tssrc/util/plugins.tssrc/util/serve.tssrc/util/types.tstest/phasic-dial-solver/phasicDialSolver.test.tstest/resources/shiftingMosaicTest.jsontest/resources/shiftingMosaicTest1.jsontest/resources/shiftingMosaicTest10.jsontest/resources/shiftingMosaicTest11.jsontest/resources/shiftingMosaicTest12.jsontest/resources/shiftingMosaicTest13.jsontest/resources/shiftingMosaicTest14.jsontest/resources/shiftingMosaicTest15.jsontest/resources/shiftingMosaicTest16.jsontest/resources/shiftingMosaicTest17.jsontest/resources/shiftingMosaicTest18.jsontest/resources/shiftingMosaicTest19.jsontest/resources/shiftingMosaicTest2.jsontest/resources/shiftingMosaicTest20.jsontest/resources/shiftingMosaicTest21.jsontest/resources/shiftingMosaicTest22.jsontest/resources/shiftingMosaicTest23.jsontest/resources/shiftingMosaicTest24.jsontest/resources/shiftingMosaicTest25.jsontest/resources/shiftingMosaicTest26.jsontest/resources/shiftingMosaicTest27.jsontest/resources/shiftingMosaicTest28.jsontest/resources/shiftingMosaicTest29.jsontest/resources/shiftingMosaicTest3.jsontest/resources/shiftingMosaicTest30.jsontest/resources/shiftingMosaicTest31.jsontest/resources/shiftingMosaicTest32.jsontest/resources/shiftingMosaicTest33.jsontest/resources/shiftingMosaicTest34.jsontest/resources/shiftingMosaicTest35.jsontest/resources/shiftingMosaicTest36.jsontest/resources/shiftingMosaicTest37.jsontest/resources/shiftingMosaicTest38.jsontest/resources/shiftingMosaicTest39.jsontest/resources/shiftingMosaicTest4.jsontest/resources/shiftingMosaicTest40.jsontest/resources/shiftingMosaicTest41.jsontest/resources/shiftingMosaicTest42.jsontest/resources/shiftingMosaicTest43.jsontest/resources/shiftingMosaicTest5.jsontest/resources/shiftingMosaicTest6.jsontest/resources/shiftingMosaicTest7.jsontest/resources/shiftingMosaicTest8.jsontest/resources/shiftingMosaicTest9.jsontest/rollingBlocksSolver/block.musttouch.test.tstest/rollingBlocksSolver/block.roll.test.tstest/rollingBlocksSolver/block.validity.test.tstest/shiftingMosaicSolver/aStar.collision.test.tstest/shiftingMosaicSolver/aStar.primitives.test.tstest/shiftingMosaicSolver/aStar.search.test.tstest/shiftingMosaicSolver/board.geometry.test.tstest/shiftingMosaicSolver/board.pointer.test.tstest/shiftingMosaicSolver/board.render.test.tstest/shiftingMosaicSolver/board.state.test.tstest/shiftingMosaicSolver/board.test.tstest/shiftingMosaicSolver/config.test.tstest/shiftingMosaicSolver/mem64.node.test.mjstest/shiftingMosaicSolver/portfolio.worker.tstest/shiftingMosaicSolver/wasm.test.ts
💤 Files with no reviewable changes (2)
- e2e/shiftingMosaicSolver.test.ts
- .claude/settings.local.json
🛑 Comments failed to post (1)
test/rollingBlocksSolver/block.validity.test.ts (1)
365-385: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test correctness depends on cross-iteration execution order.
The shared
indexvariable is set by the firsttest.eachiteration and consumed by the second. This only works if both iterations run strictly in sequence and order — fragile under reordering/parallel execution, and would throw (blocks[index]!undefined) or silently check the wrong block otherwise.♻️ Proposed fix: pair each case with its own index
- const nonOverlappingBlocksCase = [ + const nonOverlappingBlocksCase: [Block[], number][] = [ [ [ new Block(1, 0, 0, 4, 1, 1), new Block(2, 4, 0, 1, 4, 1), new Block(3, 1, 4, 4, 1, 1), new Block(4, 0, 1, 1, 4, 1), ], + 0, ], [ [ new Block(1, 0, 0, 1, 1, 1), // ...unchanged... ], + 13, ], ]; - - let index = 0; test.each(nonOverlappingBlocksCase)( "there should be no overlaps between blocks (%#)", - blocks => { + (blocks, index) => { checkOutOfBoundsSpy.mockReturnValue(true); checkBlockingCellsSpy.mockReturnValue(true); const result = blocks[index]!.checkValidity( 5, 5, cells, blocks, mustTouchCellsSatisfied, ); expect(result).toBeTrue(); expect((blocks[index]! as any).checkBlockingCells).toHaveBeenCalled(); - index = 13; }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/rollingBlocksSolver/block.validity.test.ts` around lines 365 - 385, Remove the shared index state from the test.each block and include the intended block index as part of each nonOverlappingBlocksCase test case. Destructure the case-specific index in the test callback and use it when selecting blocks[index], so each iteration is independent of execution order and parallelism.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts (2)
533-545: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDispose the active solution view when applying a config.
applyLoadedConfig()hides the solution view without callingSolutionView.dispose()or clearingthis.solutionView. Because drag-and-drop works globally, this path is reachable while a solution is open, leaving its listeners or animation resources alive until another solution is entered.Proposed fix
private applyLoadedConfig(config: ShiftingMosaicTest) { + this.solutionView?.dispose(); + this.solutionView = null; this.gridWidth = config.gridWidth;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts` around lines 533 - 545, Update applyLoadedConfig() to dispose the currently active solution view before hiding the solution UI. Call SolutionView.dispose() through this.solutionView when present, then clear this.solutionView so global drag-and-drop listeners and animation resources are released before loading the new configuration.
376-408: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIgnore callbacks from canceled solver runs.
stopCurrentWorker()only clears the bookkeeping field; theonDone/onErrorclosures do not verify that they belong to the active solve. A late callback after editing, reset, config loading, or starting another solve can show an obsolete solution or error for the current board. Add a monotonically increasing solve token and check it before handling callbacks or clearingcurrentWorker.Proposed fix
private currentWorker: SolverHandle | null = null; + private solveGeneration = 0; this.stopCurrentWorker(); + const generation = ++this.solveGeneration; onDone: turns => { + if (generation !== this.solveGeneration) return; this.currentWorker = null; this.handleSolution(puzzle, turns); }, onError: error => { + if (generation !== this.solveGeneration) return; this.currentWorker = null; this.showSolverError(error); }, private stopCurrentWorker() { + this.solveGeneration++; if (this.currentWorker) {Also applies to: 477-483, 585-589
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts` around lines 376 - 408, Update calculateSolution and the related solver callback paths to use a monotonically increasing solve token for each new solve. Capture the token in each searchShiftingMosaicWasm callback and ignore onProgress, onPhase, onDone, and onError when it no longer matches the active token; only the current run may clear currentWorker or update progress, solution, or error state. Increment the token when canceling or replacing a solve so callbacks from stopped runs are invalidated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts`:
- Around line 533-545: Update applyLoadedConfig() to dispose the currently
active solution view before hiding the solution UI. Call SolutionView.dispose()
through this.solutionView when present, then clear this.solutionView so global
drag-and-drop listeners and animation resources are released before loading the
new configuration.
- Around line 376-408: Update calculateSolution and the related solver callback
paths to use a monotonically increasing solve token for each new solve. Capture
the token in each searchShiftingMosaicWasm callback and ignore onProgress,
onPhase, onDone, and onError when it no longer matches the active token; only
the current run may clear currentWorker or update progress, solution, or error
state. Increment the token when canceling or replacing a solve so callbacks from
stopped runs are invalidated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47fffde6-be92-48c4-a795-fea7fb8e59f5
📒 Files selected for processing (30)
.github/workflows/test.ymlsrc/common/common.csssrc/pages/rolling-blocks-solver/a-star/AStar.hsrc/pages/rolling-blocks-solver/a-star/AStarGoals.cppsrc/pages/shifting-mosaic-solver/a-star/AStarSearch.cppsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.cppsrc/pages/shifting-mosaic-solver/a-star/BenchFixture.hsrc/pages/shifting-mosaic-solver/a-star/BitGrid.hsrc/pages/shifting-mosaic-solver/a-star/DragSolver.cppsrc/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cppsrc/pages/shifting-mosaic-solver/a-star/PARALLEL.mdsrc/pages/shifting-mosaic-solver/a-star/ParallelCascade.hsrc/pages/shifting-mosaic-solver/a-star/test/json_test.cppsrc/pages/shifting-mosaic-solver/editorHelpers.tssrc/pages/shifting-mosaic-solver/index.htmlsrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.csssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.tssrc/pages/shifting-mosaic-solver/solutionView.tssrc/pages/shifting-mosaic-solver/wasmBridge.tssrc/util/benchShiftingMosaic.tssrc/util/benchWasmSimd.tssrc/util/buildWasm.tssrc/util/fuzzShiftingMosaic.tssrc/util/wasmModule.tstest/rollingBlocksSolver/aStar.test.tstest/shiftingMosaicSolver/aStar.collision.test.tstest/shiftingMosaicSolver/aStar.search.test.tstest/shiftingMosaicSolver/mem64.node.test.mjstest/shiftingMosaicSolver/portfolio.worker.tstest/shiftingMosaicSolver/wasm.test.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- src/common/common.css
- test/shiftingMosaicSolver/aStar.collision.test.ts
- test/shiftingMosaicSolver/mem64.node.test.mjs
- test/shiftingMosaicSolver/portfolio.worker.ts
- src/util/benchWasmSimd.ts
- src/pages/shifting-mosaic-solver/editorHelpers.ts
- test/shiftingMosaicSolver/aStar.search.test.ts
- src/pages/shifting-mosaic-solver/a-star/BenchFixture.h
- .github/workflows/test.yml
- src/pages/shifting-mosaic-solver/shiftingMosaicSolver.css
- test/shiftingMosaicSolver/wasm.test.ts
- src/pages/shifting-mosaic-solver/a-star/PARALLEL.md
- src/util/buildWasm.ts
- src/pages/shifting-mosaic-solver/a-star/test/json_test.cpp
- src/util/fuzzShiftingMosaic.ts
- src/util/benchShiftingMosaic.ts
- src/pages/shifting-mosaic-solver/index.html
- src/pages/shifting-mosaic-solver/wasmBridge.ts
- src/pages/shifting-mosaic-solver/a-star/DragSolver.cpp
- src/pages/shifting-mosaic-solver/a-star/BitGrid.h
- src/pages/shifting-mosaic-solver/a-star/ParallelCascade.h
- src/pages/shifting-mosaic-solver/a-star/AStarSearch.cpp
- src/pages/shifting-mosaic-solver/a-star/DragSolverBeam.cpp
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/buildWasm.ts (1)
130-169: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the shifting-mosaic WASM builds atomic.
Promise.allwon’t stop the otherem++processes, so a failure in one variant can still overwrite files insrc/pages/shifting-mosaic-solver/wasmand leave that directory in a mixed state. Build each variant in a temp dir and swap it in on success, or terminate the sibling processes as soon as one fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/buildWasm.ts` around lines 130 - 169, Update the SHIFTING_MOSAIC_VARIANTS build flow so all variant outputs are produced atomically rather than directly in the shared wasm directory. Use a temporary output directory for the builds and replace the existing directory only after every build succeeds, or otherwise terminate sibling builds and clean up on failure; preserve the existing variant configuration and output filenames.
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
70-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the Emscripten SDK version for reproducible builds.
The new WASM step relies on the unversioned SDK setup above, while the MEMORY64 flags and comments assume a specific Emscripten generation. Select the exact SDK version validated by the repository instead of allowing toolchain drift. Emscripten supports explicit SDK version selection. (github.com)
- name: Setup Emscripten uses: emscripten-core/setup-emsdk@v16 + with: + version: "6.0.4" # or the exact version validated by the project🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 70 - 74, Pin the Emscripten SDK version in the “Build shifting-mosaic WASM” workflow step using the exact repository-validated version, rather than relying on the unversioned SDK setup. Keep the existing MEMORY64 build configuration unchanged.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 134-150: Update the MEMORY64 CI test invocation around
mem64.node.test.mjs so unsupported wasm64 runtimes cause a nonzero exit instead
of silently passing due to the test’s skip condition. Preserve skipping for
local runs by adding an explicit CI-only capability check or failure path, while
ensuring the workflow imports and exercises astar.mem64.mjs when support is
available.
- Line 14: Update the actions/checkout step to set persist-credentials to false,
and scope the workflow or job permissions to contents: read so subsequent
repository-controlled install and test steps cannot reuse a write-capable
checkout token.
---
Outside diff comments:
In `@src/util/buildWasm.ts`:
- Around line 130-169: Update the SHIFTING_MOSAIC_VARIANTS build flow so all
variant outputs are produced atomically rather than directly in the shared wasm
directory. Use a temporary output directory for the builds and replace the
existing directory only after every build succeeds, or otherwise terminate
sibling builds and clean up on failure; preserve the existing variant
configuration and output filenames.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 70-74: Pin the Emscripten SDK version in the “Build
shifting-mosaic WASM” workflow step using the exact repository-validated
version, rather than relying on the unversioned SDK setup. Keep the existing
MEMORY64 build configuration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0156333-17eb-48fc-b2de-7fda498b90b6
📒 Files selected for processing (13)
.github/workflows/test.ymle2e/coi.tse2e/index.test.tse2e/shiftingMosaicHeapLimit.slow.test.tse2e/shiftingMosaicSolver.blocks.test.tse2e/shiftingMosaicSolver.config.test.tse2e/shiftingMosaicSolver.editing.test.tse2e/shiftingMosaicSolver.solving.test.tse2e/shiftingMosaicWasmVariants.test.tssrc/pages/shifting-mosaic-solver/shiftingMosaicSolver.tssrc/pages/shifting-mosaic-solver/wasmBridge.tssrc/util/buildWasm.tstest/shiftingMosaicSolver/mem64.node.test.mjs
🚧 Files skipped from review as they are similar to previous changes (9)
- test/shiftingMosaicSolver/mem64.node.test.mjs
- e2e/shiftingMosaicHeapLimit.slow.test.ts
- e2e/shiftingMosaicSolver.solving.test.ts
- e2e/shiftingMosaicSolver.config.test.ts
- e2e/shiftingMosaicWasmVariants.test.ts
- e2e/shiftingMosaicSolver.blocks.test.ts
- e2e/shiftingMosaicSolver.editing.test.ts
- src/pages/shifting-mosaic-solver/shiftingMosaicSolver.ts
- src/pages/shifting-mosaic-solver/wasmBridge.ts
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v6 | ||
| uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files .github/workflows/test.yml
echo '---'
cat -n .github/workflows/test.ymlRepository: Valle12/islands-of-insight-tools
Length of output: 7350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect checkout action defaults and any explicit permissions in this workflow.
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/test.yml')
text = p.read_text()
for needle in ['permissions:', 'persist-credentials', 'actions/checkout@v7', 'bun ci', 'e2e', 'unit']:
print(f'### {needle}')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start = max(1, i-3)
end = min(len(text.splitlines()), i+3)
for j in range(start, end+1):
print(f'{j:4}: {text.splitlines()[j-1]}')
print()
PYRepository: Valle12/islands-of-insight-tools
Length of output: 1476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n .github/workflows/test.ymlRepository: Valle12/islands-of-insight-tools
Length of output: 7319
🌐 Web query:
actions/checkout@v7 persist-credentials default documentation
💡 Result:
In the actions/checkout version 7 action, the persist-credentials input maintains a default value of true [1][2]. When persist-credentials is set to its default (true), the action configures the authentication token or SSH key with the local git configuration [3][4][1]. This enables subsequent steps in your workflow to perform authenticated git commands (such as git fetch or git push) [3][4][5]. The credentials are removed during the post-job cleanup [3][5]. If you prefer to disable this behavior, you must explicitly set persist-credentials to false in your workflow step [3][5]: - uses: actions/checkout@v7 with: persist-credentials: false While there has been community discussion regarding changing this default to false to improve security [6][7], it remains true as of version 7 [1][2]. Notably, starting in version 6, the action was updated to store these credentials in a separate file under $RUNNER_TEMP rather than directly within the.git/config file, which improved credential security [8][5].
Citations:
- 1: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 2: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 3: https://github.com/actions/checkout/?tab=readme-ov-file
- 4: https://github.com/actions/checkout
- 5: https://github.com/marketplace/actions/checkout
- 6: [security] Escalate concerning default
persist-credentials=trueplease actions/checkout#2312 - 7: Change the default value of persist-credentials to false actions/checkout#1687
- 8: actions/checkout@v4.2.2...v7
Disable persisted checkout credentials. actions/checkout@v7 still defaults persist-credentials to true, and this job runs repository-controlled install/test steps afterward; set it to false (and scope permissions to contents: read) so later scripts can’t reuse the checkout token.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 13-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml at line 14, Update the actions/checkout step to
set persist-credentials to false, and scope the workflow or job permissions to
contents: read so subsequent repository-controlled install and test steps cannot
reuse a write-capable checkout token.
Sources: MCP tools, Linters/SAST tools
| # bun cannot instantiate a Memory64 module, so the mem64 build is tested | ||
| # under node instead of in `bun test`. | ||
| # | ||
| # The node version is PINNED rather than taken from the runner image: the | ||
| # ubuntu-24.04 default validates a 64-bit MEMORY but rejects the 64-bit | ||
| # TABLE that emscripten 6.x emits for every wasm64 build, so the module | ||
| # fails to compile with "invalid table elements limits flags". The test's | ||
| # own probe now covers tables too, so an under-capable runtime SKIPS — | ||
| # which means without this pin the gate would quietly stop testing | ||
| # anything at all. | ||
| - name: Setup node | ||
| uses: actions/setup-node@v7 | ||
| with: | ||
| node-version: 24 | ||
|
|
||
| - name: Run MEMORY64 wasm test (node) | ||
| run: node --test test/shiftingMosaicSolver/mem64.node.test.mjs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Workflow excerpt\n'
sed -n '120,170p' .github/workflows/test.yml
printf '\n## Test file outline\n'
ast-grep outline test/shiftingMosaicSolver/mem64.node.test.mjs --view expanded || true
printf '\n## Test file excerpt\n'
cat -n test/shiftingMosaicSolver/mem64.node.test.mjsRepository: Valle12/islands-of-insight-tools
Length of output: 6901
Fail the MEMORY64 gate when wasm64 support is missing
{ skip: !memory64 } lets node --test exit 0 on an unsupported runtime, so this step can turn green without importing or exercising astar.mem64.mjs. Make the CI path fail explicitly and keep the skip only for local runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml around lines 134 - 150, Update the MEMORY64 CI
test invocation around mem64.node.test.mjs so unsupported wasm64 runtimes cause
a nonzero exit instead of silently passing due to the test’s skip condition.
Preserve skipping for local runs by adding an explicit CI-only capability check
or failure path, while ensuring the workflow imports and exercises
astar.mem64.mjs when support is available.
Source: MCP tools
Summary by CodeRabbit
New Features
Bug Fixes
Tests