Repository navigation
Conversation
…t Up as a Hand Edit (#2493) ## Summary When the installer left `CLAUDE.md` holding bytes it wrote itself, the next run read that file as a hand edit. It backed the file up as "the edited prior file", and `--report` said it "was edited since the last install". Issue #2454 names two states that reach this, and this change closes both. - **A file this run created.** `write_regular_file` opens exclusively first, so it can tell a file it created from one it found. A path that is a link always counts as found. When the write fails, the created file is removed, but only where the path still holds the same device and inode. The path is then left absent, as it was found. - **A write cut short on a file that existed.** `record_leftover_instructions` reads the leftover back. Where it is a byte prefix of the render or of the prior content this run wrote, the leftover's digest goes into a stamp that passes its shape check, together with `instructionsLeftover: true`. The function then reads the stamp back, and the error carries a note only where the stamp does not hold that digest. - **Report.** `--report` names a stamped leftover as what an install left when its write failed, separately from an earlier render and from a hand edit. A completed install writes a fresh stamp, which drops the flag. Part of #2454. ## Verification - `python3 -m unittest test_install`: 150 tests, OK, on Linux. - Issue #2454's reproduction runs as a test under a real 4096-byte `RLIMIT_FSIZE`. The report no longer says the file was edited, and the re-run writes no backup. - Each new test was run against the code it guards with that code reverted or mutated, and failed there. - `ruff`, `prose_lint.py --diff` (sentence-length included) and `repo_gate.py --check eol` are clean. - `local-strict-review`: four passes. The fourth is recorded, and it raised the findings listed below. ## Local Review Findings and Their Dispositions The first push's recorded pass raised 7 findings. Copilot's first round raised 6, which overlapped with them. - **Fixed in cece0cc, fa5e822 and cdd698e:** - An interrupt is re-raised as is, in the cleanup and in the failed-close conversion. - A created file that is removed, or is already gone, is reported as no longer existing, with the write's own cause. - `IncompleteWrite`'s docstring covers every raise site. - A removal that fails after a write left incomplete names both failures. - A test covers a leftover of the prior content alone. - The swap and removal tests act once the descriptor is closed. - **Kept by decision:** the `lstat`-then-`unlink` cleanup is best effort, and its docstring says it narrows the window rather than closing it. - **Filed:** - #2494: the Windows-native check that `fstat` and `lstat` agree on file identity. - #2499: four wording findings from the last recorded pass. - #2498: a pre-existing stage-order gap with the same symptom. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
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 #2502 +/- ##
=======================================
Coverage 58.50% 58.50%
=======================================
Files 16 16
Lines 8066 8066
=======================================
Hits 4719 4719
Misses 3347 3347
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.
🟡 Changes recommended
Unresolved moderate findings remain in the installer implementation.
4 open findings
What changed in this PR
Promotes the installer update that recognizes and recovers from its own partial CLAUDE.md writes.
Changes:
- Tracks installer-owned partial writes and cleans up failed creations.
- Records leftover output and improves reporting.
- Adds regression coverage for recovery and failure handling.
| File | Summary | Findings |
|---|---|---|
host-setup/agent-safety/claude/install.py |
Implements cleanup, ownership tracking, stamping, and reporting. | 4 moderate findings and 1 nit remain unresolved. |
host-setup/agent-safety/claude/test_install.py |
Adds tests for partial writes, cleanup, and error handling. | 1 nit remains unresolved. |
🧠 Review effort: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ree's Name (#2503) ## Summary Two bootstrap runs sharing a `--dir` used the same fixed tree names with nothing locking them, and a removal that failed on a tree's final directory left a leftover every later run refused. This change locks the directory for the length of a run and treats that leftover as removable, in both loaders, to the design settled on #1792 and #1795 for the `windows` and `wsl` lanes together. - **The lock (#1792):** each run holds an OS lock on `$DIR/<tree>.lock` for its whole length. - `bootstrap.sh` takes `flock -n` on it, and `bootstrap.ps1` opens it with `FileShare.None`, which .NET implements as a `flock` on Linux, so the two loaders exclude each other there. - Each loader takes it before anything that removes a tree: before `bootstrap.sh` sets its exit trap, and before the `try` whose `finally` runs `bootstrap.ps1`'s cleanup. A refused run therefore never reaches the cleanup that would delete the holder's trees. - A run that dies releases the lock with its process, so no stale lock is left. `run_tool` closes the lock's descriptor for each tool, so a daemon a tool starts cannot inherit it. - A held lock refuses rather than waits. `bootstrap.sh` now checks for `flock` beside `curl` and `tar`, and the docs name it. - **The leftover (#1795):** an empty, non-link directory at a managed name now counts as the loader's own, since it holds nothing to lose and no live run can own it while the lock is held. A directory that cannot be listed or read stays somebody else's in both loaders. - **Tests:** both loaders' kept-tree classes gain cases for the lock refusing a second run, the lock freeing when its holder ends, the empty leftover at the staging and the tree name, and an unlistable directory. A cross-loader case holds a Python `flock` and checks pwsh's lock refuses, on Linux. A platform-independent class asserts each `main` takes the lock before its cleanup is armed. Closes #1792 and #1795 once promoted. ## Verification - **Windows 11, pwsh 7, native:** `python3 -m unittest tests.test_bootstrap` runs 72 tests, OK, 29 skipped (the Linux-only cases). - **WSL Ubuntu:** `TestKeptTreeHandling` and `TestDirectoryLockOrder` pass. WSL's git cannot read a Windows worktree's `.git` pointer, so `TestScriptPresence` fails there on that alone. - **Cross-loader exclusion:** probed in a Linux `mcr.microsoft.com/powershell` container in both directions. While pwsh holds `FileShare.None`, `flock -n` exits 1. While `flock` holds the file, pwsh's open throws `IOException`. Each succeeds once the other releases. Linux CI runs the cross-loader test. - **Mutation checks:** each new test fails against the defect it names. The checks covered the PowerShell share mode, the empty-directory check, the lock and `try` order, the unlistable-directory guard, bash's `flock -n`, the descriptor close, the bash empty and unlistable checks, and the bash lock and trap order. - **Linters:** `ruff`, `mypy`, `pyright`, shellcheck, shfmt, PSScriptAnalyzer and markdownlint are clean. The prose gate is clean with the `comments` label, which this PR carries for the new loader comments. - **`local-strict-review`, three rounds:** - Round 1 raised four findings: an unlistable directory taken for empty in bash, the same throwing out of the PowerShell cleanup, two assertions that could not fail, and lock messages naming the wrong cause. All four are fixed in 6d9c6a9. - Round 2 raised two. The marker check could still throw on Linux pwsh, and a plain `IOException` was claimed to mean contention only. Both are fixed in efa90ec. - Round 3 found nothing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 7, 2026
Merge a Reported Promotion With --match-head-commit Set to the Reported Head, Not a Fresh Read
#2505
Closed
ptr727
added a commit
that referenced
this pull request
Oct 8, 2026
## Summary Implements #2505. `merge-and-release` "The Procedure": - **Step 3** passes `--match-head-commit` the head the go-ahead covers, rather than always the live read. Attended, that is the head `drive-pr`'s ready report named, or the moved head the maintainer approved after step 2's re-ask. Under an `unattended-handoff` grant, or where no report named a head, it is the head step 2 recorded. The server then refuses a merge whose head moved at any point after the go-ahead, which closes the window #2502 hit, where reading the live head and merging in one command skipped step 2's comparison. - **Step 2** states that the reported-head comparison is skipped under an `unattended-handoff` grant even where the worker holds a ready report, rather than asserting that no report exists. - **"Stop and Report"** words a refusal as the head moving past the SHA it was given, which holds for each of step 3's sources. ## Verification - `scripts/build_dist.py --check` and the prose gate pass, and the hook gates ran on each commit. - Three `local-strict-review` passes ran. The first two each found one path that the guard broke (an unattended worker refused forever, and an attended re-ask approval that could never merge), and each was fixed. The third pass checked every path and found nothing. Refs #2505, which closes on the promotion PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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 two changes from
develop:CLAUDE.mdrather than backing it up as a hand edit.CLAUDE.mdwrite stamps what it left, when those bytes are a prefix of what the run wrote. The next run then replaces the file without a backup, and--reportnames it as a failed write rather than an edit.--dirfor its whole length, and both loaders take the same lock.bootstrap.shtakesflockon a.lockfile named for its tree, andbootstrap.ps1opens it withFileShare.None, which .NET implements as aflockon Linux. A second run against the same directory stops rather than deleting a tree the first is still extracting.Closes #2454.
Closes #1792.
Closes #1795.
Verification
test_install.pypassed, 155 tests, on Linux, and every check on Recognize the Installer's Own Partial CLAUDE.md Rather Than Backing It Up as a Hand Edit #2493 passed.🤖 Generated with Claude Code