Fix everything the Fable reviews found - #12
Merged
Merged
Conversation
The fallback payload -- the one built from `declare -p`/`declare -f` when
BASH_SOURCE is empty, which is every `curl | bash` and `bash -c "$(curl ...)"`
invocation -- was missing two things, and each was independently fatal on the
remote node.
READY_PROBE_TIMEOUT and READY_POLL_INTERVAL were absent from the declare list
while parse_args normalises both. `${!name}` on an unset name under set -u
aborts, so every remote node died before touching a container. The EX_* exit
constants were absent too, so even past that, any remote error path -- `die
"$EX_LOCKED"` on a held lock -- hit a second unbound variable at exactly the
moment it needed to report cleanly. The README advertises this transport
explicitly; it never worked.
Two exit codes were also wrong. An unopenable lock file returned 75, which
docs/exit-codes.md defines as "another instance holds the lock, retry later,
nothing is wrong" -- so an unwritable /run/lock made a cron job retry a
permanent misconfiguration forever instead of reporting it once. It is now
EX_PREREQ, matching what maintenance.sh already did for the identical
condition. And a remote node that was merely locked landed in NODES_FAILED and
failed the whole cluster run, contradicting both the help text ("a node that is
already running the updater is reported, not disturbed") and the repo's stated
reason for having 75 at all. Those nodes now have their own bucket, reported
and counted separately, with an lzc_lxc_nodes{state="locked"} series.
Also documents the ten environment tunables that appeared in neither --help nor
the README, adds the --logs-url/--metrics-url flags to --help (parsed and in
the README, but not in the help text), and records that in file-mode cluster
runs the driver's LZC_UPDATE_LXCS_* settings do not travel to remote nodes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects, all in the multi-distro dispatch. Arch reboot detection was a false negative on the one distribution it was tested against. The check was "the running kernel's /usr/lib/modules/<ver> directory is gone", but Manjaro's kernel tooling deliberately keeps that tree alive across an upgrade so the running kernel can still load modules. On this host -- running 7.1.8, with 7.1.9 installed -- the directory exists and the report said no reboot was needed while the report's own kernel section listed the two versions side by side. It now also treats a present-but-unowned modules directory as the signal, which is what an upgraded-away kernel actually leaves behind: `pacman -Qoq` fails on it. Verified against this host, which correctly reports yes. On RHEL and SUSE, any non-zero status from `needs-restarting -r` was read as "reboot required". The inverted convention there is that 1 means yes, but the guard was a bare `|| REBOOT_REQUIRED=1`, so a probe timeout (124), a dnf error, or zypper's exit 7 for "another zypper holds the ZYPP lock" -- a state this report is explicitly built to tolerate elsewhere -- all produced a permanent phantom pending reboot. Only 1 means yes now; anything else warns and reports unknown, which is the same "absent means unknown, never yes" rule the tool availability check already followed. The rhel update arm passed --refresh and --setopt=strict=0 unconditionally. Both are dnf-only, and PKG_TOOL is `yum` precisely when dnf is absent, so the task failed before it started on classic yum hosts. yum now gets an explicit `clean expire-cache` instead, checked the same way the debian refresh is. The arch orphan purge expanded `$orphans` unquoted. Word splitting was the intent; glob expansion came with it, so a local-db entry named `[a-z]*` would have become every matching name in the current directory, handed to a root `pacman -Rns`. Pacman's name grammar excludes those characters so an intact db cannot trigger it, but a root deletion argv should not depend on that. Reproduced with a stub, then fixed with mapfile and a quoted expansion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The library's first design rule is that nothing in it can change the outcome of the script that sourced it. Two paths still could. `date` was the only external command left called unguarded -- curl is probed with `command -v`, hostname has a $HOSTNAME fallback, date had nothing. On a host without it, obs_init died outright, and because obs_finish runs from the caller's EXIT trap, a completely successful run exited 127. The irony is that the hostname fallback's own comment reasons about "a minimal container with no coreutils hostname" -- the same container has no date. Both timestamp helpers now use printf's %(...)T, which is a bash builtin and cannot be missing. TZ=UTC because %(...)T formats local time where `date -u` did not; verified identical output to `date -u`. A readonly LZC_* variable in the caller's environment aborted the source. That one took two attempts: assigning to a readonly name is a *fatal* shell error, so the obvious `printf -v ... || true` does not rescue it -- the shell exits at the assignment. The setter now checks `declare -p` for the readonly flag and declines to write instead. That fix alone re-opened the original crash from a different direction, though: a variable that is readonly *and* invalid cannot be normalised, so `(( _obs_buffered >= LZC_OBS_BUFFER ))` was back to dying with "bogus: unbound variable". The normalised values now live in private `_obs_*` variables that logic reads, mirrored onto the public LZC_* names only when those are writable. A caller can no longer make a setting this library depends on both immutable and broken. Verified: with LZC_OBS_BUFFER, DEBUG, INSECURE, TIMEOUT or RETRIES readonly and set to nonsense, a `set -Eeuo pipefail` caller with an unreachable collector completes and exits 0, across all four log formats. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
as_float caught TypeError and ValueError but not OverflowError, which is an
ArithmeticError. json.loads produces Python ints of arbitrary precision, so one
drive reporting an absurd raw SMART attribute raised here, escaped through
collect_families, and hit the collector's deliberately-broad guard -- taking
every disk's series down and reporting hostwatch_collector_up 0 for the whole
subsystem. The function's contract is per-field degradation; one bad field must
cost that field alone. The same guard now rejects infinities as well as NaN:
`as_float('inf')` returned inf and serialized as +Inf, which Prometheus accepts
but which contradicts _finite(), and an infinite reading is not a measurement.
Installer: a dependency change installed and then did not restart. NEEDS_RESTART
was set for the payload, the unit and the environment file, but neither the
requirements.txt render nor the pip run touched it -- so bumping a pinned
version put the new package in the venv and left the long-lived process running
the old one, logging "already running with the current configuration".
--force-pip on its own was a complete no-op against the running service. Both
sites now set it.
Two smaller installer fixes. The venv health gate tested only that the
interpreter existed, so an interrupted first install -- python present, pip
broken -- reported "already present" and then died in the pip step on every
subsequent run, with no way to self-heal; it now checks pip too and rebuilds.
And --uninstall guarded the service account by uid range alone, which would
have removed any system account in 1..999 that --user happened to name. It now
also requires the home directory and shell to match what this installer
actually creates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…code Clean-Disk's DESCRIPTION promises that "reparse points are skipped, so a junction or symlink inside a temp directory cannot be used to escape the target". It filtered reparse-point *leaves* out of a `Get-ChildItem -Recurse` walk, which is not a weaker version of that promise but no version of it: Windows PowerShell 5.1 descends into directory junctions, and a file inside one is an ordinary file whose FullName still begins with the root, so it passed both the containment test and the reparse test. Removing it then resolved through the junction and deleted a file outside the swept tree. Replaced with an explicit iterative walk that prunes reparse-point directories, so nothing beneath one is ever returned. Both passes use it -- the file enumeration and the empty-directory sweep, which had the same shape. ManageService gated its "cannot be stopped" precheck on `-Action Stop` alone. Restart stops the service too, so a Restart of a running service reporting CanStop false skipped the clean exit 2, ran into the Stop-Service timeout, and surfaced as a generic exit 1 -- while both the help text and the README list that condition under exit 2 with no action qualifier. Neither change is executed-tested: there is no PowerShell on the machine this was written on. Both are static changes to logic whose defect was demonstrable by reading, and CI runs PSScriptAnalyzer under Windows PowerShell 5.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A review checked every documented claim against the code. These are the ones that disagreed -- mostly rules written after the code they describe, which then drifted. Four scripts' --help promised that every option has an LZC_* variable while --color had none: setting LZC_CLEAN_LOGS_COLOR=bogus was silently ignored where --color bogus correctly exits 2. Wired the variable in all four rather than softening the claim, so --color is now uniform across every script, and corrected linux/README.md, which listed the old partial set. CONTRIBUTING's locking rule named a path convention that update-lxcs.sh does not follow (it predates it) and omitted install.sh, the one mutating script with no lock. Its environment-variable rule claimed LZC_<FOLDER>_<OPTION> repo-wide, which is true for linux/<name>/ and false for the exporter (LZC_EXPORTER_, folder is monitoring), the LXC updater (LZC_UPDATE_LXCS_, folder is ve) and the library (LZC_OBS_). Both rules now describe the repository that exists, with the reasoning for the exceptions. docs/exit-codes.md is the page every other doc points at as the single table, and it was the only one that never mentioned ResetNetwork.ps1's 3010. docs/ README.md said lzc-obs.sh "defines functions only"; it executes one statement at source time. CLAUDE.md's error-model roster omitted network_restart.sh. tests/README.md claimed py_compile also runs in CI. It does not -- lint.yml has no Python job at all, which also makes the workflow's own "every script in the repo" header an overstatement. Both corrected, and the count of what the local runner adds over CI is now four everywhere instead of two in some places and three in others. It reads one pinned version out of the workflow, not several. Also: the undocumented --install flag in the exporter installer, the dry-run printing [OK] for work it had not done, a note that a custom completion directory must be passed to --uninstall too, and a stray /universal scripts entry at the top of .gitignore. The generated bash-completion file had three SC2207 findings. It is written by install.sh rather than committed, so neither CI nor run_checks.sh ever globbed it -- an artifact shipped by this repository that would fail this repository's own zero-findings bar. Rewritten with mapfile and checked by extracting the heredoc and running shellcheck over it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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.
Six independent reviews covered the whole repository. This fixes every confirmed defect they raised, plus the documentation drift. I re-verified each finding myself before acting on it; two needed a different fix than proposed.
Correctness
Cluster mode never worked when piped from the network. The fallback payload omitted
READY_PROBE_TIMEOUT/READY_POLL_INTERVAL(whichparse_argsnormalises) and theEX_*constants. Underset -uevery remote node died before touching a container, and any error path hit a second unbound variable. This is the transport the README advertises.Arch reboot detection was a false negative on the one distro it was tested on. Manjaro keeps the running kernel's modules tree alive across an upgrade, so "directory is gone" never fires. This host — running 7.1.8 with 7.1.9 installed — reported no reboot needed. This corrects something reported as verified earlier: when it was first tested, the tree happened to be absent. It now also treats a present-but-unowned modules dir as the signal.
Any non-zero
needs-restarting -rmeant "reboot required" on RHEL and SUSE — including a probe timeout (124) and zypper's exit 7 for a held ZYPP lock. Only 1 means yes.as_floatdidn't catchOverflowError, so one absurd SMART value from one drive collapsed the entire smart collector toup 0for every disk.A dependency change installed but never restarted the service —
--force-pipwas a complete no-op against the running process.Clean-Diskpromised junction-escape protection it didn't implement. It filtered reparse-point leaves from a-Recursewalk, but PowerShell 5.1 descends into junctions and a file inside one passes every guard. Replaced with an explicit walk that prunes reparse-point directories.Also:
yumwas passed dnf-only flags; the arch orphan list was glob-expanded into a rootpacman -Rns; an unopenable lock file returned 75 ("retry later") for a permanent misconfiguration; a merely-locked cluster node failed the whole run;ManageServiceskipped its clean exit 2 onRestart; the venv health gate ignored a broken pip;--uninstallguarded the service account by uid range alone.The observability library took two attempts.
datewas the last unguarded external command — a missingdateturned a successful run into exit 127 via the EXIT trap. Then: assigning to a readonly variable is a fatal shell error that|| truecannot rescue, so the setter had to detect readonly instead. That fix re-opened the original crash from another angle — a readonly and invalid value cannot be normalised — so normalised values now live in private variables that logic reads.Documentation
Every documented claim was checked against the code. Four scripts'
--helppromised an env var for--colorthat did not exist (wired it, rather than weakening the claim). CONTRIBUTING's locking and env-naming rules described a repo that does not exist.docs/exit-codes.md— the page everything else points at — never mentioned the one 3010 exception.tests/README.mdclaimedpy_compileruns in CI; it does not, which also made lint.yml's "every script" header an overstatement.Verification
tests/run_checks.shpasses; ShellCheck 0.10.0,bash -n,py_compileand YAML all clean. Reproduced-then-fixed: the reboot predicate against this host's real state, all fourneeds-restartingexit codes, yum/pacman argv via stubs,as_floatacross overflow/inf/NaN, a full install→uninstall cycle underunshare -rleaving nothing behind, and the library surviving readonly-and-invalid settings across all four log formats.The two Windows changes are not executed-tested — there is no PowerShell on this machine. Both fix defects demonstrable by reading; CI runs PSScriptAnalyzer under 5.1. CRLF endings were also repaired after a Python edit stripped them; all ten
.ps1are byte-verified CRLF.