Fail a Menu Fetch Whose Old Hub or Marker Cannot Be Changed, and Take Its Token Only Once Written

[ptr727]

Summary

Answers the Copilot finding on promotion PR #2545: "menu.sh can ignore marker-write failures and leave fetched trees behind". It also fixes that finding's siblings in the same fetch functions of both menus. Refs #1093.

• Marker write. menu.sh's fetch_hub_locked runs with errexit suspended (fetch_hub_locked || rc=$?), so a failed write to hub.owned let the clone go ahead unmarked. Cleanup then skipped that tree, and every later fetch refused it as a tree this run did not create. The write now fails the fetch before the clone. In menu.ps1, a Set-Content failure threw past the task. It now fails the task, the same way Lock menu.ps1's Hub Cache Through hub.lock So It Coordinates with menu.sh #2537 made a lock that cannot be taken fail it.
• Old-tree removal. A file held under the old tree made menu.ps1's Remove-Item throw past the task and end the menu session. In menu.sh it made the clone run into the surviving tree, and the clone-failure path then dropped the marker. Both now fail the fetch and leave the tree and its marker as they were, so the owning session's exit cleanup can still remove them.
• When the token becomes the session's. Each fetch builds its token in a local variable and assigns it to the session only once the marker holds it. Before this, a fetch that failed after generating its token replaced the session's own token, and the session's exit cleanup no longer matched the tree its earlier fetch had marked.

Verification (Windows host)

• New failure cases, one per path per menu: token read in bash, old-tree removal, and marker write. Each stubs or provokes the failure, then asserts the task fails, nothing is cloned, the marker is kept where it should be, and the session's token is unchanged. The menu.ps1 stubs shadow the cmdlet in the harness body, so each case reaches its catch.
• Mutation-checked against the code before each fix. The menus from develop fail both write cases. The removal guards' parent fails both removal cases. The early token assignment fails 4 cases.
• Full suite under Git Bash: 2275 tests, OK, 100 skipped. ruff, mypy, the prose gate and the EOL gate pass, and so do shellcheck, shfmt and PSScriptAnalyzer through scripts/docker_lint.py.
• Three local strict-review passes. The second raised one finding, the early token assignment, which is fixed in 31a69ad. The third raised none.

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: Repository: ptr727/ProjectTemplate/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: a5aa7eb9-dec7-4be2-93c5-7b4836e0c9b9

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.40%. Comparing base (9d8937b) to head (31a69ad).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2548   +/-   ##
========================================
  Coverage    58.40%   58.40%           
========================================
  Files           16       16           
  Lines         8090     8090           
========================================
  Hits          4725     4725           
  Misses        3365     3365           

Flag	Coverage Δ
python-3.13	58.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🔵 Needs a closer look

Two moderate findings remain unresolved in the hub-removal safeguards.

0 open findings

What changed in this PR

Hardens hub-fetch failure handling in Bash and PowerShell while preserving session token ownership.

Changes:

• Guards old-tree removal and marker writes.
• Delays token assignment until marker persistence succeeds.
• Adds regression tests for failure paths.

File	Summary
tests/​test_bootstrap.py	Adds coverage for removal, marker-write, and token-preservation failures.
host-setup/​menu.sh	Adds guarded removal and marker creation; one moderate finding remains regarding reported removal failures.
host-setup/​menu.ps1	Adds failure handling; one moderate finding remains regarding dangling hub links.

🧠 Review effort: Lite

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[ptr727]

Copilot's round on 31a69ad opened no thread, but its headline and file table named two moderate findings. That is the shape filed as #2544. Both are declined with evidence:

1. menu.sh, "reported removal failures": declined. The guard after rm -rf "$DIR/hub" does not trust rm's exit status. It checks the result, [[ -e "$DIR/hub" || -L "$DIR/hub" ]], which is the same pair remove_unowned_hub_check uses. A failure that leaves anything behind, a dangling link included, fails the fetch with Could not remove ..., and the tree and marker stay as they were. A non-zero rm that still removed everything leaves nothing to protect, so the fetch going ahead is correct. test_a_tree_that_cannot_be_removed_fails_the_fetch_and_keeps_its_marker covers the first case with rm stubbed to remove nothing.

2. menu.ps1, "dangling hub links": declined, disproved by running it. Test-Path -LiteralPath does not report a dangling link as absent, so the Remove-Item branch is reached. In mcr.microsoft.com/powershell:latest (pwsh 7.4.2, Linux, --network none), with hub a symlink to a missing target:

   Test-Path: True
   Get-Item: True
   Remove-Item: ok, exists after=False
   

   On Windows (pwsh 7, this host), a junction whose target was removed also gives Test-Path: True. Test-HubRemovable uses the same Test-Path, so it treats the link as present and requires the marker, matching menu.sh's -L.