Repository navigation
fix: localize gate03 sweep helper variables - #192
hermanngeorge15 merged 3 commits into
Conversation
hermanngeorge15
left a comment
There was a problem hiding this comment.
Reviewed. The fix is correct and, importantly, complete — I checked every assignment in the three functions against the local declarations rather than taking the diff at face value:
| function | variables assigned | declared local |
|---|---|---|
sweep_one |
out_dir dir abs_dir slug report status files_scanned findings |
all 8 |
build_summary |
out_dir |
yes |
main |
out_dir, plus the loop variable dir |
both |
Nothing is left global. do_compare correctly needs none, since its assignments live inside the Python heredoc, as you said.
Keeping the scanner invocation and the status capture on separate lines is the right call and worth the comment you gave it — local status=$(...) would have masked the command's exit code behind local's own, which is exactly the class of bug this PR is fixing.
"The check fails against upstream and passes with the fix" is the right way to demonstrate a regression test. Thank you for stating it that way.
CI is red on PERF-01 only; that is a pre-existing flake on main, not this PR. See #188.
Two non-blocking suggestions:
-
tests/gate03_sweep_test.shis added with mode100644, whilescripts/gate03-sweep.shis100755. CI invokes it asbash tests/...so it works, but./tests/gate03_sweep_test.shwill not. Worthchmod +xfor consistency with the rest of the repo. -
source <(sed '/^main "\$@"$/d' ...)depends on the entry point being the exact literal linemain "$@". If that line is ever reformatted — wrapped in a guard, given an argument, indented — thesedsilently stops matching and the test starts executing the realmainat source time, which would be confusing to debug. The sturdier idiom is to guard it in the script itself:if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then main "$@"; fi
Then the test can source the script unmodified. Your call whether that is worth the churn in this PR or a follow-up.
Approving.
eff15b3 to
d86ff2a
Compare
d86ff2a to
1384f9e
Compare
The sweep helpers assigned their working variables globally, allowing calls to overwrite caller state. Declare Bash variables local in
sweep_one,build_summary, andmain, including the loop variable and scanner exit status. Keep the scanner command and status capture separate solocalcannot mask its exit code.do_compareneeds no Bash declarations because its assignments occur inside a separate Python process.Add
tests/gate03_sweep_test.shand run it in CI. It checks caller-variable preservation through direct helper calls andmain, scanner exit codes 0/1/2, missing directories, manifest and summary output, and equal/different finding comparisons.Validation:
bash -n scripts/gate03-sweep.shandbash -n tests/gate03_sweep_test.shpass.python3locally because Windows' python3 alias is inactive).git diff --checkpasses. No Rust code or patterns changed; the full Rust suite was not rerun locally.Closes #181.