Admin Settings: enforce the form field contract before stage D moves cards

application/single_app/config.py

@@ -96,7 +96,7 @@
 EXECUTOR_TYPE = 'thread'
 EXECUTOR_MAX_WORKERS = 30
 SESSION_TYPE = 'filesystem'
-VERSION = "0.260.008"
+VERSION = "0.260.009"
 IS_DEVELOPMENT = is_development_env_enabled()
 
 SESSION_COOKIE_SAMESITE = os.getenv('SESSION_COOKIE_SAMESITE', 'Lax')

docs/explanation/features/ADMIN_SETTINGS_IA_REWORK_STATUS.md

@@ -0,0 +1,154 @@
+# Admin Settings IA rework — handoff
+
+Everything a new conversation needs to continue this work. The session-local
+scratch scripts do **not** survive, so the parts that mattered were moved into
+the repository.
+
+---
+
+## Where things stand
+
+Staging branch `feature/admin-settings-ia` on `microsoft/simplechat`, cut from
+`Development`. Nothing reaches `Development` until the whole rework is tested
+and merged in one final reviewed PR.
+
+```
+Development ─── feature/admin-settings-ia ──┬── #1297 stage 1    MERGED
+                                            ├── #1304 stage A+B  MERGED
+                                            ├── stage C          next
+                                            ├── stage D
+                                            └── stage E
+                                                      └──► final PR ──► Development
+```
+
+Current version: **0.260.009**. Fingerprint: **462 field names / 110 card ids**.
+
+### Shipped
+
+| Stage | Content |
+|---|---|
+| 1 | Template split into 18 per-tab partials under `templates/admin/_panes/`; Latest Features moved last and General made the landing tab; Global Identities heading; File Sync sidebar submenu; `sectionMap` pruned 72 → 6; classification banner preview fixed; CodeQL XSS sink fixed |
+| A | 12 cross-tab links converted from tab-coupled `switchTab` to `data-admin-link` card targets; two were already broken; `switchTab` removed |
+| B | `data-requires` dependency announcements with inline mirror and link; File Sync and Permissions wired |
+| — | Form field contract enforced in CI |
+
+---
+
+## Guardrails — these now live in the repo
+
+The single most important rule: **never rename a `name=` attribute.** Admin
+Settings posts one form and the backend reads by field name, so a rename
+silently stops a setting from saving with no error anywhere.
+
+| Check | How to run |
+|---|---|
+| **Field contract** | `python -m pytest functional_tests/test_admin_settings_field_contract.py` |
+| Regenerate baseline *(only for a deliberate removal)* | `python functional_tests/test_admin_settings_field_contract.py --update-baseline` |
+| Composition contract | `python -m pytest functional_tests/test_admin_settings_template_composition.py` |
+| Card links | `python -m pytest functional_tests/test_admin_card_links.py` |
+| Dependencies | `python -m pytest functional_tests/test_admin_settings_dependencies.py` |
+| Sidebar parity | `python -m pytest functional_tests/test_admin_settings_sidebar_card_parity.py` |
+| XSS | `python scripts/check_xss_sinks.py --full-file <changed files>` |
+
+**Regression baseline:** the 75 functional test files that reference
+`admin_settings.html` sit at **33 pre-existing failures**. That number must not
+change. To reproduce the list:
+
+```
+cd functional_tests
+python -m pytest $(grep -rl admin_settings.html test_*.py) -q -p no:randomly
+```
+
+Exclude `test_enhanced_pii_analysis_standalone.py`, which needs live Azure
+credentials.
+
+### Reading templates in tests
+
+`admin_settings.html` is only a shell now. Tests must compose it:
+
+```python
+from test_support.templates import read_admin_settings_template
+markup = read_admin_settings_template()
+```
+
+`test_admin_settings_template_composition.py` fails any test that reads the
+template uncomposed while referencing a partial-backed card or field.
+
+---
+
+## Next: Stage C
+
+Add the group navigation level **against the current 18 tabs**, so grouping is
+proven before any card moves.
+
+1. Group row in the top-tab strip; collapsible group headers in
+   `_sidebar_nav.html` with persisted state.
+2. Three-level sidebar search (group → tab → card).
+3. Legacy hash redirect map inside `showAdminTab`.
+
+A server-side nav map is worth introducing here, since both navs need to render
+the same group structure. Note it is **not** needed for link resolution —
+`admin_card_links.js` resolves card → tab from the DOM via
+`closest('.tab-pane')`, which is why links already survive any IA change.
+
+## Then Stage D — the risky one
+
+Re-home cards into the 14 groups, **one group per commit**, running the field
+contract test on each. Three complications found while splitting:
+
+- **I1** Modals are interleaved *between* cards, not collected at the end. Each
+  modal moves with the tab that owns its trigger.
+- **I2** Three cards are nested inside other cards and must move with their
+  parents: `conversation-contents-drawer-section` (inside chat file uploads),
+  `content-understanding-section` and `office-embedded-image-section` (inside
+  document intelligence). All three land in the same target tab as their parent.
+- **I4** The Data Management migration workflow is ~985 lines of non-card markup
+  forming one unit. Split it across Backup/Migrate/Restore/Jobs **last and on
+  its own**.
+
+## Then Stage E
+
+- Split `system-settings-section`: `max_file_size_mb` → Workspaces;
+  `conversation_history_limit` and `default_system_prompt` → Chat; idle timeout
+  fields → Security; `access_denied_message` → Security. **No renames.**
+- Access and Roles roster mirroring the 10 `require_member_of_*` toggles, built
+  on the Stage B proxy handling. Mirrors carry **no `name` attribute** — the
+  field contract test enforces this.
+
+---
+
+## Decisions already locked
+
+| | Decision |
+|---|---|
+| D1 | Split `system-settings-section` across Chat / Security / Workspaces |
+| D2 | Workflow becomes its own group |
+| D3 | Mirror role toggles rather than move them |
+| D4 | Legacy hash redirect map |
+| D5 | Per-tab partials |
+| Q2 | File Sync warns rather than blocks; hard-disable elsewhere |
+| — | Land each stage as its own PR into the staging branch |
+
+## Target structure — 14 groups
+
+`Appearance · Chat · AI Models · Agents & Actions · Workspaces · Workflow ·
+Knowledge · Security · Governance · Data Lifecycle · Backup & Recovery · Scale ·
+Operations · Help`
+
+Full card-by-card placement is in the PR description of #1304 and in the
+stage 2 plan. Key rationale: grouping follows *capability weight*, not card
+titles — Governance has 196 KB of backing JS so it cannot be a Security
+sub-tab; Video and audio are extraction pipelines, not chat features; Retention
+is data lifecycle, not a workspace toggle.
+
+---
+
+## Before the final merge to Development
+
+Revert the CI branch filters added in #1297. Six workflows list
+`feature/admin-settings-ia` in their `pull_request` branch filters so staged
+PRs are gated; `Development` should end up with its original configuration.
+
+Files: `codeql.yml`, `xss-sink-check.yml`, `broken-access-control-check.yml`,
+`malicious-pr-security-review.yml`, `python-syntax-check.yml`,
+`swagger-route-check.yml`.

docs/explanation/release_notes.md

@@ -2,6 +2,16 @@
 
 For feature-focused and fix-focused drill-downs by version, see [Features by Version](/explanation/features/) and [Fixes by Version](/explanation/fixes/).
 
+### **(v0.260.009)**
+
+#### New Features
+
+*   **Admin Settings Form Field Contract Is Now Enforced**
+    *   Admin Settings submits one form and the backend reads every value by field name, so the set of `name` attributes is the real contract between the template and the settings backend. Renaming or dropping one silently stops that setting from saving, with no error anywhere.
+    *   A new test pins every field name against a committed baseline, and fails the build if one disappears. Adding settings is unaffected; removing one now requires regenerating the baseline in the same commit, which makes it a visible, reviewed decision.
+    *   The same test rejects duplicate field names, which is what prevents a mirrored control from submitting a value twice.
+    *   (Ref: `test_admin_settings_field_contract.py`, `admin_settings_field_baseline.json`)
+
 ### **(v0.260.008)**
 
 #### New Features

functional_tests/test_admin_settings_field_contract.py

@@ -0,0 +1,169 @@
+#!/usr/bin/env python3
+# test_admin_settings_field_contract.py
+"""
+Functional test pinning the Admin Settings form field contract.
+Version: 0.260.009
+Implemented in: 0.260.009
+
+Admin Settings submits a single form, and the backend reads every value by
+field name. That makes the set of `name` attributes the real contract between
+the template and `route_frontend_admin_settings.py`. Moving a card between tabs
+does not change it; renaming or dropping a `name` silently stops a setting from
+saving, with no error anywhere.
+
+The information architecture rework moves most cards between tabs, so this test
+holds that contract still:
+
+  1. Every field name recorded in the baseline still exists.
+  2. No field name is duplicated except the known radio and checkbox groups,
+     which is what stops a mirrored control from submitting a value twice.
+
+New field names are allowed and reported, because adding settings is ordinary
+work. Removing or renaming one requires regenerating the baseline in the same
+commit:
+
+    python functional_tests/test_admin_settings_field_contract.py --update-baseline
+
+That makes losing a setting a visible, reviewed decision rather than an
+accident.
+"""
+
+import json
+import re
+import sys
+from collections import Counter
+from pathlib import Path
+
+from test_support.templates import read_admin_settings_template
+from test_support.versioning import assert_app_version_at_least
+
+
+BASELINE_PATH = (
+    Path(__file__).resolve().parent
+    / "test_support"
+    / "admin_settings_field_baseline.json"
+)
+
+FIELD_RE = re.compile(r'\sname="([^"]+)"')
+
+# Jinja-templated names cannot be compared literally.
+TEMPLATED = re.compile(r"\{\{|\{%")
+
+
+def collect_field_names(markup):
+    """Return a Counter of literal form field names in the composed template."""
+    return Counter(
+        name for name in FIELD_RE.findall(markup) if not TEMPLATED.search(name)
+    )
+
+
+def load_baseline():
+    """Read the committed field-name contract."""
+    assert BASELINE_PATH.is_file(), (
+        f"Missing baseline at {BASELINE_PATH}. Generate it with "
+        "--update-baseline."
+    )
+    return json.loads(BASELINE_PATH.read_text(encoding="utf-8"))
+
+
+def test_no_admin_settings_field_is_lost():
+    """A dropped or renamed field name silently stops a setting from saving."""
+    print("Testing Admin Settings field name contract...")
+
+    assert_app_version_at_least("0.260.009")
+    baseline = load_baseline()
+    current = collect_field_names(read_admin_settings_template())
+
+    required = set(baseline["field_names"])
+    present = set(current)
+
+    missing = sorted(required - present)
+    assert not missing, (
+        "These form fields disappeared from Admin Settings, so the settings "
+        "they carry can no longer be saved. If a removal is intended, "
+        "regenerate the baseline in the same commit with --update-baseline:\n  "
+        + "\n  ".join(missing)
+    )
+
+    added = sorted(present - required)
+    if added:
+        print(f"  {len(added)} new field(s) since the baseline: {added[:5]}")
+
+    print(f"All {len(required)} baseline field(s) still present.")
+
+
+def test_no_unexpected_duplicate_field_names():
+    """Two controls sharing a name submit the value twice."""
+    print("Testing Admin Settings duplicate field names...")
+
+    baseline = load_baseline()
+    current = collect_field_names(read_admin_settings_template())
+
+    allowed = baseline["allowed_duplicates"]
+    unexpected = {
+        name: count
+        for name, count in current.items()
+        if count > 1 and name not in allowed
+    }
+
+    assert not unexpected, (
+        "These field names appear more than once, so the form would submit "
+        "each of them multiple times. Mirrored controls must omit the name "
+        "attribute rather than repeat it:\n  "
+        + "\n  ".join(f"{name} x{count}" for name, count in sorted(unexpected.items()))
+    )
+
+    # A group that stops being a group is also a contract change worth noticing.
+    shrunk = sorted(
+        name for name, count in allowed.items() if current.get(name, 0) < count
+    )
+    if shrunk:
+        print(f"  note: known group(s) now smaller than baseline: {shrunk}")
+
+    print(f"No unexpected duplicates; {len(allowed)} known group(s) intact.")
+
+
+def update_baseline():
+    """Rewrite the baseline from the current template, deliberately."""
+    current = collect_field_names(read_admin_settings_template())
+    payload = {
+        "_comment": (
+            "Contract for Admin Settings form field names. Regenerate with "
+            "test_admin_settings_field_contract.py --update-baseline only when "
+            "a field is intentionally removed or renamed."
+        ),
+        "field_names": sorted(current),
+        "allowed_duplicates": {
+            name: count for name, count in sorted(current.items()) if count > 1
+        },
+    }
+    BASELINE_PATH.write_text(
+        json.dumps(payload, indent=2) + "\n", encoding="utf-8"
+    )
+    print(f"Baseline written: {len(payload['field_names'])} field(s), "
+          f"{len(payload['allowed_duplicates'])} known group(s)")
+
+
+if __name__ == "__main__":
+    if "--update-baseline" in sys.argv:
+        update_baseline()
+        sys.exit(0)
+
+    tests = [
+        test_no_admin_settings_field_is_lost,
+        test_no_unexpected_duplicate_field_names,
+    ]
+
+    results = []
+    for test in tests:
+        try:
+            test()
+            results.append(True)
+        except Exception as exc:
+            print(f"FAILED {test.__name__}: {exc}")
+            import traceback
+            traceback.print_exc()
+            results.append(False)
+
+    print(f"\nResults: {sum(results)}/{len(results)} passed")
+    sys.exit(0 if all(results) else 1)