Repository navigation
Conversation
…It Fetched (#2542) ## Summary Refs #1093. `menu.ps1` and `menu.sh` share a hub cache, `$DIR/hub`, marked by `$DIR/hub.owned`. A session's exit cleanup removed that tree whenever its own process-local `HUB_FETCHED` was true and the marker existed. A session that fetched earlier could therefore delete a tree another session sharing the directory fetched since, a `-Keep` / `--keep` session's included. - Each fetch now writes a token unique to that fetch into the marker: a GUID in `menu.ps1` and 16 bytes of `/dev/urandom` as hex in `menu.sh`. - The exit cleanup, once it holds the hub lock, removes the tree only when the marker still holds this session's own token (`Test-HubFetchedHere` / `hub_fetched_here`). A session that has no token, or finds an empty marker, removes nothing. - The issue says `menu.sh` lacks this shape. It does not: it has the same marker and the same cleanup gate, and both menus can share one `--dir`, so both are fixed here. ## Known limit An older copy of either menu, one fetched before this lands, still deletes whenever any marker exists. Until it is replaced, it can remove a tree a new session fetched. Nothing here can close that, since an old copy never reads the token. ## Verification (Windows host) - New `TestMenuHubCleanup` (bash, run under Git Bash with `flock` stubbed) and `TestPowerShellMenuHubCleanup` (pwsh, under the real lock) share one set of cases. Two of them run a real fetch with `git` stubbed: a session's own fetch is removed at exit, and a fetch by another session made since survives the first session's exit. - Mutation-checked in both menus. Disabling the token gate fails 3 cases per menu, and writing an empty marker fails the own-fetch case per menu. - Full suite under Git Bash: 2270 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`. - Two local strict-review passes. The first raised one finding, a test name claiming a cross-menu match that cannot occur, which is fixed in e921ba5. The second raised none. - Not run on Linux locally. CI's Linux run covers `menu.sh` under real bash and `menu.ps1` under Linux pwsh. The review also found one pre-existing defect, filed as #2541: the freshness check ignores `--ref`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2545 +/- ##
=======================================
Coverage 58.40% 58.40%
=======================================
Files 16 16
Lines 8090 8090
=======================================
Hits 4725 4725
Misses 3365 3365
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:
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
menu.sh can ignore marker-write failures and leave fetched trees behind.
0 open findings
What changed in this PR
Promotes the per-fetch hub ownership token changes from develop to main, preventing stale sessions from deleting another session’s cache.
Changes:
- Adds ownership tokens to Bash and PowerShell hub cleanup.
- Adds cross-menu cleanup and failure-handling tests.
menu.shmust still handle marker-write failures before proceeding.
| File | Summary |
|---|---|
tests/test_bootstrap.py |
Adds cross-menu cleanup coverage. |
host-setup/menu.sh |
Adds token generation and guarded cleanup; marker-write failure handling remains unresolved. |
host-setup/menu.ps1 |
Adds GUID-based ownership validation and guarded cleanup. |
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
This was referenced Oct 8, 2026
Owner
Author
|
Copilot's round on 9d8937b opened no thread, but its headline named one finding: |
… Its Token Only Once Written (#2548) ## 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 #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](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues were identified.
0 open findings
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
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
Promotes one change from
develop.menu.ps1andmenu.sheach write a token unique to the fetch into the sharedhub.ownedmarker. The exit cleanup, holding the hub lock, removes the cached hub only when the marker still holds the session's own token. A session that fetched earlier no longer deletes a tree another session sharing the cache directory fetched since, a-Keep/--keepsession's included. Inmenu.shthe token is read before the fetch changes anything, and a failed read fails the fetch, since errexit is suspended there.Closes #1093.
Verification
TestMenuHubCleanup(bash) andTestPowerShellMenuHubCleanup(pwsh, real lock) cases pass on Windows, and the full suite passes under Git Bash (2270 tests, 100 skipped). Mutation checks fail the new cases in both menus. CI covers the Linux side.menu.shfindings. The token-read failure was fixed in 912b557, and cross-menu CRLF was declined with evidence, since a session only ever compares the marker with its own token. The reader gap that let the digest pass that headline is filed as pr_review.py Passes a Copilot Round Whose Headline Names Findings Beside a Zero Open-Findings Count #2544.--ref.🤖 Generated with Claude Code