docs(plans): file todo-1 brief — cfg save path refuses non-string widget values

Followup from commit dba8a2d's findings. ConfigMixin._save_knoe_cfg in knoe/ui/screens/cfg.py reads Tk widget vars via `var.get()` and writes the result to conf/<mode>.cfg. When a var is a MagicMock (interactive ./install.py run in a non-Tk context, partially-mocked widgets), the save path serializes the mock's repr-string into the cfg, then the next installer pass calls os.makedirs() on those values and produces directories literally named `<MagicMock name='Canvas().tk.call().strip()' id='4743999712'>/`. The brief lays out a TDD approach for Junie: 1. Write failing test at tests/installer/test_cfg_save_refuses_mock_values.py that passes MagicMock widget vars and asserts _save_knoe_cfg raises TypeError naming the field. 2. Implement the minimal fix: a `_str_value(var, field=...)` helper in cfg.py that validates widget reads and raises if non-str. Use it in the .get()/.strip() chains across lines 103-155. 3. Verify the 750 existing installer tests still pass. Brief includes file pointers (cfg.py:44 _save_knoe_cfg, line 102 globals_to_save assembly, line 277 cfg_path.write_text), the canonical failing test stub, both fix-approach options (per-read validator vs end-of-flow dict walk), and explicit commit-shape guidance. Index updates: docs/plans/junie/README.md — todo-1 row added under Active docs/TODO.md §"In progress" — todo-1 promoted above Phase 3 (smaller scope, easy to land first) Naming convention: `todo-N-<slug>.md` for follow-up bugs, distinct from the `NN-<slug>.md` pattern reserved for ranked queue items.
2026-09-23 11:03:59 +00:00 · 2026-05-05 20:43:44 -04:00 · 2026-05-05 20:43:44 -04:00 · 6f506a97b2
commit 6f506a97b2
parent 296bcd15a5
3 changed files with 293 additions and 0 deletions
--- a/docs/TODO.md
+++ b/docs/TODO.md
@ -13,6 +13,8 @@ The Kanban "Now" section at top is the only place this doc imposes structure. Ev
 ## Now (Kanban)

 ### In progress
+- **TODO-1 — cfg save path refuses non-string widget values** — assigned to Junie. Followup from commit `dba8a2d`'s findings. `ConfigMixin._save_knoe_cfg` in `knoe/ui/screens/cfg.py` calls `var.get()` on Tk widget vars and serializes the result; if a var is a `MagicMock` (interactive run in a non-Tk context, headless env, partial mock setup, etc.), the save path writes `<MagicMock name='Canvas().tk.call().strip()' id='…'>` strings into `conf/<mode>.cfg`. The next installer pass then `os.makedirs()` on those values, producing absurdly-named directories on disk. Saw it firsthand on 2026-05-02; cleaned up in `dba8a2d`. This brief is the durable fix: TDD-style failing test first, then a `_str_value()` validator in the save path that raises `TypeError` on non-str widget reads. Brief: [`docs/plans/junie/todo-1-cfg-save-path-bug.md`](plans/junie/todo-1-cfg-save-path-bug.md).
+
 - **k3d-mirror-of-GKE Phase 3 — knoe-auth as a pod inside k3d** — assigned to Junie. Pre-merge smoke loop: build the `knoe-auth:latest` image (using the `make docker-build-auth` target shipped in commit `903f84f`), import it into the k3d cluster (`k3d image import`), apply a k3d-flavoured `Deployment` manifest (sibling of `deploy/gcp/gke/knoe-auth-deployment.yaml`, but with `imagePullPolicy: Never` and realm `KNOE.LOCAL`), and run the keytab-bootstrap initContainer + KDC sidecar pattern in-cluster. New `make k3d-knoe-{deploy,redeploy,undeploy}` targets. Phase 2 OIDC signing key flows from `etc/secrets/knoe-auth-oidc-key.b64` into a K8s Secret (`knoe-auth-oidc-signing-key` in `knoe-system`) so the in-cluster pod gets it the same way GKE does. Brief: [`docs/plans/junie/k3d-knoe-auth-pod-deploy.md`](plans/junie/k3d-knoe-auth-pod-deploy.md). Architectural plan: [`docs/plans/k3d-gke-mirror.md`](plans/k3d-gke-mirror.md) §6 Phase 3.

 ### Paused
--- a/docs/plans/junie/README.md
+++ b/docs/plans/junie/README.md
@ -51,6 +51,7 @@ in TODO" note). When Junie lands a brief:
 | File | Tracked at | Subject |
 |---|---|---|
 | [`k3d-knoe-auth-pod-deploy.md`](k3d-knoe-auth-pod-deploy.md) | TODO §"In progress"; parent [`../k3d-gke-mirror.md`](../k3d-gke-mirror.md) §6 Phase 3 | Phase 3 of k3d-mirror-of-GKE: build the knoe-auth image, `k3d image import`, run as a pod inside the cluster. Pre-merge smoke loop with `make k3d-knoe-{deploy,redeploy,undeploy}`. |
+| [`todo-1-cfg-save-path-bug.md`](todo-1-cfg-save-path-bug.md) | followup from commit `dba8a2d` | TDD fix: `ConfigMixin._save_knoe_cfg` must refuse to serialize non-string widget values into `conf/<mode>.cfg`. Currently leaks `<MagicMock …>` reprs when widgets aren't real Tk StringVars. |

 ### Shipped (kept as design record)

--- a/docs/plans/junie/todo-1-cfg-save-path-bug.md
+++ b/docs/plans/junie/todo-1-cfg-save-path-bug.md
@ -0,0 +1,290 @@
+# 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.