fix(update): scope bootstrap-cache refresh to the target ref + match installer pin rules (follow-up to #82229) - #82247
Merged
Conversation
…match installer pin rules Two cache-key correctness follow-ups to #82229 (review feedback): 1. Abbreviated commit pins are immutable too. The installer's is_valid_commit() accepts 7-40 hex chars, but the Python refresh exempted only exactly-40-hex names — an abbreviated pin like install-4ce1994.ps1 could be overwritten with a branch script. The predicate now mirrors the Rust rule (7-40 hex = immutable, never rewritten), applied to the sanitized target ref. 2. Refresh only the update-target ref's cache key. The helper rewrote EVERY mutable-ref entry with the active checkout's script: with install-main.ps1 and install-bb_gui.ps1 coexisting, updating main replaced both with main's script — cross-branch cache poisoning in the other direction. It now computes the single cache key for the branch being updated, using the installer's own ref sanitization (sanitize_ref: non [A-Za-z0-9._-] -> '_', so bb/gui -> install-bb_gui.ps1), and touches nothing else. Entries the bootstrapper never wrote are not created. The branch is threaded from the existing `branch = _resolve_update_branch(args)` in both _cmd_update_impl call sites and _update_via_zip (main-only by its own guard). Regression tests lock down both invariants: abbreviated-SHA pin untouched (including when passed as the branch), coexisting mutable refs (main refresh leaves install-bb_gui.ps1 byte-identical), sanitize_ref parity, and uncached-ref no-op. E2E on the incident machine's real bootstrap-cache: planted a stale install-main.ps1 + sibling install-bb_gui.ps1 + abbreviated pin install-4ce1994.ps1; refresh("main") healed main byte-exact and left both others untouched; refresh("4ce1994") was a no-op. The pre-existing 40-hex pin entry in the real cache was also untouched.
Contributor
૮ >ﻌ< ა ci reviewran on 57a0bb1
|
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.
What does this PR do?
Implements both cache-key follow-ups from the review feedback on #82229:
1. Abbreviated commit pins are immutable too
install_script.rs::is_valid_commit()treats 7–40 hex chars as an immutable commit pin, but_refresh_bootstrap_cache_scripts()exempted only exactly-40-hex names — an abbreviated pin likeinstall-4ce1994.ps1could be overwritten with a branch script. The Python predicate now mirrors the Rust rule: the sanitized target ref matching[0-9a-fA-F]{7,40}is never rewritten.2. Refresh only the update-target ref's cache key
The helper rewrote every mutable-ref entry with the active checkout's script: with
install-main.ps1andinstall-bb_gui.ps1coexisting, updating main replaced both with main's script — cross-branch cache poisoning in the other direction. It now computes the single cache key for the branch being updated, using the installer's own ref sanitization (sanitize_ref: non-[A-Za-z0-9._-]→_, sobb/gui→install-bb_gui.ps1), and touches nothing else. Refs the bootstrapper never cached are not created.The branch is threaded from the existing
branch = _resolve_update_branch(args)at all three call sites (both_cmd_update_implcompletion paths +_update_via_zip, which is main-only by its own guard).Type of Change
How to Test
pytest tests/hermes_cli/test_update_bootstrap_cache_refresh.py -o "addopts=--timeout-method=thread"— 12 passed, including the two requested regressions:test_abbreviated_commit_pin_left_alone— 7-hex pin untouched, even when passed as the branchtest_only_target_branch_key_is_refreshed— coexistinginstall-main.ps1+install-bb_gui.ps1: main refresh heals main, leaves bb_gui byte-identicaltest_branch_ref_is_sanitized_like_the_installer(bb/gui→install-bb_gui.ps1) andtest_uncached_branch_is_noopE2E verification (real incident machine's bootstrap-cache)
Planted stale
install-main.ps1+ siblinginstall-bb_gui.ps1+ abbreviated pininstall-4ce1994.ps1in the real%LOCALAPPDATA%\hermes\bootstrap-cache:refresh("main")healed main byte-exact (BOM intact) and left both others untouched;refresh("4ce1994")was a no-op. The pre-existing real 40-hex pin entry was untouched throughout.Rollout note (from the same review)
Acknowledged and inherent: the first update that pulls this change still runs pre-change bytecode, so existing installs need one subsequent
hermes updatebefore the refresh executes — and this mitigates stale scripts only, not the stagedhermes-setup.exebinary itself. Refreshing the staged binary is a separate follow-up if wanted.