Engine::new() reaches out to ambient disk and git state during construction, so any test that builds an Engine silently inherits the test runner's home directory and surrounding checkout. The fixture test_engine() in src/tui_main/render_impl.rs compensates by resetting fields one at a time, reactively, after each leak is discovered the hard way.
Four leaks have now been found this way:
| leak |
ambient source |
found via |
git_branch |
git::current_branch(cwd) |
#439 |
sc_ahead |
surrounding repo state |
#439 |
sc_behind |
surrounding repo state |
#439 |
session.explorer_visible |
~/.config/vimcode/session.json |
#615 |
The fourth one cost multiple investigation rounds and produced three separate "verified" wrong root causes (unpinned unicode-width, rustc toolchain skew, cargo feature unification) before anyone reproduced it — because it is invisible on any machine where a human has ever run vimcode. A fix was written, reviewed, and approved against the first wrong theory before the real cause surfaced.
The reactive pattern is at diminishing returns. Each new field read from disk or git inside Engine::new() is a future CI-only snapshot failure waiting to happen, and the failure mode is maximally expensive: it cannot be reproduced locally without deliberately clobbering $HOME.
Proposal
Add a constructor that never touches ambient state:
impl Engine {
/// Construct an Engine with no disk or git reads. For tests and any
/// context that must be reproducible.
pub fn new_for_test() -> Self { ... }
}It should skip Settings::load(), SessionState::load(), HistoryState::load(), and git::current_branch() entirely, rather than calling them and overwriting the results afterward — the #615 leak proved that overwriting-after is not always possible. explorer_visible was consumed inside Engine::new() to decide app_shell.hide_sidebar(), so by the time the fixture ran, the decision was already baked into app_shell and no field assignment could undo it.
Then have test_engine() call it and drop the accumulated per-field resets.
Acceptance
The gate is a clean-home run, since that is the only thing that catches this class:
HOME=$(mktemp -d) cargo test --no-default-features # preserve RUSTUP_HOME/CARGO_HOME
Ideally this becomes a standing CI job so the next leak is caught by the suite instead of by a multi-day investigation. Worth considering as part of #613's CI work.
Context
Root cause detail and the elimination trail for the four dead hypotheses are pinned on #615. Follow-up to the fix landing there, which patches the fourth leak in the existing reactive style to unblock CI; this issue is the structural fix.
Engine::new()reaches out to ambient disk and git state during construction, so any test that builds anEnginesilently inherits the test runner's home directory and surrounding checkout. The fixturetest_engine()insrc/tui_main/render_impl.rscompensates by resetting fields one at a time, reactively, after each leak is discovered the hard way.Four leaks have now been found this way:
git_branchgit::current_branch(cwd)sc_aheadsc_behindsession.explorer_visible~/.config/vimcode/session.jsonThe fourth one cost multiple investigation rounds and produced three separate "verified" wrong root causes (unpinned
unicode-width, rustc toolchain skew, cargo feature unification) before anyone reproduced it — because it is invisible on any machine where a human has ever run vimcode. A fix was written, reviewed, and approved against the first wrong theory before the real cause surfaced.The reactive pattern is at diminishing returns. Each new field read from disk or git inside
Engine::new()is a future CI-only snapshot failure waiting to happen, and the failure mode is maximally expensive: it cannot be reproduced locally without deliberately clobbering$HOME.Proposal
Add a constructor that never touches ambient state:
It should skip
Settings::load(),SessionState::load(),HistoryState::load(), andgit::current_branch()entirely, rather than calling them and overwriting the results afterward — the #615 leak proved that overwriting-after is not always possible.explorer_visiblewas consumed insideEngine::new()to decideapp_shell.hide_sidebar(), so by the time the fixture ran, the decision was already baked intoapp_shelland no field assignment could undo it.Then have
test_engine()call it and drop the accumulated per-field resets.Acceptance
The gate is a clean-home run, since that is the only thing that catches this class:
Ideally this becomes a standing CI job so the next leak is caught by the suite instead of by a multi-day investigation. Worth considering as part of #613's CI work.
Context
Root cause detail and the elimination trail for the four dead hypotheses are pinned on #615. Follow-up to the fix landing there, which patches the fourth leak in the existing reactive style to unblock CI; this issue is the structural fix.