From c5db386b757f5d392d2a436d294c64ff5a17b014 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 8 Oct 2026 08:50:52 -0700 Subject: [PATCH 1/3] Fail a Menu Fetch Whose hub.owned Marker Cannot Be Written, Before It Clones menu.sh's fetch runs with errexit suspended, so a failed marker write let the clone proceed unmarked, which cleanup then skips and every later fetch refuses as a tree this run did not create. menu.ps1's Set-Content failure threw past the task rather than failing it. Co-Authored-By: Claude Opus 5.5 --- host-setup/menu.ps1 | 7 ++++++- host-setup/menu.sh | 5 ++++- tests/test_bootstrap.py | 43 +++++++++++++++++++++++++++++++++++------ 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/host-setup/menu.ps1 b/host-setup/menu.ps1 index 0f0e1f475..a929be08e 100644 --- a/host-setup/menu.ps1 +++ b/host-setup/menu.ps1 @@ -260,7 +260,12 @@ function Invoke-FetchHubLocked { if (Test-Path -LiteralPath $hubPath) { Remove-Item -LiteralPath $hubPath -Recurse -Force } # Marked as ours before git can create anything under $hubPath, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. $script:HUB_FETCH_TOKEN = [guid]::NewGuid().ToString('N') - Set-Content -LiteralPath (Get-MarkerPath) -Value $script:HUB_FETCH_TOKEN + try { + Set-Content -LiteralPath (Get-MarkerPath) -Value $script:HUB_FETCH_TOKEN + } catch { + fail "Could not write $(Get-MarkerPath). Check that $script:DIR is writable: $($_.Exception.Message)" + return $false + } # A full clone of the default branch first, whatever -Ref names: spec\audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary. # Piped to Out-Host rather than left bare: an unassigned native call's stdout otherwise joins this function's own return value, which return $true/$false below would then be appended to instead of replacing. & git clone --quiet --branch $script:DEFAULT_REF --single-branch $script:HUB_URL $hubPath | Out-Host diff --git a/host-setup/menu.sh b/host-setup/menu.sh index d94696ffc..bfce4b36b 100755 --- a/host-setup/menu.sh +++ b/host-setup/menu.sh @@ -177,7 +177,10 @@ fetch_hub_locked() { remove_unowned_hub_check || return 1 rm -rf "$DIR/hub" # Marked as ours before git can create anything under $DIR/hub, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. - printf '%s\n' "$HUB_FETCH_TOKEN" >"$(marker_path)" + if ! printf '%s\n' "$HUB_FETCH_TOKEN" >"$(marker_path)"; then + fail "Could not write $(marker_path). Check that $DIR is writable." + return 1 + fi # A full clone of the default branch first, whatever $REF names: spec/audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary and misreport every repo as stale. git clone --quiet --branch "$DEFAULT_REF" --single-branch "$HUB_URL" "$DIR/hub" || { diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 0235b6970..c7233cb37 100755 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -1459,6 +1459,17 @@ def test_a_token_that_cannot_be_read_fails_the_fetch_before_it_changes_anything( self.assertTrue((self.dir / "hub" / "README.md").exists()) self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) + def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) -> None: + """A directory at the marker's name refuses the write, which errexit would not catch here.""" + (self.dir / "hub.owned").mkdir() + result = self.run_body( + 'git() { mkdir -p "${!#}"; }\nrc=0\nfetch_hub_locked || rc=$?\nprintf "rc=%s\\n" "$rc"\n' + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("rc=1", result.stdout) + self.assertIn("Could not write", result.stderr) + self.assertFalse((self.dir / "hub").exists()) + def run_body(self, body: str) -> subprocess.CompletedProcess[str]: script = self.scripts / "body.sh" script.write_text( @@ -1658,6 +1669,13 @@ class TestPowerShellMenuHubCleanup(HubCleanupCases, unittest.TestCase): """`menu.ps1`'s `Invoke-Cleanup`, under its real lock, its marker ending CRLF as on Windows.""" LINE_BREAK = "\r\n" + FETCH_SETUP = ( + "$script:HUB_REPO = 'owner/hub'\n$script:HUB_URL = 'https://example.invalid/hub'\n" + "$script:DEFAULT_REF = 'main'\n$script:REF = 'main'\n$script:HUB_ROOT = ''\n" + "function step { param([string]$Message) }\n" + "function git { New-Item -ItemType Directory -Path $args[-1] -Force | Out-Null; " + "$global:LASTEXITCODE = 0 }\n" + ) def setUp(self) -> None: self.dir = Path(self.enterContext(tempfile.TemporaryDirectory())) @@ -1674,14 +1692,27 @@ def fetch_body(self, refetched: bool) -> str: "$script:HUB_FETCH_TOKEN = $mine\n" ) return ( - "$script:HUB_REPO = 'owner/hub'\n$script:HUB_URL = 'https://example.invalid/hub'\n" - "$script:DEFAULT_REF = 'main'\n$script:REF = 'main'\n$script:HUB_ROOT = ''\n" - "function step { param([string]$Message) }\n" - "function git { New-Item -ItemType Directory -Path $args[-1] -Force | Out-Null; " - "$global:LASTEXITCODE = 0 }\n" - "[void](Invoke-FetchHubLocked)\n" + (again if refetched else "") + "Invoke-Cleanup\n" + self.FETCH_SETUP + + "[void](Invoke-FetchHubLocked)\n" + + (again if refetched else "") + + "Invoke-Cleanup\n" ) + def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) -> None: + """It fails the one task rather than ending the menu's whole session. + + The refusal is stubbed, since what refuses a write differs by platform and by account. + """ + result = self.run_body( + self.FETCH_SETUP + "function Set-Content { throw 'refused' }\n" + "$ok = Invoke-FetchHubLocked\n" + '[Console]::Out.WriteLine("ok=$ok")\n' + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip().splitlines()[-1], "ok=False") + self.assertIn("Could not write", result.stderr) + self.assertFalse((self.dir / "hub").exists()) + def run_body(self, body: str) -> subprocess.CompletedProcess[str]: body_file = self.scripts / "body.ps1" body_file.write_text(body, encoding="utf-8") From b8f2ba312c3bd64a4c7722cc4bd33d7291f57f9a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 8 Oct 2026 08:54:11 -0700 Subject: [PATCH 2/3] Fail a Menu Fetch Whose Old Hub Cannot Be Removed, Rather Than Cloning Into It or Ending the Session A held file under the old tree made menu.ps1's Remove-Item throw past the task and end the menu session, and made menu.sh clone into the surviving tree, whose clone-failure cleanup then dropped the marker and blocked every retry. Co-Authored-By: Claude Opus 5.5 --- host-setup/menu.ps1 | 9 ++++++++- host-setup/menu.sh | 4 ++++ tests/test_bootstrap.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/host-setup/menu.ps1 b/host-setup/menu.ps1 index a929be08e..eaf955cda 100644 --- a/host-setup/menu.ps1 +++ b/host-setup/menu.ps1 @@ -257,7 +257,14 @@ function Invoke-FetchHubLocked { step "Fetching $script:HUB_REPO at $script:REF" if (-not (Test-HubRemovable)) { return $false } $hubPath = Join-Path $script:DIR 'hub' - if (Test-Path -LiteralPath $hubPath) { Remove-Item -LiteralPath $hubPath -Recurse -Force } + if (Test-Path -LiteralPath $hubPath) { + try { + Remove-Item -LiteralPath $hubPath -Recurse -Force + } catch { + fail "Could not remove $hubPath to fetch it again. Check that nothing holds a file under it: $($_.Exception.Message)" + return $false + } + } # Marked as ours before git can create anything under $hubPath, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. $script:HUB_FETCH_TOKEN = [guid]::NewGuid().ToString('N') try { diff --git a/host-setup/menu.sh b/host-setup/menu.sh index bfce4b36b..fe009dfbf 100755 --- a/host-setup/menu.sh +++ b/host-setup/menu.sh @@ -176,6 +176,10 @@ fetch_hub_locked() { fi remove_unowned_hub_check || return 1 rm -rf "$DIR/hub" + if [[ -e "$DIR/hub" || -L "$DIR/hub" ]]; then + fail "Could not remove $DIR/hub to fetch it again. Check that nothing holds a file under it." + return 1 + fi # Marked as ours before git can create anything under $DIR/hub, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. if ! printf '%s\n' "$HUB_FETCH_TOKEN" >"$(marker_path)"; then fail "Could not write $(marker_path). Check that $DIR is writable." diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index c7233cb37..7be2fe1b9 100755 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -1459,6 +1459,18 @@ def test_a_token_that_cannot_be_read_fails_the_fetch_before_it_changes_anything( self.assertTrue((self.dir / "hub" / "README.md").exists()) self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) + def test_a_tree_that_cannot_be_removed_fails_the_fetch_and_keeps_its_marker(self) -> None: + """A clone into the surviving tree would fail, and its cleanup would then drop the marker.""" + self.make_cache(self.TOKEN) + result = self.run_body( + 'git() { mkdir -p "${!#}"; }\nrm() { return 0; }\n' + 'rc=0\nfetch_hub_locked || rc=$?\nprintf "rc=%s\\n" "$rc"\n' + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("rc=1", result.stdout) + self.assertIn("Could not remove", result.stderr) + self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) + def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) -> None: """A directory at the marker's name refuses the write, which errexit would not catch here.""" (self.dir / "hub.owned").mkdir() @@ -1698,6 +1710,22 @@ def fetch_body(self, refetched: bool) -> str: + "Invoke-Cleanup\n" ) + def test_a_tree_that_cannot_be_removed_fails_the_fetch_and_keeps_its_marker(self) -> None: + """It fails the one task rather than ending the menu's whole session. + + The refusal is stubbed, since what holds a file open differs by platform. + """ + self.make_cache(self.TOKEN) + result = self.run_body( + self.FETCH_SETUP + "function Remove-Item { throw 'held' }\n" + "$ok = Invoke-FetchHubLocked\n" + '[Console]::Out.WriteLine("ok=$ok")\n' + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip().splitlines()[-1], "ok=False") + self.assertIn("Could not remove", result.stderr) + self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) + def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) -> None: """It fails the one task rather than ending the menu's whole session. From 31a69adf0e3dd621cee91f3fa8229d68714df9c2 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 8 Oct 2026 08:57:39 -0700 Subject: [PATCH 3/3] Take a Fetch's Token as the Session's Only Once the Marker Holds It A fetch that failed after generating its token replaced the session's own, so the session's exit cleanup no longer matched the tree its earlier fetch marked and left it behind. Co-Authored-By: Claude Opus 5.5 --- host-setup/menu.ps1 | 5 +++-- host-setup/menu.sh | 6 ++++-- tests/test_bootstrap.py | 31 +++++++++++++++++++------------ 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/host-setup/menu.ps1 b/host-setup/menu.ps1 index eaf955cda..57e61fdf8 100644 --- a/host-setup/menu.ps1 +++ b/host-setup/menu.ps1 @@ -266,13 +266,14 @@ function Invoke-FetchHubLocked { } } # Marked as ours before git can create anything under $hubPath, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. - $script:HUB_FETCH_TOKEN = [guid]::NewGuid().ToString('N') + $token = [guid]::NewGuid().ToString('N') try { - Set-Content -LiteralPath (Get-MarkerPath) -Value $script:HUB_FETCH_TOKEN + Set-Content -LiteralPath (Get-MarkerPath) -Value $token } catch { fail "Could not write $(Get-MarkerPath). Check that $script:DIR is writable: $($_.Exception.Message)" return $false } + $script:HUB_FETCH_TOKEN = $token # A full clone of the default branch first, whatever -Ref names: spec\audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary. # Piped to Out-Host rather than left bare: an unassigned native call's stdout otherwise joins this function's own return value, which return $true/$false below would then be appended to instead of replacing. & git clone --quiet --branch $script:DEFAULT_REF --single-branch $script:HUB_URL $hubPath | Out-Host diff --git a/host-setup/menu.sh b/host-setup/menu.sh index fe009dfbf..4961a8f2a 100755 --- a/host-setup/menu.sh +++ b/host-setup/menu.sh @@ -170,7 +170,8 @@ fetch_hub() { fetch_hub_locked() { step "Fetching $HUB_REPO at $REF" - if ! HUB_FETCH_TOKEN=$(od -An -N16 -tx1 /dev/urandom | tr -d ' \n') || [[ -z $HUB_FETCH_TOKEN ]]; then + local token + if ! token=$(od -An -N16 -tx1 /dev/urandom | tr -d ' \n') || [[ -z $token ]]; then fail "Could not read /dev/urandom for the token marking this fetch as this session's." return 1 fi @@ -181,10 +182,11 @@ fetch_hub_locked() { return 1 fi # Marked as ours before git can create anything under $DIR/hub, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. - if ! printf '%s\n' "$HUB_FETCH_TOKEN" >"$(marker_path)"; then + if ! printf '%s\n' "$token" >"$(marker_path)"; then fail "Could not write $(marker_path). Check that $DIR is writable." return 1 fi + HUB_FETCH_TOKEN="$token" # A full clone of the default branch first, whatever $REF names: spec/audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary and misreport every repo as stale. git clone --quiet --branch "$DEFAULT_REF" --single-branch "$HUB_URL" "$DIR/hub" || { diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 7be2fe1b9..f581593f1 100755 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -1451,10 +1451,11 @@ def test_a_token_that_cannot_be_read_fails_the_fetch_before_it_changes_anything( self.make_cache(self.TOKEN) result = self.run_body( 'git() { mkdir -p "${!#}"; }\nod() { return 1; }\n' - 'rc=0\nfetch_hub_locked || rc=$?\nprintf "rc=%s\\n" "$rc"\n' + f"HUB_FETCH_TOKEN={self.TOKEN}\nrc=0\nfetch_hub_locked || rc=$?\n" + 'printf "rc=%s token=%s\\n" "$rc" "$HUB_FETCH_TOKEN"\n' ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("rc=1", result.stdout) + self.assertIn(f"rc=1 token={self.TOKEN}", result.stdout) self.assertIn("Could not read /dev/urandom", result.stderr) self.assertTrue((self.dir / "hub" / "README.md").exists()) self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) @@ -1464,10 +1465,11 @@ def test_a_tree_that_cannot_be_removed_fails_the_fetch_and_keeps_its_marker(self self.make_cache(self.TOKEN) result = self.run_body( 'git() { mkdir -p "${!#}"; }\nrm() { return 0; }\n' - 'rc=0\nfetch_hub_locked || rc=$?\nprintf "rc=%s\\n" "$rc"\n' + f"HUB_FETCH_TOKEN={self.TOKEN}\nrc=0\nfetch_hub_locked || rc=$?\n" + 'printf "rc=%s token=%s\\n" "$rc" "$HUB_FETCH_TOKEN"\n' ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("rc=1", result.stdout) + self.assertIn(f"rc=1 token={self.TOKEN}", result.stdout) self.assertIn("Could not remove", result.stderr) self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) @@ -1475,10 +1477,11 @@ def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) - """A directory at the marker's name refuses the write, which errexit would not catch here.""" (self.dir / "hub.owned").mkdir() result = self.run_body( - 'git() { mkdir -p "${!#}"; }\nrc=0\nfetch_hub_locked || rc=$?\nprintf "rc=%s\\n" "$rc"\n' + f'git() {{ mkdir -p "${{!#}}"; }}\nHUB_FETCH_TOKEN={self.TOKEN}\nrc=0\n' + 'fetch_hub_locked || rc=$?\nprintf "rc=%s token=%s\\n" "$rc" "$HUB_FETCH_TOKEN"\n' ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("rc=1", result.stdout) + self.assertIn(f"rc=1 token={self.TOKEN}", result.stdout) self.assertIn("Could not write", result.stderr) self.assertFalse((self.dir / "hub").exists()) @@ -1717,12 +1720,14 @@ def test_a_tree_that_cannot_be_removed_fails_the_fetch_and_keeps_its_marker(self """ self.make_cache(self.TOKEN) result = self.run_body( - self.FETCH_SETUP + "function Remove-Item { throw 'held' }\n" + self.FETCH_SETUP + + f"$script:HUB_FETCH_TOKEN = '{self.TOKEN}'\n" + + "function Remove-Item { throw 'held' }\n" "$ok = Invoke-FetchHubLocked\n" - '[Console]::Out.WriteLine("ok=$ok")\n' + '[Console]::Out.WriteLine("ok=$ok token=$script:HUB_FETCH_TOKEN")\n' ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(result.stdout.strip().splitlines()[-1], "ok=False") + self.assertEqual(result.stdout.strip().splitlines()[-1], f"ok=False token={self.TOKEN}") self.assertIn("Could not remove", result.stderr) self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) @@ -1732,12 +1737,14 @@ def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) - The refusal is stubbed, since what refuses a write differs by platform and by account. """ result = self.run_body( - self.FETCH_SETUP + "function Set-Content { throw 'refused' }\n" + self.FETCH_SETUP + + f"$script:HUB_FETCH_TOKEN = '{self.TOKEN}'\n" + + "function Set-Content { throw 'refused' }\n" "$ok = Invoke-FetchHubLocked\n" - '[Console]::Out.WriteLine("ok=$ok")\n' + '[Console]::Out.WriteLine("ok=$ok token=$script:HUB_FETCH_TOKEN")\n' ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(result.stdout.strip().splitlines()[-1], "ok=False") + self.assertEqual(result.stdout.strip().splitlines()[-1], f"ok=False token={self.TOKEN}") self.assertIn("Could not write", result.stderr) self.assertFalse((self.dir / "hub").exists())