diff --git a/.tech-debt/localstorage-allowlist-budget.json b/.tech-debt/localstorage-allowlist-budget.json new file mode 100644 index 000000000..76974ab71 --- /dev/null +++ b/.tech-debt/localstorage-allowlist-budget.json @@ -0,0 +1,4 @@ +{ + "production": 19, + "rationale": "Baseline 2026-05-04: 11 storage primitives + 4 cloud-sync internals + 4 module wrappers (see eslint.config.js). Burndown plan in docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md §2.2 + docs/tech-debt/frontend.md §2." +} diff --git a/docs/diagnostics/2026-05-03-web-deep-dive/00-overview.md b/docs/diagnostics/2026-05-03-web-deep-dive/00-overview.md index b2f0b0cba..096eface1 100644 --- a/docs/diagnostics/2026-05-03-web-deep-dive/00-overview.md +++ b/docs/diagnostics/2026-05-03-web-deep-dive/00-overview.md @@ -82,7 +82,7 @@ | 3 | CSP report-only on Vercel | 3 | 1 | 3.00 | [04 §6.4](./04-security-observability-testing-devx.md) — done [#1551](https://github.com/Skords-01/Sergeant/pull/1551) | | 4 | Module prefetch on hover + on-idle | 3 | 1 | 3.00 | [03 §5.2 + §10.4](./03-backend-and-performance.md) — done (idle + connection gate) | | 5 | `` wrapper | 4 | 2 | 2.00 | [01 §3.2](./01-frontend-ergonomics.md) — done (component + 10 tests) | -| 6 | `localStorage` 17 → 0 codemod | 4 | 2 | 2.00 | [02 §2.2](./02-architecture-and-state.md) | +| 6 | `localStorage` allowlist burndown CI metric | 4 | 2 | 2.00 | [02 §2.2](./02-architecture-and-state.md) — done (`pnpm lint:localstorage-allowlist`) | | 7 | Audit docs status-table + archive >6-mo | 2 | 1 | 2.00 | [04 §11](./04-security-observability-testing-devx.md) — done (Status / Implemented / Outstanding) | | 8 | Form-engine unification | 5 | 3 | 1.67 | [01 §3.1](./01-frontend-ergonomics.md) | | 9 | CloudSync split-brain integration tests | 5 | 3 | 1.67 | [02 §2.3](./02-architecture-and-state.md) | diff --git a/docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md b/docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md index 043941974..9efb894dd 100644 --- a/docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md +++ b/docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md @@ -1,6 +1,6 @@ # Web deep-dive — Architecture & state -> **Last validated:** 2026-05-03 by @Skords-01. +> **Last validated:** 2026-05-04 by @Skords-01. > **Status:** Active > **Scope:** Provider tree, routing, sync v1↔v2, `index.css`, in-process workers, React Query patterns, `localStorage` migration, CloudSync split-brain risk, `useCloudSync` shape. > **Related:** [`00-overview.md`](./00-overview.md), `docs/tech-debt/frontend.md`, `docs/audits/2026-04-28-sergeant-comprehensive-audit.md`. @@ -203,6 +203,8 @@ ShortcutRegistryProvider ## 2.2 [Bad] `localStorage` allowlist у 17 файлах +> **2026-05-04 update.** Burn-down KPI запиновано у `pnpm lint:localstorage-allowlist` (`scripts/check-localstorage-allowlist.mjs`) — лічильник production-entries проти `.tech-debt/localstorage-allowlist-budget.json`. CI падає, якщо allowlist розросся понад бюджет; зменшення → треба бампнути бюджет вниз у тому ж PR + оновити `rationale`. Baseline 19 (11 storage primitives + 4 cloud-sync internals + 4 module wrappers). + **Що бачу.** `docs/tech-debt/frontend.md:89-100` — є TODO-список з 17 файлами, які усе ще читають `localStorage` напряму через `eslint.config.js` allowlist. Допустима тимчасова фаза, але burn-down треба **запланувати**, а не «коли руки дійдуть». **Чому це дороге.** Кожен з цих файлів — потенційний краш у Safari Private Mode (де `localStorage.setItem` кидає `QuotaExceededError`) або у iOS WebKit з очищеним сховищем. diff --git a/package.json b/package.json index 699c4ef3d..168e0d9e7 100644 --- a/package.json +++ b/package.json @@ -23,13 +23,14 @@ "build:analyze": "pnpm --filter @sergeant/web build:analyze", "build:check-size": "node scripts/check-bundle-size.mjs", "preview": "pnpm --filter @sergeant/web preview", - "lint": "turbo run lint && node scripts/check-imports.mjs && node tools/tsconfig-guard/check.mjs && pnpm lint:plugins && pnpm lint:tech-debt-freshness && pnpm api:check-openapi", + "lint": "turbo run lint && node scripts/check-imports.mjs && node tools/tsconfig-guard/check.mjs && pnpm lint:plugins && pnpm lint:tech-debt-freshness && pnpm lint:localstorage-allowlist && pnpm api:check-openapi", "lint:imports": "node scripts/check-imports.mjs", "lint:migrations": "node scripts/lint-migrations.mjs", "ops:n8n:validate": "node scripts/n8n/validate-n8n-workflows.mjs", "n8n:import": "node scripts/n8n/n8n-workflows.mjs import", "n8n:export": "node scripts/n8n/n8n-workflows.mjs export", "lint:tech-debt-freshness": "node scripts/check-tech-debt-freshness.mjs", + "lint:localstorage-allowlist": "node scripts/check-localstorage-allowlist.mjs", "lint:governance-sync": "node scripts/check-governance-sync.mjs", "lint:hard-rules-registry": "node scripts/check-hard-rules-registry.mjs", "api:generate-openapi": "node scripts/api/generate-openapi.mjs", diff --git a/scripts/__tests__/check-localstorage-allowlist.test.mjs b/scripts/__tests__/check-localstorage-allowlist.test.mjs new file mode 100644 index 000000000..377af5f2c --- /dev/null +++ b/scripts/__tests__/check-localstorage-allowlist.test.mjs @@ -0,0 +1,177 @@ +// scripts/__tests__/check-localstorage-allowlist.test.mjs +// +// Unit tests for the localStorage allowlist budget guard. +// Run with: +// node --test scripts/__tests__/check-localstorage-allowlist.test.mjs +// +// We test the pure parsing helpers (no FS, no env). The CLI runner +// itself is exercised end-to-end via `pnpm lint:localstorage-allowlist` +// in CI. + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; + +import { + extractWebIgnoresBlock, + countProductionEntries, + parseBudgetFile, +} from "../check-localstorage-allowlist.mjs"; + +// ── extractWebIgnoresBlock ─────────────────────────────────────────────────── + +describe("extractWebIgnoresBlock", () => { + it("returns null when the rule wiring is absent", () => { + const source = ` + module.exports = [ + { rules: { "no-console": "warn" } }, + ]; + `; + assert.equal(extractWebIgnoresBlock(source), null); + }); + + it("locates the web app's ignores block adjacent to the rule wiring", () => { + const source = [ + "export default [", + " {", + ' files: ["apps/web/src/**/*.{js,jsx,ts,tsx}"],', + " ignores: [", + ' "apps/web/src/**/*.test.{js,jsx,ts,tsx}",', + ' "apps/web/src/shared/lib/storage/storage.ts",', + ' "apps/web/src/shared/hooks/useDarkMode.ts",', + " ],", + " rules: {", + ' "sergeant-design/no-raw-local-storage": "error",', + " },", + " },", + "];", + "", + ].join("\n"); + + const block = extractWebIgnoresBlock(source); + assert.ok(block, "block should be located"); + assert.match(block, /storage\.ts/); + assert.match(block, /useDarkMode\.ts/); + }); + + it("does NOT match the mobile rule wiring", () => { + const source = [ + "export default [", + " {", + ' files: ["apps/mobile/src/**/*.{js,jsx,ts,tsx}"],', + " ignores: [", + ' "apps/mobile/src/**/*.test.{js,jsx,ts,tsx}",', + " ],", + " rules: {", + ' "sergeant-design/no-raw-local-storage": "error",', + " },", + " },", + "];", + "", + ].join("\n"); + + assert.equal( + extractWebIgnoresBlock(source), + null, + "must not match the mobile glob", + ); + }); +}); + +// ── countProductionEntries ─────────────────────────────────────────────────── + +describe("countProductionEntries", () => { + it("returns 0 for an empty block", () => { + assert.equal(countProductionEntries("[]"), 0); + }); + + it("ignores the two test-fixture entries", () => { + const block = ` + ignores: [ + "apps/web/src/**/*.test.{js,jsx,ts,tsx}", + "apps/web/src/**/__tests__/**", + "apps/web/src/shared/lib/storage/storage.ts", + "apps/web/src/shared/hooks/useDarkMode.ts", + ], + `; + assert.equal(countProductionEntries(block), 2); + }); + + it("ignores comments so reviewer notes don't shift the count", () => { + const block = ` + ignores: [ + // Tests can use localStorage freely as fixtures. + "apps/web/src/**/*.test.{js,jsx,ts,tsx}", + // "apps/web/src/shared/hooks/oldHook.ts" — migrated PR #999 + "apps/web/src/shared/hooks/useDarkMode.ts", + ], + `; + assert.equal(countProductionEntries(block), 1); + }); + + it("counts every non-test path exactly once", () => { + const block = ` + ignores: [ + "apps/web/src/**/*.test.{js,jsx,ts,tsx}", + "apps/web/src/**/__tests__/**", + "apps/web/src/a.ts", + "apps/web/src/b.ts", + "apps/web/src/c.ts", + ], + `; + assert.equal(countProductionEntries(block), 3); + }); +}); + +// ── parseBudgetFile ────────────────────────────────────────────────────────── + +describe("parseBudgetFile", () => { + it("accepts a well-formed budget", () => { + const json = JSON.stringify({ + production: 17, + rationale: "Baseline 2026-05-04: 11 primitives + 4 cloud-sync + 2 misc.", + }); + const out = parseBudgetFile(json); + assert.equal(out.production, 17); + assert.match(out.rationale, /Baseline/); + }); + + it("floors fractional production counts", () => { + const json = JSON.stringify({ + production: 17.9, + rationale: "Reasonable rationale text long enough.", + }); + assert.equal(parseBudgetFile(json).production, 17); + }); + + it("rejects negative production counts", () => { + const json = JSON.stringify({ + production: -1, + rationale: "Reasonable rationale text long enough.", + }); + assert.throws(() => parseBudgetFile(json), /≥ 0/); + }); + + it("rejects non-numeric production counts", () => { + const json = JSON.stringify({ + production: "17", + rationale: "Reasonable rationale text long enough.", + }); + assert.throws(() => parseBudgetFile(json), /finite number/); + }); + + it("rejects a missing or too-short rationale", () => { + assert.throws( + () => parseBudgetFile(JSON.stringify({ production: 1, rationale: "" })), + /≥ 8 chars/, + ); + assert.throws( + () => + parseBudgetFile(JSON.stringify({ production: 1, rationale: "short" })), + /≥ 8 chars/, + ); + assert.throws( + () => parseBudgetFile(JSON.stringify({ production: 1 })), + /rationale/, + ); + }); +}); diff --git a/scripts/check-localstorage-allowlist.mjs b/scripts/check-localstorage-allowlist.mjs new file mode 100644 index 000000000..051764e67 --- /dev/null +++ b/scripts/check-localstorage-allowlist.mjs @@ -0,0 +1,277 @@ +#!/usr/bin/env node +// scripts/check-localstorage-allowlist.mjs +// +// CI metric for the `sergeant-design/no-raw-local-storage` allowlist +// in `eslint.config.js`. Closes diagnostic +// `docs/diagnostics/2026-05-03-web-deep-dive/02-architecture-and-state.md` §2.2 +// (and Hard Rule #20 / `docs/tech-debt/frontend.md` §2). +// +// Why a separate script? +// The ESLint rule itself only fires on **non-allowlisted** files. +// New `localStorage` users keep going into the allowlist instead of +// migrating to `safeReadLS`/`safeWriteLS`/`createModuleStorage`. The +// doc says "17 files" but nothing actually pins that number — the +// list silently grows on each PR. This script: +// +// 1. Counts the production entries (excludes test globs). +// 2. Compares against a checked-in budget in `.tech-debt/localstorage-allowlist-budget.json`. +// 3. Fails CI if the count exceeds the budget — so adding a new +// site requires either migrating an existing one OR a +// deliberate budget bump documented in the same PR. +// 4. Prints the current count + delta from budget for dashboards. +// +// Usage: +// pnpm lint:localstorage-allowlist +// LS_ALLOWLIST_BUDGET=20 node scripts/check-localstorage-allowlist.mjs +// +// Exits 1 on budget overrun or a parse failure, 0 otherwise. + +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, resolve } from "node:path"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = resolve(__dirname, ".."); + +const ESLINT_CONFIG_PATH = resolve(REPO_ROOT, "eslint.config.js"); +const BUDGET_PATH = resolve( + REPO_ROOT, + ".tech-debt/localstorage-allowlist-budget.json", +); + +// ── Pure helpers (exported for tests) ──────────────────────────────────────── + +/** + * Slice the `eslint.config.js` source into the rule block for + * `sergeant-design/no-raw-local-storage` scoped to `apps/web/src`. + * + * The block is recognised by: + * 1. A `rules:` line containing `"sergeant-design/no-raw-local-storage"`. + * 2. Walking BACK to the nearest `files: ["apps/web/src/**...` line + * (the rule is also applied to `apps/mobile`; we only count the + * web app since that's where the burn-down lives). + * 3. Walking BACK from there to the nearest `ignores: [` line and + * forward to the matching `]`. + * + * Returns the raw `ignores` array body as a string, or `null` if the + * block can't be located. + */ +export function extractWebIgnoresBlock(source) { + const lines = source.split("\n"); + + let ruleLine = -1; + for (let i = 0; i < lines.length; i++) { + if ( + lines[i].includes('"sergeant-design/no-raw-local-storage"') && + // Skip the plugin-author `rules:` map (an object literal) — we + // want the call-site that wires the rule into the web glob. + lines[i].includes('"error"') + ) { + // Look back for the matching `files: ["apps/web/src/...`. + for (let j = i; j >= Math.max(0, i - 80); j--) { + if ( + lines[j].includes("files:") && + lines[j].includes('"apps/web/src/') + ) { + ruleLine = j; + break; + } + } + if (ruleLine !== -1) break; + } + } + + if (ruleLine === -1) return null; + + // Walk forward to the `ignores:` line. + let ignoresStart = -1; + for (let i = ruleLine; i < Math.min(lines.length, ruleLine + 60); i++) { + if (lines[i].includes("ignores:")) { + ignoresStart = i; + break; + } + } + if (ignoresStart === -1) return null; + + // Walk forward to the matching `]`. The block is a flat string array + // — no nested brackets — so a simple counter suffices. + let depth = 0; + let blockEnd = -1; + for (let i = ignoresStart; i < lines.length; i++) { + for (const ch of lines[i]) { + if (ch === "[") depth++; + else if (ch === "]") { + depth--; + if (depth === 0) { + blockEnd = i; + break; + } + } + } + if (blockEnd !== -1) break; + } + if (blockEnd === -1) return null; + + return lines.slice(ignoresStart, blockEnd + 1).join("\n"); +} + +/** + * Count production allowlist entries inside the `ignores: [...]` block. + * + * Test-fixture entries (`**\/*.test.{js,jsx,ts,tsx}`, `**\/__tests__/**`) + * are intentionally excluded — they aren't burn-down items. + * + * Comments (`//`) are stripped so reviewer notes can't shift the count. + */ +export function countProductionEntries(blockText) { + const stringPaths = blockText + // Drop line comments so trailing `// note` doesn't hide a path. + .replace(/\/\/[^\n]*/g, "") + .match(/"[^"]+"/g); + if (!stringPaths) return 0; + let count = 0; + for (const raw of stringPaths) { + const path = raw.slice(1, -1); + // Skip the unconditional test-file ignores. They are NOT burn-down. + if ( + path === "apps/web/src/**/*.test.{js,jsx,ts,tsx}" || + path === "apps/web/src/**/__tests__/**" + ) { + continue; + } + count++; + } + return count; +} + +/** + * Parse the JSON budget file. + * + * Shape: `{ "production": , "rationale": "" }`. + * + * `rationale` is mandatory — bumping the budget without a one-line + * justification is the exact bad-faith review path we want to gate. + */ +export function parseBudgetFile(text) { + const json = JSON.parse(text); + if (typeof json.production !== "number" || !Number.isFinite(json.production)) { + throw new Error("budget.production must be a finite number"); + } + if (json.production < 0) { + throw new Error("budget.production must be ≥ 0"); + } + if (typeof json.rationale !== "string" || json.rationale.trim().length < 8) { + throw new Error( + "budget.rationale must be a non-empty string ≥ 8 chars (cite the diagnostic / tech-debt section)", + ); + } + return { production: Math.floor(json.production), rationale: json.rationale }; +} + +// ── CLI runner ─────────────────────────────────────────────────────────────── + +function loadConfig() { + return readFileSync(ESLINT_CONFIG_PATH, "utf8"); +} + +function loadBudget() { + try { + return readFileSync(BUDGET_PATH, "utf8"); + } catch (e) { + if (e?.code === "ENOENT") return null; + throw e; + } +} + +export function run({ envBudget } = {}) { + const source = loadConfig(); + const block = extractWebIgnoresBlock(source); + if (!block) { + return { + ok: false, + count: null, + budget: null, + reason: + "Could not locate the `sergeant-design/no-raw-local-storage` ignores block " + + "for `apps/web/src` in eslint.config.js. Did the rule wiring change?", + }; + } + + const count = countProductionEntries(block); + + // Resolve budget: env var wins (for one-off CI dry runs); else file. + let budget = null; + let rationale = null; + if (envBudget !== undefined && envBudget !== null && envBudget !== "") { + const n = Number(envBudget); + if (Number.isFinite(n) && n >= 0) budget = Math.floor(n); + } + if (budget === null) { + const raw = loadBudget(); + if (raw === null) { + return { + ok: false, + count, + budget: null, + reason: + `Budget file missing: ${BUDGET_PATH}. ` + + "Create it with `{ \"production\": , \"rationale\": \"...\" }`.", + }; + } + try { + const parsed = parseBudgetFile(raw); + budget = parsed.production; + rationale = parsed.rationale; + } catch (e) { + return { + ok: false, + count, + budget: null, + reason: `Budget file is malformed: ${e.message}`, + }; + } + } + + if (count > budget) { + return { + ok: false, + count, + budget, + rationale, + reason: + `localStorage allowlist grew to ${count} production entries ` + + `(budget: ${budget}). Either migrate an existing site to ` + + `safeReadLS/safeWriteLS/createModuleStorage and drop it from ` + + `eslint.config.js, OR bump the budget in ` + + `.tech-debt/localstorage-allowlist-budget.json with a clear rationale.`, + }; + } + + return { ok: true, count, budget, rationale }; +} + +const isCli = + import.meta.url === `file://${process.argv[1]}` || + import.meta.url.endsWith(process.argv[1] || ""); + +if (isCli) { + const result = run({ envBudget: process.env.LS_ALLOWLIST_BUDGET }); + if (!result.ok) { + console.error(`✗ ${result.reason}`); + if (result.count !== null && result.budget !== null) { + console.error( + ` current=${result.count} budget=${result.budget} delta=+${ + result.count - result.budget + }`, + ); + } + process.exit(1); + } + console.log( + `✓ localStorage allowlist: ${result.count}/${result.budget} ` + + `(headroom ${result.budget - result.count})`, + ); + if (result.rationale) { + console.log(` rationale: ${result.rationale}`); + } +}