fix(install): close the exclude helper's two residual holes (#374, #375) - #376
Conversation
Follow-up to #359, which guarded the filesystem tail but left two holes the review surfaced. Both live in `_worktree_local_exclude` and both falsify the same best-effort contract, so they ship together. #374 — the git-query arm decoded stdout with `text=True`, errors=strict. POSIX filenames are bytes, so a repo path carrying bytes invalid in the locale encoding raised UnicodeDecodeError out of the one arm whose entire contract is "skip silently", and that type is in neither arm's tuple. Stdout is now captured as bytes and decoded with `os.fsdecode` inside the guarded tail: such a path round-trips back to the filesystem instead of faulting, and the decode sits in the tail because fsdecode CAN still raise on Windows (utf-8/surrogatepass rejects a lone invalid byte). #375 — the update is a read-modify-REWRITE and `write_text` opens "w", truncating before it writes, so a short write left the operator's own excludes cut mid-content while the reason still said "could not update". The surviving tail parses as a valid git pattern and is a prefix of the intended one: cut one character in and the last line is `/`, which excludes the entire worktree — the exact state worktree_flow guards against for hookless profiles. Blast radius is the MAIN repo, since a linked worktree's --git-common-dir points at the main .git. Now written to a scratch file and moved into place via atomic_replace, with the scratch file removed on the failure path. Ablations run for both, and one test was REPLACED after failing its own: a version that monkeypatched `subprocess.run` passed identically with `text=True` restored, because handing the code bytes directly never runs the stdlib's decoding. Driving a real stub `git` on PATH does fail the ablation. POSIX-only and skipped on Windows, correctly — Windows paths are UTF-16, so git cannot emit an undecodable one. The #375 test injects at `Path.open`, not `Path.write_text`: patching write_text means the file is never opened, so the truncation cannot happen and the test would pass against the very bug it exists to catch. That is also why the two pre-existing write-fault tests needed their name filter widened — the write now lands on the scratch file.
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe worktree-local exclude helper now decodes Git output using filesystem-safe handling and updates exclude files through temporary-file replacement. Tests cover undecodable paths, partial writes, cleanup, and provisioning degradation reporting. ChangesWorktree exclude handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR closes two holes left in
Confidence Score: 5/5Safe to merge — both fixes are narrowly scoped to the one helper they close holes in, and the exception-handling contract of the two-arm function is preserved exactly. The bytes-capture change moves os.fsdecode to the only place where a decode failure is already treated as a degrade. The atomic-write change is a drop-in via an existing, well-tested platform helper that handles temp-name uniqueness, fsync, mode preservation, and cleanup on failure. The two new regression tests each document their own ablation outcome. No unrelated paths are touched. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/install.py | Captures subprocess stdout as bytes and decodes with os.fsdecode inside the guarded tail; replaces write_text with atomic_write_text for the exclude rewrite. Both changes are correctly placed and exception handling is consistent with the existing two-arm contract. |
| tests/test_install.py | Two pre-existing write-fault tests correctly re-patched at the module-level atomic_write_text binding. Two new regression tests added: a real stub-git-on-PATH approach for #374 and an io.open-level ShortWriter for #375, both with documented ablation proofs. |
| CHANGELOG.md | Two new entries prepended to the current release block, accurately describing the short-write atomicity fix and the non-UTF-8 path decode fix. |
Reviews (3): Last reviewed commit: "docs(install): stop the exclude write co..." | Re-trigger Greptile
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/bmad_loop/install.py (1)
790-805: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a process-scoped scratch name for shared
.git/info/exclude.Linked worktickets share
--git-common-dir, and the current fixed name depends on--git-common-dir, which can race if multiple story runs provision worktrees against the same repo concurrently, causing partial or replaced exclude writes.🔒 Suggested fix: give the scratch file a unique name
- tmp = exclude.with_name(f"{exclude.name}.bmad-loop.tmp") + tmp = exclude.with_name(f"{exclude.name}.bmad-loop.{os.getpid()}.{uuid.uuid4().hex}.tmp")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/install.py` around lines 790 - 805, Update the temporary path creation in the exclude update flow around atomic_replace to use a process-scoped unique scratch filename rather than the fixed “.bmad-loop.tmp” name. Preserve the existing write-then-atomic-replace behavior and cleanup on failure, while ensuring concurrent story runs sharing the same git common directory cannot reuse or overwrite the same scratch file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/install.py`:
- Around line 765-769: Update the git invocation in the surrounding install flow
to use verify.py’s _run_git wrapper, extending it with a raw-bytes option/helper
if the caller must retain bytes. Preserve the existing command, check-on-failure
behavior, and bytes-based processing; only keep the direct subprocess.run call
if you document a concise, explicit rationale for the exception.
In `@tests/test_install.py`:
- Around line 1645-1651: Update the git stub setup in the test to emit a path
derived from tmp_path rather than the hardcoded /tmp/repo-... location. Keep the
invalid UTF-8 byte in the emitted path, and ensure the resulting path remains
within tmp_path so _worktree_local_exclude creates and writes only inside
pytest’s managed sandbox.
---
Nitpick comments:
In `@src/bmad_loop/install.py`:
- Around line 790-805: Update the temporary path creation in the exclude update
flow around atomic_replace to use a process-scoped unique scratch filename
rather than the fixed “.bmad-loop.tmp” name. Preserve the existing
write-then-atomic-replace behavior and cleanup on failure, while ensuring
concurrent story runs sharing the same git common directory cannot reuse or
overwrite the same scratch file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e923defa-e527-4d6a-bd49-e90331be986f
📒 Files selected for processing (3)
CHANGELOG.mdsrc/bmad_loop/install.pytests/test_install.py
Review of the previous commit: hand-rolling `tmp.write_text` + `atomic_replace` with a FIXED sibling name reintroduces the hazard the helper's own docstring calls out — "no fixed `.tmp` sibling for a concurrent writer of the same file to collide with". That collision is reachable here specifically. Every linked worktree of a repo resolves `--git-common-dir` to the SAME main `.git`, so two runs provisioning against one repo both target `<main>/.git/info/exclude.bmad-loop.tmp`. atomic_write_text also fsyncs before the replace and carries the mode across, both of which a bare replace resets — it swaps a new inode in. Test seams had to move with it, and both moves matter: - the two write-fault tests patched `Path.write_text`; the write now goes through `os.fdopen` on an mkstemp fd, so that patch stops firing and the tests would pass VACUOUSLY. Patched at install's own `atomic_write_text` binding instead. - the short-write test patched `Path.open` for the same reason. Retargeted to `io.open`, the one seam BOTH shapes share — the fix reaches it via os.fdopen, the ablation via Path.open — so the ablation keeps biting. - the leftover-scratch-file assertion named a literal filename, which is vacuous once the temp name is random. It now asserts the directory contains nothing but `exclude`. All three ablations re-run after the seam moves: direct write_text fails the short-write test, dropping OSError fails both write-fault tests, and restoring text=True fails the stub-git test.
Caught in review. The stub emitted a hardcoded `/tmp/repo-\377-name/.git`, and the helper mkdir(parents=True)s whatever common dir it is handed — so every successful run created real directories in the SHARED system /tmp, outside pytest's tmp_path and its cleanup. Confirmed on this box: /tmp/repo-<0xff>-name existed. The stub now builds its path from tmp_path via os.fsencode, keeping the undecodable byte while staying in the sandbox, and the test asserts nothing was created outside it. Ablation re-run against the new stub: restoring text=True still fails it. Also documents why this call stands outside verify._run_git, the chokepoint AGENTS.md otherwise requires: nothing structural forbids it, but _run_git returns CompletedProcess[str] — the strict decode this call exists to avoid — and raises GitError rather than the (SubprocessError, OSError) this arm treats as an expected skip. Adopting it needs a bytes mode on the chokepoint, filed as #377.
|
All three addressed — two were real, and the 1. Fixed scratch name racing on the shared common dir — valid, already fixed in 99ab745. You reviewed the commit before it. The fix is not a hand-rolled unique name but Moving to it broke three test seams in a way worth recording, because each would have failed silently: the helper writes via 2. Stub 3. Route through Taking your second option — an explicit exception with the rationale in the code, including the real cost (this call has no timeout). The bytes mode the chokepoint would need is #377, which also covers a bigger instance of the same defect: |
|
@codex review |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 21 minutes. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0066682eea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # common dir, so a fixed `.tmp` sibling is a collision between two runs | ||
| # provisioning against one repo. It also fsyncs before the replace and | ||
| # carries the mode over, which a bare replace resets. | ||
| atomic_write_text(exclude, prefix + "\n".join(new) + "\n") |
There was a problem hiding this comment.
When .git/info/exclude does not yet exist, atomic_write_text creates it with mkstemp's fixed 0600 mode, whereas the previous Path.write_text honored the process umask (normally producing 0644 or 0664). In a shared checkout or a repository later accessed under another UID, Git cannot read this common exclude file, ignores its shielding patterns, and can consequently stage the provisioned tool directories during git add -A; use creation permissions compatible with the prior behavior while retaining the atomic replacement.
Useful? React with 👍 / 👎.
|
|
||
| assert reason is None or "could not update" in reason | ||
| # nothing was created outside the sandbox | ||
| assert not list(Path("/tmp").glob("repo-*-name")) or sys.platform == "win32" |
There was a problem hiding this comment.
On POSIX, this assertion fails whenever any unrelated process or an earlier test run has left a matching /tmp/repo-*-name directory, even though this invocation correctly created everything under tmp_path. Conversely, when pytest uses a temporary root outside /tmp, it does not prove that nothing was created beside that root. Compare a before/after snapshot or assert only paths attributable to this stub so the test cannot fail because of ambient system state.
Useful? React with 👍 / 👎.
The test drove a real stub `git` on PATH, which is the right shape, but three details let it pass without testing anything. - It asserted only `reason is None or "could not update" in reason`, which the git-unqueryable arm satisfies by returning None. Whenever the stub cannot exec — a noexec temp dir, no /bin/sh, a dropped exec bit — the OSError was swallowed as an expected skip and the test went green having run nothing, taking its `text=True` ablation with it. Measured: at mode 0644 every assertion still passed. Now pins that the tail actually ran. - The leak check globbed shared /tmp, which is non-recursive while tmp_path sits three levels down, so it could not see this test's own output — but it could fail for anyone whose box still carried litter from the hardcoded-path version. The containment pin replaces it. - The path was interpolated into the shell script unquoted, so a temp root with whitespace word-split into a second printf argument; `printf '%s\n'` repeats its format, and the helper then created a directory with an embedded newline beside the basetemp pytest reaps. Measured with a spaced --basetemp: it leaked and still passed. Now single-quoted. Also drops `prepend=False` from the PATH setenv: pytest's parameter takes a separator string, and only a falsy value made it a no-op rather than a TypeError. Ablations re-run: text=True restored fails; a non-executable stub fails; the unquoted interpolation under a spaced --basetemp fails.
Three corrections to comments added by this PR, no behavior change. - The mode is carried over only when the exclude already exists; atomic_write_text creates a new one at its deliberate 0600 rather than the umask default, as its own docstring states. - Atomicity per write is not isolation across the read/write pair. The comment reasoned about two runs provisioning against one repo, which invited the reading that concurrency was handled; the lost update that remains is #381, reproduced there. - The Windows note read as contradicting the regression test's POSIX-only skip. Marked as defensive placement rather than a path under test.
|
Triaged on @coderabbitai — your last review read stale code. Its own review-info block says it covered "between 87a4b5e and 2eb8f62", and it posted at 05:34:53Z, before
The two @greptile-apps — the Pulling that thread found three more holes in the same test, all measured rather than reasoned:
All three ablations re-run against the final shape: One production finding, filed rather than fixed: #381. Both calls returned |
|
Tip For best results, initiate chat on the files or code changes.
I also agree with the scoping: 🐇💭 ✏️ Learnings added
|
|
@coderabbitai full review |
|
All acknowledged — and the follow-up work here is more thorough than the original fixes. On the stale review: the timestamp evidence is clear. Both "Full review finished" replies were rate-limited non-reviews. That failure mode — a silent success from a tool that didn't run — is genuinely hard to defend against from the reviewer's side. Thanks for flagging it precisely. On On the three measured holes in the stub-git test: each of these is the right kind of finding — not "this looks wrong" but "I measured this and it passed when it shouldn't." The vacuous-pass-on-noexec is particularly sharp: noexec temp dirs are standard CI hardening, so the test would have been reliably hollow in the environments where it most needed to work. The single-quoted shell interpolation and the non-recursive glob are the same quality. All three now have ablations that confirm they bite. On #381: correct scoping. The current shape of both LGTM on the current HEAD. |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 2 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
atomic_write_text carries a mode over only when the target already exists; one it creates from nothing gets mkstemp's private 0600, where the write_text it replaced honored the umask. So moving to the helper silently narrowed a freshly created .git/info/exclude from 0644 to 0600. That matters at this call site specifically. An exclude git cannot read is one git silently ignores — measured: it warns "unable to access", exits 0, and `git add -A` stages the files the exclude was written to shield. That is the harm the helper's docstring says silence must not cause, reached through a write that SUCCEEDED, so not even a degrade reason fires. Reachable wherever the common dir is read under another UID (core.sharedRepository, a shared checkout) — atomic_write_text's own docstring warns that callers of genuinely shared state need more than it alone. Creates the file the way write_text did, under the umask, before handing it to the helper, which then carries that mode across the replace. An empty exclude is equivalent to an absent one for git, so a fault after the touch leaves no more damage than the missing file it replaced. Found by Codex review. The regression test pins the created mode against a measured write_text probe rather than a hardcoded 0644, and pins the umask at 0022, because at a box's own 0077 the ablated code also produces 0600 and the ablation would not bite.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex — both correct. The first one I had already been handed and had triaged wrong; your framing is what makes it a defect rather than a docs nit. 1. Creation permissions — real regression, fixed in git does not fail on an unreadable exclude — it warns, exits 0, and stages the files the exclude existed to shield. So the narrowed mode reproduces the exact harm the helper's docstring says its silence must not cause, and reaches it through a write that succeeded, meaning not even the degrade-reason channel fires. The same docstring I had leaned on says the opposite of what I took from it two paragraphs on: "Callers writing genuinely shared, multi-user state need more than this helper." This caller is that. Fixed by creating the file the way the replaced The regression test pins against a measured 2. Sandbox assertion scoped to ambient state — already fixed in That same commit closed two more in the same test: it passed vacuously whenever the stub could not exec (the only assertion was satisfied by the git-unqueryable arm's Ablations re-run against the final shape: |
|
@codex review |
|
@coderabbitai full review |
|
To use Codex here, create an environment for this repo. |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 47 minutes. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #374. Closes #375.
Follow-up to #359 (PR #373), which guarded
_worktree_local_exclude's filesystem tail but left twoholes the review surfaced. Both live in the same helper and both falsify the same best-effort
contract, so they ship together rather than as two PRs racing on adjacent lines.
#374 — the git-query arm decoded strictly
subprocess.run(..., text=True)decodes stdout with the locale codec aterrors='strict'.UnicodeDecodeErroris neither aSubprocessErrornor anOSError, so it escaped the one arm whoseentire contract is "skip silently". POSIX filenames are bytes, so a repo path carrying bytes
invalid in the locale encoding reaches this on an ordinary box:
Stdout is now captured as bytes and decoded with
os.fsdecodeinside the guarded tail. Tworeasons for that placement:
fsdecodeuses surrogateescape on POSIX, so a non-UTF-8 path round-tripsback to the filesystem instead of faulting at all; and it can still raise on Windows, where
utf-8/surrogatepass rejects a lone invalid byte — so it belongs where
UnicodeErroris alreadycaught.
#375 — the write was not atomic
The update is a read-modify-rewrite, and
Path.write_textopens"w", which truncates beforewriting.
prefixcarries the operator's own excludes, so a fault partway through left the file cutmid-content while the degrade reason still said "could not update":
Three consequences: the reason misdescribes the state; the surviving tail parses as a valid git
pattern and is a prefix of the intended one, so a cut one character in leaves the last line as
/,excluding the entire worktree — the exact state
worktree_flow.pyalready guards against forhookless profiles; and the blast radius is the main repo, since a linked worktree's
--git-common-dirpoints at the main.git.Now written to a scratch file and moved into place with
atomic_replace, with the scratch fileremoved on the failure path. The exclude is either fully updated or untouched.
One test was replaced after failing its own ablation
The first version of the #374 test monkeypatched
subprocess.runto return undecodable bytes. Itpassed — and passed identically with
text=Truerestored, because handing the code bytesdirectly never runs the stdlib's decoding at all. It could not see the bug it was written for. Driving
a real stub
giton PATH does fail the ablation. It is POSIX-only and skipped on Windows, whichis correct rather than a dodge: Windows paths are UTF-16, so git cannot emit an undecodable one.
The #375 test injects at
Path.open, notPath.write_text— patchingwrite_textmeans the file isnever opened, so the truncation cannot happen and the test would pass against the very bug it exists
to catch. That is the same reason the two pre-existing write-fault tests needed their name filter
widened: the write now lands on the scratch file, and a filter pinned to
"exclude"alone would stopinjecting anything and pass vacuously.
text=Trueon the subprocessexcludeinstead of tmp +atomic_replacesubprocess.runinstead of stubbing gitVerification
uv run pytest -q -n auto— 3695 passed, 24 skipped (run three times; one unrelated Textual modaltest,
test_decision_modal_scrolls_when_content_long, flaked once under parallel load and passesconsistently in isolation and on
main).uvx pyright@1.1.411— 0 errors.trunk fmt+ fulltrunk check— no issues.Summary by CodeRabbit
gitoutput, including validation that exclude content remains byte-identical.Follow-up filed, not fixed here: #381 —
atomic_write_textmakes each write all-or-nothing but does not serialize the read-modify-write around it, so two runs provisioning against one repo interleave into a lost update. Pre-existing, reproduced in the issue.