From 5cacf70a0d223a46f419f1a114f0bc0ec8175ae7 Mon Sep 17 00:00:00 2001 From: User Date: Thu, 16 Jul 2026 20:32:20 +0200 Subject: [PATCH] fix: bootstrap user preferences without invalidating DWH cache --- backend/src/app.ts | 8 +- backend/test/routes-settings.test.ts | 61 +++++++++- .../2026-07-16-user-preference-bootstrap.md | 110 ++++++++++++++++++ ...-07-16-user-preference-bootstrap-design.md | 34 ++++++ harness/tests/test_dwh_preprocess_job.py | 18 +++ harness/tht/jobs/dwh_pipeline.py | 14 ++- 6 files changed, 242 insertions(+), 3 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-16-user-preference-bootstrap.md create mode 100644 docs/superpowers/specs/2026-07-16-user-preference-bootstrap-design.md diff --git a/backend/src/app.ts b/backend/src/app.ts index b9c51baf..f2a79bcc 100644 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -64,7 +64,13 @@ export function buildApp(config: AppConfig, deps?: BuildAppDeps): FastifyInstanc // The real runner persists preferences through the harness repository. The file fallback // only keeps older isolated route tests and externally injected runners compatible. if (typeof runner.preferencesGet === "function") { - return effectiveSettings(config, await runner.preferencesGet()); + const stored = await runner.preferencesGet() as Settings; + if (Object.keys(stored).length === 0) { + const seeded = effectiveSettings(config, loadSettings(config)); + await runner.preferencesSet(seeded); + return seeded; + } + return effectiveSettings(config, stored); } return effectiveSettings(config, loadSettings(config)); }; diff --git a/backend/test/routes-settings.test.ts b/backend/test/routes-settings.test.ts index cfed13ec..f25a4964 100644 --- a/backend/test/routes-settings.test.ts +++ b/backend/test/routes-settings.test.ts @@ -1,5 +1,5 @@ import { test, expect } from "vitest"; -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { buildApp } from "../src/app.js"; @@ -77,6 +77,65 @@ test("settings are isolated by the authenticated repository principal", async () expect(bob.json()).not.toMatchObject({ workspace: "psd", thinking: "high" }); }); +test("GET /settings seeds an empty private profile from complete legacy settings once", async () => { + let preferences: Record = {}; + const writes: Record[] = []; + const runner = { + withPrincipal: () => ({ + preferencesGet: async () => preferences, + preferencesSet: async (next: Record) => { + writes.push(next); + preferences = next; + }, + }), + }; + const { app, dir } = appWithTmpSettings({}, { thtRunner: runner as any, listModels: async () => [] }); + try { + writeFileSync(join(dir, "settings.json"), JSON.stringify({ + workspace: "local", provider: "local-qwen", model: "qwen3.6-35b-a3b", thinking: "low", + })); + + const first = await app.inject({ method: "GET", url: "/settings" }); + const second = await app.inject({ method: "GET", url: "/settings" }); + + const expected = { + workspace: "local", provider: "local-qwen", model: "qwen3.6-35b-a3b", thinking: "low", + }; + expect(first.statusCode).toBe(200); + expect(first.json()).toEqual(expected); + expect(second.json()).toEqual(expected); + expect(writes).toEqual([expected]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("GET /settings does not overwrite an existing private profile with legacy settings", async () => { + const privateSettings = { + workspace: "private", provider: "zai", model: "glm-5.2", thinking: "high", + }; + const preferencesSet = async () => { throw new Error("must not seed an existing profile"); }; + const runner = { + withPrincipal: () => ({ + preferencesGet: async () => privateSettings, + preferencesSet, + }), + }; + const { app, dir } = appWithTmpSettings({}, { thtRunner: runner as any, listModels: async () => [] }); + try { + writeFileSync(join(dir, "settings.json"), JSON.stringify({ + workspace: "local", provider: "local-qwen", model: "qwen3.6-35b-a3b", thinking: "low", + })); + + const response = await app.inject({ method: "GET", url: "/settings" }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toEqual(privateSettings); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + test("PUT /settings rejects an unknown model when a model list is available", async () => { const { app, dir } = appWithTmpSettings({}, { listModels: async () => [{ provider: "zai", id: "glm-5.2", name: "GLM 5.2", reasoning: true }], diff --git a/docs/superpowers/plans/2026-07-16-user-preference-bootstrap.md b/docs/superpowers/plans/2026-07-16-user-preference-bootstrap.md new file mode 100644 index 00000000..dd0e5e2e --- /dev/null +++ b/docs/superpowers/plans/2026-07-16-user-preference-bootstrap.md @@ -0,0 +1,110 @@ +# User Preference Bootstrap Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Seed a new principal's private settings from complete legacy settings so a first session can configure and start Pi. + +**Architecture:** `buildApp` continues to resolve settings through the principal-bound harness runner. When `preferencesGet()` returns an empty object, it derives the existing effective legacy settings from `SETTINGS_FILE`, persists them once via `preferencesSet()`, and returns that same object. A non-empty private object remains authoritative. + +**Tech Stack:** TypeScript, Fastify, Vitest. + +## Global Constraints + +- Never overwrite a non-empty private preference object. +- Persist only provider, model, thinking, and workspace values; no credentials enter preferences. +- Keep storage failures fail-closed through the existing 503 route contract. +- Follow TDD: observe the regression test fail before adding implementation. + +--- + +### Task 1: Bootstrap legacy settings for an empty private profile + +**Files:** +- Modify: `backend/test/routes-settings.test.ts` +- Modify: `backend/src/app.ts` + +**Interfaces:** +- Consumes: `ThtRunner.preferencesGet(): Promise>`, `ThtRunner.preferencesSet(settings): Promise`, `loadSettings(config)`, and `effectiveSettings(config, settings)`. +- Produces: `getSettings(principal): Promise` that returns a complete persisted profile for first-time principals. + +- [ ] **Step 1: Write the failing regression tests** + +```ts +test("GET /settings seeds an empty private profile from complete legacy settings once", async () => { + // Seed SETTINGS_FILE with local-qwen/qwen3.6-35b-a3b/low. + // Make preferencesGet return {} and record preferencesSet calls. + // Assert the first GET returns and persists all four settings, and a second GET does not write again. +}); + +test("GET /settings keeps a non-empty private profile authoritative", async () => { + // Seed different legacy settings, return an existing private profile, + // and assert no preferencesSet call occurs. +}); +``` + +- [ ] **Step 2: Run the focused test file and verify RED** + +Run: `cd backend && npx vitest run test/routes-settings.test.ts` + +Expected: the first test fails because the current resolver returns only defaults and never calls `preferencesSet`. + +- [ ] **Step 3: Implement the minimal resolver change** + +```ts +const stored = await runner.preferencesGet(); +if (Object.keys(stored).length === 0) { + const seeded = effectiveSettings(config, loadSettings(config)); + await runner.preferencesSet(seeded); + return seeded; +} +return effectiveSettings(config, stored); +``` + +- [ ] **Step 4: Run focused tests and TypeScript verification** + +Run: `cd backend && npx vitest run test/routes-settings.test.ts && npx tsc --noEmit -p .` + +Expected: exit 0. + +- [ ] **Step 5: Run backend regression suite** + +Run: `cd backend && npx vitest run && npm run build` + +Expected: exit 0. + +### Task 2: Preserve DWH artifact ownership across the optional session-storage field + +**Files:** +- Modify: `harness/tests/test_dwh_preprocess_job.py` +- Modify: `harness/tht/jobs/dwh_pipeline.py` + +**Interfaces:** +- Consumes: `config_dwh_binding(cfg)` and Pydantic's `Config.model_dump(mode="json")`. +- Produces: a DWH binding whose `config_fingerprint` excludes only `session_storage`. + +- [ ] **Step 1: Write the failing regression test** + +```python +def test_session_storage_does_not_change_the_dwh_artifact_binding(tmp_path): + assert config_dwh_binding(config()) == config_dwh_binding(config(session_storage={...})) +``` + +- [ ] **Step 2: Run the focused test and verify RED** + +Run: `cd harness && .venv/bin/pytest tests/test_dwh_preprocess_job.py::test_session_storage_does_not_change_the_dwh_artifact_binding -q` + +Expected: FAIL because the full configuration JSON currently includes `session_storage`. + +- [ ] **Step 3: Implement the minimal DWH-only fingerprint** + +```python +payload = cfg.model_dump(mode="json") +payload.pop("session_storage", None) +config_fingerprint = fingerprint(json.dumps(payload, separators=(",", ":"), ensure_ascii=False)) +``` + +- [ ] **Step 4: Run focused and complete harness verification** + +Run: `cd harness && .venv/bin/pytest tests/test_dwh_preprocess_job.py -q && .venv/bin/pytest -q && .venv/bin/ruff check tht/jobs/dwh_pipeline.py tests/test_dwh_preprocess_job.py` + +Expected: exit 0. diff --git a/docs/superpowers/specs/2026-07-16-user-preference-bootstrap-design.md b/docs/superpowers/specs/2026-07-16-user-preference-bootstrap-design.md new file mode 100644 index 00000000..d5b6baec --- /dev/null +++ b/docs/superpowers/specs/2026-07-16-user-preference-bootstrap-design.md @@ -0,0 +1,34 @@ +# User preference bootstrap design + +## Problem + +The user-owned-session branch reads settings only from the current principal's +repository preferences. Existing deployments still have their provider, model, +thinking level, and workspace only in the legacy backend settings JSON file. +For a principal with no private preference record, a new session is therefore +created without a provider or model and Pi is rejected before it can spawn. + +## Decision + +On the first settings read for a principal whose private preference object is +empty, the backend computes the complete effective legacy settings from +`SETTINGS_FILE` and the configured environment defaults, writes that complete +object to the principal's repository preferences, and returns it. + +After this one-time bootstrap, the private preference object is the sole +source for that principal. A non-empty private object is never replaced with +the legacy values. If either reading or writing the private preferences fails, +the request remains fail-closed with the existing 503 response. + +The addition of optional session storage must not change DWH artifact +ownership. The DWH binding therefore excludes `session_storage` while retaining +every schema, retrieval, vector, and execution setting in its fingerprint. + +## Scope and verification + +The change is confined to the backend settings resolver. Vitest coverage must +prove that an empty private profile is seeded exactly once with the complete +legacy settings, and that an existing private profile is neither changed nor +replaced. Harness coverage must also prove that adding session storage leaves +the DWH binding unchanged. Existing session-route coverage continues to prove +that new-session creation consumes the resolved settings. diff --git a/harness/tests/test_dwh_preprocess_job.py b/harness/tests/test_dwh_preprocess_job.py index cdc0dbf1..b4119a44 100644 --- a/harness/tests/test_dwh_preprocess_job.py +++ b/harness/tests/test_dwh_preprocess_job.py @@ -31,6 +31,24 @@ def test_dwh_and_evidence_jobs_have_distinct_lock_names(): assert _lock_name("demo", "dwh") != _lock_name("demo", "evidence") +def test_session_storage_does_not_change_the_dwh_artifact_binding(tmp_path): + from types import SimpleNamespace + + def config(session_storage=None): + payload = {"dwh": {"connection": {"database": "warehouse"}}} + if session_storage is not None: + payload["session_storage"] = session_storage + cfg = SimpleNamespace(_workspace_id="demo", _config_source="test") + cfg.model_dump = lambda **_kwargs: payload + cfg.model_dump_json = lambda: json.dumps(payload, separators=(",", ":")) + return cfg + + without_session_storage = config() + with_session_storage = config({"type": "postgres_direct", "connection": {"database": "sessions"}}) + + assert config_dwh_binding(without_session_storage) == config_dwh_binding(with_session_storage) + + def test_unowned_reads_fail_closed_without_creating_any_files(tmp_path): import pytest diff --git a/harness/tht/jobs/dwh_pipeline.py b/harness/tht/jobs/dwh_pipeline.py index a483a404..e67b9164 100644 --- a/harness/tht/jobs/dwh_pipeline.py +++ b/harness/tht/jobs/dwh_pipeline.py @@ -48,9 +48,21 @@ def config_dwh_binding(cfg) -> dict[str, str]: config_source = getattr(cfg, "_config_source", None) if not isinstance(workspace_id, str) or not isinstance(config_source, str): raise CorruptCheckpointError("DWH workspace identity is unavailable; reload configuration") + model_dump = getattr(cfg, "model_dump", None) + if callable(model_dump): + payload = model_dump(mode="json") + if not isinstance(payload, dict): + raise CorruptCheckpointError("DWH workspace configuration is unavailable; reload configuration") + # Session persistence has no bearing on schema/LSH artifacts. Excluding it keeps an + # opt-in session-storage deployment from invalidating an otherwise identical DWH cache. + payload.pop("session_storage", None) + config_fingerprint = fingerprint(json.dumps(payload, separators=(",", ":"), ensure_ascii=False)) + else: + # Lightweight test doubles predating Pydantic's model_dump() retain the legacy seam. + config_fingerprint = fingerprint(cfg.model_dump_json()) return { "workspace_id": workspace_id, - "config_fingerprint": fingerprint(cfg.model_dump_json()), + "config_fingerprint": config_fingerprint, "input_fingerprint": fingerprint(config_source), }