From 5160a00db5663ce4012c566aabed3feb603180b7 Mon Sep 17 00:00:00 2001 From: ahmed mohammed Date: Tue, 22 Sep 2026 09:10:29 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20final=20review=20pass=20=E2=80=94=20emai?= =?UTF-8?q?l=20collision,=20orphaned-run=20loop,=20null=20confidence=20gua?= =?UTF-8?q?rds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - changeEmail validates and refuses an address another account owns (users.email is UNIQUE; a collision was a 500 inside the settings form). - /app/[id] no longer redirects a 'running' row to itself; it renders what exists and says the run was interrupted. - ScoreBar / OptionRow / VerdictBlock survive null numbers from the model instead of throwing and losing the block (seen live: '[openui] runtime/render-error: Component ScoreBar render failed'). - drop the ad-hoc render-debug scripts now that Playwright covers the browser. Co-Authored-By: Claude Opus 4.7 --- .gitignore | 3 +-- app/app/[id]/page.tsx | 8 ++++++-- lib/actions.ts | 10 ++++++---- lib/auth.ts | 12 +++++++++++- lib/report-ui.ts | 15 +++++++++----- scripts/e2e-debug.mjs | 24 ----------------------- scripts/render-check.mjs | 27 ------------------------- scripts/render-debug.mjs | 13 ------------ tests-e2e/edge.spec.ts | 38 ++++++++++++++++++++++++++++++++++++ tests-e2e/responsive.spec.ts | 5 +++-- tests/auth.test.ts | 13 ++++++++++++ tests/report-ui.test.ts | 13 ++++++++++++ 12 files changed, 101 insertions(+), 80 deletions(-) delete mode 100644 scripts/e2e-debug.mjs delete mode 100644 scripts/render-check.mjs delete mode 100644 scripts/render-debug.mjs create mode 100644 tests-e2e/edge.spec.ts diff --git a/.gitignore b/.gitignore index 12094bf..3efe2a3 100644 --- a/.gitignore +++ b/.gitignore @@ -19,7 +19,6 @@ next-env.d.ts /test-results /playwright-report -# local fixtures -/tests/__fixtures__ +# e2e fixture input is committed; scratch output is not .DS_Store .playwright-data diff --git a/app/app/[id]/page.tsx b/app/app/[id]/page.tsx index b9624d7..33f806d 100644 --- a/app/app/[id]/page.tsx +++ b/app/app/[id]/page.tsx @@ -1,4 +1,4 @@ -import { notFound, redirect } from "next/navigation"; +import { notFound } from "next/navigation"; import { requireUser } from "@/lib/auth"; import { getAnalysis } from "@/lib/db"; import { ReportView } from "@/components/report-view"; @@ -9,9 +9,13 @@ export default async function Stored({ params }: { params: Promise<{ id: string const u = await requireUser(); const a = getAnalysis((await params).id, u.id); if (!a) notFound(); - if (a.status === "running") redirect(`/app/${a.id}`); // still streaming elsewhere; rare, bounce once + // Never redirect here: a row left "running" by a crashed process would bounce this page to itself forever. + const interrupted = a.status === "running"; return

{a.title}

+ {interrupted &&

+ This run was interrupted before it finished — what is below is everything it managed to produce. +

} {a.error &&

{a.error}

} {a.verdict && } {a.oul ? <> diff --git a/lib/actions.ts b/lib/actions.ts index 42cf039..cb0ae3e 100644 --- a/lib/actions.ts +++ b/lib/actions.ts @@ -2,7 +2,7 @@ import { redirect } from "next/navigation"; import { revalidatePath } from "next/cache"; import { userByEmail, getSettings, renameAnalysis, deleteAnalysis, db } from "@/lib/db"; -import { signUpNewUser, verifyPassword, startSession, endSession, requireUser, emailOk, hashPassword } from "@/lib/auth"; +import { signUpNewUser, verifyPassword, startSession, endSession, requireUser, changeEmail, emailOk, hashPassword } from "@/lib/auth"; import { saveSettingsForm } from "@/lib/settings"; export async function signUp(_: unknown, fd: FormData) { @@ -36,16 +36,18 @@ export async function remove(_: unknown, fd: FormData) { export async function saveSettingsAction(_: unknown, fd: FormData) { const u = await requireUser(); - const email = String(fd.get("email") ?? "").trim().toLowerCase(); + const email = String(fd.get("email") ?? "").trim(); const password = String(fd.get("password") ?? ""); - if (email && !emailOk(email)) return { ok: false, error: "Enter a valid email address." }; if (password && password.length < 10) return { ok: false, error: "Use at least 10 characters." }; saveSettingsForm(u.id, fd); // The wizard sends `after`: a configured user must land on the readiness screen, and the setup page // forwards configured users away from steps 1-2, so "Saved." would never be seen there. if (String(fd.get("after") ?? "").startsWith("/setup")) redirect("/setup?step=3"); if (password) db.prepare("UPDATE users SET pass = ? WHERE id = ?").run(hashPassword(password), u.id); - if (email) db.prepare("UPDATE users SET email = ? WHERE id = ?").run(email, u.id); + if (email) { + const r = changeEmail(u.id, email); + if (!r.ok) return r; + } revalidatePath("/settings"); return { ok: true }; } diff --git a/lib/auth.ts b/lib/auth.ts index 465c33a..10ffae8 100644 --- a/lib/auth.ts +++ b/lib/auth.ts @@ -1,5 +1,5 @@ import { randomBytes, scryptSync, createHash, timingSafeEqual } from "node:crypto"; -import { createUser, userByEmail, userEmail, putSession, sessionUserId, dropSession, getSettings } from "./db.ts"; +import { createUser, userByEmail, userEmail, putSession, sessionUserId, dropSession, getSettings, db } from "./db.ts"; export const SESSION_COOKIE = "qai_s"; export const SESSION_DAYS = 30; @@ -34,6 +34,16 @@ export function signUpNewUser(email: string, password: string): { ok: true; id: // request-scoped binding must not be touched at import time. const jar = async () => (await import("next/headers")).cookies(); +export function changeEmail(userId: string, email: string): { ok: true } | { ok: false; error: string } { + const e = email.trim().toLowerCase(); + if (!emailOk(e)) return { ok: false, error: "Enter a valid email address." }; + const taken = userByEmail(e); + if (taken && taken.id !== userId) return { ok: false, error: "That email already has an account." }; + // users.email is UNIQUE: without this check a collision is a 500 inside the settings form. + if (!taken) db.prepare("UPDATE users SET email = ? WHERE id = ?").run(e, userId); + return { ok: true }; +} + export async function startSession(userId: string) { const token = randomBytes(32).toString("hex"); putSession(hashToken(token), userId, Date.now() + SESSION_DAYS * 864e5); diff --git a/lib/report-ui.ts b/lib/report-ui.ts index 75523a3..3d34c25 100644 --- a/lib/report-ui.ts +++ b/lib/report-ui.ts @@ -10,6 +10,11 @@ const muted = { color: "var(--qai-ink-muted)" }; const small = { fontSize: "var(--qai-text-xs)", color: "var(--qai-ink-muted)" }; const disp = { fontFamily: "var(--qai-font-display)" }; +// Model output is untrusted: the schema says a number is optional, models still send null, and one throw +// deletes the whole block from the report. 0 is the honest stand-in for "the model did not give a number". +const num = (v: unknown) => (typeof v === "number" && Number.isFinite(v) ? v : 0); +const conf = (v: unknown) => (typeof v === "number" ? ` ${v.toFixed(2)}` : " not reported"); + const renderers: Record any> = { Headline: ({ props }) => el("header", null, el("p", { className: "label" }, props.severity === "critical" ? "Critical" : props.severity === "warning" ? "Warning" : "Note"), @@ -50,14 +55,14 @@ const renderers: Record any> = { props.caption ? el("p", { style: small }, props.caption) : null), ScoreBar: ({ props }) => el("div", null, - el("p", { className: "label" }, props.label + (props.confidence !== undefined - ? ` · confidence ${props.confidence.toFixed(2)}` : " · no confidence reported")), - el("div", { className: "bar", style: { width: `${Math.round(props.value * 100)}%` } }), + el("p", { className: "label" }, props.label + (props.confidence === undefined + ? " · no confidence reported" : ` · confidence${conf(props.confidence)}`)), + el("div", { className: "bar", style: { width: `${Math.round(num(props.value) * 100)}%` } }), props.note ? el("p", { style: small }, props.note) : null), OptionRow: ({ props }) => el("tr", { "data-headline": props.headline || undefined }, el("td", null, String(props.rank)), el("td", null, props.title), - el("td", { className: "sql" }, props.composite.toFixed(2)), + el("td", { className: "sql" }, num(props.composite).toFixed(2)), el("td", null, props.flags || "—")), OptionTable: ({ props, renderNode }: any) => el("section", { className: "card" }, @@ -67,7 +72,7 @@ const renderers: Record any> = { VerdictBlock: ({ props }) => el("section", { className: "card", "data-verdict": "" }, el("p", { className: "label" }, "First move · " + (props.source === "jev" - ? `Jev, confidence ${props.confidence.toFixed(2)}` : "composite order — Jev did not decide")), + ? `Jev, confidence${conf(props.confidence)}` : "composite order — Jev did not decide")), p({ ...disp, fontSize: "var(--qai-text-lg)" }, props.headline), p({}, props.why)), diff --git a/scripts/e2e-debug.mjs b/scripts/e2e-debug.mjs deleted file mode 100644 index fa01dcd..0000000 --- a/scripts/e2e-debug.mjs +++ /dev/null @@ -1,24 +0,0 @@ -import { chromium } from "@playwright/test"; -const b = await chromium.launch(); -const page = await (await b.newContext()).newPage(); -page.on("console", (m) => console.log("QAI console", m.type(), m.text().slice(0, 200))); -page.on("pageerror", (e) => console.log("QAI pageerror", String(e).slice(0, 300))); -await page.goto("http://localhost:3000/signup"); -await page.fill("#email", `dbg4328+${Date.now()}@qai.test`); -await page.fill("#password", "playwright password"); -await page.getByRole("button", { name: "Create account" }).click(); -await page.waitForTimeout(3000); -console.log("QAI url after signup", page.url()); -console.log("QAI body after signup", (await page.locator("body").innerText()).replace(/\n+/g, " | ").slice(0, 260)); -console.log("QAI cookie", (await page.context().cookies()).map(c=>c.name+"="+c.value.slice(0,8)).join(",")); -await page.getByRole("link", { name: "Next" }).click(); -await page.waitForTimeout(2000); -await page.fill("#analyst_base_url", "http://192.168.88.131:8888/v1"); -await page.fill("#analyst_key", "EMPTY"); -await page.fill("#analyst_model", "qwen3.8-flash-next"); -await page.fill("#jev_key", "ts-test-key"); -await page.getByRole("button", { name: "Save" }).click(); -await page.waitForTimeout(4000); -console.log("QAI url after save", page.url()); -console.log("QAI body", (await page.locator("body").innerText()).replace(/\n+/g, " | ").slice(0, 320)); -await b.close(); diff --git a/scripts/render-check.mjs b/scripts/render-check.mjs deleted file mode 100644 index c14b059..0000000 --- a/scripts/render-check.mjs +++ /dev/null @@ -1,27 +0,0 @@ -import { chromium } from "@playwright/test"; -const ID = process.argv[2] ?? "660dd007-cc6f-4644-a57b-9c32a012e60a";; -const b = await chromium.launch(); -const ctx = await b.newContext(); -await ctx.addCookies([{ name: "qai_s", value: "tokAPI", url: "http://localhost:3117" }]); -const page = await ctx.newPage(); -const errs = []; -page.on("pageerror", (e) => errs.push(String(e.message).slice(0, 140))); -await page.goto(`http://localhost:3117/app/${ID}`, { waitUntil: "domcontentloaded" }); -await page.waitForSelector("#report", { timeout: 25000 }); -const q = async (sel) => page.locator(sel).count(); -console.log("QAI report", await q("#report"), "cards", await q("#report .card"), "sql blocks", await q("#report pre.sql"), - "copy buttons", await q("#report button"), "tables", await q("#report table"), "bars", await q("#report .bar"), - "verdict", await q("[data-verdict]"), "metrics", await q(".grid-metrics .card")); -console.log("QAI headline", (await page.locator("#report h2").first().textContent())?.slice(0, 90)); -console.log("QAI console errors", errs.length, errs.slice(0, 2)); -for (const w of [320, 375, 414, 768]) { - await page.setViewportSize({ width: w, height: 800 }); - await page.waitForTimeout(250); - const over = await page.evaluate(() => document.documentElement.scrollWidth - document.documentElement.clientWidth); - console.log(`QAI ${w}px overflow ${over}`); -} -await page.setViewportSize({ width: 1280, height: 900 }); -await page.emulateMedia({ media: "print" }); -console.log("QAI print rail hidden", await page.locator(".rail").isHidden(), "toolbar hidden", await page.locator(".toolbar").first().isHidden(), - "body bg", await page.evaluate(() => getComputedStyle(document.body).backgroundColor)); -await b.close(); diff --git a/scripts/render-debug.mjs b/scripts/render-debug.mjs deleted file mode 100644 index 8c3ae84..0000000 --- a/scripts/render-debug.mjs +++ /dev/null @@ -1,13 +0,0 @@ -import { chromium } from "@playwright/test"; -const ID = process.argv[2]; -const PORT = process.argv[3] ?? "3117"; -const b = await chromium.launch(); -const ctx = await b.newContext(); -await ctx.addCookies([{ name: "qai_s", value: "tokAPI", url: `http://localhost:${PORT}` }]); -const page = await ctx.newPage(); -page.on("console", (m) => { if (m.type() === "error") console.log("QAI console", m.text().slice(0, 400)); }); -page.on("pageerror", (e) => console.log("QAI pageerror", String(e).slice(0, 500))); -await page.goto(`http://localhost:${PORT}/app/${ID}`, { waitUntil: "load" }); -await page.waitForTimeout(6000); -console.log("QAI report count", await page.locator("#report").count()); -await b.close(); diff --git a/tests-e2e/edge.spec.ts b/tests-e2e/edge.spec.ts new file mode 100644 index 0000000..39946ea --- /dev/null +++ b/tests-e2e/edge.spec.ts @@ -0,0 +1,38 @@ +import { test, expect } from "@playwright/test"; +import { DatabaseSync } from "node:sqlite"; +import { randomUUID } from "node:crypto"; + +test.use({ storageState: ".auth/user.json" }); +const DB = ".playwright-data/qai.db"; + +// The app opens this file too (WAL), so writes here are just another connection. +function withDb(fn: (d: DatabaseSync) => T): T { + const d = new DatabaseSync(DB); + try { return fn(d); } finally { d.close(); } +} + +test("a row left running renders what exists instead of redirecting forever", async ({ page }) => { + const id = withDb((d) => { + const userId = d.prepare("SELECT id FROM users ORDER BY rowid DESC LIMIT 1").get() as { id: string }; + const aid = randomUUID(); + // Backdated so it never becomes the rail's first row: history.spec.ts depends on that row being a real run. + d.prepare("INSERT INTO analyses (id, user_id, title, xml, status, created_at, updated_at) VALUES (?,?,?,?,?,?,?)") + .run(aid, userId.id, "Orphaned run", "", "running", Date.now() - 3_600_000, Date.now() - 3_600_000); + return aid; + }); + await page.goto(`/app/${id}`); + await expect(page.getByText(/This run was interrupted before it finished/)).toBeVisible({ timeout: 15_000 }); + expect(page.url()).toContain(`/app/${id}`); // the old redirect sent this to itself + await expect(page.getByRole("button", { name: "Analyse" })).toHaveCount(0); +}); + +test("an email another account already has is refused inline", async ({ page }) => { + withDb((d) => d.prepare("INSERT INTO users (id, email, pass, created_at) VALUES (?,?,?,?)") + .run(randomUUID(), "taken@qai.test", "salt:hash", Date.now())); + await page.goto("/settings"); + await page.fill("#email", "taken@qai.test"); + await page.getByRole("button", { name: "Save" }).click(); + await expect(page.locator("form p[role=alert]")).toContainText("That email already has an account", { timeout: 15_000 }); + const still = withDb((d) => (d.prepare("SELECT email FROM users WHERE email = ?").get("taken@qai.test") as { email: string }).email); + expect(still).toBe("taken@qai.test"); +}); diff --git a/tests-e2e/responsive.spec.ts b/tests-e2e/responsive.spec.ts index c27d039..02f7733 100644 --- a/tests-e2e/responsive.spec.ts +++ b/tests-e2e/responsive.spec.ts @@ -19,7 +19,8 @@ test("every settings field has a visible label", async ({ page }) => { await page.goto("/settings"); // Scoped to the form: the rail's rename boxes are labelled for assistive tech with aria-label, not visibly. const orphans = await page.evaluate(() => [...document.querySelectorAll("form input")] - .filter((i) => i.type !== "hidden" && (!i.id || !document.querySelector(`label[for="${i.id}"]`))) - .map((i) => i.name || i.type)); + .filter((el) => { const i = el as HTMLInputElement; + return i.type !== "hidden" && (!i.id || !document.querySelector(`label[for="${i.id}"]`)); }) + .map((el) => (el as HTMLInputElement).name || (el as HTMLInputElement).type)); expect(orphans).toEqual([]); }); diff --git a/tests/auth.test.ts b/tests/auth.test.ts index df73f8d..86b605f 100644 --- a/tests/auth.test.ts +++ b/tests/auth.test.ts @@ -18,3 +18,16 @@ test("token hash is stable, 64 hex", () => { assert.equal(hashToken("abc"), hashToken("abc")); assert.match(hashToken("abc"), /^[0-9a-f]{64}$/); }); +test("changeEmail refuses an address another account already has", async () => { + const { signUpNewUser, changeEmail } = await import("../lib/auth.ts"); + const db = await import("../lib/db.ts"); + const a = signUpNewUser("first@x.test", "0123456789ab"); + const b = signUpNewUser("second@x.test", "0123456789ab"); + assert.ok(a.ok && b.ok); + assert.deepEqual(changeEmail(b.id, "first@x.test"), { ok: false, error: "That email already has an account." }); + assert.deepEqual(changeEmail(b.id, "not an email"), { ok: false, error: "Enter a valid email address." }); + assert.deepEqual(changeEmail(b.id, " FIRST@X.TEST "), { ok: false, error: "That email already has an account." }); + assert.equal(changeEmail(a.id, "first@x.test").ok, true, "own address is not a conflict"); + assert.equal(changeEmail(b.id, "fresh@x.test").ok, true); + assert.equal(db.userByEmail("fresh@x.test")!.id, b.id, "stored lower-cased"); +}); diff --git a/tests/report-ui.test.ts b/tests/report-ui.test.ts index a8c3c30..bf29a2d 100644 --- a/tests/report-ui.test.ts +++ b/tests/report-ui.test.ts @@ -13,3 +13,16 @@ test("every spec has a renderer, so the model can never emit an unrenderable blo const { library } = await import("../lib/report-ui.ts"); for (const s of specs) assert.ok(library.components[s.name], `no renderer for ${s.name}`); }); + +test("a null confidence from the model does not blank the block", async () => { + // The prompt says optional; models still send null. confidence != undefined let null through to .toFixed(). + const { library } = await import("../lib/report-ui.ts"); + const renderNode = () => null; + for (const [name, props] of [ + ["ScoreBar", { label: "fit", value: 0.5, confidence: null }], + ["VerdictBlock", { headline: "h", why: "w", source: "jev", confidence: null }], + ] as const) { + const el = library.components[name].component({ props: props as any, renderNode } as any); + assert.ok(el, `${name} returned nothing`); + } +});