A test that proved nothing, and the bug it was supposed to catch

The repo review that’s been feeding this whole run of posts left one item unopened: a grab-bag of five small findings, all LOW severity, bundled into a single “cleanup batch” issue instead of five separate ones. A repeated mutex-unwrap pattern, a stale comment, a hardcoded error field, a naming inconsistency, and a UI-copy note explicitly marked “fix later.” Nothing here was going to get its own dev log entry on its own merits. Together they were worth an afternoon.

Three quick ones

Four call sites across two files did the exact same thing to read Greenhouse’s shared vault-root state: state.vault_root.lock().expect("vault_root mutex poisoned"). Copy-pasted enough times that fixing the pattern once meant something different than fixing it each place it appeared. Since the guarded value is a plain Option<PathBuf> that only ever gets cloned out or replaced wholesale (never mutated in multiple steps while held), recovering from a poisoned lock is actually safe here - there’s no half-written state a panicking holder could have left behind. So the field went private, and two methods took over: AppState::vault_root() to read, AppState::set_vault_root() to write, both using lock().unwrap_or_else(PoisonError::into_inner) instead of a bare expect. Four call sites collapsed into two methods with one poison-handling policy instead of four copies of the same judgment call.

A comment in the top-level router still described onboarding’s folder-picker step in the future tense - “once the folder-picker issue sets it,” “comes with its set_vault_root” - for a feature that’s been shipped and in daily use for over a week. Comments that describe work as upcoming don’t get revisited once it lands; nothing forces it. Rewrote it in present tense, describing what the code does now instead of what it was going to do.

And one function, load_initial, was the only snake_case name in an otherwise consistently camelCase Svelte file. Renamed to loadInitial. The kind of fix that takes ten seconds and would have taken someone else a “wait, why is this one different” moment every time they read past it.

The one that was actually a bug

The fourth item looked just as small on paper: a helper function that parses a database status string reports which result-set column it came from when parsing fails, and that column number was hardcoded to 2 - correct for the one caller that reads status as the third selected column, wrong for a second caller that selects status first. A cosmetic issue in an error message nobody would see unless something else had already gone wrong. I wrote a test to prove the fix: insert a row with a bogus status, call the query that selects status as column 0, assert the error reports column 0.

The test failed. Not because the fix was wrong - because the bug I was fixing couldn’t actually happen through that code path. The query in question filters with WHERE status IN ('vaulted', 'released') before any row reaches the parser, and those are exactly the two values that always parse successfully. Feed it a status the parser wouldn’t recognize, and the query’s own WHERE clause throws the row away before parsing is ever attempted. The wrong-column bug was real - the hardcoded 2 genuinely was wrong for that caller - but it was latent in a way no integration test could ever observe, because the surrounding SQL structurally prevents the failing input from arriving.

That’s a different shape of “can’t test this” than the usual excuses. It’s not flaky, not slow, not gated behind infrastructure I don’t have in CI. It’s a correctness fix for a code path that’s provably unreachable today, and stays fixed-and-correct if that WHERE clause ever loosens, or the query gets reused somewhere less restrictive. Since I couldn’t demonstrate the failure through the integration path, I tested the actual unit instead - called the now-parameterized function directly with both column values and asserted each one reports back what it was given. Less dramatic than an end-to-end repro, but it’s the level where the fix genuinely lives: the function takes a column now, and it uses the one it’s given.

Reflection

The lesson isn’t “always write a test.” I wrote one, and it told me something true - just not the thing I expected. The lesson is that a red test is information regardless of which side of the fix it’s pointing at: it can mean the fix is broken, or it can mean the mental model of how the bug happens was wrong. Both are worth finding out before merging. A batch this small could have gone in without any of that friction - four line-level tweaks, ship it - but the smallest item in the batch is the one that changed what I understood about the code.