fix: correct make upgrade version reporting (yq, gh-aw, google-workspace-cli, node, npm globals)#94
Merged
Merged
Conversation
yq's -v flag means --verbose, not --version. The generic version probe tries -v first, so it captured a DEBUG log line and parsed the timestamp millis (e.g. 19.169) as the version. Pin version_flag to --version so detection reads 'yq ... version v4.53.3'. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The release asset is google-workspace-cli-<arch>-unknown-linux-gnu.tar.gz,
not gws-<arch>-... The wrong template 404'd ('Download failed'). The
binary inside the archive is still 'gws'.
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…binary gh-aw prints 'gh aw version vX.Y.Z' to stderr, but the before/after probes redirected stderr to /dev/null, so both showed <none>. Add a shared detect_version_string helper that prefers stdout and falls back to stderr. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…prefix Three PATH-trust bugs surfaced during make upgrade: - prefers_nvm_node compared the unresolved symlink path, so a ~/.local/bin/node shim into ~/.nvm was misread as a manual install and triggered the apt removal path. Resolve symlinks first. - ensure_nvm checked 'nvm' before sourcing nvm.sh, re-running the nvm web installer every run (which hijacked NODE_VERSION, printing 'Failed to install Node.js <ver>'). Source nvm first; clear NODE_VERSION on bootstrap. - npm installs globals into 'npm prefix -g'/bin, which is not always on PATH; command -v then reported freshly-installed tools as <none> or 'binary not found'. Add resolve_global_bin/warn_if_bin_off_path and use them in npm_global.sh and reconcile.sh. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Covers yq version_flag, google-workspace-cli asset URL, github_release stderr fallback, prefers_nvm_node symlink resolution, and off-PATH npm global resolution. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 68.87% 69.68% +0.80%
==========================================
Files 22 22
Lines 3345 3345
==========================================
+ Hits 2304 2331 +27
+ Misses 1041 1014 -27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several fixes to improve tool installation, version detection, and path resolution. Key changes include correcting the download URL for google-workspace-cli, adding an explicit version flag for yq, resolving symlinks for NVM-managed Node.js, and handling globally installed npm binaries that are off-PATH. Additionally, version detection has been updated to capture stderr output for tools like gh-aw. Feedback on these changes suggests making version detection in github_release_binary.sh more robust by falling back to the target installation directory, optimizing get_npm_tool_version in npm_global.sh to exit early if the binary path is empty, and prepending the resolved binary's directory to PATH in reconcile.sh to ensure detect_install_method functions correctly when the binary is off-PATH.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…talled display The guide rendered 'installed:' from a possibly-stale snapshot while the installers detect the live 'before:'. When the snapshot lagged reality the two contradicted each other (e.g. 'installed: not installed' directly above 'before: 10.5.0', or 'installed: 11.12.1' above 'before: 11.17.0'). Refresh local installation state (network-free 'audit.py --update-local') at guide startup so the displayed status matches what the installers see. Upstream 'target' versions still come from the cached baseline, so the guide stays offline. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
- github_release_binary: gate the stderr version fallback on a version-like token so a stderr banner/warning is not surfaced as the version. - npm_global: only prepend the npm-global bin dir to PATH for a version_command when it is genuinely off PATH, so a stray or hostile npm prefix cannot shadow normal PATH lookups in the common (on-PATH) case. - common: generalize the off-PATH warning message (it is used for non-nvm tools too). - tests: pin the yq verbose-misparse regression to the exact misparsed value instead of a weak inequality. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
TestVersionLineRespectsFlag executes a #!/bin/sh fake binary via get_version_line, which cannot run on the Windows CI runner. Mark it @skip_on_windows like the other shell-based test classes; the JSON-only catalog tests still run on all platforms. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The startup 'audit.py --update-local' added to the guide writes local_state.json, but the guide's render reads tools_snapshot.json (load_snapshot), so the refresh never updated the displayed 'installed:' status -- it only added ~5s latency. Remove it. Installed-vs-snapshot consistency is a separate data-model concern (tools_snapshot.json vs local_state.json vs the upstream baseline). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
- github_release_binary: extract the version-token regex to a constant (Sonar S1192), use [[ ]] (S7688), and broaden the stderr gate to also accept 8-digit dates so a dotless/CalVer version is not dropped (matches normalize_version_output). - npm_global / common: use [[ ]]; add path_contains_dir helper and reuse it; only prepend the npm-global bin dir to PATH when it is genuinely off PATH. - common: npm_global_bin_dir now always returns 0 (safe to call bare). - install_node: guard ensure_nvm against an endless re-bootstrap when ~/.nvm/nvm.sh exists but is corrupt. - tests: add behavioral tests for the stderr/banner gate and the npm PATH-prepend gating; strengthen the yq verbose-misparse assertion; drop the obsolete guide test. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
detect_version_string and get_npm_tool_version use 'timeout', which BSD/macOS lacks, so the extracted-function tests returned empty on the macOS runner. Add a passthrough timeout() shim when the command is absent so the tests run on macOS. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The render (make upgrade) showed 53 'outdated' while make update showed 3. Two root causes, both fixed: - Status was computed by EQUALITY (installed != latest => OUTDATED), so a tool AHEAD of the (possibly stale) committed upstream baseline was falsely flagged OUTDATED (e.g. ansible-core 2.21.1 vs baseline 2.20.1). Add compute_status() with DIRECTIONAL comparison (installed >= latest => UP-TO-DATE; missing latest => UNKNOWN, never a false OUTDATED) and use it at all three sites (audit_tool, multi-version, cmd_update_local). False 'outdated' dropped from ~41 to 0. - make update's per-category summary omitted multi-version cycles while make upgrade's Readiness line counts them. Emit the same Readiness summary at the end of make update so both report identical totals. Verified: make update and make upgrade now print the identical Readiness line. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
compute_status pushed the 'NOT INSTALLED' literal to 6 occurrences, which Sonar flagged (S1192). Name it once as STATUS_NOT_INSTALLED. Behavior unchanged (the string value is identical, so JSON/status filtering still match). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Address gemini-code-assist review feedback: - detect_version_string (github_release_binary): when command -v misses the binary (install dir not yet on PATH on a first-time install), fall back to the install dir from get_install_dir and run the version probe on that path. - get_npm_tool_version (npm_global): return early when the binary was not found, instead of running a version_command that cannot resolve it. - reconcile verify: prepend the resolved binary's dir to PATH so detect_install_method (which uses command -v) can classify an off-PATH install instead of returning 'none'. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
CybotTM
added a commit
that referenced
this pull request
Jun 20, 2026
…before: (#95) ## Summary In `make upgrade`, the `installed:` line (from the cached snapshot) could disagree with the installer's live `before:` line — e.g. `installed: not installed` directly above `before: v10.5.0`, or `installed: 11.12.1` above `before: 11.17.0`. They describe the same thing (the version present before the upgrade), so they must match. **Root cause:** the guide renders `installed:` from `tools_snapshot.json`, which goes stale (e.g. after tools change outside the tool), while the installers detect live. ## Fix The guide now refreshes the snapshot's **installed** state at startup (network-free) so `installed:` matches `before:`. Crucially, the full `audit.py --update-local` path now **refreshes `installed_version` + status from live detection but PRESERVES each tool's existing `latest_version`** — instead of rebuilding `latest` from the older committed baseline (which would show a `target` *lower* than `installed` and re-open the `make update` vs `make upgrade` divergence fixed in #94). Status is recomputed with the directional `compute_status` from #94. Multi-version cycles (`node@24`, `python@3.12`, …) are left to `make update` / the per-cycle re-audit, since `--update-local` has no per-cycle local-only data. ## Verified | tool | before (stale snapshot) | after refresh | live `before:` | |------|------------------------|---------------|----------------| | npm | installed `11.12.1`, latest `11.17.0` | installed **`11.17.0`**, latest `11.17.0` (preserved) → UP-TO-DATE | `11.17.0` ✓ | | prettier | installed `—`, latest `3.8.4` | installed **`3.8.4`**, latest `3.8.4` (preserved) → UP-TO-DATE | `3.8.4` ✓ | | eslint | installed `10.5.0`, latest `10.5.0` | unchanged → UP-TO-DATE | `v10.5.0` ✓ | Behavioral test added (`--update-local` preserves a seeded `latest` while refreshing `installed`). Full suite **630 passed**; `flake8`/`shellcheck` clean on changed lines. Trade-off: adds a ~5s network-free local detection at guide startup — the accepted cost of an accurate `installed:` before modifying the system.
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
make upgradereported wrong, missing, or confusing information — andmake updatevsmake upgradedisagreed on the outdated count. This PR fixes the underlying detection/reporting and data-model bugs. No tool's actual upgrade behavior changed.Version detection / reporting fixes
installed: 19.169(garbage)-vfirst =--verbose; grabs a DEBUG-log timestampversion_flag: --versionbefore/after: <none>--versionto stderrDownload faileddownload_url_templatesudo/apt+Failed to install Node.js 26prefers_nvm_nodecompared the unresolved symlink;ensure_nvmchecked before sourcing nvmNODE_VERSION; guard corrupt-nvm re-bootstrap<none>/binary not foundresolve_global_bin/warn_if_bin_off_path; off-PATH install-dir fallbackData-model fix —
make updateandmake upgradenow agreeThe render showed 53 "outdated" while
make updateshowed 3. Root causes:installed != latest → OUTDATED), so a tool ahead of the (stale) committed baseline was falsely flagged (e.g. ansible-core 2.21.1 vs baseline 2.20.1). Newcompute_status()uses directional comparison (installed >= latest → UP-TO-DATE; missing latest → UNKNOWN). ~41 false "outdated" → 0.make update's category summary omitted multi-version cycles; it now emits the same Readiness line asmake upgrade.Verified: both print the identical
Readiness:line.Testing & review
gemini-code-assistthreads addressed.shellcheck/flake8clean on changed lines; CI green across ubuntu/macOS/Windows.