fix: bootstrap user preferences without invalidating DWH cache
This commit is contained in:
+7
-1
@@ -64,7 +64,13 @@ export function buildApp(config: AppConfig, deps?: BuildAppDeps): FastifyInstanc
|
|||||||
// The real runner persists preferences through the harness repository. The file fallback
|
// The real runner persists preferences through the harness repository. The file fallback
|
||||||
// only keeps older isolated route tests and externally injected runners compatible.
|
// only keeps older isolated route tests and externally injected runners compatible.
|
||||||
if (typeof runner.preferencesGet === "function") {
|
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));
|
return effectiveSettings(config, loadSettings(config));
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { test, expect } from "vitest";
|
import { test, expect } from "vitest";
|
||||||
import { mkdtempSync, rmSync } from "node:fs";
|
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
|
||||||
import { tmpdir } from "node:os";
|
import { tmpdir } from "node:os";
|
||||||
import { join } from "node:path";
|
import { join } from "node:path";
|
||||||
import { buildApp } from "../src/app.js";
|
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" });
|
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<string, unknown> = {};
|
||||||
|
const writes: Record<string, unknown>[] = [];
|
||||||
|
const runner = {
|
||||||
|
withPrincipal: () => ({
|
||||||
|
preferencesGet: async () => preferences,
|
||||||
|
preferencesSet: async (next: Record<string, unknown>) => {
|
||||||
|
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 () => {
|
test("PUT /settings rejects an unknown model when a model list is available", async () => {
|
||||||
const { app, dir } = appWithTmpSettings({}, {
|
const { app, dir } = appWithTmpSettings({}, {
|
||||||
listModels: async () => [{ provider: "zai", id: "glm-5.2", name: "GLM 5.2", reasoning: true }],
|
listModels: async () => [{ provider: "zai", id: "glm-5.2", name: "GLM 5.2", reasoning: true }],
|
||||||
|
|||||||
@@ -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<Record<string, unknown>>`, `ThtRunner.preferencesSet(settings): Promise<void>`, `loadSettings(config)`, and `effectiveSettings(config, settings)`.
|
||||||
|
- Produces: `getSettings(principal): Promise<Settings>` 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.
|
||||||
@@ -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.
|
||||||
@@ -31,6 +31,24 @@ def test_dwh_and_evidence_jobs_have_distinct_lock_names():
|
|||||||
assert _lock_name("demo", "dwh") != _lock_name("demo", "evidence")
|
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):
|
def test_unowned_reads_fail_closed_without_creating_any_files(tmp_path):
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
|
|||||||
@@ -48,9 +48,21 @@ def config_dwh_binding(cfg) -> dict[str, str]:
|
|||||||
config_source = getattr(cfg, "_config_source", None)
|
config_source = getattr(cfg, "_config_source", None)
|
||||||
if not isinstance(workspace_id, str) or not isinstance(config_source, str):
|
if not isinstance(workspace_id, str) or not isinstance(config_source, str):
|
||||||
raise CorruptCheckpointError("DWH workspace identity is unavailable; reload configuration")
|
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 {
|
return {
|
||||||
"workspace_id": workspace_id,
|
"workspace_id": workspace_id,
|
||||||
"config_fingerprint": fingerprint(cfg.model_dump_json()),
|
"config_fingerprint": config_fingerprint,
|
||||||
"input_fingerprint": fingerprint(config_source),
|
"input_fingerprint": fingerprint(config_source),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user