prole/docs/plans/junie/todo-1-cfg-save-path-bug.md

# Junie brief — TODO-1: cfg save path leaks `MagicMock` reprs into `conf/k3d.cfg`

> **Self-contained brief.** Filed as a follow-up to commit `dba8a2d`'s
> findings. Single-MR scope. TDD approach — write the failing test
> first, then fix.

---

## 1. Problem

`knoe/ui/screens/cfg.py::ConfigMixin._save_knoe_cfg` reads values from Tk
widget vars via `self.<var>.get()` and writes them to `conf/<mode>.cfg`
(via `_render_knoe_cfg` → `cfg_path.write_text`). When a widget var is a
`unittest.mock.MagicMock` instance instead of a real `tk.StringVar`, the
chain is:

```python
mock_var.get()          # returns another MagicMock
.strip()                # returns another MagicMock
str(...)                # calls MagicMock.__str__ → repr-like string
```

The result: the cfg file gets serialized values like

```ini
KNOE_CONF = <MagicMock name='Canvas().tk.call().strip()' id='4743999712'>
argocd.node_selector = <MagicMock name='mock.StringVar().get().strip()' id='4780691808'>
```

When the installer next reads the file and acts on those values
(`os.makedirs(KNOE_CONF)`, `kubectl apply --context $ctx`, etc.), it
either creates absurdly-named directories on disk, calls subprocesses
with non-existent paths, or does both. We saw the first failure mode
firsthand on 2026-05-02: 10 directories named
`<MagicMock name='Canvas().tk.call().strip()' id='…'>/` appeared in the
repo root with 5–86 install artifacts inside each, and `conf/k3d.cfg`
was corrupted with the mock-strings as values.

Both were cleaned up in `dba8a2d`. **This brief is the durable fix.**

### How the misconfiguration arises

The failure mode is **not in the test suite** — Junie's tests use a
proper `_Var` stub class that returns real strings (see
`tests/installer/test_cfg_save_kubecontext.py::_Var`). The corruption
happened when the user ran `./install.py -c conf/k3d.cfg` interactively
in an environment where the Tk widgets weren't fully initialized
(headless / partial-mock / partial-Tk state — the exact trigger
deserves a small investigation, but the fix should be robust whether
or not we ever reproduce the trigger).

The point: the cfg save path **trusts** that `var.get()` returns a
string. It must not.

## 2. Reproduction (the canonical failing case)

Write this test first, in `tests/installer/test_cfg_save_refuses_mock_values.py`
(new file). Confirm it **fails** before changing any production code.

```python
from __future__ import annotations

from pathlib import Path
from unittest.mock import MagicMock

import pytest

from knoe.ui.screens.cfg import ConfigMixin


class _DummyCfgAppWithMockVar(ConfigMixin):
    """Worst-case input: every widget var is a bare MagicMock.

    This is what happens when an interactive `./install.py -c conf/k3d.cfg`
    run launches the TUI in a context where Tk variables aren't real
    StringVars (headless env, partial mock setup, etc.).
    """
    def __init__(self, conf_dir: Path):
        self._conf_dir = conf_dir
        self._cfg_path_override = conf_dir / "knoe.cfg"
        self._mode = "k3d"
        self._resolved_service_namespace = "knoe-system"
        # Every var is a MagicMock — `var.get()` returns MagicMock,
        # `.strip()` returns MagicMock, `str(...)` returns a repr.
        for name in (
            "cluster_env", "selected_kubectx", "db_username", "db_password",
            "db_namespace", "cnpg_cluster_name", "db_host_port",
            "supabase_pv_node", "supabase_pv_base_dir",
            "gitops_node_selector", "argocd_node_selector",
            "k3s_server_url", "k3s_token", "prod_artifacts_path",
        ):
            setattr(self, name, MagicMock())

    # ConfigMixin needs these helpers; stub them minimally.
    def _secret_cfg_value(self, *_args, **_kwargs):
        return ""

    def _save_ansible_knoe_vault(self, *_args, **_kwargs):
        return None


def test_save_knoe_cfg_refuses_non_string_widget_values(tmp_path):
    """The save path must not serialize MagicMock (or any non-str) values
    into the cfg file. Either raise a clear TypeError, or skip the
    offending field. Silent str()-coercion-to-repr is forbidden.
    """
    app = _DummyCfgAppWithMockVar(tmp_path)
    cfg_path = tmp_path / "knoe.cfg"

    with pytest.raises((TypeError, ValueError)) as excinfo:
        app._save_knoe_cfg()

    # The error must name the offending field/var so the engineer can
    # diagnose without grepping mock-reprs out of files.
    assert "MagicMock" in str(excinfo.value) or "non-string" in str(excinfo.value).lower()

    # The cfg file must NOT contain any MagicMock string-reprs even if
    # save partially succeeded before raising.
    if cfg_path.exists():
        text = cfg_path.read_text()
        assert "<MagicMock" not in text, f"cfg leaked MagicMock repr:\n{text}"
```

This test should fail today: `_save_knoe_cfg` will silently coerce the
mocks via `str()` and write the bad cfg, no exception raised.

A second follow-up test should verify the **real-string path is
unaffected** — re-use the `_Var` stub from
`tests/installer/test_cfg_save_kubecontext.py` (move it to a shared
`tests/installer/conftest.py` if helpful) and assert `_save_knoe_cfg`
still produces a clean cfg file.

## 3. Expected behavior

Two acceptable resolutions; pick whichever has the smaller blast radius.

### Option A — fail loudly at save time (recommended)

A type-validating helper used wherever a widget value is read in
`_save_knoe_cfg`:

```python
def _str_value(var, *, field: str) -> str:
    """Read a Tk var's value and assert it's actually a string."""
    raw = var.get() if hasattr(var, "get") else var
    if not isinstance(raw, str):
        raise TypeError(
            f"cfg field {field!r}: expected str from {type(var).__name__}.get(), "
            f"got {type(raw).__name__} ({raw!r}). "
            "This usually means the Tk widget wasn't initialized properly "
            "(headless env, partial mocking, etc.)."
        )
    return raw
```

Replace every `self.<x>.get()` and `(self.<x>.get() or "").strip()` in
`_save_knoe_cfg` (lines 103–155 in cfg.py) with `_str_value(self.<x>,
field="<X>")` and call `.strip()` on the result if needed. Tests using
the real `_Var` stub keep working unchanged because `_Var.get()`
returns strings. Tests passing `MagicMock` get a clear `TypeError`.

The error must surface BEFORE `cfg_path.write_text(cfg_text)` — i.e.
no partial cfg gets written.

### Option B — validate the assembled `globals_to_save` dict

Less invasive: just before line 277 (`cfg_text = _render_knoe_cfg(...)`),
walk the `inputs` dict and `globals_to_save` dict, raise on any
non-string value, and don't write the file if anything's wrong.

```python
def _validate_cfg_values(name: str, mapping: dict) -> None:
    for k, v in mapping.items():
        if not isinstance(v, str):
            raise TypeError(
                f"cfg {name}: field {k!r} has non-string value "
                f"{type(v).__name__} ({v!r}). Refusing to serialize."
            )

_validate_cfg_values("globals", globals_to_save)
_validate_cfg_values("inputs", inputs)
```

Pros: one validation point. Cons: error message is later in the call
stack and slightly less helpful for debugging the originating widget.

**Pick A** unless the diff for A blows up larger than a screen of
changes — in which case B is fine. Document the choice in the commit.

## 4. Where the bug lives (file pointers)

| File | Lines | What's there |
|---|---|---|
| `knoe/ui/screens/cfg.py` | 44 (`_save_knoe_cfg`) → 277 (`cfg_path.write_text`) | The save flow. The `.get().strip()` pattern is sprinkled across lines 103–155. |
| `knoe/ui/screens/cfg.py` | 102 (`globals_to_save = {...}`) | Where the dict is assembled from widget reads. |
| `tests/installer/test_cfg_save_kubecontext.py` | top | Has the `_Var` stub class. The good pattern — values are real strings. |
| `tests/installer/test_cluster_save_triggers_status_check.py` | (modified by Junie 2026-05-02) | Existing test file; check whether its mocking pattern is similar enough that it could trip the bug if the assertion were broader. |

## 5. TDD approach (Junie convention)

1. **Write the failing test first** at
   `tests/installer/test_cfg_save_refuses_mock_values.py` per §2 above.
   Run pytest, confirm it fails. Capture the failure mode in a one-line
   note for the commit message.
2. **Implement the minimal fix** — Option A or B from §3. Don't refactor
   beyond what the fix needs.
3. **Run pytest** until the new test passes. Crucially: also run the
   full `tests/installer/` suite (≈750 tests) and make sure none
   regressed. The existing `_Var`-based tests should still pass without
   modification.
4. **Refactor only if needed** to keep the tests readable. If you move
   the `_Var` helper to a shared `conftest.py`, fix all the tests that
   imported it inline.
5. **Add a brief README note** in `docs/local-dev-knoe-auth.md` (only
   if the failure is something an engineer is likely to hit) — e.g.
   "If `./install.py -c conf/k3d.cfg` raises `TypeError: cfg field
   'KNOE_CONF': expected str…`, your Tk widgets aren't initialized.
   Common cause: …" — only if there's a generally useful diagnostic
   path to surface; skip otherwise.

## 6. Don't break

- The 750 existing `tests/installer/` tests must still pass. The fix
  should be invisible to any test that uses real strings.
- The existing `_Var` stub (in
  `tests/installer/test_cfg_save_kubecontext.py`) must keep working.
  Don't tighten the validator beyond `isinstance(value, str)` — accept
  any string, including empty.
- The cfg file format must not change. This brief is about input
  validation, not serialization format.

## 7. Definition of done

- [ ] New test `test_cfg_save_refuses_mock_values.py` exists; fails on
  the unmodified production code; passes after the fix.
- [ ] All `tests/installer/` tests pass (750+, currently 24s wall).
- [ ] `_save_knoe_cfg` raises a clear `TypeError` (or `ValueError`) on
  any non-string widget value, naming the offending field. No partial
  cfg gets written.
- [ ] Manual sanity: from a Python repl,
  ```python
  from unittest.mock import MagicMock
  from knoe.ui.screens.cfg import ConfigMixin
  app = type("X", (ConfigMixin,), {})()
  app.cluster_env = MagicMock()
  ...
  app._save_knoe_cfg()  # → TypeError naming 'cluster_env' (or whatever)
  ```
- [ ] No `<MagicMock` string ever appears in any committed cfg file.

## 8. Commit shape

```
fix(installer): cfg save refuses non-string widget values

When `./install.py -c conf/k3d.cfg` runs in a non-Tk context (or with
partially-mocked widgets), `ConfigMixin._save_knoe_cfg` previously
called `var.get()` on each widget and serialized the result as-is. If
the var was a MagicMock, `str()` produced repr-strings like
`<MagicMock name='Canvas().tk.call().strip()' id='4743999712'>` which
got written into `conf/k3d.cfg` and then turned into directory names
on the next install pass. Cleaned up in dba8a2d; this is the durable
fix.

  knoe/ui/screens/cfg.py     _str_value() helper validates each widget
                             read; non-str values raise TypeError naming
                             the offending field. No partial cfg gets
                             written.

  tests/installer/test_cfg_save_refuses_mock_values.py  NEW; failing
                             test that produced the original repro
                             (MagicMock vars across the board); passes
                             after the fix.

All 750+ installer tests still pass. Closes the followup flagged in
dba8a2d.
```

## 9. Out of scope

- **Investigating why Tk vars were MagicMocks during interactive
  runs.** Probably a partial-init path in `KnoeInstaller` when run
  outside a real X / Aqua display, or the `mock` test harness leaking
  into a real run. Worth a separate followup; this brief is purely
  defensive at the save boundary.
- **Schema/validation for cfg keys themselves.** Whether
  `argocd.node_selector` should be a node-selector string vs an empty
  string is a different question. We're only asserting "is a string."
- **Refactoring `_save_knoe_cfg`** beyond the fix. The function is long
  but works; restructuring it is a different MR.