Enhancements and cleanup#10
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (34)
📝 WalkthroughWalkthroughThis PR migrates the AtomVM support database to a per-platform ChangesSpectrometer core changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/spectrometer_updater_tests.erl (1)
271-346: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
write_db_file_test_was not updated for the new 3-tuple schema — will crash at runtime.
write_db_file/2now expects accumulator values shaped as{Platforms, SinceMap, Removed}(seespectrometer_updater.erl,fun({M, F, A}, {_Platforms, SinceMap, Removed}, MAcc) -> ...). This test still buildsAccwith 2-tuple values (e.g.{all, {unreleased, <<"main">>}}), which cannot match that 3-element pattern and will raise afunction_clauseerror as soon asmaps:foldinvokes the fun on any entry.Additionally, the on-disk assertions checking
{F, A, P, _S}whereP =:= all orelse is_list(P)are validating the oldPlatformsshape; the written file's third element is now aSinceMap(a map), notPlatforms.🐛 Proposed fix
?_test(begin Acc = #{ - {lists, map, 2} => {all, {unreleased, <<"main">>}}, - {io, format, 2} => {all, {unreleased, <<"main">>}} + {lists, map, 2} => + {all, #{all => {unreleased, <<"main">>}}, undefined}, + {io, format, 2} => + {all, #{all => {unreleased, <<"main">>}}, undefined} }, Path = filename:join(Dir, "test.data"), ok = spectrometer_updater:write_db_file(Path, Acc), ?assert(filelib:is_file(Path)), %% Verify it can be read back {ok, [Data]} = file:consult(Path), ?assert(is_list(Data)) end) end, fun(Dir) -> ?_test(begin Acc = #{ - {lists, map, 2} => {all, {unreleased, <<"main">>}}, - {esp32_module, func, 1} => { - [esp32], {unreleased, <<"main">>} - } + {lists, map, 2} => + {all, #{all => {unreleased, <<"main">>}}, undefined}, + {esp32_module, func, 1} => + {[esp32], #{esp32 => {unreleased, <<"main">>}}, undefined} }, Path = filename:join(Dir, "test_platforms.data"), ok = spectrometer_updater:write_db_file(Path, Acc), {ok, [Data]} = file:consult(Path), ?assert(is_list(Data)), - %% Each entry should be {Module, [{Func, Arity, Platforms, Since}]} + %% Each entry should be {Module, [{Func, Arity, SinceMap, Removed}]} lists:foreach( fun({Mod, Funs}) -> ?assert(is_atom(Mod)), ?assert(is_list(Funs)), lists:foreach( - fun({F, A, P, _S}) -> + fun({F, A, SM, _Removed}) -> ?assert(is_atom(F)), ?assert(is_integer(A)), - ?assert(P =:= all orelse is_list(P)) + ?assert(is_map(SM)) end, Funs ) end, Data ) end) end, fun(Dir) -> ?_test(begin Acc = #{ - {module1, func1, 1} => {all, {unreleased, <<"main">>}}, - {module2, func2, 2} => { - [esp32, rp2], {unreleased, <<"main">>} - } + {module1, func1, 1} => + {all, #{all => {unreleased, <<"main">>}}, undefined}, + {module2, func2, 2} => + {[esp32, rp2], + #{esp32 => {unreleased, <<"main">>}, + rp2 => {unreleased, <<"main">>}}, undefined} }, Path = filename:join(Dir, "roundtrip.data"), ok = spectrometer_updater:write_db_file(Path, Acc), %% Read back and verify {ok, [Data]} = file:consult(Path), FlatList = [ - {M, F, A, P} - || {M, Funs} <- Data, {F, A, P, _S} <- Funs + {M, F, A, SM} + || {M, Funs} <- Data, {F, A, SM, _Removed} <- Funs ], ?assert(length(FlatList) =:= 2) end) end🤖 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/spectrometer_updater_tests.erl` around lines 271 - 346, `write_db_file_test_` is still using the old accumulator/value shape, so update the test data to match the new 3-tuple schema expected by `spectrometer_updater:write_db_file/2` and its `maps:fold` callback. Replace each `{Platforms, Since}` value in the `Acc` maps with `{Platforms, SinceMap, Removed}`-style entries that the updater now consumes, and adjust the read-back assertions to validate the emitted `{F, A, SinceMap, _Removed}` structure instead of treating the third field as `Platforms`. Use the `write_db_file_test_`, `write_db_file/2`, and `file:consult/1` checks to locate and update all affected cases.
🧹 Nitpick comments (10)
test/spectrometer_ecosystem_tests.erl (1)
75-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigration test exercises a local copy, not the production decoder.
load_state_migration/1duplicates thespectrometer_v1/spectrometer_v0_r2tuple handling fromspectrometer_ecosystem:load_state/0. The test therefore validates this helper rather than the real function, so a regression inload_state/0's actual decoding (e.g. thepackage_map_validgate or the v1 branch) would not be caught, and the two implementations can silently diverge. Consider writing the tuple to a temp state file and asserting onspectrometer_ecosystem:load_state/0directly (the coordinator'ssave_stateuses?ECOSYSTEM_STATEundercache_dir, which you can override viaapplication:set_env(spectrometer, cache_dir, ...)).🤖 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/spectrometer_ecosystem_tests.erl` around lines 75 - 107, The migration test is duplicating the tuple-decoding logic instead of exercising the real `spectrometer_ecosystem:load_state/0` path, so it can miss regressions in the production decoder. Replace the local `load_state_migration/1` helper usage by writing a test state file through the same `?ECOSYSTEM_STATE` location used by `save_state`, override `cache_dir` with `application:set_env(spectrometer, cache_dir, ...)`, and assert directly on `spectrometer_ecosystem:load_state/0` for both the `spectrometer_v1` and `spectrometer_v0_r2` cases. Keep the test focused on the actual `load_state/0` branch behavior, including the `package_map_valid` gate and default fallback.src/spectrometer_ecosystem_worker.erl (1)
20-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
next_worker_idis effectively dead state. It's assigned once ininit/1fromstart_node/2and never read again (each replacement is a freshsimple_one_for_onechild that re-derives its id from the ETS counter). Consider dropping the field to reduce noise.🤖 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/spectrometer_ecosystem_worker.erl` around lines 20 - 45, The worker_state record currently carries next_worker_id, but it is only initialized in init/1 and never read afterward. Remove next_worker_id from the worker_state definition and from the state construction in init/1, and keep the start_node/2 return handling focused on node, port, and cookie only.test/atomvm_spectrometer_tests.erl (1)
14-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
coord_staterecord risks silent drift. This mirror ofspectrometer_ecosystem_coordinator's record must be kept in sync by hand; a field reorder/rename in the source will compile-mismatch or silently misalign these tests. Consider extracting the record into a shared.hrlincluded 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 `@test/atomvm_spectrometer_tests.erl` around lines 14 - 36, The duplicated coord_state record in the test module can drift from spectrometer_ecosystem_coordinator and cause the tests to become stale or misaligned. Move the record definition into a shared .hrl include used by both the coordinator and test module, and replace the local mirror in the test file with that shared definition so coord_state stays identical everywhere.src/spectrometer_atomvm.erl (3)
14-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModuledoc still describes the old flat DB format.
The module doc still documents the format as
[{module(), [{function(), arity(), platforms(), since()}]}], but the code now reads/writes the newsince_map/removedschema (seewrite_db_file's header inspectrometer_updater.erl, which was updated). Worth updating this doc block to avoid misleading future readers/users of the bundled DB format.🤖 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/spectrometer_atomvm.erl` around lines 14 - 47, The moduledoc in spectrometer_atomvm still documents the old flat supported_functions.data shape, so update that description to match the current since_map/removed schema used by the database and writer logic. Keep the existing module overview, but revise the format examples and override text so they reference the new structure consistently with write_db_file in spectrometer_updater and the data loading/checking functions in spectrometer_atomvm.
735-746: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
string:join/2is obsolete; preferlists:join/2.Per the OTP
stringmodule docs,join/2is obsolete. Use lists:join/2.lists:join/2is already used correctly elsewhere in this PR (e.g.find_exports/1inspectrometer_updater.erl).♻️ Proposed fix
- lists:flatten(string:join(Pairs, ", ")). + lists:flatten(lists:join(", ", Pairs)).🤖 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/spectrometer_atomvm.erl` around lines 735 - 746, The `format_platform_versions/1` helper is using the obsolete `string:join/2`; replace that join call with `lists:join/2` and keep the surrounding `lists:map/2` and `lists:flatten/1` flow intact. Use the existing `format_platform_versions/1` function as the target for the change, matching the `lists:join/2` pattern already used elsewhere in the PR.
510-524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing
begin ... endfilter hack to renameSMtoPlatforms.begin Platforms = SM, true endThis binds an extra variable purely so the resulting tuple can use the name
Platforms, but the value is actually asince_map(), not a platform list — the name is misleading. Simplify by usingSMdirectly in the produced tuple.♻️ Proposed simplification
Matches = [ - {Arity, Platforms, Removed} - || {M, `#function`{ - name = Fn, arity = Arity, since_map = SM, removed = Removed - }} <- Supported, - M =:= Mod, - Fn =:= Fun, - begin - Platforms = SM, - true - end + {Arity, SM, Removed} + || {M, `#function`{ + name = Fn, arity = Arity, since_map = SM, removed = Removed + }} <- Supported, + M =:= Mod, + Fn =:= Fun ],🤖 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/spectrometer_atomvm.erl` around lines 510 - 524, The `show_query/1` list comprehension uses a misleading `begin ... end` block to rename `SM` to `Platforms`, even though the value is still the `since_map()` from `#function{}`. Remove that hack in `show_query/1` and use `SM` directly in the tuple being built, keeping the match/filter logic around `get_supported_functions()` and the `#function{ since_map = SM }` pattern unchanged.src/spectrometer_updater.erl (1)
384-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
scan_atomvm_repo/3return type spec doesn't match its actual (2-tuple) return values.The
-specdeclares the map values asentry(), i.e.{platforms(), since_map(), removed()}(3-tuple), but the implementation and all downstream consumers treat the returned map's values as 2-tuples{platforms(), since_map()}:
update_datafile/2destructuresNewAccvalues via{EP, ESM} = NewEntry(Line 337).merge_entry/2is spec'd and used only with 2-tuples.- The (unchanged)
scan_repo_test_assertsmaps:get({erlang, abs, 1}, Acc)equals the 2-tuple{all, {unreleased, <<"main">>}}.The
Removedfield is only added later, inupdate_datafile/2's merge fold. Consider introducing a distinct type (e.g.scan_entry() :: {platforms(), since_map()}) for the scan-stage accumulator instead of reusingentry(), and update the stale doc text at Line 389 ({Platforms, Since}) accordingly.🤖 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/spectrometer_updater.erl` around lines 384 - 399, The return contract for scan_atomvm_repo/3 is stale: it is typed and documented as if it returns entry() values, but the scan stage actually produces only 2-tuples {platforms(), since_map()}. Update the spec and docstring in scan_atomvm_repo/3 to use a dedicated scan-stage type (for example, scan_entry()) instead of entry(), and keep it consistent with update_datafile/2, merge_entry/2, and the existing scan_repo_test_ assertions that expect 2-tuples.src/spectrometer_scanner.erl (2)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicated/leftover debug artifacts.
Lines 61-62 log the same thing twice (
scan_directory enteredandscan_directory FUNCTION ENTERED), followed by a third entry log at 63-65. These look like debugging leftovers; keep at most the informative one.♻️ Proposed cleanup
- ?LOG_DEBUG("Scanner: scan_directory entered", []), - ?LOG_DEBUG("Scanner: scan_directory FUNCTION ENTERED", []), ?LOG_DEBUG("Scanner: scan_directory called for ~p with ~p include paths", [ Dir, IncludePaths ]),🤖 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/spectrometer_scanner.erl` around lines 61 - 65, Remove the duplicated debug artifacts in scan_directory: the two entry logs in spectrometer_scanner should be reduced to a single useful message. Keep the informative ?LOG_DEBUG call in scan_directory that includes Dir and IncludePaths, and delete the redundant "scan_directory entered" and "scan_directory FUNCTION ENTERED" logs so the function only logs once on entry.
135-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid logging the full parse result / per-call
code:which.Line 138 logs the entire
epp_dodgerResult(all parsed forms), which can be very large for real modules, and line 135 runscode:which(epp_dodger)on every file. Even guarded by?LOG_DEBUG, this is noisy; prefer logging just an outcome tag (e.g.ok/error) or the form count.🤖 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/spectrometer_scanner.erl` around lines 135 - 138, The scanner logging in spectrometer_scanner:parse_file should avoid expensive and noisy debug output: stop calling code:which(epp_dodger) for every file and remove the full epp_dodger parse Result from the debug log. Update the logging around the parse_file/Result handling to emit only a lightweight outcome such as ok/error or the number of parsed forms, and keep any availability check for epp_dodger outside the per-file hot path.src/spectrometer_otp.erl (1)
101-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop trailing
\nin logger format strings.Logger handlers add their own line termination, so the trailing
\n(lines 102 and 109) produces an extra blank line. The other converted?LOG_WARNINGcalls in this module correctly omit it.♻️ Proposed cleanup
- ?LOG_WARNING( - "Unable to write to otp module data file ~s, reason: ~p\n", + ?LOG_WARNING( + "Unable to write to otp module data file ~s, reason: ~p", [ModFile, Reason] ),- ?LOG_WARNING( - "Unable to create cache dir for OTP module data ~s, reason: ~p\n", + ?LOG_WARNING( + "Unable to create cache dir for OTP module data ~s, reason: ~p", [ModFile, Reason] ),🤖 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/spectrometer_otp.erl` around lines 101 - 110, The ?LOG_WARNING calls in spectrometer_otp should not include trailing newline characters in their format strings because the logger already terminates messages. Update the warning logs in the write-failure and cache-dir creation branches to omit the final “\n”, matching the other ?LOG_WARNING usages in this module and keeping the behavior consistent.
🤖 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 `@rebar.config.script`:
- Around line 115-116: The license file path in the rebar config is using the
wrong casing, which will break on case-sensitive filesystems. Update the license
entry in the config list to match the existing LICENSES/Apache-2.0.txt path
exactly, and keep the surrounding file list unchanged.
In `@src/atomvm_spectrometer.erl`:
- Around line 96-97: The unsupported-count path in atomvm_spectrometer should
not pass raw values from maybe_halt/1 directly to halt/1 because counts above
255 can wrap to success. Update maybe_halt/1 (and its Unsupported branch call
site) to clamp the exit status to 255 or use a fixed non-zero exit code while
preserving the exact unsupported count in the output.
In `@src/spectrometer_analyzer.erl`:
- Around line 145-147: The unique function call count in
spectrometer_analyzer:scan_target/1 is being read from a non-existent
total_unique key, so it always logs 0. Update the ?LOG_INFO call to use the
actual map size of Stats, consistent with the later check in the same module,
and keep merge_stats/2 and the Stats map structure unchanged.
In `@src/spectrometer_atomvm.erl`:
- Around line 331-373: ensure_compat_version/1 is dropping the special
unreleased main branch and falling back to {release, 0, 0}. Update the branch
parsing in ensure_compat_version/1 to recognize <<"main">> and return the
documented {main, Major, Minor} form (or the equivalent unreleased main tuple
expected by format_version_tuple/1), while keeping the existing Maj.x and
Maj.Min.x compatibility cases intact. Also mirror the same fix in the duplicated
ensure_compat_version/1 logic in spectrometer_updater.erl so
derive_since(undefined, undefined) formats as unreleased main instead of 0.0.x.
In `@src/spectrometer_ecosystem_worker_node.erl`:
- Around line 188-190: The log call in the undef throw handler is malformed
because the message uses `~p` but `?LOG_ERROR/1` is being called with no
arguments, so the node value is never interpolated. Update the `{throw, {error,
undef}}` clause in `spectrometer_ecosystem_worker_node` to use the format+args
logging form, matching the sibling error clauses, and pass the current node
value explicitly so the worker identifier is rendered correctly.
- Around line 32-33: The `@doc` comment in spectrometer_ecosystem_worker_node.erl
has an unterminated edoc inline-code quote because the `-eval` text opens a
backtick quote without a matching closer. Update the module docstring near the
worker node entry point so the inline code around -eval is properly balanced
using edoc’s backtick/apostrophe convention. Keep the rest of the `@doc` text
unchanged and verify the comment parses cleanly for the entry-point description
around the worker node start logic.
- Around line 314-317: The download-failure fallback in the worker error branch
is using a sentinel map that gets folded into persisted stats; update the error
path in the worker handling logic to return an empty map instead. Keep the
existing ?LOG_ERROR call, but change the {error, Reason} branch in the same
case/end flow to match the Hex path by returning #{} rather than the
<<"download_failed">> stat entry.
In `@src/spectrometer_ecosystem.erl`:
- Around line 437-520: Resume the next scan round using the compensated cursors
from compensate_fork_losses/8 instead of the pre-compensation values. In
scan_loop_round/9, replace the recursive call that currently passes
NextGithubPage2 and NextHexPage2 so it uses NextGithubPage3 and NextHexPage3,
keeping the next round aligned with the updated PagesConsumed state and avoiding
duplicate refetches.
In `@src/spectrometer_help.erl`:
- Around line 80-82: The help text for spectrometer audit is misleading about
exit codes: the command does not use a negative exit value for errors, and
`maybe_halt(1)` is used on failure while the unsupported-function count is the
audit result status. Update the wording in `spectrometer_help` so it describes a
non-zero error exit for audit failures and keep the unsupported-functions count
as the reported result.
In `@src/spectrometer_scanner.erl`:
- Around line 130-157: `parse_file/1` was removed from `spectrometer_scanner`,
but the test suite still calls it directly and will hit `undef`. Add a
one-argument wrapper on `parse_file/1` in `spectrometer_scanner.erl` that
delegates to `parse_file/2` with an empty include-path list, or update the
existing `spectrometer_scanner_tests` call sites to pass `[]`; keep the current
`parse_file/2` implementation unchanged.
In `@src/spectrometer_updater.erl`:
- Around line 711-757: Fix ensure_compat_version/1 so the unreleased branch case
for <<"main">> returns the {main, Major, Minor} form instead of defaulting to
{release, 0, 0}, matching the expected version normalization used by
detect_removals/3. Update the ensure_compat_version/1 clauses in
spectrometer_updater to handle the main branch explicitly, and consider
extracting this helper into a shared module such as spectrometer_utils so it
stays aligned with the duplicate implementation in spectrometer_atomvm.
In `@src/spectrometer_utils.erl`:
- Around line 202-218: The run_git_command/2 port setup is missing stderr
capture, so git failures can lose the real error text before gather_git_output/2
and ?LOG_ERROR(...) see it. Update the PortOpts in run_git_command/2 to route
stderr into the port output, and keep the existing output-gathering flow so
clone/checkout errors surface in the logged Output.
- Around line 200-201: The run_git_command/2 spec in spectrometer_utils needs to
accept environment entries that can include false values and should not use the
atom '[]' to represent an empty list. Update the type annotation on
run_git_command/2 so the second argument allows the env tuple values accepted by
open_port/2, including false, and rely on the list type itself to cover the
empty list case. Locate the spec by the run_git_command/2 function name and
adjust its argument type accordingly.
---
Outside diff comments:
In `@test/spectrometer_updater_tests.erl`:
- Around line 271-346: `write_db_file_test_` is still using the old
accumulator/value shape, so update the test data to match the new 3-tuple schema
expected by `spectrometer_updater:write_db_file/2` and its `maps:fold` callback.
Replace each `{Platforms, Since}` value in the `Acc` maps with `{Platforms,
SinceMap, Removed}`-style entries that the updater now consumes, and adjust the
read-back assertions to validate the emitted `{F, A, SinceMap, _Removed}`
structure instead of treating the third field as `Platforms`. Use the
`write_db_file_test_`, `write_db_file/2`, and `file:consult/1` checks to locate
and update all affected cases.
---
Nitpick comments:
In `@src/spectrometer_atomvm.erl`:
- Around line 14-47: The moduledoc in spectrometer_atomvm still documents the
old flat supported_functions.data shape, so update that description to match the
current since_map/removed schema used by the database and writer logic. Keep the
existing module overview, but revise the format examples and override text so
they reference the new structure consistently with write_db_file in
spectrometer_updater and the data loading/checking functions in
spectrometer_atomvm.
- Around line 735-746: The `format_platform_versions/1` helper is using the
obsolete `string:join/2`; replace that join call with `lists:join/2` and keep
the surrounding `lists:map/2` and `lists:flatten/1` flow intact. Use the
existing `format_platform_versions/1` function as the target for the change,
matching the `lists:join/2` pattern already used elsewhere in the PR.
- Around line 510-524: The `show_query/1` list comprehension uses a misleading
`begin ... end` block to rename `SM` to `Platforms`, even though the value is
still the `since_map()` from `#function{}`. Remove that hack in `show_query/1`
and use `SM` directly in the tuple being built, keeping the match/filter logic
around `get_supported_functions()` and the `#function{ since_map = SM }` pattern
unchanged.
In `@src/spectrometer_ecosystem_worker.erl`:
- Around line 20-45: The worker_state record currently carries next_worker_id,
but it is only initialized in init/1 and never read afterward. Remove
next_worker_id from the worker_state definition and from the state construction
in init/1, and keep the start_node/2 return handling focused on node, port, and
cookie only.
In `@src/spectrometer_otp.erl`:
- Around line 101-110: The ?LOG_WARNING calls in spectrometer_otp should not
include trailing newline characters in their format strings because the logger
already terminates messages. Update the warning logs in the write-failure and
cache-dir creation branches to omit the final “\n”, matching the other
?LOG_WARNING usages in this module and keeping the behavior consistent.
In `@src/spectrometer_scanner.erl`:
- Around line 61-65: Remove the duplicated debug artifacts in scan_directory:
the two entry logs in spectrometer_scanner should be reduced to a single useful
message. Keep the informative ?LOG_DEBUG call in scan_directory that includes
Dir and IncludePaths, and delete the redundant "scan_directory entered" and
"scan_directory FUNCTION ENTERED" logs so the function only logs once on entry.
- Around line 135-138: The scanner logging in spectrometer_scanner:parse_file
should avoid expensive and noisy debug output: stop calling
code:which(epp_dodger) for every file and remove the full epp_dodger parse
Result from the debug log. Update the logging around the parse_file/Result
handling to emit only a lightweight outcome such as ok/error or the number of
parsed forms, and keep any availability check for epp_dodger outside the
per-file hot path.
In `@src/spectrometer_updater.erl`:
- Around line 384-399: The return contract for scan_atomvm_repo/3 is stale: it
is typed and documented as if it returns entry() values, but the scan stage
actually produces only 2-tuples {platforms(), since_map()}. Update the spec and
docstring in scan_atomvm_repo/3 to use a dedicated scan-stage type (for example,
scan_entry()) instead of entry(), and keep it consistent with update_datafile/2,
merge_entry/2, and the existing scan_repo_test_ assertions that expect 2-tuples.
In `@test/atomvm_spectrometer_tests.erl`:
- Around line 14-36: The duplicated coord_state record in the test module can
drift from spectrometer_ecosystem_coordinator and cause the tests to become
stale or misaligned. Move the record definition into a shared .hrl include used
by both the coordinator and test module, and replace the local mirror in the
test file with that shared definition so coord_state stays identical everywhere.
In `@test/spectrometer_ecosystem_tests.erl`:
- Around line 75-107: The migration test is duplicating the tuple-decoding logic
instead of exercising the real `spectrometer_ecosystem:load_state/0` path, so it
can miss regressions in the production decoder. Replace the local
`load_state_migration/1` helper usage by writing a test state file through the
same `?ECOSYSTEM_STATE` location used by `save_state`, override `cache_dir` with
`application:set_env(spectrometer, cache_dir, ...)`, and assert directly on
`spectrometer_ecosystem:load_state/0` for both the `spectrometer_v1` and
`spectrometer_v0_r2` cases. Keep the test focused on the actual `load_state/0`
branch behavior, including the `package_map_valid` gate and default fallback.
🪄 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
Run ID: 26daa355-528e-47ad-b55c-781b83343779
📒 Files selected for processing (34)
LICENSETODO.mdgenerate_fun_data.shinclude/ecosystem.hrlinclude/function.hrlpriv/supported_functions.datapriv/supported_functions.data.backup2rebar.config.scriptsrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.erlsrc/spectrometer_ecosystem_coordinator.erlsrc/spectrometer_ecosystem_sup.erlsrc/spectrometer_ecosystem_worker.erlsrc/spectrometer_ecosystem_worker_node.erlsrc/spectrometer_ecosystem_worker_sup.erlsrc/spectrometer_help.erlsrc/spectrometer_http.erlsrc/spectrometer_otp.erlsrc/spectrometer_reporter.erlsrc/spectrometer_scanner.erlsrc/spectrometer_updater.erlsrc/spectrometer_utils.erltest/atomvm_spectrometer_tests.erltest/cli_main_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_ecosystem_tests.erltest/spectrometer_http_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
💤 Files with no reviewable changes (2)
- LICENSE
- test/spectrometer_utils_tests.erl
| Unsupported -> | ||
| maybe_halt(Unsupported) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file and surrounding lines.
git ls-files src/atomvm_spectrometer.erl
wc -l src/atomvm_spectrometer.erl
cat -n src/atomvm_spectrometer.erl | sed -n '1,220p'Repository: UncleGrumpy/atomvm_spectrometer
Length of output: 9083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect audit return shape and any existing exit-code handling around unsupported counts.
ast-grep outline src/spectrometer_analyzer.erl --view expanded
printf '\n---\n'
rg -n "audit\\(|Unsupported|halt\\(" src/spectrometer_analyzer.erl src/atomvm_spectrometer.erlRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 2513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the audit return shape and any exit-code handling around unsupported counts.
ast-grep outline src/spectrometer_analyzer.erl --view expanded
printf '\n---\n'
rg -n "audit\\(|Unsupported|halt\\(" src/spectrometer_analyzer.erl src/atomvm_spectrometer.erlRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 2513
🌐 Web query:
Erlang halt/1 exit status truncated to 0..255 documentation
💡 Result:
The Erlang documentation explicitly states that when using erlang:halt/1 or erlang:halt/2 with an integer status code, many operating systems only support status codes in the range 0-255 [1][2]. If an integer larger than 255 is provided, the value is truncated to fit this range, typically by clearing the higher-order bits [2]. Official documentation advises that for portability, developers should ensure that the exit status provided to halt/1 falls within this 0-255 range [1][2]. Other non-integer status types, such as strings or the atom abort, have their own defined behaviors for how the runtime system exits [3][2].
Citations:
- 1: https://www.erlang.org/doc/apps/erts/erlang.html
- 2: https://www.erlang.org/docs/20/man/erlang.html
- 3: https://www.erlang.org/docs/17/man/erlang
Cap the audit exit status at 255
maybe_halt(Unsupported) forwards the unsupported count straight to halt/1, but exit statuses are limited to 0–255. Values above 255 wrap, so a count like 256 can exit as 0 and report success. Clamp the code before halting, or use a fixed non-zero sentinel and print the exact count separately.
🤖 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/atomvm_spectrometer.erl` around lines 96 - 97, The unsupported-count path
in atomvm_spectrometer should not pass raw values from maybe_halt/1 directly to
halt/1 because counts above 255 can wrap to success. Update maybe_halt/1 (and
its Unsupported branch call site) to clamp the exit status to 255 or use a fixed
non-zero exit code while preserving the exact unsupported count in the output.
| ?LOG_INFO("Analyzing ~p unique function calls", [ | ||
| maps:get(total_unique, Stats, 0) | ||
| ]), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline src/spectrometer_analyzer.erl --view expanded || true
printf '\n== relevant lines ==\n'
sed -n '120,190p' src/spectrometer_analyzer.erl
printf '\n== search for total_unique and stats_map ==\n'
rg -n "total_unique|stats_map|maps:size\\(Stats\\)|merge_stats|scan_target|scan_multi" src/spectrometer_analyzer.erl src -g '!**/dist/**' -g '!**/build/**' || trueRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 7302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== analyzer scan/merge sections ==\n'
sed -n '199,360p' src/spectrometer_analyzer.erl
printf '\n== reporter total_unique usage ==\n'
sed -n '1,220p' src/spectrometer_reporter.erl | rg -n "total_unique|generate_report|maps:size|maps:get" -n -A3 -B3 || trueRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 8422
total_unique is not a key in Stats. scan_target/1 and merge_stats/2 build a map keyed by {binary(), binary(), arity()}, so maps:get(total_unique, Stats, 0) will always log 0. Use maps:size(Stats) here, as the later check already does.
🤖 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/spectrometer_analyzer.erl` around lines 145 - 147, The unique function
call count in spectrometer_analyzer:scan_target/1 is being read from a
non-existent total_unique key, so it always logs 0. Update the ?LOG_INFO call to
use the actual map size of Stats, consistent with the later check in the same
module, and keep merge_stats/2 and the Stats map structure unchanged.
| %% Ensure backward compatibility for version values (old binary → version tuple) | ||
| -spec ensure_compat_version(term()) -> version_tuple() | undefined. | ||
| ensure_compat_version(undefined) -> | ||
| undefined; | ||
| ensure_compat_version({unreleased, Branch}) when is_binary(Branch) -> | ||
| case binary:split(Branch, <<".">>, [global]) of | ||
| [Maj, <<"x">>] -> | ||
| try | ||
| {release, binary_to_integer(Maj), 0} | ||
| catch | ||
| _:_ -> {release, 0, 0} | ||
| end; | ||
| [Maj, Min, <<"x">>] -> | ||
| try | ||
| {release, binary_to_integer(Maj), binary_to_integer(Min)} | ||
| catch | ||
| _:_ -> {release, 0, 0} | ||
| end; | ||
| _ -> | ||
| {release, 0, 0} | ||
| end; | ||
| ensure_compat_version(V) when is_tuple(V) -> V; | ||
| ensure_compat_version(V) when is_binary(V) -> | ||
| case V of | ||
| <<"v", Rest/binary>> -> | ||
| try | ||
| Parts = binary:split(Rest, <<".">>, [global]), | ||
| case Parts of | ||
| [Maj, Min, Pat] -> | ||
| { | ||
| binary_to_integer(Maj), | ||
| binary_to_integer(Min), | ||
| binary_to_integer(Pat) | ||
| }; | ||
| _ -> | ||
| V | ||
| end | ||
| catch | ||
| _:_ -> V | ||
| end; | ||
| _ -> | ||
| V | ||
| end. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ensure_compat_version/1 never produces the documented {main, _, _} variant.
version_tuple() in function.hrl defines a dedicated {main, Major, Minor} case, and format_version_tuple/1 (Line 759) explicitly formats it as "unreleased main". However ensure_compat_version/1 only special-cases branch strings matching "Maj.x"/"Maj.Min.x"; the literal branch <<"main">> (no dot) falls through to the generic _ -> {release, 0, 0} clause. Since derive_since(undefined, undefined) defaults to {unreleased, <<"main">>}, every in-development/unreleased function will be mislabeled as "unreleased 0.0.x" instead of "unreleased main".
🐛 Proposed fix
ensure_compat_version({unreleased, Branch}) when is_binary(Branch) ->
+ case Branch of
+ <<"main">> ->
+ {main, 0, 0};
+ _ ->
case binary:split(Branch, <<".">>, [global]) of
[Maj, <<"x">>] ->
try
{release, binary_to_integer(Maj), 0}
catch
_:_ -> {release, 0, 0}
end;
[Maj, Min, <<"x">>] ->
try
{release, binary_to_integer(Maj), binary_to_integer(Min)}
catch
_:_ -> {release, 0, 0}
end;
_ ->
{release, 0, 0}
- end;
+ end
+ end;This exact logic is duplicated in spectrometer_updater.erl (ensure_compat_version/1, lines 711-756) — see the corresponding comment there.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| %% Ensure backward compatibility for version values (old binary → version tuple) | |
| -spec ensure_compat_version(term()) -> version_tuple() | undefined. | |
| ensure_compat_version(undefined) -> | |
| undefined; | |
| ensure_compat_version({unreleased, Branch}) when is_binary(Branch) -> | |
| case binary:split(Branch, <<".">>, [global]) of | |
| [Maj, <<"x">>] -> | |
| try | |
| {release, binary_to_integer(Maj), 0} | |
| catch | |
| _:_ -> {release, 0, 0} | |
| end; | |
| [Maj, Min, <<"x">>] -> | |
| try | |
| {release, binary_to_integer(Maj), binary_to_integer(Min)} | |
| catch | |
| _:_ -> {release, 0, 0} | |
| end; | |
| _ -> | |
| {release, 0, 0} | |
| end; | |
| ensure_compat_version(V) when is_tuple(V) -> V; | |
| ensure_compat_version(V) when is_binary(V) -> | |
| case V of | |
| <<"v", Rest/binary>> -> | |
| try | |
| Parts = binary:split(Rest, <<".">>, [global]), | |
| case Parts of | |
| [Maj, Min, Pat] -> | |
| { | |
| binary_to_integer(Maj), | |
| binary_to_integer(Min), | |
| binary_to_integer(Pat) | |
| }; | |
| _ -> | |
| V | |
| end | |
| catch | |
| _:_ -> V | |
| end; | |
| _ -> | |
| V | |
| end. | |
| %% Ensure backward compatibility for version values (old binary → version tuple) | |
| -spec ensure_compat_version(term()) -> version_tuple() | undefined. | |
| ensure_compat_version(undefined) -> | |
| undefined; | |
| ensure_compat_version({unreleased, Branch}) when is_binary(Branch) -> | |
| case Branch of | |
| <<"main">> -> | |
| {main, 0, 0}; | |
| _ -> | |
| case binary:split(Branch, <<".">>, [global]) of | |
| [Maj, <<"x">>] -> | |
| try | |
| {release, binary_to_integer(Maj), 0} | |
| catch | |
| _:_ -> {release, 0, 0} | |
| end; | |
| [Maj, Min, <<"x">>] -> | |
| try | |
| {release, binary_to_integer(Maj), binary_to_integer(Min)} | |
| catch | |
| _:_ -> {release, 0, 0} | |
| end; | |
| _ -> | |
| {release, 0, 0} | |
| end | |
| end; | |
| ensure_compat_version(V) when is_tuple(V) -> V; | |
| ensure_compat_version(V) when is_binary(V) -> | |
| case V of | |
| <<"v", Rest/binary>> -> | |
| try | |
| Parts = binary:split(Rest, <<".">>, [global]), | |
| case Parts of | |
| [Maj, Min, Pat] -> | |
| { | |
| binary_to_integer(Maj), | |
| binary_to_integer(Min), | |
| binary_to_integer(Pat) | |
| }; | |
| _ -> | |
| V | |
| end | |
| catch | |
| _:_ -> V | |
| end; | |
| _ -> | |
| V | |
| end. |
🤖 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/spectrometer_atomvm.erl` around lines 331 - 373, ensure_compat_version/1
is dropping the special unreleased main branch and falling back to {release, 0,
0}. Update the branch parsing in ensure_compat_version/1 to recognize <<"main">>
and return the documented {main, Major, Minor} form (or the equivalent
unreleased main tuple expected by format_version_tuple/1), while keeping the
existing Maj.x and Maj.Min.x compatibility cases intact. Also mirror the same
fix in the duplicated ensure_compat_version/1 logic in spectrometer_updater.erl
so derive_since(undefined, undefined) formats as unreleased main instead of
0.0.x.
| %% @doc Entry point for worker nodes. Called via `-eval` from the controller. | ||
| %% Starts a gen_server, registers it, and notifies the controller. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
edoc build break: unterminated quote in @doc. The backtick before -eval opens an edoc inline-code quote that is never closed, producing the CI failure -quote ended unexpectedly at line 33. edoc inline code uses a backtick opener and an apostrophe closer.
🔧 Proposed fix
-%% `@doc` Entry point for worker nodes. Called via `-eval` from the controller.
+%% `@doc` Entry point for worker nodes. Called via `-eval' from the controller.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| %% @doc Entry point for worker nodes. Called via `-eval` from the controller. | |
| %% Starts a gen_server, registers it, and notifies the controller. | |
| %% `@doc` Entry point for worker nodes. Called via `-eval' from the controller. | |
| %% Starts a gen_server, registers it, and notifies the controller. |
🧰 Tools
🪛 GitHub Actions: Build and Test / 0_build-and-test (28).txt
[error] 32-33: edoc failed while running edoc for spectrometer. Error: -quote ended unexpectedly at line 33 (in function start/1).
🪛 GitHub Actions: Build and Test / 1_build-and-test (29, 3.27.0).txt
[error] 32-33: edoc failed while generating documentation for spectrometer. Error: -quote ended unexpectedly at line 33 (edoc_doclet_chunks). Unknown error occurred generating doc chunks with edoc. Run with DIAGNOSTICS=1 for more details.
🪛 GitHub Actions: Build and Test / 2_build-and-test (27).txt
[error] 32-33: edoc failed while running edoc for spectrometer: -quote ended unexpectedly at line 33 (error in doclet 'edoc_doclet_chunks': {'EXIT',error}).
🪛 GitHub Actions: Build and Test / 3_build-and-test (master).txt
[error] 32-33: edoc failed while running edoc for spectrometer: -quote ended unexpectedly at line 33 in function start/1 at line 32.
🪛 GitHub Actions: Build and Test / build-and-test (27)
[error] 32-33: edoc failed while generating docs with DIAGNOSTICS unavailable. Error: -quote ended unexpectedly at line 33 (function start/1).
🪛 GitHub Actions: Build and Test / build-and-test (28)
[error] 32-33: edoc: -quote ended unexpectedly while generating docs. edoc: error in doclet 'edoc_doclet_chunks' ({'EXIT',error}). Run with DIAGNOSTICS=1 for more details.
🪛 GitHub Actions: Build and Test / build-and-test (29, 3.27.0)
[error] 32-33: edoc error while generating docs for spectrometer during rebar3 as doc ex_doc: -quote ended unexpectedly at line 33 (edoc_doclet_chunks failed with EXIT/error). Run with DIAGNOSTICS=1 for more details.
🪛 GitHub Actions: Build and Test / build-and-test (master)
[error] 32-33: edoc failed while generating documentation. Error: "-quote ended unexpectedly at line 33" (function start/1). edoc: error in doclet 'edoc_doclet_chunks': {'EXIT',error}. Run with DIAGNOSTICS=1 for more details.
🤖 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/spectrometer_ecosystem_worker_node.erl` around lines 32 - 33, The `@doc`
comment in spectrometer_ecosystem_worker_node.erl has an unterminated edoc
inline-code quote because the `-eval` text opens a backtick quote without a
matching closer. Update the module docstring near the worker node entry point so
the inline code around -eval is properly balanced using edoc’s
backtick/apostrophe convention. Keep the rest of the `@doc` text unchanged and
verify the comment parses cleanly for the entry-point description around the
worker node start logic.
Source: Pipeline failures
| -spec parse_file(string(), [string()]) -> {ok, map()} | {error, term()}. | ||
| parse_file(File, IncludePaths) -> | ||
| ?LOG_DEBUG("Scanner: parse_file called for ~p with include paths ~p", [ | ||
| File, IncludePaths | ||
| ]), | ||
| ?LOG_DEBUG("Scanner: epp_dodger available: ~p", [code:which(epp_dodger)]), | ||
| try | ||
| case epp_dodger:parse_file(File) of | ||
| Result = epp_dodger:parse_file(File, [{i, Path} || Path <- IncludePaths]), | ||
| ?LOG_DEBUG("Scanner: epp_dodger returned ~p", [Result]), | ||
| case Result of | ||
| {ok, Forms} -> | ||
| ?LOG_DEBUG("Scanner: got forms, extracting calls", []), | ||
| Calls = lists:foldl( | ||
| fun extract_calls/2, | ||
| #{}, | ||
| Forms | ||
| ), | ||
| ?LOG_DEBUG("Scanner: extracted ~p calls", [maps:size(Calls)]), | ||
| {ok, Calls}; | ||
| {error, Reason} -> | ||
| ?LOG_DEBUG("Scanner: epp_dodger error: ~p", [Reason]), | ||
| {error, Reason} | ||
| end | ||
| catch | ||
| _:Err -> | ||
| {error, Err} | ||
| Class:Err -> | ||
| ?LOG_DEBUG("Scanner: caught ~p:~p", [Class, Err]), | ||
| {error, {Class, Err}} | ||
| end. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all parse_file/1 call sites and confirm no parse_file/1 definition remains
rg -nP 'parse_file\s*\(\s*[^,)]+\s*\)' --type=erl
echo "--- definitions ---"
ast-grep run --pattern 'parse_file($_) -> $$$' --lang erlang src/spectrometer_scanner.erlRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files 'src/spectrometer_scanner.erl' 'test/spectrometer_scanner_tests.erl' 'tests/spectrometer_scanner_tests.erl' '*spectrometer_scanner_tests.erl'
printf '\n== scanner outline ==\n'
ast-grep outline src/spectrometer_scanner.erl --view expanded || true
printf '\n== scanner defs/search ==\n'
rg -n "parse_file\s*\(" src/spectrometer_scanner.erl test/spectrometer_scanner_tests.erl tests/spectrometer_scanner_tests.erl || trueRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 1943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== locate test file ==\n'
fd -a 'spectrometer_scanner_tests\.erl$' .
printf '\n== locate parse_file call sites in test file(s) ==\n'
for f in $(fd -a 'spectrometer_scanner_tests\.erl$' .); do
echo "--- $f ---"
rg -n "parse_file\s*\(" "$f" || true
done
printf '\n== parse_file definitions in scanner ==\n'
sed -n '1,240p' src/spectrometer_scanner.erl | cat -nRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 11409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== locate spectrometer_scanner_tests.erl ==\n'
fd -a '^spectrometer_scanner_tests\.erl$' .
printf '\n== parse_file call sites in all Erlang files ==\n'
rg -n --glob '*.erl' 'parse_file\s*\(' .
printf '\n== parse_file definitions in src/spectrometer_scanner.erl ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('src/spectrometer_scanner.erl')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 'parse_file' in line:
print(f"{i}: {line}")
PYRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 2355
Restore parse_file/1 for the test suite
src/spectrometer_scanner.erl now only defines parse_file/2, but test/spectrometer_scanner_tests.erl still calls spectrometer_scanner:parse_file(File) at lines 96, 106, 119, 369, 396, 523, and 542. Those calls will raise undef when the tests run. Add a parse_file(File) -> parse_file(File, []). wrapper or update the test call sites to pass [].
🤖 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/spectrometer_scanner.erl` around lines 130 - 157, `parse_file/1` was
removed from `spectrometer_scanner`, but the test suite still calls it directly
and will hit `undef`. Add a one-argument wrapper on `parse_file/1` in
`spectrometer_scanner.erl` that delegates to `parse_file/2` with an empty
include-path list, or update the existing `spectrometer_scanner_tests` call
sites to pass `[]`; keep the current `parse_file/2` implementation unchanged.
| %% Ensure backward compatibility for version values (old binary → version tuple) | ||
| %% Used when reading/writing the database to normalize version representations. | ||
| -spec ensure_compat_version(since() | version_tuple() | undefined) -> | ||
| version_tuple() | undefined. | ||
| ensure_compat_version(undefined) -> | ||
| undefined; | ||
| ensure_compat_version({unreleased, Branch}) when is_binary(Branch) -> | ||
| case binary:split(Branch, <<".">>, [global]) of | ||
| [Maj, <<"x">>] -> | ||
| try | ||
| {release, binary_to_integer(Maj), 0} | ||
| catch | ||
| _:_ -> {release, 0, 0} | ||
| end; | ||
| [Maj, Min, <<"x">>] -> | ||
| try | ||
| {release, binary_to_integer(Maj), binary_to_integer(Min)} | ||
| catch | ||
| _:_ -> {release, 0, 0} | ||
| end; | ||
| _ -> | ||
| {release, 0, 0} | ||
| end; | ||
| ensure_compat_version(V) when is_tuple(V) -> | ||
| V; | ||
| ensure_compat_version(V) when is_binary(V) -> | ||
| case V of | ||
| <<"v", Rest/binary>> -> | ||
| try | ||
| Parts = binary:split(Rest, <<".">>, [global]), | ||
| case Parts of | ||
| [Maj, Min, Pat] -> | ||
| { | ||
| binary_to_integer(Maj), | ||
| binary_to_integer(Min), | ||
| binary_to_integer(Pat) | ||
| }; | ||
| _ -> | ||
| V | ||
| end | ||
| catch | ||
| _:_ -> V | ||
| end; | ||
| _ -> | ||
| V | ||
| end. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Same ensure_compat_version/1 bug as spectrometer_atomvm.erl, plus code duplication.
This is a byte-for-byte duplicate of spectrometer_atomvm:ensure_compat_version/1, which also never produces the {main, Major, Minor} variant for the <<"main">> unreleased branch (see the corresponding comment on spectrometer_atomvm.erl for the fix). Given detect_removals/3 (lines 2091-2127) feeds Since (frequently {unreleased, <<"main">>} by default) into this same function to compute the Removed marker, functions removed during active development will be mislabeled as removed "in release 0.0.0" rather than "on main".
Consider extracting this helper (and the fix) into a shared module (e.g. spectrometer_utils) so both call sites stay in sync.
🤖 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/spectrometer_updater.erl` around lines 711 - 757, Fix
ensure_compat_version/1 so the unreleased branch case for <<"main">> returns the
{main, Major, Minor} form instead of defaulting to {release, 0, 0}, matching the
expected version normalization used by detect_removals/3. Update the
ensure_compat_version/1 clauses in spectrometer_updater to handle the main
branch explicitly, and consider extracting this helper into a shared module such
as spectrometer_utils so it stays aligned with the duplicate implementation in
spectrometer_atomvm.
| -spec run_git_command([string()], [{string(), string()}] | '[]') -> | ||
| {ok, string()} | {error, term()}. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does erlang open_port env option accept false as a value to unset an environment variable, and what is the exact type for the env tuple list in the OTP documentation?
💡 Result:
Yes, the {env, Env} option for Erlang's open_port/2 accepts the atom false as a value to unset an environment variable [1]. In the OTP documentation, the type for the environment variable list is defined as: {env, Env:: [{Name:: string, Val:: string | false}]} [1] Here, Name must be a string (or a charlist in Erlang terminology), and Val is either a string (the value to set) or the atom false, which instructs the system to remove that specific environment variable from the spawned port process [1]. This behaves analogously to os:getenv/1 [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the reported lines
sed -n '170,230p' src/spectrometer_utils.erl
# Find the TEST macro and any open_port env usage
rg -n "GIT_ENV|open_port|run_git_command|-spec run_git_command" src/spectrometer_utils.erl
# Check whether the codebase documents env tuple typing anywhere else
rg -n "string\(\) \| false|false\}\]|env.*false|SSH_ASKPASS" srcRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 3700
🌐 Web query:
Erlang/OTP open_port env option type documentation string | false false environment variable list
💡 Result:
In Erlang/OTP, the env option for open_port/2 is used to extend the environment of the spawned port process [1]. It is only valid when using the {spawn, Command} or {spawn_executable, FileName} port name options [1]. The env option expects a list of tuples in the format {Name, Val} [1]. Type Specifications: - Name: Must be a string (in Erlang, a charlist) [1]. - Val: Must be a string (charlist) or the atom false [1]. If Val is provided as a string, it sets the environment variable Name to that value [1]. If Val is provided as the atom false, the environment variable Name is removed (unset) for the spawned process, which is analogous to the behavior of os:getenv/1 [1]. Note for Elixir users: Since Erlang strings are charlists (e.g., 'VALUE'), you must ensure you are using single quotes for both the Name and Val when calling this option from Elixir, or you may encounter argument errors [1].
Citations:
Fix the run_git_command/2 spec
open_port/2 allows false in {env, ...} to unset a variable, so this spec should accept it. '[]' is an atom, and the empty list is already covered by the list type.
🩹 Proposed spec fix
--spec run_git_command([string()], [{string(), string()}] | '[]') ->
+-spec run_git_command([string()], [{string(), string() | false}]) ->
{ok, string()} | {error, term()}.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -spec run_git_command([string()], [{string(), string()}] | '[]') -> | |
| {ok, string()} | {error, term()}. | |
| -spec run_git_command([string()], [{string(), string() | false}]) -> | |
| {ok, string()} | {error, term()}. |
🤖 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/spectrometer_utils.erl` around lines 200 - 201, The run_git_command/2
spec in spectrometer_utils needs to accept environment entries that can include
false values and should not use the atom '[]' to represent an empty list. Update
the type annotation on run_git_command/2 so the second argument allows the env
tuple values accepted by open_port/2, including false, and rely on the list type
itself to cover the empty list case. Locate the spec by the run_git_command/2
function name and adjust its argument type accordingly.
| run_git_command(Args, EnvVars) -> | ||
| Cmd = "git", | ||
| case find_executable(Cmd) of | ||
| {ok, ExecPath} -> | ||
| PortOpts = [{args, Args}, exit_status, {line, 16384}], | ||
| PortOpts1 = | ||
| case EnvVars of | ||
| [] -> PortOpts; | ||
| _ -> [{env, EnvVars} | PortOpts] | ||
| end, | ||
| case os:find_executable(Cmd) of | ||
| false -> | ||
| {error, {executable_not_found, Cmd}}; | ||
| ExecPath -> | ||
| PortOpts = [ | ||
| {args, Args}, exit_status, {line, 16384}, {env, EnvVars} | ||
| ], | ||
| try | ||
| Port = open_port({spawn_executable, ExecPath}, PortOpts1), | ||
| Port = open_port({spawn_executable, ExecPath}, PortOpts), | ||
| gather_git_output(Port, []) | ||
| catch | ||
| error:Reason -> | ||
| {error, Reason} | ||
| end; | ||
| {error, not_found} -> | ||
| {error, {executable_not_found, Cmd}} | ||
| end. | ||
|
|
||
| -doc "Find an executable in PATH or return error if not found". | ||
| -spec find_executable(string()) -> {ok, string()} | {error, not_found}. | ||
| find_executable(Cmd) -> | ||
| case os:find_executable(Cmd) of | ||
| false -> {error, not_found}; | ||
| Path -> {ok, Path} | ||
| end | ||
| end. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does erlang open_port with spawn_executable capture child stderr by default, or is stderr_to_stdout required to include it in port messages?
💡 Result:
In Erlang, the open_port/2 function with the {spawn_executable, Command} option does not capture the child process's stderr by default [1]. By default, the stderr output of the spawned process is typically directed to the same destination as the original Erlang process's stderr (e.g., the console or log file), rather than being sent back to the Erlang port as messages [1]. To include stderr output in the messages received by the Erlang port, you must explicitly use the stderr_to_stdout option when opening the port [1]. When this option is enabled, the external process's stderr is redirected to its stdout, which the Erlang port mechanism then captures and delivers to the Erlang process as {Port, {data, Data}} messages [1]. Summary of behavior: - Default: stderr is not captured by the port and generally writes directly to the parent process's stderr stream [1]. - With stderr_to_stdout: stderr is redirected to stdout, and both are captured and delivered as port data [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate related functions/callers.
git ls-files src/spectrometer_utils.erl
ast-grep outline src/spectrometer_utils.erl --view expanded || true
printf '\n--- matches ---\n'
rg -n "run_git_command|gather_git_output|Could not clone|stderr_to_stdout|exit_status" src/spectrometer_utils.erlRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 1147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,235p' src/spectrometer_utils.erl
printf '\n---\n'
sed -n '310,360p' src/spectrometer_utils.erlRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 3442
Capture git stderr in the port output. open_port/2 won’t include the child’s stderr here, so clone/checkout failures can lose git’s actual error text; add stderr_to_stdout if you want it surfaced in Output and the ?LOG_ERROR(...) messages.
🤖 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/spectrometer_utils.erl` around lines 202 - 218, The run_git_command/2
port setup is missing stderr capture, so git failures can lose the real error
text before gather_git_output/2 and ?LOG_ERROR(...) see it. Update the PortOpts
in run_git_command/2 to route stderr into the port output, and keep the existing
output-gathering flow so clone/checkout errors surface in the logged Output.
Signed-off-by: Winford <winford@object.stream>
- Remove duplicate license file - Add missing specs - Rename reload_db/0 to flush_db_cache/0 for clarity - Cleanup and streamline ensure_binary/1 helper Signed-off-by: Winford <winford@object.stream>
Signed-off-by: Winford <winford@object.stream>
Changes from worker processes to supervised worker nodes, keeping the scanned atoms out of the controlling processes atom table. Workers track atom count and use a conservative level of 85% of the max atom table size to signal the coordinator to spawn a replacement node. Signed-off-by: Winford <winford@object.stream>
Duplicates entries from forks, or the same package scanned on hex.pm that was already scanned from GitHub (or vice versa) are no longer added to the data, the package sources are still recorded, to track that the package souce has already been processed for future resumed scans. Signed-off-by: Winford <winford@object.stream>
Add support for new opt-29 debugger to rebar.config.script Updates the internal AtomVM supported functions data with the latest merges to AtomVM. Signed-off-by: Winford <winford@object.stream>
1 participant
Avoids atom exhaustion by using worker nodes rather than worker processes. Workers track their own atom table usage and signal the controller to spawn a replacement node when their table reaches a conservative 85% full.
Adds per-platform version tracking of AtomVM functions, including removals due to deprecation.
Logger is used for all output that is not strictly results data.
The
auditcommand now exits with the number of unsupported functions in the target for the error value. All other errors exithalt(1).When performing
ecosystemscans, if the same package is encountered that has already been scanned, whether a github fork, or from hem.pm, the duplicate data will not be added. The package source is still recorded, so that a future PR can add the capability to list all of the package sources that use a particular function, and resume scans can skip these sources.Added support for the new opt-29 debugger in
rebar.config.scriptfor non-release builds.