Self-Heal a Shadowing uv, jq, or git-restore-mtime Copy on Install - #689
Conversation
install-tools.sh already detected a copy of uv, jq, or git-restore-mtime earlier on PATH than $BIN_DIR (tool_note's report), but nothing acted on it. --install/--upgrade only ever touched $BIN_DIR, so a report run right after still repeated the same note. Worse, apply_tool() reads a tool's version via 'command -v', so a shadow that already happened to be current made the tool report as current and return before ever reaching its own install function. That is how uv installed per #483's own documented method (the astral.sh installer, into ~/.local/bin, ahead of /usr/local/bin on a stock Debian PATH) never converged on repeated --upgrade runs. tool_shadow_path() factors the existing PATH-vs-$BIN_DIR check out of tool_note() so the report and the fix below share one answer. tool_unshadow() acts on it: before the version read, --install and --upgrade remove a shadowing copy, prompting first and respecting --yes/--dry-run like the apt_install_displacing confirm already does, covering uv's uvx companion from the same archive too. Fixes#688. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Linux tooling installer to detect when a user- or system-installed uv, jq, or git-restore-mtime earlier on PATH would shadow the managed copy in /usr/local/bin, and attempts to “self-heal” that state during --install / --upgrade so subsequent --report runs converge.
Changes:
- Factor shadow detection into a shared helper (
tool_shadow_path) used by both reporting and install/upgrade paths. - Add
tool_unshadowand invoke it early inapply_tool()to remove shadowing copies (includinguvxalongsideuv). - Update the report note logic to use the shared shadow detection helper.
Suppressed comments (1)
host-setup/linux/install-tools.sh:757
tool_unshadowwill currently delete whatevertool_shadow_pathreturns, including system package paths like/usr/bin/jqif PATH order is unusual. This contradicts the earlier design note that the distrojqpackage should remain installed (see comment abovejq_install). It also creates a failure mode where the system copy is removed before the managed one is successfully installed. Consider refusing to remove commands from system directories and instead warning that PATH must prefer$BIN_DIR.
resolved=$(tool_shadow_path "$name")
[[ -n $resolved ]] || continue
log "$tool: $resolved shadows $BIN_DIR/$name, removing it"
if confirm " Remove $resolved?"; then
run_root rm -f "$resolved"
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Two review findings on #689: - tool_shadow_path used 'command -v', which answers for an alias or a shell function with no file behind it, and tool_unshadow passed that string straight to rm -f. 'type -P' searches PATH for a real file only, ignoring aliases, functions, builtins, and keywords. - tool_unshadow removed whatever tool_shadow_path returned, including a distro package's own file if PATH happened to put it ahead of $BIN_DIR. That contradicts jq_install's own design note that the distro jq package stays installed, and rm -f-ing a dpkg-owned file desyncs dpkg's database from the filesystem. A dpkg-owned shadow now gets a warning instead, naming the PATH order as the actual fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ptr727
commented
Aug 14, 2026
host-setup/linux/install-tools.sh:757 (suppressed finding, round 1, #689 (review)):
Fixed in 60eab56. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
host-setup/linux/install-tools.sh:783
tool_unshadowruns before the version/status check even in--installmode. If a newer shadowed copy exists on PATH and an older managed copy exists in$BIN_DIR, confirming removal can effectively downgrade the tool (and--yeswould do this unattended), because--installreturns early foroutdatedand won’t upgrade the managed copy.
Consider only unshadowing when upgrading, or when the managed copy doesn’t exist yet (so removal can’t expose an older version).
# Ahead of the version read below, since a shadowed tool's "command -v" answers for the shadow, not $BIN_DIR.
# That let a shadowed tool report "current" and skip its own install function entirely.
tool_unshadow "$tool"
host-setup/linux/install-tools.sh:647
- The doc comment for
tool_shadow_pathsays it returns a copy that resolves “ahead of $BIN_DIR/$1 on PATH”, but the implementation returns the current PATH resolution whenever it is not exactly$BIN_DIR/$1(even if$BIN_DIR/$1doesn’t exist or isn’t on PATH). Updating the comment to match the actual behavior would avoid confusion for future maintainers.
# The path of a copy of $1 that resolves ahead of $BIN_DIR/$1 on PATH, or empty when there is none.
# Shared by the report, which only names the shadow, and --install/--upgrade, which removes it.
# "type -P" skips aliases and shell functions, which "command -v" answers for with no file behind them.
Two more review findings on #689: - tool_unshadow ran ahead of the version read in every mode. Under --install with a managed $BIN_DIR copy already present, an outdated status returns early by design (--install leaves an installed tool at its version), so removing a newer shadow first, with nothing to replace it, silently downgraded what PATH resolved to afterward. apply_tool now only unshadows ahead of the version read under --upgrade (which brings $BIN_DIR current regardless) or when the managed copy does not exist yet (nothing to protect). - tool_shadow_path's doc comment said "resolves ahead of $BIN_DIR/$1 on PATH", which does not hold when $BIN_DIR/$1 does not exist yet. Reworded to state what the comparison actually is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ptr727
commented
Aug 14, 2026
Two suppressed findings from round 2 (#689 (review), on 60eab56): host-setup/linux/install-tools.sh:783 (
Fixed in e59d26b. host-setup/linux/install-tools.sh:647 (
Fixed in e59d26b. Reworded to "Where PATH currently resolves $1 to, when that is not $BIN_DIR/$1, or empty when it already is", which holds regardless of whether $BIN_DIR/$1 exists. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
host-setup/linux/install-tools.sh:770
tool_unshadowremoves at most the first PATH match for each tool name. If multiple shadowing copies exist earlier on PATH (e.g.,~/bin/uvand~/.local/bin/uv), this will remove only the first and still leave the tool shadowed afterward. Consider looping untiltool_shadow_pathis empty (or break on dpkg-owned paths / declined confirmation).
local name resolved
for name in "${names[@]}"; do
resolved=$(tool_shadow_path "$name")
[[ -n $resolved ]] || continue
# A distro package's own file, found only when PATH puts it ahead of $BIN_DIR, which this script does not set up.
# Removing it directly would desync dpkg's database from the filesystem, so it stays, and the fix is the PATH order.
if dpkg-query -S "$resolved" > /dev/null 2>&1; then
warn "$tool: $resolved belongs to a distro package and stays, put $BIN_DIR ahead of it on PATH instead"
continue
fi
log "$tool: $resolved shadows $BIN_DIR/$name, removing it"
if confirm " Remove $resolved?"; then
run_root rm -f "$resolved"
else
warn "$tool: left $resolved in place, it will keep shadowing $BIN_DIR/$name"
fi
done
host-setup/linux/install-tools.sh:785
- The PR description says
--installremoves a shadowing copy before version detection, but the implementation only unshadows under--installwhen the managed binary is missing (! -x "$BIN_DIR/$tool"). When a managed copy exists but PATH is still shadowed,--installcurrently does nothing and also emits no warning, so the run may still not converge (and can still mis-detect the installed version via PATH). Either update the description or warn explicitly in this case.
# Unshadowing first is safe only when nothing at $BIN_DIR could be made worse by it.
# --upgrade brings $BIN_DIR current regardless, and --install with nothing there yet has nothing to protect.
# --install with a managed copy already in place leaves it at its version by design, so removing a newer shadow first would downgrade what PATH resolves to.
if [[ $MODE == "upgrade" || ! -x "$BIN_DIR/$tool" ]]; then
tool_unshadow "$tool"
fi
Two more review findings on #689: - tool_unshadow removed at most one shadow per name. A tool with more than one copy earlier on PATH than $BIN_DIR (a ~/bin/uv and a ~/.local/bin/uv both ahead of it) still left the nearer one removed and the managed copy shadowed by the next. Now loops per name until tool_shadow_path answers empty, breaking on a dpkg-owned path, a declined confirm, or (to avoid spinning forever, since nothing is actually removed) one pass under --dry-run. - apply_tool's --install/--upgrade gate (added for the prior downgrade finding) left --install silent when a managed copy exists but is still shadowed: neither unshadowed nor warned, so a --report run right after named the same unfixed state with no explanation of why --install did not act. Logs an explicit note pointing at --upgrade instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ptr727
commented
Aug 14, 2026
Two suppressed findings from round 3 (#689 (review), on e59d26b), plus a note on the earlier two. host-setup/linux/install-tools.sh:770 (
Fixed in 2c00459. host-setup/linux/install-tools.sh:785 (the
Fixed in 2c00459 (code) and the PR description (docs). For the record, the two earlier-round findings on lines 757 and 783/647 remain answered as posted in the two comments above (#689 (comment) and #689 (comment)); they keep resurfacing in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
host-setup/linux/install-tools.sh:647
- The comment says "--install/--upgrade ... removes it", but
apply_tool()only unshadows in--installwhen the managed copy at$BIN_DIR/$tooldoes not exist yet. When the managed copy exists,--installexplicitly leaves the shadow in place (to avoid downgrading what PATH resolves to), so this comment is currently inaccurate/misleading.
# Where PATH currently resolves $1 to, when that is not $BIN_DIR/$1, or empty when it already is.
# Shared by the report, which only names the shadow, and --install/--upgrade, which removes it.
# "type -P" skips aliases and shell functions, which "command -v" answers for with no file behind them.
host-setup/linux/install-tools.sh:686
- This comment says a shadow reported here is "stale the moment"
--installruns, but--installdoes not always remove shadows (it leaves them when a managed copy already exists). Update the wording to match the actual unshadowing behavior so future readers aren't misled.
# A copy earlier on the PATH keeps answering after this script installs a newer one, which reads as an upgrade that did not take.
# --install/--upgrade removes it, so a report naming one here is stale the moment either runs.
Round 3's fix left both comments still saying --install/--upgrade unconditionally removes a shadow, stale the moment the mode-gate from the prior round shipped: --install only removes one when nothing managed exists yet at $BIN_DIR, and leaves an existing managed copy's shadow in place with a warning otherwise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ptr727
commented
Aug 14, 2026
Two suppressed findings from round 4 (#689 (review), on 2c00459), both stale prose from before round 3's host-setup/linux/install-tools.sh:647 (
Fixed in 05ecef6. Reworded to "and host-setup/linux/install-tools.sh:686 (
Fixed in 05ecef6. Reworded to "--upgrade removes it. --install removes it only when nothing managed exists yet, and warns instead when it leaves one in place." Both are comment-only fixes, no logic change, so shellcheck/prose_lint were re-run but no new functional test was needed for this round. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
host-setup/linux/install-tools.sh:775
tool_unshadowloops until the shadow disappears, but it doesn’t stop ifrm -ffails (e.g., filesystem is read-only or some other error). In that case,tool_shadow_pathwill keep returning the same path and the loop can repeatedly prompt/attempt forever. Consider breaking with a warning when the removal fails.
run_root rm -f "$resolved"
# Under --dry-run nothing is actually removed, so the same path would resolve again forever.
[[ $DRY_RUN == true ]] && break
done
host-setup/linux/install-tools.sh:652
tool_shadow_pathcan return a relative path if$PATHcontains relative entries (e.g..or./bin). Sincetool_unshadowlater passes this value torm -f(and--yescan make it non-interactive), it’s safer to only treat absolute paths as removable shadows.
This issue also appears on line 772 of the same file.
local name="$1" resolved
resolved=$(type -P "$name" 2> /dev/null || true)
[[ -n $resolved && $resolved != "$BIN_DIR/$name" ]] && printf '%s' "$resolved"
return 0
Two more review findings on #689: - tool_shadow_path could return a relative path (a '.' or './bin' entry earlier on PATH than $BIN_DIR), which tool_unshadow would then rm -f relative to whatever directory the operator happened to be in when invoking the script, not the shadow's real location. Now only an absolute path is ever treated as a shadow. - tool_unshadow's rm -f was a bare call inside the removal loop, so a real failure (a read-only filesystem) took the entire run down via set -e rather than leaving this one tool shadowed and moving on, consistent with how the rest of the script treats a single tool's failure. Verified empirically first: set -e already prevents the infinite loop the raised finding described (confirmed with a read-only-directory case, exit 1 immediately, no repeated prompt), but the hard, whole-run crash it causes instead is a real problem the finding's own suggested fix (warn and stop trying this shadow) still resolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ptr727
commented
Aug 14, 2026
Two suppressed findings from round 5 (#689 (review), on 05ecef6). host-setup/linux/install-tools.sh:775 (
Partially disproven, fix applied anyway, in 6fd292f. Checked empirically before trusting either the finding or a fix: host-setup/linux/install-tools.sh:652 (
Fixed in 6fd292f. |
Uh oh!
There was an error while loading. Please reload this page.
## Why A re-analysis of `docs/fleet-map.md`, applying the same gap-finding method that created it, after the P0-P4 closures, the #689/#701 install-model changes, and the #702 procedure-doc diagrams. ## What - **Diagram authority**: #702 put authoritative flow diagrams into STANDUP.md, RESYNC.md, AUDIT.md, and AGENTS.md. The map's own copies of the STANDUP and RESYNC flows were coarser duplicates with no cross-reference in either direction, and by the map's own rule the procedure doc wins. The two derivative diagrams are replaced with pointer sentences, and the Entry Points intro states the ownership rule. The five diagrams no procedure doc draws (System Map, Pre-Agent Cold Start, Daily Development, Hub-Side Operations, G3 remedy loop) stay. - **Maintenance rule extended**: it covered register rows but not drawn flows, which is exactly the class this batch fixes. It also cited G4 for register staleness, which is at best an analogy, so the sentence states the class directly. - **Stale content**: the vestigial `Checked` evidence-anchor sentence (the shipped register has no such cells, and the `ba392f9` pin is historical) is removed, the cold-start tool node points at `spec/host-tools.json` instead of an enumeration that drifted twice in two weeks (#689, #701), and the dead "dashed arrows mark a gap" clause (zero dashed arrows exist) is removed. - **Unmapped entry points surfaced**: OPERATIONS.md is named as the runnable form of the Hub-Side Operations door and of the daily-dev gates node, and `scripts/repo_gate.py`, `scripts/pr_review.py`, and `spec/audit.py` get reference links. - **Cross-links**: #699 and #700 join the Decision Ledger Cross-References per the #671-cluster precedent, and the #689 PATH self-heal is noted in the cold-start prose. - **Discoverability**: README links the map from the doors diagram it sits behind. AGENTS.md is deliberately untouched (byte-locked Fleet Bootstrap, and a carried file must not reference a hub-only doc). - `worktree` joins `cspell.json` words (used by the #699 cross-link). ## Flag, not a task `docs/fleet-map.md` and the #702 diagrams exist only on `develop` (main is 22 commits behind and lacks the file entirely). A `develop` to `main` promotion is a maintainer decision and is not part of this change. ## Verification All nine CI prose_lint checks pass tree-wide and `--diff develop` is clean, markdownlint reports 0 issues on both edited files, cspell is clean on README and leaves only the six pre-existing fleet-map findings (the file is not CI spell-gated), editorconfig-checker passes (CRLF preserved, diff is line-scoped), `spec/validate.py` and `jq` pass, all 6 remaining mermaid fences (5 in fleet-map, README doors) parse OK via mermaid@11, and a two-way reference-link check (every use defined, every relative target exists) passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…Python CI Gates (#718) Thirty-one squashes, `56f4d7d..d54862a`. 115 files, +20436/-5298. **Merge with a merge commit, never a squash, and never with `--delete-branch`.** This pull request's head is `develop` itself. ## What lands **Fleet Skills.** The `.agents/skills/` source tree, the generated `.claude-plugin/` distribution, `scripts/build_dist.py` with its `--check` gate, and `scripts/skills_install.py` with its host stamp (#676). Packaged as skills on top of the scaffold: PR review conduct and Copilot instructions upkeep (#677), comment and doc style (#678), resync-a-repo and fleet-conformance-check (#679), the per-language codestyles (#680), git commit conventions and operational vs release workflow (#681), stand up a repo (#683), and repo-worktree (#717). Coverage gaps closed in three passes (#690, #691, #692) plus the P4 sentence-length opt-in (#697). **Host setup.** The Windows host-setup tooling and its PowerShell gate (#674), the Windows bootstrap loader (#682), Docker install and upgrade on Linux and Windows with a version floor (#701, #705), a `uv` floor in `spec/host-tools.json` (#698), self-healing of a shadowing `uv`, `jq`, or `git-restore-mtime` copy (#689), node's real winget package id (#696), and a README for the Linux host-setup nuances (#710). **Python and CI.** Python tooling in CI with the script tests moved to `scripts/tests` (#704), `ruff format` adopted and gated (#709), and the PSScriptAnalyzer claim conditioned on repos that carry `.ps1` files (#686). **Conduct rules.** Triage-order and scope guardrails in pr-review-conduct (#684), `pr_review.py wait` requesting a review rather than only polling for one (#685), a tech-agnostic signed-commit verification (#708), execution rather than analogy to verify platform-specific code (#715), and a unique worktree for every task (#717). **Docs.** The fleet map and gap register with peer messaging declared (#687), mermaid flow diagrams in the kept-authority docs (#702), and the map pointed at the shipped diagrams and current tooling (#703). ## Issues this promotion closes Each landed on `develop` on its own pull request. The keyword fires only on a merge into `main`, so it sits here rather than on the feature pull requests. Closes#700Closes#707Closes#711Closes#712Closes#714Closes#688#699 stays open on purpose: #717 shipped the layout convention and the skill, and the physical migration of existing checkouts is still tracked there. ## Review record Every squash closed its own Copilot loop on its own pull request before merging to `develop`. This promotion carries no new content of its own, so its review is the merged tree as a whole. ## Consequence worth stating The `GOVERNANCE.md` and `AGENTS.md` sections these squashes changed become the canonical the moment this reaches `main`, and every carrying repository reads as drifted from that point until it resyncs. That is the ordinary consequence of a canonical moving rather than a defect. The Skills installer added here is also how a machine picks the new skills up, so a session that keeps restating a rule already packaged as a skill is the signal to run it.
Why
install-tools.shalready detected a copy ofuv,jq, orgit-restore-mtimeearlier onPATHthan$BIN_DIR(tool_note's report note), but nothing acted on it.--install/--upgradeonly ever touched$BIN_DIR, so a--reportrun right after still repeated the same note, unchanged.Worse:
apply_tool()reads a tool's version viacommand -v, so a shadow that already happened to be current made the tool report ascurrentand return before ever reaching its own install function. That is exactly howuvinstalled per #483's own documented method (curl -LsSf https://astral.sh/uv/install.sh | sh, into~/.local/bin, ahead of/usr/local/binon a stock DebianPATH) never converged on repeated--upgraderuns, per the repro in #688.What changed
Went with option (b) from the issue (detect and self-heal), since option (a) (ride the vendor's own install path, the pattern already used for
gh/node's apt repos) doesn't fituvthe way it fits those two: the astral installer's own path is a per-user~/.local/bin, where this script installs shared tools system-wide as root into/usr/local/bin.tool_shadow_path()factors the existing PATH-vs-$BIN_DIRcheck out oftool_note(), so the report and the fix below share one answer. Usestype -Prather thancommand -v, so an alias or shell function shadowing the name (no file behind it) is never mistaken for a shadow.tool_unshadow()acts on it, looping per tool name until nothing shadows$BIN_DIRany more (PATH can stack more than one copy ahead of it), prompting before each removal and respecting--yes/--dry-runthe same wayapt_install_displacing's confirm already does. A distro package's own file is never removed this way (dpkg-query -S), sincerm -f-ing it would desync dpkg's database from the filesystem, matchingjq_install's own design note that the distrojqpackage stays installed. Coversuv'suvxcompanion from the same archive too, since it would otherwise stay shadowed whileuvitself got fixed.apply_tool()only unshadows ahead of the version read under--upgrade(which brings$BIN_DIRcurrent regardless of what gets removed) or when the managed copy at$BIN_DIRdoes not exist yet (nothing there to protect).--installwith a managed copy already present leaves a shadow alone rather than risk removing a newer copy with nothing to replace it, and logs an explicit note pointing at--upgradeinstead of going silent.Fixes#688.
Verification
bash -n host-setup/linux/install-tools.shand--help/--liststill run.shellcheckclean via the pinnedkoalaman/shellcheck:stableimage.scripts/prose_lint.py --diff HEADclean.PATHstates and confirmed:tool_shadow_pathfinds a real shadow and ignores a same-named alias/function with no file behind it;tool_unshadowremoves a single shadow, removes two stacked shadows in one call, leaves a distro-package-owned path alone with a warning, leaves a declined path alone with a warning, and performs one simulated pass under--dry-runrather than looping forever; andapply_toolleaves a newer shadow untouched (with an explicit note) under--installwhen an older managed copy already exists, but removes it under--upgrade.🤖 Generated with Claude Code