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:

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

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.

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:

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.

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,

  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.