fix(guide): judge auto-updates by the version, not the exit code - #149
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
The auto-update branch of make upgrade counted SUMMARY_UPDATED whenever the install script exited 0. A run in which black, isort, python@3.14, codex, sd and bwrap all kept their old version reported "Updated: 6". The interactive Y and a answers compared versions, but each in its own copy of the logic, and they already disagreed on the already-current case. upgrade_verdict now classifies every upgrade after the re-audit: updated, failed, unchanged, already-current or held-back. report_upgrade_verdict prints it and updates the counters, for the auto-update branch and both interactive answers. - unchanged: counts as Failed, and names the old and the target version. - held-back: package_manager.sh writes a marker when the package manager has no newer version (bwrap 0.9.0 on apt, upstream 0.12.0). Counts as Skipped. - already-current: the binary is identical to the target release but reports an older version (sd 1.1.0 prints 1.0.0). Counts as Skipped and pins the release, so the next run does not download it again. Touching guide.sh makes the shellcheck hook lint the whole file: its 26 SC2155 warnings are split into declaration and assignment, with "|| true" to keep the masked exit status they had. An unused variable is removed, and INTERRUPTED (written only by the INT trap) gets a shellcheck directive. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_017qjwSFFBZpMj3uw5bPdrnD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
Review findings on the upgrade verdict: - already-current pinned the release. The guide skips every pinned tool whatever the pin value, so the pin hid the next real release too, and auto-update with it. The verdict no longer pins; sd shows up each run and counts as Skipped. - package_manager.sh wrote the held-back marker whenever the version was unchanged, but apt-get, brew, dnf and pacman all ran with "|| true". A dpkg lock, a refused sudo or no network therefore read as "no newer version". The marker now needs the install command to succeed and, for apt, the installed version to equal the candidate. - Markers were only removed after a verdict. One left by make upgrade-<tool>, an interrupted run or another cycle of the same tool could decide a later verdict. They are cleared before each install. - The short-version match had no dot boundary: 1.1 matched 1.12.0 and counted as Updated. It now needs "<short>." as prefix. - Y answer: held-back gets the pin offer again, as before. Tests run under set -euo pipefail and cover each case; package_manager.sh runs against stub apt tools. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_017qjwSFFBZpMj3uw5bPdrnD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
The guide skipped every tool that had any pin. The "s" answer ("Skip
only <latest>, ask again when newer patch available") pins the skipped
release, so the tool never came back when a newer release appeared.
pin_version.sh prints the same promise.
pin_applies keeps a tool hidden while the pin is "never", equals the
target release (s), equals the installed version (p, "don't ask for
upgrades"), or, for a cycle, equals the cycle. Everything else shows the
tool again.
On the reporting machine this changes 1 of 12 pins: git-branchless is
pinned to the skipped 0.10.0, and 0.11.1 is out.
Assisted-by: claude-code:claude-opus-5
Agent-Session: https://claude.ai/code/session_017qjwSFFBZpMj3uw5bPdrnD
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
…ee probe
Review round 2:
- apt-cache policy translates "Candidate:" (de: "Installationskandidat:"),
so under a German locale the candidate was never found and every held-
back package counted as Failed. It runs with LC_ALL=C now.
- A package installed at its candidate but shadowed by another copy on
PATH still counted as held back. For apt, the upstream part of the
package version must now start with the detected version.
- With no upstream version known, an unchanged version counted as
Failed and the Y answer offered a pin to "<current>". It is reported
as "unverified" (Skipped), and no pin is offered without an installed
version.
- probe_installed_version used command -v on the full PATH, so with a
venv active the fallback probe read the venv copy. It uses a PATH
without venv/conda dirs (parameter expansion, no dirname: the probe
must not depend on PATH to filter PATH).
- The marker dir can be set with CLI_AUDIT_MARKER_DIR, so tests no
longer share /tmp/.cli-audit with each other or with a real run.
- The pin check reads latest/installed only when a pin exists.
- github_release_binary.sh: ${LIB_DIR:?}/${PRESERVE_DIR:?} (SC2115,
required by the shellcheck hook for the touched file).
Assisted-by: claude-code:claude-opus-5
Agent-Session: https://claude.ai/code/session_017qjwSFFBZpMj3uw5bPdrnD
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
Review round 3: - The shadow check compared the package's upstream version with the version the binary prints. universal-ctags 5.9.20210829.0-1 prints 5.9.0, so an up-to-date apt ctags counted as Failed on every run; and without a dot boundary a shadowing "2.4" matched package 2.43.0. package_manager.sh now asks dpkg -S which package owns the binary found on PATH; held-back requires it to be one of the catalog's packages. Checked on Ubuntu 24.04: bwrap, ctags (via alternatives), entr, git, rename.ul, prename, sponge and php8.5 resolve to their catalog package. - An install that left no detectable binary and has no upstream version was "unverified"; it is "unchanged" (Failed) again. - Comments no longer name a Python function that only exists on another branch. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_017qjwSFFBZpMj3uw5bPdrnD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
Review round 4 (latent; none of the catalog's apt tools is affected on
Ubuntu 24.04, and each case failed towards "Failed", never towards a
false "held back"):
- A diverted file lists "diversion by X from/to:" lines first, and
"head -1" took that line as the owner. dpkg_owners skips diversion
lines.
- Several owners ("moreutils, parallel: /usr/bin/parallel") were not
split.
- On merged-/usr systems some packages still record /bin/x while
readlink -f gives /usr/bin/x (iproute2 on 24.04; many on Debian 12).
The owner lookup tries the resolved path, the PATH entry, then
/bin/<name>.
- Tests feed the real output shapes: diversion with several owners,
"pkg:amd64", and a /bin-only record.
Assisted-by: claude-code:claude-opus-5
Agent-Session: https://claude.ai/code/session_017qjwSFFBZpMj3uw5bPdrnD
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
Review round 5 (no correctness bug): - The comment on the owner lookup read as if a path with a foreign owner falls through to the next candidate; dpkg_owners stops at the first path dpkg knows. The comment says so. - Cutting the "<owners>: /path" line at ": /" had no test. A path with ", <package>" in it would otherwise become a second owner; the new negative test fails without the cut. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_017qjwSFFBZpMj3uw5bPdrnD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
CybotTM
force-pushed
the
fix/upgrade-verdict
branch
from
September 22, 2026 06:07
9a103a6 to
4f320aa
Compare
|
Member
Author
|
Self-review: 4f320aa The bot review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). The diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
After merge,
make upgradecounts an upgrade as "Updated" only when the version changed. It no longer counts a tool as updated just because the install script exited 0. In the reported run, black, isort, python@3.14, codex, sd and bwrap all kept their version, and the summary said "Updated: 6".guide.shincrementedSUMMARY_UPDATEDon the exit code alone and deleted the already-current marker without reading it. The interactiveYandaanswers compared versions, each in its own copy of the logic, and the two copies disagreed on the already-current case.upgrade_verdictclassifies each upgrade after the re-audit:updated,failed,unchanged,already-currentorheld-back.report_upgrade_verdictprints the verdict and updates the counters. The auto-update branch and both interactive answers use these two functions.unchangedcounts as Failed and prints the old and the target version.make upgrade-<tool>or an interrupted run cannot decide a later verdict.held-back:package_manager.shwrites a marker only when the install command succeeded and, for apt, the installed version equals the candidate. This is bwrap: apt has 0.9.0, upstream has 0.12.0, and upstream ships source only. Counts as Skipped. The candidate is read withLC_ALL=C(apt translates the label), anddpkg -Smust name one of the catalog's packages as owner of the binary found on PATH, so a copy elsewhere on PATH does not count as held back. (Comparing version strings does not work: universal-ctags 5.9.20210829.0 prints 5.9.0.) The owner lookup skips diversion lines, splits several owners, drops:arch, and falls back to/bin/<name>for packages that still record/binon merged-/usr systems. A failed install (dpkg lock, refused sudo, no network), a newer candidate that did not take effect, or a shadowing copy counts as Failed.unverified: no upstream version to compare with. Counts as Skipped; no pin offer.already-current: the binary is identical to the target release, but its version string is older. This is sd: the 1.1.0 release binary prints 1.0.0, because itsCargo.tomlat v1.1.0 says 1.0.0. Counts as Skipped. It is not pinned: the guide hides every pinned tool, whatever the pin value, so a pin would also hide the next real release.Pins: the guide skipped every tool with any pin. The
sanswer promises "ask again when newer patch available", but its pin hid the tool for good.pin_applieskeeps a tool hidden only while the pin isnever, equals the target release (s), equals the installed version (p), or equals the cycle. On the reporting machine this changes 1 of 12 pins: git-branchless, pinned to the skipped 0.10.0, is offered 0.11.1.Touching
guide.shmakes the shellcheck hook lint the whole file. Most of the diff clears its 26 existing SC2155 warnings: each declaration is split from its assignment, with|| true, so a failing command stays non-fatal as before.Type of change
fix:)Test plan
uv run pytest tests/ --ignore=tests/integration: 818 passed, 1 skippedshellcheck --severity=warningon the changed scripts: 0 findings (32 before)./scripts/test_smoke.sh: exit 0tests/test_guide_upgrade_verdict.py(28 tests) runs the real functions, extracted fromguide.sh, underset -euo pipefail, and runspackage_manager.shagainst stub apt tools. Removing each round-1 fix makes its test fail. Case one is the black run: exit 0, version still 25.11.0. When theunchangedverdict is changed toupdated, case one fails. When the auto-update branch is reverted to count on the exit code, the wiring test fails.Checklist
CHANGELOG.mdupdatedAssisted by claude-code:claude-opus-5 — Session