Repository navigation
Fail visibly on a full module list, and hash any size with SHA-1 - #164
Merged
Merged
Conversation
A boot that registers a 33rd top-level module now logs it and gets false back instead of losing the module silently, and SHA-1 hashes input of any length instead of silently skipping the digest past 503 bytes. KPI: 256lights | Desktop:2221KB | tick:7/10/7/1/7/7/163/7/7/8us(FPS:142857/100000/142857/1000000/142857/142857/6134/142857/142857/125000) | src:345(88766) | test:261(61559) | functions-over:345 Commit: src +64/-54 lines | test +69/-0 lines | duplicated lines 3268 -> 3268 (+0) **Core** - Scheduler::addModule returns bool: false, with a "no room" log line, when the 32 top-level slots are full: the bool `addChild` returns and the log line `Control` writes. - sha1 streams whole 64-byte blocks from the input through `detail::sha1Block`, the FIPS 180-4 compression step, and pads only the tail: no size limit, and stack use falls from 832 to 448 bytes. - Scheduler.h and Sha1.h leave the docgen warning list: `@xref`s from `tick`, `prepareTree` and `setControl` to the sections they explain, and Sha1's appendix one line per paragraph, said once. **Tests** - unit_Scheduler_capacity: the 33rd add is refused and the first 32 stay in order. - unit_sha1: the FIPS 180-4 vectors, the 55, 56 and 64-byte padding boundaries, and one million 'a'. - build_desktop: PASS, no warnings. check_code: PASS, counts equal to the committed ones. test_desktop: PASS, 2,848 test cases, 179,557 assertions. - check_esp32_built --firmware esp32s3-n16r8: PASS, rebuilt with these sources. - check_specs: PASS. check_docgen: PASS, 0 errors, warnings 2050 → 2045. - check_prose: NegatedHeading 6→8 and SelfReference 17→18 against the committed counts with no change of ours, the same rise in the other branch, which changes no prose (Vale 3.25.1); left for the maintainer. **Reviews** - Fable review: the KPI line and triggered checks listed, done; "512 bytes" corrected to 503, done; `<cstdio>` included, done; all 32 slots checked, done; the WebSocket-only sentence said once, done. - Fable review, round two: `addChild` returns a bool and logs nothing, so the message names each precedent, done; a negative clause in Sha1.h cut, done; a public constant for the 32-slot limit, declined: it grows the API for one test, and the test documents the limit. - check_code: the block lambda counted into `sha1` and raised complex function 293→294, so the FIPS 180-4 compression step is its own function, `detail::sha1Block`, done. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
There was a problem hiding this comment.
🟡 Changes recommended
The capacity-rejection test never asserts the diagnostic, so it would still pass if the visible failure signal for boot registrations that ignore the return value were removed.
1 open issue
What changed in this PR
This PR removes two silent-failure paths in MoonLight’s core: scheduler capacity overflow and SHA-1 inputs exceeding the fixed buffer.
Changes:
- Make
Scheduler::addModulereturn success or failure and log capacity rejection. - Process SHA-1 in blocks with tail-only padding, removing the 503-byte input limit and reducing stack usage.
- Add capacity and SHA-1 regression tests, register them in the desktop suite, and refine header documentation.
| File | Description |
|---|---|
test/unit/core/unit_sha1.cpp |
Adds known-digest, padding-boundary, and million-byte tests. |
test/unit/core/unit_Scheduler_capacity.cpp |
Tests capacity rejection, registration order, and null input. |
test/CMakeLists.txt |
Registers both new test files. |
src/core/system/Sha1.h |
Replaces fixed-buffer hashing with block processing. |
src/core/module/Scheduler.h |
Declares the boolean result and adds documentation cross-references. |
src/core/module/Scheduler.cpp |
Returns registration status and logs capacity overflow. |
Review effort: Balanced
The boot ignores addModule's answer, so the printed line is what makes a full list visible, and the test now reads it. **Tests** - unit_Scheduler_capacity: stdout is captured around the refused add alone, and the test checks the line names the module that had no room and the limit; removing the printf now fails it. The capture uses dup and dup2, _dup and _dup2 under MSVC. **Reviews** - Copilot: the capacity test never asserted the diagnostic, so removing the printf left it passing; done, the test reads the printed line.
Collaborator
|
Merged, thanks James! Both fixes are right and well pinned; the FIPS vectors with the boundary lengths are exactly the tests a hash wants. In our next commit Sha1.h gets back one line on why it exists (the WebSocket accept key, RFC 6455, is its only use), since a reader meeting a broken hash wants that reason. |
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
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.
Two places that fail silently, which CLAUDE.md's robustness principle asks to fail visibly:
Scheduler::addModuledropped a 33rd top-level module with no log line. It now returns false and logs "no room", the booladdChildreturns and the log lineControlwrites.sha1padded into a fixed 512-byte stack buffer and returned without writing the digest for input over 503 bytes. It now streams 64-byte blocks and pads only the tail (FIPS 180-4), so any length hashes, with less stack than before. Today only the WebSocket accept key uses it, which is short, so this removes a trap rather than fixing a live failure.Both are pinned: a capacity test, and the FIPS 180-4 vectors with the block-boundary lengths and one million 'a'. Scheduler.h and Sha1.h leave the docgen warning list, since the change touched them.
What an agent wrote: Claude found both and drafted the code and tests; I reviewed every line.
Tested: build_desktop --tests and test_desktop, check_code, check_docgen, check_specs, and the esp32s3-n16r8 firmware build. Not flashed: neither path is platform-specific. check_prose reports a rise in two rules against the committed counts that this branch does not cause (details in the commit).