test(store-types): reach the HOME tier on every platform, cover APPDATA on Windows

[iceteaSA]

Fixes #17. Test-only: Windows CI has been red on master since ecaec0f.

What was wrong

resolve_data_home_path has four tiers — XDG_DATA_HOME, then APPDATA and USERPROFILE under #[cfg(windows)], then HOME + .local/share. Two tests set HOME and assert the tier-4 result, but a Windows runner always has APPDATA set, so tier 2 fires and tier 4 is never reached.

The resolver is correct and this change does not touch it. The subc seat confirmed the daemon's default_data_home at source: same four tiers, same order, XDG_DATA_HOME outranking APPDATA even on Windows. Removing the #[cfg(windows)] block — which the issue's original framing left open as a possibility — would have created the divergence this crate exists to prevent.

The forward-slash format! joining in module_data_dir/sqlite_store_path is also untouched. It looks like a POSIX assumption and is deliberate byte-identity with the daemon's wire descriptors; a PathBuf::join there would break byte-match with the daemon and with every store already on disk.

What changed

1. The HOME-tier tests now reach the HOME tier on every platform. Under #[cfg(windows)] they clear and restore APPDATA and USERPROFILE so tier 4 is actually exercised, using the same ENV_LOCK discipline the module already has. The assertions are unchanged — they were always testing the right thing, just only where the Windows tiers happen not to exist.

2. Tier 2 gains its first test. module_store_path_prefers_appdata_over_home sets both APPDATA and HOME and asserts the APPDATA-derived result wins. That is the tier every real Windows deployment uses and it had no coverage at all — which is why nothing caught the precedence mismatch from the other direction either.

Gating the two failing tests behind #[cfg(not(windows))] would also turn CI green, and would leave that hole open. Both parts land together for that reason.

68 lines added, 0 removed, confined to the resolver_tests module.

Verification, and one honest gap

Locally verified on Linux: cargo test -p cortexkit-store-types green, cargo clippy -- -D warnings clean, cargo fmt --check clean.

The Windows behaviour is not locally verified. This box has no rustup and no Windows target installed, so I could not even cross-compile-check the new #[cfg(windows)] test. CI is the only thing that can prove either part works there, and I would rather say so than imply I tested it. If the Windows job is still red after this, the diagnosis in #17 is right but my fix for it is not, and I will take another pass.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Exercises the HOME tier in resolver tests on Windows and adds APPDATA precedence coverage; updates Windows assertions to match platform separators. Stabilizes Windows CI without changing resolver behavior.

• In HOME-tier tests, capture and restore APPDATA and USERPROFILE, clear them to reach HOME, using ENV_LOCK.
• Adjust Windows assertions to expect platform separators (PathBuf on data-home; forward slashes in module paths).
• Add Windows-only test module_store_path_prefers_appdata_over_home asserting APPDATA outranks HOME.
• No changes to resolver or path-joining logic; tests only (+85 lines in crates/cortexkit-store-types/src/lib.rs).

^{Written for commit 634a2d4. Summary will update on new commits.}

[iceteaSA]

634a2d4 — second round. The first commit did its job and uncovered a second problem behind it.

What the first commit proved. The Windows failure moved from C:\Users\runneradmin\AppData\Roaming/... to /tmp/home-test..., so clearing APPDATA/USERPROFILE reached tier 4 as intended. And module_store_path_prefers_appdata_over_home passed on Windows — tier 2 has real coverage now, which was the half of this PR that could only be confirmed there.

What was still red, and why nothing in the resolver should move.

left:  "/tmp/home-test\.local\share/cortexkit/m/store.db"
right: "/tmp/home-test/.local/share/cortexkit/m/store.db"

Tier 4 is PathBuf::from(home).join(".local").join("share"), and PathBuf::join inserts backslashes on Windows. The module path is appended by the forward-slash format! afterwards. So the genuine Windows result is mixed — backslashes inside .local\share, forward slashes after it.

The daemon's default_data_home uses the identical PathBuf::join for that tier, so both sides produce the same mixed string and byte-identity holds. Making either side uniform on its own would break the match — the same hazard as the forward-slash joining, one layer deeper. If that shape ever changes it needs the daemon and this crate in a single commit, which is not this PR's business.

So the assertions were wrong, not the code. They now select platform-specific literals:

#[cfg(not(windows))]
assert_eq!(got, "/tmp/home-test/.local/share/cortexkit/astrocyte/store.db");

#[cfg(windows)]
{
    // PathBuf joins the data-home tier while module paths retain the
    // daemon's forward slashes, so this mixed form is byte-identical.
    assert_eq!(got, "/tmp/home-test\\.local\\share/cortexkit/astrocyte/store.db");
}

Literals rather than rebuilding the expectation with PathBuf::join — a test that constructs its expected value the way the code does cannot catch a change in that construction. The comment is there because the next person to read that string will otherwise file it as a bug, which is exactly how byte-identity gets broken by someone being helpful.

85 lines added, zero removed, across both commits. No production function touched.

Locally: tests, clippy -D warnings, and fmt --check all green on Linux. The Windows assertions still cannot run here — no rustup, no Windows target — so CI remains the only proof, same caveat as the first round. It earned that caveat once already.

[cubic-dev-ai]

1 issue found and verified against the latest diff

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/cortexkit-store-types/src/lib.rs">

<violation number="1" location="crates/cortexkit-store-types/src/lib.rs:340">
P2: On Windows these tests now reach the HOME tier (the new APPDATA/USERPROFILE removal forces it), but they still assert forward-slash paths like "/tmp/home-test/.local/share/...". On Windows the resolver's HOME branch does `PathBuf::from(home).join(".local").join("share")`, and PathBuf joins use the platform separator `\`. The crate's own golden fixture pins this: `windows_empty_appdata_userprofile_falls_to_home` → `C:/golden-home\.local\\share`. So on Windows `module_store_path` returns a string containing a backslash-joined `.local\share` component (e.g. `\tmp\home-test\.local\share/cortexkit/...`), which never equals the asserted forward-slash string — the asserted changed tests (`module_store_path_defaults_to_home_local_share` and `empty_env_values_count_as_unset`) will still fail on Windows CI. The fix needs to assert a Windows-aware expected string (or gate the forward-slash expectation off Windows) to match the golden/daemon contract.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: On Windows these tests now reach the HOME tier (the new APPDATA/USERPROFILE removal forces it), but they still assert forward-slash paths like "/tmp/home-test/.local/share/...". On Windows the resolver's HOME branch does PathBuf::from(home).join(".local").join("share"), and PathBuf joins use the platform separator \. The crate's own golden fixture pins this: windows_empty_appdata_userprofile_falls_to_home → C:/golden-home\.local\\share. So on Windows module_store_path returns a string containing a backslash-joined .local\share component (e.g. \tmp\home-test\.local\share/cortexkit/...), which never equals the asserted forward-slash string — the asserted changed tests (module_store_path_defaults_to_home_local_share and empty_env_values_count_as_unset) will still fail on Windows CI. The fix needs to assert a Windows-aware expected string (or gate the forward-slash expectation off Windows) to match the golden/daemon contract.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/cortexkit-store-types/src/lib.rs, line 340:

<comment>On Windows these tests now reach the HOME tier (the new APPDATA/USERPROFILE removal forces it), but they still assert forward-slash paths like "/tmp/home-test/.local/share/...". On Windows the resolver's HOME branch does `PathBuf::from(home).join(".local").join("share")`, and PathBuf joins use the platform separator `\`. The crate's own golden fixture pins this: `windows_empty_appdata_userprofile_falls_to_home` → `C:/golden-home\.local\\share`. So on Windows `module_store_path` returns a string containing a backslash-joined `.local\share` component (e.g. `\tmp\home-test\.local\share/cortexkit/...`), which never equals the asserted forward-slash string — the asserted changed tests (`module_store_path_defaults_to_home_local_share` and `empty_env_values_count_as_unset`) will still fail on Windows CI. The fix needs to assert a Windows-aware expected string (or gate the forward-slash expectation off Windows) to match the golden/daemon contract.</comment>

<file context>
@@ -330,9 +330,29 @@ mod resolver_tests {
         std::env::remove_var("XDG_DATA_HOME");
+        #[cfg(windows)]
+        {
+            std::env::remove_var("APPDATA");
+            std::env::remove_var("USERPROFILE");
+        }
</file context>

[iceteaSA]

Windows is green. All four checks pass on 634a2d4 — ubuntu, macOS, Windows, and the live Postgres backend.

That is the part I could not verify locally, so recording the outcome rather than leaving the PR asserting an untested claim:

tier	covered where	verified
2 — APPDATA on Windows	new test, #[cfg(windows)]	CI (had no coverage before this PR)
4 — HOME + .local/share	existing tests, now reached on every platform	CI + local

Both halves needed Windows to prove anything. The tier-2 test cannot run anywhere else, and the tier-4 tests were passing on Linux the whole time while asserting something that was never exercised there.

Master is still red until this merges — the two failures on master are the same two tests, and PR #14's Windows check is inheriting them through its merge commit rather than from anything in that branch.

One note for whoever reads the diff later: the #[cfg(windows)] literal

"/tmp/home-test\\.local\\share/cortexkit/astrocyte/store.db"

is correct and load-bearing. PathBuf::join produces the backslashes in the data-home tier, the module path keeps forward slashes, and the daemon's default_data_home does the identical thing — so that mixed shape is what byte-identity with the wire descriptor requires. Normalising it on either side alone breaks the match with the other and with every store already on disk.

[ualtinok]

Reviewed and merging. The substance is exactly right: the two HOME-tier tests never controlled APPDATA/USERPROFILE, which Windows runners always set — so the resolver's (correct) Windows preference shadowed the tier the tests meant to reach, and master went red on a platform the authoring machine cannot run. Your save/clear/restore under the existing lock is the right shape, the mixed-separator assertion comment (PathBuf joins the data-home tier, module paths keep the daemon's forward slashes) documents a byte-level fact the next reader would otherwise misread as a bug, and the APPDATA-preference test closes the arm that had no coverage at all — the missing-coverage half of this incident, since a Windows-arm test would have caught the original commit at authoring time. One note for the record rather than a change request: the save/restore boilerplate now appears three times; if a fourth env-sensitive test lands, an RAII guard is the move (restore-on-drop also survives assert panics, which the manual restores do not — a failing assertion currently leaks the cleared vars into later tests on the same thread). Not blocking on it. This unblocks the cortexkit-provider-usage-v0.7.0 publish, which failed precisely because your target was red — the tag rides the merge.

[iceteaSA]

Thanks for merging, and for the RAII note — the conclusion is right and I want to correct the reason, because the version in the note would send someone hunting for a bug that is not there.

Assert panics do not currently leak. Every env-sensitive test in the module, including the two I touched and the one I added, follows the same order: mutate, capture the result, restore, then assert. Checked against the merged file at 4a83b80:

test	sequence
module_store_path_honours_xdg_data_home	E E A
module_store_path_defaults_to_home_local_share	E×6 A A
relative_xdg_data_home_is_honored_matching_the_daemon	E E E A
empty_env_values_count_as_unset	E×7 A A
module_store_path_prefers_appdata_over_home	E×8 A

E = env write, A = assertion. No assertion precedes a restore anywhere, so a failing assert unwinds with the environment already put back. That ordering is the module's existing idiom — let got = module_store_path(...) on its own line before the restore — and I followed it rather than introducing it.

There is still a real leak window, and RAII still closes it. If the code under test panics — module_store_path itself, between the set_var and the restore — the restore is skipped and the cleared vars leak to later tests on the same thread. Narrow, since that function is currently panic-free, but it is exactly the case a drop guard covers and manual restores cannot. So: right prescription, different hazard.

Your trigger is also already met, by a count. You read three instances of the boilerplate; there are five env-sensitive tests, three of them carrying the full save/clear/restore. The fourth-test condition has effectively arrived.

Happy to send the guard as a follow-up if you want it — struct EnvGuard(Vec<(&'static str, Option<String>)>) with restore in Drop, replacing the manual blocks and folding the ENV_LOCK acquisition into the same guard so the two cannot be separated by accident. Test-only, no production change, same shape as this PR. Say the word; not assuming it.

Good to hear it unblocks the cortexkit-provider-usage v0.7.0 tag — I had not connected the red target to a blocked publish, and that makes the "authoring machine cannot run the failing platform" gap more expensive than it looked from the CI page.