diff --git a/.github/workflows/react-doctor.yml b/.github/workflows/react-doctor.yml index f81b046e05..b9076bcb8a 100644 --- a/.github/workflows/react-doctor.yml +++ b/.github/workflows/react-doctor.yml @@ -1,13 +1,13 @@ # React Doctor — finds security, performance, correctness, accessibility, # bundle-size, and architecture issues in React codebases. # -# Advisory-only and least-privilege: findings appear in the step log and the -# Actions run summary. All write-scoped outputs (sticky PR comments, inline -# review comments, commit statuses) are explicitly disabled so the workflow -# needs no write permissions. Do not re-add write scopes without revisiting -# tests/ci-workflows.test.ts, which pins this contract. +# Gating and least-privilege: findings fail the job (`blocking: warning`). +# Write-scoped outputs (sticky PR comments, inline review comments, commit +# statuses) stay disabled so the workflow needs no write permissions. Do not +# re-add write scopes without revisiting tests/ci-workflows.test.ts, which +# pins this contract. # -# Docs: https://www.react.doctor/ci +# Docs: https://www.react.doctor/docs/ci-and-prs/github-actions-setup # Source: https://github.com/millionco/react-doctor name: React Doctor @@ -24,7 +24,7 @@ permissions: contents: read # Needed so the action can list PR files for --changed-files-from. # Without this, listFiles fails, the changed-files file is never written, - # and the CLI exits 1 on ENOENT even with blocking: none (fork PRs). + # and the CLI exits 1 on ENOENT even for fork PRs. pull-requests: read # Cancels any in-flight scan for the same PR (or branch, on push) the moment a @@ -50,9 +50,9 @@ jobs: directory: gui # Pin the npm engine — the action wrapper would otherwise fetch # react-doctor@latest, silently skewing CI from the local pinned runs. - version: "0.9.1" - # Advisory contract: report to the step log only; never gate, never write. - blocking: none + version: "0.9.2" + # Fail the job on any finding (errors or warnings). + blocking: warning comment: false review-comments: false commit-status: false diff --git a/gui/README.md b/gui/README.md index fdbd4f2be0..8981908b95 100644 --- a/gui/README.md +++ b/gui/README.md @@ -34,8 +34,8 @@ the package layout used by `ocx gui`. ```bash cd gui bun run lint # ESLint — hard local/CI gate (`GUI lint` in CI) -bun run doctor # React Doctor vs origin/main (changed-scope, advisory) -bun run doctor:full # Full-project React Doctor scan +bun run doctor # React Doctor vs origin/main (changed-scope, gates on findings) +bun run doctor:full # Full-tree React Doctor (gates on findings) ``` From the repo root: @@ -49,6 +49,6 @@ bun run setup:hooks # pre-push runs doctor when gui/ changed | Tool | Role | |------|------| | **ESLint** (`bun run lint`) | Hard gate in CI and expected before merge | -| **React Doctor** (`bun run doctor`) | Advisory React health check pinned to react-doctor 0.9.1. Pre-push runs it only if `gui/` changed and never blocks the push. The CI workflow reports to the step log only | +| **React Doctor** (`bun run doctor`) | Gating React health check pinned to react-doctor 0.9.2 (`blocking: warning`). Pre-push runs it only if `gui/` changed and fails the push on findings. The CI workflow fails the job on any finding | Fix ESLint errors first. Use `doctor` / `doctor:full` for deeper React triage. diff --git a/gui/doctor.config.json b/gui/doctor.config.json index f9bd272c20..46169c5bf0 100644 --- a/gui/doctor.config.json +++ b/gui/doctor.config.json @@ -1,7 +1,7 @@ { "$schema": "https://react.doctor/schema/config.json", "scope": "full", - "blocking": "none", + "blocking": "warning", "ignore": { "files": ["dist/**", "node_modules/**"] }, diff --git a/gui/package.json b/gui/package.json index b8e26c169d..ce7f602b0b 100644 --- a/gui/package.json +++ b/gui/package.json @@ -9,8 +9,8 @@ "lint": "eslint .", "test": "bun test tests", "lint:i18n": "eslint src/pages src/components src/App.tsx src/ui.tsx", - "doctor": "npx --yes react-doctor@0.9.1 --verbose --scope changed --base origin/main --no-telemetry", - "doctor:full": "npx --yes react-doctor@0.9.1 --verbose --scope full --no-telemetry", + "doctor": "npx --yes react-doctor@0.9.2 --verbose --scope changed --base origin/main --no-telemetry", + "doctor:full": "npx --yes react-doctor@0.9.2 --verbose --scope full --no-telemetry", "preview": "vite preview" }, "dependencies": { diff --git a/gui/src/bounded-fetch.ts b/gui/src/bounded-fetch.ts new file mode 100644 index 0000000000..bec2d83edf --- /dev/null +++ b/gui/src/bounded-fetch.ts @@ -0,0 +1,31 @@ +/** + * Bound a fetch with AbortSignal.timeout when available; otherwise a manual + * timer that must be cleared after settlement / unmount. + */ + +export type BoundedFetch = { + controller: AbortController; + signal: AbortSignal; + clear: () => void; +}; + +export function createBoundedFetch(ms: number): BoundedFetch { + const controller = new AbortController(); + if ( + typeof AbortSignal !== "undefined" + && typeof AbortSignal.any === "function" + && typeof AbortSignal.timeout === "function" + ) { + return { + controller, + signal: AbortSignal.any([controller.signal, AbortSignal.timeout(ms)]), + clear: () => undefined, + }; + } + const timeoutId = setTimeout(() => controller.abort(), ms); + return { + controller, + signal: controller.signal, + clear: () => clearTimeout(timeoutId), + }; +} diff --git a/gui/src/combo-workspace-data.ts b/gui/src/combo-workspace-data.ts index 9634c885c6..bc29bff703 100644 --- a/gui/src/combo-workspace-data.ts +++ b/gui/src/combo-workspace-data.ts @@ -15,6 +15,7 @@ export function intersectComboEfforts( ): ComboEffort[] { const complete = targets.filter((t) => t.provider.trim() && t.model.trim()); if (complete.length === 0) return [...COMBO_EFFORTS]; + const effortSet = new Set(COMBO_EFFORTS); let common: string[] | null = null; for (const target of complete) { const key = `${target.provider.trim()}/${target.model.trim()}`; @@ -23,12 +24,16 @@ export function intersectComboEfforts( // supportedLadderFor is undefined (#488 / Codex review). const member: string[] = listed === undefined ? [] - : listed.filter((effort) => (COMBO_EFFORTS as readonly string[]).includes(effort)); - common = common === null - ? member - : common.filter((effort) => member.includes(effort)); + : listed.filter((effort) => effortSet.has(effort)); + if (common === null) { + common = member; + } else { + const memberSet = new Set(member); + common = common.filter((effort) => memberSet.has(effort)); + } } - return COMBO_EFFORTS.filter((effort) => common?.includes(effort) === true); + const commonSet = new Set(common ?? []); + return COMBO_EFFORTS.filter((effort) => commonSet.has(effort)); } export interface ComboTarget { diff --git a/gui/src/components/MemoryObservabilityCard.tsx b/gui/src/components/MemoryObservabilityCard.tsx index fe968e7533..d703f5262c 100644 --- a/gui/src/components/MemoryObservabilityCard.tsx +++ b/gui/src/components/MemoryObservabilityCard.tsx @@ -2,6 +2,7 @@ import { useEffect, useState } from "react"; import { formatUptime } from "../formatUptime"; import { IconActivity } from "../icons"; import { useI18n, type Locale } from "../i18n/shared"; +import { createBoundedFetch, type BoundedFetch } from "../bounded-fetch"; /** * Memory observability card. Polls GET /api/system/memory (#314 WP3) every 5s @@ -163,59 +164,48 @@ export default function MemoryObservabilityCard({ apiBase }: { apiBase: string } useEffect(() => { let cancelled = false; let inFlight = false; - let activeController: AbortController | null = null; + let active: BoundedFetch | null = null; const fetchMemory = async () => { // Serialize polls: a stalled request must not stack up or let an older // payload land after a newer one. if (inFlight) return; inFlight = true; - // Bound each poll so a hung request cannot pin inFlight forever and - // starve the unavailable fallback. Prefer AbortSignal.timeout; fall back - // to a manual timer when the browser lacks AbortSignal.any/timeout. - const controller = new AbortController(); - activeController = controller; - let timeoutId: ReturnType | undefined; - const signal = typeof AbortSignal !== "undefined" && "any" in AbortSignal && "timeout" in AbortSignal - ? AbortSignal.any([controller.signal, AbortSignal.timeout(10_000)]) - : (() => { - timeoutId = setTimeout(() => controller.abort(), 10_000); - return controller.signal; - })(); + // Bound each poll so a hung request cannot pin inFlight forever. + const bounded = createBoundedFetch(10_000); + active = bounded; try { - const res = await fetch(`${apiBase}/api/system/memory`, { signal }); + const res = await fetch(`${apiBase}/api/system/memory`, { signal: bounded.signal }); if (!res.ok) throw new Error("memory unavailable"); const json = await res.json() as SystemMemory; - if (!cancelled) { - setData(json); - setUnavailable(false); - setSupportsRestart(typeof json.activeTurnCount === "number"); - if (json.isDraining && restartPhase === "idle") setRestartPhase("draining"); - // Fast recycle can finish between polls with no observed outage — detect pid change. - if ( - (restartPhase === "draining" || restartPhase === "reconnecting") - && restartFromPid != null - && typeof json.pid === "number" - && json.pid !== restartFromPid - && !json.isDraining - ) { - setRestartPhase("idle"); - setRestartFromPid(null); - setRestartError(null); - } + if (cancelled) return; + setData(json); + setUnavailable(false); + setSupportsRestart(typeof json.activeTurnCount === "number"); + if (json.isDraining && restartPhase === "idle") setRestartPhase("draining"); + // Fast recycle can finish between polls with no observed outage — detect pid change. + if ( + (restartPhase === "draining" || restartPhase === "reconnecting") + && restartFromPid != null + && typeof json.pid === "number" + && json.pid !== restartFromPid + && !json.isDraining + ) { + setRestartPhase("idle"); + setRestartFromPid(null); + setRestartError(null); } } catch { // Old servers (pre-#314) 404 this route; degrade to a quiet unavailable note. // During drain/restart the proxy goes away — switch to reconnect polling. - if (!cancelled) { - if (restartPhase === "draining" || restartPhase === "reconnecting") { - setRestartPhase("reconnecting"); - } else { - setUnavailable(true); - } + if (cancelled) return; + if (restartPhase === "draining" || restartPhase === "reconnecting") { + setRestartPhase("reconnecting"); + } else { + setUnavailable(true); } } finally { - if (timeoutId !== undefined) clearTimeout(timeoutId); - if (activeController === controller) activeController = null; + bounded.clear(); + if (active === bounded) active = null; inFlight = false; } }; @@ -223,7 +213,8 @@ export default function MemoryObservabilityCard({ apiBase }: { apiBase: string } const interval = setInterval(() => void fetchMemory(), 5000); return () => { cancelled = true; - activeController?.abort(); + active?.controller.abort(); + active?.clear(); clearInterval(interval); }; }, [apiBase, restartPhase, restartFromPid]); @@ -232,15 +223,23 @@ export default function MemoryObservabilityCard({ apiBase }: { apiBase: string } if (restartPhase !== "reconnecting") return; let cancelled = false; let inFlight = false; + let active: BoundedFetch | null = null; const started = Date.now(); - const tick = async () => { - if (inFlight) return; + const tick = () => { + if (inFlight || cancelled) return; inFlight = true; - const controller = new AbortController(); - const timeoutId = setTimeout(() => controller.abort(), 5_000); - try { - const res = await fetch(`${apiBase}/healthz`, { cache: "no-store", signal: controller.signal }); - if (res.ok && !cancelled) { + const bounded = createBoundedFetch(5_000); + active = bounded; + void fetch(`${apiBase}/healthz`, { cache: "no-store", signal: bounded.signal }) + .then(async (res) => { + if (cancelled) return; + if (!res.ok) { + if (Date.now() - started >= RECONNECT_GIVE_UP_MS) { + setRestartPhase("error"); + setRestartError(t("dash.mem.restartFailed")); + } + return; + } let replaced = restartFromPid == null; if (restartFromPid != null) { try { @@ -250,28 +249,37 @@ export default function MemoryObservabilityCard({ apiBase }: { apiBase: string } replaced = true; } } + if (cancelled) return; if (replaced) { setRestartPhase("idle"); setRestartFromPid(null); setRestartError(null); return; } - } - } catch { - /* still down / aborted */ - } finally { - clearTimeout(timeoutId); - inFlight = false; - } - if (!cancelled && Date.now() - started >= RECONNECT_GIVE_UP_MS) { - setRestartPhase("error"); - setRestartError(t("dash.mem.restartFailed")); - } + if (Date.now() - started >= RECONNECT_GIVE_UP_MS) { + setRestartPhase("error"); + setRestartError(t("dash.mem.restartFailed")); + } + }) + .catch(() => { + if (cancelled) return; + if (Date.now() - started >= RECONNECT_GIVE_UP_MS) { + setRestartPhase("error"); + setRestartError(t("dash.mem.restartFailed")); + } + }) + .finally(() => { + bounded.clear(); + if (active === bounded) active = null; + inFlight = false; + }); }; - void tick(); - const interval = setInterval(() => void tick(), RECONNECT_POLL_MS); + tick(); + const interval = setInterval(tick, RECONNECT_POLL_MS); return () => { cancelled = true; + active?.controller.abort(); + active?.clear(); clearInterval(interval); }; }, [apiBase, restartPhase, restartFromPid, t]); diff --git a/gui/src/components/provider-workspace/AnthropicAccountPoolSettings.tsx b/gui/src/components/provider-workspace/AnthropicAccountPoolSettings.tsx index 257e566f8d..2bd21d454d 100644 --- a/gui/src/components/provider-workspace/AnthropicAccountPoolSettings.tsx +++ b/gui/src/components/provider-workspace/AnthropicAccountPoolSettings.tsx @@ -27,7 +27,7 @@ export default function AnthropicAccountPoolSettings({ useEffect(() => { let cancelled = false; const ac = new AbortController(); - void (async () => { + const load = async () => { try { const res = await fetch(`${apiBase}/api/oauth/accounts/pool?provider=anthropic`, { signal: ac.signal, @@ -44,10 +44,12 @@ export default function AnthropicAccountPoolSettings({ if (cancelled || ac.signal.aborted) return; setLoadError(true); } - })(); + }; + const timer = window.setTimeout(() => { void load(); }, 0); return () => { cancelled = true; ac.abort(); + window.clearTimeout(timer); }; }, [apiBase]); diff --git a/gui/src/fetch-json.ts b/gui/src/fetch-json.ts index e788ba5fc7..7250292409 100644 --- a/gui/src/fetch-json.ts +++ b/gui/src/fetch-json.ts @@ -7,9 +7,22 @@ /** Parse an OK response body; 204 / empty bodies yield `undefined`. */ async function readJsonBody(res: Response): Promise { if (res.status === 204) return undefined; - const text = await res.text(); - if (!text.trim()) return undefined; - return JSON.parse(text) as T; + // Prefer text() so empty OK bodies are detectable. Fall back to json() for + // Response-like test doubles that only implement json(). + if (typeof res.text === "function") { + const text = await res.text(); + if (!text.trim()) return undefined; + return JSON.parse(text) as T; + } + return await res.json() as T; +} + +function errorMessageFromBody(errBody: { error?: unknown; message?: unknown }, fallback: string): string { + if (typeof errBody.error === "string" && errBody.error) return errBody.error; + // Some management routes (e.g. Grok apply orphan repair) put the actionable + // copy in `message` rather than `error`. + if (typeof errBody.message === "string" && errBody.message) return errBody.message; + return fallback; } export async function readJsonOrThrow( @@ -19,8 +32,8 @@ export async function readJsonOrThrow( if (!res.ok) { let message = fallbackMessage; try { - const errBody = await res.json() as { error?: unknown }; - if (typeof errBody?.error === "string" && errBody.error) message = errBody.error; + const errBody = await res.json() as { error?: unknown; message?: unknown }; + message = errorMessageFromBody(errBody, fallbackMessage); } catch { // non-JSON error bodies keep the fallback message } diff --git a/gui/src/pages/Claude.tsx b/gui/src/pages/Claude.tsx index f8b611b5b1..9dfd048682 100644 --- a/gui/src/pages/Claude.tsx +++ b/gui/src/pages/Claude.tsx @@ -1,7 +1,7 @@ import { useRef, useState, type KeyboardEvent } from "react"; import ClaudeCode from "./ClaudeCode"; import ClaudeDesktop from "./ClaudeDesktop"; -import { useT } from "../i18n"; +import { useT } from "../i18n/shared"; type ClaudeTab = "code" | "desktop"; diff --git a/gui/src/pages/ClaudeDesktop.tsx b/gui/src/pages/ClaudeDesktop.tsx index 069fdf0c81..2eaae112c6 100644 --- a/gui/src/pages/ClaudeDesktop.tsx +++ b/gui/src/pages/ClaudeDesktop.tsx @@ -3,7 +3,9 @@ import { LANE_PAGE, defaultCollapsedFamilies, laneView, rowStartsOpen } from "./ import { makeCollapseStore, toggleInSet } from "./collapse-store"; import { IconChevron } from "../icons"; import { EmptyState, Notice } from "../ui"; -import { useT, type TFn, type TKey } from "../i18n"; +import { useT, type TFn, type TKey } from "../i18n/shared"; +import { readJsonIfOk, readJsonOrThrow } from "../fetch-json"; +import { createBoundedFetch } from "../bounded-fetch"; const FAMILIES = ["opus", "fable", "sonnet", "haiku"] as const; type Family = typeof FAMILIES[number]; @@ -150,8 +152,11 @@ export default function ClaudeDesktop({ apiBase }: { apiBase: string }) { setLoadError(""); try { const response = await fetch(`${apiBase}/api/claude-desktop`); - const payload = await response.json() as DesktopResponse | { error?: string }; - if (!response.ok || !("profile" in payload) || !("models" in payload)) { + const payload = await readJsonOrThrow( + response, + t("claudeDesktop.loadFail"), + ); + if (!payload || !("profile" in payload) || !("models" in payload)) { throw new Error(errorMessage(payload, t("claudeDesktop.loadFail"))); } const normalized = normalizeProfile(payload); @@ -204,10 +209,34 @@ export default function ClaudeDesktop({ apiBase }: { apiBase: string }) { // Poll Desktop status every 5s for applied-state + health. useEffect(() => { let cancelled = false; - const poll = () => fetch(`${apiBase}/api/claude-desktop/status`).then(r => r.json()).then(d => { if (!cancelled) setStatus(d as DesktopStatus); }).catch(() => {}); + let inFlight = false; + let active: ReturnType | null = null; + const poll = () => { + if (inFlight) return; + inFlight = true; + const bounded = createBoundedFetch(10_000); + active = bounded; + void fetch(`${apiBase}/api/claude-desktop/status`, { signal: bounded.signal }) + .then((response) => readJsonIfOk(response)) + .then((data) => { + if (cancelled) return; + if (data) setStatus(data); + }) + .catch(() => { /* offline / older proxy / aborted */ }) + .finally(() => { + bounded.clear(); + if (active === bounded) active = null; + inFlight = false; + }); + }; poll(); const timer = setInterval(poll, 5000); - return () => { cancelled = true; clearInterval(timer); }; + return () => { + cancelled = true; + clearInterval(timer); + active?.controller.abort(); + active?.clear(); + }; }, [apiBase]); const moveModel = (route: string, family: Family) => { @@ -248,15 +277,13 @@ export default function ClaudeDesktop({ apiBase }: { apiBase: string }) { headers: { "Content-Type": "application/json" }, body: JSON.stringify({ profile }), }); - const payload = await response.json().catch(() => ({})) as { error?: string }; - if (!response.ok) throw new Error(errorMessage(payload, t("claudeDesktop.saveFailed"))); + await readJsonOrThrow<{ error?: string }>(response, t("claudeDesktop.saveFailed")); setSavedProfile(cloneProfile(profile)); if (applyAfter) { setPending("apply"); const applyResponse = await fetch(`${apiBase}/api/claude-desktop/apply`, { method: "POST" }); - const applyPayload = await applyResponse.json().catch(() => ({})) as { error?: string }; - if (!applyResponse.ok) throw new Error(errorMessage(applyPayload, t("claudeDesktop.applyFailed"))); + await readJsonOrThrow<{ error?: string }>(applyResponse, t("claudeDesktop.applyFailed")); setMessage({ tone: "ok", text: t("claudeDesktop.savedApplied") }); setAnnouncement(t("claudeDesktop.savedAppliedAnnounce")); } else { diff --git a/gui/src/pages/Grok.tsx b/gui/src/pages/Grok.tsx index 7dc5b5f2a8..e30a38ba16 100644 --- a/gui/src/pages/Grok.tsx +++ b/gui/src/pages/Grok.tsx @@ -1,7 +1,8 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import { EmptyState, Notice, Switch } from "../ui"; import { IconChevron } from "../icons"; -import { useT, type TKey } from "../i18n"; +import { useT, type TKey } from "../i18n/shared"; +import { readJsonOrThrow } from "../fetch-json"; import { makeCollapseStore, toggleInSet } from "./collapse-store"; import { grokGroupView, type GrokCandidate } from "./grok-groups"; @@ -67,8 +68,8 @@ export default function Grok({ apiBase }: { apiBase: string }) { setError(""); try { const response = await fetch(`${apiBase}/api/grok`); - const payload = await response.json() as GrokStatus & { error?: string }; - if (!response.ok) throw new Error(payload.error || t("grok.loadFail")); + const payload = await readJsonOrThrow(response, t("grok.loadFail")); + if (!payload) throw new Error(t("grok.loadFail")); // Tolerate an older proxy that predates the selection routes: the page degrades // to the read-only fence view instead of crashing on a missing field. setStatus({ ...payload, candidates: payload.candidates ?? [], excluded: payload.excluded ?? [] }); @@ -124,15 +125,22 @@ export default function Grok({ apiBase }: { apiBase: string }) { headers: { "Content-Type": "application/json" }, body: JSON.stringify({ excluded: [...excluded] }), }); - const savePayload = await response.json().catch(() => ({})) as { error?: string }; - if (!response.ok) throw new Error(savePayload.error ?? t("grok.saveFailed")); + await readJsonOrThrow<{ error?: string }>(response, t("grok.saveFailed")); setSavedExcluded(new Set(excluded)); if (applyAfter) { setPending("apply"); const applied = await fetch(`${apiBase}/api/grok/apply`, { method: "POST" }); - const payload = await applied.json().catch(() => ({})) as { message?: string; skippedReason?: string }; - if (!applied.ok) throw new Error(payload.message ?? t("grok.applyFailed")); + // Apply errors use `{ message, skippedReason }` (not always `error`); preserve that + // actionable copy for orphan-marker repair and policy skips. + if (!applied.ok) { + const failed = await applied.json().catch(() => ({})) as { message?: string; error?: string }; + throw new Error(failed.message ?? failed.error ?? t("grok.applyFailed")); + } + const payload = await applied.json().catch(() => ({})) as { + message?: string; + skippedReason?: string; + }; // A policy skip is not success theatre: the Grok config did NOT change // (non-loopback bind, or no ~/.grok), so say that instead of "applied". if (payload.skippedReason) { diff --git a/gui/src/pages/Logs.tsx b/gui/src/pages/Logs.tsx index 92bfdb66d4..92464397df 100644 --- a/gui/src/pages/Logs.tsx +++ b/gui/src/pages/Logs.tsx @@ -295,8 +295,9 @@ function summarizeFilteredLogs(entries: LogEntry[]): { continue; } const cost = entry.displayMetrics?.cost; - if (cost?.kind === "value" && Number.isFinite(cost.estimate.cost.total) && cost.estimate.cost.total >= 0) { - estimatedCostUsd += cost.estimate.cost.total; + const total = cost?.kind === "value" ? cost.estimate.cost.total : undefined; + if (total !== undefined && Number.isFinite(total) && total >= 0) { + estimatedCostUsd += total; continue; } unpricedRequests += 1; diff --git a/gui/src/pages/Storage.tsx b/gui/src/pages/Storage.tsx index 171172666a..0fc3e9e9be 100644 --- a/gui/src/pages/Storage.tsx +++ b/gui/src/pages/Storage.tsx @@ -274,8 +274,11 @@ function ArchivedCleanupPanel({ headers: { "content-type": "application/json" }, body: JSON.stringify({ percent }), }); - const json = await res.json() as CleanupPreview & { error?: string }; - if (!res.ok) throw new Error(mapCleanupError(json.error, t("storage.cleanup.previewFailed"))); + if (!res.ok) { + const json = await res.json().catch(() => ({})) as { error?: string }; + throw new Error(mapCleanupError(json.error, t("storage.cleanup.previewFailed"))); + } + const json = await res.json() as CleanupPreview; setPreview(json); setConfirmOpen(true); } catch (e) { @@ -299,14 +302,21 @@ function ArchivedCleanupPanel({ digest: preview.digest, }), }); - const json = await res.json() as CleanupResult; - if (!res.ok || !json.ok) { + if (!res.ok) { + const json = await res.json().catch(() => ({})) as CleanupResult; if (json.error === "stale_preview") { // Digest can never succeed again — send the user back to Preview. closeConfirm(true); } throw new Error(mapCleanupError(json.error, json.message, json.trashDir)); } + const json = await res.json() as CleanupResult; + if (!json.ok) { + if (json.error === "stale_preview") { + closeConfirm(true); + } + throw new Error(mapCleanupError(json.error, json.message, json.trashDir)); + } closeConfirm(true); setStatus( permanent @@ -479,9 +489,12 @@ function QuarantineTrashPanel({ setLoading(true); try { const res = await fetch(`${apiBase}/api/storage/trash`, { signal }); - const json = await res.json() as TrashList & { error?: string }; + if (!res.ok) { + if (signal?.aborted || generation !== loadGenerationRef.current) return; + throw new Error(t("storage.trash.listFailed")); + } + const json = await res.json() as TrashList; if (signal?.aborted || generation !== loadGenerationRef.current) return; - if (!res.ok) throw new Error(json.error ?? "list_failed"); const next = Array.isArray(json.entries) ? json.entries : []; setEntries(next); onEntriesChange?.(next); @@ -537,8 +550,12 @@ function QuarantineTrashPanel({ headers: { "content-type": "application/json" }, body: JSON.stringify({ id: confirmEntry.id }), }); + if (!res.ok) { + const json = await res.json().catch(() => ({})) as RestoreResult; + throw new Error(mapRestoreError(json.error, json.message)); + } const json = await res.json() as RestoreResult; - if (!res.ok || !json.ok) { + if (!json.ok) { throw new Error(mapRestoreError(json.error, json.message)); } closeConfirm(); @@ -691,7 +708,8 @@ function AutoCleanupPolicyPanel({ const [status, setStatus] = useState(null); const [error, setError] = useState(null); const [targetMode, setTargetMode] = useState<"percent" | "reduce">("percent"); - const [percent, setPercent] = useState(25); + /** Draft string so blank/invalid percent targets are rejected instead of coerced. */ + const [percent, setPercent] = useState("25"); /** Draft string so blank/invalid reduce targets are rejected instead of coerced to 0. */ const [reduceGb, setReduceGb] = useState("4"); /** Draft string so a cleared threshold is rejected instead of coerced to 0. */ @@ -714,7 +732,7 @@ function AutoCleanupPolicyPanel({ setReduceGb(String(Math.max(0, Math.round((json.target.reduceToBytes / GB) * 100) / 100))); } else { setTargetMode("percent"); - setPercent(Math.min(100, Math.max(1, Math.floor(json.target.removeOldestPercent ?? 25)))); + setPercent(String(Math.min(100, Math.max(1, Math.floor(json.target.removeOldestPercent ?? 25))))); } } catch { if (signal?.aborted) return; @@ -788,8 +806,12 @@ function AutoCleanupPolicyPanel({ headers: { "content-type": "application/json" }, body: JSON.stringify(body), }); + if (!res.ok) { + setError(t("storage.policy.saveFailed")); + return; + } const json = await res.json() as { ok?: boolean; policy?: CleanupPolicy; error?: string }; - if (!res.ok || !json.policy) { + if (!json.policy) { setError(t("storage.policy.saveFailed")); return; } @@ -823,9 +845,14 @@ function AutoCleanupPolicyPanel({ body: JSON.stringify(base), signal, }); + if (signal.aborted) return; + if (!saveRes.ok) { + setError(t("storage.policy.saveFailed")); + return; + } const saved = await saveRes.json() as { policy?: CleanupPolicy; error?: string }; if (signal.aborted) return; - if (!saveRes.ok || !saved.policy) { + if (!saved.policy) { setError(t("storage.policy.saveFailed")); return; } @@ -835,6 +862,31 @@ function AutoCleanupPolicyPanel({ method: "POST", signal, }); + if (signal.aborted) return; + if (res.status === 409) { + const conflict = await res.json().catch(() => ({})) as { + error?: string; + policy?: CleanupPolicy; + }; + if (signal.aborted) return; + if (conflict.policy) setPolicy(policyFieldsFromResponse(conflict.policy)); + setError(t("storage.policy.alreadyRunning")); + return; + } + if (!res.ok) { + const failed = await res.json().catch(() => ({})) as { + error?: string; + policy?: CleanupPolicy; + }; + if (signal.aborted) return; + if (failed.policy) setPolicy(policyFieldsFromResponse(failed.policy)); + if (failed.error === "already_running") { + setError(t("storage.policy.alreadyRunning")); + return; + } + setError(t("storage.policy.runFailed")); + return; + } const startJson = await res.json() as { ok?: boolean; started?: boolean; @@ -844,14 +896,10 @@ function AutoCleanupPolicyPanel({ }; if (signal.aborted) return; if (startJson.policy) setPolicy(policyFieldsFromResponse(startJson.policy)); - if (startJson.error === "already_running" || res.status === 409) { + if (startJson.error === "already_running") { setError(t("storage.policy.alreadyRunning")); return; } - if (!res.ok) { - setError(t("storage.policy.runFailed")); - return; - } if (!startJson.started || !startJson.job?.startedAt) { setError(t("storage.policy.runFailed")); return; @@ -1000,7 +1048,8 @@ function AutoCleanupPolicyPanel({ max={100} value={percent} disabled={saving || running} - onChange={e => setPercent(Number(e.target.value))} + aria-label={t("storage.policy.targetPercent")} + onChange={e => setPercent(e.target.value)} onBlur={() => void savePolicy()} style={{ display: "block", marginTop: 4, width: "100%" }} /> @@ -1022,6 +1071,7 @@ function AutoCleanupPolicyPanel({ step={0.1} value={reduceGb} disabled={saving || running} + aria-label={t("storage.policy.targetReduce")} onChange={e => setReduceGb(e.target.value)} onBlur={() => void savePolicy()} style={{ display: "block", marginTop: 4, width: "100%" }} diff --git a/gui/tests/bounded-fetch.test.ts b/gui/tests/bounded-fetch.test.ts new file mode 100644 index 0000000000..0685897b28 --- /dev/null +++ b/gui/tests/bounded-fetch.test.ts @@ -0,0 +1,46 @@ +import { expect, test } from "bun:test"; +import { createBoundedFetch } from "../src/bounded-fetch"; + +test("createBoundedFetch aborts when the controller is aborted (native path)", () => { + const bounded = createBoundedFetch(60_000); + expect(bounded.signal.aborted).toBe(false); + bounded.controller.abort(); + expect(bounded.signal.aborted).toBe(true); + bounded.clear(); +}); + +test("createBoundedFetch aborts after the timeout on the compatibility path", async () => { + const originalAny = Object.getOwnPropertyDescriptor(AbortSignal, "any"); + const originalTimeout = Object.getOwnPropertyDescriptor(AbortSignal, "timeout"); + // Force the manual setTimeout fallback so we exercise the timer path. + Object.defineProperty(AbortSignal, "any", { value: undefined, configurable: true }); + Object.defineProperty(AbortSignal, "timeout", { value: undefined, configurable: true }); + + try { + const bounded = createBoundedFetch(20); + expect(bounded.signal.aborted).toBe(false); + await new Promise(resolve => setTimeout(resolve, 50)); + expect(bounded.signal.aborted).toBe(true); + bounded.clear(); + } finally { + if (originalAny) Object.defineProperty(AbortSignal, "any", originalAny); + if (originalTimeout) Object.defineProperty(AbortSignal, "timeout", originalTimeout); + } +}); + +test("createBoundedFetch clear cancels a pending manual timeout", async () => { + const originalAny = Object.getOwnPropertyDescriptor(AbortSignal, "any"); + const originalTimeout = Object.getOwnPropertyDescriptor(AbortSignal, "timeout"); + Object.defineProperty(AbortSignal, "any", { value: undefined, configurable: true }); + Object.defineProperty(AbortSignal, "timeout", { value: undefined, configurable: true }); + + try { + const bounded = createBoundedFetch(100); + bounded.clear(); + await new Promise(resolve => setTimeout(resolve, 150)); + expect(bounded.signal.aborted).toBe(false); + } finally { + if (originalAny) Object.defineProperty(AbortSignal, "any", originalAny); + if (originalTimeout) Object.defineProperty(AbortSignal, "timeout", originalTimeout); + } +}); diff --git a/gui/tests/fetch-json.test.ts b/gui/tests/fetch-json.test.ts index cf93d3f864..9489fb76c9 100644 --- a/gui/tests/fetch-json.test.ts +++ b/gui/tests/fetch-json.test.ts @@ -23,3 +23,24 @@ test("readJsonOrThrow surfaces server error messages", async () => { const res = Response.json({ error: "locked" }, { status: 503 }); await expect(readJsonOrThrow(res, "fallback")).rejects.toThrow("locked"); }); + +test("readJsonOrThrow prefers error, then message, then fallback", async () => { + await expect( + readJsonOrThrow(Response.json({ message: "repair marker" }, { status: 500 }), "fallback"), + ).rejects.toThrow("repair marker"); + await expect( + readJsonOrThrow(Response.json({ error: "err", message: "msg" }, { status: 500 }), "fallback"), + ).rejects.toThrow("err"); + await expect( + readJsonOrThrow(Response.json({ detail: "other" }, { status: 500 }), "fallback"), + ).rejects.toThrow("fallback"); +}); + +test("readJsonOrThrow accepts Response-like mocks that only implement json()", async () => { + const mock = { + ok: true, + status: 200, + json: async () => ({ hello: "world" }), + } as unknown as Response; + expect(await readJsonOrThrow<{ hello: string }>(mock)).toEqual({ hello: "world" }); +}); diff --git a/scripts/doctor-gui-if-changed.ts b/scripts/doctor-gui-if-changed.ts index 9990913056..b341e0eab2 100644 --- a/scripts/doctor-gui-if-changed.ts +++ b/scripts/doctor-gui-if-changed.ts @@ -2,15 +2,16 @@ * Run React Doctor in gui/ when this push includes gui/ changes. * Used by `bun run prepush`. Skip with: git push --no-verify * - * Advisory by contract (doctor.config.json blocking: "none"): findings never - * gate the push, so an unavailable engine (offline npx fetch, registry outage) - * degrades to a warning instead of blocking. + * Gating by contract (doctor.config.json blocking: "warning"): findings fail + * the push. An unavailable engine (offline npx fetch, missing binary) still + * degrades to a warning so infrastructure outages do not brick pushes. * * Test hooks: DOCTOR_DRY_RUN=1 prints the run/skip decision without spawning; * DOCTOR_FILES (newline-separated) overrides git-derived changed files; - * DOCTOR_CMD overrides the spawned command (offline-degradation testing). + * DOCTOR_CMD overrides the spawned command (offline-degradation testing); + * OCX_DOCTOR_MAX_BUFFER overrides spawnSync maxBuffer (overflow hard-fail testing). */ -import { execFileSync } from "node:child_process"; +import { spawnSync } from "node:child_process"; import { join, resolve } from "node:path"; /** True when any changed path is the gui directory or inside it (slash-guarded). */ @@ -18,14 +19,40 @@ export function guiPathsChanged(files: string[]): boolean { return files.some(f => f === "gui" || f.startsWith("gui/")); } +/** True when stderr/stdout looks like a registry/network failure rather than findings. */ +export function looksLikeDoctorInfraFailure(output: string): boolean { + // Keep this narrow: bare words like "network"/"offline" also appear in findings text. + // `npm ERR!` is matched on its own — `!` is non-word, so a shared trailing `\b` never fires after it. + return /npm ERR!|(?:^|\b)(?:ENOTFOUND|ECONNRESET|ECONNREFUSED|ETIMEDOUT|ENETUNREACH|EAI_AGAIN|getaddrinfo ENOTFOUND|fetch failed|ERR_INVALID_URL|UNABLE_TO_GET_ISSUER_CERT|certificate has expired|unable to resolve|could not resolve host|socket hang up|connect ECONNREFUSED|connect ENOTFOUND|connect ECONNRESET)(?:\b|$)/im + .test(output); +} + +/** Large enough for verbose doctor scans; overflows must not soft-skip the gate. */ +export const DOCTOR_MAX_BUFFER = 20 * 1024 * 1024; + +/** True when spawnSync failed because child stdout/stderr exceeded maxBuffer. */ +export function isDoctorBufferOverflow(code: string | undefined): boolean { + return code === "ENOBUFS" || code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"; +} + +function resolveMaxBuffer(): number { + const raw = process.env.OCX_DOCTOR_MAX_BUFFER; + if (raw === undefined || raw === "") return DOCTOR_MAX_BUFFER; + const n = Number(raw); + return Number.isFinite(n) && n > 0 ? Math.floor(n) : DOCTOR_MAX_BUFFER; +} + if (import.meta.main) { const repoRoot = resolve(import.meta.dirname, ".."); const guiDir = join(repoRoot, "gui"); const hasRef = (ref: string): boolean => { try { - execFileSync("git", ["rev-parse", "--verify", ref], { cwd: repoRoot, stdio: "ignore" }); - return true; + const probe = spawnSync("git", ["rev-parse", "--verify", ref], { + cwd: repoRoot, + stdio: "ignore", + }); + return probe.status === 0; } catch { return false; } @@ -33,7 +60,12 @@ if (import.meta.main) { const diffNames = (range: string): string[] => { try { - return execFileSync("git", ["diff", "--name-only", range], { cwd: repoRoot, encoding: "utf8" }) + const diff = spawnSync("git", ["diff", "--name-only", range], { + cwd: repoRoot, + encoding: "utf8", + }); + if (diff.status !== 0) return []; + return (diff.stdout ?? "") .split(/\r?\n/) .map(line => line.trim()) .filter(Boolean); @@ -72,16 +104,44 @@ if (import.meta.main) { const [cmd, ...args] = process.env.DOCTOR_CMD ? process.env.DOCTOR_CMD.split(" ") : ["bun", "run", "doctor"]; - try { - execFileSync(cmd!, args, { - cwd: guiDir, - stdio: "inherit", - env: { ...process.env, npm_config_yes: "true" }, - }); - } catch { - // Findings are non-gating (blocking: none), so any failure here is - // infrastructure noise, not a content signal. Never block the push on it. - console.warn("doctor:gui: react-doctor unavailable (offline?) — skipping advisory scan"); + + // spawnSync (not execFileSync): explicit maxBuffer + status/error channels so + // oversized doctor output cannot be mistaken for an offline soft-skip. + const result = spawnSync(cmd!, args, { + cwd: guiDir, + encoding: "utf8", + maxBuffer: resolveMaxBuffer(), + env: { ...process.env, npm_config_yes: "true" }, + }); + + const stdout = result.stdout ?? ""; + const stderr = result.stderr ?? ""; + if (stdout) process.stdout.write(stdout); + if (stderr) process.stderr.write(stderr); + + const overflowCode = result.error && "code" in result.error + ? String((result.error as NodeJS.ErrnoException).code ?? "") + : ""; + if (isDoctorBufferOverflow(overflowCode)) { + console.error("doctor:gui: react-doctor output exceeded buffer — failing push"); + process.exit(1); + } + + // Spawn failed (missing binary) — do not brick the push on infra. + if (result.error && typeof result.status !== "number") { + console.warn("doctor:gui: react-doctor unavailable (offline?) — skipping scan"); + process.exit(0); + } + + if (result.status === 0) process.exit(0); + + const combined = `${stdout}\n${stderr}`; + // Doctor started but npx/registry failed — same soft-skip contract. + if (looksLikeDoctorInfraFailure(combined)) { + console.warn("doctor:gui: react-doctor unavailable (offline?) — skipping scan"); process.exit(0); } + + // Real findings (or other doctor/engine errors) gate the push. + process.exit(result.status === null || result.status === 0 ? 1 : result.status); } diff --git a/scripts/fixtures/doctor-findings-exit.ts b/scripts/fixtures/doctor-findings-exit.ts new file mode 100644 index 0000000000..c0af45362e --- /dev/null +++ b/scripts/fixtures/doctor-findings-exit.ts @@ -0,0 +1,2 @@ +process.stdout.write("All 2 issues\n"); +process.exit(1); diff --git a/scripts/fixtures/doctor-huge-output.ts b/scripts/fixtures/doctor-huge-output.ts new file mode 100644 index 0000000000..6ab4c3cea8 --- /dev/null +++ b/scripts/fixtures/doctor-huge-output.ts @@ -0,0 +1,3 @@ +// Emit more than a tiny OCX_DOCTOR_MAX_BUFFER so the prepush wrapper hard-fails. +process.stdout.write("x".repeat(64 * 1024)); +process.exit(0); diff --git a/scripts/fixtures/doctor-offline-exit.ts b/scripts/fixtures/doctor-offline-exit.ts new file mode 100644 index 0000000000..a072d75763 --- /dev/null +++ b/scripts/fixtures/doctor-offline-exit.ts @@ -0,0 +1,2 @@ +process.stderr.write("npm ERR! network getaddrinfo ENOTFOUND registry.npmjs.org\n"); +process.exit(1); diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index d61363ebcc..ec95bc200e 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1912,41 +1912,45 @@ describe("GitHub Actions hardening", () => { expect(helperSrc).not.toContain(".ocx-translation-state"); }); - test("React Doctor workflow is SHA-pinned, engine-pinned, advisory, and read-only", async () => { + test("React Doctor workflow is SHA-pinned, engine-pinned, gating, and read-only", async () => { const workflow = await readText(".github/workflows/react-doctor.yml"); expect(workflow).toContain("actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8"); expect(workflow).toContain("millionco/react-doctor@938008119a288f2fb47c66a69cd9279a21f31784"); - expect(workflow).not.toMatch(/uses:\s+\S+@(?:v\d+|main|master)\b/); + expect(workflow).not.toMatch( + /^\s*-\s+uses:\s+\S+@(?![0-9a-f]{40}(?=[ \t]*(?:#.*)?$))\S+/m, + ); // Engine pin: the action wrapper would fetch react-doctor@latest without it. - expect(workflow).toContain('version: "0.9.1"'); + expect(workflow).toContain('version: "0.9.2"'); - // Action pin must accept CLI JSON schemaVersion 3 (baseline reports from 0.9.1). + // Action pin must accept CLI JSON schemaVersion 3 (baseline reports from 0.9.x). // v2.1.0's ensure-json-report only knew schemas 1–2 and failed every PR scan. - // Advisory + least privilege: read-only token, all write-scoped outputs off. + // Gating + least privilege: read-only token, all write-scoped outputs off. // pull-requests: read is required so the action can list PR files for // --changed-files-from; without it, fork PRs fail with ENOENT on that file. expect(workflow).toContain("contents: read"); expect(workflow).toContain("pull-requests: read"); expect(workflow).not.toContain(": write"); - expect(workflow).toContain("blocking: none"); - expect(workflow).toContain("comment: false"); - expect(workflow).toContain("review-comments: false"); - expect(workflow).toContain("commit-status: false"); + expect(workflow).toMatch(/^\s+blocking:\s+warning\s*$/m); + expect(workflow).toMatch(/^\s+comment:\s+false\s*$/m); + expect(workflow).toMatch(/^\s+review-comments:\s+false\s*$/m); + expect(workflow).toMatch(/^\s+commit-status:\s+false\s*$/m); expect(workflow).toContain("timeout-minutes: 10"); }); test("React Doctor package scripts pin the exact engine version with no @latest anywhere", async () => { const guiPkg = await readText("gui/package.json"); const rootPkg = await readText("package.json"); + const doctorConfig = await readText("gui/doctor.config.json"); - expect(guiPkg).toContain("react-doctor@0.9.1"); + expect(guiPkg).toContain("react-doctor@0.9.2"); expect(guiPkg).not.toContain("react-doctor@latest"); expect(rootPkg).not.toContain("react-doctor@latest"); + expect(doctorConfig).toContain('"blocking": "warning"'); expect(rootPkg).toContain('"doctor:gui:if-changed": "bun scripts/doctor-gui-if-changed.ts"'); expect(rootPkg).toContain('"lint:gui": "cd gui && bun run lint"'); - // Gating steps (typecheck, eslint, tests, privacy) run before advisory React Doctor. + // Gating steps include React Doctor after privacy scan on gui/ pushes. expect(rootPkg).toContain("bun run typecheck && bun run lint:gui && bun run test"); expect(rootPkg).toContain("bun run privacy:scan && bun run doctor:gui:if-changed"); }); @@ -1964,6 +1968,16 @@ describe("doctor-gui-if-changed", () => { expect(guiPathsChanged([])).toBe(false); }); + test("looksLikeDoctorInfraFailure detects registry/network outages", async () => { + const { looksLikeDoctorInfraFailure } = await import("../scripts/doctor-gui-if-changed"); + expect(looksLikeDoctorInfraFailure("npm ERR! network getaddrinfo ENOTFOUND registry.npmjs.org")).toBe(true); + expect(looksLikeDoctorInfraFailure("npm ERR! code ECONNRESET")).toBe(true); + expect(looksLikeDoctorInfraFailure("npm ERR! network timeout")).toBe(true); + expect(looksLikeDoctorInfraFailure("All 2 issues\nBugs > 1 errors")).toBe(false); + // Findings copy can mention "network" without being an infra outage. + expect(looksLikeDoctorInfraFailure("Network requests > 1 errors")).toBe(false); + }); + test("DRY_RUN prints the run/skip decision without spawning the doctor", () => { const run = Bun.spawnSync(["bun", doctorGuiIfChangedScript], { env: { ...process.env, DOCTOR_DRY_RUN: "1", DOCTOR_FILES: "gui/src/App.tsx\nscripts/x.ts" }, @@ -1987,6 +2001,54 @@ describe("doctor-gui-if-changed", () => { }, }); expect(run.exitCode).toBe(0); - expect(run.stderr.toString()).toContain("skipping advisory scan"); + expect(run.stderr.toString()).toContain("skipping scan"); + }); + + test("soft-skips when doctor exits nonzero due to a registry/network failure", () => { + // Simulate `bun run doctor` starting, then npx failing offline: numeric status + // plus registry noise in stderr — must not gate the push. + // cwd for DOCTOR_CMD is gui/, so reach fixtures via ../scripts/... + const run = Bun.spawnSync(["bun", doctorGuiIfChangedScript], { + env: { + ...process.env, + DOCTOR_FILES: "gui/src/App.tsx", + DOCTOR_CMD: "bun ../scripts/fixtures/doctor-offline-exit.ts", + }, + }); + expect(run.exitCode).toBe(0); + expect(run.stderr.toString()).toContain("skipping scan"); + }); + + test("propagates a non-zero doctor exit so findings gate the push", () => { + const run = Bun.spawnSync(["bun", doctorGuiIfChangedScript], { + env: { + ...process.env, + DOCTOR_FILES: "gui/src/App.tsx", + DOCTOR_CMD: "bun ../scripts/fixtures/doctor-findings-exit.ts", + }, + }); + expect(run.exitCode).not.toBe(0); + }); + + test("isDoctorBufferOverflow recognizes ENOBUFS / maxBuffer errors", async () => { + const { isDoctorBufferOverflow } = await import("../scripts/doctor-gui-if-changed"); + expect(isDoctorBufferOverflow("ENOBUFS")).toBe(true); + expect(isDoctorBufferOverflow("ERR_CHILD_PROCESS_STDIO_MAXBUFFER")).toBe(true); + expect(isDoctorBufferOverflow("ENOENT")).toBe(false); + expect(isDoctorBufferOverflow(undefined)).toBe(false); + }); + + test("hard-fails when doctor output exceeds maxBuffer (does not soft-skip)", () => { + const run = Bun.spawnSync(["bun", doctorGuiIfChangedScript], { + env: { + ...process.env, + DOCTOR_FILES: "gui/src/App.tsx", + DOCTOR_CMD: "bun ../scripts/fixtures/doctor-huge-output.ts", + // Tiny buffer so the fixture's stdout trips the overflow branch. + OCX_DOCTOR_MAX_BUFFER: "256", + }, + }); + expect(run.exitCode).not.toBe(0); + expect(run.stderr.toString()).toContain("exceeded buffer"); }); });