Static analysis as reports: compiler-checked hot path, clang-tidy, lizard, clang-query, CodeQL - #57
Merged
Conversation
MoonModule's three tick methods now carry MM_NONBLOCKING, so Clang verifies transitively that nothing the render path reaches can block or allocate — the half a regex over the tick body could never see. A new MoonDeck card reports the 181 findings that surfaced, split by tick tier. Five more compiler warnings join -Wall -Wextra -Werror, catching three real defects. KPI: 16384lights | Desktop:856KB | ESP32:1499KB | tick:4231us(FPS:236) | heap:8294KB | lizard:136w **Core** - MM_NONBLOCKING (platform.h): `noexcept [[clang::nonblocking]]` on Clang 20+, bare `noexcept` on GCC. The ESP32 toolchain has neither the attribute nor the warning and builds with -Werror, so a bare attribute there is a build break — same compiler-gated shape as MM_PRINTF_FORMAT. - The attribute is inherited by overrides, so three annotations cover ~90 modules. It also sits in `tickChildren`'s member-POINTER TYPE: without that, the indirect dispatch is a hole every module's tick escapes through. Passing an unannotated method is now a compile error. - Five platform reads (millis, micros, ethLinkUp, ethConnected, wifiStaConnected) annotated: they are register/variable reads, and marking them took the finding count from 209 to 10. Most of the 209 were unannotated helpers, not violations. - Rings241Layout: `1.1` → `1.1f`. The double literal promoted a float on a chip with no FPU — a silent softfloat call in a layout path. - RubiksCubeEffect: `init(uint8_t cubeSize)` shadowed the effect's cubeSize control field; renamed to `order`. **Scripts/MoonDeck** - check_nonblocking.py + the "clang-hotpath" card. Reports unique SITES: a header included by N translation units warns N times, so a raw build prints ~1350 lines for 181 findings. - Split by tier — 70 on tick() (every frame), 6 on tick20ms(), 95 on tick1s(), 10 unresolved — because the same blocking call costs orders of magnitude more in one than another. Clang names the call and the callee but NOT the enclosing function, so the tier is read back from source. - Columns: CALLS / IN / WHY IT BLOCKS / FILE:LINE, aligned with the other reports. WHY comes from clang's own `note:` line. - clang-query card renamed from "AST Rules" so every tools card names its tool. **Tests** - unit_PreviewDriver: CaptureBroadcaster had virtual functions and a public non-virtual destructor. It cannot copy the base's protected-destructor trick (that forbids the stack construction the tests rely on), so a pragma scoped to the one struct carries the reason. **Docs/CI** - Plan-20260727: steps 1 and 3 recorded, net -68 lines. Three sections deleted as superseded by shipped code — the nonblocking probe, the clang-query probe, and the clang-tidy configuration walkthrough, whose reasons live in .clang-tidy itself. - CORRECTION: earlier text claimed check_hotpath.py covers src/platform/esp32/. It does not. Measured: it scans 67 tick METHODS, all in src/core/ and src/light/, zero in the platform layer — which has no tick methods, being free functions the tick path calls into. All 60 of its files compile on desktop, so the compiler check covers the identical set. It reports 0 where the compiler reports 181. - Backlogged: triage the 181, then delete check_hotpath.py (170 lines). The blocker is gate ORDER, not coverage — the script fails pre-commit today while -Wfunction-effects carries -Wno-error, so deleting it first leaves nothing enforcing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…otations Unblocks CI: the clang-only pragma added with the hot-path check failed the GCC sanitizer lanes. Also makes three cross-task connection flags atomic — a real data race, independent of the hot-path work — and removes two MM_NONBLOCKING annotations that were asserting something false. MoonDeck cards gain a 📄 button that replays the script's last run. KPI: 16384lights | Desktop:856KB | ESP32:1499KB | tick:4215us(FPS:237) | heap:8294KB | lizard:136w **Core** - platform_esp32: ethLinkUp_, ethConnected_ and wifiStaConnected_ are written by the IDF event loop and read from the render task every tick. Plain bools there are a data race — benign in practice on this target, but UB the compiler may fold or reorder, and TSan reports it. Now std::atomic<bool> with relaxed ordering: each is an independent state signal, nothing else is published through them. - platform_desktop: `#pragma clang diagnostic` is an UNKNOWN PRAGMA to GCC, and the CI sanitizer lanes build with GCC + -Werror. Guarded with #ifdef __clang__. Verified against the real ESP32 GCC: guarded compiles clean, unguarded reproduces the CI error exactly. **Light domain** - ParallelLedDriver::tick and PreviewDriver::tick keep MM_NONBLOCKING (it is inherited from the base and cannot be dropped without breaking the override) but suppress -Wfunction-effects at the site, with the reason. Both genuinely block: busWaitIfBusy() spins for the DMA peripheral; sendFrame() writes a socket and buildAndSendCoordTable() resizes keptIdx_. Suppressing where the code blocks is honest; letting the annotation claim otherwise was not. tick() findings: 68 → 56. - LedPeripheral::lanesAvailable / busIsRing annotated through their overrides and test doubles — const accessors, no allocation. **Scripts/MoonDeck** - Every run is teed to build/moondeck-logs/<id>.log AS IT STREAMS, not buffered to the end, so a run stopped mid-way still leaves what it printed. A 📄 button per card replays it. Own button rather than a click on the status dot: a dot is a status indicator, and making it secretly clickable gave a new reader no way to guess the feature existed. - check_nonblocking: a FAILED build now errors instead of returning partial output that would read as "nearly clean" — the silent-zero trap again. check_clang_tidy imported directly rather than through another module's attribute. NO_CAUSE named, since a backslash escape inside an f-string expression is a syntax error before Python 3.12. **Tests** - unit_Drivers_rendersplit: SlowDriver::tick's mutex and timed wait ARE the test — they hold the use-after-free window open. Documented so the clang-hotpath report on those lines reads as expected, not as a defect. **Docs/CI** - Backlogged: move blocking work off the render callbacks. 17 sites named with what needs moving where — synchronous HTTP in HueDriver, inline transport in HttpServerModule, applyState() from Layer::tick, effect lifecycle from DemoReelEffect::tick. Each is a design change, not a lint fix. - Explicitly NOT backlogged: the float-math findings. Review suggested fixed-point, but the float trajectory IS the ported MoonLight behaviour and fidelity is deliberate; if the FPU-less Xtensa cost is real that is a profiling question first. - "three orders of magnitude" → "roughly two" in three places: 50/s vs 1/s. **Reviews** - 🐇 CodeRabbit (PR #56): pragma guard, build-failure guard, direct import, f-string escape, plan wording, magnitude overstatement, test-double comment → all done. Atomics and the two annotation removals → done. The architecture findings → backlogged with their specifics. Float-math findings → declined with the fidelity reason above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MM_NONBLOCKING now has a frozen baseline, so the report says what is NEW rather than re-listing 107 known blocking calls. check_hotpath.py (170 lines of regex that could not see past a tick body) is deleted and its gate replaced by the compiler check running incrementally in 0.7s. RealtimeSanitizer joins the sanitizer matrix as the runtime half of the same attribute. KPI: 16384lights | Desktop:856KB | ESP32:1499KB | tick:4164us(FPS:240) | heap:8294KB | lizard:136w **The reports report; consumers decide.** `-Wno-error=function-effects` stays and the gate never fails the event. Failing on ~50 known findings would block every build until the architecture work lands, and a gate nobody can satisfy gets disabled rather than obeyed. A NEW blocking call may also be entirely legitimate — a driver that must wait for hardware — so forcing a failure pushes people to suppress it under time pressure, which is what this exists to prevent. The report states the finding and marks it NEW. **Core** - platform_esp32: wifiStaAssociated_ made atomic. Same cross-task pattern as the three flags in the previous commit — written by the IDF event handler, read by netSetStaticIPv4 — and missed there. - AudioService::kSilence is constexpr, not a function-local static: a static needs a guard variable and a one-time lock on first use, and this is reached from eight audio-reactive effects' tick(). One fix, eight findings. - platform.h: MM_NONBLOCKING's comment corrected — on GCC it expands to `noexcept`, not to nothing. Only the clang attribute and the warning are absent. **Light domain** - ParallelLedDriver::tick and PreviewDriver::tick no longer suppress their findings. They genuinely block (a DMA wait; a socket write plus a resize) and the report exists to show that. Hiding the two worst offenders made the number look better and the report worthless. - LavaLamp/Metaballs constant tables hoisted to class scope: -Wfunction-effects flags ANY static local in a nonblocking function, including a constexpr that needs no guard. - nsToTicks, rmtWs2812Resolution, ethGetIPv4 annotated — stored-value reads and integer math. **Scripts/MoonDeck** - docs/metrics/hotpath-baseline.txt: 107 (file, callee) pairs frozen. Keyed on file+callee, not line, so an entry survives edits above it. - check_nonblocking --incremental (~0.7s) replaces check_hotpath.py in the pre-commit gate: it rebuilds only what the commit touched, answering "did this add a blocking call" rather than re-listing the baseline. Verified by adding a static local to RainbowEffect::tick — reported 1 NEW — then reverting. - FAIL CLOSED on a missing warning: -Wfunction-effects exists only on Clang 20+ and CMake omits it silently otherwise, so "0 findings" would be indistinguishable from a clean tree. The script now says so instead. - Log persistence hardened: capped at 1 MB (an ESP32 build prints megabytes), best-effort writes so a full disk cannot kill the stream, timezone-aware stamps, bounded reads. The 📄 button shows only on cards that have actually run, so its absence marks what nobody has used yet. - .clang-tidy disables clang-diagnostic-function-effects: with MM_NONBLOCKING in the source it double-reported all 165 hot-path findings, burying clang-tidy's own 47. One rule, one owner — the clang-hotpath card owns that rule. **Docs/CI** - test.yml: `realtime` added to the sanitizer matrix. The lane pins clang (GCC rejects -fsanitize=realtime and ubuntu-latest defaults to GCC) and sets RTSAN_OPTIONS=halt_on_error=0 for the same report-don't-gate reason. - Plan steps 3 and 4 marked done, with what the estimates got wrong. - coding-standards: the hot-path lint entry replaced by the compiler check. - MoonDeck.md § Tools: "a report shows the real situation" written down — a finding is fixed, or shown with its reason, never suppressed to tidy output. **Reviews** - 🐇 CodeRabbit (PR #56, second round): wifiStaAssociated_ atomic, log size bound, best-effort writes, tz-aware timestamps, bounded reads, type annotations, check=False, aria-label, serialized log loads, fail-closed contract, GCC-expansion doc fix, analyser→analyzer → all done. Declined with reasons: removing MM_NONBLOCKING from the two blocking ticks and suppressing the SlowDriver fixture (both contradict "reports show the real situation"); per-run generation log files (over-engineering for a last-run record). Deferred: SSE child reaping, Ethernet static-IP snapshot locking, doc de-duplication. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-one copies of the same "nothing to draw" check are gone from the effects and replaced by one in Layer::tick, which already owns the enabled/role/extrude gates. A dead channelsPerLight guard is deleted outright. clang-tidy is complete as a report, and the lizard baseline turns out to pin far less than it claimed. KPI: 16384lights | Desktop:856KB | ESP32:1498KB | tick:7258us(FPS:137) | heap:8277KB | lizard:136w **Orchestration belongs to the caller.** An effect answering "is there a grid at all?" is answering a question about the pipeline, not about itself. Layer::tick already decides which children run and when; the grid check is the same kind of decision, so it lives there once rather than in every effect an author might write next. Effects may now assume width/height/depth >= 1. The guard gates only the effect loop, not the whole tick: the modifier pass below advances per-frame state (RandomMap accumulates a bpm phase off elapsed()), and skipping it during a zero-grid interval would leave that phase stale and the chain in the wrong place when the grid returns. **Light domain** - Layer::tick holds the single degenerate-grid gate; 21 guards removed across 20 effects (w/h/d, and the cols/rows and sizeX/sizeY aliases of the same thing). - channelsPerLight() < 3 deleted from 15 effects as dead code. The member defaults to 3, the only setter is called with 3 everywhere in src, tests and scenario JSON, and the sole non-3 value in the tree is 4 (RGBW). There is no control, no persistence key and no HTTP write path that can lower it. - MetaballsEffect carried a comment describing a guard that was not there; FreqMatrixEffect kept half of one (len but not cols). Both removed — the rule has one home now, and half a guard reads as "this axis is special". **Core** - platform_esp32: wifiStaStop() now clears wifiStaAssociated_. A stale true from a torn-down STA would let netSetStaticIPv4 apply a static IP to nothing. **Tests** - The 0x0x0 row of the effect sweep drove Layer::tick, which now returns before reaching any effect — so it was testing the new guard 39 times instead of the 39 effects. It calls the effect directly on the degenerate grid, which is what proves the removals safe rather than assuming it. All 39 survive. **Scripts/MoonDeck** - clang-tidy reports 30 findings (was 47; a vendored-header filter and zeroed uint8_t out[4] account for most of the drop). WarningsAsErrors stays empty by design, and .clang-tidy, the plan and the backlog now say so instead of promising a ratchet: a report states what it finds and consumers decide. - check_nonblocking distinguishes a missing baseline from an empty one. Empty means "nothing is known-good", so everything is new; only a missing file means no baseline was ever taken. - CI passes the realtime lane its compiler via -DCMAKE_CXX_COMPILER=clang++-20 rather than exporting CXX. An empty CXX="" on the other lanes is not the same as unset, and some toolchain probes read it as a broken compiler path. UNVERIFIED: this only exercises on push. **Docs** - New backlog card: lizard measures 94 of 2404 functions under a mis-parsed name. Its C++ parser loses the name on some bodies and falls back to the first keyword inside, so SolidEffect::tick is measured as SolidEffect::static_cast<lengthType>. Because whitelizard.txt matches by name, 35 of 162 baseline entries pin nothing, and the check reports FAIL - 9 NEW on an unmodified tree. Re-baselining does not fix it: it would freeze today's fallback names, which shift again on the next edit inside the body. - FreqSawsEffect's baseline entry re-keyed to the name lizard now emits. Its complexity fell from CCN 24 to 13; only the key changed, not the standard. **Reviews** - Reviewer (pre-commit, 38 files): 3 findings, all fixed above — the vacuous 0x0x0 sweep row, the Metaballs comment, the FreqMatrix half-guard. It independently confirmed the channelsPerLight deletions and the modifier-pass reasoning. Gates: 9 passed, 0 failed, 3 skipped (device-model catalog, firmware list and JS tests are untriggered by this diff). Desktop 0 warnings, ESP32 S3 0 warnings, 872 unit tests, 19 scenarios. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Three sanitizer lanes were failing: two on a test line from the previous commit that called effects on a zero grid, one on a clang-20 diagnostic GCC does not emit. Desktop config files (WiFi PSKs, MQTT passwords) now land 0600 instead of 0644, and CodeQL stops re-reporting vendored code. Plan-20260727 is complete. KPI: 16384lights | Desktop:856KB | ESP32:1498KB | tick:7382us(FPS:135) | heap:8271KB | lizard:136w **The zero-grid contract runs one way.** The previous commit moved the degenerate-grid guard into Layer::tick, which left the sweep's 0x0x0 row testing the Layer 39 times instead of the 39 effects. The fix I reached for — calling fx->tick() directly on a zero grid — asserts a contract no effect makes, and it crashed: GEQ3DEffect divides by width() via imap over a zero range, so ASan took SIGFPE and TSan caught doctest's signal handler allocating. Same one line, two lanes. Effects MAY assume width/height/depth >= 1; that is the whole point of the single Layer gate. The 0x0x0 row pins the Layer's behaviour (clean no-op, zero-byte buffer) and the three non-degenerate rows cover effect bodies, where they run. The header now says so and names GEQ3D, so the next person does not re-add it. **Tests** - unit_Effects_gridsweep: direct-tick call removed, header corrected. - mm_tests gets -Wno-error=double-promotion. doctest::Approx takes a double, so every CHECK against a float literal promotes inside doctest.h, which is vendored. -Wdouble-promotion stays -Werror for the firmware — the Xtensa has no FPU, so a stray promotion is a silent softfloat call in the render path — it just cannot also police a third-party header. Test target only, error only. **Core** - platform_desktop: one openTempOwnerOnly helper behind both atomic-write paths. std::fopen creates 0666 & ~umask (0644 in practice) and these are /.config/*.json holding WiFi PSKs and MQTT passwords. POSIX gets O_CREAT|O_EXCL with an explicit 0600; Windows keeps plain fopen (no mode_t — files inherit the parent directory ACL, which is that platform's answer to the same question). Verified on disk and end-to-end: a live control change against the running desktop binary rewrote SystemModule.json as -rw-------. **Docs/CI** - .github/codeql-config.yml excludes test/doctest.h, the only vendored source in the repo and the source of the one critical we do not own. paths-ignore is the standard mechanism rather than dismissing the same alert after every scan. - CodeQL stays a sweep, never a PR gate, and the workflow now records that as settled rather than provisional. Same rule as clang-tidy and the hot-path check: a report states what it finds and consumers decide. - Plan-20260727 steps 6 and 7 marked done. Step 7's "file modes 0666 -> 0600" was never a literal 0666 — nothing in the tree ever had one; it was fopen's default creation mode, which is why grepping for it found nothing. Open alerts are 0. **Branch** - Renamed next -> next-iteration, local and remote. The CodeQL workflow triggers on next-iteration, so with the branch named next it had silently never run on a push — the last scan was two commits stale. Deleting the old remote branch closed PR #56; recreated as #57 with the same commits and content. Gates: all green, 8 changed files. Desktop 0 warnings, ESP32 S3 0 warnings, 872 unit tests, 19 scenarios, full ASan suite clean locally (the lane that failed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
…review Plan-20260727 is complete and deleted; its durable content now lives in testing.md, lessons.md and the backlog, in present tense. The pre-merge Reviewer found six things — one real bug, one backlog card asking for shipped work, and four cases of the subtraction pass this branch skipped. KPI: 16384lights | Desktop:856KB | ESP32:1498KB | tick:7382us(FPS:135) | heap:8271KB | lizard:136w **A deleted file is not a finished subtraction.** Removing check_hotpath.py and the plan left three references pointing at them, including a CMakeLists comment claiming the ESP32 build still runs the deleted script and a -Wno-error whose only stated removal criterion was "once check_hotpath reports zero". Both read as live instructions. Deleting the thing is half the job; the other half is every place that mentioned it. **Light domain** - ParticlesEffect: initParticles() is called from prepare(), NOT tick(), so Layer::tick's degenerate-grid gate does not cover it — its guard was removed with the 21 that the gate does cover. It is safe today only because resize(0) frees trail_, which is a different guard doing the work by accident. That is now stated at the site: without it, every particle seeds from (rand8() * w) >> 4 with w == 0. **Core** - platform_esp32: the last raw read of a converted atomic made explicit. It sat in netSetStaticIPv4, next to an explicit store — the one place the diff broke its own convention. **Docs** - Plan-20260727 deleted. testing.md gains a Static analysis section (the five layers, the one-rule-one-owner table, report-not-gate, and Verify a zero before believing it with the seven silent-failure modes); lessons.md gains the /code-scanning/alerts non-default-branch trap; the backlog keeps the lizard mis-parse card. testing.md had nothing on static analysis before this. - Removed the clang-query backlog card added earlier this branch: everything it asked for — constexpr exclusion, no size threshold, the name_[16] false positive — already shipped and is documented on the card. A backlog item that requests built work is worse than none; it gets picked up and redone. - De-duplicated what the plan move scattered. "Report, not a gate" had four hand-written homes and "verify a zero" two; each now states the rule once and the rest reference it. The CodeQL workflow header described an unanswered POC, which stopped being true when it found and fixed three criticals. - Dropped two wrong cached counts rather than re-caching them: .clang-tidy said 47 clang-tidy findings (30) and test.yml said ~50 blocking calls (the baseline file has 107). Both now name the file instead of a number. - CLAUDE.md § Commit: "commit now" applies to the diff the PO just reviewed, and any later edit cancels it. Written down because I got this wrong twice — "we commit in one go" answers how MANY commits, not WHEN. **Reviews** - Reviewer (pre-merge, 126 files): 6 findings, all fixed above. It confirmed the channelsPerLight deletions are genuinely dead code, that Layer::tick's gate placement is right, and that the hot path gained no per-frame cost. Not done, deliberately: performance.md is unchanged (its numbers are hardware-measured), and the permission list is pruned 98 -> 71 locally but NOT snapshotted — it has doubled against the approved 34 and the additions need PO review, not an agent's. Gates: 7 mechanical passed, 0 failed. Desktop 0 warnings, ESP32 S3 0 warnings, 872 unit tests, 19 scenarios. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
…deQL trigger Four findings from the branch review, three of them this branch's own rules failing on their last application: a de-duplication link pointing at the wrong section, a "report not a gate" copy that survived the pass that collapsed it, and a CodeQL trigger that would have gone silent the moment this branch merges. KPI: 16384lights | Desktop:856KB | ESP32:1498KB | tick:7382us(FPS:135) | heap:8271KB | lizard:136w **A reference to the wrong home is worse than the duplication it replaced.** MoonDeck.md pointed at coding-standards.md#tests for the -Wfunction-effects transitivity rule, which actually lives under ## Static checks. The anchor resolves to a real heading, so nothing looks broken — the reader just lands on test-file placement and concludes the rule is not written down. Duplication is at least visible; a confidently wrong pointer is not. **Docs/CI** - codeql.yml: the push trigger now lists `main` as well as `next-iteration`. It named only this branch, so once this merges the job would fire on a branch nobody pushes to and report nothing while looking healthy — the exact silent-idle failure lessons.md was written to record two commits ago, and the second time this branch has hit it. - codeql.yml: the POC framing is gone from the trigger comment too. The top header was rewritten last commit to say the POC is over; the block ten lines below still called this "the POC branch" and said the workflow lives here "until the POC is judged". Same edit, half applied. - codeql.yml: dropped the second full-length copy of the report-not-gate argument. The same file both referenced the single home and restated it. - testing.md: "A analyser" -> "An analyser", first line of a new section on a published docs site. **Scripts/MoonDeck** - check_clang_query.py records the three AST constraints the array rule was built around, two of which are NEGATIVE results and had no home once the plan was deleted: there is no size-threshold matcher (hasSize is exact-match, sizeGreaterThan does not exist); varDecl alone silently misses every class member and needs fieldDecl beside it; and "fixed number vs named constant" is not recoverable, because the AST folds kMaxLanes to [16] before a matcher sees it. A negative result with no home is one somebody re-derives. **Reviews** - Reviewer (pre-merge, 130 files): verified all six findings from the previous pass as completely fixed (it re-audited all 34 atomic uses and found no raw reads remaining), then raised these four. It also confirmed no hot-path cost was added and that the plan deletion lost nothing else. Gates: 7 mechanical passed, 0 failed. CI green on the pushed commit including all three sanitizer lanes and CodeQL. ESP32 S3 rebuilt (platform.h changed after the last build), 0 warnings. Still open for the PO, deliberately not done here: CodeRabbit skipped PR #57 because auto-review is disabled repo-wide by config, so external review has not run on this PR; the permission list is pruned 98 -> 71 but not snapshotted; the PR description still describes only the first commit; performance.md is unchanged since its numbers are hardware-measured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ewowi
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this lands
A static-analysis stack where every layer is a report, not a gate, plus the refactor and fixes that came out of using it.
Compiler-checked hot-path discipline.
MoonModule::tick/tick20ms/tick1scarryMM_NONBLOCKING, and Clang verifies under-Wfunction-effectsthat nothing the render path reaches can block or allocate — transitively, through the whole call graph. That is the half a regex over a tick body cannot see: it reads the text and is blind to what its callees do. The attribute is inherited by overrides, so three annotations cover ~90 modules, and it sits intickChildren's member-pointer type — without that, indirect dispatch is a hole every module's tick escapes through.check_hotpath.py(170 lines of regex) is deleted; its gate is now the compiler check running incrementally in 0.7s.The stack, documented in docs/testing.md § Static analysis: compiler warnings → clang-tidy → sanitizers → RTSan +
[[clang::nonblocking]]→ CodeQL, with lizard counting complexity for the trend and clang-query as the home for bespoke AST rules. Each has a MoonDeck card. RealtimeSanitizer joins the sanitizer matrix as the runtime half of the same attribute.Why nothing gates.
WarningsAsErrorsis empty, CodeQL never runs onpull_request, the hot-path check never fails the event. A gate nobody can satisfy gets disabled rather than obeyed, and it pushes people to suppress a finding under time pressure — the opposite of why the tool is there. Reports state what they find; consumers decide. The exception is compiler warnings, which are-Werrorbecause they are few and fixed on sight.Orchestration moved to the caller. 21 copies of
if (w <= 0 || h <= 0) return;are gone from 20 effects, replaced by one gate inLayer::tick— which already owns the enabled/role/extrude/timing decisions. Effects may now assumewidth/height/depth >= 1. The gate covers only the effect loop: the modifier pass below advances per-frame state (RandomMap's bpm phase) that must keep running so the chain is in the right phase when the grid returns. A deadchannelsPerLight() < 3guard came out of 15 more effects — the member defaults to 3, is never set below it anywhere insrc/, tests or scenario JSON, and has no control, persistence key or HTTP write path.Fixes found along the way
wifiStaStop()did not clearwifiStaAssociated_; a staletruewould apply a static IP to a torn-down interface. Four cross-task flags made atomic./.config/*.json— WiFi PSKs, MQTT passwords) were created0644. OneopenTempOwnerOnlyhelper now creates them0600on POSIX; Windows keepsfopen, where files inherit the parent ACL. Verified on disk and end-to-end against the running binary.<numbers>included inside a POSIX-only guard) and a GCC break (unguarded clang pragma under-Werror).Reviewed
The Reviewer agent ran twice over the branch diff. Ten findings, all fixed, including a real bug:
ParticlesEffect::initParticlesruns fromprepare(), nottick(), soLayer::tick's gate never covered it — it is safe only becausetrail_is empty on a zero grid, which is now stated at the site.Three findings were this branch's own rules failing on their last application: a de-duplication link pointing at the wrong section (it resolved to a real heading, so it looked fine while landing readers somewhere else), a "report not a gate" copy that survived the pass meant to collapse it, and a CodeQL trigger naming only
next-iteration— which would have gone silent the moment this branch merged and was retired.External review (CodeRabbit) was skipped deliberately — auto-review is disabled repo-wide in
.coderabbit.yaml, and the PO chose to merge on the Reviewer passes plus green CI rather than take the round trip.Verification
131 files, +1318/−889. All 8 PR checks pass, including the three sanitizer lanes and CodeQL. 7 pre-merge mechanical gates pass. Desktop 0 warnings, ESP32-S3 0 warnings, 872 unit tests, 19 scenarios.
Measured, not assumed: the guard refactor was A/B-built against its parent — desktop scenario ticks 129/130/131µs before, 127/130/131µs after. The KPI line's ESP32 tick moved on this branch, but that number is read from a live bench device, so it reflects the attached board rather than this code.
Known and recorded, not fixed
SolidEffect::tickis measured asSolidEffect::static_cast<lengthType>. Becausewhitelizard.txtmatches by name, 35 of 162 baseline entries pin nothing, and the check reportsFAIL — 9 NEWon an unmodified tree. Backlogged with the measured numbers; re-baselining does not fix it.unchecked-string-to-number-conversionsites were each read and confirmed safe.performance.mdis unchanged — its numbers are hardware-measured.Plan-20260727is complete and deleted; its durable content lives intesting.md,lessons.md, the backlog, and — for the negative results — incheck_clang_query.pybeside the matcher they constrain.🤖 Generated with Claude Code