Introduce spectrometer ecosystem anaysis tool#1
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (29)
📝 WalkthroughWalkthroughAdds a new spectrometer escript and OTP app that scan Erlang sources and ecosystem packages, fetches GitHub/Hex artifacts, builds/updates a bundled AtomVM-supported-functions DB, provides query/reporting, utilities, CI workflows, docs, a regeneration script, licenses, and extensive EUnit tests. ChangesAtomVM Spectrometer
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (17)
.gitignore-21-21 (1)
21-21:⚠️ Potential issue | 🟠 MajorAvoid blanket hidden-file ignore.
Line 21 (
.*) is too broad and can silently block tracking important files/directories (e.g.,.github/workflows, project dot-configs). Prefer explicit patterns instead.Proposed fix
-.* +.DS_Store +.idea/ +.vscode/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 21, The .gitignore entry '.*' is too broad and will ignore important dotfiles (e.g., .github workflows); remove this blanket pattern and replace it with explicit ignores or safe negations: delete '.*' and add specific patterns for temporary dotfiles you want ignored (e.g., .DS_Store, .env.local) and add explicit allow patterns for essential directories/files (e.g., '!/.github/', '!/.gitignore' or other project dot-configs) so only intended hidden files are excluded.test/spectrometer_scanner_tests.erl-273-275 (1)
273-275:⚠️ Potential issue | 🟠 MajorReplace shell-based recursive delete with Erlang filesystem API.
Using
os:cmd("rm -rf ...")is Unix-specific and inconsistent with cross-platform temp path handling elsewhere in this test module. Erlang/OTP 27 providesfile:del_dir_r/1(available since OTP 23.0) for portable recursive directory deletion.Proposed fix
cleanup_temp_dir({Dir, _Files}) -> - os:cmd(lists:flatten(io_lib:format("rm -rf '~s'", [Dir]))). + ok = file:del_dir_r(Dir). cleanup_temp_dir_simple(Dir) -> - os:cmd(lists:flatten(io_lib:format("rm -rf '~s'", [Dir]))). + ok = file:del_dir_r(Dir).Also applies to: 284-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_scanner_tests.erl` around lines 273 - 275, The cleanup_temp_dir function currently calls os:cmd with a shell rm -rf; replace that with the Erlang file API by calling file:del_dir_r(Dir) (where cleanup_temp_dir/1 pattern-matches {Dir, _Files}); handle the return value (accept {ok, _Removed} or ok, and on {error, Reason} either log/throw or ignore as appropriate for tests) to make deletion portable; apply the same change to the other similar helper usage in the test (the block around the other occurrence that currently calls os:cmd).src/spectrometer_updater.erl-321-349 (1)
321-349:⚠️ Potential issue | 🟠 MajorVersion ordering needs semantic comparison, not binary comparison.
Line 323 and Line 472 compare tags lexicographically, so
<<"v0.10.0">>sorts before<<"v0.9.0">>. That meansmerge_entry/2andmerge_since/2can preserve a newer release as if it were the older one.Also applies to: 469-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_updater.erl` around lines 321 - 349, The code incorrectly compares version tags as raw binaries (lexicographic) in is_older_since and thus in merge_entry and merge_since; implement semantic version comparison instead: parse tag binaries like <<"vMAJOR.MINOR.PATCH">> into numeric triples (handle optional "v" prefix and pre-release/metadata if needed) and replace binary comparisons (First < Second and symmetric checks) with a semver comparator (e.g., compare_semver/2 returning -1/0/1). Preserve existing {unreleased, Branch} guards behavior but use compare_semver when both args are version tags; update merge_entry/2 and merge_since/2 to call the new comparator to decide which Since value is older. Ensure parsing errors fall back safely (e.g., treat unparsable tags as equal or use binary fallback).src/spectrometer_reporter.erl-312-346 (1)
312-346:⚠️ Potential issue | 🟠 MajorThe CSV schema here doesn't round-trip through
parse_csv_rows/1.
src/spectrometer_analyzer.erl:492-525treats a 5th CSV column as numeric repo count, but this writer emitsatomvm_supportedwith the stringno. Feeding this file back into the filter/import path will cause those rows to be skipped instead of parsed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_reporter.erl` around lines 312 - 346, The CSV writer do_write_csv/2 currently emits a fifth column "atomvm_supported" with the string "no", which doesn't match parse_csv_rows/1 that expects a numeric repo count; update the header line in do_write_csv to "module,function,arity,calls,repo_count" and change the per-row format from emitting "no" to a numeric 0 (e.g. replace the header string and the "~ts,~ts,~p,~p,no\n" row format with a header "...,repo_count\n" and "~ts,~ts,~p,~p,0\n") so the file round-trips through parse_csv_rows/1 correctly.test/spectrometer_http_tests.erl-16-97 (1)
16-97:⚠️ Potential issue | 🟠 MajorGuard every network test behind
AVM_SKIP_NETWORK_TESTS.
fetch_github_repos_test_/0and the Hex fetch tests honor the skip flag, but the clone/download/integration tests here still hit GitHub/Hex unconditionally. That makes the opt-out incomplete, so offline CI or local runs will still fail even whenAVM_SKIP_NETWORK_TESTSis set.Also applies to: 191-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_http_tests.erl` around lines 16 - 97, Tests download_github_repo_success_test_, download_github_repo_invalid_test_, download_hex_tarball_valid_test_, download_hex_tarball_nonexistent_test_, and download_hex_tarball_cleanup_test_ perform real network calls and must be skipped when AVM_SKIP_NETWORK_TESTS is set; add an environment guard at the start of each test (check os:getenv("AVM_SKIP_NETWORK_TESTS") or the existing helper used elsewhere in the suite) and immediately return a skipped/safe result (or a no-op fun) when the flag is present so these tests do not attempt GitHub/Hex access in offline runs.test/spectrometer_http_tests.erl-220-221 (1)
220-221:⚠️ Potential issue | 🟠 MajorAvoid
rm -rfin the test cleanup helper.
system_temp_dir/0has a Windows branch, butcleanup_temp_dir/1only works on Unix and will also break on paths containing'. Reusespectrometer_utils:purge_dir/1or a recursive file API instead of shelling out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_http_tests.erl` around lines 220 - 221, cleanup_temp_dir/1 currently shells out with os:cmd("rm -rf 'Dir'") which is Unix-only and unsafe for paths containing quotes; replace that implementation to call the cross-platform recursive deletion helper spectrometer_utils:purge_dir(Dir) (or implement equivalent using file:list_dir/1, filelib:delete_dir/1 and file:delete/1) so cleanup works on Windows and avoids shell quoting issues; update any callers expecting os:cmd return values to handle the purge_dir result.src/spectrometer_http.erl-287-296 (1)
287-296:⚠️ Potential issue | 🟠 MajorDisable TLS verification in GitHub and Hex requests must be removed.
Both
fetch_hex_tarball/2(line 287-296) and the genericfetch/1function (line 359-369) use{ssl, [{verify, verify_none}]}, which disables TLS certificate verification for all HTTPS requests toapi.github.com,hex.pm, andrepo.hex.pm. This makes the tool vulnerable to man-in-the-middle attacks and compromises the integrity of downloaded packages and API responses used for source analysis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_http.erl` around lines 287 - 296, The request blocks in fetch_hex_tarball/2 and fetch/1 disable TLS verification ({ssl, [{verify, verify_none}]}), which must be removed; change the ssl options passed to httpc:request in those functions to enforce certificate verification (e.g., use {ssl, [{verify, verify_peer}, {cacertfile, "/etc/ssl/certs/ca-bundle.crt"}, {depth, 3}]} or equivalent platform-appropriate CA bundle and enable hostname verification), and remove any verify_none entries so HTTPS calls to api.github.com, hex.pm and repo.hex.pm validate certificates properly.src/spectrometer_updater.erl-848-875 (1)
848-875:⚠️ Potential issue | 🟠 Major
calculate_arity/3currently miscomputes call arity.Line 853 turns the call tail into a list of substrings with
string:split/2, butcount_args/1walks a character list and matches"(",")", and","one character at a time. Those clause heads never line up with the actual input shape, sofoo(),foo(A,B), and nested calls all end up with the wrong arity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_updater.erl` around lines 848 - 875, calculate_arity/3 currently passes a list of substrings to count_args/1 which expects a charlist; change the code so that after finding Start you take the tail slice (string:slice(Line, Start)) and convert it to a charlist (e.g. via string:to_charlist/1) before calling count_args/1; update the binding currently named Rest in calculate_arity/3 to hold that charlist and leave count_args/3 unchanged so its character-by-character pattern matches work correctly.src/spectrometer_atomvm.erl-195-215 (1)
195-215:⚠️ Potential issue | 🟠 MajorInvalidate the cached DB when
cache_dirchanges.The DB is cached only under
supported_db, butquery/1andreport_supported/1switchcache_dirat runtime. In the same process, a later-c/--cachecan still read the previously loaded database instead of the requested one.Minimal fix
query(Opts) -> case Opts of #{cache_dir := CacheDir} -> - application:set_env(spectrometer, cache_dir, CacheDir); + application:set_env(spectrometer, cache_dir, CacheDir), + reload_db(); #{} -> ok end, @@ report_supported(Opts) -> case Opts of #{cache_dir := CacheDir} -> - application:set_env(spectrometer, cache_dir, CacheDir); + application:set_env(spectrometer, cache_dir, CacheDir), + reload_db(); #{} -> ok end,Also applies to: 275-280, 407-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_atomvm.erl` around lines 195 - 215, Cached DB under the single key supported_db can be stale when cache_dir changes at runtime; modify load_db/0 and load_data/0 to include the cache_dir in the cache key (e.g. use put({supported_db, CacheDir}, DB) and fetch via get({supported_db, CacheDir})) or alternatively store the last cached cache_dir and invalidate when it differs from the current cache_dir (compare current cache_dir via the existing cache_dir() accessor), and apply the same change to the other cached sites mentioned (the similar blocks around the other occurrences).src/spectrometer_analyzer.erl-497-507 (1)
497-507:⚠️ Potential issue | 🟠 MajorThe CSV parser rejects the 5-column format the reporter writes.
src/spectrometer_reporter.erl:312-330emitsmodule,function,arity,calls,atomvm_supported, but this branch expects the fifth column to be an integer repo count. As written,filter --csvwill drop rows from the generated report instead of reading them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_analyzer.erl` around lines 497 - 507, The CSV parser in parse_csv_rows currently requires the fifth column (RCStr) to parse to an integer, but reporter emits a boolean/atom column (atomvm_supported); change the parsing branch so it only enforces integer conversion for ArityStr and CallsStr, and accept RCStr as-is (or normalize "true"/"false" to an atom/boolean) instead of calling string:to_integer on it; update the tuple construction to use the normalized RC value (e.g., RCBool or RCStr) when building [{ModStr, FunStr, Arity, Calls, RCValue} | Acc] so generated reports with atomvm_supported are preserved.src/atomvm_spectrometer.erl-497-505 (1)
497-505:⚠️ Potential issue | 🟠 MajorDon't print arbitrary error terms with
~s.
update_datafile/2returns{error, term()}. IfReasonis an atom or tuple, thisio:format/3call can fail while handling the original updater error. Use~phere.Proposed fix
io:format( - standard_error, "Error: unable to update data, ~s\n", [ + standard_error, "Error: unable to update data: ~p\n", [ Reason ] ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/atomvm_spectrometer.erl` around lines 497 - 505, Change the io:format call that logs errors returned from spectrometer_updater:update_datafile(Opts, OutputFile) to use a safe pretty-printer for arbitrary Erlang terms: replace the "~s" format with "~p" (and keep the newline) so the {error, Reason} case prints any atom/tuple/list/etc. without crashing when Reason is not a string; locate the clause handling {error, Reason} in the case around spectrometer_updater:update_datafile and update the format string accordingly.src/spectrometer_analyzer.erl-203-219 (1)
203-219:⚠️ Potential issue | 🟠 Major
scan_multi/1is parsing the file incorrectly and injecting a fake row.
"\\n"matches a backslash plusn, not a newline, so a normal multi-target file is treated as one long line. The seeded accumulator also leaves{undefined, undefined, 0}in every non-empty result set.Proposed fix
scan_multi(File) -> {ok, Bin} = file:read_file(File), - Lines = string:split(binary_to_list(Bin), "\\n", all), + Lines = string:split(binary_to_list(Bin), "\n", all), Targets = parse_target_lines(Lines), - io:format("Scanning ~p targets from ~s...\\n\\n", [length(Targets), File]), + io:format("Scanning ~p targets from ~s...\n\n", [length(Targets), File]), {_, FinalAcc} = lists:foldl( fun(Target, {Count, Acc}) -> NewCount = Count + 1, - io:format("[~p/~p]\\n", [NewCount, length(Targets)]), + io:format("[~p/~p]\n", [NewCount, length(Targets)]), Stats0 = scan_target(Target), NewAcc = merge_stats(Stats0, Acc), {NewCount, NewAcc} end, - {0, #{{undefined, undefined, 0} => 0}}, + {0, #{}}, Targets ), FinalAcc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_analyzer.erl` around lines 203 - 219, In scan_multi/1 the file splitting uses the literal "\\n" (backslash + n) which prevents proper newline splitting, and the seeded accumulator includes a bogus {undefined,undefined,0} entry; change string:split(binary_to_list(Bin), "\\n", all) to use a real newline delimiter string:split(binary_to_list(Bin), "\n", all) (or use binary:split(Bin, <<"\n">>, [global]) for binaries) and replace the initial accumulator {0, #{{undefined, undefined, 0} => 0}} with {0, #{}} so merge_stats is fed an empty map rather than a fake row; keep function names scan_multi/1, parse_target_lines/1, scan_target/1 and merge_stats/2 to locate the edits.src/atomvm_spectrometer.erl-380-400 (1)
380-400:⚠️ Potential issue | 🟠 MajorAdd cache-dir parsing for
filter.
filterreads cached ecosystem state, but this parser gives users no way to select a non-default cache directory. That makesecosystem --cache-dir ...and a laterfilteragainst the same cache impossible from the CLI.Proposed fix
parse_filter_args([], Opts) -> Opts#{min_repos => maps:get(min_repos, Opts, 1)}; +parse_filter_args(["--cache-dir", Dir | Rest], Opts) -> + parse_filter_args(Rest, Opts#{cache_dir => Dir}); +parse_filter_args(["--cache", Dir | Rest], Opts) -> + parse_filter_args(Rest, Opts#{cache_dir => Dir}); +parse_filter_args(["-c", Dir | Rest], Opts) -> + parse_filter_args(Rest, Opts#{cache_dir => Dir}); parse_filter_args(["--min-repos", N | Rest], Opts) ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/atomvm_spectrometer.erl` around lines 380 - 400, Add support for a "--cache-dir" option in parse_filter_args: implement a new clause in the parse_filter_args/2 function to match ["--cache-dir", Dir | Rest] and return parse_filter_args(Rest, Opts#{cache_dir => Dir}); place this clause before the generic [MaybeFile | Rest] clause so it’s recognized, and mirror the same semantics as the "--csv" clause (store the directory string under the key cache_dir in the Opts map). Ensure no other behavior changes.src/spectrometer_utils.erl-68-82 (1)
68-82:⚠️ Potential issue | 🟠 MajorFix application name mismatch in
code:priv_dir/1call.Line 100 uses
code:priv_dir(atomvm_spectrometer), but the application is registered asspectrometer(fromspectrometer.app.src). This will fail at runtime. Change tocode:priv_dir(spectrometer).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_utils.erl` around lines 68 - 82, The call to code:priv_dir/1 uses the wrong application atom (code:priv_dir(atomvm_spectrometer)), which will fail because the app is registered as spectrometer; update that invocation to code:priv_dir(spectrometer) (search for code:priv_dir(atomvm_spectrometer) and replace with code:priv_dir(spectrometer)) and ensure any related references (e.g., in functions that compute resource paths) consistently use the spectrometer atom.src/atomvm_spectrometer.erl-277-285 (1)
277-285:⚠️ Potential issue | 🟠 MajorMake Hex
--versionorder-independent.If the user passes
--versionbefore--hex, the version is left inOptsand never folded into the{hex, Package, Version}target, so the audit silently falls back to"latest". CLI option order should not change semantics here.Proposed fix
parse_audit_args(["--hex", Pkg | Rest], Opts) -> - parse_audit_args(Rest, Opts#{target => {hex, Pkg}}); + NextOpts = + case maps:find(version, Opts) of + {ok, Ver} -> + maps:remove(version, Opts)#{target => {hex, Pkg, Ver}}; + error -> + Opts#{target => {hex, Pkg}} + end, + parse_audit_args(Rest, NextOpts); parse_audit_args(["--version", Ver | Rest], Opts) -> case Opts of #{target := {hex, Name}} -> parse_audit_args(Rest, Opts#{target => {hex, Name, Ver}}); #{} -> parse_audit_args(Rest, Opts#{version => Ver}) end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/atomvm_spectrometer.erl` around lines 277 - 285, The parse_audit_args clause handling "--version" is order-dependent: when "--version" appears before "--hex" the version is stored under Opts#{version => Ver} and never merged into the hex target; update parse_audit_args so that the "--hex" clause checks for an existing Opts version and folds it into the target (creating {hex, Pkg, Ver} and removing the standalone version key), and update the "--version" clause to either merge into an existing {hex, Name} target (as now) or set Opts#{version => Ver} so a later "--hex" can pick it up; ensure you modify the parse_audit_args clauses that pattern-match ["--hex", Pkg | Rest] and ["--version", Ver | Rest] in the atomvm_spectrometer.erl file to implement this symmetric logic.src/spectrometer_analyzer.erl-283-286 (1)
283-286:⚠️ Potential issue | 🟠 MajorUse
binary_to_term/2with[safe]option for deserializing cache data.Data from user cache is untrusted, so
binary_to_term(Bin, [safe])should be used to prevent accepting dangerous term shapes while maintaining compatibility. This applies not only to line 285 in this file but also to similar patterns inspectrometer_ecosystem.erl:375andspectrometer_otp.erl:31, which all deserialize from user cache without the safe option.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_analyzer.erl` around lines 283 - 286, Replace direct binary_to_term(Bin) deserialization with binary_to_term(Bin, [safe]) and guard against exceptions: wrap the call in a try/catch so malformed or unsafe binaries are treated as a cache miss (log the error and continue) rather than crashing; specifically update the branch that matches {spectrometer_v1, _, Stats, _} (using StateFile and Stats) in spectrometer_analyzer.erl and make the same change to the corresponding deserialization sites in spectrometer_ecosystem.erl and spectrometer_otp.erl so all user-cache binary_to_term calls use the [safe] option and handle errors gracefully.src/spectrometer_analyzer.erl-316-320 (1)
316-320:⚠️ Potential issue | 🟠 MajorHonor
cache_dirbefore loading filter data.
load_ecosystem_state/0resolves its path throughspectrometer_utils:user_cache_path/0, butfilter/1never appliesOpts.cache_dir. Even after the CLI parser is extended,filterwill still read the default cache.Proposed fix
filter(Opts) -> + case Opts of + #{cache_dir := CacheDir} -> + application:set_env(spectrometer, cache_dir, CacheDir); + #{} -> + ok + end, MinRepos = maps:get(min_repos, Opts, 1), AvmFilter = maps:get(avm, Opts, false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_analyzer.erl` around lines 316 - 320, filter/1 currently ignores Opts.cache_dir so downstream zero-arity loaders like load_ecosystem_state/0 (which call spectrometer_utils:user_cache_path/0) still use the default cache; fix by reading CacheDir = maps:get(cache_dir, Opts, undefined) at the top of filter/1 and, when present, apply it to the global/path accessor before calling load_filter_data(Opts) — e.g. call the existing helper that sets the user cache path (spectrometer_utils:set_user_cache_path(CacheDir) or the equivalent application env setter used elsewhere) so load_filter_data(Opts) and load_ecosystem_state/0 will resolve the cache from Opts.cache_dir.
🟡 Minor comments (8)
README.md-24-132 (1)
24-132:⚠️ Potential issue | 🟡 MinorAddress markdownlint MD014 warnings in command examples.
The
$prompt style across command blocks triggers the reported MD014 warnings. Either remove prompt characters or explicitly disable MD014 for this README/linter config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 24 - 132, The README triggers markdownlint MD014 because command blocks include the "$ " shell prompt; fix by removing the leading "$ " from all command example lines (e.g., the blocks showing "spectrometer audit", "spectrometer ecosystem", "spectrometer query", "spectrometer update", and help examples) so lines read "spectrometer audit ..." etc., or alternatively wrap affected sections with a markdownlint disable/restore for MD014 if you prefer keeping prompts (use <!-- markdownlint-disable MD014 --> before and <!-- markdownlint-restore MD014 --> after the blocks).generate_fun_data.sh-14-17 (1)
14-17:⚠️ Potential issue | 🟡 MinorIncorrect argument count check.
[ -z "$@" ]doesn't work correctly for checking if arguments were provided. Arrays don't work as operands in[ ]. Use$#to check argument count instead.Proposed fix
-if [ -z "$@" ]; then +if [ $# -eq 0 ]; then echo "Usage: $0 <path-to-atomvm-dir>" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate_fun_data.sh` around lines 14 - 17, The argument check in generate_fun_data.sh is wrong: replace the current conditional if [ -z "$@" ]; then with a proper argument-count check using $# (e.g., if [ "$#" -eq 0 ]; then) so the script correctly detects no arguments and still prints the usage/exit behavior; keep the existing echo and exit 1 lines unchanged.generate_fun_data.sh-125-126 (1)
125-126:⚠️ Potential issue | 🟡 MinorDate format typo: duplicate month instead of minutes.
The format
%Y%m%d%H%muses%m(month) twice. The second occurrence should be%M(minutes) for a proper timestamp.Proposed fix
- echo "Backing up existing $DEST_FILE to ${DEST_FILE}.$(date +%Y%m%d%H%m).bak" - mv "${DEST_FILE}" "${DEST_FILE}.$(date +%Y%m%d%H%m).bak" + echo "Backing up existing $DEST_FILE to ${DEST_FILE}.$(date +%Y%m%d%H%M).bak" + mv "${DEST_FILE}" "${DEST_FILE}.$(date +%Y%m%d%H%M).bak"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate_fun_data.sh` around lines 125 - 126, The timestamp format used when backing up DEST_FILE is incorrect (uses %m twice); update the date format in both the echo and mv commands that reference DEST_FILE to use %Y%m%d%H%M (replace the second %m with %M for minutes) so the backup suffix is year/month/day/hour/minute and keep the same date invocation for both lines to ensure matching filenames.src/spectrometer_otp.erl-29-38 (1)
29-38:⚠️ Potential issue | 🟡 MinorAdd error handling for
binary_to_termdeserialization.If the cache file is corrupted,
binary_to_term/1will throwbadarg, crashing the function. Consider wrapping in try/catch to fall back gracefully to regeneration.Proposed fix with error handling
case file:read_file(ModFile) of {ok, Bin} -> - binary_to_term(Bin); + try + binary_to_term(Bin) + catch + error:badarg -> + io:format("Corrupted OTP module cache, regenerating...\n"), + [M || {M, _, _} <- code:all_available()] + end; {error, Reason} ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_otp.erl` around lines 29 - 38, The current call to binary_to_term(Bin) in the file:read_file result path can throw (e.g., badarg) for a corrupted cache and crash the function; wrap the binary_to_term(Bin) call in a try/catch inside the case clause that handles {ok, Bin} so any exceptions (at least badarg) are caught, log the error with the Reason and Bin summary, and fall back to regenerating the module list (the existing [M || {M, _, _} <- code:all_available()]) instead of letting the function crash; ensure you reference the same spot where file:read_file(ModFile) is handled and replace the direct binary_to_term(Bin) call with the try/catch block for robust deserialization.src/spectrometer_otp.erl-40-44 (1)
40-44:⚠️ Potential issue | 🟡 MinorEnsure cache directory exists before writing.
If
spectrometer_utils:user_cache_path()returns a path that doesn't exist yet,file:write_file/2will fail silently (the result is ignored with_). Callfilelib:ensure_dir/1first.Proposed fix
false -> Modules = [M || {M, _, _} <- code:all_available()], + _ = filelib:ensure_dir(ModFile), _ = file:write_file(ModFile, term_to_binary(Modules)), Modules🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_otp.erl` around lines 40 - 44, Ensure the cache directory exists before writing the module list: compute Dir = filename:dirname(ModFile) and call filelib:ensure_dir(Dir) before invoking file:write_file(ModFile, term_to_binary(Modules)) in the false branch where Modules and ModFile are used (spectrometer_otp module); this guarantees the directory is created if missing and prevents write failures.src/spectrometer_help.erl-76-77 (1)
76-77:⚠️ Potential issue | 🟡 MinorMissing space in help text string concatenation.
The string literal on line 76 ends with "OTP" and line 77 starts with "function", resulting in "OTPfunction" when printed instead of "OTP function".
Proposed fix
"Usage: spectrometer audit [TARGET] [OPTIONS]\n" "\n" - "Audit a single target, or a list of targets from a file for OTP" - "function usage and\nreport which functions are NOT supported by AtomVM.\n" + "Audit a single target, or a list of targets from a file for OTP " + "function usage and\nreport which functions are NOT supported by AtomVM.\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_help.erl` around lines 76 - 77, The help text concatenation currently produces "OTPfunction" because the adjacent string literals ending with "OTP" and starting with "function usage..." lack a space; fix the string literals shown in spectrometer_help.erl by adding a single space either at the end of the first literal ("OTP ") or at the start of the second literal (" function usage...") so the printed help reads "OTP function" (ensure no duplicate spaces are introduced).test/spectrometer_reporter_tests.erl-216-242 (1)
216-242:⚠️ Potential issue | 🟡 MinorThis test never hits the integer-limit path.
write_csv_limit_integer_test_/0callsspectrometer_reporter:write_csv(File, Report, all), so a regression in the numericLimitbranch still passes. Use an actual integer and assert that fewer rows are written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_reporter_tests.erl` around lines 216 - 242, The test write_csv_limit_integer_test_/0 never exercises the integer Limit branch because it calls spectrometer_reporter:write_csv(File, Report, all); change the call to pass a concrete integer limit (e.g., 1 or 2) instead of all, then update the assertion on length(Lines) to expect header + limited number of data rows (e.g., 2 for limit 1). Keep the same Report fixture and cleanup logic; ensure the assertion references length(Lines) to verify fewer rows are written when an integer Limit is used.test/cli_main_tests.erl-31-64 (1)
31-64:⚠️ Potential issue | 🟡 Minor
ensure_atomvm_repo/0never hits its reuse path.Because
spectrometer_utils:make_temp_dir/1returns a fresh directory every call,AtomVMDir/.gitcannot already exist here. The whole "update existing repo" branch is dead, and the helper always reclones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli_main_tests.erl` around lines 31 - 64, The reuse branch never runs because spectrometer_utils:make_temp_dir("spectrometer_git_clone_") always returns a new dir; change ensure_atomvm_repo/0 to use a persistent cache path instead of a fresh temp each call (for example build a cache root like filename:join(os:getenv("HOME") || ".", ".cache", "spectrometer_git_clone") and set AtomVMDir = filename:join(CacheRoot, "AtomVM")), then preserve the existing .git-check, stash and pull logic; update references to TempDir/AtomVMDir in ensure_atomvm_repo/0 accordingly so the "update existing AtomVM repo" branch can be reached.
🧹 Nitpick comments (9)
test/spectrometer_utils_tests.erl (1)
228-230: Minor: Section comment doesn't match tested function.The comment says
http_get/1 testsbut the tests callspectrometer_http:fetch/1. Consider updating the comment for consistency.Proposed fix
%% ============================================================================= -%% http_get/1 tests +%% spectrometer_http:fetch/1 tests %% =============================================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_utils_tests.erl` around lines 228 - 230, The section comment header currently reads "http_get/1 tests" but the tests exercise spectrometer_http:fetch/1; update the comment to match the tested function (e.g., change the header to "spectrometer_http:fetch/1 tests" or "fetch/1 tests") so the comment and the tests (spectrometer_http:fetch/1) are consistent.src/spectrometer_otp.erl (1)
20-22: Uselists:member/2for simpler and more efficient membership check.
lists:any(fun(X) -> X =:= AtomStr end, OTPmods)can be replaced withlists:member(AtomStr, OTPmods)which is more idiomatic and slightly more efficient.Proposed fix
is_otp_module(AtomStr) when is_list(AtomStr) -> OTPmods = modules_list(), - lists:any(fun(X) -> X =:= AtomStr end, OTPmods). + lists:member(AtomStr, OTPmods).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_otp.erl` around lines 20 - 22, Replace the manual membership check in is_otp_module/1 with the idiomatic lists:member/2: instead of building OTPmods and calling lists:any(fun(X) -> X =:= AtomStr end, OTPmods), call lists:member(AtomStr, OTPmods) (keeping the call to modules_list() to populate OTPmods) to simplify the code and improve performance; update the implementation of is_otp_module/1 accordingly.test/spectrometer_analyzer_tests.erl (3)
163-165: Same tautological assertion pattern.Same issue as above -
maps:size(Stats) >= 0is always true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_analyzer_tests.erl` around lines 163 - 165, The assertion maps:size(Stats) >= 0 is tautological; update the test to assert a meaningful condition about Stats (e.g., that it contains at least one entry or specific keys). Replace ?assert(maps:size(Stats) >= 0) with something like ?assert(maps:size(Stats) > 0) or assert that Stats contains expected keys/values (inspect Stats and use ?assert(maps:is_key(ExpectedKey, Stats)) or ?assertEqual(ExpectedCount, maps:size(Stats))) so the test in spectrometer_analyzer_tests.erl actually verifies results.
192-193: Preferpurge_dirover shell command for cleanup.The current implementation uses
os:cmdwith string formatting which could be problematic ifDircontains shell metacharacters. Other tests in this file usespectrometer_utils:purge_dir/1directly for cleanup, which is safer and more consistent.Proposed fix
cleanup_temp_dir(Dir) -> - os:cmd(lists:flatten(io_lib:format("rm -rf '~s'", [Dir]))). + spectrometer_utils:purge_dir(Dir).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_analyzer_tests.erl` around lines 192 - 193, Replace the unsafe os:cmd-based cleanup in function cleanup_temp_dir/1 with the project's safe helper: call spectrometer_utils:purge_dir(Dir) instead of using os:cmd(lists:flatten(io_lib:format("rm -rf '~s'", [Dir]))), ensuring the directory is removed without invoking the shell or allowing shell metacharacter injection; update the body of cleanup_temp_dir to delegate to spectrometer_utils:purge_dir/1.
145-147: Tautological assertion:maps:size(Stats) >= 0is always true.Every map has a non-negative size. This assertion doesn't validate meaningful behavior. Consider either removing it or checking for a more specific condition (e.g., expecting it to be empty or non-empty based on the test case).
Proposed fix - just verify it's a map
Stats = spectrometer_analyzer:scan_target( {github_url, "https://github.com/githubtraining/hellogitworld"} ), - ?assert(is_map(Stats)), - %% Should have found some function calls - ?assert(maps:size(Stats) >= 0) + ?assert(is_map(Stats)) end}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_analyzer_tests.erl` around lines 145 - 147, The assertion maps:size(Stats) >= 0 is tautological and should be replaced with a meaningful check: verify that Stats is a map and, depending on the test's intention, assert its expected emptiness/non-emptiness (e.g., use is_map(Stats) to confirm the type or assert maps:size(Stats) > 0 if you expect function calls to be found). Update the assertion in the test that currently references Stats to one of these checks so the test actually validates behavior.src/spectrometer_help.erl (1)
49-68: Spec/implementation mismatch for unknown command handling.The
-spec usage(command()) -> okrestricts input to thecommand()type (lines 16, 50), but lines 65-67 handle unknown commands. Dialyzer will consider the catch-all clause unreachable. If you intend to handle arbitrary atoms, broaden the spec toatom(). Otherwise, the defensive clause is fine but will trigger dialyzer warnings.Option 1: Broaden spec to accept any atom
--spec usage(command()) -> ok. +-spec usage(atom()) -> ok. usage(Command) ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_help.erl` around lines 49 - 68, The spec for usage/1 currently declares -spec usage(command()) -> ok but the function includes a catch-all clause for unknown commands; update the spec to -spec usage(atom()) -> ok so the type matches the implementation (or alternatively remove the catch-all branch and keep the original spec if you only want command() values); modify the -spec line near usage/1 and keep the usage/1 function (and its unknown-command clause) consistent with the chosen approach.test/spectrometer_atomvm_tests.erl (1)
135-137: Duplicate section comment - should reflect actual test focus.This section header says
is_supported/1 testsbut that's already the header at lines 90-92. This section actually tests platform-specific behavior. Consider renaming for clarity.Proposed fix
%% ============================================================================= -%% is_supported/1 tests +%% is_supported/1 platform-specific tests %% =============================================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/spectrometer_atomvm_tests.erl` around lines 135 - 137, The duplicate section comment currently reads "is_supported/1 tests" but this block actually contains platform-specific behavior tests; update the comment to accurately describe the section (e.g., "is_supported/1 platform-specific behavior tests" or "Platform-specific tests for is_supported/1") so readers can distinguish it from the earlier is_supported/1 tests; adjust the section header comment above the tests that exercise platform behavior accordingly.generate_fun_data.sh (1)
121-121: Portability concern:stat -cis GNU-specific.
stat -c '%i'works on Linux but not on macOS/BSD where the syntax isstat -f '%i'. Consider using a portable alternative if cross-platform support is needed.Portable alternative using test
-if [ "$(stat -c '%i' "$OUTPUT_FILE")" = "$(stat -c '%i' "$DEST_FILE" 2>/dev/null || echo "different")" ]; then +# Use cmp for content comparison instead of inode check +if [ -f "$DEST_FILE" ] && cmp -s "$OUTPUT_FILE" "$DEST_FILE"; then echo "Files are already the same, no copy needed"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate_fun_data.sh` at line 121, The portability issue is that the if condition uses GNU-specific stat -c to compare inodes; replace that check with the POSIX portable file-equivalence test using the -ef operator. Update the if condition that references OUTPUT_FILE and DEST_FILE (currently using stat -c '%i') to use: if [ "$OUTPUT_FILE" -ef "$DEST_FILE" ]; then so the script correctly detects whether the two paths refer to the same file across Linux and macOS/BSD.test/cli_main_tests.erl (1)
341-355: Move live network coverage behind an opt-in integration switch.These tests depend on GitHub, Hex, and a real AtomVM clone, so the default suite becomes slow and flaky in offline or rate-limited CI. I'd gate them with an env var/profile and keep the default path mocked or fixture-based.
Also applies to: 438-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli_main_tests.erl` around lines 341 - 355, The tests (e.g., main_filter_min_repos_test_()) call atomvm_spectrometer:main and perform live network actions (GitHub/Hex/AtomVM) via creating CacheDir and setting application env—gate these integration tests behind an opt-in switch by checking an environment variable or test tag at the top of each such test and skipping when not enabled; update main_filter_min_repos_test_() (and the other tests around the same area referenced 438-527) to early-exit or call eunit:skip/0 unless something like os:getenv("INTEGRATION_TESTS") == "1" or a test profile flag is present, and include a short comment explaining they require network access so maintainers can enable them when running integration suites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d60d113-e190-4b1b-a5fd-a0a108c1be74
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
UncleGrumpy
changed the title
UncleGrumpy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (2)
.gitignore (1)
10-16: 💤 Low valueDuplicate
erl_crash.dumpentry.Line 10 and Line 16 both ignore
erl_crash.dump. One can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 10 - 16, The .gitignore contains a duplicate entry "erl_crash.dump" (appears twice in the shown diff); remove the redundant line so "erl_crash.dump" is listed only once, leaving other entries (*.swp, *.swo, .erlang.cookie, ebin, log) unchanged.src/spectrometer_otp.erl (1)
22-26: ⚡ Quick win
is_otp_module/1triggers a full disk read + O(n) linear scan on every callEach invocation of
is_otp_module/1callsmodules_list/0, which performs: afilelib:is_file/1syscall, afile:read_file/1that reads the entire cache into memory, abinary_to_term/2decode, and then alists:member/2linear search over hundreds of module name strings. For a single check this is acceptable, but as a public API it is a trap for any call site that iterates over thousands of MFAs.Note that
spectrometer_reporter.erl(get_otp_module_set/0, line 136) already follows the correct pattern — callingmodules_list/0once and building asetsstructure — butis_otp_module/1does not benefit from this.Consider memoising the result inside
modules_list/0usingpersistent_term(written once on first call, zero-copy reads thereafter):modules_list() -> case persistent_term:get({?MODULE, otp_modules}, undefined) of undefined -> Mods = do_load_modules_list(), persistent_term:put({?MODULE, otp_modules}, Mods), Mods; Mods -> Mods end.This makes repeated
is_otp_module/1calls effectively free after the first call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spectrometer_otp.erl` around lines 22 - 26, is_otp_module/1 currently calls modules_list/0 on every invocation causing heavy disk reads and linear scans; change modules_list/0 to memoise its result in persistent_term under a unique key like {?MODULE, otp_modules} so the module list is loaded once (call your existing do_load_modules_list() to produce the list on first access), persistent_term:put it, and return the cached value on subsequent calls; optionally also build and cache a set (used by get_otp_module_set/0 or for fast membership checks) but the minimal fix is to store the list in persistent_term so is_otp_module/1 and other callers become cheap reads thereafter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@generate_fun_data.sh`:
- Around line 124-126: The backup log and actual filename use two different
timestamp formats causing mismatch and possible overwrites; standardize to a
single timestamp variable and use it in both places: compute a TIMESTAMP="$(date
+%Y%m%d%H%M)" once, then use that variable in the echo and in the mv that
references DEST_FILE so both the printed backup name and the actual moved
filename match (references: DEST_FILE, the echo line and the mv command).
- Around line 56-58: The script reuses TMP_CACHE_DIR and sets OUTPUT_FILE
without clearing prior runs, allowing stale entries to be merged by
update_datafile/2; to fix, ensure the temp cache is cleaned before creating it
by removing any existing "${TMP_CACHE_DIR}" or at least removing/truncating
"${OUTPUT_FILE}" before mkdir -p and assigning OUTPUT_FILE (reference
TMP_CACHE_DIR and OUTPUT_FILE and the merge behavior of update_datafile/2) so
each run starts from a clean slate.
- Around line 60-82: The current construct using "git checkout ... &&
\"$SPECTROMETER\" update ... || { echo 'Warning: Could not checkout ...'; }"
swallows failures from the "$SPECTROMETER" update step; change the flow to run
git checkout and detect its failure separately from running "$SPECTROMETER"
update so that update failures cause the script to stop or report an error.
Concretely, run git checkout "branch" and if it fails print the checkout warning
and continue, otherwise run "$SPECTROMETER" update with explicit error handling
(exit non-zero or print a distinct error and exit) so that failures in the
update command (invocations of "$SPECTROMETER" update) are not treated as
checkout failures; reference the git checkout commands and the "$SPECTROMETER"
update invocations in the script when making this change.
In `@src/spectrometer_atomvm.erl`:
- Around line 39-45: Update the moduledoc paths and typo to match the actual
loader and helper functions: change occurrences of "atomvm_spectrometer" in the
three example cache paths to "spectrometer" so they match
spectrometer_utils:user_db_file/0, and correct the function name typo from
spectrometer_upater:update_datafile/2 to spectrometer_updater:update_datafile/2
(or whatever the actual exported name in the updater module is) so the
documentation points to the real function.
- Around line 118-134: The find_arity/3 function doesn't handle entries where
the arity field is the atom all, so wildcard arity rows are treated as
unsupported; add clauses in find_arity/3 to match when the second element is the
atom all (e.g., change or add a clause matching [{_, all, Platforms, Since} |
Rest], Arity, _Acc and return {Platforms, Since}) or when the first clause's A
is the atom all (for consistency), ensuring that any entry with arity = all is
treated as a match regardless of the concrete Arity being searched for.
In `@src/spectrometer_ecosystem.erl`:
- Around line 170-175: The coordinator currently discards monitor refs from
spawn_monitor in the worker creation loop, so coordinator_loop cannot receive
'DOWN' when workers crash; either replace spawn_monitor with plain spawn in the
lists:foreach call that spawns worker_loop(CoordPid) to let workers report via
{worker_done, _}, or collect and store monitor refs returned by spawn_monitor in
the coordinator's state (e.g., active_workers/monitors) and add handling for
{'DOWN', MonitorRef, _, Pid, Reason} in coordinator_loop to decrement/cleanup
active_workers when a worker dies; update the spawn call site (where
spawn_monitor(fun() -> worker_loop(CoordPid) end) is used) and the
coordinator_loop message handling accordingly.
In `@src/spectrometer_http.erl`:
- Around line 472-490: The ssl_verify_fun currently accepts any {bad_cert,
hostname_check_failed} unconditionally which disables hostname verification;
remove that top-level clause and instead handle hostname failures only inside
the detailed case branch in ssl_verify_fun/3: use the existing pattern matching
branch that calls is_valid_wildcard_match(Req, Recv) and only return {valid,
State} when that function returns true and the expected hostname from State (or
a configured allowlist) matches; otherwise return {fail, Reason}. Alternatively,
rely on OTP's built-in hostname verification by not overriding hostname errors
or by comparing the expected hostname stored in State against Req before
accepting wildcard matches.
In `@src/spectrometer_otp.erl`:
- Around line 41-42: Replace the unsafe decode and unconditional binding with a
safe decode and type check: call binary_to_term(Bin, [safe]) instead of
binary_to_term(Bin), then validate the decoded value (the variable currently
named Modules) is a list before returning it; if it's not a list (or decoding
fails), return a safe fallback (e.g., empty list or an error tuple) so callers
like spectrometer_reporter that expect a list won't crash. Ensure you update the
try/of block that currently reads "try binary_to_term(Bin) of Modules ->
Modules" to perform the safe decode and list guard.
- Around line 41-50: The try/catch around binary_to_term(Bin) currently uses
catch _ which only matches throws, so binary_to_term's error:badarg will escape;
update the catch in the modules_list (the block using binary_to_term(Bin)) to
catch all exception classes (use the catch pattern matching class:reason, e.g.
catch _:_ ) so the warning, fallback io:format and generated module list ([M ||
{M, _, _} <- code:all_available()]) run on decode failure.
In `@src/spectrometer_utils.erl`:
- Around line 317-328: The fetch and checkout Git calls use
run_git_command(["fetch", ...], []) and run_git_command(["checkout", TagStr],
[]) but don't set the cloned repo working directory (TmpDir), so they run in the
wrong CWD; update those calls to run inside TmpDir by either adding the "-C",
TmpDir args (e.g., ["-C", TmpDir, "fetch", "--tags", "--quiet"] and ["-C",
TmpDir, "checkout", "--quiet", TagStr]) or pass the directory via the open_port
option list (e.g., include {cd, TmpDir} in the second argument to
run_git_command), ensuring run_git_command, TagStr and TmpDir are used to locate
and run the commands in the cloned repo.
In `@test/cli_main_tests.erl`:
- Around line 530-542: The cleanup misses removing the cloned AtomVM repo and
mistakenly purges filename:basename(OutputFile); capture the AtomVM clone path
returned by ensure_atomvm_repo/0 in the test setup (e.g., AtomVMRepoDir) and in
the after block call spectrometer_utils:purge_dir(AtomVMRepoDir) to remove the
checkout, replace spectrometer_utils:purge_dir(filename:basename(OutputFile))
with spectrometer_utils:purge_dir(OutputFile) (or the correct temp directory
path you created for OutputFile) so you delete the actual file/path, and keep
purging CacheDir as before.
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 17-188: The tests call unexported functions scan_target/1 and
merge_stats/2 in module spectrometer_analyzer, so either add scan_target/1 and
merge_stats/2 to the module's -export([...]) list in spectrometer_analyzer.erl
(ensuring the exported names exactly match those symbols) or modify the tests to
exercise the public API (e.g., call audit/1 or filter/1) instead of calling
these internal functions; choose one approach and apply it consistently so tests
no longer trigger undef errors.
In `@test/spectrometer_scanner_tests.erl`:
- Around line 138-180: The tests call the unexported function merge_file_calls/2
on module spectrometer_scanner, which will cause undef at runtime; fix by either
exporting merge_file_calls/2 from spectrometer_scanner (add it to the module's
-export([...]) so the test can call it, keeping its implementation and -doc
false if desired) or remove/replace the unit tests that directly call
merge_file_calls/2 and instead cover behavior via the public scan_directory/1
API.
In `@test/spectrometer_updater_tests.erl`:
- Around line 17-99: The tests call unexported functions derive_since/2 and
is_older_since/2 in module spectrometer_updater which will cause undef errors;
fix by adding these functions to the module export list (e.g. include
derive_since/2 and is_older_since/2 alongside scan_atomvm_repo/3 and
update_datafile/2 in the -export attribute in src/spectrometer_updater.erl) or
alternatively change the tests to exercise equivalent behavior via the exported
APIs (scan_atomvm_repo/3 or update_datafile/2) so no private functions are
invoked directly.
- Around line 255-331: Tests call the unexported function write_db_file/2 in
module spectrometer_updater which will cause undefined function errors; either
export write_db_file/2 from spectrometer_updater (add it to the -export([...])
list) or change the tests to exercise an exported API such as update_datafile/2
or scan_atomvm_repo/3 that delegates to write_db_file/2; locate write_db_file/2
in spectrometer_updater and either add it to the module export list or update
the tests in spectrometer_updater_tests.erl to call an exported wrapper.
---
Nitpick comments:
In @.gitignore:
- Around line 10-16: The .gitignore contains a duplicate entry "erl_crash.dump"
(appears twice in the shown diff); remove the redundant line so "erl_crash.dump"
is listed only once, leaving other entries (*.swp, *.swo, .erlang.cookie, ebin,
log) unchanged.
In `@src/spectrometer_otp.erl`:
- Around line 22-26: is_otp_module/1 currently calls modules_list/0 on every
invocation causing heavy disk reads and linear scans; change modules_list/0 to
memoise its result in persistent_term under a unique key like {?MODULE,
otp_modules} so the module list is loaded once (call your existing
do_load_modules_list() to produce the list on first access), persistent_term:put
it, and return the cached value on subsequent calls; optionally also build and
cache a set (used by get_otp_module_set/0 or for fast membership checks) but the
minimal fix is to store the list in persistent_term so is_otp_module/1 and other
callers become cheap reads thereafter.
🪄 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: fb4967d0-06dd-4728-9a2f-ecb7bfd94d9e
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (9)
- src/spectrometer.erl
- test/spectrometer_utils_tests.erl
- include/ecosystem.hrl
- src/spectrometer.app.src
- rebar.lock.license
- test/spectrometer_atomvm_tests.erl
- src/spectrometer_updater.erl
- src/spectrometer_scanner.erl
- test/spectrometer_reporter_tests.erl
🚧 Files skipped from review as they are similar to previous changes (5)
- rebar.config
- src/spectrometer_reporter.erl
- test/atomvm_spectrometer_tests.erl
- src/spectrometer_analyzer.erl
- src/spectrometer_help.erl
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
generate_fun_data.sh (3)
129-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse one shared timestamp for backup log and
mv.Line 129 uses
%M(minutes) while Line 130 uses%m(month). This can create mismatched names and overwrite risk. Compute oneBACKUP_TSvariable and reuse it in both lines.🤖 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 `@generate_fun_data.sh` around lines 129 - 130, The echo and mv use different date formats causing mismatched backup names; fix by computing a single timestamp variable (e.g., BACKUP_TS) once before the echo/mv and reuse it in both commands so both the log and the actual backup filename match; update the echo that references ${DEST_FILE} and the mv that moves "${DEST_FILE}" to "${DEST_FILE}.${BACKUP_TS}.bak", ensuring the timestamp uses the correct date format (e.g., date +%Y%m%d%H%M) and that DEST_FILE remains the same symbol used in the diff.
56-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStart with a clean temp cache/output before scanning.
Line 57 creates the cache dir, but stale files from interrupted previous runs can be merged into the new dataset. Clean
"$TMP_CACHE_DIR"(or at least truncate"$OUTPUT_FILE") before scanning.🤖 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 `@generate_fun_data.sh` around lines 56 - 58, The script currently creates the cache dir but doesn't clear stale data; before scanning, ensure TMP_CACHE_DIR is cleaned or OUTPUT_FILE truncated to avoid merging old results—e.g., remove existing files under "${TMP_CACHE_DIR}" (or at minimum truncate "${OUTPUT_FILE}") prior to creating the dir and writing; use the TMP_CACHE_DIR and OUTPUT_FILE variables so the cleanup targets the same paths used later in the script and keep the mkdir -p call to recreate the directory if needed.
62-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t use
checkout && update || warningfor control flow here.Lines 62-69 and 79-86 can execute the warning branch when
updatefails, which masks real scan failures and may leave a partial output. Split checkout and update into explicitifblocks so update errors are handled distinctly.#!/bin/bash # Verify problematic SC2015 patterns are still present rg -n -C2 'git checkout.*&&|"\$SPECTROMETER" update|Could not checkout' generate_fun_data.shAlso applies to: 79-86
🤖 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 `@generate_fun_data.sh` around lines 62 - 69, The current combined "git checkout ... && \"$SPECTROMETER\" update ... || { echo 'Warning: Could not checkout main'; }" control flow can misattribute failures from the update command as checkout failures and leave partial outputs; in generate_fun_data.sh split the steps: run git checkout separately and if it fails log a clear warning and continue or exit as intended, then run "$SPECTROMETER" update (using ATOMVM_DIR, TMP_CACHE_DIR, OUTPUT_FILE, --branch main, --force, --no-tests) in its own if/then/else so update failures are detected and handled distinctly (log an error and exit non-zero), and ensure you clean up or remove OUTPUT_FILE on update failure to avoid leaving partial output.src/spectrometer_http.erl (1)
472-475:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftSSL hostname verification is still unconditionally bypassed.
This top-level clause continues to return
{valid, State}for any{bad_cert, hostname_check_failed}, which masks the more careful wildcard logic added in the second clause and disables hostname verification across every TLS connection this module makes. As previously noted, this should either be removed (so the detailed{requested, _}/{received, _}clause runs and gates acceptance onis_valid_wildcard_match/2) or replaced entirely by OTP's built-in hostname check.🤖 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_http.erl` around lines 472 - 475, Remove or modify the top-level ssl_verify_fun clause that unconditionally returns {valid, State} for {bad_cert, hostname_check_failed} so hostname verification is not bypassed; either delete that clause so the more specific clause (the one that matches {bad_cert, hostname_check_failed, {requested, _}} / {received, _}} and calls is_valid_wildcard_match/2) runs, or replace it to call :ssl_verify_hostname/2 (OTP built-in) and only accept when that returns ok, ensuring ssl_verify_fun defers to is_valid_wildcard_match/2 or OTP hostname checks rather than always returning {valid, State}.
🧹 Nitpick comments (2)
src/spectrometer_help.erl (1)
51-68: ⚡ Quick winAlign
usage/1typespec with its fallback behavior.Line 51 declares
usage(command()), but Lines 66-68 intentionally handle unknown inputs. Please widen the spec (e.g.,usage(term()) -> ok.orusage(command() | term()) -> ok.) so Dialyzer/docs match actual behavior.🤖 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_help.erl` around lines 51 - 68, The typespec for usage/1 is too narrow (currently spec'ed as usage(command()) -> ok) but the function handles unknown inputs in the catch-all clause, so widen the spec to reflect that (for example usage(term()) -> ok or usage(command() | term()) -> ok) so Dialyzer and docs match the actual behavior; update the -spec declaration for usage/1 accordingly while keeping the implementation of usage/1 (and its fallback clause) unchanged.src/spectrometer_http.erl (1)
434-450: ⚡ Quick winReplace hard-coded CA bundle paths with
public_key:cacerts_get/0.The hard-coded paths are fragile across distributions (RHEL/Fedora use
/etc/pki/tls/certs/ca-bundle.crt, Alpine uses/etc/ssl/cert.pem, the Windows path is non-standard, and the macOS Homebrew path only exists on Intel installs). On any system where the file is missing,cacertfilewill silently produce a TLS handshake failure. OTP 25+ providespublic_key:cacerts_get/0, which loads the OS trust store and is portable; pair it with{cacerts, Certs}instead of{cacertfile, Path}.♻️ Suggested replacement
-ssl_options(Hostname) -> - [ - {verify, verify_peer}, - {cacertfile, get_ca_certfile()}, - {depth, 3}, - {server_name_indication, Hostname}, - {verify_fun, {fun ssl_verify_fun/3, Hostname}} - ]. +ssl_options(Hostname) -> + [ + {verify, verify_peer}, + {cacerts, public_key:cacerts_get()}, + {depth, 3}, + {server_name_indication, Hostname}, + {customize_hostname_check, + [{match_fun, public_key:pkix_verify_hostname_match_fun(https)}]} + ].
get_ca_certfile/0and the customssl_verify_fun/3then become removable, since OTP's built-in hostname verification handles standard wildcards correctly.🤖 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_http.erl` around lines 434 - 450, The get_ca_certfile/0 function currently returns hard-coded platform CA bundle paths which are fragile; replace its usage by calling public_key:cacerts_get/0 to obtain the system CA certs and pass them in the SSL options as {cacerts, Certs} instead of {cacertfile, Path}; after switching to {cacerts, Certs} remove get_ca_certfile/0 and the custom ssl_verify_fun/3 (rely on OTP's built-in hostname verification) and update any call sites that construct ssl options to use the Certs value from public_key:cacerts_get/0.
🤖 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 `@src/spectrometer_http.erl`:
- Around line 273-286: download_github_repo currently calls open_port with the
result of os:find_executable("git") which can be false and causes a badarg
crash; update download_github_repo to check GitPath = os:find_executable("git")
and if it is false return a clean error tuple (for example {error,
{git_not_found}} or similar) instead of proceeding to open_port, otherwise
continue with the existing open_port/await_git_port flow so callers like
spectrometer_analyzer:scan_target/1 receive a proper {error, Reason}.
- Around line 290-293: The await_git_port/1 function currently blocks forever on
a hung git subprocess; modify await_git_port(Port) to include a receive after
timeout clause (e.g. a sensible ms value), where on timeout you call
port_close(Port), then attempt to drain any pending {Port, {exit_status, _}}
messages before returning {error, clone_timeout}; update callers such as
download_github_repo/2 to translate that timeout result into {error,
clone_timeout} so the caller can handle the failure.
- Around line 494-505: The wildcard matcher in is_valid_wildcard_match
incorrectly trims the leading "." making Suffix "github.com" which matches
unrelated hosts; keep the leading dot (Suffix should be ".github.com") and
enforce the wildcard only spans a single DNS label by verifying Req ends with
that Suffix, the prefix (string:substr(Req, 1, length(Req) - length(Suffix))) is
non-empty, and that this prefix contains no '.' characters; update the checks
around Pattern, Suffix and Req accordingly in is_valid_wildcard_match to
implement these conditions.
- Around line 197-225: Replace the logic that infers LatestVersion from the
Releases array inside the lists:filtermap fun (the mapping that builds Packages
from Items) to instead read latest_version directly from the Item map: use
maps:get(<<"latest_version">>, Item, <<>>) (convert to a string with
binary_to_list like other fields) and remove the Releases / case on Releases /
first-element logic; keep the rest (Name, Meta, Links, GithubUrl) and the final
filtermap return structure unchanged so the tuple uses the new LatestVersion
value when non-empty.
In `@src/spectrometer_otp.erl`:
- Around line 50-61: modules_list/0 currently returns atoms from the three list
comprehensions ([M || {M, _, _} <- code:all_available()]) but the spec and
callers expect strings; update all three comprehensions inside modules_list/0 to
convert each atom M to a string (e.g., use atom_to_list(M) or equivalent) so the
returned list elements are strings, leaving the spec as -spec modules_list() ->
[string()].
In `@src/spectrometer_reporter.erl`:
- Around line 231-261: print_summary/3's false branch is crashing because it
treats Supported as a map while generate_report stores Supported as a list of
{Key, Count}; either (A) call the existing true branch that already handles
generate_report output (replace print_summary(Report, TopN, false) to delegate
to the true branch), or (B) adapt the false branch and sort_stats/1 to accept
the list shape: stop calling maps:to_list/maps:size on Supported, change
sort_stats/1 to sort the list by count (e.g. sort by the second element), and
update the foldl pattern in print_summary (currently
fun({{Mod,Fun,Arity},{TotalCalls,RepoCount}},Idx) -> ...) to match {Key, Count}
items and use length(Results) instead of maps:size(Supported) for the total
count; make these changes to the functions print_summary/3 and sort_stats/1 so
types align with generate_report's output.
In `@TODO.md`:
- Around line 25-27: Fix the wording typo in the TODO note: locate the sentence
that currently reads "will case noticeable errors" in the TODO.md content (the
sentence discussing brittleness when comparing versions in the `update` command)
and change "case" to "cause" so it correctly reads "will cause noticeable
errors".
---
Duplicate comments:
In `@generate_fun_data.sh`:
- Around line 129-130: The echo and mv use different date formats causing
mismatched backup names; fix by computing a single timestamp variable (e.g.,
BACKUP_TS) once before the echo/mv and reuse it in both commands so both the log
and the actual backup filename match; update the echo that references
${DEST_FILE} and the mv that moves "${DEST_FILE}" to
"${DEST_FILE}.${BACKUP_TS}.bak", ensuring the timestamp uses the correct date
format (e.g., date +%Y%m%d%H%M) and that DEST_FILE remains the same symbol used
in the diff.
- Around line 56-58: The script currently creates the cache dir but doesn't
clear stale data; before scanning, ensure TMP_CACHE_DIR is cleaned or
OUTPUT_FILE truncated to avoid merging old results—e.g., remove existing files
under "${TMP_CACHE_DIR}" (or at minimum truncate "${OUTPUT_FILE}") prior to
creating the dir and writing; use the TMP_CACHE_DIR and OUTPUT_FILE variables so
the cleanup targets the same paths used later in the script and keep the mkdir
-p call to recreate the directory if needed.
- Around line 62-69: The current combined "git checkout ... && \"$SPECTROMETER\"
update ... || { echo 'Warning: Could not checkout main'; }" control flow can
misattribute failures from the update command as checkout failures and leave
partial outputs; in generate_fun_data.sh split the steps: run git checkout
separately and if it fails log a clear warning and continue or exit as intended,
then run "$SPECTROMETER" update (using ATOMVM_DIR, TMP_CACHE_DIR, OUTPUT_FILE,
--branch main, --force, --no-tests) in its own if/then/else so update failures
are detected and handled distinctly (log an error and exit non-zero), and ensure
you clean up or remove OUTPUT_FILE on update failure to avoid leaving partial
output.
In `@src/spectrometer_http.erl`:
- Around line 472-475: Remove or modify the top-level ssl_verify_fun clause that
unconditionally returns {valid, State} for {bad_cert, hostname_check_failed} so
hostname verification is not bypassed; either delete that clause so the more
specific clause (the one that matches {bad_cert, hostname_check_failed,
{requested, _}} / {received, _}} and calls is_valid_wildcard_match/2) runs, or
replace it to call :ssl_verify_hostname/2 (OTP built-in) and only accept when
that returns ok, ensuring ssl_verify_fun defers to is_valid_wildcard_match/2 or
OTP hostname checks rather than always returning {valid, State}.
---
Nitpick comments:
In `@src/spectrometer_help.erl`:
- Around line 51-68: The typespec for usage/1 is too narrow (currently spec'ed
as usage(command()) -> ok) but the function handles unknown inputs in the
catch-all clause, so widen the spec to reflect that (for example usage(term())
-> ok or usage(command() | term()) -> ok) so Dialyzer and docs match the actual
behavior; update the -spec declaration for usage/1 accordingly while keeping the
implementation of usage/1 (and its fallback clause) unchanged.
In `@src/spectrometer_http.erl`:
- Around line 434-450: The get_ca_certfile/0 function currently returns
hard-coded platform CA bundle paths which are fragile; replace its usage by
calling public_key:cacerts_get/0 to obtain the system CA certs and pass them in
the SSL options as {cacerts, Certs} instead of {cacertfile, Path}; after
switching to {cacerts, Certs} remove get_ca_certfile/0 and the custom
ssl_verify_fun/3 (rely on OTP's built-in hostname verification) and update any
call sites that construct ssl options to use the Certs value from
public_key:cacerts_get/0.
🪄 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: 1db8f313-af89-4fb2-9b21-5b9d0dfe1045
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (8)
- rebar.lock.license
- include/ecosystem.hrl
- test/spectrometer_utils_tests.erl
- src/spectrometer.app.src
- rebar.config
- src/spectrometer_atomvm.erl
- src/spectrometer_analyzer.erl
- test/atomvm_spectrometer_tests.erl
🚧 Files skipped from review as they are similar to previous changes (13)
- src/spectrometer.erl
- test/spectrometer_scanner_tests.erl
- src/spectrometer_scanner.erl
- test/spectrometer_http_tests.erl
- src/spectrometer_ecosystem.erl
- src/atomvm_spectrometer.erl
- test/spectrometer_analyzer_tests.erl
- test/cli_main_tests.erl
- test/spectrometer_atomvm_tests.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_updater.erl
- src/spectrometer_utils.erl
- .gitignore
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
src/spectrometer_otp.erl (1)
41-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
[safe]and validate decoded type.Two small hardening concerns at the cache decode:
binary_to_term(Bin)without[safe]allows a corrupted/tampered cache to create new atoms, contributing to atom-table exhaustion. The cache lives under the user's cache dir, but bit-flips or stale formats from older versions can also inject unknown atoms.- The
of Modules -> Modulesbranch is unconditional. If the file decodes to a non-list (e.g., older format),is_otp_module/1'slists:member/2will crash at the call site rather than fall back tocode:all_available/0.🛡️ Proposed fix
- try binary_to_term(Bin) of - Modules -> Modules + try binary_to_term(Bin, [safe]) of + Modules when is_list(Modules) -> Modules; + _ -> + io:format( + "Warning: unexpected data in OTP module cache file ~s\n", + [ModFile] + ), + io:format("Using generated module list...\n"), + [M || {M, _, _} <- code:all_available()] catch _:_ ->🤖 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 41 - 51, Use binary_to_term(Bin, [safe]) and validate the decoded value before returning it: call binary_to_term(Bin, [safe]) in the try and ensure the result is the expected list of module names (or convert a list-of-tuples like [{M,_,_}|...] to [M|..]) by checking is_list(Modules) and element types (is_atom/1 or tuple shape) and only return Modules if validation passes; otherwise log the warning (using ModFile) and fall back to generating the module list via code:all_available(); this prevents atom injection and avoids crashing callers like is_otp_module/1 when the cache contains a non-list.
🧹 Nitpick comments (24)
src/spectrometer_http.erl (2)
429-438: 💤 Low valueUse
$\\instead of the magic number92.Line 434 uses ASCII 92 to denote a backslash.
$\\is the idiomatic Erlang character literal and reads more clearly next to the other$-prefixed character literals in the same function.♻️ Proposed fix
is_windows_absolute_path(Path) -> case Path of [Drive, $:] when Drive >= $A, Drive =< $Z -> true; [Drive, $:] when Drive >= $a, Drive =< $z -> true; - % backslash after drive colon - [_, $:, 92 | _] -> true; + % backslash after drive colon + [_, $:, $\\ | _] -> true; % forward slash after drive colon [_, $:, $/ | _] -> true; _ -> false 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_http.erl` around lines 429 - 438, The is_windows_absolute_path function uses the magic number 92 to represent a backslash; replace that numeric literal with the idiomatic Erlang character literal $\\ (so the pattern [_, $:, 92 | _] becomes [_, $:, $\\ | _]) to match the style of the other $-prefixed character literals and improve readability.
83-88: 💤 Low value
star_filter_range/2mishandlesMinStars =:= infinityonce a cursor has advanced.The first clause (line 83) protects only
(infinity, undefined). Once any batch returns and aLastStarsinteger is established, the third clause (line 87) executesio_lib:format("~p..~p", [infinity, N]), which yields"infinity..N"— not a valid GitHub query. The current callers infetch_github_repos/1always pass an integerMinStars, so this is latent rather than active, but the(infinity, undefined)clause implies infinity is intended to be supported.♻️ Suggested clause
star_filter_range(infinity, undefined) -> ">=1"; +star_filter_range(infinity, LastStars) when is_integer(LastStars) -> + io_lib:format("1..~p", [LastStars]); star_filter_range(MinStars, undefined) -> io_lib:format(">=~p", [MinStars]); star_filter_range(MinStars, LastStars) -> io_lib:format("~p..~p", [MinStars, LastStars]).🤖 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_http.erl` around lines 83 - 88, star_filter_range/2 currently only handles the (infinity, undefined) case and falls through to produce "infinity..N" when LastStars is set; add a clause to handle MinStars = infinity with a concrete LastStars (e.g., produce a "<=N" style filter) and place it before the general range clause so star_filter_range(infinity, LastStars) returns a valid GitHub query instead of "infinity..N"; update the existing clauses in star_filter_range/2 accordingly.include/ecosystem.hrl (1)
9-9: 💤 Low valueConsider adding an include guard to the header.
If this header gets transitively included more than once in any compilation unit,
?ECOSYSTEM_STATEwill trigger a redefinition warning. Wrap the macro definition with-ifndef/-define/-endifto make the header safe under repeated inclusion.♻️ Proposed include guard
-%% SPDX-License-Identifier: Apache-2.0 -define(ECOSYSTEM_STATE, "beam_ecosystem.bin"). +%% SPDX-License-Identifier: Apache-2.0 +-ifndef(ECOSYSTEM_HRL). +-define(ECOSYSTEM_HRL, true). +-define(ECOSYSTEM_STATE, "beam_ecosystem.bin"). +-endif.🤖 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 `@include/ecosystem.hrl` at line 9, Wrap the macro definition with an include guard: check -ifndef ECOSYSTEM_STATE before the -define and close with -endif so that the existing -define(ECOSYSTEM_STATE, "beam_ecosystem.bin"). is only declared once; reference the macro ?ECOSYSTEM_STATE (and the -define(ECOSYSTEM_STATE, ...) line) when making the change to ensure the header is safe under repeated inclusion.src/spectrometer_utils.erl (1)
419-425: 💤 Low value
os_temp_dir/0falls back to a non-standard Windows path.
"C:/temp"is not a standard Windows temp directory and is unlikely to exist on most installs. The conventional fallback when neitherTEMPnorTMPis set isC:\Windows\Temp(or%LOCALAPPDATA%\Temp). In practice TEMP/TMP are almost always set, so this only manifests in stripped environments — but worth fixing.🤖 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 419 - 425, The os_temp_dir/0 function currently returns a non-standard Windows path; update os_temp_dir/0 to first prefer Windows environment variables (check os:getenv("TEMP") and os:getenv("TMP") and use whichever is set), and only if both are unset fall back to the standard Windows temp location "C:/Windows/Temp" for the {win32, _} branch (leave the default "/tmp" for non-Windows). Ensure the function returns the chosen string consistently.src/spectrometer_scanner.erl (1)
24-46: ⚡ Quick winAdd a
-specfor the only exported function.
scan_directory/1is the public API of this module but lacks a-spec. Adding one helps Dialyzer (which the PR description states is clean) keep callers honest as the return shape evolves.♻️ Proposed spec
-export([scan_directory/1]). -include_lib("kernel/include/file.hrl"). -doc """ ... """. +-spec scan_directory(file:filename_all()) -> + #{{module(), atom(), arity()} => non_neg_integer()}. scan_directory(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 `@src/spectrometer_scanner.erl` around lines 24 - 46, Add a Dialyzer spec for the exported function scan_directory/1: declare the argument as a string() (Dir :: string()) and the return as a map whose keys are {module(), atom(), non_neg_integer()} tuples and whose values are non_neg_integer() counts (i.e. #{ {module(), atom(), non_neg_integer()} => non_neg_integer() }). Add this -spec immediately above the scan_directory/1 implementation to make the public API explicit for callers and static analysis.src/spectrometer_help.erl (1)
74-74: 💤 Low valueStray quoted strings in
%%comments look like leftover doc attributes.Lines 74, 110, 139, 174, 196, 219, and 251 each contain a comment of the form
%% "Print help text for the ... command."with quotes. These read like-docstrings that were converted to comments without removing the surrounding quotes. Either restore proper-docattributes or remove the quotes for plain comments — both are more idiomatic than the current quoted-comment hybrid.Also applies to: 110-110, 139-139, 174-174, 196-196, 219-219, 251-251
🤖 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_help.erl` at line 74, In spectrometer_help.erl there are several comment lines like %% "Print help text for the `audit` command." (lines for audit and other commands) that use quoted strings inside %% comments; change them to idiomatic comments by removing the surrounding double quotes (e.g., %% Print help text for the `audit` command.) or, if they were intended as documentation attributes, convert them to proper -doc or -spec attributes; update each occurrence referencing the same quoted-comment pattern (e.g., the help comment for audit and the other command help comments) so all quoted strings are either unquoted plain comments or real -doc attributes.src/spectrometer_updater.erl (1)
386-392: 💤 Low value
merge_platforms_all/2doesn't collapse toallwhen the union covers every platform.
merge_platforms/2(single-platform variant) collapses a complete list to the atomall(lines 530–542), but the list/list case inmerge_platforms_all/2doesn't. The merged DB will then carry verbose[emscripten, esp32, generic_unix, rp2, stm32]entries instead ofalleven though they're equivalent — slight inconsistency in the on-disk format.♻️ Proposed fix
merge_platforms_all(OldList, NewList) when is_list(OldList), is_list(NewList) -> - lists:umerge(lists:sort(OldList), lists:sort(NewList)). + Merged = lists:umerge(lists:sort(OldList), lists:sort(NewList)), + case Merged of + ?ALL_PLATFORMS -> all; + _ -> Merged + 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_updater.erl` around lines 386 - 392, The list/list clause in merge_platforms_all/2 currently returns the sorted union but never collapses to the atom all; update merge_platforms_all/2 to mirror merge_platforms/2 by computing the union of OldList and NewList, sorting/uniq'ing it, and if that result equals the canonical complete-platform list then return the atom all, otherwise return the merged list; reference the existing merge_platforms/2 behavior and the function name merge_platforms_all/2 when making the change and use the same canonical full-platform list (the same source used by merge_platforms/2) for the equality check.test/spectrometer_utils_tests.erl (1)
233-247: 💤 Low valueNetwork-dependent assertions without a skip gate.
Unlike the HTTP test module, these
spectrometer_http:fetch/1cases aren't gated bySKIP_NETWORK_TESTS. The non-existent domain test is robust, butlocalhost:59999may collide with a service on the runner. Consider gating both behind the same env var asspectrometer_http_testsor binding to a unique closed port (e.g., 0 then close).🤖 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_utils_tests.erl` around lines 233 - 247, The two tests in http_get_test_() that call spectrometer_http:fetch/1 should be guarded like the other network tests: check the SKIP_NETWORK_TESTS env var (or the same mechanism used by spectrometer_http_tests) and skip these cases when set, or alternatively replace the localhost:59999 case with a deterministically-closed port (bind to 0 to get an ephemeral port and immediately close it) to avoid colliding with services on CI; update http_get_test_() to perform that gating or closed-port setup around the spectrometer_http:fetch("http://localhost:59999/test") and the domain test to ensure both are network-safe.test/spectrometer_updater_tests.erl (1)
92-99: 💤 Low valueTest name "two unreleased are never older" is misleading.
is_older_since/2does compare two{unreleased, _}values viabranch_sort_key/1, so for some pairs it returnstrue(e.g.,{unreleased, <<"0.7.x">>}vs{unreleased, <<"main">>}). The single assertion here happens to befalsebecausebranch_sort_key(<<"main">>) > branch_sort_key(<<"0.7.x">>). Either rename the test (e.g. "main is newer than versioned branches") or add the symmetric assertion to actually exercise the "never older" claim — otherwise a future change to ordering could regress silently.🤖 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 92 - 99, The test is_older_since_both_unreleased_test_ currently asserts only that is_older_since({unreleased, <<"main">>}, {unreleased, <<"0.7.x">>}) is false, but the comment notes that is_older_since/2 uses branch_sort_key/1 and some unreleased pairs can return true; fix by either renaming the test to accurately reflect the specific case (e.g., "main is newer than versioned branches") or expand the test to assert symmetry/never-older behavior by adding the complementary assertion(s) such as checking is_older_since({unreleased, <<"0.7.x">>}, {unreleased, <<"main">>}) and/or other pair(s) to exercise branch_sort_key/1 ordering so the intended "never older" claim is actually tested.src/spectrometer_analyzer.erl (1)
285-294: 💤 Low value
parse_target_linessilently treats any non-github.com, non-directory line as a GitHub URL.Lines that aren't comments, blank, prefixed with
hex:, an existing local directory, or that containgithub.comstill fall through to{github_url, Url}. A typo or junk line becomes a (failed) clone attempt with a confusing error rather than an upfront input error.♻️ Possible improvement
Url -> case string:find(Url, "github.com") of nomatch -> case filelib:is_dir(Url) of true -> {true, {local_dir, Url}}; - false -> {true, {github_url, Url}} + false -> + %% Treat short forms like "user/repo" as GitHub, + %% otherwise warn and skip. + case string:split(Url, "/", all) of + [_Owner, _Repo | _] -> {true, {github_url, Url}}; + _ -> + io:format( + standard_error, + "Warning: ignoring unrecognized target: ~ts\n", + [Url] + ), + false + end end; _ -> {true, {github_url, Url}} 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_analyzer.erl` around lines 285 - 294, parse_target_lines currently assumes any non-github.com, non-directory string is a GitHub URL which causes confusing clone failures; update the Url handling in parse_target_lines to validate the input before returning {github_url, Url} — specifically, after checking string:find(Url, "github.com") and filelib:is_dir(Url) add a validation step (e.g., ensure Url matches expected URL/SSH patterns or has http/https/ssh prefix) and return a clear error tuple (for example {false, {invalid_target, Url}}) for anything that doesn't pass validation, keeping the existing branches for {local_dir, Url} and {github_url, Url} only for confirmed valid inputs.generate_fun_data.sh (4)
22-22: 💤 Low valueHardcoded
/tmpignoresTMPDIR.
TMP_CACHE_DIR="/tmp/spectrometer_version_cache"bypasses$TMPDIR, which is the standard way users (and macOS) override temp locations. Concurrent runs by multiple users on the same host will also collide on this fixed path. Consider"${TMPDIR:-/tmp}/spectrometer_version_cache.$$"ormktemp -dfor isolation.🤖 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 `@generate_fun_data.sh` at line 22, TMP_CACHE_DIR is hardcoded to /tmp which ignores $TMPDIR and can cause cross-user collisions; change the assignment that sets TMP_CACHE_DIR to use a TMPDIR-aware, unique temp directory (e.g. use "${TMPDIR:-/tmp}/spectrometer_version_cache.$$" or create one via mktemp -d) and ensure the rest of the script (references to TMP_CACHE_DIR) handles cleanup on exit; update the TMP_CACHE_DIR assignment in generate_fun_data.sh accordingly and add trap cleanup if using mktemp -d.
78-78: 💤 Low value
grepsubstring match may misidentify branches.
git branch -r | grep "release-0.7"will match any remote branch whose name containsrelease-0.7(e.g.,origin/foo-release-0.7-bar). Use a fixed/anchored pattern such asgrep -Fx " origin/release-0.7"orgrep -E '^\s+origin/release-0\.7$'(or, more robustly,git show-ref --verify --quiet refs/remotes/origin/release-0.7).🤖 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 `@generate_fun_data.sh` at line 78, The check `git branch -r | grep "release-0.7"` can false-positive on substring matches; replace it with a precise existence check: either use an anchored/whole-line grep (e.g. matching "origin/release-0.7" exactly) or, preferably, use Git's native verify command `git show-ref --verify --quiet refs/remotes/origin/release-0.7` in place of the pipeline so the script only treats the specific remote branch `origin/release-0.7` as present; update the conditional that currently references `git branch -r | grep "release-0.7"` accordingly.
14-27: 💤 Low valueRedundant empty-string check on
$ATOMVM_DIR.The argument-count guard at lines 14-17 already exits when no positional argument is provided, so
$1is guaranteed to be set (and non-empty if the user supplied any non-empty token) by line 19. The[ -z "$ATOMVM_DIR" ]test at lines 24-27 is unreachable. Either drop it, or move the check earlier and remove the$#test if you intend to allow an explicit empty argument.🤖 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 `@generate_fun_data.sh` around lines 14 - 27, Remove the redundant empty-string check for ATOMVM_DIR: since the script already exits when "$#" -eq 0, drop the second conditional that tests [ -z "$ATOMVM_DIR" ] (the block referencing ATOMVM_DIR at the end) so you only keep the initial argument-count guard; ensure ATOMVM_DIR="$1" remains and subsequent code (e.g., uses of ATOMVM_DIR, SCRIPT_DIR, SPECTROMETER, TMP_CACHE_DIR) assumes a provided positional argument.
1-12: 💤 Low valueHeader references the wrong script name.
The header comment block (lines 2 and 7) calls this file
regenerate_version_data.sh, but the actual script isgenerate_fun_data.sh. The README and PR summary also referencegenerate_fun_data.sh. Update the comment to match for clarity.📝 Proposed fix
#!/bin/bash -# regenerate_version_data.sh +# generate_fun_data.sh # # Regenerates supported_functions.data with version information # by scanning each AtomVM release tag and branch. # -# Usage: ./regenerate_version_data.sh <path-to-atomvm-dir> +# Usage: ./generate_fun_data.sh <path-to-atomvm-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 `@generate_fun_data.sh` around lines 1 - 12, Update the header comment in generate_fun_data.sh so it references the correct script name: replace both occurrences of "regenerate_version_data.sh" (the header title and the Usage line) with "generate_fun_data.sh"; edit the top comment block around the shebang where the misleading name appears to ensure README/PR-consistent naming and leave the SPDX/license lines unchanged.test/spectrometer_analyzer_tests.erl (2)
145-148: 💤 Low value
?assert(maps:size(Stats) >= 0)is a tautology.
maps:size/1always returns a non‑negative integer, so this assertion never fails and adds no coverage. Either tighten it (e.g.,?assert(maps:size(Stats) > 0)if the repo is expected to contain Erlang sources, or assert membership of a known call key) or remove it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/spectrometer_analyzer_tests.erl` around lines 145 - 148, The assertion ?assert(maps:size(Stats) >= 0) is a tautology; update the test to make it meaningful by either asserting there are entries (e.g., replace with ?assert(maps:size(Stats) > 0)) or asserting a specific expected key/value exists in Stats (e.g., use maps:is_key(ExpectedKey, Stats) or equal lookup on Stats). Locate the assertion that references Stats and maps:size in the test function (the failing clause using ?assert) and replace the tautological check with one of the two stronger checks above to validate actual analyzer output.
139-188: ⚡ Quick winNetwork‑dependent tests aren't gated by
SKIP_NETWORK_TESTS.These five tests hit
github.com(clone) andhex.pm(download), but unlike the network tests intest/cli_main_tests.erl(e.g.,main_audit_github_small_repo_test_/0,main_update_with_local_repo_test_/0) they aren't wrapped in acase os:getenv("SKIP_NETWORK_TESTS") of false -> ...; _ -> {"skipped (...)", fun() -> ok end} end. Offline CI runs and contributors without network access will see spurious failures, and theatomvm_packbeam0.8.2 assertions (lines 168‑171) make the suite brittle to upstream package changes/yanks.Wrap each of these tests in the same
SKIP_NETWORK_TESTSguard used bycli_main_testsfor consistency.🤖 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_analyzer_tests.erl` around lines 139 - 188, Wrap each network-dependent test (scan_target_github_url_test_, scan_target_github_url_nonexistent_test_, scan_target_hex_package_test_, scan_target_hex_package_nonexistent_test_, scan_target_hex_latest_test_) in the same SKIP_NETWORK_TESTS guard used in cli_main_tests: use case os:getenv("SKIP_NETWORK_TESTS") of false -> <original test body> ; _ -> {"skipped (network tests disabled)", fun() -> ok end} end. While doing this, for scan_target_hex_package_test_ consider relaxing or removing the brittle specific assertions about atomvm_packbeam keys ({packbeam_api, create, 3}, etc.) so the test won’t fail if upstream Hex package contents change.src/spectrometer_ecosystem.erl (1)
100-104: 💤 Low value
try/catchonly handleserrorclass.
run/1'scatch error:Reasonletsthrowandexitescape. Anything in the (long) call chain (HTTP client, gun, ssl, scanner) that usesthrow/1orexit/1will propagate past this handler and crash the caller, whileerror/1is converted to{error, _}. Either broaden to_:Reason(with a stacktrace capture if you want to log it) or document that onlyerrorexceptions are wrapped.🤖 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.erl` around lines 100 - 104, The current try/catch in run/1 only matches the error class (catch error:Reason) so throw/1 and exit/1 will escape; change the catch to handle all exception classes (e.g., catch Class:Reason:Stack or catch _:Reason with an explicit stack capture) and return a uniform error tuple (for example {error, {Class, Reason, Stack}}) so run/1 wraps throw/exit/error alike; update the catch clause in the run/1 function to match Class/Stack and include the stacktrace for logging/diagnosis.src/atomvm_spectrometer.erl (1)
64-121: 💤 Low valueAudit and examine dispatch blocks are essentially identical.
The two arms only differ in whether
run_audit/1orrun_examine/1is called and the error label ("Audit failed"is even reused for the examine path on lines 104 and 112). Extract a helper that takes the runner fun + label and call it from both arms; this also makes it harder for the two paths to drift (e.g., one being updated without the other when new error wrappers are added).♻️ Sketch
run_with_compiler(Opts, Runner, ErrLabel) -> case application:ensure_all_started([compiler, syntax_tools]) of {ok, _} -> dispatch(Runner, Opts, ErrLabel); {error, {already_started, _}} -> dispatch(Runner, Opts, ErrLabel); {error, Reason} -> io:format("Failed to start required applications: ~p\n", [Reason]), maybe_halt(1) end. dispatch(Runner, Opts, ErrLabel) -> case Runner(Opts) of ok -> maybe_halt(0); {error, Reason} -> io:format("~s failed: ~p\n", [ErrLabel, Reason]), maybe_halt(1) end.Then:
{command, audit, Opts} -> run_with_compiler(Opts, fun run_audit/1, "Audit"); {command, examine, Opts} -> run_with_compiler(Opts, fun run_examine/1, "Examine");🤖 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 64 - 121, The audit and examine branches duplicate the same ensure_all_started + result handling; extract a helper like run_with_compiler(Opts, Runner, ErrLabel) (and an internal dispatch(Runner, Opts, ErrLabel)) to centralize application startup and error logging, then replace the {command, audit, Opts} arm with run_with_compiler(Opts, fun run_audit/1, "Audit") and the {command, examine, Opts} arm with run_with_compiler(Opts, fun run_examine/1, "Examine") so both paths share the same startup/error handling and use the correct error label.test/cli_main_tests.erl (3)
288-296: 💤 Low valueTypo: "auditner".
auditner returns empty map for non-existent dirs— should beauditoror justaudit.📝 Proposed fix
- %% auditner returns empty map for non-existent dirs + %% audit returns empty map for non-existent dirs🤖 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/cli_main_tests.erl` around lines 288 - 296, Update the test description and inline comment in main_audit_missing_dir_test_(): replace the typo "auditner" with the correct term ("auditor" or "audit") so the human-readable test string and the comment match the intended behavior; ensure the test name main_audit_missing_dir_test_(), the assertion calling atomvm_spectrometer:main([...]) and the expected Result handling remain unchanged.
392-433: 💤 Low valueReplace integer-byte binaries with binary string literals for readability.
<<48, 46, 55, 46, 120>>and<<118, 48, 46, 53, 46, 48>>are simply<<"0.7.x">>and<<"v0.5.0">>. The byte-list form is opaque, easy to mistype, and forces reviewers to decode ASCII to understand what version the test mocks. Same applies on lines 426-429.📝 Proposed fix
CustomDB = [ {mock_pkg, [ - {custom_func, 1, all, {unreleased, <<48, 46, 55, 46, 120>>}} + {custom_func, 1, all, {unreleased, <<"0.7.x">>}} ]}, - {lists, [{map, 2, all, <<118, 48, 46, 53, 46, 48>>}]} + {lists, [{map, 2, all, <<"v0.5.0">>}]} ],CustomDB = [ {mock_pkg, [ - {custom_func, 1, all, - {unreleased, <<48, 46, 55, 46, 120>>}}, - {another_func, 2, all, - {unreleased, <<48, 46, 55, 46, 120>>}} + {custom_func, 1, all, + {unreleased, <<"0.7.x">>}}, + {another_func, 2, all, + {unreleased, <<"0.7.x">>}} ]} ],🤖 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/cli_main_tests.erl` around lines 392 - 433, The CustomDB test data uses integer-byte binaries like <<48,46,55,46,120>> and <<118,48,46,53,46,48>> which are hard to read; update those byte-list binaries to binary string literals (e.g., <<"0.7.x">> and <<"v0.5.0">>) in the CustomDB definitions used by the test cases (see CustomDB in the first test block and in main_supported_mock_module_test_), ensuring the same replacements are made before writing DbFile via file:write_file so the written supported_functions.data contains the readable binary literals.
132-139: 💤 Low valueComment is stale —
parse_args/1no longer crashes withfunction_clause.The body comment says “parse_args has no catch-all clause, so unknown commands cause function_clause”, but
src/atomvm_spectrometer.erl(lines 255-256) ends withparse_args([Unknown | _]) -> {error, "Unsupported command " ++ Unknown}. The assertion{error, {halt, 1}}is correct, but the rationale in the comment is misleading; please update it so future readers don't try to reproduce a crash that no longer happens.🤖 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/cli_main_tests.erl` around lines 132 - 139, Update the misleading explanatory comment in the test main_unknown_command_test_(): remove the assertion that parse_args/1 lacks a catch-all and causes a function_clause, and instead state that parse_args([Unknown | _]) now returns an error tuple (e.g., {error, "Unsupported command " ++ Unknown}) so atomvm_spectrometer:main(["unknown_command"]) yields {error, {halt, 1}}; reference the parse_args/1 clause in src/atomvm_spectrometer.erl and keep the existing assertion unchanged.test/spectrometer_atomvm_tests.erl (2)
330-347: 💤 Low valueTest writes binary garbage rather than a "non-list term".
The comment on line 338 says “Write a non-list term” but
file:write_file(File, term_to_binary(not_a_list))writes the External Term Format encoding ofnot_a_list— a binary blob thatfile:consult/1cannot tokenize at all. So this exercises the parse-error branch ofconsult_db/1, not the non-list term branch. To actually cover the latter, write the textual representation of a non-list term:📝 Proposed fix
- %% Write a non-list term - ok = file:write_file(File, term_to_binary(not_a_list)), + %% Write a non-list term in Erlang text format (parsable by file:consult/1) + ok = file:write_file(File, <<"not_a_list.\n">>),🤖 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_atomvm_tests.erl` around lines 330 - 347, The test consult_db_invalid_test_() currently writes a binary ETF blob via file:write_file(File, term_to_binary(not_a_list)), which triggers the parse-error path; to exercise the "non-list term" branch, write the textual Erlang term instead (e.g. use io_lib:format("~p.", [not_a_list]) or similar) so file:consult/1 can tokenize it; ensure the written string includes the terminating period (and optional newline) so consult parses it as a term rather than failing to tokenize.
304-309: 💤 Low value
reload_db/0 clears cachetest does not actually verify cache invalidation.Asserting
DB1 =:= DB2after reload only proves the database is deterministic, not thatreload_db/0cleared and reloaded anything (the same result would hold ifreload_db/0were a no‑op). To validate the cache‑clearing behavior, mutate the on‑disk DB between the twoload_db/0calls (e.g., viaapplication:set_env(spectrometer, cache_dir, …)pointing at a temp dir with a different DB) and assertDB1 =/= DB2.🤖 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_atomvm_tests.erl` around lines 304 - 309, The test "reload_db/0 clears cache" currently only compares DB1 and DB2 which doesn't prove reload_db/0 invalidated any cached state; modify the test so between the first spectrometer_atomvm:load_db() and ok = spectrometer_atomvm:reload_db() you change the on‑disk DB used by load_db/0 (e.g., create two temp dirs or DB files and call application:set_env(spectrometer, cache_dir, TempDir) or otherwise write a modified DB into the cache location), then call reload_db/0 and load_db/0 again and assert the results differ (use ?assertNotEqual or ?assert(DB1 =/= DB2)) to confirm cache was cleared and reloaded; reference spectrometer_atomvm:load_db/0 and spectrometer_atomvm:reload_db/0 when locating the code to update.test/atomvm_spectrometer_tests.erl (1)
696-704: 💤 Low valueDead clause in
cleanup_temp_dir/1.
file:del_dir_r/1returns onlyok | {error, posix() | badarg}; it never returns{ok, _}. The second clause is unreachable. Either drop it, or replace the helper with a singlecaseoverok/{error, _}for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/atomvm_spectrometer_tests.erl` around lines 696 - 704, The clause in cleanup_temp_dir/1 that matches {ok, _Removed} is unreachable because file:del_dir_r/1 only returns ok or {error, Reason}; remove that second clause and simplify the case to only handle ok -> ok; and {error, Reason} -> io:format("Warning: failed to cleanup ~s: ~p~n", [Dir, Reason]). Ensure you update the function head cleanup_temp_dir/1 accordingly so it no longer contains the dead pattern.
🤖 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.lock.license`:
- Around line 2-8: Remove the ambiguous "All rights reserved." line so the
SPDX-License-Identifier: Apache-2.0 declaration is unambiguous; locate the
header block containing the copyright line "Copyright (c) 2026 Winford
(UncleGrumpy) <winford@object.stream>" and remove the line that reads "All
rights reserved." leaving the SPDX-FileCopyrightText and SPDX-License-Identifier
entries intact.
In `@src/atomvm_spectrometer.erl`:
- Around line 258-279: The parse_help_args/1 function is missing the "examine"
clause and contains a duplicated case body; update parse_help_args/1 to include
the "examine" -> {help, examine} arm and remove the redundant second clause by
handling the empty-list input directly (match [] -> help or pattern-match the
empty string as appropriate) so there is a single case expression that covers ""
and "examine" along with the other commands; use the existing parse_help_args/1
function name to locate and modify the clause.
- Around line 364-365: Update the typespecs to use the formal term() syntax:
change any occurrences of the bare atom term to term() in the
parse_ecosystem_args typespec and the related typespec that mentions Reason
(e.g., replace {error, Reason :: term} with {error, Reason :: term()} and any
other bare term to term()); this ensures consistency with OTP typespec
conventions and keeps parse_arg_result(), opts_map(), parse_ecosystem_args and
the error tuple signatures using term() not term.
In `@src/spectrometer_analyzer.erl`:
- Around line 350-358: The catch in the load_ecosystem_state/0 (invoked by
load_filter_data/0 used by filter/1) only matches thrown terms (`catch _ ->`) so
errors raised by binary_to_term/1 (e.g. error:badarg) escape; change the catch
clause to catch _:_ -> to catch all exception classes, keeping the existing
io:format warning and returning #{} as the graceful fallback.
In `@src/spectrometer_help.erl`:
- Around line 22-49: The general help text in usage/0 omits the "examine"
command even though the command() type includes examine and usage_examine/0
exists and examine is dispatched in usage/1; update usage/0 to add "examine" to
the "Commands:" list and include a corresponding "spectrometer help examine"
line in the "Get detailed help on a command:" block so users can discover the
command, referencing usage/0 and usage_examine/0 to mirror the exact wording and
placement used for other commands.
In `@src/spectrometer_http.erl`:
- Around line 59-81: fetch_github_cursor is advancing the star range inclusively
so repos with stars == MinStarsInBatch can reappear; change the recursion to
advance the lower bound to MinStarsInBatch - 1 (i.e., call
fetch_github_cursor(MinStars, MinStarsInBatch - 1, NewAcc, Max)) or
alternatively dedupe by tracking seen clone_url keys in the Acc (update
filter_repos/Acc handling) so duplicate repos are not added; adjust references
around star_filter_range, fetch_github_query, filter_repos and the
MinStarsInBatch calculation accordingly.
In `@src/spectrometer_reporter.erl`:
- Around line 138-148: The `@spec` for print_summary/1 incorrectly declares a
return type of none(), but the function actually returns ok; update the spec for
print_summary (the clause with -spec print_summary(#{...}) -> none().) to return
ok instead so the type signature matches the implementation and Dialyzer won't
flag it.
- Around line 225-252: The footer currently prints the count of Results (the
TopN sublist) which under-reports when TopN < total; in print_summary/3 replace
the use of length(Results) for the "Total unique MFAs" message with the actual
total (e.g., length(Sorted) or maps:get(total_unique, Report)) so the label
reflects the full report total rather than the displayed sublist.
In `@src/spectrometer_scanner.erl`:
- Around line 63-91: find_erl_files/1 and its accumulator variant
find_erl_files/2 currently traverse every directory (including _build, deps,
.rebar3) causing duplicate counts; update find_erl_files to skip known
build/output directories by name (e.g. "_build", "deps", ".rebar3") before
recursing: add a SkipList (hardcoded or passed as an optional argument) and
short-circuit when Entry matches any skip name (or matches a dot/hidden rule) so
the recursion in find_erl_files(Path, A) never enters those paths; keep the rest
of the file_info/type and .erl extension logic unchanged and reference
find_erl_files/1, find_erl_files/2 and filename:extension when implementing the
check.
In `@src/spectrometer_updater.erl`:
- Around line 881-908: calculate_arity/3 currently uses string:split/2 and
count_args/3 which operates on split segments, causing mismatches and a constant
arity of 1; replace that logic by extracting the substring starting at the
opening paren (using the found Start from re:run with Pattern), convert that
slice to a char list, then perform a character-level scan (in a new or updated
count_args/3) that walks each char, maintains a nesting Depth, increments a
TopLevelComma counter when seeing ',' at Depth==0, stops when the matching
top-level ')' is found and returns 0 for no args or TopLevelComma+1 otherwise;
update references to Pattern, calculate_arity/3 and count_args/3 so the scanner
operates on characters (char list) rather than split segments.
- Around line 832-868: scan_calls/3 builds OTPSet from
spectrometer_otp:modules_list() which returns strings, but later compares with
atoms (Mod) so the check always fails; fix by converting the module-name strings
to atoms when constructing OTPSet (e.g., map spectrometer_utils:atom_from_string
over spectrometer_otp:modules_list() before calling sets:from_list) so
sets:is_element(Mod, OTPSet) compares atoms to atoms (mirror the conversion used
in spectrometer_reporter.erl), keeping the rest of scan_calls, OTPSet,
spectrometer_utils:atom_from_string and sets:from_list intact.
In `@src/spectrometer_utils.erl`:
- Around line 251-269: The doc examples for normalize_github_url are incorrect:
they show "github.com/..." but the function implementation returns full
"https://github.com/... .git"; update the examples in the function's
documentation to show the actual returned value (e.g.
"https://github.com/atomvm/atomvm.git") for the given inputs so the docs match
the normalize_github_url behavior.
- Around line 315-348: The fetch-and-checkout branches call run_git_command(...,
[]) without disabling interactive prompts; update the two calls that
run_git_command for ["-C", TmpDir, "fetch", "--tags", "--quiet"] and ["-C",
TmpDir, "checkout", "--quiet", TagStr] to pass an environment list that includes
{"GIT_TERMINAL_PROMPT","0"} (same env used for the initial clone) so git cannot
prompt and hang gather_git_output/2; keep existing error handling intact.
- Around line 229-241: gather_git_output/2 currently can block forever; modify
it to include a receive after clause with a sensible timeout (e.g. configurable
or a constant) so that if no messages arrive within the timeout you call
port_close(Port), drain any remaining messages from that Port, and return
{error, timeout} to callers (e.g. clone_temp_repo/2). Locate gather_git_output/2
and add the after branch to close the port, loop to flush any {Port, ...}
messages, then surface {error, timeout}; mirror the same draining behavior used
in await_git_port/1 for consistency.
In `@test/spectrometer_atomvm_tests.erl`:
- Around line 298-324: The tests inside db_loading_test_/0 use the 0-arity fun
generator form but call the underscored macro ?_assert which only returns a test
tuple instead of executing the assertion; replace each ?_assert with the
non-underscored ?assert in the fun bodies for the "load_db/0 returns a map",
"bundled_data_path/0 returns a string", and "user_cache_path/0 returns
platform-appropriate path" tests so the assertions
(is_map(spectrometer_atomvm:load_db()) and is_list(Path) / string:str checks)
are actually executed.
In `@test/spectrometer_http_tests.erl`:
- Around line 162-168: The test currently asserts an exact count (length(Repos)
=:= 200) for Repos returned by spectrometer_http:fetch_github_repos({200, 1}),
which is flaky against GitHub Search API; change the assertion to either request
a smaller, reliable page size (e.g., call fetch_github_repos with {100,1}) or
relax the check to assert length(Repos) >= <LOWER_BOUND> (e.g., 50) while
keeping the uniqueness verification (FullNames / UniqueNames) intact; update the
test around the Repos/FullNames/UniqueNames variables and the fetch_github_repos
call accordingly.
---
Duplicate comments:
In `@src/spectrometer_otp.erl`:
- Around line 41-51: Use binary_to_term(Bin, [safe]) and validate the decoded
value before returning it: call binary_to_term(Bin, [safe]) in the try and
ensure the result is the expected list of module names (or convert a
list-of-tuples like [{M,_,_}|...] to [M|..]) by checking is_list(Modules) and
element types (is_atom/1 or tuple shape) and only return Modules if validation
passes; otherwise log the warning (using ModFile) and fall back to generating
the module list via code:all_available(); this prevents atom injection and
avoids crashing callers like is_otp_module/1 when the cache contains a non-list.
---
Nitpick comments:
In `@generate_fun_data.sh`:
- Line 22: TMP_CACHE_DIR is hardcoded to /tmp which ignores $TMPDIR and can
cause cross-user collisions; change the assignment that sets TMP_CACHE_DIR to
use a TMPDIR-aware, unique temp directory (e.g. use
"${TMPDIR:-/tmp}/spectrometer_version_cache.$$" or create one via mktemp -d) and
ensure the rest of the script (references to TMP_CACHE_DIR) handles cleanup on
exit; update the TMP_CACHE_DIR assignment in generate_fun_data.sh accordingly
and add trap cleanup if using mktemp -d.
- Line 78: The check `git branch -r | grep "release-0.7"` can false-positive on
substring matches; replace it with a precise existence check: either use an
anchored/whole-line grep (e.g. matching "origin/release-0.7" exactly) or,
preferably, use Git's native verify command `git show-ref --verify --quiet
refs/remotes/origin/release-0.7` in place of the pipeline so the script only
treats the specific remote branch `origin/release-0.7` as present; update the
conditional that currently references `git branch -r | grep "release-0.7"`
accordingly.
- Around line 14-27: Remove the redundant empty-string check for ATOMVM_DIR:
since the script already exits when "$#" -eq 0, drop the second conditional that
tests [ -z "$ATOMVM_DIR" ] (the block referencing ATOMVM_DIR at the end) so you
only keep the initial argument-count guard; ensure ATOMVM_DIR="$1" remains and
subsequent code (e.g., uses of ATOMVM_DIR, SCRIPT_DIR, SPECTROMETER,
TMP_CACHE_DIR) assumes a provided positional argument.
- Around line 1-12: Update the header comment in generate_fun_data.sh so it
references the correct script name: replace both occurrences of
"regenerate_version_data.sh" (the header title and the Usage line) with
"generate_fun_data.sh"; edit the top comment block around the shebang where the
misleading name appears to ensure README/PR-consistent naming and leave the
SPDX/license lines unchanged.
In `@include/ecosystem.hrl`:
- Line 9: Wrap the macro definition with an include guard: check -ifndef
ECOSYSTEM_STATE before the -define and close with -endif so that the existing
-define(ECOSYSTEM_STATE, "beam_ecosystem.bin"). is only declared once; reference
the macro ?ECOSYSTEM_STATE (and the -define(ECOSYSTEM_STATE, ...) line) when
making the change to ensure the header is safe under repeated inclusion.
In `@src/atomvm_spectrometer.erl`:
- Around line 64-121: The audit and examine branches duplicate the same
ensure_all_started + result handling; extract a helper like
run_with_compiler(Opts, Runner, ErrLabel) (and an internal dispatch(Runner,
Opts, ErrLabel)) to centralize application startup and error logging, then
replace the {command, audit, Opts} arm with run_with_compiler(Opts, fun
run_audit/1, "Audit") and the {command, examine, Opts} arm with
run_with_compiler(Opts, fun run_examine/1, "Examine") so both paths share the
same startup/error handling and use the correct error label.
In `@src/spectrometer_analyzer.erl`:
- Around line 285-294: parse_target_lines currently assumes any non-github.com,
non-directory string is a GitHub URL which causes confusing clone failures;
update the Url handling in parse_target_lines to validate the input before
returning {github_url, Url} — specifically, after checking string:find(Url,
"github.com") and filelib:is_dir(Url) add a validation step (e.g., ensure Url
matches expected URL/SSH patterns or has http/https/ssh prefix) and return a
clear error tuple (for example {false, {invalid_target, Url}}) for anything that
doesn't pass validation, keeping the existing branches for {local_dir, Url} and
{github_url, Url} only for confirmed valid inputs.
In `@src/spectrometer_ecosystem.erl`:
- Around line 100-104: The current try/catch in run/1 only matches the error
class (catch error:Reason) so throw/1 and exit/1 will escape; change the catch
to handle all exception classes (e.g., catch Class:Reason:Stack or catch
_:Reason with an explicit stack capture) and return a uniform error tuple (for
example {error, {Class, Reason, Stack}}) so run/1 wraps throw/exit/error alike;
update the catch clause in the run/1 function to match Class/Stack and include
the stacktrace for logging/diagnosis.
In `@src/spectrometer_help.erl`:
- Line 74: In spectrometer_help.erl there are several comment lines like %%
"Print help text for the `audit` command." (lines for audit and other commands)
that use quoted strings inside %% comments; change them to idiomatic comments by
removing the surrounding double quotes (e.g., %% Print help text for the `audit`
command.) or, if they were intended as documentation attributes, convert them to
proper -doc or -spec attributes; update each occurrence referencing the same
quoted-comment pattern (e.g., the help comment for audit and the other command
help comments) so all quoted strings are either unquoted plain comments or real
-doc attributes.
In `@src/spectrometer_http.erl`:
- Around line 429-438: The is_windows_absolute_path function uses the magic
number 92 to represent a backslash; replace that numeric literal with the
idiomatic Erlang character literal $\\ (so the pattern [_, $:, 92 | _] becomes
[_, $:, $\\ | _]) to match the style of the other $-prefixed character literals
and improve readability.
- Around line 83-88: star_filter_range/2 currently only handles the (infinity,
undefined) case and falls through to produce "infinity..N" when LastStars is
set; add a clause to handle MinStars = infinity with a concrete LastStars (e.g.,
produce a "<=N" style filter) and place it before the general range clause so
star_filter_range(infinity, LastStars) returns a valid GitHub query instead of
"infinity..N"; update the existing clauses in star_filter_range/2 accordingly.
In `@src/spectrometer_scanner.erl`:
- Around line 24-46: Add a Dialyzer spec for the exported function
scan_directory/1: declare the argument as a string() (Dir :: string()) and the
return as a map whose keys are {module(), atom(), non_neg_integer()} tuples and
whose values are non_neg_integer() counts (i.e. #{ {module(), atom(),
non_neg_integer()} => non_neg_integer() }). Add this -spec immediately above the
scan_directory/1 implementation to make the public API explicit for callers and
static analysis.
In `@src/spectrometer_updater.erl`:
- Around line 386-392: The list/list clause in merge_platforms_all/2 currently
returns the sorted union but never collapses to the atom all; update
merge_platforms_all/2 to mirror merge_platforms/2 by computing the union of
OldList and NewList, sorting/uniq'ing it, and if that result equals the
canonical complete-platform list then return the atom all, otherwise return the
merged list; reference the existing merge_platforms/2 behavior and the function
name merge_platforms_all/2 when making the change and use the same canonical
full-platform list (the same source used by merge_platforms/2) for the equality
check.
In `@src/spectrometer_utils.erl`:
- Around line 419-425: The os_temp_dir/0 function currently returns a
non-standard Windows path; update os_temp_dir/0 to first prefer Windows
environment variables (check os:getenv("TEMP") and os:getenv("TMP") and use
whichever is set), and only if both are unset fall back to the standard Windows
temp location "C:/Windows/Temp" for the {win32, _} branch (leave the default
"/tmp" for non-Windows). Ensure the function returns the chosen string
consistently.
In `@test/atomvm_spectrometer_tests.erl`:
- Around line 696-704: The clause in cleanup_temp_dir/1 that matches {ok,
_Removed} is unreachable because file:del_dir_r/1 only returns ok or {error,
Reason}; remove that second clause and simplify the case to only handle ok ->
ok; and {error, Reason} -> io:format("Warning: failed to cleanup ~s: ~p~n",
[Dir, Reason]). Ensure you update the function head cleanup_temp_dir/1
accordingly so it no longer contains the dead pattern.
In `@test/cli_main_tests.erl`:
- Around line 288-296: Update the test description and inline comment in
main_audit_missing_dir_test_(): replace the typo "auditner" with the correct
term ("auditor" or "audit") so the human-readable test string and the comment
match the intended behavior; ensure the test name
main_audit_missing_dir_test_(), the assertion calling
atomvm_spectrometer:main([...]) and the expected Result handling remain
unchanged.
- Around line 392-433: The CustomDB test data uses integer-byte binaries like
<<48,46,55,46,120>> and <<118,48,46,53,46,48>> which are hard to read; update
those byte-list binaries to binary string literals (e.g., <<"0.7.x">> and
<<"v0.5.0">>) in the CustomDB definitions used by the test cases (see CustomDB
in the first test block and in main_supported_mock_module_test_), ensuring the
same replacements are made before writing DbFile via file:write_file so the
written supported_functions.data contains the readable binary literals.
- Around line 132-139: Update the misleading explanatory comment in the test
main_unknown_command_test_(): remove the assertion that parse_args/1 lacks a
catch-all and causes a function_clause, and instead state that
parse_args([Unknown | _]) now returns an error tuple (e.g., {error, "Unsupported
command " ++ Unknown}) so atomvm_spectrometer:main(["unknown_command"]) yields
{error, {halt, 1}}; reference the parse_args/1 clause in
src/atomvm_spectrometer.erl and keep the existing assertion unchanged.
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 145-148: The assertion ?assert(maps:size(Stats) >= 0) is a
tautology; update the test to make it meaningful by either asserting there are
entries (e.g., replace with ?assert(maps:size(Stats) > 0)) or asserting a
specific expected key/value exists in Stats (e.g., use maps:is_key(ExpectedKey,
Stats) or equal lookup on Stats). Locate the assertion that references Stats and
maps:size in the test function (the failing clause using ?assert) and replace
the tautological check with one of the two stronger checks above to validate
actual analyzer output.
- Around line 139-188: Wrap each network-dependent test
(scan_target_github_url_test_, scan_target_github_url_nonexistent_test_,
scan_target_hex_package_test_, scan_target_hex_package_nonexistent_test_,
scan_target_hex_latest_test_) in the same SKIP_NETWORK_TESTS guard used in
cli_main_tests: use case os:getenv("SKIP_NETWORK_TESTS") of false -> <original
test body> ; _ -> {"skipped (network tests disabled)", fun() -> ok end} end.
While doing this, for scan_target_hex_package_test_ consider relaxing or
removing the brittle specific assertions about atomvm_packbeam keys
({packbeam_api, create, 3}, etc.) so the test won’t fail if upstream Hex package
contents change.
In `@test/spectrometer_atomvm_tests.erl`:
- Around line 330-347: The test consult_db_invalid_test_() currently writes a
binary ETF blob via file:write_file(File, term_to_binary(not_a_list)), which
triggers the parse-error path; to exercise the "non-list term" branch, write the
textual Erlang term instead (e.g. use io_lib:format("~p.", [not_a_list]) or
similar) so file:consult/1 can tokenize it; ensure the written string includes
the terminating period (and optional newline) so consult parses it as a term
rather than failing to tokenize.
- Around line 304-309: The test "reload_db/0 clears cache" currently only
compares DB1 and DB2 which doesn't prove reload_db/0 invalidated any cached
state; modify the test so between the first spectrometer_atomvm:load_db() and ok
= spectrometer_atomvm:reload_db() you change the on‑disk DB used by load_db/0
(e.g., create two temp dirs or DB files and call
application:set_env(spectrometer, cache_dir, TempDir) or otherwise write a
modified DB into the cache location), then call reload_db/0 and load_db/0 again
and assert the results differ (use ?assertNotEqual or ?assert(DB1 =/= DB2)) to
confirm cache was cleared and reloaded; reference spectrometer_atomvm:load_db/0
and spectrometer_atomvm:reload_db/0 when locating the code to update.
In `@test/spectrometer_updater_tests.erl`:
- Around line 92-99: The test is_older_since_both_unreleased_test_ currently
asserts only that is_older_since({unreleased, <<"main">>}, {unreleased,
<<"0.7.x">>}) is false, but the comment notes that is_older_since/2 uses
branch_sort_key/1 and some unreleased pairs can return true; fix by either
renaming the test to accurately reflect the specific case (e.g., "main is newer
than versioned branches") or expand the test to assert symmetry/never-older
behavior by adding the complementary assertion(s) such as checking
is_older_since({unreleased, <<"0.7.x">>}, {unreleased, <<"main">>}) and/or other
pair(s) to exercise branch_sort_key/1 ordering so the intended "never older"
claim is actually tested.
In `@test/spectrometer_utils_tests.erl`:
- Around line 233-247: The two tests in http_get_test_() that call
spectrometer_http:fetch/1 should be guarded like the other network tests: check
the SKIP_NETWORK_TESTS env var (or the same mechanism used by
spectrometer_http_tests) and skip these cases when set, or alternatively replace
the localhost:59999 case with a deterministically-closed port (bind to 0 to get
an ephemeral port and immediately close it) to avoid colliding with services on
CI; update http_get_test_() to perform that gating or closed-port setup around
the spectrometer_http:fetch("http://localhost:59999/test") and the domain test
to ensure both are network-safe.
🪄 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: d5a2f7f9-8c89-443d-a7ad-9f4ac710649b
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
🚧 Files skipped from review as they are similar to previous changes (4)
- src/spectrometer.erl
- rebar.config
- src/spectrometer_atomvm.erl
- test/spectrometer_scanner_tests.erl
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
src/spectrometer_updater.erl (1)
887-916:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
calculate_arity/3returns 0 for all calls because top-level commas are never counted.
Rest = string:slice(Line, Start)includes the literalMod:Fun(prefix. Whencount_args/3reaches the opening(, it incrementsDepthto 1, so all argument-list commas are atDepth =:= 1and fall through to the wildcard[_ | T]clause —count_args([$, | T], 0, Commas)only fires at depth 0 and never matches inside the call.Trace for
foo:bar(a, b):
- identifier chars → wildcard, depth=0
(→ depth=1a→ wildcard,→ depth=1, falls through to wildcard (not counted))→ matches theDepth =/= 0arm, depth=0- end →
Commas =:= 0→ returns0(expected:2)Net effect: every external call recorded by
scan_calls/3gets arity0, populatingpriv/supported_functions.datawith bogus{Mod, Fun, 0}keys for any function only discovered via test-file scanning.Two viable fixes — pick whichever you prefer:
Fix A — skip past
Mod:Fun(so args are at depth 0calculate_arity(Line, ModStr, FunStr) -> Pattern = ModStr ++ ":" ++ FunStr ++ "\\(", - case re:run(Line, Pattern, [{capture, first}]) of - {match, [{Start, _Len}]} -> - Rest = string:slice(Line, Start), - Chars = unicode:characters_to_list(Rest), - count_args(Chars, 0, 0); + case re:run(Line, Pattern, [{capture, first}]) of + {match, [{Start, Len}]} -> + %% Start scanning *inside* the argument list + Rest = string:slice(Line, Start + Len), + Chars = unicode:characters_to_list(Rest), + count_args(Chars, 0, 0); _ -> 0 end.Fix B — count at depth 1 (treat the call's own paren as the boundary)
-count_args([$) | _], 0, Commas) -> +count_args([$) | _], 1, Commas) -> case Commas of 0 -> 0; _ -> Commas + 1 end; count_args([$) | T], Depth, Commas) -> count_args(T, Depth - 1, Commas); -count_args([$, | T], 0, Commas) -> - count_args(T, 0, Commas + 1); +count_args([$, | T], 1, Commas) -> + count_args(T, 1, Commas + 1);Either way, please add a couple of unit tests (
foo:bar()→ 0,foo:bar(a)→ 1,foo:bar(a, b)→ 2,foo:bar(g(1, 2), x)→ 2) so this regresses if the depth bookkeeping breaks again.🤖 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 887 - 916, calculate_arity/3 currently slices the line at Start but ignores the matched length so the "Mod:Fun(" prefix (including the opening paren) remains, causing count_args/3 to see commas at Depth=1 and never count them; fix by using the captured length from re:run and slice starting at Start+Len (use the {Start,Len} tuple returned) so the argument parsing begins after the opening paren and top-level commas are at Depth=0; update calculate_arity/3 to bind Len and call string:slice(Line, Start+Len) and add unit tests exercising foo:bar(), foo:bar(a), foo:bar(a, b), and foo:bar(g(1,2), x) to ensure arities 0,1,2,2 respectively (scan_calls/3 and count_args/3 are the relevant symbols to validate via tests).
🧹 Nitpick comments (5)
test/spectrometer_scanner_tests.erl (1)
32-55: ⚡ Quick winAdd one regression case for skipped build directories.
find_erl_files/2now has explicit skip logic for_build,deps, and.rebar3, but the recursive tests only cover positive discovery. A fixture under one of those directories would protect the duplicate-count fix from regressing later.🤖 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_scanner_tests.erl` around lines 32 - 55, Add a regression case to the existing find_erl_files_recursive_test_() that creates one of the skipped directories (e.g., "_build", "deps" or ".rebar3") under the temporary Dir, places a .erl file inside that skipped dir, and then calls spectrometer_scanner:find_erl_files(Dir) to assert the returned list still contains only the expected files (mod1.erl and mod2.erl) and does not include the file inside the skipped directory; ensure you create the skipped dir (file:make_dir/1), create the file there, and keep the existing cleanup in the after block so spectrometer_utils:purge_dir(Dir) removes the fixture.src/spectrometer_updater.erl (1)
64-110: 💤 Low valueDuplicated fold logic for the existing/bundled DB load path.
Lines 68-80 and 89-103 are identical aside from the data source (user output file vs. bundled data). Extracting the fold into a small helper would tighten this up:
Suggested extraction
+db_to_map(Data) -> + lists:foldl( + fun({Mod, Funs}, Acc) -> + lists:foldl( + fun({F, A, Platforms, Since0}, A2) -> + maps:put({Mod, F, A}, {Platforms, Since0}, A2) + end, + Acc, + Funs + ) + end, + #{}, + Data + ). + update_datafile(Opts, OutputFile) -> ... ExistingDB = case file:consult(OutputFile) of {ok, [Data]} when is_list(Data) -> io:format("Loading existing data set from ~s\n", [OutputFile]), - lists:foldl( ... ) + db_to_map(Data); _ -> Datafile = spectrometer_utils:bundled_data_path(), case file:consult(Datafile) of {ok, [Data]} when is_list(Data) -> io:format("Loading bundled data set from ~s\n", [Datafile]), - lists:foldl( ... ) + db_to_map(Data); _ -> io:format("No existing data found, starting with empty data set\n"), #{} 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 `@src/spectrometer_updater.erl` around lines 64 - 110, The duplicated nested lists:foldl logic that builds the map for ExistingDB (the inner funs that take {Mod, Funs} and {F, A, Platforms, Since0}) should be extracted into a small helper (e.g., build_db_from_list/1 or fold_funs_into_map/2) and called from both branches (the file:consult OutputFile branch and the bundled Datafile branch) to remove the repeated code; implement the helper to accept the Data list and return the constructed map (used where ExistingDB is assigned) and replace both inline lists:foldl blocks with calls to that helper (keeping the same map key shape {Mod, F, A} => {Platforms, Since0}).test/spectrometer_updater_tests.erl (2)
1-483: ⚡ Quick winAdd unit coverage for
calculate_arity/3/count_args/3.There's no direct test for the call-site arity scanner in
spectrometer_updater, which is where the test-file scan flow ultimately depends for theAritycomponent of every recorded{Mod, Fun, Arity}key. A few small cases here would have flagged the depth-tracking bug I noted insrc/spectrometer_updater.erl(lines 887-916):calculate_arity_test_() -> {"counts top-level args of a Mod:Fun( call", fun() -> ?assertEqual(0, spectrometer_updater:calculate_arity( "foo:bar()", "foo", "bar")), ?assertEqual(1, spectrometer_updater:calculate_arity( "foo:bar(X)", "foo", "bar")), ?assertEqual(2, spectrometer_updater:calculate_arity( "foo:bar(X, Y)", "foo", "bar")), ?assertEqual(2, spectrometer_updater:calculate_arity( "foo:bar(g(1, 2), 3)", "foo", "bar")) 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 1 - 483, The test suite is missing coverage for the call-site arity scanner which exposed a depth-tracking bug in calculate_arity/3 (and its helper count_args/3) in spectrometer_updater; add a new eunit test function (e.g. calculate_arity_test_()) in spectrometer_updater_tests.erl that calls spectrometer_updater:calculate_arity/3 with the provided cases ("foo:bar()", "foo:bar(X)", "foo:bar(X, Y)", "foo:bar(g(1, 2), 3)") asserting arities 0,1,2,2 respectively (and consider adding a few nested/edge cases) so the failing depth/count logic in calculate_arity/3 and count_args/3 is exercised and will guide the fix in src/spectrometer_updater.erl.
395-433: ⚡ Quick winStrengthen the
scan_repo_libs_test_assertions to catch parsing regressions.The test currently only asserts
is_map(Acc)andmaps:size(Acc) >= 0(always true), so a complete parser breakage inparse_exports/find_module_namewould still pass. Since the fake source defines a clear-module(my_lists)and-export([map/2, filter/2]), you can directly assert what the scanner found:Suggested assertions
- ?assert(is_map(Acc)), - %% Note: The actual function names might differ based on parsing - %% Just verify we got some results from the estdlib scan - - %% At least verify it runs without error - ?assert(maps:size(Acc) >= 0) + ?assert(is_map(Acc)), + ?assertEqual( + {all, {unreleased, <<"main">>}}, + maps:get({my_lists, map, 2}, Acc) + ), + ?assertEqual( + {all, {unreleased, <<"main">>}}, + maps:get({my_lists, filter, 2}, Acc) + )🤖 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 395 - 433, The test scan_repo_libs_test_ currently uses weak assertions that always pass; replace them with concrete checks against the expected parsing results from spectrometer_updater:scan_atomvm_repo: assert that the returned Acc map contains an entry for the module name "my_lists" (as returned by find_module_name/parse_exports) and that this module entry includes exported function entries for map/2 and filter/2 (i.e., verify the specific function arities are present in the scan result rather than only checking is_map/Acc or maps:size). Ensure you use the same key types the scanner returns (atom or binary) when checking Acc and reference the spectrometer_updater scan result structure to read function entries.src/spectrometer_analyzer.erl (1)
251-267: 💤 Low value
scan_multi/1crashes with{badmatch, {error, _}}when the multi-target file is unreadable.
{ok, Bin} = file:read_file(File)will throwerror:{badmatch, {error, enoent}}(or similar) for missing/unreadable files, which the outertryinanalyze/2(line 179-181) wraps as{error, {badmatch, {error, enoent}}}— not very actionable for the user. A small explicit case improves the error surface:Suggested handling
scan_multi(File) -> - {ok, Bin} = file:read_file(File), - Lines = string:split(binary_to_list(Bin), "\n", all), - Targets = parse_target_lines(Lines), + case file:read_file(File) of + {ok, Bin} -> + Lines = string:split(binary_to_list(Bin), "\n", all), + Targets = parse_target_lines(Lines), + scan_multi_targets(File, Targets); + {error, Reason} -> + io:format( + standard_error, + "Error: cannot read multi-target file ~s: ~s\n", + [File, file:format_error(Reason)] + ), + #{} + 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_analyzer.erl` around lines 251 - 267, scan_multi/1 currently uses a hard match "{ok, Bin} = file:read_file(File)" which crashes with a badmatch for unreadable files; change it to explicitly handle the file:read_file(File) result (case or receive) so that when file:read_file(File) returns {error, Reason} you return or propagate a clear error tuple (e.g. {error, {could_not_read_multi_target_file, Reason}}) instead of letting a badmatch escape, and keep the happy path to convert Bin -> Lines -> Targets -> fold as-is (references: function scan_multi/1, parse_target_lines/1 and the fold that produces FinalAcc).
🤖 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 `@generate_fun_data.sh`:
- Around line 73-85: The script only verifies the remote ref but doesn't update
the working tree, so SPECTROMETER is scanning whatever commit is checked out
(usually main); before running "$SPECTROMETER" update with --branch
"release-0.7" you must check out that remote branch (for example by creating a
detached head from refs/remotes/origin/release-0.7 or using git switch/checkout
to that ref) so the working tree matches release-0.7; modify the block that
tests refs/remotes/origin/release-0.7 to perform a git checkout (or git switch
--detach) to origin/release-0.7 and handle errors before invoking SPECTROMETER
with ATOMVM_DIR, TMP_CACHE_DIR and OUTPUT_FILE.
In `@src/atomvm_spectrometer.erl`:
- Around line 64-71: The audit failure branch incorrectly logs "Examine failed"
due to a copy/paste typo; update the io:format call in the {command, audit,
Opts} -> case that handles {error, Reason} (the branch invoking
run_analyzer_dispatch with fun run_audit/1 and maybe_halt/1) to print "Audit
failed, ~p.\n" (matching the examine branch style) so the error message
correctly reflects the audit subcommand.
In `@src/spectrometer_atomvm.erl`:
- Around line 143-160: The spec for get_supported_functions/0 is too narrow:
update its arity type to include the existing wildcard/list forms used by
find_arity/2 and print_supported/1 (i.e., allow non_neg_integer() | all |
[non_neg_integer()]) so the public contract matches DB rows; adjust the -spec
tuple to reflect this arity union while keeping Platforms and Since types as
they are and ensure get_supported_functions() signature and returned tuple shape
({Module, Function, Arity, Platforms, Since}) are consistent with those symbols.
In `@src/spectrometer_help.erl`:
- Line 31: The help header string in src/spectrometer_help.erl contains a stray
']' ("Commands:]\n"); update that string literal to remove the bracket so it
reads "Commands:\n" (locate the help text or function that builds the CLI help
output where the "Commands:]" literal appears and replace it with "Commands:").
In `@src/spectrometer_http.erl`:
- Around line 421-429: The validator validate_tar_path/1 can be bypassed because
has_dotdot_segment/1 only splits once on "/" and once on "\"; update the check
to normalize separators before splitting (e.g., convert all "\" to "/" or vice
versa) or else scan for ".." as a path segment using boundary-aware logic so
mixed separators like "a\\..//secret.erl" produce a standalone ".." segment;
modify has_dotdot_segment/1 accordingly and ensure the same fix is applied to
the duplicate validation logic referenced around lines 446-449 so both code
paths use the normalized-splitting or boundary-aware check.
- Around line 57-58: The termination guard in fetch_github_cursor is too
permissive and drops the final MinStars bucket; change the stop-condition from
LastStars =< MinStars to LastStars < MinStars so the recursion includes the case
where LastStars == MinStars (ensure you update the same guard in the other
occurrence around the fetch_github_cursor handling at the later block referenced
in the review). Locate the function fetch_github_cursor and replace the =< guard
with a strict < in both places to ensure the stars:MinStars..MinStars batch is
processed.
In `@src/spectrometer_reporter.erl`:
- Around line 170-187: The footer in print_summary/3 is printing Report:Total
(which generate_report/2 stores as supported+unsupported) causing a misleading
"total calls" value; change print_summary(Report, TopN, true) to compute the
unsupported-only call total (e.g. UnsupportedTotal) by summing the per-call
counts in the Unsupported structure (use lists:foldl to accumulate numeric
counts from the tuples/records in Unsupported) and replace Total with
UnsupportedTotal in the io:format call so the printed "(~p total calls)" shows
the sum of unsupported calls only.
In `@src/spectrometer_utils.erl`:
- Around line 323-360: The tag-checkout branch currently logs failures and
returns TmpDir, causing callers to continue as if TagStr was checked out; change
the checkout handling in the TagStr branch (the run_git_command call for ["-C",
TmpDir, "checkout", "--quiet", TagStr]) so that on any non-success ({ok, Err} or
{error, {exit_status, Status, Err}}) you first remove/clean TmpDir (the
temporary clone) and then return an error tuple (e.g. {error, {checkout_failed,
TagStr, Reason}}) instead of returning ok/TmpDir; use the existing TmpDir
identifier and run_git_command results to build the Reason so callers can detect
and abort further scanning.
- Around line 38-48: The atom_from_string/1 helper currently falls back to
list_to_atom/1 which will permanently intern arbitrary CLI input; change it to
reject or error for unrecognized module names from untrusted sources (the CLI
--module parsing) instead of creating atoms. Specifically, update
atom_from_string/1 to first check the string against a bounded whitelist (e.g.,
spectrometer_otp:modules_list() or a validated modules set) and only convert to
an existing atom (list_to_existing_atom/1) when present; if not in the whitelist
return an error tuple or throw a controlled error rather than calling
list_to_atom/1. Ensure callers in the CLI parsing path handle the returned error
and avoid creating atoms from untrusted input.
In `@test/cli_main_tests.erl`:
- Around line 289-297: Replace hardcoded non-portable paths in the negative-path
tests by creating a unique temporary parent directory and pointing the CLI at a
child path you deliberately do NOT create; specifically, in
main_audit_missing_dir_test_ (and the similar test around lines 322-329, e.g.,
the cache-missing-dir test) create a unique parent using os:getenv/1 or
os:getenv fallback to "/tmp" combined with erlang:unique_integer([positive]) (or
similar), create that parent directory with file:make_dir/1, set the CLI --dir
to filename:join(Parent, "missing_child") (do NOT create the child), call
atomvm_spectrometer:main/1 as before, then rmdir the parent for cleanup so the
test reliably exercises the "missing directory" branch across environments.
---
Duplicate comments:
In `@src/spectrometer_updater.erl`:
- Around line 887-916: calculate_arity/3 currently slices the line at Start but
ignores the matched length so the "Mod:Fun(" prefix (including the opening
paren) remains, causing count_args/3 to see commas at Depth=1 and never count
them; fix by using the captured length from re:run and slice starting at
Start+Len (use the {Start,Len} tuple returned) so the argument parsing begins
after the opening paren and top-level commas are at Depth=0; update
calculate_arity/3 to bind Len and call string:slice(Line, Start+Len) and add
unit tests exercising foo:bar(), foo:bar(a), foo:bar(a, b), and foo:bar(g(1,2),
x) to ensure arities 0,1,2,2 respectively (scan_calls/3 and count_args/3 are the
relevant symbols to validate via tests).
---
Nitpick comments:
In `@src/spectrometer_analyzer.erl`:
- Around line 251-267: scan_multi/1 currently uses a hard match "{ok, Bin} =
file:read_file(File)" which crashes with a badmatch for unreadable files; change
it to explicitly handle the file:read_file(File) result (case or receive) so
that when file:read_file(File) returns {error, Reason} you return or propagate a
clear error tuple (e.g. {error, {could_not_read_multi_target_file, Reason}})
instead of letting a badmatch escape, and keep the happy path to convert Bin ->
Lines -> Targets -> fold as-is (references: function scan_multi/1,
parse_target_lines/1 and the fold that produces FinalAcc).
In `@src/spectrometer_updater.erl`:
- Around line 64-110: The duplicated nested lists:foldl logic that builds the
map for ExistingDB (the inner funs that take {Mod, Funs} and {F, A, Platforms,
Since0}) should be extracted into a small helper (e.g., build_db_from_list/1 or
fold_funs_into_map/2) and called from both branches (the file:consult OutputFile
branch and the bundled Datafile branch) to remove the repeated code; implement
the helper to accept the Data list and return the constructed map (used where
ExistingDB is assigned) and replace both inline lists:foldl blocks with calls to
that helper (keeping the same map key shape {Mod, F, A} => {Platforms, Since0}).
In `@test/spectrometer_scanner_tests.erl`:
- Around line 32-55: Add a regression case to the existing
find_erl_files_recursive_test_() that creates one of the skipped directories
(e.g., "_build", "deps" or ".rebar3") under the temporary Dir, places a .erl
file inside that skipped dir, and then calls
spectrometer_scanner:find_erl_files(Dir) to assert the returned list still
contains only the expected files (mod1.erl and mod2.erl) and does not include
the file inside the skipped directory; ensure you create the skipped dir
(file:make_dir/1), create the file there, and keep the existing cleanup in the
after block so spectrometer_utils:purge_dir(Dir) removes the fixture.
In `@test/spectrometer_updater_tests.erl`:
- Around line 1-483: The test suite is missing coverage for the call-site arity
scanner which exposed a depth-tracking bug in calculate_arity/3 (and its helper
count_args/3) in spectrometer_updater; add a new eunit test function (e.g.
calculate_arity_test_()) in spectrometer_updater_tests.erl that calls
spectrometer_updater:calculate_arity/3 with the provided cases ("foo:bar()",
"foo:bar(X)", "foo:bar(X, Y)", "foo:bar(g(1, 2), 3)") asserting arities 0,1,2,2
respectively (and consider adding a few nested/edge cases) so the failing
depth/count logic in calculate_arity/3 and count_args/3 is exercised and will
guide the fix in src/spectrometer_updater.erl.
- Around line 395-433: The test scan_repo_libs_test_ currently uses weak
assertions that always pass; replace them with concrete checks against the
expected parsing results from spectrometer_updater:scan_atomvm_repo: assert that
the returned Acc map contains an entry for the module name "my_lists" (as
returned by find_module_name/parse_exports) and that this module entry includes
exported function entries for map/2 and filter/2 (i.e., verify the specific
function arities are present in the scan result rather than only checking
is_map/Acc or maps:size). Ensure you use the same key types the scanner returns
(atom or binary) when checking Acc and reference the spectrometer_updater scan
result structure to read function entries.
🪄 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: 07aa1754-7955-40ae-ba32-d349f066bb86
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
🚧 Files skipped from review as they are similar to previous changes (4)
- rebar.lock.license
- .gitignore
- test/spectrometer_analyzer_tests.erl
- src/spectrometer_otp.erl
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/spectrometer_help.erl (1)
31-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStray
]in"Commands:]\n"still present — user-facing CLI output is malformed.The general help printed by
spectrometer --helpshowsCommands:]instead ofCommands:. This was raised in a prior review but hasn't been fixed yet.🐛 One-character fix
- "Commands:]\n" + "Commands:\n"🤖 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_help.erl` at line 31, The help output contains a stray bracket in the literal "Commands:]\n" within src/spectrometer_help.erl; locate that string (e.g., in the help-generation function) and remove the extra ']' so it reads "Commands:\n", and scan the file for any other occurrences of "Commands:]" to correct them as well.src/atomvm_spectrometer.erl (1)
64-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAudit failure path still prints
"Examine failed".When
auditfails, the user seesExamine failed, …even though they ranaudit. Same string is also used by the matchingexaminearm at lines 80–87, so this is a copy/paste leftover.🛠️ Suggested fix
{command, audit, Opts} -> case run_analyzer_dispatch(Opts, fun run_audit/1) of ok -> maybe_halt(0); {error, Reason} -> - io:format("Examine failed, ~p.\n", [Reason]), + io:format("Audit failed, ~p.\n", [Reason]), maybe_halt(1) 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/atomvm_spectrometer.erl` around lines 64 - 71, The audit command's error path prints "Examine failed" due to copy/paste; update the error message in the {command, audit, Opts} clause (which calls run_analyzer_dispatch(Opts, fun run_audit/1) and then maybe_halt/1) to print a correct "Audit failed, ~p.\n" (or equivalent) using the Reason variable, mirroring the examine arm's structure but with "Audit" instead of "Examine".src/spectrometer_atomvm.erl (1)
146-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpec for
get_supported_functions/0still excludesalland list arities.The comprehension at line 160 binds
Afrom raw DB entries, which can beallor[arity()](handled byfind_arity/3andprint_supported/1). The currentnon_neg_integer()arity in the spec contradicts that, will trip dialyzer on downstream callers, and forces tests to reject valid rows.📝 Suggested fix
-spec get_supported_functions() -> [ { atom(), atom(), - non_neg_integer(), + arity() | [arity()] | all, [atom()] | all, binary() | {unreleased, binary()} } ].🤖 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 146 - 161, The `@spec` for get_supported_functions/0 is incorrect: the third element A in the returned tuples can be the atom all or a list of arities (handled by find_arity/3 and print_supported/1), not just non_neg_integer(); update the type to reflect this by changing the third element's type to non_neg_integer() | all | [non_neg_integer()] so the function spec matches the comprehension ({M, F, A, Platforms, Since}) and downstream Dialyzer checks; ensure the overall return type still matches a list of {atom(), atom(), (non_neg_integer() | all | [non_neg_integer()]), [atom()] | all, binary() | {unreleased, binary()}}.test/cli_main_tests.erl (1)
289-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNegative-path tests still rely on hardcoded
/nonexistentand/tmp/nonexistentpaths.Both
main_audit_missing_dir_test_(line 294) andmain_filter_no_csv_test_(line 326) assume/nonexistent/path/xyz123and/tmp/nonexistent/cacheare absent on every runner. That is not portable (Windows runners, or any runner where someone happened to create those paths) and silently degrades coverage of the intended branch when the path actually exists. Create a unique temp parent and point the CLI at a child path you deliberately do not create.🛠️ Suggested pattern
main_audit_missing_dir_test_() -> {"main(['audit', '--dir', '<missing>']) handles missing directory", fun() -> - %% audit returns empty map for non-existent dirs - Result = atomvm_spectrometer:main([ - "audit", "--dir", "/nonexistent/path/xyz123" - ]), - ?assertEqual(ok, Result) + BaseDir = setup_temp_dir("audit_missing_dir_"), + MissingDir = filename:join(BaseDir, "does_not_exist"), + try + Result = atomvm_spectrometer:main([ + "audit", "--dir", MissingDir + ]), + ?assertEqual(ok, Result) + after + spectrometer_utils:purge_dir(BaseDir) + end end}.Apply the same temp-parent + missing-child pattern to
main_filter_no_csv_test_for the cache path.Also applies to: 322-329
🤖 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/cli_main_tests.erl` around lines 289 - 297, Tests main_audit_missing_dir_test_ and main_filter_no_csv_test_ use hardcoded non-existent paths; change them to create a unique temp parent at runtime (e.g., use filelib:tmp_dir/0 or the system temp + a process-unique suffix via erlang:unique_integer() or now()) and then pass a child path under that parent which you deliberately do NOT create so the missing-dir branch is tested reliably; update the calls to atomvm_spectrometer:main in main_audit_missing_dir_test_ and the cache path in main_filter_no_csv_test_ to use filename:join of the temp parent and a non-created child.
🧹 Nitpick comments (1)
src/spectrometer_otp.erl (1)
22-26: ⚡ Quick win
modules_list/0is called on everyis_otp_module/1invocation — consider memoizing.Each call to
is_otp_module/1reads and deserializes the cache file from disk. When the reporter checks every function from a large scanned project, this means O(N) file reads for a result that is stable across the entire run.
persistent_termis a clean fit here — the OTP module list never changes within a single process invocation:♻️ Suggested approach
-modules_list() -> - ModFile = module_cache(), - ... +modules_list() -> + case persistent_term:get({?MODULE, otp_modules}, undefined) of + undefined -> + Modules = load_modules_list(), + persistent_term:put({?MODULE, otp_modules}, Modules), + Modules; + Cached -> + Cached + end. + +load_modules_list() -> + ModFile = module_cache(), + case filelib:is_file(ModFile) of + true -> + ... %% existing file-read logic + false -> + ... %% existing generate-and-cache logic + 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_otp.erl` around lines 22 - 26, The is_otp_module/1 function currently calls modules_list() on every invocation which repeatedly reads/deserializes the cache file; change it to fetch a cached value from persistent_term once per VM run: introduce a unique persistent_term key (e.g. {spectrometer_otp, otp_modules}), on first access call modules_list(), persistent_term:put(Key, OTPmods) and thereafter read with persistent_term:get(Key) inside is_otp_module/1 (preserve both clauses that accept atoms and lists and keep atom_to_list/1 conversion), ensuring you only call modules_list() when the persistent_term key is missing.
🤖 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 `@src/atomvm_spectrometer.erl`:
- Around line 395-401: parse_filter_args currently treats any unmatched token as
the csv_file which lets mistyped flags (e.g. "--avmm") become a bogus filepath;
update parse_filter_args([MaybeFile | Rest], Opts) so before assigning csv_file
it rejects tokens that look like options: if MaybeFile begins with "-" (i.e.
first char is $-), return {error, "Unknown option: " ++ MaybeFile}; otherwise
proceed to set Opts#{csv_file => MaybeFile} and continue. Reference
parse_filter_args and the csv_file key in Opts when making this change.
In `@src/spectrometer_atomvm.erl`:
- Around line 197-219: load_db/0 and load_data/0 contain identical bodies and
should be collapsed to a single canonical implementation to avoid future drift:
pick one function name (e.g., load_db/0) as the real implementation (the body
that reads/puts get(supported_db) and calls load_db_internal()), then make the
other function (load_data/0) delegate by calling the canonical function (or
remove load_data/0 entirely if there are no external callers); update any module
exports or call sites accordingly and keep references to load_db_internal()
intact.
In `@src/spectrometer_ecosystem.erl`:
- Around line 210-221: The current logic in the {result, …} handling loop leaves
NewSS2 = NewSS when save_state/3 fails, so NewSS remains ≥ ?SAVE_INTERVAL and
the next message immediately retries and floods I/O; change the failure branch
in the NewSS2 case so that on {error, Reason} you reset the counter (set NewSS2
to 0) and emit a single warning/error log containing the Reason (use your
project's logger in this module) so the next save attempt happens only after
another ?SAVE_INTERVAL items and the failure is visible; update references to
save_state/3, NewSS/NewSS2 and the {result, …} handling accordingly.
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 51-67: The test claims to exercise scan_target/2 but calls
spectrometer_analyzer:scan_target/1; update the test to call the two-argument
function or change the header/description to match arity-1. Specifically, either
change the invocation in scan_target_with_opts_test_ to
spectrometer_analyzer:scan_target({local_dir, Dir}, #{}) (or pass a realistic
options map) and keep the "scan_target/2 tests" header and "accepts options map"
description, or rename the section and test description to reflect that it calls
scan_target/1; ensure the assertion remains is_map(Stats) and cleanup still uses
spectrometer_utils:purge_dir(Dir).
In `@test/spectrometer_reporter_tests.erl`:
- Around line 202-215: The test write_csv_limit_all_test_ currently never calls
write_csv/3 with the "all" limit and instead uses write_csv_to_temp/0 (which
uses write_csv/2), so update the test to directly exercise write_csv/3 with the
"all" literal: replace the call to write_csv_to_temp/0 in
write_csv_limit_all_test_ with a direct invocation of write_csv/3 (or create a
temporary file path and call write_csv(Records, FilePath, "all")), then read and
assert the output lines as before and ensure cleanup via
spectrometer_utils:purge_dir/1; keep the test name write_csv_limit_all_test_ and
assert the expected 3 lines to validate the "all" branch.
---
Duplicate comments:
In `@src/atomvm_spectrometer.erl`:
- Around line 64-71: The audit command's error path prints "Examine failed" due
to copy/paste; update the error message in the {command, audit, Opts} clause
(which calls run_analyzer_dispatch(Opts, fun run_audit/1) and then maybe_halt/1)
to print a correct "Audit failed, ~p.\n" (or equivalent) using the Reason
variable, mirroring the examine arm's structure but with "Audit" instead of
"Examine".
In `@src/spectrometer_atomvm.erl`:
- Around line 146-161: The `@spec` for get_supported_functions/0 is incorrect: the
third element A in the returned tuples can be the atom all or a list of arities
(handled by find_arity/3 and print_supported/1), not just non_neg_integer();
update the type to reflect this by changing the third element's type to
non_neg_integer() | all | [non_neg_integer()] so the function spec matches the
comprehension ({M, F, A, Platforms, Since}) and downstream Dialyzer checks;
ensure the overall return type still matches a list of {atom(), atom(),
(non_neg_integer() | all | [non_neg_integer()]), [atom()] | all, binary() |
{unreleased, binary()}}.
In `@src/spectrometer_help.erl`:
- Line 31: The help output contains a stray bracket in the literal
"Commands:]\n" within src/spectrometer_help.erl; locate that string (e.g., in
the help-generation function) and remove the extra ']' so it reads
"Commands:\n", and scan the file for any other occurrences of "Commands:]" to
correct them as well.
In `@test/cli_main_tests.erl`:
- Around line 289-297: Tests main_audit_missing_dir_test_ and
main_filter_no_csv_test_ use hardcoded non-existent paths; change them to create
a unique temp parent at runtime (e.g., use filelib:tmp_dir/0 or the system temp
+ a process-unique suffix via erlang:unique_integer() or now()) and then pass a
child path under that parent which you deliberately do NOT create so the
missing-dir branch is tested reliably; update the calls to
atomvm_spectrometer:main in main_audit_missing_dir_test_ and the cache path in
main_filter_no_csv_test_ to use filename:join of the temp parent and a
non-created child.
---
Nitpick comments:
In `@src/spectrometer_otp.erl`:
- Around line 22-26: The is_otp_module/1 function currently calls modules_list()
on every invocation which repeatedly reads/deserializes the cache file; change
it to fetch a cached value from persistent_term once per VM run: introduce a
unique persistent_term key (e.g. {spectrometer_otp, otp_modules}), on first
access call modules_list(), persistent_term:put(Key, OTPmods) and thereafter
read with persistent_term:get(Key) inside is_otp_module/1 (preserve both clauses
that accept atoms and lists and keep atom_to_list/1 conversion), ensuring you
only call modules_list() when the persistent_term key is missing.
🪄 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: de746d7a-466e-491b-a524-3c03de2aca14
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (4)
- rebar.lock.license
- include/ecosystem.hrl
- src/spectrometer.app.src
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (12)
- src/spectrometer.erl
- generate_fun_data.sh
- test/spectrometer_atomvm_tests.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_analyzer.erl
- src/spectrometer_scanner.erl
- test/spectrometer_http_tests.erl
- src/spectrometer_reporter.erl
- src/spectrometer_utils.erl
- src/spectrometer_http.erl
- test/atomvm_spectrometer_tests.erl
- src/spectrometer_updater.erl
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/spectrometer_utils.erl (1)
356-379: ⚡ Quick win
version/0does far more than needed and runs on every--help.Reading the
vsnkey from the application resource file only requiresapplication:load/1; there's no need to startcompiler,syntax_tools,inets,ssl, or evenspectrometeritself. Sincespectrometer_help:usage/0callsversion/0, everyspectrometer --helpinvocation currently pays the cost of startinginets/ssl/etc. Consider simplifying:♻️ Suggested simplification
-version() -> - Startup = application:ensure_all_started([ - compiler, syntax_tools, inets, ssl - ]), - case Startup of - {ok, _} -> - case application:start(spectrometer) of - ok -> - case application:get_key(spectrometer, vsn) of - {ok, Vsn} -> Vsn; - undefined -> {error, version_not_found} - end; - {error, {already_started, _}} -> - case application:get_key(spectrometer, vsn) of - {ok, Vsn} -> Vsn; - undefined -> {error, version_not_found} - end; - {error, R1} -> - {error, R1} - end; - {error, Reason} -> - {error, Reason} - end. +version() -> + _ = application:load(spectrometer), + case application:get_key(spectrometer, vsn) of + {ok, Vsn} -> Vsn; + undefined -> {error, version_not_found} + end.
start_applications/0remains the right place to bring up the network stack for commands that actually need it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_utils.erl` around lines 356 - 379, version/0 currently starts multiple applications (compiler, syntax_tools, inets, ssl) and even attempts to start spectrometer just to read the vsn, which runs on every --help; change version/0 to only load the application resources (use application:load(spectrometer) or application:load/1) and then call application:get_key(spectrometer, vsn) to fetch the version, returning {error, version_not_found} when undefined and passing through load errors; leave start_applications/0 as the place to start inets/ssl/other runtime deps for commands that actually need them.src/spectrometer_http.erl (1)
85-92: 💤 Low value
star_filter_range(infinity, LastStars)would emit invalid GitHub syntax (likely unreachable).
">=1..~p"is not a validstars:qualifier — GitHub accepts either>=NorN..M, not both. In practice this clause is unreachable: onceMinStars =:= infinity, the guardLastStars < MinStars(Line 57) is satisfied for any integerLastStarsdue to Erlang's term ordering (number < atom), so recursion terminates before this clause runs. Either drop the dead clauses or guardMinStarsso theinfinitypath is consistent (e.g., always treat it as>=1).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_http.erl` around lines 85 - 92, The star_filter_range/2 clause for star_filter_range(infinity, LastStars) produces invalid GitHub syntax (">=1..~p") and appears unreachable; update the star_filter_range function to either remove this dead clause or make the infinity branch consistent by returning ">=1" (same as star_filter_range(infinity, undefined)) and/or add an explicit guard so MinStars =:= infinity is handled only by the >=1 case; locate and change the clauses inside star_filter_range/2 to ensure no branch emits a combined ">=N..M" pattern.src/spectrometer_reporter.erl (1)
99-111: 💤 Low value
int/1guard adds little.
FilteredUnsupported/FilteredSupportedalready carry integer counts produced by this module, so wrapping them inint/1(Line 111) is a no-op guard. You can replacelists:sum([int(C0) || {_, C0} <- ...])with a directlists:sum([C0 || {_, C0} <- ...])and dropint/1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_reporter.erl` around lines 99 - 111, The int/1 guard is redundant — remove the int/1 clause and simplify the TotalCalls computation to sum the counts directly by replacing lists:sum([int(C0) || {_, C0} <- FilteredUnsupported]) and lists:sum([int(C1) || {_, C1} <- FilteredSupported]) with lists:sum([C0 || {_, C0} <- FilteredUnsupported]) and lists:sum([C1 || {_, C1} <- FilteredSupported]) respectively, and then delete the int/1 function definition to avoid the no-op wrapper; keep all other bindings (FilteredUnsupported, FilteredSupported, TotalCalls, TotalUnique) intact.
🤖 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 `@src/spectrometer_utils.erl`:
- Around line 313-353: Summary: io:format calls use "\\n" (literal backslash+n)
so newlines are printed incorrectly; replace them with the idiomatic newline
directive. Fix: in the cloning/error handling block that uses CloneResult,
CheckoutResult, run_git_command and purge_dir, replace all occurrences of the
string "\\n" in io:format format strings with either "\n" or preferably the
io:format directive "~n" (e.g., "Cloning ~s (branch ~s) to ~s...~n", "Checking
out tag ~s...~n", and the two error messages) so actual newlines are emitted.
Ensure you update all four occurrences in that block.
---
Nitpick comments:
In `@src/spectrometer_http.erl`:
- Around line 85-92: The star_filter_range/2 clause for
star_filter_range(infinity, LastStars) produces invalid GitHub syntax
(">=1..~p") and appears unreachable; update the star_filter_range function to
either remove this dead clause or make the infinity branch consistent by
returning ">=1" (same as star_filter_range(infinity, undefined)) and/or add an
explicit guard so MinStars =:= infinity is handled only by the >=1 case; locate
and change the clauses inside star_filter_range/2 to ensure no branch emits a
combined ">=N..M" pattern.
In `@src/spectrometer_reporter.erl`:
- Around line 99-111: The int/1 guard is redundant — remove the int/1 clause and
simplify the TotalCalls computation to sum the counts directly by replacing
lists:sum([int(C0) || {_, C0} <- FilteredUnsupported]) and lists:sum([int(C1) ||
{_, C1} <- FilteredSupported]) with lists:sum([C0 || {_, C0} <-
FilteredUnsupported]) and lists:sum([C1 || {_, C1} <- FilteredSupported])
respectively, and then delete the int/1 function definition to avoid the no-op
wrapper; keep all other bindings (FilteredUnsupported, FilteredSupported,
TotalCalls, TotalUnique) intact.
In `@src/spectrometer_utils.erl`:
- Around line 356-379: version/0 currently starts multiple applications
(compiler, syntax_tools, inets, ssl) and even attempts to start spectrometer
just to read the vsn, which runs on every --help; change version/0 to only load
the application resources (use application:load(spectrometer) or
application:load/1) and then call application:get_key(spectrometer, vsn) to
fetch the version, returning {error, version_not_found} when undefined and
passing through load errors; leave start_applications/0 as the place to start
inets/ssl/other runtime deps for commands that actually need them.
🪄 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: aba7dd43-71c7-4b1e-8b3d-e41cee31c377
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (7)
- include/ecosystem.hrl
- rebar.lock.license
- src/spectrometer.erl
- .gitignore
- src/spectrometer.app.src
- test/spectrometer_scanner_tests.erl
- src/spectrometer_otp.erl
🚧 Files skipped from review as they are similar to previous changes (13)
- test/spectrometer_utils_tests.erl
- test/spectrometer_atomvm_tests.erl
- test/spectrometer_reporter_tests.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_analyzer.erl
- generate_fun_data.sh
- test/spectrometer_analyzer_tests.erl
- src/spectrometer_ecosystem.erl
- src/atomvm_spectrometer.erl
- test/cli_main_tests.erl
- test/atomvm_spectrometer_tests.erl
- src/spectrometer_atomvm.erl
- src/spectrometer_updater.erl
UncleGrumpy
force-pushed
the
WIP
branch
2 times, most recently
from
cf936b0 to
b098475
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/cli_main_tests.erl (1)
362-376:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
cache_direnv in teardown to prevent cross-test state leakage.Line 367 sets application env, but cleanup at Line 374-375 never unsets it. This can make later tests order-dependent.
💡 Suggested fix
fun() -> CacheDir = spectrometer_utils:make_temp_dir("mock_cache_"), ok = filelib:ensure_path(CacheDir), application:set_env(spectrometer, cache_dir, CacheDir), try Result = atomvm_spectrometer:main([ "filter", "--min-repos", "10" ]), ?assertEqual({error, {halt, 1}}, Result) after + application:unset_env(spectrometer, cache_dir), + spectrometer_atomvm:reload_db(), spectrometer_utils:purge_dir(CacheDir) 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/cli_main_tests.erl` around lines 362 - 376, The test main_filter_no_user_state_test_() sets application env cache_dir via application:set_env(spectrometer, cache_dir, CacheDir) but never restores it, causing cross-test leakage; update the try/after to capture the previous value with application:get_env(spectrometer, cache_dir) before set, then in the after block call spectrometer_utils:purge_dir(CacheDir) and restore the original env (either application:unset_env(spectrometer, cache_dir) if none or application:set_env(spectrometer, cache_dir, Prev) if Prev existed) so the CacheDir and application env are cleaned up and restored.
🧹 Nitpick comments (2)
.github/workflows/check-formatting.yaml (1)
64-64: ⚡ Quick winCache key has a duplicate
otp_prefix for the rebar3 version segment.Line 64 contains
…-otp_${{steps.otp.outputs.otp-version}}-otp_${{steps.otp.outputs.rebar3-version}}-…. The secondotp_should berebar3_(or similar). Cache invalidation still works in practice because the values differ, but the mislabelled key makes it harder to reason about what changed when a cache is invalidated.🔧 Proposed fix
- key: ci-${{runner.os}}-${{env.cache-name}}-${{env.ACTIONLINT_VERSION}}-otp_${{steps.otp.outputs.otp-version}}-otp_${{steps.otp.outputs.rebar3-version}}-erlfmt_${{env.ERLFMT_VERSION}} + key: ci-${{runner.os}}-${{env.cache-name}}-${{env.ACTIONLINT_VERSION}}-otp_${{steps.otp.outputs.otp-version}}-rebar3_${{steps.otp.outputs.rebar3-version}}-erlfmt_${{env.ERLFMT_VERSION}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check-formatting.yaml at line 64, The cache key string in the workflow uses a duplicated prefix "otp_" for the rebar3 version segment (the fragment shown as key: ci-${{runner.os}}-${{env.cache-name}}-${{env.ACTIONLINT_VERSION}}-otp_${{steps.otp.outputs.otp-version}}-otp_${{steps.otp.outputs.rebar3-version}}-erlfmt_${{env.ERLFMT_VERSION}}), which mislabels the rebar3 version; change the second "otp_" prefix to a clear identifier such as "rebar3_" so the segment reads rebar3_${{steps.otp.outputs.rebar3-version}} (or another unambiguous name) to correctly reflect the value in the cache key.src/spectrometer_scanner.erl (1)
78-83: ⚡ Quick winConsider also skipping
.gitin the directory exclusion list.The
.gitdirectory never contains.erlfiles, but it is traversed (potentially deeply for large repos), adding pointless syscall overhead. The{error, _}fallback already handles any permission errors safely, so this is purely a performance nit.🔧 Proposed fix
case Entry of "_build" -> A; "deps" -> A; ".rebar3" -> A; + ".git" -> A; _ -> find_erl_files(Path, A) 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_scanner.erl` around lines 78 - 83, The directory traversal in find_erl_files currently skips "_build", "deps", and ".rebar3" but not ".git", causing unnecessary recursion; update the case inside find_erl_files (the clause matching Entry) to also match ".git" -> A so the .git directory is ignored in the same way as the other excluded directories to avoid extra syscall overhead.
🤖 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 `@README.md`:
- Around line 36-43: Add the missing "examine" CLI command to the README
commands table and examples: insert an `examine` row mirroring the `audit` row's
description ("Examine a single target for AtomVM support (GitHub repo, Hex
package, or directory)") alongside `audit` in the table and add `examine` to the
help/examples block so it's shown with the same usage as `audit`; reference the
implemented command in src/atomvm_spectrometer.erl (comment notes "examine and
audit have the same options") to ensure text matches the code behavior.
In `@test/cli_main_tests.erl`:
- Around line 65-140: The startup crash is caused by passing the
{already_loaded,spectrometer} tuple into a logging/format path in
atomvm_spectrometer:main/1 which expects a string and raises badarg; update
main/1 to properly handle application load results by pattern-matching the
return of application:load(spectrometer) (and/or
application:ensure_all_started/1) so that {ok, ...}, {error,{already_loaded,
spectrometer}} and other {error, Reason} cases are handled explicitly; for
logging use a safe formatter like io_lib:format("~p", [Reason]) or convert the
term to string before passing to processLogger/error (or skip logging for
already_loaded and treat it as success), and ensure any code paths that
previously forwarded the raw tuple to io:format or similar are replaced with
these safe handlers.
---
Duplicate comments:
In `@test/cli_main_tests.erl`:
- Around line 362-376: The test main_filter_no_user_state_test_() sets
application env cache_dir via application:set_env(spectrometer, cache_dir,
CacheDir) but never restores it, causing cross-test leakage; update the
try/after to capture the previous value with application:get_env(spectrometer,
cache_dir) before set, then in the after block call
spectrometer_utils:purge_dir(CacheDir) and restore the original env (either
application:unset_env(spectrometer, cache_dir) if none or
application:set_env(spectrometer, cache_dir, Prev) if Prev existed) so the
CacheDir and application env are cleaned up and restored.
---
Nitpick comments:
In @.github/workflows/check-formatting.yaml:
- Line 64: The cache key string in the workflow uses a duplicated prefix "otp_"
for the rebar3 version segment (the fragment shown as key:
ci-${{runner.os}}-${{env.cache-name}}-${{env.ACTIONLINT_VERSION}}-otp_${{steps.otp.outputs.otp-version}}-otp_${{steps.otp.outputs.rebar3-version}}-erlfmt_${{env.ERLFMT_VERSION}}),
which mislabels the rebar3 version; change the second "otp_" prefix to a clear
identifier such as "rebar3_" so the segment reads
rebar3_${{steps.otp.outputs.rebar3-version}} (or another unambiguous name) to
correctly reflect the value in the cache key.
In `@src/spectrometer_scanner.erl`:
- Around line 78-83: The directory traversal in find_erl_files currently skips
"_build", "deps", and ".rebar3" but not ".git", causing unnecessary recursion;
update the case inside find_erl_files (the clause matching Entry) to also match
".git" -> A so the .git directory is ignored in the same way as the other
excluded directories to avoid extra syscall overhead.
🪄 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: 7dda9455-6788-48ba-8cdb-b7238a4bfee5
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignoreREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (11)
- rebar.lock.license
- .github/workflows/code_quality_check.yaml
- test/spectrometer_scanner_tests.erl
- src/spectrometer.app.src
- src/spectrometer_help.erl
- .gitignore
- src/spectrometer_atomvm.erl
- src/spectrometer_http.erl
- test/spectrometer_http_tests.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_utils.erl
🚧 Files skipped from review as they are similar to previous changes (12)
- include/ecosystem.hrl
- src/spectrometer.erl
- generate_fun_data.sh
- test/spectrometer_utils_tests.erl
- rebar.config
- test/spectrometer_analyzer_tests.erl
- test/spectrometer_atomvm_tests.erl
- src/spectrometer_analyzer.erl
- src/spectrometer_ecosystem.erl
- src/spectrometer_reporter.erl
- src/atomvm_spectrometer.erl
- src/spectrometer_updater.erl
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/cli_main_tests.erl (1)
561-568:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
ensure_atomvm_repo()'s parent temp directory is not cleaned up.
ensure_atomvm_repo()creates a temp dir viamake_temp_dir("spectrometer_git_clone_")and returnsAtomVMDir = TempDir/AtomVM. The cleanup here callspurge_dir(AtomVMDir), which removes the repository subdirectory but leaves the parentTempDir(an empty directory) behind. The same applies to themain_update_no_force_overwrite_test_cleanup at line 608. Usefilename:dirname(AtomVMDir)to remove the full parent.🧹 Proposed fix (both update tests)
- spectrometer_utils:purge_dir(AtomVMDir), + spectrometer_utils:purge_dir(filename:dirname(AtomVMDir)),🤖 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/cli_main_tests.erl` around lines 561 - 568, The cleanup currently purges only the repository subdirectory (AtomVMDir) returned by ensure_atomvm_repo(), leaving its parent temporary directory behind; update the after blocks in the tests (including main_update_no_force_overwrite_test_) to purge the parent directory by calling filename:dirname(AtomVMDir) and passing that result to spectrometer_utils:purge_dir/1 instead of purging AtomVMDir directly so the full temp dir created by make_temp_dir("spectrometer_git_clone_") is removed.
🧹 Nitpick comments (1)
test/cli_main_tests.erl (1)
47-47: 💤 Low valueInconsistent
GIT_TERMINAL_PROMPTvalue — use"0"to match the rest of the codebase.This uses
"false"while all production call sites inspectrometer_utils.erlpass"0". Both are accepted by git for boolean env vars, but the inconsistency is a readability nit.🧹 Proposed fix
- [{"GIT_TERMINAL_PROMPT", "false"}] + [{"GIT_TERMINAL_PROMPT", "0"}]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli_main_tests.erl` at line 47, Replace the inconsistent GIT_TERMINAL_PROMPT value in the test setup: locate the environment entry [{"GIT_TERMINAL_PROMPT", "false"}] in test/cli_main_tests.erl and change the value to "0" so it matches the production usage in spectrometer_utils.erl; ensure no other tests use "false" so the codebase consistently uses "0" for this git boolean env var.
🤖 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 `@src/spectrometer_utils.erl`:
- Around line 352-367: The version/0 function contains a dead clause because
application:ensure_all_started/1 never returns a bare ok; change the control
flow in version/0 to remove the unreachable bare ok branch and handle the
successful tuple only (the existing {ok, _} branch) and the {error, Reason}
case, keeping the logic that calls application:get_key(spectrometer, vsn) and
returns {error, version_not_found} when undefined; ensure you only match the
real success shape from application:ensure_all_started/1 and simplify the
function accordingly.
- Around line 314-350: clone_temp_repo/2 currently only matches specific
CloneResult and CheckoutResult shapes and will crash on other error tuples from
run_git_command/2 (e.g. {error, timeout} or {error, {executable_not_found,
Cmd}}); add catch-all error clauses to both the CloneResult and CheckoutResult
case expressions to handle {error, Reason} (and any other unmatched error tuple)
by purging TmpDir when appropriate and returning {error, Reason} (or forwarding
the original error tuple) instead of letting a case_clause occur; update the
branches handling CheckoutResult (the case bound to CheckoutResult) and the
outer case bound to CloneResult to include these catch-all patterns and ensure
TmpDir is cleaned up before returning.
---
Duplicate comments:
In `@test/cli_main_tests.erl`:
- Around line 561-568: The cleanup currently purges only the repository
subdirectory (AtomVMDir) returned by ensure_atomvm_repo(), leaving its parent
temporary directory behind; update the after blocks in the tests (including
main_update_no_force_overwrite_test_) to purge the parent directory by calling
filename:dirname(AtomVMDir) and passing that result to
spectrometer_utils:purge_dir/1 instead of purging AtomVMDir directly so the full
temp dir created by make_temp_dir("spectrometer_git_clone_") is removed.
---
Nitpick comments:
In `@test/cli_main_tests.erl`:
- Line 47: Replace the inconsistent GIT_TERMINAL_PROMPT value in the test setup:
locate the environment entry [{"GIT_TERMINAL_PROMPT", "false"}] in
test/cli_main_tests.erl and change the value to "0" so it matches the production
usage in spectrometer_utils.erl; ensure no other tests use "false" so the
codebase consistently uses "0" for this git boolean env var.
🪄 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: 5979a58d-3d3b-48a1-8145-a3c3b668e36b
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignoreLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datarebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (18)
- include/ecosystem.hrl
- LICENSES/CC0-1.0.txt
- rebar.lock.license
- LICENSES/Apache-2.0.txt
- LICENSES/LGPL-2.1-or-later.txt
- .github/workflows/code_quality_check.yaml
- src/spectrometer.erl
- src/spectrometer.app.src
- .gitignore
- .github/workflows/publish_docs.yml
- .github/workflows/build-and-test.yaml
- test/spectrometer_analyzer_tests.erl
- .github/workflows/check-formatting.yaml
- test/spectrometer_utils_tests.erl
- test/atomvm_spectrometer_tests.erl
- src/spectrometer_help.erl
- src/spectrometer_otp.erl
- src/spectrometer_analyzer.erl
🚧 Files skipped from review as they are similar to previous changes (15)
- .github/workflows/reuse-lint.yaml
- generate_fun_data.sh
- src/spectrometer_scanner.erl
- rebar.config
- test/spectrometer_reporter_tests.erl
- test/spectrometer_atomvm_tests.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_atomvm.erl
- src/spectrometer_ecosystem.erl
- src/spectrometer_reporter.erl
- test/spectrometer_http_tests.erl
- src/spectrometer_updater.erl
- test/spectrometer_scanner_tests.erl
- src/atomvm_spectrometer.erl
- src/spectrometer_http.erl
UncleGrumpy
force-pushed
the
WIP
branch
5 times, most recently
from
83a217d to
e857787
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
README.md (1)
54-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
spectrometer help examineis missing from the help command examples.The commands table (line 40) lists
examine, but the "Get detailed help on any command" block omits it. Users following this section of the README can't discover per-command help forexamine.📝 Proposed fix
$ spectrometer help audit $ spectrometer help ecosystem + $ spectrometer help examine $ spectrometer help supported🤖 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 `@README.md` around lines 54 - 61, Add the missing "examine" command to the help examples by inserting "spectrometer help examine" into the "Get detailed help on any command:" block so it matches the commands table; update the example list that currently contains audit, ecosystem, supported, filter, update, query to also include the examine example to ensure users can discover per-command help for examine.src/spectrometer_http.erl (1)
79-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCursor still re-fetches boundary repos at the exact
MinStarsInBatch.
fetch_github_cursor(MinStars, MinStarsInBatch, ...)passes the lower bound unchanged, thenstar_filter_range/2builds an inclusive"<MinStars>..<MinStarsInBatch>"range. Repos withstars =:= MinStarsInBatch(frequent at low star counts) come back again on the next page untilMaxis hit, padding the result with duplicates. Either advance withMinStarsInBatch - 1or dedupe byclone_url/full_nameinAcc.♻️ Suggested cursor advancement
Stars = [maps:get(stars, R) || R <- Repos], MinStarsInBatch = lists:min(Stars), fetch_github_cursor( - MinStars, MinStarsInBatch, NewAcc, Max + MinStars, MinStarsInBatch - 1, NewAcc, Max )🤖 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_http.erl` around lines 79 - 81, The cursor re-fetches boundary repos because fetch_github_cursor/3 is called with MinStars unchanged so star_filter_range/2 produces an inclusive MinStars..MinStarsInBatch and items with stars == MinStarsInBatch appear on the next page; fix by advancing the next lower bound to MinStarsInBatch - 1 when recursing from fetch_github_cursor (or alternatively dedupe entries added to Acc by unique key like clone_url or full_name before returning) and ensure the recursion uses the updated MinStars value so duplicates are not accumulated up to Max.
🧹 Nitpick comments (2)
.github/workflows/build-and-test.yaml (1)
65-77: ⚡ Quick winNo
rebar3 dialyzerstep despite PR claiming "dialyzer is clean".Without a CI dialyzer step, future changes can silently break the type-checking guarantee. The cache step is even named "rebar3 dialyzer and test-coverage cache" (line 54–55), suggesting this was intended.
🔧 Proposed addition
- name: "Run Tests" run: | rebar3 eunit rebar3 cover + - name: "Run Dialyzer" + run: | + rebar3 dialyzer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-and-test.yaml around lines 65 - 77, CI is missing a Dialyzer step even though the PR claims "dialyzer is clean"; add a job step named similarly to the cache ("rebar3 dialyzer") that runs "rebar3 dialyzer" (and ensure it runs before or alongside "Run Tests") so type-checking is executed in CI; update the workflow steps around "Build escripts", "Build docs", and "Run Tests" to include a "Build/Run dialyzer" step that invokes rebar3 dialyzer and uses the existing dialyzer cache key to speed runs.src/spectrometer_http.erl (1)
253-271: 💤 Low valueDefensive type guard on link
Value.
binary_to_list(Value)will crash withbadargif a Hexlinksmap ever contains a non-binary value (e.g., a nested map ornulldecoded tonil). Since the surroundingtry/catchinfetch_hex_pages/3only wraps the json path and would discard the rest of the page on failure, a single malformed package could drop a whole page of results. Cheap to harden:🛡️ Suggested guard
find_github_link(Links) when is_map(Links) -> maps:fold( fun(_Key, Value, Acc) -> - case Acc of - "" -> - Url = binary_to_list(Value), + case {Acc, is_binary(Value)} of + {"", true} -> + Url = binary_to_list(Value), case string:find(Url, "github.com") of nomatch -> ""; _ -> Url end; _ -> Acc end end, "", Links );🤖 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_http.erl` around lines 253 - 271, The fold in find_github_link/1 currently calls binary_to_list(Value) unguarded and will crash if Value isn't a binary; update the folding function in find_github_link to only convert/check Values that are binaries (e.g., test is_binary(Value) before binary_to_list) and otherwise skip/return Acc unchanged so non-binary entries (maps, nil) are ignored; this prevents a bad Value from crashing the function and dropping a whole page (see find_github_link and its use in fetch_hex_pages/3).
🤖 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 `@README.md`:
- Line 134: The README example shows "spectrometer update --atomvm-dir
~/work/AtomVM --branch release-0.7" which is misleading because the update
command's usage (usage_update/0) documents that the --atomvm-dir option ignores
--branch/--tag; update the example to either remove --branch or add an explicit
caveat so it doesn't imply the branch will be used. Specifically, edit the
example line and/or the surrounding text to (a) show only the
mutually-compatible flags (e.g., drop --branch when using --atomvm-dir) or (b)
append a short note like "Note: --atomvm-dir ignores --branch/--tag" so readers
understand the behavior when using the update command.
In `@test/cli_main_tests.erl`:
- Around line 32-59: ensure_atomvm_repo/0 creates a TempDir via
spectrometer_utils:make_temp_dir("spectrometer_git_clone_") but only returns and
later purges the AtomVM child directory, leaving the parent TempDir behind;
update the tests that clean up after network runs (the after blocks for the
tests referencing ensure_atomvm_repo/0, including the
main_update_no_force_overwrite_test_ cleanup) to also remove the parent TempDir
(the variable TempDir created in ensure_atomvm_repo/0) by calling
spectrometer_utils:purge_dir(TempDir) or equivalent; locate usages around the
cleanup blocks that currently call purge_dir(AtomVMDir) and add the additional
purge for TempDir so both AtomVMDir and its parent are removed.
---
Duplicate comments:
In `@README.md`:
- Around line 54-61: Add the missing "examine" command to the help examples by
inserting "spectrometer help examine" into the "Get detailed help on any
command:" block so it matches the commands table; update the example list that
currently contains audit, ecosystem, supported, filter, update, query to also
include the examine example to ensure users can discover per-command help for
examine.
In `@src/spectrometer_http.erl`:
- Around line 79-81: The cursor re-fetches boundary repos because
fetch_github_cursor/3 is called with MinStars unchanged so star_filter_range/2
produces an inclusive MinStars..MinStarsInBatch and items with stars ==
MinStarsInBatch appear on the next page; fix by advancing the next lower bound
to MinStarsInBatch - 1 when recursing from fetch_github_cursor (or alternatively
dedupe entries added to Acc by unique key like clone_url or full_name before
returning) and ensure the recursion uses the updated MinStars value so
duplicates are not accumulated up to Max.
---
Nitpick comments:
In @.github/workflows/build-and-test.yaml:
- Around line 65-77: CI is missing a Dialyzer step even though the PR claims
"dialyzer is clean"; add a job step named similarly to the cache ("rebar3
dialyzer") that runs "rebar3 dialyzer" (and ensure it runs before or alongside
"Run Tests") so type-checking is executed in CI; update the workflow steps
around "Build escripts", "Build docs", and "Run Tests" to include a "Build/Run
dialyzer" step that invokes rebar3 dialyzer and uses the existing dialyzer cache
key to speed runs.
In `@src/spectrometer_http.erl`:
- Around line 253-271: The fold in find_github_link/1 currently calls
binary_to_list(Value) unguarded and will crash if Value isn't a binary; update
the folding function in find_github_link to only convert/check Values that are
binaries (e.g., test is_binary(Value) before binary_to_list) and otherwise
skip/return Acc unchanged so non-binary entries (maps, nil) are ignored; this
prevents a bad Value from crashing the function and dropping a whole page (see
find_github_link and its use in fetch_hex_pages/3).
🪄 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: 661fce47-c41d-4f6d-8860-36cf903e4a50
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignoreLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (12)
- priv/supported_functions.data.license
- rebar.lock.license
- LICENSES/LGPL-2.1-or-later.txt
- LICENSES/Apache-2.0.txt
- .github/workflows/reuse-lint.yaml
- src/spectrometer.app.src
- .gitignore
- LICENSES/CC0-1.0.txt
- rebar.config
- src/spectrometer_reporter.erl
- test/spectrometer_http_tests.erl
- src/spectrometer_atomvm.erl
🚧 Files skipped from review as they are similar to previous changes (16)
- include/ecosystem.hrl
- .github/workflows/code_quality_check.yaml
- src/spectrometer_otp.erl
- .github/workflows/check-formatting.yaml
- .github/workflows/publish_docs.yml
- generate_fun_data.sh
- test/spectrometer_analyzer_tests.erl
- src/spectrometer_scanner.erl
- test/spectrometer_reporter_tests.erl
- src/spectrometer_analyzer.erl
- src/spectrometer.erl
- src/spectrometer_ecosystem.erl
- src/spectrometer_utils.erl
- src/atomvm_spectrometer.erl
- test/spectrometer_atomvm_tests.erl
- src/spectrometer_updater.erl
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/cli_main_tests.erl (1)
32-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the parent temp directory created by
ensure_atomvm_repo/0.Line 33 creates
TempDir, but cleanup only purgesAtomVMDir(Lines 573 and 615). This leaves empty temp dirs after each run.Suggested patch
- spectrometer_utils:purge_dir(AtomVMDir), + spectrometer_utils:purge_dir( + filename:dirname(AtomVMDir) + ), spectrometer_utils:purge_dir(CacheDir) @@ - spectrometer_utils:purge_dir(AtomVMDir) + spectrometer_utils:purge_dir( + filename:dirname(AtomVMDir) + )Also applies to: 568-575, 610-616
🤖 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/cli_main_tests.erl` around lines 32 - 35, The helper ensure_atomvm_repo/0 currently creates TempDir and purges only AtomVMDir, leaving TempDir behind; after you call spectrometer_utils:purge_dir(AtomVMDir) add a call to spectrometer_utils:purge_dir(TempDir) (or otherwise remove TempDir) so the parent temporary directory is cleaned up; update both occurrences of this pattern (the blocks around ensure_atomvm_repo/0 and the similar code at the other noted locations) to purge TempDir after AtomVMDir.
🧹 Nitpick comments (1)
test/spectrometer_analyzer_tests.erl (1)
121-133: ⚡ Quick winAvoid brittle expectations against mutable external sources in network tests.
Lines 121-133 / 153-164 / 183-192 assert content-level outcomes from third-party repos/packages. Those can change and make CI flaky even when your code is correct. Prefer stable invariants (shape/error handling) or pinned fixtures you control.
Also applies to: 153-164, 183-192
🤖 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_analyzer_tests.erl` around lines 121 - 133, The test scan_target_github_url_test_ (and the other two similar tests) currently asserts exact repository content which is brittle; change them to verify stable invariants instead—e.g. confirm spectrometer_analyzer:scan_target returns a map or returns an error tuple when network tests are skipped, or pin a local fixture repo/archive and assert expected contents from that controlled fixture; update the assertions in scan_target_github_url_test_, and the analogous tests around the other mentions, to check for shape/type (is_map/keys present) or proper error handling rather than exact empty-map content so CI won’t break when external repos change.
🤖 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 `@test/cli_main_tests.erl`:
- Around line 386-388: The tests currently run network paths when
os:getenv("SKIP_NETWORK_TESTS") is unset (checked as false); change them to be
opt-in by checking os:getenv("RUN_NETWORK_TESTS") == "true" instead so network
tests are skipped by default. Locate the case expressions that guard the network
examples (the ones returning tuples whose descriptions include "main(['filter',
'--min-repos', '1']) returns ok on success" and the similar blocks at the other
noted locations) and replace the SKIP_NETWORK_TESTS check with a
RUN_NETWORK_TESTS == "true" check, ensuring the branches invert so the network
test cases are only included when RUN_NETWORK_TESTS is explicitly "true"; apply
the same change to the other occurrences mentioned in the review.
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 45-47: Replace the hardcoded absolute path passed to
spectrometer_analyzer:scan_target with a guaranteed-missing temp child: create a
unique temporary directory (e.g., via filename:module:join/2 with os:getpid or
make_ref) and then point scan_target at a child path beneath it that you
intentionally do not create; update the test to call
spectrometer_analyzer:scan_target({local_dir, MissingChildPath}) where
MissingChildPath is constructed from the temp parent plus a child name (so the
path is environment-independent and guaranteed absent).
---
Duplicate comments:
In `@test/cli_main_tests.erl`:
- Around line 32-35: The helper ensure_atomvm_repo/0 currently creates TempDir
and purges only AtomVMDir, leaving TempDir behind; after you call
spectrometer_utils:purge_dir(AtomVMDir) add a call to
spectrometer_utils:purge_dir(TempDir) (or otherwise remove TempDir) so the
parent temporary directory is cleaned up; update both occurrences of this
pattern (the blocks around ensure_atomvm_repo/0 and the similar code at the
other noted locations) to purge TempDir after AtomVMDir.
---
Nitpick comments:
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 121-133: The test scan_target_github_url_test_ (and the other two
similar tests) currently asserts exact repository content which is brittle;
change them to verify stable invariants instead—e.g. confirm
spectrometer_analyzer:scan_target returns a map or returns an error tuple when
network tests are skipped, or pin a local fixture repo/archive and assert
expected contents from that controlled fixture; update the assertions in
scan_target_github_url_test_, and the analogous tests around the other mentions,
to check for shape/type (is_map/keys present) or proper error handling rather
than exact empty-map content so CI won’t break when external repos change.
🪄 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: 1b188d73-317d-49c3-854c-19283163d858
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignoreLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (5)
- priv/supported_functions.data.license
- src/spectrometer.app.src
- rebar.lock.license
- test/spectrometer_scanner_tests.erl
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (26)
- include/ecosystem.hrl
- .github/workflows/reuse-lint.yaml
- src/spectrometer_otp.erl
- src/spectrometer_scanner.erl
- .github/workflows/publish_docs.yml
- test/spectrometer_reporter_tests.erl
- .github/workflows/build-and-test.yaml
- .github/workflows/code_quality_check.yaml
- .github/workflows/check-formatting.yaml
- src/spectrometer.erl
- rebar.config
- generate_fun_data.sh
- test/spectrometer_http_tests.erl
- src/spectrometer_atomvm.erl
- src/spectrometer_updater.erl
- src/spectrometer_help.erl
- test/spectrometer_atomvm_tests.erl
- src/atomvm_spectrometer.erl
- test/spectrometer_utils_tests.erl
- src/spectrometer_ecosystem.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_utils.erl
- src/spectrometer_analyzer.erl
- test/atomvm_spectrometer_tests.erl
- src/spectrometer_reporter.erl
- src/spectrometer_http.erl
Owner
Author
@coderabbitai Dialyzer is run in the code_quality_check.yaml workflow. |
UncleGrumpy
force-pushed
the
WIP
branch
3 times, most recently
from
5ed9582 to
428aa8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/spectrometer_analyzer_tests.erl (1)
45-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid hardcoded
/tmpin this test path setup.Line 45 hardcodes a Unix-specific temp root, which makes this test less portable than the rest of the file (which already uses
spectrometer_utils:make_temp_dir/1).Suggested patch
scan_local_dir_nonexistent_test_() -> {"returns empty map for non-existent directory", fun() -> - TempParent = filename:join([ - "/tmp", - "spectrometer_test_" ++ os:getpid() ++ "_" ++ - integer_to_list(erlang:unique_integer([positive])) - ]), - MissingChild = filename:join(TempParent, "nonexistent_child"), - Stats = spectrometer_analyzer:scan_target({local_dir, MissingChild}), - ?assertEqual(0, maps:size(Stats)) + TempParent = spectrometer_utils:make_temp_dir("analyzer_missing_"), + MissingChild = filename:join(TempParent, "nonexistent_child"), + try + Stats = spectrometer_analyzer:scan_target({local_dir, MissingChild}), + ?assertEqual(0, maps:size(Stats)) + after + spectrometer_utils:purge_dir(TempParent) + 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_analyzer_tests.erl` around lines 45 - 53, Replace the hardcoded "/tmp" path by using the existing helper spectrometer_utils:make_temp_dir/1 to create TempParent; specifically, remove the manual filename:join([... "/tmp", ...]) construction (variables TempParent and MissingChild) and call spectrometer_utils:make_temp_dir/1 to obtain a portable TempParent, then derive MissingChild from filename:join(TempParent, "nonexistent_child") and pass that to spectrometer_analyzer:scan_target({local_dir, MissingChild}) so the test uses the same temp-dir helper as other tests.
🧹 Nitpick comments (1)
test/spectrometer_analyzer_tests.erl (1)
109-118: ⚡ Quick winStrengthen order-independence assertion to compare full results.
Right now only
{lists, map, 2}is compared, so asymmetric behavior on other keys could slip through.Suggested patch
Result1 = spectrometer_analyzer:merge_stats(Stats1, Stats2), Result2 = spectrometer_analyzer:merge_stats(Stats2, Stats1), - ?assertEqual( - maps:get({lists, map, 2}, Result1), - maps:get({lists, map, 2}, Result2) - ) + ?assertEqual(Result1, Result2) 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_analyzer_tests.erl` around lines 109 - 118, The test merge_stats_order_independent_test_ currently only compares the {lists, map, 2} entry, which can miss other asymmetric keys; update the assertion to compare the full merged maps by replacing the partial compare with a full equality assertion between Result1 and Result2 (i.e., assert that Result1 == Result2) so spectrometer_analyzer:merge_stats is verified order-independent across all keys.
🤖 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 `@README.md`:
- Line 149: The README.md currently triggers MD047 because it doesn't end with a
single trailing newline; open the README.md at the line containing "See:
[todo](TODO.md)" and ensure the file ends with exactly one newline character
(remove any extra blank lines or add a single newline at EOF so there is one
trailing newline).
- Around line 37-45: Reflow the README table rows so no source line exceeds 80
chars to satisfy MD013: break long rows for commands like `audit`, `examine`,
`ecosystem`, `supported`, `filter`, `update`, and `query` into shorter lines
(wrap the Description cell text) while preserving the table pipe separators and
punctuation so the rendered table remains the same; ensure each physical line in
the table is under 80 chars.
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 125-137: The test scan_target_github_url_test_() is asserting an
empty map (#{}) for a real repo which makes success indistinguishable from
failure; change the test to use a known Erlang-containing repository URL (call
spectrometer_analyzer:scan_target with that URL) and assert that the returned
Stats is a map and contains at least one expected key (or is non-empty) instead
of asserting #{}, keeping the existing non-existent-repo case to assert
empty-map behaviour; update the test expectations (e.g. replace the
?assertEqual(#{}, Stats) with ?assert(not maps:is_empty(Stats)) or an assertion
for a specific key) to ensure the happy-path is diagnostic.
In `@TODO.md`:
- Around line 19-20: The long markdown bullet for the supported command exceeds
line-length rules (MD013); edit the TODO.md bullet describing the `supported`
command so the sentence is wrapped into multiple shorter lines (preserving the
backticks and the `--erl` and `--ex` flags), e.g. split after "default" or
before the flags so the text reads across two or more wrapped lines and
satisfies markdownlint.
---
Duplicate comments:
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 45-53: Replace the hardcoded "/tmp" path by using the existing
helper spectrometer_utils:make_temp_dir/1 to create TempParent; specifically,
remove the manual filename:join([... "/tmp", ...]) construction (variables
TempParent and MissingChild) and call spectrometer_utils:make_temp_dir/1 to
obtain a portable TempParent, then derive MissingChild from
filename:join(TempParent, "nonexistent_child") and pass that to
spectrometer_analyzer:scan_target({local_dir, MissingChild}) so the test uses
the same temp-dir helper as other tests.
---
Nitpick comments:
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 109-118: The test merge_stats_order_independent_test_ currently
only compares the {lists, map, 2} entry, which can miss other asymmetric keys;
update the assertion to compare the full merged maps by replacing the partial
compare with a full equality assertion between Result1 and Result2 (i.e., assert
that Result1 == Result2) so spectrometer_analyzer:merge_stats is verified
order-independent across all keys.
🪄 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: 86ca2f4b-6939-4139-b5da-996968b8944a
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignoreLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (5)
- priv/supported_functions.data.license
- src/spectrometer.app.src
- rebar.lock.license
- src/spectrometer_help.erl
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (27)
- src/spectrometer.erl
- include/ecosystem.hrl
- .github/workflows/publish_docs.yml
- .github/workflows/reuse-lint.yaml
- .github/workflows/code_quality_check.yaml
- rebar.config
- test/spectrometer_reporter_tests.erl
- .github/workflows/check-formatting.yaml
- generate_fun_data.sh
- src/spectrometer_atomvm.erl
- src/spectrometer_analyzer.erl
- .github/workflows/build-and-test.yaml
- test/spectrometer_http_tests.erl
- src/spectrometer_http.erl
- src/spectrometer_scanner.erl
- test/spectrometer_scanner_tests.erl
- src/spectrometer_ecosystem.erl
- src/spectrometer_otp.erl
- test/spectrometer_utils_tests.erl
- test/spectrometer_updater_tests.erl
- test/cli_main_tests.erl
- src/spectrometer_utils.erl
- test/spectrometer_atomvm_tests.erl
- src/spectrometer_updater.erl
- src/atomvm_spectrometer.erl
- src/spectrometer_reporter.erl
- test/atomvm_spectrometer_tests.erl
UncleGrumpy
force-pushed
the
WIP
branch
6 times, most recently
from
9e8d4ac to
e96f9ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/publish_docs.yml (1)
11-11: 💤 Low valueFix grammar in trigger comment.
The comment reads "Triggers the workflow merges to main" but should include a preposition for clarity.
📝 Suggested wording
- # Triggers the workflow merges to main + # Triggers the workflow on pushes to main🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish_docs.yml at line 11, Update the comment string "Triggers the workflow merges to main" to include the missing preposition for clarity; replace it with something like "Triggers the workflow on merges to main" (or "Triggers the workflow when merges are made to main") so the intent reads correctly.src/spectrometer_updater.erl (2)
250-257: 💤 Low valueMissing comma between
"\n"and"[\n"relies on adjacent-string-literal concatenation.Erlang silently concatenates adjacent string literals, so
"\n" "[\n"becomes a single"\n[\n"element — functionally correct, but easy to misread as a missing list separator and a likely future copy/paste hazard. Adding the comma makes the intent explicit.🧹 Proposed fix
"%% Since: binary version string like <<\"v0.5.0\">> or {unreleased, <<\"0.7.x\">>}\n", - "\n" + "\n", "[\n" ],🤖 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 250 - 257, The Header list currently relies on adjacent-string concatenation between the elements "\"\\n\"" and "\"[\\n]\"", which is confusing and error-prone; update the Header list (variable Header) to explicitly separate those entries by adding a comma between the "\n" element and the "[\n" element so they are distinct list items and the intent is clear.
803-808: ⚡ Quick winReplace deprecated
string:len/1andstring:join/2with their modern equivalents.Both functions are obsolete and marked for future deprecation in the Erlang/OTP
stringmodule. The rest of the module already uses the recommended modern API, making these replacements straightforward.
- Line 803:
string:len(BaseName)→string:length(BaseName)- Line 848:
string:join(Lines, " ")→lists:join(" ", Lines)Proposed changes
- BaseLen = string:len(BaseName), + BaseLen = string:length(BaseName),- Joined = string:join(Lines, " "), + Joined = lists:join(" ", Lines),🤖 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 803 - 808, The code uses deprecated string:len/1 and string:join/2; replace string:len(BaseName) with string:length(BaseName) where BaseLen is computed (e.g., the BaseLen assignment used before ActualPlatforms) and replace string:join(Lines, " ") with lists:join(" ", Lines) where Lines are joined later in the module; update occurrences around the BaseLen/ActualPlatforms logic and the join usage to use string:length/1 and lists:join/2 respectively so the module consistently uses the modern APIs.
🤖 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 `@src/spectrometer_updater.erl`:
- Around line 696-703: The regex in parse_file_entries/6 currently uses the
function-name capture `[A-Za-z_][A-Za-z0-9_]+` which forces at least two
characters and thus drops single-character NIF names; update the pattern passed
to parse_file_entries (the string
"\\s*?\"?([a-z_][a-z0-9_]*):([A-Za-z_][A-Za-z0-9_]+)/(\\d+)\"?") to use
`[A-Za-z_][A-Za-z0-9_]*` for the function-name capture so it matches
single-character names (making it consistent with parse_platform_nifs/4) and
ensure any callers or tests relying on parse_file_entries continue to pass
unchanged inputs.
---
Nitpick comments:
In @.github/workflows/publish_docs.yml:
- Line 11: Update the comment string "Triggers the workflow merges to main" to
include the missing preposition for clarity; replace it with something like
"Triggers the workflow on merges to main" (or "Triggers the workflow when merges
are made to main") so the intent reads correctly.
In `@src/spectrometer_updater.erl`:
- Around line 250-257: The Header list currently relies on adjacent-string
concatenation between the elements "\"\\n\"" and "\"[\\n]\"", which is confusing
and error-prone; update the Header list (variable Header) to explicitly separate
those entries by adding a comma between the "\n" element and the "[\n" element
so they are distinct list items and the intent is clear.
- Around line 803-808: The code uses deprecated string:len/1 and string:join/2;
replace string:len(BaseName) with string:length(BaseName) where BaseLen is
computed (e.g., the BaseLen assignment used before ActualPlatforms) and replace
string:join(Lines, " ") with lists:join(" ", Lines) where Lines are joined later
in the module; update occurrences around the BaseLen/ActualPlatforms logic and
the join usage to use string:length/1 and lists:join/2 respectively so the
module consistently uses the modern APIs.
🪄 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: cbcce03a-6b74-434c-87fd-e4893e1cff2a
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignore.markdownlint.jsonLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (4)
- .markdownlint.json
- rebar.lock.license
- priv/supported_functions.data.license
- README.md
🚧 Files skipped from review as they are similar to previous changes (29)
- include/ecosystem.hrl
- src/spectrometer.app.src
- src/spectrometer.erl
- rebar.config
- test/spectrometer_reporter_tests.erl
- src/spectrometer_otp.erl
- src/spectrometer_help.erl
- test/spectrometer_http_tests.erl
- .github/workflows/code_quality_check.yaml
- .github/workflows/reuse-lint.yaml
- .gitignore
- src/spectrometer_atomvm.erl
- test/spectrometer_analyzer_tests.erl
- generate_fun_data.sh
- src/spectrometer_http.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_reporter.erl
- test/spectrometer_scanner_tests.erl
- .github/workflows/check-formatting.yaml
- .github/workflows/build-and-test.yaml
- src/spectrometer_scanner.erl
- src/spectrometer_analyzer.erl
- src/spectrometer_utils.erl
- test/spectrometer_utils_tests.erl
- test/cli_main_tests.erl
- test/atomvm_spectrometer_tests.erl
- src/atomvm_spectrometer.erl
- src/spectrometer_ecosystem.erl
- test/spectrometer_atomvm_tests.erl
Owner
Author
|
@coderabbitai please do a final review. Be thorough and rigorous. |
|
Please do a comprehensive, thorough, and rigorous final review. Pay close attention to:
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (5)
generate_fun_data.sh (1)
72-86:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCheckout failure for
release-0.7silently scans the wrong branch.Line 74 only tests whether the remote ref exists; line 75's
git checkoutruns inside the block but is not part of the condition. If the checkout fails (e.g., local modifications prevent switching branches), execution continues to line 76 and$SPECTROMETER update --branch "release-0.7"scans whatever commit is currently checked out (typicallymainfrom line 58), mislabeling that data asrelease-0.7in the output database.The
mainbranch block (line 58) and the tags loop (line 104) correctly handle checkout failures, but therelease-0.7block does not.🔧 Proposed fix
echo '=== Scanning branch: release-0.7 (stored as {unreleased, <<"0.7.x">>}) ===' git fetch origin --quiet 2>/dev/null -if git show-ref --verify --quiet refs/remotes/origin/release-0.7; then - git checkout "release-0.7" --quiet 2>/dev/null +if git show-ref --verify --quiet refs/remotes/origin/release-0.7 && \ + git checkout "release-0.7" --quiet 2>/dev/null; then "$SPECTROMETER" update \ --atomvm-dir "$ATOMVM_DIR" \ --branch "release-0.7" \ --cache "$TMP_CACHE_DIR" \ --output "$OUTPUT_FILE" \ --force \ --no-tests echo "" else echo "Warning: Could not checkout release-0.7" fi🤖 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 `@generate_fun_data.sh` around lines 72 - 86, The release-0.7 branch block in generate_fun_data.sh currently tests only for the remote ref and then runs git checkout without verifying it succeeded, so a failed checkout can let SPECTROMETER scan the wrong commit; update the block to mirror the main/tags handling by checking the git checkout exit status (e.g., replace the current unconditional git checkout line with an if git checkout "release-0.7" --quiet; then run the "$SPECTROMETER" update --branch "release-0.7" ... else echo a warning and skip running SPECTROMETER) so the update is only executed when checkout actually succeeds.src/spectrometer_atomvm.erl (1)
45-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo still present:
spectrometer_upater→spectrometer_updater.The moduledoc still references
spectrometer_upater:update_datafile/2, missing thedin "updater". A prior review flagged the same typo and it was reportedly fixed, but the typo remains in the current code.📝 Proposed fix
-`spectrometer_upater:update_datafile/2`. +`spectrometer_updater:update_datafile/2`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_atomvm.erl` at line 45, The moduledoc in spectrometer_atomvm.erl still has a typo referencing spectrometer_upater:update_datafile/2; update that reference to spectrometer_updater:update_datafile/2 so the module and function name are spelled correctly (fix the missing "d" in the moduledoc string that mentions spectrometer_upater:update_datafile/2).test/spectrometer_analyzer_tests.erl (1)
125-140:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGitHub success test is still non-diagnostic — empty stats can mean both success and failure.
scan_target_github_url_nonexistent_test_(Line 142-155) assertsmaps:size(Stats) =:= 0for a failed clone, and this happy-path test ongithubtraining/hellogitworldalso assertsStats =:= #{}. With both success and failure paths producing the same empty map, this test cannot detect regressions where a working clone silently turns into a failure. Pick a small repo known to contain.erlfiles and assert at least one expected MFA key, or assertmaps:size(Stats) > 0, so the test actually exercises the success path.🤖 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_analyzer_tests.erl` around lines 125 - 140, The test scan_target_github_url_test_() is non-diagnostic because it expects an empty map like the failure test scan_target_github_url_nonexistent_test_; change the success-case assertion to verify a real positive result from spectrometer_analyzer:scan_target by using a small GitHub repo that contains .erl files (or asserting a known MFA key that should be found) and replace ?assertEqual(#{}, Stats) with either ?assert(maps:size(Stats) > 0) or an assertion that a specific expected key exists in Stats so the test distinguishes success from clone/failure.src/spectrometer_ecosystem.erl (2)
210-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
save_state/3failure still leavesNewSS2 = NewSS, producing a retry storm on every subsequent result.When
NewSS >= ?SAVE_INTERVALandsave_state/3returns{error, _},NewSS2is set toNewSS(which is still ≥SAVE_INTERVAL). The very next{result, _, _}message will re-enter the save branch and fail again, with no log emitted. A transientenospc/eacceswill floodsave_state/3calls at the per-item cadence with no user-visible warning. Reset the counter (to e.g.0) on failure and emit a single warning so the next save attempt happens after another?SAVE_INTERVALitems.🛡️ Proposed fix
case save_state(NewScanned, NewStats, NewTP) of ok -> 0; - {error, _} -> - NewSS + {error, Reason} -> + io:format( + standard_error, + "\nWarning: save_state failed: ~p\n", + [Reason] + ), + 0 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_ecosystem.erl` around lines 210 - 221, The save branch leaves NewSS2 as NewSS when save_state(NewScanned, NewStats, NewTP) returns {error,_}, causing immediate repeated retries; change the case in the NewSS >= ?SAVE_INTERVAL branch so that on {error, Reason} you set NewSS2 to 0 (reset the save counter) and call the process logger/warning function once with the error (include Reason) before continuing; update the clause handling save_state/3 in the case expression that computes NewSS2 to reset the counter and emit a warning on failure.
170-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWorker
spawn_monitorreferences are discarded and'DOWN'messages are never handled.Line 172 uses
spawn_monitorfor workers but discards the monitor ref.coordinator_loop/1only matches{get_work, _},{result, _, _}, and{worker_done, _}— there is no clause for'DOWN'. If a worker exits normally,'DOWN'messages accumulate in the coordinator's mailbox (mild leak). If a worker crashes outside the per-itemtry/catch(e.g., port failure during clone/scan, or memory-related issues), the coordinator never receives{worker_done, _}, never decrementsactive_workers, and hangs indefinitely.Either downgrade workers to plain
spawn/1(simpler, since they already report completion via messages and have try/catch around per-item work), or capture the monitor refs and handle'DOWN'incoordinator_loop/1to decrementactive_workerson abnormal exit.🔧 Simpler fix: use `spawn/1`
lists:foreach( fun(_) -> - _ = spawn_monitor(fun() -> worker_loop(CoordPid) end) + spawn(fun() -> worker_loop(CoordPid) end) end, lists:seq(1, NumWorkers) ),🤖 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.erl` around lines 170 - 180, The spawn_monitor call that discards monitor refs causes unhandled 'DOWN' messages and can leave coordinator_loop/1 stuck because active_workers isn't decremented when a worker crashes; fix by switching to plain spawn for workers: replace spawn_monitor(fun() -> worker_loop(CoordPid) end) with spawn(fun() -> worker_loop(CoordPid) end) in the worker creation loop (or alternatively capture and store the monitor refs and add a 'DOWN' clause in coordinator_loop/1 to decrement active_workers and treat it like a worker failure), and ensure coordinator_loop/1 still handles worker_done messages and decrements active_workers accordingly.
🧹 Nitpick comments (4)
.github/workflows/code_quality_check.yaml (1)
20-20: ⚡ Quick winRename job to match workflow purpose.
The job is named
build-and-testbut this workflow performs code quality checks (xref and dialyzer), not build/test operations. Consider renaming tocode-qualityorstatic-analysisfor clarity.📝 Suggested fix
jobs: - build-and-test: + code-quality: runs-on: "ubuntu-24.04"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/code_quality_check.yaml at line 20, The job id "build-and-test" is misleading for this workflow; rename the job to a clearer id such as "code-quality" or "static-analysis" by replacing the "build-and-test" job identifier and update any references to that job id within the workflow (e.g., needs:, if any) to the new name so all dependent steps keep working; ensure the new id is used consistently throughout the YAML file (replace all occurrences of "build-and-test").src/spectrometer_atomvm.erl (1)
234-234: 💤 Low valueRemove commented-out dead code.
Line 234 carries a stray comment placeholder
% maybe_update_user_db(UserPath, BundledPath) ->, and Line 507-519 insupported_db_lookup/1retain a large block of commented-out alternative lookup logic with a "test compatibility" note. Either flesh these out into supported behavior or delete them — dead comments accumulate cruft and confuse future readers about the intended design.Also applies to: 507-519
🤖 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` at line 234, Remove dead commented-out code: delete the stray comment "% maybe_update_user_db(UserPath, BundledPath) ->" and remove the large commented alternative logic block inside supported_db_lookup/1 (the "test compatibility" snippet). If that logic is intended to be kept, instead implement it as real clauses or helper functions (e.g., rename to maybe_update_user_db/2 or extract into a helper called supported_db_lookup_alt/1) and remove the commented versions; otherwise simply delete the commented lines to avoid cruft and keep only the active supported_db_lookup/1 implementation.test/atomvm_spectrometer_tests.erl (1)
689-732: 💤 Low valueTest asserts 4 targets parsed but does not verify the 4th entry's classification.
Line 700 asserts
length(Targets) =:= 4, but lines 701-709 only verify 3 of them. The"/path/to/local/dir"line is non-existent locally, soparse_target_lines/1(insrc/spectrometer_analyzer.erl) will fall throughfilelib:is_dir/1 → false, thenis_valid_url/1→true(contains "/"), and end up classifying it as{github_url, "/path/to/local/dir"}— likely not the intended classification for this test data. Consider asserting on the 4th element explicitly or using a clearly-shaped GitHub URL for that line to make intent unambiguous.🤖 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 689 - 732, The test parse_target_lines_test_ asserts 4 targets but only checks three; the ambiguous "/path/to/local/dir" is being classified by spectrometer_analyzer:parse_target_lines/1 (which calls filelib:is_dir/1 and is_valid_url/1) as a github_url because it contains "/" — fix by making the intent explicit: either replace "/path/to/local/dir" with a clearly shaped GitHub URL (e.g., "https://github.com/user/local-repo") or add an explicit assertion checking for the actual classification returned (e.g., assert lists:member({github_url, "/path/to/local/dir"}, Targets) or the expected {local_dir, Path} if you change setup to create that directory) so parse_target_lines_test_ fully validates all four entries.src/spectrometer_http.erl (1)
450-459: 💤 Low valueUnreachable clauses in
is_windows_absolute_path/1.The first two clauses at Line 450-453 match any list whose first element is
A..Z/a..zand second element is$:, with the rest unconstrained. That subsumes the more specific[_, $:, $\\ | _]and[_, $:, $/ | _]clauses at Line 454-457, which can never be reached. Either restrict the first clauses to require a separator on the third position, or remove the latter two clauses.♻️ Proposed simplification
is_windows_absolute_path([Drive, $: | _]) when Drive >= $A, Drive =< $Z -> true; is_windows_absolute_path([Drive, $: | _]) when Drive >= $a, Drive =< $z -> true; -is_windows_absolute_path([_, $:, $\\ | _]) -> - true; -is_windows_absolute_path([_, $:, $/ | _]) -> - true; is_windows_absolute_path(_) -> false.🤖 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_http.erl` around lines 450 - 459, The clauses in is_windows_absolute_path/1 are shadowing the more specific third-position-separator clauses, making [_, $:, $\\ | _] and [_, $:, $/ | _] unreachable; update the first two clauses (the ones matching [Drive, $: | _]) to require and check a third-element separator (e.g. pattern [Drive, $:, Sep | _] with guards Sep == $\\ orelse Sep == $/) so they only match drive-letter + separator, or alternatively remove the redundant third-position clauses and keep the two-letter-only forms—ensure the final implementation of is_windows_absolute_path/1 consistently matches drive-letter, colon and a path separator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/code_quality_check.yaml:
- Around line 38-39: Remove the unnecessary submodules setting from the checkout
step: delete the "submodules: 'recursive'" configuration (the checkout action
block that currently includes submodules) so the workflow uses the default
checkout behavior; this mirrors other workflows and avoids extra overhead when
the repository has no git submodules.
- Line 49: The cache key template contains an extra closing brace in the string
used for the workflow "key:" (the fragment containing `${{env.OTP_VERSION}}}`);
remove the stray `}` so the expression reads `${{env.OTP_VERSION}}` and ensure
the entire key string remains
`ci-${{runner.os}}-${{env.cache-name}}-otp_${{env.OTP_VERSION}}-rebar_${{env.REBAR3_VERSION}}-${{hashFiles('rebar.config')}}`
to fix the malformed GitHub Actions expression and restore proper cache
matching.
In `@src/spectrometer_analyzer.erl`:
- Around line 254-278: scan_multi/1 currently returns {error,
{could_not_read_multi_target_file, Reason}} when file:read_file/1 fails which
later causes maps:size/1 to crash in analyze/2; change scan_multi/1 to raise the
failure instead of returning an error tuple (e.g., call
erlang:error({could_not_read_multi_target_file, Reason}) or use error/1) so the
existing try/catch in analyze/2 will surface the original reason, referencing
scan_multi/1, analyze/2, file:read_file/1 and maps:size/1 to locate the change.
In `@src/spectrometer_http.erl`:
- Around line 77-83: The recursive call in fetch_github_cursor is overwriting
the original lower bound (MinStars) by passing MinStarsInBatch - 1 as the first
argument, causing the floor to drift downward each recursion; change the
recursive invocation so the original floor (the incoming MinStars parameter from
fetch_github_repos / fetch_github_cursor) is preserved as the first argument and
only the upper bound is reduced to MinStarsInBatch - 1 (i.e., call
fetch_github_cursor(OriginalMinStars, MinStarsInBatch - 1, NewAcc, Max) or
equivalent), and adjust any tests (e.g., fetch_github_cursor_advances_test) to
include a meaningful MinStars floor to validate this behavior.
In `@src/spectrometer_updater.erl`:
- Around line 103-115: The code assigns the raw result of
spectrometer_utils:clone_temp_repo(Branch0, Tag0) to RepoDir and passes it to
scan_atomvm_repo/3, so clone failures ({error, Reason}) become later crashes;
change the branch that calls clone_temp_repo to pattern-match its return and
handle errors early: call spectrometer_utils:clone_temp_repo(Branch0, Tag0) and
case its result of {ok, Dir} -> use Dir for RepoDir and continue; {error,
Reason} -> return {error, Reason} from update_datafile/2 (or propagate the
error) instead of assigning the tuple to RepoDir and calling scan_atomvm_repo/3.
- Around line 81-101: Change the consult handling so we only fall back to
bundled_data_path() or empty DB when file:consult(OutputFile) returns {error,
enoent}; for any other error from file:consult(OutputFile) return/propagate
{error, Reason} instead of seeding a new DB. Locate the ExistingDB binding
around file:consult(OutputFile) and replace the wildcard fallback branch with
explicit matches: {ok, [Data]} -> build_db_from_list(Data); {error, enoent} ->
run the existing bundled-data/empty-DB logic (using
spectrometer_utils:bundled_data_path() and build_db_from_list); {error, Reason}
-> return {error, Reason} so malformed/unreadable DB errors are surfaced.
In `@src/spectrometer_utils.erl`:
- Around line 116-152: The bundled-data lookup in try_script_relative/0
incorrectly includes user_db_file() (which can create cache dir and mutate
cache_dir); remove user_db_file() from the Candidates list so
try_script_relative/0 only searches the bundled priv/... paths (the
filename:join(...) entries and "priv/supported_functions.data"), preserving a
stable bundled_data_path/0 and avoiding side effects from user-cache creation.
In `@test/cli_main_tests.erl`:
- Around line 378-414: The setup currently captures Prev (the original
application:get_env(spectrometer, cache_dir)) in the cleanup fun after the test
mutates it; instead capture and return Prev from the setup fun so the cleanup
receives the original value. Modify main_filter_no_user_state_test_() so the
setup fun returns {CacheDir, Prev} (where Prev =
application:get_env(spectrometer, cache_dir)) and update the cleanup fun to
accept {CacheDir, Prev} and restore Prev (unset if undefined, or set_env to Val)
before removing CacheDir; keep the test body using
application:set_env(spectrometer, cache_dir, CacheDir) and
spectrometer_atomvm:reload_db() as before.
In `@test/spectrometer_updater_tests.erl`:
- Around line 353-485: The grouped tests in scan_repo_test_() share a single
RepoDir so later cases run against mutated state; ensure each case runs against
a fresh repo by either creating a new temp dir per inner test (use
spectrometer_utils:make_temp_dir/1 at the start of each fun) or purge the shared
RepoDir before each case (call spectrometer_utils:purge_dir(RepoDir) then
re-create the minimal structure) so that functions like
spectrometer_updater:scan_atomvm_repo/3 are exercised with the intended empty or
minimal repo state.
---
Duplicate comments:
In `@generate_fun_data.sh`:
- Around line 72-86: The release-0.7 branch block in generate_fun_data.sh
currently tests only for the remote ref and then runs git checkout without
verifying it succeeded, so a failed checkout can let SPECTROMETER scan the wrong
commit; update the block to mirror the main/tags handling by checking the git
checkout exit status (e.g., replace the current unconditional git checkout line
with an if git checkout "release-0.7" --quiet; then run the "$SPECTROMETER"
update --branch "release-0.7" ... else echo a warning and skip running
SPECTROMETER) so the update is only executed when checkout actually succeeds.
In `@src/spectrometer_atomvm.erl`:
- Line 45: The moduledoc in spectrometer_atomvm.erl still has a typo referencing
spectrometer_upater:update_datafile/2; update that reference to
spectrometer_updater:update_datafile/2 so the module and function name are
spelled correctly (fix the missing "d" in the moduledoc string that mentions
spectrometer_upater:update_datafile/2).
In `@src/spectrometer_ecosystem.erl`:
- Around line 210-221: The save branch leaves NewSS2 as NewSS when
save_state(NewScanned, NewStats, NewTP) returns {error,_}, causing immediate
repeated retries; change the case in the NewSS >= ?SAVE_INTERVAL branch so that
on {error, Reason} you set NewSS2 to 0 (reset the save counter) and call the
process logger/warning function once with the error (include Reason) before
continuing; update the clause handling save_state/3 in the case expression that
computes NewSS2 to reset the counter and emit a warning on failure.
- Around line 170-180: The spawn_monitor call that discards monitor refs causes
unhandled 'DOWN' messages and can leave coordinator_loop/1 stuck because
active_workers isn't decremented when a worker crashes; fix by switching to
plain spawn for workers: replace spawn_monitor(fun() -> worker_loop(CoordPid)
end) with spawn(fun() -> worker_loop(CoordPid) end) in the worker creation loop
(or alternatively capture and store the monitor refs and add a 'DOWN' clause in
coordinator_loop/1 to decrement active_workers and treat it like a worker
failure), and ensure coordinator_loop/1 still handles worker_done messages and
decrements active_workers accordingly.
In `@test/spectrometer_analyzer_tests.erl`:
- Around line 125-140: The test scan_target_github_url_test_() is non-diagnostic
because it expects an empty map like the failure test
scan_target_github_url_nonexistent_test_; change the success-case assertion to
verify a real positive result from spectrometer_analyzer:scan_target by using a
small GitHub repo that contains .erl files (or asserting a known MFA key that
should be found) and replace ?assertEqual(#{}, Stats) with either
?assert(maps:size(Stats) > 0) or an assertion that a specific expected key
exists in Stats so the test distinguishes success from clone/failure.
---
Nitpick comments:
In @.github/workflows/code_quality_check.yaml:
- Line 20: The job id "build-and-test" is misleading for this workflow; rename
the job to a clearer id such as "code-quality" or "static-analysis" by replacing
the "build-and-test" job identifier and update any references to that job id
within the workflow (e.g., needs:, if any) to the new name so all dependent
steps keep working; ensure the new id is used consistently throughout the YAML
file (replace all occurrences of "build-and-test").
In `@src/spectrometer_atomvm.erl`:
- Line 234: Remove dead commented-out code: delete the stray comment "%
maybe_update_user_db(UserPath, BundledPath) ->" and remove the large commented
alternative logic block inside supported_db_lookup/1 (the "test compatibility"
snippet). If that logic is intended to be kept, instead implement it as real
clauses or helper functions (e.g., rename to maybe_update_user_db/2 or extract
into a helper called supported_db_lookup_alt/1) and remove the commented
versions; otherwise simply delete the commented lines to avoid cruft and keep
only the active supported_db_lookup/1 implementation.
In `@src/spectrometer_http.erl`:
- Around line 450-459: The clauses in is_windows_absolute_path/1 are shadowing
the more specific third-position-separator clauses, making [_, $:, $\\ | _] and
[_, $:, $/ | _] unreachable; update the first two clauses (the ones matching
[Drive, $: | _]) to require and check a third-element separator (e.g. pattern
[Drive, $:, Sep | _] with guards Sep == $\\ orelse Sep == $/) so they only match
drive-letter + separator, or alternatively remove the redundant third-position
clauses and keep the two-letter-only forms—ensure the final implementation of
is_windows_absolute_path/1 consistently matches drive-letter, colon and a path
separator.
In `@test/atomvm_spectrometer_tests.erl`:
- Around line 689-732: The test parse_target_lines_test_ asserts 4 targets but
only checks three; the ambiguous "/path/to/local/dir" is being classified by
spectrometer_analyzer:parse_target_lines/1 (which calls filelib:is_dir/1 and
is_valid_url/1) as a github_url because it contains "/" — fix by making the
intent explicit: either replace "/path/to/local/dir" with a clearly shaped
GitHub URL (e.g., "https://github.com/user/local-repo") or add an explicit
assertion checking for the actual classification returned (e.g., assert
lists:member({github_url, "/path/to/local/dir"}, Targets) or the expected
{local_dir, Path} if you change setup to create that directory) so
parse_target_lines_test_ fully validates all four entries.
🪄 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: ad6a1bbf-92aa-4974-9502-26c5f6ba20bd
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignore.markdownlint.json.markdownlint.json.licenseLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@README.md`:
- Around line 37-50: The markdown table in README.md has inconsistent column
alignment (MD060); fix the table so every row has the same number of
pipe-separated columns and consistent padding aligning the "Command" and
"Description" columns for all entries (rows containing `audit`, `ecosystem`,
`examine`, `supported`, `filter`, `update`, `query`), ensuring header separator
row (---) matches column count and each data row starts and ends with a pipe;
update the rows with wrapped descriptions so they remain within the same two
columns rather than introducing extra pipes or missing ones.
In `@src/spectrometer_scanner.erl`:
- Around line 179-180: parse_calls/1 is currently treating implicit_fun ->
extract_implicit_fun(Node, A) without excluding self-references, causing fun
<same_module>:f/A to be reported as external; update the logic so
extract_implicit_fun/2 (or the caller parse_calls/1) checks the fun's module
against FilterMod and excludes it when in filtered mode (i.e., if the implicit
fun's module equals FilterMod, do not emit it as an external call). Apply the
same fix to the other occurrences in the range that call extract_implicit_fun/2
(lines ~283-314) so all implicit_fun paths respect FilterMod.
🪄 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: 27568518-995d-4027-9a73-a57ede8821e0
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignore.markdownlint.json.markdownlint.json.licenseLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (3)
- .markdownlint.json
- .markdownlint.json.license
- rebar.lock.license
🚧 Files skipped from review as they are similar to previous changes (26)
- priv/supported_functions.data.license
- include/ecosystem.hrl
- src/spectrometer.app.src
- src/spectrometer.erl
- .github/workflows/reuse-lint.yaml
- .github/workflows/code_quality_check.yaml
- src/spectrometer_help.erl
- .github/workflows/publish_docs.yml
- rebar.config
- .gitignore
- .github/workflows/check-formatting.yaml
- src/spectrometer_otp.erl
- test/spectrometer_utils_tests.erl
- .github/workflows/build-and-test.yaml
- src/spectrometer_reporter.erl
- test/spectrometer_atomvm_tests.erl
- src/atomvm_spectrometer.erl
- src/spectrometer_ecosystem.erl
- generate_fun_data.sh
- test/spectrometer_scanner_tests.erl
- test/spectrometer_http_tests.erl
- src/spectrometer_analyzer.erl
- src/spectrometer_http.erl
- test/spectrometer_updater_tests.erl
- test/cli_main_tests.erl
- src/spectrometer_updater.erl
UncleGrumpy
force-pushed
the
WIP
branch
2 times, most recently
from
d691f6b to
389d45a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/spectrometer_ecosystem.erl (1)
170-175:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHandle worker death paths in coordinator accounting.
Line 172 monitors workers but drops monitor refs, while completion relies on
{worker_done, _}. If any worker exits unexpectedly,active_workersnever reaches zero and the coordinator can stall indefinitely.Also applies to: 236-256
🤖 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.erl` around lines 170 - 175, The coordinator currently calls spawn_monitor(fun() -> worker_loop(CoordPid) end) and discards the monitor refs, so unexpected worker exits (DOWN) aren't accounted for and active_workers only decrements on {worker_done, _}; modify the coordinator to capture and store each MonitorRef returned by spawn_monitor (e.g., into a map or list keyed by worker id) and update the coordinator loop (the clause handling messages where worker completion is processed) to also handle {'DOWN', MonitorRef, _, _, Reason} by treating it the same as {worker_done, _} (decrement active_workers, clean up the MonitorRef from the registry, and perform any necessary logging), and apply the same change to the other location mentioned (lines ~236-256) where spawn_monitor is used.
🧹 Nitpick comments (1)
.github/workflows/build-and-test.yaml (1)
64-64: ⚡ Quick winInclude
rebar.lockin the cache key.Line 64 only keys on
rebar.config; lockfile-only dependency updates can reuse stale_buildartifacts and make CI results inconsistent.Suggested patch
- key: ci-${{runner.os}}-${{env.cache-name}}-otp_${{matrix.otp}}-rebar_${{matrix.rebar3}}-${{hashFiles('rebar.config')}} + key: ci-${{runner.os}}-${{env.cache-name}}-otp_${{matrix.otp}}-rebar_${{matrix.rebar3}}-${{hashFiles('rebar.config', 'rebar.lock')}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-and-test.yaml at line 64, The cache key for CI currently only hashes rebar.config which allows lockfile-only dependency changes to reuse stale _build artifacts; update the workflow step that defines the cache key (the key string containing ci-${{runner.os}}-${{env.cache-name}}-otp_${{matrix.otp}}-rebar_${{matrix.rebar3}}-${{hashFiles('rebar.config')}}) to include rebar.lock in the hashFiles call (e.g., hashFiles('rebar.config','rebar.lock')) so cache invalidation occurs when either the config or lockfile changes.
🤖 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 `@src/spectrometer_ecosystem.erl`:
- Around line 155-175: Before spawning coordinator/worker processes in
run_coordinator/5, validate NumWorkers (maps:get(workers, Opts)) and reject
values < 1; if workers <= 0, return an error (or throw) instead of proceeding to
spawn the coordinator or calling coordinator_loop/1, so coordinator_loop and
worker_loop are not started when no workers will be spawned. Add the same
validation earlier in option parsing if present, and ensure any callers handle
the error return rather than assuming coordinator and worker processes were
started.
- Around line 403-413: The state-file deserialization uses binary_to_term(Bin)
without safety or exception handling; update the {ok, Bin} branch to wrap the
decode in a try/catch and call binary_to_term(Bin, [safe]) so malformed or
unsafe binaries do not crash the process; inside the try handle the
{spectrometer_v1, Scanned, Stats, TotalProcessed} clause exactly as now and
treat any other decoded term or any catch clause as the "invalid state file,
starting fresh" path (returning the same {sets:new([{version, 2}]), #{}, 0}
fallback) so the code safely falls back on failure.
---
Duplicate comments:
In `@src/spectrometer_ecosystem.erl`:
- Around line 170-175: The coordinator currently calls spawn_monitor(fun() ->
worker_loop(CoordPid) end) and discards the monitor refs, so unexpected worker
exits (DOWN) aren't accounted for and active_workers only decrements on
{worker_done, _}; modify the coordinator to capture and store each MonitorRef
returned by spawn_monitor (e.g., into a map or list keyed by worker id) and
update the coordinator loop (the clause handling messages where worker
completion is processed) to also handle {'DOWN', MonitorRef, _, _, Reason} by
treating it the same as {worker_done, _} (decrement active_workers, clean up the
MonitorRef from the registry, and perform any necessary logging), and apply the
same change to the other location mentioned (lines ~236-256) where spawn_monitor
is used.
---
Nitpick comments:
In @.github/workflows/build-and-test.yaml:
- Line 64: The cache key for CI currently only hashes rebar.config which allows
lockfile-only dependency changes to reuse stale _build artifacts; update the
workflow step that defines the cache key (the key string containing
ci-${{runner.os}}-${{env.cache-name}}-otp_${{matrix.otp}}-rebar_${{matrix.rebar3}}-${{hashFiles('rebar.config')}})
to include rebar.lock in the hashFiles call (e.g.,
hashFiles('rebar.config','rebar.lock')) so cache invalidation occurs when either
the config or lockfile changes.
🪄 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: dcf5685c-7afb-4095-9a5f-61c0ef1c858f
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignore.markdownlint.json.markdownlint.json.licenseLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (7)
- .markdownlint.json
- .markdownlint.json.license
- rebar.lock.license
- src/spectrometer.app.src
- priv/supported_functions.data.license
- README.md
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (27)
- test/spectrometer_reporter_tests.erl
- include/ecosystem.hrl
- .github/workflows/reuse-lint.yaml
- src/spectrometer.erl
- .github/workflows/check-formatting.yaml
- test/spectrometer_utils_tests.erl
- .github/workflows/code_quality_check.yaml
- src/spectrometer_help.erl
- test/spectrometer_scanner_tests.erl
- .github/workflows/publish_docs.yml
- src/spectrometer_otp.erl
- test/spectrometer_atomvm_tests.erl
- test/spectrometer_analyzer_tests.erl
- src/spectrometer_atomvm.erl
- src/spectrometer_reporter.erl
- rebar.config
- src/spectrometer_updater.erl
- src/spectrometer_http.erl
- test/spectrometer_http_tests.erl
- test/spectrometer_updater_tests.erl
- src/spectrometer_utils.erl
- src/spectrometer_scanner.erl
- src/atomvm_spectrometer.erl
- generate_fun_data.sh
- src/spectrometer_analyzer.erl
- test/atomvm_spectrometer_tests.erl
- test/cli_main_tests.erl
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/spectrometer_ecosystem.erl (1)
170-197:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
'DOWN'handling is wired to the wrong process for worker monitors.Line [194] creates worker monitors in
do_run_coordinator/6, so'DOWN'is delivered to the caller process, notcoordinator_loop/1. The coordinator’s Line [256] branch can miss worker crashes and stall waiting foractive_workersto reach zero.Suggested fix
do_run_coordinator(Work, Scanned, Stats, TotalProcessed, _Opts, NumWorkers) -> TotalWork = length(Work) + TotalProcessed, Self = self(), {CoordPid, CoordRef} = spawn_monitor(fun() -> + WorkerMonitors = spawn_workers(self(), NumWorkers), coordinator_loop(#{ work => Work, scanned => Scanned, stats => Stats, total_processed => TotalProcessed, total_work => TotalWork, since_save => 0, active_workers => NumWorkers, - worker_monitors => #{}, + worker_monitors => WorkerMonitors, parent => Self }) end), - WorkerMonitors = spawn_workers(CoordPid, NumWorkers), - CoordPid ! {set_monitors, WorkerMonitors}, receive {coordinator_done, FinalStats} -> {ok, FinalStats}; {error, Reason} -> {error, Reason}; {'DOWN', CoordRef, process, CoordPid, Reason} -> {error, Reason} end.- {set_monitors, Monitors} -> - coordinator_loop(State#{worker_monitors => Monitors});In Erlang/OTP 27, does spawn_monitor/1 send 'DOWN' only to the process that called spawn_monitor, and can monitor ownership be transferred to another process?Also applies to: 256-266
🤖 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.erl` around lines 170 - 197, The coordinator is not the owner of worker monitors because spawn_workers/2 is called in do_run_coordinator/6, so 'DOWN' messages go to the parent and not coordinator_loop/1; fix by ensuring the coordinator process creates or owns the monitors: either move the worker creation into the fun passed to spawn_monitor so spawn_workers/2 is invoked inside coordinator_loop/1 (i.e., call spawn_monitor/spawn_workers from within the anonymous fun that starts coordinator_loop), or keep worker pids created in do_run_coordinator/6 but send them to the coordinator and have coordinator_loop/1 call erlang:monitor/2 for each pid; update spawn_workers/2 and the {set_monitors, ...} handshake accordingly so coordinator_loop/1 receives and tracks monitor refs it owns.
🤖 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 `@src/spectrometer_ecosystem.erl`:
- Around line 429-431: The final state path (EcoState) is being built from
spectrometer_utils:user_cache_path() instead of using the configured CacheDir,
causing temp and final paths to diverge; update the code that constructs
EcoState to join CacheDir with ?ECOSYSTEM_STATE (instead of calling
spectrometer_utils:user_cache_path()), ensuring all uses of EcoState (and the
subsequent rename logic) reference the same CacheDir-derived path so temp and
final locations match.
---
Duplicate comments:
In `@src/spectrometer_ecosystem.erl`:
- Around line 170-197: The coordinator is not the owner of worker monitors
because spawn_workers/2 is called in do_run_coordinator/6, so 'DOWN' messages go
to the parent and not coordinator_loop/1; fix by ensuring the coordinator
process creates or owns the monitors: either move the worker creation into the
fun passed to spawn_monitor so spawn_workers/2 is invoked inside
coordinator_loop/1 (i.e., call spawn_monitor/spawn_workers from within the
anonymous fun that starts coordinator_loop), or keep worker pids created in
do_run_coordinator/6 but send them to the coordinator and have
coordinator_loop/1 call erlang:monitor/2 for each pid; update spawn_workers/2
and the {set_monitors, ...} handshake accordingly so coordinator_loop/1 receives
and tracks monitor refs it owns.
🪄 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: 0e53dcfd-dc38-44ec-b52f-7c28cbf0fe30
⛔ Files ignored due to path filters (1)
rebar.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/build-and-test.yaml.github/workflows/check-formatting.yaml.github/workflows/code_quality_check.yaml.github/workflows/publish_docs.yml.github/workflows/reuse-lint.yaml.gitignore.markdownlint.json.markdownlint.json.licenseLICENSES/Apache-2.0.txtLICENSES/CC0-1.0.txtLICENSES/LGPL-2.1-or-later.txtREADME.mdTODO.mdgenerate_fun_data.shinclude/ecosystem.hrlpriv/supported_functions.datapriv/supported_functions.data.licenserebar.configrebar.lock.licensesrc/atomvm_spectrometer.erlsrc/spectrometer.app.srcsrc/spectrometer.erlsrc/spectrometer_analyzer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_ecosystem.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_analyzer_tests.erltest/spectrometer_atomvm_tests.erltest/spectrometer_http_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (6)
- .markdownlint.json.license
- .markdownlint.json
- priv/supported_functions.data.license
- rebar.lock.license
- README.md
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (28)
- src/spectrometer.app.src
- include/ecosystem.hrl
- .github/workflows/reuse-lint.yaml
- src/spectrometer.erl
- .github/workflows/code_quality_check.yaml
- src/spectrometer_otp.erl
- .github/workflows/publish_docs.yml
- test/spectrometer_reporter_tests.erl
- test/spectrometer_utils_tests.erl
- src/spectrometer_help.erl
- .github/workflows/build-and-test.yaml
- generate_fun_data.sh
- rebar.config
- test/spectrometer_analyzer_tests.erl
- src/spectrometer_atomvm.erl
- test/spectrometer_updater_tests.erl
- .github/workflows/check-formatting.yaml
- test/spectrometer_http_tests.erl
- src/spectrometer_utils.erl
- src/spectrometer_reporter.erl
- test/spectrometer_scanner_tests.erl
- src/spectrometer_http.erl
- src/atomvm_spectrometer.erl
- test/spectrometer_atomvm_tests.erl
- src/spectrometer_scanner.erl
- src/spectrometer_updater.erl
- test/cli_main_tests.erl
- src/spectrometer_analyzer.erl
MVP, likely still bugs, and refactoring is needed. Signed-off-by: Winford <winford@object.stream>
Signed-off-by: Winford <winford@object.stream>
Owner
Author
|
@coderabbitai please do a comprehensive final review, listing any unresolved change requests. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 675 🏁 Script executed: Length of output: 9682 🏁 Script executed: Length of output: 17258 🏁 Script executed: Length of output: 10163 🏁 Script executed: Length of output: 2407 🏁 Script executed: Length of output: 14973 🏁 Script executed: Length of output: 4321 🏁 Script executed: Length of output: 15300 🏁 Script executed: Length of output: 5999
Final Review:
|
| Area | Issue | Status |
|---|---|---|
spectrometer_http.erl |
GitHub search cursor never advanced (fetched same repos repeatedly) | ✅ Fixed — MinStarsInBatch - 1 sliding window |
spectrometer_http.erl |
Clone URL shell injection via os:cmd |
✅ Fixed — spawn_executable + {args, [...]} list |
spectrometer_http.erl |
git not on PATH caused badarg crash |
✅ Fixed — returns {error, git_not_found} |
spectrometer_http.erl |
receive could block forever on a hung git clone |
✅ Fixed — await_git_port with after 180000 + port_close + drain |
spectrometer_http.erl |
Tar path traversal (../ escape from temp dir) |
✅ Fixed — validate_tar_paths/1 + validate_tar_path/1 implemented |
spectrometer_http.erl |
SSL: wildcard hostname suffix check accepted evil-github.com |
✅ Fixed — public_key:pkix_verify_hostname_match_fun(https) + verify_peer + cacerts_get() |
spectrometer_ecosystem.erl |
Coordinator could hang; worker spawn_monitor refs discarded |
✅ Fixed — worker_monitors map tracks MonitorRef → WorkerPid; 'DOWN' handler in coordinator |
spectrometer_reporter.erl |
print_summary/1 would crash (sort_stats received map, not list) |
✅ Fixed — Supported ++ Unsupported (both lists); sort_stats/1 operates on lists |
spectrometer_updater.erl |
OTP-call filter logic inverted | ✅ Fixed — maps:filter keeps entries where predicate is true (OTP + external only) |
spectrometer_atomvm.erl |
find_arity/3 couldn't match wildcard all arity entries |
✅ Fixed — {_, all, Platforms, Since} clause added at line 117 |
spectrometer_atomvm.erl |
Module doc named wrong cache dir (atomvm_spectrometer vs spectrometer) |
✅ Fixed |
generate_fun_data.sh |
Stale entries carried forward from previous runs | ✅ Fixed — rm -rf "${TMP_CACHE_DIR}" before mkdir -p |
generate_fun_data.sh |
Backup timestamp mismatch (%H%M vs %H%m) |
✅ Fixed — single BACKUP_TS variable |
spectrometer_help.erl |
Stray ] in "Commands:]\n" |
✅ Fixed — now "Commands:\n" |
README.md |
spectrometer help examine missing from help examples |
✅ Fixed |
README.md |
Example combined incompatible --atomvm-dir and --branch/--tag flags |
✅ Fixed — listed separately with explanatory note |
TODO.md |
Typo: "will case noticeable errors" | ✅ Fixed — "will cause noticeable errors" |
test/cli_main_tests.erl |
cache_dir app env leaked across tests |
✅ Fixed — main_filter_no_user_state_test_ saves/restores env in setup/teardown |
test/cli_main_tests.erl |
ensure_atomvm_repo parent TempDir not cleaned up |
✅ Fixed — teardown calls purge_dir(TempDir) at lines 620 and 673 |
test/*_tests.erl |
Tests call unexported merge_file_calls/2, derive_since/2, is_older_since/2, etc. |
✅ Not a problem — test profile enables export_all in rebar.config |
🔴 Still Unresolved — Blocking
1. spectrometer_otp.erl Line 41: binary_to_term without [safe]
% Line 41 — currently:
try binary_to_term(Bin) ofThe OTP module list is loaded from a user-writable cache file (~/.cache/spectrometer/otp_<vsn>_modules.bin) using binary_to_term/1 without the [safe] option. A crafted binary in that file could silently create atoms beyond the atom table limit (atoms are never GC'd in the BEAM) and cause a node crash. The try/catch wrapper catches the crash but by that point atom table exhaustion may already have occurred.
Fix: add the [safe] option:
try binary_to_term(Bin, [safe]) of🟡 Unresolved Minor Items
2. spectrometer_otp.erl: Decode failures don't self-heal the cache
When binary_to_term fails (or yields invalid content), the fallback path calls regenerate_and_write/1 — which does rewrite the cache. However, the {error, Reason} branch from file:read_file/1 (line 75) also calls regenerate_and_write/1. Verified both branches do rewrite, so this is actually resolved. ✅
3. generate_fun_data.sh: TMP_CACHE_DIR not cleaned up on early set -e exit
The rm -rf "${TMP_CACHE_DIR}" at the end of the script only runs on normal exit. If rebar3 escriptize or a spectrometer update call fails and set -e terminates the script early, the temp cache directory is left behind. Consider using a trap for cleanup:
trap 'rm -rf "${TMP_CACHE_DIR}"' EXITSummary
| Severity | Count | Status |
|---|---|---|
| 🔴 Critical | 1 | Unresolved — binary_to_term without [safe] |
| 🟡 Minor | 1 | Unresolved — generate_fun_data.sh trap for cleanup |
| ✅ Resolved | 20+ | All previously flagged issues addressed |
The codebase is in good shape overall. The one blocking item is the binary_to_term(Bin) call in spectrometer_otp.erl — adding [safe] is a one-word fix.
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
1 participant
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.
Basic functionality is working. All tests pass, and dialyzer is clean.