fix(install): guard the worktree exclude's filesystem tail (#359) - #373
Conversation
`_worktree_local_exclude` documented itself as best-effort but wrapped only its `git rev-parse --git-common-dir` call. Everything after it — resolve, mkdir, read_text, write_text — propagated out of a function whose contract says it never does, crashing the run over housekeeping. The two arms cannot merge, because OSError means the opposite thing in each: in the subprocess arm it is "no git to query", an expected skip callers hit routinely on plain temp dirs; in the tail it is a real filesystem fault worth surfacing. So the tail gets its own guard catching exactly (OSError, RuntimeError, UnicodeDecodeError) — RuntimeError because a symlink loop under `.resolve()` raises that, not OSError, on the 3.11/3.12 legs, and UnicodeDecodeError (a ValueError subclass) because an exclude file that is not UTF-8 breaks `read_text`. The helper returns `str | None`: None for success AND for the expected skip, a reason string for a degradation. Swallowing alone would be a regression, so the surfacing lands with it: `provision_worktree` takes a keyword-only `on_degraded` (return contract unchanged) and `run_isolated` wires it to a `worktree-exclude-degraded` journal entry, mirroring `on_teardown_degraded`. Journal-only, like the teardown precedent — the run is unharmed and the operator has nothing to do right now. Without it a lost exclude turns a loud crash into the unit's `git add -A` silently committing the provisioned skill trees and tool configs into the story's merge. Corrects the issue's framing of the resolve branch: it is the PLAIN-CHECKOUT case, not the linked-worktree one. Verified against git 2.55 in a scratch repo — `git -C <plain checkout> rev-parse --git-common-dir` prints a relative `.git` (so the `is_absolute()` guard falls through to `.resolve()`), while the same command in a linked worktree prints the main repo's ABSOLUTE `.git` and never reaches it. The degrade test therefore drives the helper against the main checkout; a linked worktree could not exercise that line. Faults in the tests are injected by name-filtered monkeypatch rather than built for real: 3.13+ resolves a symlink loop without raising, so a hand-built loop would false-green on this box and on the newer CI legs. The undecodable -exclude test uses real bytes, since that fault reproduces everywhere. Every degrade and negative assertion carries its ablation in the docstring; all six were run (tests 4 and 7 needed the inverse ablation — making the expected-skip arm return a reason).
WalkthroughWorktree-local git exclude handling now distinguishes expected git-query skips from filesystem failures, returns degradation reasons, and journals those reasons during isolated runs. Provisioning continues, while tests cover failure, success, callback, and end-to-end journal behavior. ChangesWorktree exclude resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WorktreeFlow
participant provision_worktree
participant _worktree_local_exclude
participant Journal
WorktreeFlow->>provision_worktree: provision isolated worktree
provision_worktree->>_worktree_local_exclude: update local git exclude
_worktree_local_exclude-->>provision_worktree: return degradation reason
provision_worktree->>WorktreeFlow: invoke on_degraded(reason)
WorktreeFlow->>Journal: append worktree-exclude-degraded
WorktreeFlow-->>WorktreeFlow: continue provisioning and merge
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 guards the filesystem tail of
Confidence Score: 5/5Safe to merge — the change converts a function that could crash the run into a properly guarded best-effort helper, with the degrade surfaced to the journal rather than swallowed. Both arms of _worktree_local_exclude are correctly guarded and independently tested with ablation notes. The UnicodeError catch covers both codec directions precisely. The on_degraded callback is keyword-only with a None default leaving all existing callers unaffected. The engine-wiring test confirms the degrade journal entry lands and the unit still merges. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/install.py | Splits _worktree_local_exclude into two guarded arms; return type widened to str |
| src/bmad_loop/worktree_flow.py | Adds keyword-only on_degraded to provision_worktree (existing callers unaffected); run_isolated wires it to a journal append matching the teardown-degraded precedent |
| tests/test_install.py | Seven new tests covering symlink-loop RuntimeError, write-fault OSError, undecodable exclude UnicodeDecodeError, unencodable pattern UnicodeEncodeError, non-git expected skip, happy-path, and on_degraded forwarding — all with ablation notes |
| tests/test_engine_worktree.py | Engine-wiring test patches _worktree_local_exclude at the worktree_flow binding and verifies a worktree-exclude-degraded journal entry is written while the unit still merges |
| CHANGELOG.md | Accurate changelog entry describing the two-arm guard, the codec fix, and the journal surfacing |
Reviews (4): Last reviewed commit: "test(install): correct a stale ablation ..." | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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 55 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 777-779: Update src/bmad_loop/install.py lines 777-779 to raise
the established typed degradation error for post-query exclude-write failures
instead of returning a string; update src/bmad_loop/worktree_flow.py lines
293-295 to forward that typed failure after optional reporting, and lines
486-488 to journal it before pausing/escalating the unit so no drive or
integration proceeds. Update tests/test_install.py lines 1465-1542 and 1558-1584
to expect the typed failure and verify reporting plus escalation,
tests/test_engine_worktree.py lines 1549-1553 to assert the unit is not merged,
and CHANGELOG.md lines 263-270 to document the escalation behavior.
🪄 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: d730a39e-b08c-438e-9330-03b76cdf5806
📒 Files selected for processing (5)
CHANGELOG.mdsrc/bmad_loop/install.pysrc/bmad_loop/worktree_flow.pytests/test_engine_worktree.pytests/test_install.py
|
Verified against the code and rejecting this one — it is a severity-policy disagreement rather than a defect, and the repo's own escalation ladder points the other way. 1. It would invert the repo's severity ordering. Failing to mount a worktree at all is strictly more consequential than failing to write its backup shield, and that case does not escalate: 2. The exclude is a second line of defense, not the only one. Per 3. Escalation is the opposite of what #359 asked for. The issue is that 4. The doctrine line is already satisfied, and the "repair write" half doesn't reach here. "Typed escalation over bare except" is met: the tail catches an explicit 5. There is a closer precedent inside the same function. A The kernel of the concern is real and the PR does act on it: a swallowed fault would trade a loud crash for silent repo pollution, which is why the surfacing ( Leaving the design as-is. |
The tail's codec guard named only `UnicodeDecodeError`, which covers the
READ half — an exclude file that is not UTF-8. The WRITE half raises the
sibling `UnicodeEncodeError`, and the two share no subclass but
`UnicodeError`, so the best-effort contract still leaked:
_worktree_local_exclude(repo, ["/vendor/weird-\udcff-name"])
-> UnicodeEncodeError: surrogates not allowed
Reachable, not theoretical. `provision_worktree` derives one pattern per
`seed_globs` match through `rel.as_posix()`, so the pattern text comes
from a real filename; a repo file whose name is not valid UTF-8 arrives
surrogate-escaped and cannot be written back out as UTF-8.
Widened to `UnicodeError`, which covers both directions and stays far
short of the `ValueError`-broad catch the original rationale rejected —
a programming error still escapes.
The new test uses a literal "\udcff" rather than
`os.fsdecode(b"...\xff...")`: fsdecode decodes with surrogatepass on
Windows, which rejects a lone invalid byte and would have raised in the
test's own setup, reddening only the Windows legs.
Ablations run: narrowing the tuple back to `UnicodeDecodeError` fails the
new encode test alone; dropping the codec member entirely fails both the
encode and the existing decode test, so each half is independently
pinned.
The #359 guard covers the filesystem tail in both codec directions, but the git-query arm above it still decodes stdout strictly, so a repo path with bytes invalid in the locale encoding raises UnicodeDecodeError — neither arm's type. Verified with a stub git emitting byte 0xff. Left out of the fix on purpose: the minimal repair (capture bytes, then os.fsdecode) makes that path work, but the surrogate-bearing string then flows into the degrade reason and on into journal.append, whose JSON write is itself UTF-8. Deciding where filesystem paths get decoded is program-wide, so it gets its own change; #374 carries it. Docstring only — no behavior change.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Widening the tail's tuple to `UnicodeError` left the undecodable-exclude test telling a reader to "drop `UnicodeDecodeError` from the tail's except tuple" — a member that is no longer there, so the instruction cannot be followed. Caught in review. Replaced with the two ablations actually run: dropping `UnicodeError` fails this test, while narrowing it to `UnicodeDecodeError` does not — that one belongs to the encode sibling. Stating both is what shows the codec member is independently pinned from each side rather than by one test twice. Comment only — no behavior or assertion change.
|
Good catch on the stale ablation note — fixed in 6f19355. Widening the tail's tuple to Replaced with the two ablations I re-ran to confirm the corrected wording:
Stating both is the point — it shows the codec member is pinned independently from each side rather than by one test twice. |
Closes #359.
install._worktree_local_excludedocumented itself as "Best-effort — skipped if git can't bequeried", but the only
trycovered thegit rev-parse --git-common-dirsubprocess. Everythingafter it was unguarded and propagated out of a function whose contract says it never does:
(worktree / common_dir).resolve()RuntimeErroron a symlink loop — 3.11/3.12 only, and the repo floor is 3.11exclude.parent.mkdir(...)OSError(read-only.git)exclude.read_text(...)OSError, andUnicodeDecodeErroron an exclude file that is not UTF-8exclude.write_text(...)OSError, andUnicodeEncodeErroron a pattern carrying a surrogateWhy the two arms cannot merge
OSErrormeans the opposite thing on each side of the git call. In the subprocess arm it is "no gitto query" — an expected skip that must stay silent, since callers hand this plain non-repo
directories routinely. In the tail it is a filesystem fault worth surfacing. Same type, opposite
meaning, distinguishable only by location. So the tail gets its own guard catching exactly
(OSError, RuntimeError, UnicodeDecodeError)— explicit rather thanValueError-broad, which wouldswallow programming errors ("typed escalation over bare except", AGENTS.md). The helper returns
str | None:Nonefor success and for the expected skip, a reason string for a degradation.Swallowing alone would have been a regression
The exclude is what "shields [the provisioned tool files] even when a project doesn't" gitignore
them. A silently swallowed failure means the unit's
git add -Acommits.claude/,.mcp.jsonand_bmad/custom/into the story merge — a loud crash traded for silent repo pollution. So thesurfacing lands in the same PR:
provision_worktreegains a keyword-onlyon_degraded: Callable[[str], None] | None = None. Thelist[str]return contract is untouched, so the lazyinstall.__getattr__re-export and ~20existing callers/tests are unaffected.
run_isolatedwires it tojournal.append("worktree-exclude-degraded", story_key=..., error=msg),mirroring the
on_teardown_degradedlambda a few lines below.Journal-only, no
gates.notify: the precedentworktree-teardown-degraded(housekeeping failed, runcontinues) is journal-only, while
worktree-open-failedgets notify because the operator must actnow. Here the run finishes normally and the harm is conditional and greppable.
Correcting the issue's framing
The
.resolve()branch is the plain-checkout case, not the linked-worktree case. Verified in ascratch repo against git 2.55:
A linked worktree gets an absolute common dir and never reaches the
is_absolute()fall-through. Thesymlink-loop test therefore drives the helper against the main checkout — a linked worktree could not
exercise that line at all.
The issue's caller list is stale.
operatoractions._exclude_from_gitwas deleted by #356, whichmade the park record deliberately committed. There is exactly one live caller left —
worktree_flow.provision_worktreeat worktree_flow.py:287, reached only fromWorktreeFlow.run_isolated— and this covers it. The engine's park-path containment from #355 isuntouched by this change, as is
engine._write_park_record's ownexcept (OSError, RuntimeError),whose justification #356 already rewrote to "insurance".
Tests
Seven helper/provision tests in
tests/test_install.py, one engine-wiring test intests/test_engine_worktree.py. Faults are injected via name-filtered monkeypatch (thetest_engine.py:1553pattern) rather than built for real: 3.13+ resolves a symlink loop withoutraising, so a hand-built loop would false-green on the newer legs. The
UnicodeDecodeErrortest usesreal undecodable bytes, since that fault reproduces everywhere, and asserts the legacy bytes survive
untouched.
Every degrade and negative assertion carries its ablation in the docstring, and all six were run:
RuntimeErrorfrom the tail tupleOSErrorfrom the tail tupleon_degradedforwarding testUnicodeDecodeErrorfrom the tail tupleon_degraded(reason)inprovision_worktreeon_degraded=lambda inrun_isolatedThe two expected-skip tests are negative assertions, so they needed the inverse ablation — a gate
deletion cannot fail them. The engine test's
worktree_seedsetup was checked the same way: withoutit,
provision_worktreeshort-circuits before the exclude step and the wiring under test is neverreached.
The five existing exclude happy-path tests are green unmodified.
Follow-up commit: the codec fault has a write direction too
Review triage turned up one real gap in the first commit. The tail's guard named
UnicodeDecodeError, which is only the READ half.write_textraises the siblingUnicodeEncodeError, and the two share no subclass butUnicodeError— so the best-effortcontract still leaked:
Reachable rather than theoretical:
provision_worktreederives one pattern perseed_globsmatch through
rel.as_posix(), so the pattern text comes from a real filename — a repo filewhose name is not valid UTF-8 arrives surrogate-escaped and cannot be written back as UTF-8.
Widened to
UnicodeError: it covers both directions and stays far short of theValueError-broad catch the original rationale rejected, so a programming error still escapes.The new test writes a literal
"\udcff"rather thanos.fsdecode(b"...\xff...")— fsdecodedecodes with surrogatepass on Windows, which rejects a lone invalid byte and would have raised
in the test's own setup, reddening only the Windows legs.
UnicodeDecodeErrorEach half is independently pinned.
Review outcome
Three findings raised, one design objection declined.
UnicodeDecodeError);write_textraises
UnicodeEncodeErrorand the two share no subclass butUnicodeError. Reproduced end-to-endthrough
seed_globs. Widened, tested, ablated both directions.a tuple member that no longer existed. Corrected to the two ablations actually run.
It inverts this repo's severity ladder — failing to mount a worktree at all defers rather than
escalates ("Defer this unit rather than crash the whole run", worktree_flow.py), and the exclude is
a second line of defense behind the project's own
.gitignore. Escalating would also preserve theexact bug install: _worktree_local_exclude does unguarded filesystem work outside its only try #359 filed: the run still dies over a best-effort shielding step. Reasoning is in the
thread.
Two pre-existing hazards in the same helper were verified but deliberately left out of this change,
filed as #374 (the git-query arm decodes stdout strictly, so a repo path with bytes invalid in the
locale encoding still escapes) and #375 (the exclude write is not atomic, so a short write truncates
the operator's shared exclude while the reason string reports that nothing happened). Both want a
decision this PR's scope does not contain — where filesystem paths get decoded, and whether the write
becomes atomic.
Verification
uv run pytest -q -n auto— 3693 passed, 24 skipped.uvx pyright@1.1.411— 0 errors.trunk fmt+full
trunk check— no issues.Summary by CodeRabbit