Fix Windows appauthor path doubling and add Windows-specific platform tests

[utkarsha741]

Closes #115

Background

While adding Windows-specific assertions to the platform/cache tests (per #115),
I found a real cross-platform bug: platformdirs was defaulting appauthor to
the app name on Windows, which doubles the path — e.g.
AppData\Roaming\modeldock\modeldock instead of AppData\Roaming\modeldock.

Changes

• src/modeldock/common/platform.py: pass appauthor=False explicitly in
  user_config_dir(), user_cache_dir(), and user_data_dir() to prevent
  the path doubling on Windows.
• tests/unit/test_platform.py (new):
  • Mocked tests that run on any OS, forcing Windows-style platformdirs
    output and verifying our wrappers handle it correctly.
  • Real tests, skipped except on actual Windows CI runners, that exercise
    genuine platformdirs behavior against real APPDATA/LOCALAPPDATA,
    including regression tests for the doubling bug above.

Testing

Full test suite passes locally on Windows: 549 passed, 4 skipped
(skips are pre-existing, Ollama-CLI-dependent integration tests unrelated
to this change).

[himanshu231204]

Thanks for tracking down the doubling bug — the fix direction is right, but I think it's incomplete. A couple of things to fix before merge:

1. system_config_dir() still has the same bug. It calls platformdirs.site_config_dir(app_name()) without appauthor=False (src/modeldock/common/platform.py). site_config_dir has the exact same appauthor default behavior as user_config_dir/user_cache_dir/user_data_dir, so on Windows it will still produce a doubled path (e.g. ProgramData\modeldock\modeldock). Since system_config_dir() is actually used from common/config.py, this isn't just a theoretical gap — please add appauthor=False there too, and add a real-Windows regression test for it alongside the existing ones.

2. The mocked TestWindowsPathsMocked tests don't actually verify the fix. Each mock lambda declares appauthor=False as its own default parameter value, e.g.:

   lambda name, appauthor=False, roaming=True: r"C:\Users\test\AppData\Roaming\modeldock"

   This means the test passes whether or not md_platform.user_config_dir() actually passes appauthor=False to platformdirs — the lambda's own default silently absorbs a missing argument. These tests only check that the return value gets wrapped in a Path, not that the doubling fix is applied. To make them meaningful, assert on the call args, e.g.:

   captured = {}
   def fake_user_config_dir(name, **kwargs):
       captured.update(kwargs)
       return r"C:\Users\test\AppData\Roaming\modeldock"
   monkeypatch.setattr(md_platform.platformdirs, "user_config_dir", fake_user_config_dir)
   md_platform.user_config_dir()
   assert captured["appauthor"] is False

   (or use unittest.mock.Mock/MagicMock and assert on call_args). As written, this bug could regress without any non-Windows CI job catching it — only the real Windows job would catch it, and only for the three functions that were actually fixed.

3. Minor: tests/unit/test_platform.py is missing a trailing newline at EOF.

Since real Windows-only tests are skipped everywhere except the windows-latest CI job, please double check that job is green for this PR (and covers the new system_config_dir case once added).

---

[graphify-labs]

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.

---

Graphify review — findings

Passes appauthor=False to the platformdirs calls in user_config_dir, user_cache_dir, and user_data_dir so paths no longer nest the app name under an author folder (on Windows this stops producing AppData\Roaming\modeldock\modeldock). Adds tests/unit/test_platform.py covering the path wrappers with mocked Windows-style output on any OS, plus Windows-only tests that assert the directories sit under real APPDATA/LOCALAPPDATA, contain modeldock exactly once, and that MODELDOCK_CACHE_DIR still overrides default_cache_dir.

Worth a look

• New test module has unexpected top-level indentation — tests/unit/test_platform.py:1 · Escalate · high
  • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
• Test file has leading indentation on module-level imports causing IndentationError — tests/unit/test_platform.py:1 · Escalate · high
  • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
• test file indented with leading spaces on module-level imports (IndentationError) — tests/unit/test_platform.py:1 · Escalate · high
  • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
• Test file uses invalid indentation on module-level imports — tests/unit/test_platform.py:1 · Escalate · high
  • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
• Windows storage directory contract changes without migration — src/modeldock/common/platform.py:22 · Escalate · medium
  • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 63 functions depend on the 36 functions this change touches.

Health — this change adds coupling hotspots:

• new: load_settings() — 13 callers, 6 callees

Verification — 63 functions in the blast radius were not formally verified this run (proofs are advisory here).

Health delta baseline: last indexed commit 50084a7 (diverged from this PR's base — delta is approximate).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

• verification_scope: 59 function(s) in the blast radius were not formally verified this run

· 1 more finding(s) on lines outside this diff (see the check run).

[himanshu231204]

Code Review Suggestions

The appauthor=False fix in platform.py is correct and targeted. The test file structure (mocked + real-Windows tiers, skipif guard) is exactly the right approach. But there are a few issues to fix before this lands.

---

Critical

1. Module-level imports in test_platform.py are indented 3 spaces — SyntaxError

The first 9 lines of the file have 3 spaces of leading whitespace:

   from __future__ import annotations   # ← IndentationError
   
   import os
   ...

while class TestWindowsPathsMocked: is correctly at column 0. Python raises IndentationError: unexpected indent for indented module-level statements, so pytest cannot collect this file at all. The "549 passed" result in the PR description means none of these tests were ever run.

Fix: remove the leading spaces from lines 1–9.

---

2. Mocked tests don't verify appauthor=False is actually passed

The lambdas accept appauthor=False as a default parameter, so the mock succeeds whether the production code passes the argument or not:

lambda name, appauthor=False, roaming=True: r"C:\Users\test\..."
#                      ^^^^ default — would silently accept missing arg too

This means the mocked tests would still pass even if the appauthor=False fix in platform.py were reverted. To actually test the bug fix, use MagicMock and assert the call arguments:

from unittest.mock import MagicMock

mock_fn = MagicMock(return_value=r"C:\Users\test\AppData\Roaming\modeldock")
monkeypatch.setattr(md_platform.platformdirs, "user_config_dir", mock_fn)
result = md_platform.user_config_dir()
mock_fn.assert_called_once_with("modeldock", appauthor=False, roaming=True)
assert result == Path(r"C:\Users\test\AppData\Roaming\modeldock")

---

Important

3. system_config_dir() has the same doubling bug but was not fixed

platform.py still has:

def system_config_dir() -> Path:
    return Path(platformdirs.site_config_dir(app_name()))
    #                                         ^^^ no appauthor=False

On Windows, platformdirs.site_config_dir("modeldock") with the default appauthor=None (which falls back to the app name) produces C:\ProgramData\modeldock\modeldock — the same doubling as the three functions that were fixed. Should be:

return Path(platformdirs.site_config_dir(app_name(), appauthor=False))

and the TestWindowsPathsMocked.test_system_config_dir_windows lambda needs the same appauthor=False parameter.

---

4. user_data_dir() has no regression test in TestWindowsPathsReal

The fix adds appauthor=False to user_data_dir(), but TestWindowsPathsReal only has regression tests for user_config_dir and user_cache_dir. Add:

def test_user_data_dir_not_doubled(self) -> None:
    result = md_platform.user_data_dir()
    assert str(result).lower().count("modeldock") == 1

---

5. os.environ["APPDATA"] raises KeyError if the variable is absent

TestWindowsPathsReal.test_user_config_dir_under_real_appdata accesses os.environ["APPDATA"] directly. On a well-configured Windows machine this is always set, but a hardened or minimal CI image could be missing it. Prefer:

appdata = os.environ.get("APPDATA", "")
assert appdata and str(result).lower().startswith(appdata.lower())

---

Minor

• TestWindowsPathsReal has no docstring — add one (the class purpose isn't self-evident from the name alone).
• test_paths_use_backslash_separators only checks user_config_dir(). If backslash behaviour matters, checking user_cache_dir() too would add coverage for free.

---

Strengths

• The appauthor=False fix in platform.py is minimal and correct — exactly the right three call sites.
• Splitting into mocked (always-run) and real (Windows-only) test classes is the correct design for cross-platform CI.
• The @pytest.mark.skipif(sys.platform != "win32") at class level avoids repetition.
• The explicit regression test (count("modeldock") == 1) clearly documents what the bug looked like.
• default_cache_dir env-override tests cover both the mocked and real-Windows paths.