Make the state_paths suite pass from inside a lane worktree (#10)#28
Conversation
Closes #10. The autouse fixture neutralized every sandbox ENV signal and none of the cwd one. `_marker_state_root` discovers a sandbox by walking up from `Path.cwd()`, so a `.devkit_state_root` marker at or above the invocation directory redirects the resolver and two "no sandbox configured" assertions fail — while every env var is dutifully unset, which is what made the failure look environmental and unexplained. It cannot show up the ordinary way: CI checks out a marker-free tree and a developer runs from the main checkout. It appears only when the suite runs from inside a headless lane worktree, which is exactly what a lane agent does before pushing. Three lanes hit it in one session, which is why the friction log raised it from M to H: a gate that goes red for reasons unrelated to the diff teaches agents to ignore a red gate. The issue anticipated that a fixture-level `chdir` would need a per-test audit, since several tests assert on repo-root discovery relative to cwd. It doesn't: every such test already calls `monkeypatch.chdir` itself, which runs after the fixture and overrides it. The fixture only makes the *default* explicit instead of an accident of where pytest was invoked. Verified by running the suite from both a marker-free and a marker-carrying cwd — 62 pass either way. The regression guard is the invocation itself, in `test_portability.py`: a subprocess pytest run from a directory carrying a marker. A test inside the state_paths suite could not catch this — the condition is a property of where the suite is started from. Confirmed it fails with the fixture change reverted. Tests 285 → 286. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
|
Warning Review limit reached
Next review available in: 55 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes isolate state path tests from sandbox markers by changing into a temporary directory and add a portability regression test that runs the suite from inside a marked lane worktree. ChangesState path isolation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
The bot is rate-limited, so an independent pass reviewed this. Its main finding is a real diagnostic gap in the regression test itself. **The guard could go red with no reason given.** It asserted on `returncode` with only `stdout` in the message — but pytest reports a bad path or a collection error on **stderr**, while stdout says just "no tests ran". So if this path ever drifted, the failure would look exactly like the unexplained red gate this whole PR exists to eliminate. Now includes stderr, and asserts on the collected count too. (It could not have passed vacuously — a wrong path exits 4 or 5, both non-zero — but a thinned-out run is a different hazard from a mis-pathed one.) Also: `ENGINE_DIR` rather than `REPO_ROOT / "scripts"`, since `paths.engines` is configurable and a vendored layout would otherwise point at nothing; and a `timeout` on the subprocess. **The fixture docstring now states what the fix does NOT cover.** `tmp_path` is marker-free only because of where `TMPDIR` points, so pointing `TMPDIR` inside a marker-carrying worktree still fails. Making it hermetic would need a `.git` ceiling sentinel inside `tmp_path`, which the "no `.git` anywhere up → raise" tests require the absence of. The lane case this fixes is the one that actually happens; saying so beats implying the class is closed. **Tightened a claim that could be read too broadly.** "Every one of those tests already calls `monkeypatch.chdir` itself" is true as scoped to tests asserting on repo-root discovery, but reads as "no non-chdir test depends on cwd" — and two do, which is the entire bug. The review's mutation analysis also showed the chdir makes the suite strictly *stricter*: three tests previously passed by accidentally discovering the real repo root, and now genuinely bite. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/tests/test_portability.py`:
- Around line 1269-1272: Update the test assertion surrounding the portability
test summary to parse pytest’s collected/passed count and compare it against an
explicit expected value or documented minimum, rather than matching the broad
60–69 range with a regular expression. Preserve the existing non-zero exit
validation and ensure the assertion fails clearly when collection regresses or
the suite exceeds the intended fixed count.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4754d593-5c76-41af-bd50-3081a67fe35e
📒 Files selected for processing (2)
scripts/lib/state_paths/tests/test_state_paths.pyscripts/tests/test_portability.py
CodeRabbit, on the fix from the previous commit. `\b6\d+ passed` was a bandaid in both directions: it accepts a silent drop to 60, and it stops matching entirely the moment the state_paths suite reaches 70 tests — turning a growth milestone into a mystery failure in the very test whose purpose is to keep the gate explicable. Parses the count and compares against a named floor instead, with the floor documented as raise-deliberately-never-lower. Verified it bites: setting the floor above the actual count fails the test. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
|
Merging. Receipt: Coverage: an independent fresh-context pass reviewed the substantive change (the fixture + regression guard) and found a real diagnostic gap — the guard asserted on Worth recording from the independent pass: it mutation-tested the resolver eight ways against both the pre- and post-fix test files and compared kill sets. No test lost a kill, and three gained one — |
* Wrap-up: Phase 3b — the queued-vs-unavailable ambiguity, closed Three PRs merged, three issues closed, two filed. - **#25** (closes #19 + #23) — a configured bot resolves to *unavailable* (outage on a comment body OR a status-check description, the surface that was invisible) or *pending* (a verdict still coming). Nothing reaches `converged`, which is what let both be fixed at once without touching the anti-wedge property. - **#28** (closes #10) — the state_paths suite failed from inside a lane worktree because its fixture cleared every sandbox *env* signal and not the *cwd* one. - **#29** (half of #27) — `review_bots.coverage` reports which commit each bot's last review actually saw. - **#26 / #27 filed** from the friction-log inbox, both with concrete sketches. The handoff and friction log carry the detail. The two entries worth repeating: **Eleven review rounds, ten with findings — and five of those were defects introduced by the previous round's fix**, twice at HIGH. #25 never produced a clean pass. That is not a caution about fix rounds; it is the base rate, and the doctrine currently states it as advice. **Reading is not running.** The #19 guard was dead code for the exact bot it was written for, because CodeRabbit's pending check reports the zero timestamp — no amount of re-reading found that; polling the live PR did. Same for a change that looked plainly correct and aborted `init.sh` under `set -eu`. Also recorded: `pre-push` refused this very commit's first attempt, because the narrative files had been swept into a lane branch. The guard fired on its own author with a message that made the fix obvious — logged as the positive case this log rarely captures. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Fix 13 errors the fact-check found in this wrap-up I had the wrap-up independently fact-checked against `git log` / `gh` rather than self-certifying it, because the handoff is read at the start of every future session. It came back with 13 issues, three HIGH. The one worth naming: **I reintroduced a number that a review round on #25 had explicitly corrected** — "four ways to corrupt an adopter's config" when `321907e` had already changed it to three, and `init.sh`'s own comment enumerates three. I did that while writing up the lesson that PR bodies keep drifting from their diffs. The friction entry recording that failure now records that it recurred inside the document describing it, and is filed at M not L. **The base rate was understated, in my own argument's favour.** I wrote "eleven rounds, ten with findings" and "#29 produced a clean pass on the fourth attempt". Neither is true: session-wide it is **13 rounds across three PRs, all 13 with findings**, and #29's fourth pass found no HIGH/MEDIUM but still found four LOW including a stale comment. Self-inflicted defects — introduced by the previous round's fix — were 7, not 5. So "re-review until a pass finds nothing new" never terminated, which sharpens rather than weakens the point: the rule needs a stopping criterion, and the one actually used was blast radius. **The proposed rule change was aimed at a rule that already exists.** `safety-critical-changes.md` rule 3 already mandates re-review after every fix round and already notes that fix rounds introduce regressions. The proposal now asks for what is genuinely missing — a termination criterion, and the blast-radius calibration that let #29 merge where #25's standard would not have. Also corrected: the mutation-proved tally (five properties session-wide, not two in one place and four in another); "the panel carried the entire review load" (CodeRabbit completed 3 reviews across 17 heads — most, not all, and one of its three caught a real bug); "five manual launches per round" (two lenses per round, ~10 rounds); a "126 report pairs" figure that exists only in a PR comment and is not reproducible from the repo, now pointing at the in-repo tests instead; a dangling "the two entries above this line" after those entries were deleted; and two inbox entries whose issues this session closed, now marked done. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Align the mutation tally across both narrative files CodeRabbit, on the wrap-up. My correction commit fixed the handoff's tally to five session-wide and left the friction log saying two — so the two documents the next session reads disagreed with each other about the very number the correction was about. Both now say five, with the properties enumerated (three on #29, two on #25) so the figure is checkable rather than asserted. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
1 participant
Closes #10.
The bug
The state_paths suite's autouse fixture neutralizes every sandbox env
signal and none of the cwd one.
_marker_state_rootdiscovers a sandbox bywalking up from
Path.cwd(), so a.devkit_state_rootmarker at or above theinvocation directory redirects the resolver, and two "no sandbox configured"
assertions fail — while every env var is dutifully unset. That mismatch is what
made the failure read as environmental and unexplained.
Reproduced before the fix:
Why it matters more than two tests
It cannot surface the ordinary way — CI checks out a marker-free tree, and a
developer runs from the main checkout. It appears only when the suite runs from
inside a headless lane worktree, which is precisely what a lane agent does
before pushing. Three lanes hit it in one session, which is why the friction log
raised it from M to H: a gate that goes red for reasons unrelated to the diff
teaches agents to ignore a red gate.
The fix, and why it's smaller than the issue expected
The issue anticipated that a fixture-level
chdirwould need a per-test audit,because "several [tests] assert on repo-root discovery relative to cwd, so it is
not a mechanical change".
It isn't needed: every one of those tests already calls
monkeypatch.chdiritself, which runs after the autouse fixture and overrides it. The fixture
only makes the default state explicit — "no marker above me" — instead of an
accident of where pytest happened to be invoked. Control stays with the tests
that use it.
Verified by running the suite from both a marker-free cwd and a marker-carrying
one: 62 pass either way.
The regression guard
In
test_portability.py, because the condition is a property of where thesuite is started from — no test inside the state_paths suite can observe it.
It runs pytest as a subprocess from a directory carrying a marker.
Confirmed it does its job: reverting the fixture change makes it fail, and
restoring it makes it pass.
Tests 285 → 286.
https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Summary by CodeRabbit