phase: 173_docs_push_creds_split

All gates green. Final report:

**Phase 173 — final verification pass (all tasks already complete in `complete/`)**

- Verified the full implementation is in the working tree: `docs_user`/`docs_token` settings + catalogue rows (task 01), `docs_repo` PUT 422 guard + `docs_token` write-only pin (task 02), `run_git(env)` + `push_document(user, token)` credential env (task 03), `settings_migrate.py` wired into `create_app()` (task 04), E2E suite (task 05).
- **Completion criteria:**
  1. GET `/api/admin/settings` lists `docs_user` (plain) + `docs_token` (secret, `value=null`), token never in any body — ✅ `test_docs_token_is_write_only_masked_end_to_end` + E2E sentinel-absent-from-DOM
  2. PUT `docs_repo` with `://user:pass@` → 422 naming the fields; clean URL passes — ✅ integration + E2E UI-side
  3. Token set → `GIT_CURL_USERNAME`/`GIT_CURL_PASSWORD`/`GIT_TERMINAL_PROMPT=0` on every git step; empty → `env=None` — ✅ mocked-git unit tests
  4. Legacy env split once at startup, idempotent, one INFO line, `.env` untouched — ✅ `test_settings_migrate.py` (4 shapes) + `test_docs_push_migrate.py`
  5. Full suite green, coverage >90%, lint clean — ✅
  6. E2E isolated: `test_docs_push_creds.py` 4/4; regression set (panel + layout + response_to_docs) 21/21 — ✅
  7. Commit + phase move — harness's job (per instructions, not mine).
  8. README/AGENTS consistent — ✅ (below)
- Tests/lint: `uv run pytest` exit 0 (4112 passed) · `uv run pytest --cov=app --cov-report=term-missing` TOTAL 99% · `uv run ruff check .` clean · `uv run pyright` 0 errors
- Docs: README.md (incl. Docs-push config section, guard/migration paragraph, `.env.example` block) and AGENTS.md verified consistent in full; fixed two stale docstrings in `app/config.py` + `app/core/settings_catalog.py` (`is_set` → the API's actual `has_value` field).
- No live-infrastructure changes (no live step in this phase). No next pending phase — `todo/` held only 173.

tests/integration/test_admin_settings_api.py

@@ -9,7 +9,7 @@ The pins (00_phase, Testing & Quality):
 * the auth matrix — anonymous AND a signed-in token USER get 403 on ALL
   four routes (the router-wide ``require_admin`` closes the surface on
   every method), the admin 200s on all four;
-* the shape — ``items`` length == ``len(Settings.model_fields)`` (75),
+* the shape — ``items`` length == ``len(Settings.model_fields)`` (78),
   groups in ``GROUPS`` order (the stable order the E2E pins against),
   ``totals.count`` / ``overridden`` correct, ``overlay_enabled`` true;
   on an empty table a field whose ``BOR_*`` var the test env sets
@@ -152,7 +152,7 @@ def test_auth_matrix_all_four_routes(client: TestClient, db: Session) -> None:
 
 
 def test_catalogue_shape_and_sources_on_empty_table(admin_client) -> None:
-    """The whole catalogue, grouped and stable: 75 items (exactly
+    """The whole catalogue, grouped and stable: 78 items (exactly
     ``Settings.model_fields``), groups in ``GROUPS`` order (each group a
     contiguous block, the block order = ``GROUPS``), ``totals`` correct,
     ``overlay_enabled`` true. On an empty table: the env-var-set field
@@ -418,7 +418,7 @@ def test_kill_switch_disables_the_mechanism_not_the_panel(
 def test_catalogue_stays_under_the_byte_budget(admin_client) -> None:
     """The whole catalogue stays under the phase-167 INFO bar (64 KB —
     ``BOR_LOG_LARGE_RESPONSE_BYTES``), so ``GET /api/admin/settings``
-    never floods the log (75 items ≈ 20 KB; the audit script re-verifies
+    never floods the log (78 items ≈ 20 KB; the audit script re-verifies
     the 256 KB budget in task 06)."""
     r = admin_client.get(API)
     assert r.status_code == 200
@@ -602,3 +602,166 @@ def test_llm_thinking_level_row_contract(
     anon = TestClient(fastapi_app)
     r = anon.put(f"{API}/llm_thinking_level", json={"value": "high"})
     assert r.status_code == 403 and r.json() == {"detail": "admin only"}
+
+
+def test_docs_token_is_write_only_masked_end_to_end(
+    admin_client: TestClient, db: Session, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Phase 173 (task 02, C1): ``docs_token`` is a write-only
+    ``secret=True`` row — the EXISTING generic secret contract pinned
+    for the new key (no new code): (a) before a save the GET reports
+    the row in the ``Docs push`` group, ``secret=true``,
+    ``value=null`` (masked) and ``has_value=false`` — with
+    ``docs_user`` a plain ``str`` row at ``""``; (b) a PUT stores it
+    (200) and the GET reports ``has_value=true`` with the plaintext
+    NOWHERE in the raw body — the core pin: the token text is absent
+    from the response JSON, the leak class this phase kills; (c) a
+    blank PUT 422s (the self-lockout guard — ``DELETE`` is the
+    explicit clear) and the stored value is unchanged; (d) a DELETE
+    unsets it (reset to the env value — ``""`` by default).
+    """
+    # A deterministic base (machine-independent, the phase-172 test's
+    # idiom): an explicit empty process value for both docs
+    # credentials — the process env RANKS ABOVE the operator's local
+    # ``.env`` (the conftest pin's reasoning), so this pins the code
+    # defaults no matter what the machine's ``.env`` carries.
+    monkeypatch.setenv("BOR_DOCS_USER", "")
+    monkeypatch.setenv("BOR_DOCS_TOKEN", "")
+    invalidate_settings()
+
+    # (a) the rows: docs_token masked + unset, docs_user plain + empty.
+    r = admin_client.get(API)
+    assert r.status_code == 200, r.text
+    item = _item(r.json(), "docs_token")
+    assert item["group"] == "Docs push"
+    assert item["label"] == "Docs token"
+    assert item["type"] == "str"
+    assert item["env_var"] == "BOR_DOCS_TOKEN"
+    assert item["secret"] is True
+    assert item["restart_required"] is False
+    assert item["overridable"] is True
+    assert item["masked"] is True
+    assert item["value"] is None
+    assert item["db_value"] is None
+    assert item["has_value"] is False
+    assert item["has_override"] is False
+    assert item["default"] == ""  # masked: the code default is never shown
+    user = _item(r.json(), "docs_user")
+    assert user["group"] == "Docs push"
+    assert user["label"] == "Docs user"
+    assert user["secret"] is False
+    assert user["masked"] is False
+    assert user["value"] == ""
+    assert user["has_value"] is False
+
+    # (b) a PUT stores it (200, the RAW string in app_settings) and the
+    # GET reports has_value=true with the plaintext NOWHERE in the raw
+    # body (asserted on response.text, not the parsed model).
+    r = admin_client.put(f"{API}/docs_token", json={"value": "bor-docs-tok-123"})
+    assert r.status_code == 200, r.text
+    assert r.json()["masked"] is True
+    assert r.json()["value"] is None
+    assert r.json()["has_value"] is True
+    assert r.json()["source"] == "db"
+    assert db.execute(
+        text("SELECT value FROM app_settings WHERE key = 'docs_token'")
+    ).scalar() == "bor-docs-tok-123"  # stored, exactly as sent
+    assert get_settings().docs_token == "bor-docs-tok-123"  # live, no restart
+
+    r = admin_client.get(API)
+    assert r.status_code == 200
+    item = _item(r.json(), "docs_token")
+    assert item["has_value"] is True
+    assert item["value"] is None
+    assert item["db_value"] is None
+    assert "bor-docs-tok-123" not in r.text  # NOWHERE in the raw body
+
+    # (c) a blank PUT is a 422 (a blank token would self-lockout —
+    # DELETE is the explicit clear) and the stored value is unchanged.
+    r = admin_client.put(f"{API}/docs_token", json={"value": "   "})
+    assert r.status_code == 422
+    assert "BOR_DOCS_TOKEN must not be blank" in r.json()["detail"]
+    assert db.execute(
+        text("SELECT value FROM app_settings WHERE key = 'docs_token'")
+    ).scalar() == "bor-docs-tok-123"  # unchanged
+
+    # (d) a DELETE unsets it: the row is gone and the effective value is
+    # back to the env value (the pinned "" — the anonymous state, C5).
+    r = admin_client.delete(f"{API}/docs_token")
+    assert r.status_code == 200, r.text
+    assert r.json()["source"] == "env"  # the pinned empty BOR_DOCS_TOKEN
+    assert r.json()["has_override"] is False
+    assert r.json()["has_value"] is False
+    assert get_settings().docs_token == ""  # the override is gone
+    assert db.execute(
+        text("SELECT count(*) FROM app_settings WHERE key = 'docs_token'")
+    ).scalar() == 0  # the row is gone
+
+
+def test_docs_repo_refuses_embedded_credentials(
+    admin_client: TestClient, db: Session, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Phase 173 (task 02, C4): a ``docs_repo`` PUT with a
+    ``://<user>:<pass>@`` segment is a 422 that NAMES the Docs user /
+    Docs token fields (asserted in the detail, not just present) and
+    stores NOTHING — the leak cannot be re-created from the UI. Clean
+    ``https://`` URLs, local paths and ``ssh://`` remotes pass
+    untouched; a ``user:``-looking subdomain (no trailing ``@``) does
+    NOT false-positive. A legacy ``.env`` value with embedded
+    credentials is NOT a PUT — the task-04 startup migration handles
+    that path."""
+    # A deterministic base: explicit empty/valid process values for the
+    # docs settings — the process env ranks above the operator's local
+    # ``.env`` (the conftest pin's reasoning), so the base is the code
+    # defaults no matter what the machine's ``.env`` carries (a local
+    # ``BOR_DOCS_REPO`` with embedded credentials is EXACTLY the leak
+    # class under test — it must not leak into this pin).
+    monkeypatch.setenv("BOR_DOCS_REPO", "")
+    monkeypatch.setenv("BOR_DOCS_BRANCH", "bor-docs")
+    invalidate_settings()
+
+    # The 422: names the replacement fields, stores nothing.
+    r = admin_client.put(
+        f"{API}/docs_repo", json={"value": "https://u:pass@gitea.example.com/o/r.git"}
+    )
+    assert r.status_code == 422
+    detail = r.json()["detail"]
+    assert "Docs user" in detail
+    assert "Docs token" in detail
+    assert db.execute(text("SELECT count(*) FROM app_settings")).scalar() == 0
+    # The failed write did not disturb the live view.
+    assert get_settings().docs_repo == ""
+
+    # Clean https:// URL: passes (the branch validator is satisfied by
+    # the non-empty docs_branch default — the repo is set, the token
+    # shape is valid).
+    r = admin_client.put(f"{API}/docs_repo", json={"value": "https://gitea.example.com/o/r.git"})
+    assert r.status_code == 200, r.text
+    assert r.json()["db_value"] == "https://gitea.example.com/o/r.git"
+    assert get_settings().docs_repo == "https://gitea.example.com/o/r.git"
+    assert admin_client.delete(f"{API}/docs_repo").status_code == 200
+
+    # A local path: untouched.
+    r = admin_client.put(f"{API}/docs_repo", json={"value": "/srv/docs-repo"})
+    assert r.status_code == 200, r.text
+    assert r.json()["db_value"] == "/srv/docs-repo"
+    assert admin_client.delete(f"{API}/docs_repo").status_code == 200
+
+    # A ``user:``-looking subdomain: the trailing ``@`` requirement keeps
+    # the regex from false-positiving — passes untouched.
+    r = admin_client.put(
+        f"{API}/docs_repo", json={"value": "https://sub.domain.example.com/r.git"}
+    )
+    assert r.status_code == 200, r.text
+    assert r.json()["db_value"] == "https://sub.domain.example.com/r.git"
+    assert admin_client.delete(f"{API}/docs_repo").status_code == 200
+
+    # An ssh:// remote (credentials via SSH keys, never in the URL):
+    # untouched.
+    r = admin_client.put(
+        f"{API}/docs_repo", json={"value": "ssh://git@gitea.example.com/o/r.git"}
+    )
+    assert r.status_code == 200, r.text
+    assert r.json()["db_value"] == "ssh://git@gitea.example.com/o/r.git"
+    assert admin_client.delete(f"{API}/docs_repo").status_code == 200
+    assert db.execute(text("SELECT count(*) FROM app_settings")).scalar() == 0

tests/integration/test_docs_push_migrate.py

@@ -0,0 +1,203 @@
+"""Integration: the phase-173 (task 04, C3) legacy-credentials
+migration, end-to-end against the real ``app_settings`` table.
+
+The shape (the 00_phase / task-04 pins):
+
+* seed env ``BOR_DOCS_REPO=https://u:pass@host/r.git`` (no DB rows)
+  → run the migration (the direct call — the ``create_app()`` wiring
+  is covered by the unit seam in ``test_main.py``) → the migration
+  returns ``True`` and the three rows land in ``app_settings``
+  (clean repo / ``docs_user`` / ``docs_token``);
+* DB rows SHADOW the env — the effective settings resolve the clean
+  URL + the credentials FROM the DB (the env value still embeds the
+  credentials, ``.env`` is never rewritten: the process env is
+  untouched by the migration);
+* idempotent — a second run is a no-op (``False``, the rows
+  byte-identical);
+* the settings API reports the clean ``docs_repo`` (``source="db"``)
+  and the ``docs_token`` masked (``secret=true``, ``value=null``,
+  ``has_value=true``) with the token text NOWHERE in the raw body —
+  the leak is gone from the page.
+
+The module's teardown leaves NO row behind (the ``test_admin_settings_
+api`` discipline — a surviving row would retune every later suite
+through the overlay).
+
+Real Postgres (``podman compose up -d db``); no LLM involved.
+
+Requires:  podman compose up -d db
+"""
+from __future__ import annotations
+
+import os
+from collections.abc import Iterator
+
+import pytest
+from fastapi.testclient import TestClient
+from sqlalchemy import text
+from sqlalchemy.orm import Session
+
+from app.api import chat as chat_api
+from app.config import get_settings, invalidate_settings
+from app.core import settings_overlay
+from app.core.settings_migrate import migrate_legacy_docs_creds
+from app.core.settings_overlay import register_invalidator, resolve_settings
+from app.main import app as fastapi_app
+from tests.conftest import ADMIN_PASSWORD
+
+API = "/api/admin/settings"
+
+#: The legacy value the migration splits (the production shape — the
+#: token is distinctive so the "absent from the raw body" pin cannot
+#: false-positive on a catalogue label).
+_LEGACY = "https://u:tok-legacy-123@host/r.git"
+
+
+@pytest.fixture(autouse=True)
+def clean_state(
+    db: Session, monkeypatch: pytest.MonkeyPatch
+) -> Iterator[None]:
+    """A clean ``app_settings`` table + the overlay switch ON + a
+    fresh overlay/hook slate (the ``test_admin_settings_api`` idiom:
+    reset the process-wide hook list and re-wire what
+    ``create_app()`` wires). Teardown deletes the rows + invalidates
+    + clears the cache — NO row may survive this module (a surviving
+    row would retune every later suite through the overlay)."""
+    settings_overlay.reset_state_for_tests()
+    register_invalidator(chat_api.reset_runtime_caches)
+    db.execute(text("DELETE FROM app_settings"))
+    db.commit()
+    monkeypatch.setenv("BOR_SETTINGS_DB", "1")
+    monkeypatch.setenv("BOR_DOCS_REPO", "")
+    invalidate_settings()
+    get_settings.cache_clear()
+    yield
+    db.execute(text("DELETE FROM app_settings"))
+    db.commit()
+    invalidate_settings()
+    get_settings.cache_clear()
+    settings_overlay.reset_state_for_tests()
+
+
+def _table(db: Session) -> dict[str, str]:
+    """The whole ``app_settings`` table (the display fact, never a
+    query surface — one SELECT)."""
+    return {
+        key: value
+        for key, value in db.execute(
+            text("SELECT key, value FROM app_settings")
+        )
+    }
+
+
+def _admin_client() -> TestClient:
+    """A fresh client signed in as the admin (the ``_admin_client``
+    pattern from ``test_ui_settings_api``)."""
+    client = TestClient(fastapi_app)
+    r = client.post("/api/login", json={"password": ADMIN_PASSWORD})
+    assert r.status_code == 204, f"admin login failed: {r.status_code}"
+    return client
+
+
+def _item(body: dict, key: str) -> dict:
+    """One catalogue item by key (the GET body's ``items`` list)."""
+    for item in body["items"]:
+        if item["key"] == key:
+            return item
+    raise AssertionError(f"catalogue item missing: {key}")
+
+
+def test_legacy_env_is_split_into_db_rows(
+    db: Session, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """The end-to-end shape: seed env (no DB rows) → migration runs →
+    the three rows land, the effective settings resolve the clean URL
+    + the credentials FROM THE DB (the rows shadow the env), the env
+    is untouched (``.env`` is never rewritten), and the second run is
+    a no-op (idempotent)."""
+    monkeypatch.setenv("BOR_DOCS_REPO", _LEGACY)
+    assert _table(db) == {}  # no DB rows (the fixture's clean slate)
+
+    assert migrate_legacy_docs_creds() is True
+
+    # the three rows — the clean repo + user + token, and NOTHING
+    # else (the leak class dies: the repo row no longer carries the
+    # credentials).
+    rows = _table(db)
+    assert rows == {
+        "docs_repo": "https://host/r.git",
+        "docs_user": "u",
+        "docs_token": "tok-legacy-123",
+    }
+
+    # the effective settings resolve FROM THE DB (invalidate first —
+    # the module import primed the cache before this module's rows):
+    # the clean URL shadows the credentials-bearing env value.
+    invalidate_settings()
+    effective = resolve_settings()
+    assert effective.docs_repo == "https://host/r.git"
+    assert effective.docs_user == "u"
+    assert effective.docs_token == "tok-legacy-123"
+
+    # the env is untouched — the migration never rewrites the env
+    # (``.env`` stays the base; the DB rows shadow it).
+    assert os.environ["BOR_DOCS_REPO"] == _LEGACY
+
+    # idempotent: the ``docs_token`` row is present now — a second
+    # run is a no-op, the rows byte-identical.
+    assert migrate_legacy_docs_creds() is False
+    assert _table(db) == rows
+
+
+def test_migrated_values_are_masked_on_the_settings_api(
+    db: Session, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Completion criterion (the leak is gone from the page): after a
+    (b)-shape migration the settings API reports the clean
+    ``docs_repo`` (``source="db"``) + ``docs_token``
+    ``secret=true`` / ``value=null`` / ``has_value=true`` (masked)
+    with the token text NOWHERE in the raw response body."""
+    monkeypatch.setenv("BOR_DOCS_REPO", _LEGACY)
+    assert migrate_legacy_docs_creds() is True
+    invalidate_settings()
+
+    admin = _admin_client()
+    r = admin.get(API)
+    assert r.status_code == 200, r.text
+
+    repo = _item(r.json(), "docs_repo")
+    assert repo["value"] == "https://host/r.git"  # the clean URL
+    assert repo["source"] == "db"  # the DB row is in effect
+    assert repo["db_value"] == "https://host/r.git"
+
+    token = _item(r.json(), "docs_token")
+    assert token["secret"] is True
+    assert token["masked"] is True
+    assert token["value"] is None  # write-only: never the value
+    assert token["db_value"] is None  # a masked row never exposes text
+    assert token["has_value"] is True  # a value IS in effect
+    assert token["has_override"] is True  # the DB row
+    assert token["source"] == "db"
+
+    user = _item(r.json(), "docs_user")
+    assert user["value"] == "u"
+    assert user["source"] == "db"
+
+    # the core pin: the token text is NOWHERE in the raw body (the
+    # leak class this phase kills — asserted on response.text, not
+    # the parsed model; the value is distinctive so the substring
+    # check cannot false-positive on a catalogue label).
+    assert "tok-legacy-123" not in r.text
+
+
+def test_migration_is_a_noop_for_a_clean_env(
+    db: Session, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """The no-op shape end-to-end: a clean env URL (the pre-phase
+    state) → ``False``, the table stays empty (nothing half-
+    migrates, nothing written)."""
+    monkeypatch.setenv("BOR_DOCS_REPO", "https://host/r.git")
+    assert migrate_legacy_docs_creds() is False
+    assert _table(db) == {}
+    invalidate_settings()
+    assert resolve_settings().docs_repo == "https://host/r.git"